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

[Takashi Iwai <tiwai@suse.de>]

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:
> > > On Tue, Apr 23, 2019 at 04:13:35PM +0200, Takashi Iwai 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
> > > things like gain settings and pad configuration which look very board
> > > specific.  Register patches are supposed to be for things like early
> > > revisions of the chip which have different register defaults or magic
> > > sequences that vendors tell you to run on startup, usually to tune test
> > > registers.
> 
> > 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?

> > > > +#define cx2072x_plbk_eq_en_info		snd_ctl_boolean_mono_info
> 
> > > Why not just use the function directly rather than hiding it?
> 
> > Just a standard idiom.  Can be replaced if preferred.
> 
> Please.
> 
> > > > +int snd_soc_cx2072x_enable_jack_detect(struct snd_soc_component *codec)
> > > > +{
> > > > +	struct cx2072x_priv *cx2072x = snd_soc_component_get_drvdata(codec);
> > > > +	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(codec);
> > > > +
> > > > +	/* No-sticky input type */
> > > > +	regmap_write(cx2072x->regmap, CX2072X_GPIO_STICKY_MASK, 0x1f);
> > > > +
> > > > +	/* Use GPOI0 as interrupt pin */
> > > > +	regmap_write(cx2072x->regmap, CX2072X_UM_INTERRUPT_CRTL_E, 0x12 << 24);
> 
> > > This isn't board specific is it?
> 
> > I have no idea.  It's been so from the original code, and there
> > doesn't seem any other hardware implementations.
> 
> 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.

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

> > > > +	dev_dbg(codec->dev, "CX2072X_HSDETECT type=0x%X,Jack state = %x\n",
> > > > +		type, state);
> > > > +	return state;
> > > > +}
> > > > +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.

> > > > +	/* use flat eq by default */
> > > > +	for (ch = 0 ; ch < 2 ; ch++) {
> > > > +		for (band = 0; band < CX2072X_PLBK_EQ_BAND_NUM; band++) {
> > > > +			cx2072x->plbk_eq[ch][band][1] = 64;
> > > > +			cx2072x->plbk_eq[ch][band][10] = 3;
> > > > +		}
> > > > +	}
> 
> > > Why not use the register defaults?
> 
> > Because it'll become too messy for put flatten array values?
> > The initialization using loop makes more sense in such a case, IMO.
> 
> 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.


thanks,

Takashi