Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1495322 > unrolled thread

[PATCH 00/14] ASoc: sunxi: Add Allwinner A33 codec driver

Started byMylène Josserand <mylene.josserand@free-electrons.com>
First post2016-10-04 11:50 +0200
Last post2016-10-04 18:30 +0200
Articles 6 on this page of 46 — 10 participants

Back to article view | Back to linux.kernel

Contents

  [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

Page 3 of 3 — ← Prev page 1 2 [3]

#1495501 — Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2016-10-04 18:20 +0200
SubjectRe: [PATCH 06/14] ASoC: Add sun8i digital audio codec
Message-ID<soABH-57P-27@gated-at.bofh.it>
In reply to#1495411

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Tue, Oct 04, 2016 at 02:40:08PM +0200, Thomas Petazzoni wrote:
> > +	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.

Ideally, that would be even not be part of the runtime_pm
hooks. Ideally, that would be great if that driver supports it.

We'll have to go through all the drivers to support it, that would be
one less to do (and ASoC makes it very easy, you can have a look at
the sun4i-i2s driver).

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1495830 — Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec

FromMylene Josserand <mylene.josserand@free-electrons.com>
Date2016-10-05 14:00 +0200
SubjectRe: [PATCH 06/14] ASoC: Add sun8i digital audio codec
Message-ID<soT1D-tV-3@gated-at.bofh.it>
In reply to#1495411
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

[toc] | [prev] | [next] | [standalone]

#1495500 — Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2016-10-04 18:20 +0200
SubjectRe: [PATCH 06/14] ASoC: Add sun8i digital audio codec
Message-ID<soABH-57P-9@gated-at.bofh.it>
In reply to#1495336

[Multipart message — attachments visible in raw view] — view raw

Hi,

> +static const struct of_device_id sun8i_codec_of_match[] = {
> +	{ .compatible = "allwinner,sun8i-a33-codec" },
> +	{ .compatible = "allwinner,sun8i-a23-codec" },

I thought that the A23 and A33 had different codecs? In that case, it
wouldn't be a good assumption to make


> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_codec_of_match);
> +
> +static struct platform_driver sun8i_codec_driver = {
> +	.driver = {
> +		.name = "sun8i-codec",
> +		.owner = THIS_MODULE,
> +		.of_match_table = sun8i_codec_of_match,
> +	},
> +	.probe = sun8i_codec_probe,
> +	.remove = sun8i_codec_remove,
> +};
> +module_platform_driver(sun8i_codec_driver);
> +
> +MODULE_DESCRIPTION("Allwinner A33 (sun8i) codec driver");
> +MODULE_AUTHOR("huanxin<huanxin@reuuimllatech.com>");

Those obfuscated email adresses are not really helpful :)

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1496798 — Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2016-10-06 20:30 +0200
SubjectRe: [PATCH 06/14] ASoC: Add sun8i digital audio codec
Message-ID<splAB-4sH-13@gated-at.bofh.it>
In reply to#1495500
On 07/10/2016 at 00:06:57 +0800, Icenowy Zheng wrote :
> 05.10.2016, 00:20, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > Hi,
> >
> >>  +static const struct of_device_id sun8i_codec_of_match[] = {
> >>  + { .compatible = "allwinner,sun8i-a33-codec" },
> >>  + { .compatible = "allwinner,sun8i-a23-codec" },
> >
> > I thought that the A23 and A33 had different codecs? In that case, it
> > wouldn't be a good assumption to make
> 
> Yes.
> 
> >
> >>  + {}
> >>  +};
> >>  +MODULE_DEVICE_TABLE(of, sun8i_codec_of_match);
> >>  +
> >>  +static struct platform_driver sun8i_codec_driver = {
> >>  + .driver = {
> >>  + .name = "sun8i-codec",
> >>  + .owner = THIS_MODULE,
> >>  + .of_match_table = sun8i_codec_of_match,
> >>  + },
> >>  + .probe = sun8i_codec_probe,
> >>  + .remove = sun8i_codec_remove,
> >>  +};
> >>  +module_platform_driver(sun8i_codec_driver);
> >>  +
> >>  +MODULE_DESCRIPTION("Allwinner A33 (sun8i) codec driver");
> >>  +MODULE_AUTHOR("huanxin<huanxin@reuuimllatech.com>");
> >
> > Those obfuscated email adresses are not really helpful :)
> 
> This kind of email addresses  are kept in many places in mainline kernel.
> 
> e.g. drivers/mmc/host/sunxi-mmc.c have 'Aaron Maoye <leafy.myeh@reuuimllatech.com>'
> 

