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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
