Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965580AbbLPKrk (ORCPT ); Wed, 16 Dec 2015 05:47:40 -0500 Received: from down.free-electrons.com ([37.187.137.238]:38036 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751232AbbLPKri (ORCPT ); Wed, 16 Dec 2015 05:47:38 -0500 Date: Wed, 16 Dec 2015 11:47:36 +0100 From: Maxime Ripard To: Danny Milosavljevic Cc: linux-sunxi@googlegroups.com, Liam Girdwood , Mark Brown , Jaroslav Kysela , Takashi Iwai , Chen-Yu Tsai , alsa-devel@alsa-project.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v6] sun4i-codec: Add FM, Line and Mic inputs Message-ID: <20151216104736.GT19456@lukather> References: <20151209235530.7240df0a@dayas> <20151213205839.GA19456@lukather> <20151215025208.7c468673@dayas> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="EmW68jKGQIhj8inv" Content-Disposition: inline In-Reply-To: <20151215025208.7c468673@dayas> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4604 Lines: 141 --EmW68jKGQIhj8inv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Tue, Dec 15, 2015 at 02:52:08AM +0100, Danny Milosavljevic wrote: > Hi Maxime, >=20 > On Sun, 13 Dec 2015 21:58:39 +0100 > Maxime Ripard wrote: >=20 > > This is not the branch you should be basing your patch on. This is an > > ASoC patch, base it on the ASoC tree. >=20 > Okay, will do. To the branch "sunxi-next" in=20 > , right? >=20 > [...] > > > -static const struct regmap_config sun4i_codec_regmap_config =3D { > > > - .reg_bits =3D 32, > > > - .reg_stride =3D 4, > > > - .val_bits =3D 32, > > > - .max_register =3D SUN4I_CODEC_AC_MIC_PHONE_CAL, > > > -}; > > > - > >=20 > > Why is this moved? >=20 > 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= =20 > together sun7i-specific things: >=20 > > +/* 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 =3D { > > + .reg_bits =3D 32, > > + .reg_stride =3D 4, > > + .val_bits =3D 32, > > + .max_register =3D SUN7I_CODEC_AC_MIC_PHONE_CAL, > > +}; > > +/* end sun7i-specific things */ >=20 > I thought about also renaming sun4i_codec_regmap_config but decided again= st it=20 > since it's fine to use it on A10 and I think it's best if the name reflec= ts=20 > the minimum required hardware. >=20 > On the other hand, once I moved the define, sun4i-codec won't compile if= =20 > 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= =20 > A10/A20 split. >=20 > 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, an= d called=20 > "AC_DAC_CAL" in the A20 User Manual v1.4 20150510. Ok to delete?=20 > Or is it better to rename it to "SUN7I_CODEC_AC_DAC_CAL" rather than dele= te? You can rename it if you want, but it's not like it's of the highest importance :) >=20 > > > 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_devi= ce *pdev) > > > struct resource *res; > > > void __iomem *base; > > > int ret; > > > + const struct snd_soc_codec_driver* codec_codec; > >=20 > > I guess a single codec is enough :) >=20 > Modeled after the name of the original variable, see below :) >=20 > But OK, I'll rename it to "codec". >=20 > 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 --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --EmW68jKGQIhj8inv Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWcUFIAAoJEBx+YmzsjxAgvEcQAIJv1+qv9p7XjWhApereXtiF Z1h0gxxtuiuS14+DCGCz6B0+Wwfo54fcLkJYRvrfSZq0br80CaVqmTXF8Fz408/C BhUnFSbs864eVviMW2yjiNym856zitphJe6Po3LPxi1Xg1YW2a1pSRx/0bxPn6Xx 7BPpZ6vyLozFLDZn3bOy3vZ81kiw7Ygwdm5YFJTp7fj20PDUPoD58O7P09ltUSwH tY1IKuFJYuPr4X5C1vKaJukjMNRlNpNHPxjIymwofys+lPIW8uO9P4OQXjzesA9b fGKc9zIXibpCgTgy5r6wP1ZM4Lgr+4eNsM4siPett2iRbWq3YKZtmZ4H8UEoe9oY wDtIapwKmZVsQLSoH2JtXXl+kk6CdqDA2GcZF7mAiNyqhGU+ZG4OdnLFJ0IVRKjP xzEQSfyiYfmp/Xe3XPouRyBrl9A4hhyArn+CSlOJzvbir8FCT2BbtUoccbICN8xk k7YQPNUMp/qZ/d2Ijqhlaj9xcEqqKUazDorOjPnVF7HGe7GuOY4pxy6o1NzgrttP 9XWfHLZ8av4y/NwKCh5APqI8+y99TPzoVK/VcmRrPeuUMCV5l894CIYI6MA2DKGF hgkyw8ehVj/JT8VT+S7VlWLepaE2xh8yrXXRoLSkHI5rt2rSkrFE4FEPnBU/F56+ QxEY8uoNxm4bzgIpZkdK =qk1K -----END PGP SIGNATURE----- --EmW68jKGQIhj8inv-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/