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--