Re: [PATCH 1/2] ASoC: Add support for Conexant CX2072X CODEC

[Takashi Iwai <tiwai@suse.de>]

On Mon, 06 May 2019 06:26:25 +0200,
Mark Brown wrote:
> 
> On Fri, May 03, 2019 at 09:18:26AM +0200, Takashi Iwai wrote:
> > On Fri, 03 May 2019 08:47:29 +0200,
> > Mark Brown wrote:
> > > On Thu, May 02, 2019 at 09:04:06AM +0200, Takashi Iwai wrote:
> > > > Mark Brown wrote:
> 
> > > > > This looks *very* much like board configuration rather than a patch -
> > > > > there's no kind of test bit and the comments talk specifically about
> 
> > > > OK, will replace with the straight regmap_multi_reg_write().
> 
> > > That's probably not addressing the issue, a lot of that stuff just
> > > doesn't seem like it should be in some fixed configuration table at all.
> 
> > So what's your alternative suggestion?
> 
> Make it either platform data or runtime controls depending on the
> specific thing being configured.  Though...
> 
> > > Oh, joy.  What's the story here?  Do you have a datasheet for the part?
> 
> > Not at all.  I just refreshed the already submitted patches since I
> > have a laptop with the codec.  I tried to contact Conexant, but in
> > vain, so I decided to submit the renewed one.
> 
> ...this is not going to help.  At the very least it needs to be
> abundantly clear to anyone reading the code tht any magic register write
> sequences like this are being done the way they are due to a badly
> documented part from an unsupportive vendor rather than because it's a
> good idea in case someone decides to use the driver as an example.  Some
> comments or something.  For safety the bits that look board specific
> like the pinmuxing should ideally be behind DMI checks, though if nobody
> else ever bought the CODEC that's probably not so important.

Fair enough.  I'm going to put more comments on that part.

Since this is the only code for this codec on the net and all existing
platforms (Intel devices, RPi extensions and else) seem working with
it -- i.e. all these depend on the same init sequence more or less, so
splitting to platform data won't help much in practice.

> > > > The jack detection in ASoC is anyway a bit funky, especially when
> > > > involved with PM...
> 
> > > What do you mean here?  I'm not aware of any issues and the systems I've
> > > worked with seemed robust...
> 
> > There are tons of different ways of implementation for jack controls,
> > with different API usages.  IOW, no consistency.
> 
> The board registration interfaces do vary as the hardware isn't very
> standard so there's a bunch of things you can do for configuration
> (though there is a generic interface for the 90% case, the main
> limitation these days on using it is Intel's lack of any sensible
> firmware interface).  I'm not aware of any issues with power management
> though?

Yeah, I understand the various use cases and coverage for a wide range
of hardware implementations, but the lack of the documentation
is... not a thing we can be proud of.

Regarding the PM, my statement wasn't straight, sorry.  The fact is
that ASoC manages so many different levels of (a sort of) "PM".  We
have the Linux device-object system and runtime PMs, and we have BIAS
level control.  The machine needed the jack handling via ACPI GPIO
IRQ, that is outside the codec, which looks rather like a rare case, 
and  I had hard time to look for the right choice of the API usage.

> > > > > > +EXPORT_SYMBOL_GPL(snd_soc_cx2072x_get_jack_state);
> 
> > > > > Why is this symbol exported?
> 
> > > > It's called from the machine driver.
> > > > snd_soc_jack_add_gpios() is called in the machine driver side, and it
> > > > needs the jack_status_check callback that calls this function.
> 
> > > That code shouldn't be in the machine driver, the CODEC driver should
> > > request any interrupts it needs itself.
> 
> > The similar things are done on many other Intel SST board drivers.
> > The current patch just follows the pattern.
> 
> The only example I can find is the max98095 which it just plain buggy in
> this regard, that example shouldn't be followed - it looks like it's
> just plain an oversight in review.  Frankly the export was done by me
> and looks like I was just rushing to fix up a build breakage.

This is rather a question how generic the codec driver should be
written.  I changed the code in v5 patchset to embed the jack_gpio
stuff inside the codec driver side rather than the machine driver, so
the whole exported functions can be reduced now.  But, of course, it
slightly gives more implicit assumption about the hardware
implementations, too.  Though, the existing code seems to have already
fixed gpio / pin setups, so the other setups wouldn't have worked, in
anyway.

> > > No, that's not the question.  The question is why there is any
> > > initialization at all?
> 
> > Again, no idea.  These are likely no default values of the hardware
> > registers, and we need to set up some.  I *guess* ditto for the
> > initial register table in the above, too.
> 
> Or it's just a taste setting someone had for their particular system.
> With the earlier register default table an awful lot of it is fairly
> obvious board specific configuration, doing things like pinmuxing which
> will vary with the board.
> 
> It's unlikely there are *no* default values - you'd kind of have to try
> to have a specific register range like this with genuinely floating
> values.  Given that the code for configuring the EQ was broken to IIRC
> never take effect I'd not be 100% surprisd if someone couldn't figure
> out why their settings weren't taking effect and just bodged something
> directly in the driver.

Actually I'm fine to drop the whole EQ stuff that brings lots of black
magic.  Certainly it'll benefit us for code simplification.  Let's
see.


thanks,

Takashi