Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1495830
| From | Mylene Josserand <mylene.josserand@free-electrons.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec |
| Date | 2016-10-05 14:00 +0200 |
| Message-ID | <soT1D-tV-3@gated-at.bofh.it> (permalink) |
| References | <souwh-SP-1@gated-at.bofh.it> <souFY-W7-21@gated-at.bofh.it> <soxkt-2M1-9@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
Hello,
On 04/10/2016 14:40, Thomas Petazzoni wrote:
> Hello,
>
> On Tue, 4 Oct 2016 11:46:19 +0200, Mylène Josserand wrote:
>> Add the digital sun8i audio codec which handles the base register
>> (without DAI).
>
> I'm not sure what you mean by "which handles the base register".
I wanted to explain that it is registers for audio codec and not PRCM
ones. This is, maybe, unclear (and useless ?).
>
>> diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
>> index 7aee95a..9e287b0 100644
>> --- a/sound/soc/sunxi/Kconfig
>> +++ b/sound/soc/sunxi/Kconfig
>> @@ -27,6 +27,15 @@ config SND_SUN4I_SPDIF
>> Say Y or M to add support for the S/PDIF audio block in the Allwinner
>> A10 and affiliated SoCs.
>>
>> +config SND_SUN8I_CODEC
>> + tristate "Allwinner SUN8I audio codec"
>> + select REGMAP_MMIO
>> + help
>
> Indentation issue here, it should be intended with one tab, not spaces.
>
> You probably also want a "depends on OF" here.
Yes, thanks !
>
>> +/* CODEC_OFFSET represents the offset of the codec registers
>> + * and not all the DAI registers
>> + */
>
> This is not the proper comment style I believe for audio code, it
> should be:
>
> /*
> * ...
> */
>
>> +#define CODEC_OFFSET 0x200
>
> Do you really need this CODEC_OFFSET macro? Why not simply use directly
> the right offsets? I.e instead of:
>
> #define SUN8I_SYSCLK_CTL (0x20c - CODEC_OFFSET)
>
> use:
>
> #define SUN8I_SYSCLK_CTL 0xc
I thought it could be easier to find registers using offset but I guess
that register's names are enough.
>
>> +#define CODEC_BASSADDRESS 0x01c22c00
>
> This define is not used anywhere.
Yes, sorry, I forgot to remove it.
>
>> +#define SUN8I_SYSCLK_CTL (0x20c - CODEC_OFFSET)
>> +#define SUN8I_SYSCLK_CTL_AIF1CLK_ENA (11)
>> +#define SUN8I_SYSCLK_CTL_SYSCLK_ENA (3)
>> +#define SUN8I_SYSCLK_CTL_SYSCLK_SRC (0)
>
> Parenthesis around single values are not really useful.
>
>> +#define SUN8I_MOD_CLK_ENA (0x210 - CODEC_OFFSET)
>> +#define SUN8I_MOD_CLK_ENA_AIF1 (15)
>> +#define SUN8I_MOD_CLK_ENA_DAC (2)
>> +#define SUN8I_MOD_RST_CTL (0x214 - CODEC_OFFSET)
>> +#define SUN8I_MOD_RST_CTL_AIF1 (15)
>> +#define SUN8I_MOD_RST_CTL_DAC (2)
>> +#define SUN8I_SYS_SR_CTRL (0x218 - CODEC_OFFSET)
>> +#define SUN8I_SYS_SR_CTRL_AIF1_FS (12)
>> +#define SUN8I_SYS_SR_CTRL_AIF2_FS (8)
>> +#define SUN8I_AIF1CLK_CTRL (0x240 - CODEC_OFFSET)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_MSTR_MOD (15)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_INV (14)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_INV (13)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV (9)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV (6)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ (4)
>> +#define SUN8I_AIF1CLK_CTRL_AIF1_DATA_FMT (2)
>> +#define SUN8I_AIF1_DACDAT_CTRL (0x248 - CODEC_OFFSET)
>> +#define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0L_ENA (15)
>> +#define SUN8I_AIF1_DACDAT_CTRL_AIF1_DA0R_ENA (14)
>> +#define SUN8I_DAC_DIG_CTRL (0x320 - CODEC_OFFSET)
>> +#define SUN8I_DAC_DIG_CTRL_ENDA (15)
>> +#define SUN8I_DAC_MXR_SRC (0x330 - CODEC_OFFSET)
>> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA0L (15)
>> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF1DA1L (14)
>> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_AIF2DACL (13)
>> +#define SUN8I_DAC_MXR_SRC_DACL_MXR_SRC_ADCL (12)
>> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA0R (11)
>> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF1DA1R (10)
>> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_AIF2DACR (9)
>> +#define SUN8I_DAC_MXR_SRC_DACR_MXR_SRC_ADCR (8)
>
> Indentation of the value is not very clean for those last defines.
>
>> +static int sun8i_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>> +{
>> + struct sun8i_codec *scodec = snd_soc_codec_get_drvdata(dai->codec);
>> + unsigned long value;
>
> I'm not sure "unsigned long" is a very good choice here, it's going to
> be a 64 bits integer on 64 bits platform. I'd suggest to use "u32",
> which also seems to be what's used in _set_fmt() function of the
> sun4i-i2s.c driver.
Agreed, thanks !
>
>
>> +static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params,
>> + struct snd_soc_dai *dai)
>> +{
>> + int rs_value = 0;
>
> Two spaces before the = sign, not needed. Is the initialization to 0
> really needed? Also, this should be a u32.
ditto
>
>> + regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
>> + 0x3 << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ,
>
> Maybe a #define value to replace the hardcoded 0x3 ?
>
>> + rs_value << SUN8I_AIF1CLK_CTRL_AIF1_WORD_SIZ);
>> +
>> + /* calculate bclk_lrck_div Ratio */
>> + bclk_lrck_div = sample_resolution * 2;
>> + switch (bclk_lrck_div) {
>> + case 16:
>> + bclk_lrck_div = 0;
>> + break;
>> + case 32:
>> + bclk_lrck_div = 1;
>> + break;
>> + case 64:
>> + bclk_lrck_div = 2;
>> + break;
>> + case 128:
>> + bclk_lrck_div = 3;
>> + break;
>> + case 256:
>> + bclk_lrck_div = 4;
>> + break;
>
> This could quite easily be replaced by a formula, if you don't care
> about error checking:
>
> bclk_lrck_div = log2(bclk_lrck_div) - 4;
>
> Of course, if you care about error checking, this switch is nicer.
>
>> + default:
>
> So there's no error checking if the value is not supported?
You are right. I guess it should return -EINVAL.
[snip]
>
>
>> +static struct snd_soc_dai_driver sun8i_codec_dai = {
>> + .name = "sun8i",
>> + /* playback capabilities */
>> + .playback = {
>> + .stream_name = "Playback",
>> + .channels_min = 1,
>> + .channels_max = 2,
>> + .rates = SNDRV_PCM_RATE_8000_192000 |
>> + SNDRV_PCM_RATE_KNOT,
>> + .formats = SNDRV_PCM_FMTBIT_S8 |
>> + SNDRV_PCM_FMTBIT_S16_LE |
>> + SNDRV_PCM_FMTBIT_S18_3LE |
>> + SNDRV_PCM_FMTBIT_S20_3LE |
>> + SNDRV_PCM_FMTBIT_S24_LE |
>> + SNDRV_PCM_FMTBIT_S32_LE,
>> + },
>> + /* pcm operations */
>> + .ops = &sun8i_codec_dai_ops,
>> +};
>> +EXPORT_SYMBOL(sun8i_codec_dai);
>
> This EXPORT_SYMBOL looks wrong. First because it doesn't seem to be
> used outside of this module. And second because using EXPORT_SYMBOL on
> a function defined as static doesn't make much sense, as the "static"
> qualifier limits the visibility of the symbol to the current
> compilation unit.
>
Yes, sorry, I missed it from the clean-up of the original driver.
[snip]
>> +static int sun8i_codec_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res_base;
>> + struct sun8i_codec *scodec;
>> + void __iomem *base;
>> +
>> + scodec = devm_kzalloc(&pdev->dev, sizeof(*scodec), GFP_KERNEL);
>> + if (!scodec)
>> + return -ENOMEM;
>> +
>> + scodec->dev = &pdev->dev;
>> +
>> + /* Get the clocks from the DT */
>> + scodec->clk_module = devm_clk_get(&pdev->dev, "codec");
>> + if (IS_ERR(scodec->clk_module)) {
>> + dev_err(&pdev->dev, "Failed to get the module clock\n");
>> + return PTR_ERR(scodec->clk_module);
>> + }
>> + if (clk_prepare_enable(scodec->clk_module))
>> + pr_err("err:open failed;\n");
>
> Grr, pr_err, not good. Plus you want to return with an error from the
> probe() function.
Oh, sorry for that ugly use :(
>
>> +
>> + scodec->clk_apb = devm_clk_get(&pdev->dev, "apb");
>> + if (IS_ERR(scodec->clk_apb)) {
>> + dev_err(&pdev->dev, "Failed to get the apb clock\n");
>> + return PTR_ERR(scodec->clk_apb);
>> + }
>> + if (clk_prepare_enable(scodec->clk_apb))
>> + pr_err("err:open failed;\n");
>
> Ditto. + unprepare/disable the previous clock.
[snip]
ack, thank you for the review!
--
Mylène Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH 00/14] ASoc: sunxi: Add Allwinner A33 codec driver Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 11:50 +0200
[PATCH 08/14] dt-bindings: sound: Add sun8i analog codec documentation Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 11:50 +0200
Re: [PATCH 08/14] dt-bindings: sound: Add sun8i analog codec documentation Mark Brown <broonie@kernel.org> - 2016-10-04 12:40 +0200
Re: [PATCH 08/14] dt-bindings: sound: Add sun8i analog codec documentation Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-10-04 18:30 +0200
Re: [PATCH 08/14] dt-bindings: sound: Add sun8i analog codec documentation Chen-Yu Tsai <wens@csie.org> - 2016-10-05 05:10 +0200
Re: [PATCH 08/14] dt-bindings: sound: Add sun8i analog codec documentation Mylene Josserand <mylene.josserand@free-electrons.com> - 2016-10-05 14:10 +0200
[PATCH 11/14] ARM: dts: sun8i: Add analog codec on prcm node Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 11:50 +0200
[PATCH 01/14] dma: sun6i-dma: Add burst case of 4 Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 11:50 +0200
Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4 Jean-Francois Moine <moinejf@free.fr> - 2016-10-04 12:50 +0200
Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4 Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-10-04 14:20 +0200
Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4 Jean-Francois Moine <moinejf@free.fr> - 2016-10-04 15:50 +0200
Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4 Vinod Koul <vinod.koul@intel.com> - 2016-10-04 17:40 +0200
Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4 Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-10-04 19:00 +0200
[PATCH 13/14] ARM: dts: sun8i: parrot: Enable audio nodes Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 11:50 +0200
[PATCH 10/14] dt-bindings: sound: Add sun8i audio card documentation Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 11:50 +0200
Re: [PATCH 10/14] dt-bindings: sound: Add sun8i audio card documentation Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-10-04 18:40 +0200
[PATCH 12/14] ARM: dts: sun8i: Add audio codec, dai and card for A33 Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 11:50 +0200
[PATCH 03/14] ASoC: sun4i-i2s: Add apb reset Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 11:50 +0200
Re: [PATCH 03/14] ASoC: sun4i-i2s: Add apb reset Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-10-04 14:20 +0200
Re: [PATCH 03/14] ASoC: sun4i-i2s: Add apb reset Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-10-04 17:50 +0200
Re: [PATCH 03/14] ASoC: sun4i-i2s: Add apb reset Mylene Josserand <mylene.josserand@free-electrons.com> - 2016-10-05 11:50 +0200
Re: [PATCH 03/14] ASoC: sun4i-i2s: Add apb reset Code Kipper <codekipper@gmail.com> - 2016-10-04 14:30 +0200
[PATCH 02/14] clk: ccu-sun8i-a33: Add CLK_SET_RATE_PARENT to ac-dig Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 12:00 +0200
Re: [PATCH 02/14] clk: ccu-sun8i-a33: Add CLK_SET_RATE_PARENT to ac-dig Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-10-04 14:20 +0200
Re: [PATCH 02/14] clk: ccu-sun8i-a33: Add CLK_SET_RATE_PARENT to ac-dig Mylene Josserand <mylene.josserand@free-electrons.com> - 2016-10-05 11:40 +0200
[PATCH 05/14] mfd: sun6i-prcm: Add sun8i analog codec as subnode Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 12:00 +0200
Re: [PATCH 05/14] mfd: sun6i-prcm: Add sun8i analog codec as subnode Jean-Francois Moine <moinejf@free.fr> - 2016-10-04 13:00 +0200
[PATCH 04/14] ASoC: Add sun8i analog codec driver Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 12:00 +0200
Re: [PATCH 04/14] ASoC: Add sun8i analog codec driver Code Kipper <codekipper@gmail.com> - 2016-10-04 12:30 +0200
Re: [PATCH 04/14] ASoC: Add sun8i analog codec driver Chen-Yu Tsai <wens@csie.org> - 2016-10-04 13:00 +0200
[PATCH 07/14] ASoC: Add sun8i audio card Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 12:00 +0200
Re: [PATCH 07/14] ASoC: Add sun8i audio card Code Kipper <codekipper@gmail.com> - 2016-10-04 12:20 +0200
Re: [PATCH 07/14] ASoC: Add sun8i audio card Chen-Yu Tsai <wens@csie.org> - 2016-10-04 13:10 +0200
Re: [PATCH 07/14] ASoC: Add sun8i audio card Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-10-04 14:30 +0200
Re: [PATCH 07/14] ASoC: Add sun8i audio card Code Kipper <codekipper@gmail.com> - 2016-10-05 08:10 +0200
Re: [PATCH 07/14] ASoC: Add sun8i audio card Jean-Francois Moine <moinejf@free.fr> - 2016-10-05 12:10 +0200
[PATCH 06/14] ASoC: Add sun8i digital audio codec Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 12:00 +0200
Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-10-04 14:50 +0200
Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec Mark Brown <broonie@kernel.org> - 2016-10-04 15:10 +0200
Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-10-04 15:20 +0200
Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-10-04 18:20 +0200
Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec Mylene Josserand <mylene.josserand@free-electrons.com> - 2016-10-05 14:00 +0200
Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-10-04 18:20 +0200
Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-10-06 20:30 +0200
[PATCH 09/14] dt-bindings: sound: Add sun8i codec documentation Mylène Josserand <mylene.josserand@free-electrons.com> - 2016-10-04 12:00 +0200
Re: [PATCH 09/14] dt-bindings: sound: Add sun8i codec documentation Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-10-04 18:30 +0200
csiph-web