Re: [alsa-devel] [PATCH 1/2] ASoC: Add support for Conexant CX2072X CODEC
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
participants (1)
-
Takashi Iwai