From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965140AbbLRUsI (ORCPT ); Fri, 18 Dec 2015 15:48:08 -0500 Received: from down.free-electrons.com ([37.187.137.238]:36075 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932736AbbLRUsG (ORCPT ); Fri, 18 Dec 2015 15:48:06 -0500 Date: Fri, 18 Dec 2015 11:19:43 +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: [linux-sunxi] Re: [PATCH v6] sun4i-codec: Add FM, Line and Mic inputs Message-ID: <20151218101943.GI30359@lukather> References: <20151209235530.7240df0a@dayas> <20151213205839.GA19456@lukather> <20151215025208.7c468673@dayas> <20151216104736.GT19456@lukather> <20151216233051.09d34738@dayas> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="y96v7rNg6HAoELs5" Content-Disposition: inline In-Reply-To: <20151216233051.09d34738@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 --y96v7rNg6HAoELs5 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Dec 16, 2015 at 11:30:51PM +0100, Danny Milosavljevic wrote: > Hi Maxime, >=20 > On Wed, 16 Dec 2015 11:47:36 +0100 > Maxime Ripard wrote: >=20 > > > Because SUN4I_CODEC_AC_MIC_PHONE_CAL is sun7i-specific. > >=20 > > Yet, you're using it in both cases (A10 vs A20). >=20 > Yes. I'm trying to keep complexity and duplication down. > I figured it wouldn't be bad to have unused registers in the regmap. >=20 > (Technially .max_register =3D MAX(max_register_a10, max_register_a20) wou= ld be=20 > better. Should we do that?) >=20 > If it's bad in this case, we have to split it up, but frankly the *codec_= probe()=20 > function is much too long now and this would make it even longer. >=20 > Also, it was that way before, so I'm mostly using it in both cases becaus= e=20 > previously it was also used in both cases (with the too-large max-registe= r),=20 > apparently without problems.=20 >=20 > Should I duplicate and adapt the structure? No, my point was that you don't need to move it around at all. > > You can also have the defines on top, and everything just works :) >=20 > The idea is to make the compiler complain when I try to use a sun7i defin= e in a=20 > generic sun4i function (or struct, in this case) - because that would pro= bably=20 > be causing problems at runtime, too. Better to catch problems earlier. > So I kept the sun7i-specific things closely together and as much to the b= ottom=20 > of the file as possible - as a poor-mans modularity.=20 > If I kept the sun7i defines at the top I could use them anywhere with imp= unity -=20 > also in the A10 case - and it would not complain. >=20 > But it's mostly to make the life of the developer easier, so feel free to= choose=20 > otherwise. (not sarcasm) I understand your point to develop it, but now, the development is done :) Having all the defines packed together is easier to read and maintain after the development is done. > > > Or is it better to rename it to "SUN7I_CODEC_AC_DAC_CAL" rather than = delete? > >=20 > > You can rename it if you want, but it's not like it's of the highest > > importance :) >=20 > The only somewhat important part of the name is the "7".=20 > If you use a "7"-register on an A10, it's not going to work at runtime, o= r worse:=20 > do something else that wasn't intended. Right now it has a "4" although i= t isn't > an A10 register. This separation should be visible somewhere in the sourc= e code,=20 > or problems are going to slip through later. >=20 > I agree it's not at all important right now because the register is unuse= d=20 > by us :P Exactly my point ;) Like I said, if you want to rename it, go ahead. It would also be a good idea to open a github issue on allwinner's documentation repo to make them know that the register name doesn't match between the register list and the register documentations. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --y96v7rNg6HAoELs5 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWc92/AAoJEBx+YmzsjxAgXgMP/jYhLQ4yXB+e0PD2EnHpLyQR q6Sx56zsOrLuekHTQ7sDd5R6sSMMhhYM+gxSlISfqVNW4FACGjhcy31TXWySd9Bh eUgkca3/KYHnvoWaowKmdZ3QD71TvGvnmCwi4+0x0A9mtYjSjHOjPHdc5fD/IYqN 6zgT/UF6UJoxvFVjraRrfw6Q5TyLLLwa+xaiG2qlU7SVuy2vdwXkX0NnBn8LbrCn 1o6uX8vZf9yc2sAtv8f2eDDDenvfVDOIJSjSqMfVmQzXSKtWdGPEei2ikm55AUVp f/BsUfqO1+LYXgwRw9/d3orWbN12a3wouKXWXD6vbRChAo8hENwVe3HuYhcONlWh 8nZpTG7AGDzgv9aFagmpGTEq9FYAvjKRUs8CS0PxYqM6sk57O6Vf/zblAWFzotdh QmBjApO2iCaNlE6Y3LWq1iLq1mwwUG/0zf6f/Lv3QxdahlNAJPArPNzWrLvrrjFC F4FA15v+nF87lOYcAnFHDp1Xx64VitiJ59/vFs8Jq1Buh88EhnOxXSS8H07qUG79 umnMTT8EDf2UGh9Pj41wM9/2f4sD/pHb+z1j9c1EoNG7m0tmzAnZb5nq89IwQW/a PC/4oJRs7B/1WGB/yUftseej1Lr80e8k2KS9vM0QJQuR4z+tJra03KMeCoq4ZqMg ohlP85WYIaUw960rStE3 =ssRv -----END PGP SIGNATURE----- --y96v7rNg6HAoELs5-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Subject: Re: Re: [PATCH v6] sun4i-codec: Add FM, Line and Mic inputs Date: Fri, 18 Dec 2015 11:19:43 +0100 Message-ID: <20151218101943.GI30359@lukather> References: <20151209235530.7240df0a@dayas> <20151213205839.GA19456@lukather> <20151215025208.7c468673@dayas> <20151216104736.GT19456@lukather> <20151216233051.09d34738@dayas> Reply-To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="y96v7rNg6HAoELs5" Return-path: Content-Disposition: inline In-Reply-To: <20151216233051.09d34738@dayas> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Danny Milosavljevic Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, Liam Girdwood , Mark Brown , Jaroslav Kysela , Takashi Iwai , Chen-Yu Tsai , alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: alsa-devel@alsa-project.org --y96v7rNg6HAoELs5 Content-Type: text/plain; charset=UTF-8 Content-Disposition: inline On Wed, Dec 16, 2015 at 11:30:51PM +0100, Danny Milosavljevic wrote: > Hi Maxime, > > On Wed, 16 Dec 2015 11:47:36 +0100 > Maxime Ripard wrote: > > > > Because SUN4I_CODEC_AC_MIC_PHONE_CAL is sun7i-specific. > > > > Yet, you're using it in both cases (A10 vs A20). > > Yes. I'm trying to keep complexity and duplication down. > I figured it wouldn't be bad to have unused registers in the regmap. > > (Technially .max_register = MAX(max_register_a10, max_register_a20) would be > better. Should we do that?) > > If it's bad in this case, we have to split it up, but frankly the *codec_probe() > function is much too long now and this would make it even longer. > > Also, it was that way before, so I'm mostly using it in both cases because > previously it was also used in both cases (with the too-large max-register), > apparently without problems. > > Should I duplicate and adapt the structure? No, my point was that you don't need to move it around at all. > > You can also have the defines on top, and everything just works :) > > The idea is to make the compiler complain when I try to use a sun7i define in a > generic sun4i function (or struct, in this case) - because that would probably > be causing problems at runtime, too. Better to catch problems earlier. > So I kept the sun7i-specific things closely together and as much to the bottom > of the file as possible - as a poor-mans modularity. > If I kept the sun7i defines at the top I could use them anywhere with impunity - > also in the A10 case - and it would not complain. > > But it's mostly to make the life of the developer easier, so feel free to choose > otherwise. (not sarcasm) I understand your point to develop it, but now, the development is done :) Having all the defines packed together is easier to read and maintain after the development is done. > > > 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 :) > > The only somewhat important part of the name is the "7". > If you use a "7"-register on an A10, it's not going to work at runtime, or worse: > do something else that wasn't intended. Right now it has a "4" although it isn't > an A10 register. This separation should be visible somewhere in the source code, > or problems are going to slip through later. > > I agree it's not at all important right now because the register is unused > by us :P Exactly my point ;) Like I said, if you want to rename it, go ahead. It would also be a good idea to open a github issue on allwinner's documentation repo to make them know that the register name doesn't match between the register list and the register documentations. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --y96v7rNg6HAoELs5-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: maxime.ripard@free-electrons.com (Maxime Ripard) Date: Fri, 18 Dec 2015 11:19:43 +0100 Subject: [linux-sunxi] Re: [PATCH v6] sun4i-codec: Add FM, Line and Mic inputs In-Reply-To: <20151216233051.09d34738@dayas> References: <20151209235530.7240df0a@dayas> <20151213205839.GA19456@lukather> <20151215025208.7c468673@dayas> <20151216104736.GT19456@lukather> <20151216233051.09d34738@dayas> Message-ID: <20151218101943.GI30359@lukather> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Dec 16, 2015 at 11:30:51PM +0100, Danny Milosavljevic wrote: > Hi Maxime, > > On Wed, 16 Dec 2015 11:47:36 +0100 > Maxime Ripard wrote: > > > > Because SUN4I_CODEC_AC_MIC_PHONE_CAL is sun7i-specific. > > > > Yet, you're using it in both cases (A10 vs A20). > > Yes. I'm trying to keep complexity and duplication down. > I figured it wouldn't be bad to have unused registers in the regmap. > > (Technially .max_register = MAX(max_register_a10, max_register_a20) would be > better. Should we do that?) > > If it's bad in this case, we have to split it up, but frankly the *codec_probe() > function is much too long now and this would make it even longer. > > Also, it was that way before, so I'm mostly using it in both cases because > previously it was also used in both cases (with the too-large max-register), > apparently without problems. > > Should I duplicate and adapt the structure? No, my point was that you don't need to move it around at all. > > You can also have the defines on top, and everything just works :) > > The idea is to make the compiler complain when I try to use a sun7i define in a > generic sun4i function (or struct, in this case) - because that would probably > be causing problems at runtime, too. Better to catch problems earlier. > So I kept the sun7i-specific things closely together and as much to the bottom > of the file as possible - as a poor-mans modularity. > If I kept the sun7i defines at the top I could use them anywhere with impunity - > also in the A10 case - and it would not complain. > > But it's mostly to make the life of the developer easier, so feel free to choose > otherwise. (not sarcasm) I understand your point to develop it, but now, the development is done :) Having all the defines packed together is easier to read and maintain after the development is done. > > > 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 :) > > The only somewhat important part of the name is the "7". > If you use a "7"-register on an A10, it's not going to work at runtime, or worse: > do something else that wasn't intended. Right now it has a "4" although it isn't > an A10 register. This separation should be visible somewhere in the source code, > or problems are going to slip through later. > > I agree it's not at all important right now because the register is unused > by us :P Exactly my point ;) Like I said, if you want to rename it, go ahead. It would also be a good idea to open a github issue on allwinner's documentation repo to make them know that the register name doesn't match between the register list and the register documentations. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: