Re: [PATCH v1 5/6] ASoC: codecs: msm8916-wcd-analog: add MBHC support

[Srinivas Kandagatla]

On 28/07/17 14:51, Mark Brown wrote:
> On Wed, Jul 26, 2017 at 02:35:11AM +0200, srinivas.kandagatla@xxxxxxxxxx wrote:
>
> > +	if (wcd->hphl_jack_type_normally_open)
> > +		plug_type = CDC_A_HPHL_PLUG_TYPE_NO;
> > +
> > +	if (wcd->gnd_jack_type_normally_open)
> > +		plug_type |= CDC_A_GND_PLUG_TYPE_NO;
>
> It'd be clearer to use |= consistently, this is just confusing as it
> makes it look like there's a path where things aren't initialized.

Yep, I will fix this in next version.
> > +	btn_result = snd_soc_read(codec, CDC_A_MBHC_RESULT_1) &
> > +				  CDC_A_MBHC_RESULT_1_BTN_RESULT_MASK;
> > +
> > +	if (!btn_result)
> > +		priv->mbhc_btn0_pressed = false;
> > +
> > +	snd_jack_report(priv->jack->jack, 0);
>
> This checks to see if the read worked, uses it to decide if it should
> clear the button pressed flag but unconditionally reports that the
> button is not pressed.  It also discards the read error if there was one
> without reporting that.  This needs cleanup.
Okay, will clean this part in next version.
> > +	btn_result = snd_soc_read(codec, CDC_A_MBHC_RESULT_1) &
> > +				  CDC_A_MBHC_RESULT_1_BTN_RESULT_MASK;
> > +
> > +	switch (btn_result) {
> > +	case 0xf:
> > +		snd_jack_report(priv->jack->jack, SND_JACK_BTN_4);
> > +		break;
> > +	case 0x7:
> > +		snd_jack_report(priv->jack->jack, SND_JACK_BTN_3);
> > +		break;
> > +	case 0x3:
> > +		snd_jack_report(priv->jack->jack, SND_JACK_BTN_2);
> > +		break;
> > +	case 0x1:
> > +		snd_jack_report(priv->jack->jack, SND_JACK_BTN_1);
> > +		break;
> > +	case 0:
> > +		priv->mbhc_btn0_pressed = true;
> > +		snd_jack_report(priv->jack->jack, SND_JACK_BTN_0);
> > +		break;
> > +	}
>
> Here we silently ignore any value we get back from the chip that we
> didn't expect and all read errors.

I would not expect anyother values if the vrefs are programmed 
correctly, however i will add a error message in default case to notify 
this to user.

> > +struct msm8916_wcd_mbhc_data {
> > +	/* Voltage threshold when internal current source of 100uA is used */
> > +	int vref_btn_cs[MBHC_MAX_BUTTONS];
> > +	/* Voltage threshold when microphone bias is ON */
> > +	int vref_btn_micb[MBHC_MAX_BUTTONS];
> > +};
>
> I'd expect to see some mechanism for configuring the threasholds via DT,
> not all vendors use the same specs for resistive buttons.

Yep makes sense, I will do this in next version.



>