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

[Maxime Ripard]

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 <maxime.ripard@xxxxxxxxxxxxxxxxxx> 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

Attachment: signature.asc
Description: Digital signature