Well, that is only one place and it is a comment, not in the
MODULE_AUTHOR macro. I would agree that it is not useful to have a stale
email address in MODULE_AUTHOR.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1495337 — [PATCH 09/14] dt-bindings: sound: Add sun8i codec documentation

FromMylène Josserand <mylene.josserand@free-electrons.com>
Date2016-10-04 12:00 +0200
Subject[PATCH 09/14] dt-bindings: sound: Add sun8i codec documentation
Message-ID<souFY-W7-23@gated-at.bofh.it>
In reply to#1495322
Add the documentation for dt-binding of the digital audio codec driver
for sun8i SoC.

Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
---
 .../devicetree/bindings/sound/sun8i-codec.txt      | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/sun8i-codec.txt

diff --git a/Documentation/devicetree/bindings/sound/sun8i-codec.txt b/Documentation/devicetree/bindings/sound/sun8i-codec.txt
new file mode 100644
index 0000000..1808869
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/sun8i-codec.txt
@@ -0,0 +1,24 @@
+* Allwinner A23/A33 Codec
+
+Required properties:
+- compatible: must be either "allwinner,sun4i-a23-codec" or
+  "allwinner,sun7i-a33-codec"
+- reg: must contain the registers location and length
+- interrupts: must contain the codec interrupt
+- clocks: a list of phandle + clock-specifer pairs, one for each entry
+  in clock-names.
+- clock-names: should contain followings:
+   - "apb": the parent APB clock for this controller
+   - "codec": the parent module clock
+
+Example:
+codec: codec@01c22e00 {
+	#sound-dai-cells = <0>;
+	compatible = "allwinner,sun8i-a33-codec";
+	reg = <0x01c22e00 0x400>; /* SUNXI_AUDIO_PBASE + 0x200 */
+	reg-names = "audio";
+	interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&ccu CLK_BUS_CODEC>, <&ccu CLK_AC_DIG>;
+	clock-names = "apb", "codec";
+	status = "disabled";
+};
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1495506 — Re: [PATCH 09/14] dt-bindings: sound: Add sun8i codec documentation

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2016-10-04 18:30 +0200
SubjectRe: [PATCH 09/14] dt-bindings: sound: Add sun8i codec documentation
Message-ID<soALo-5bt-23@gated-at.bofh.it>
In reply to#1495337

[Multipart message — attachments visible in raw view] — view raw

On Tue, Oct 04, 2016 at 11:46:22AM +0200, Mylène Josserand wrote:
> Add the documentation for dt-binding of the digital audio codec driver
> for sun8i SoC.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
> ---
>  .../devicetree/bindings/sound/sun8i-codec.txt      | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/sun8i-codec.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/sun8i-codec.txt b/Documentation/devicetree/bindings/sound/sun8i-codec.txt
> new file mode 100644
> index 0000000..1808869
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/sun8i-codec.txt
> @@ -0,0 +1,24 @@
> +* Allwinner A23/A33 Codec
> +
> +Required properties:
> +- compatible: must be either "allwinner,sun4i-a23-codec" or
> +  "allwinner,sun7i-a33-codec"

Copy and paste issue ? :)

One compatible by line is usually favored, since when you'll add new
compatibles, you don't have to modify the context.xs

> +- reg: must contain the registers location and length
> +- interrupts: must contain the codec interrupt
> +- clocks: a list of phandle + clock-specifer pairs, one for each entry
> +  in clock-names.
> +- clock-names: should contain followings:
> +   - "apb": the parent APB clock for this controller
> +   - "codec": the parent module clock

We're usually calling them "bus" and "mod".

> +
> +Example:
> +codec: codec@01c22e00 {
> +	#sound-dai-cells = <0>;
> +	compatible = "allwinner,sun8i-a33-codec";
> +	reg = <0x01c22e00 0x400>; /* SUNXI_AUDIO_PBASE + 0x200 */
> +	reg-names = "audio";

You don't define reg-names in your bindings, while your code relies on
it. It isn't really needed, since you have only one couple of base +
size, so it should just go away.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [standalone]

Page 3 of 3 — ← Prev page 1 2 [3]

Back to top | Article view | linux.kernel

csiph-web