linux-kernel - Re: [PATCH v6] sun4i-codec: Add FM, Line and Mic inputs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151216104736.GT19456@lukather>
Date:	Wed, 16 Dec 2015 11:47:36 +0100
From:	Maxime Ripard <maxime.ripard@...e-electrons.com>
To:	Danny Milosavljevic <dannym@...atchpost.org>
Cc:	linux-sunxi@...glegroups.com, Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>, Chen-Yu Tsai <wens@...e.org>,
	alsa-devel@...a-project.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] sun4i-codec: Add FM, Line and Mic inputs

Hi,

On Tue, Dec 15, 2015 at 02:52:08AM +0100, Danny Milosavljevic wrote:
> Hi Maxime,
> 
> On Sun, 13 Dec 2015 21:58:39 +0100
> Maxime Ripard <maxime.ripard@...e-electrons.com> wrote:
> 
> > This is not the branch you should be basing your patch on. This is an
> > ASoC patch, base it on the ASoC tree.
> 
> Okay, will do. To the branch "sunxi-next" in 
> <git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git>, right?
> 
> [...]
> > > -static const struct regmap_config sun4i_codec_regmap_config = {
> > > -	.reg_bits	= 32,
> > > -	.reg_stride	= 4,
> > > -	.val_bits	= 32,
> > > -	.max_register	= SUN4I_CODEC_AC_MIC_PHONE_CAL,
> > > -};
> > > -
> > 
> > Why is this moved?
> 
> Because SUN4I_CODEC_AC_MIC_PHONE_CAL is sun7i-specific.

Yet, you're using it in both cases (A10 vs A20).

> Note: I also renamed it and moved the #define in the course of grouping 
> together sun7i-specific things:
> 
> > +/* sun7i-specific things: */
> > +/* MIC_PHONE_CAL register offsets and bit fields (A20 only) */
> > +#define SUN7I_CODEC_AC_MIC_PHONE_CAL		(0x3c)
> [...]
> > +static const struct regmap_config sun4i_codec_regmap_config = {
> > +	.reg_bits	= 32,
> > +	.reg_stride	= 4,
> > +	.val_bits	= 32,
> > +	.max_register	= SUN7I_CODEC_AC_MIC_PHONE_CAL,
> > +};
> > +/* end sun7i-specific things */
> 
> I thought about also renaming sun4i_codec_regmap_config but decided against it 
> since it's fine to use it on A10 and I think it's best if the name reflects 
> the minimum required hardware.
> 
> On the other hand, once I moved the define, sun4i-codec won't compile if 
> sun4i_codec_regmap_config is left at the top. So I had to move it, too.

You can also have the defines on top, and everything just works :)

> It will be clearer once I post a patch doing just the preparation of the 
> A10/A20 split.
> 
> I just checked A10 vs A20 some more:
> There's also SUN4I_CODEC_AC_SYS_VERI 0x38 present in original ASoC and in 4.4-rc2.
> It's unused by us, not mentioned in the A10 User manual V1.5 20130820, and called 
> "AC_DAC_CAL" in the A20 User Manual v1.4 20150510. Ok to delete? 
> Or is it better to rename it to "SUN7I_CODEC_AC_DAC_CAL" rather than delete?

You can rename it if you want, but it's not like it's of the highest
importance :)

> 
> > >  static int sun4i_codec_probe(struct platform_device *pdev)
> > >  {
> > >  	struct snd_soc_card *card;
> > > @@ -593,6 +740,7 @@ static int sun4i_codec_probe(struct platform_device *pdev)
> > >  	struct resource *res;
> > >  	void __iomem *base;
> > >  	int ret;
> > > +	const struct snd_soc_codec_driver* codec_codec;
> > 
> > I guess a single codec is enough :)
> 
> Modeled after the name of the original variable, see below :)
> 
> But OK, I'll rename it to "codec".
> 
> Note: the newest original ASoC sun4i-codec has a variable
>   "struct sun4i_codec *scodec;"
> as well in the same function (which is a different thing).

I don't know what you're refering to with "newest" and "original".

But two different variables with two different names doesn't seem so
bad, does it?

Thanks!
Maxime

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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)