Messages in this thread | | | Subject | Re: [PATCH v1 5/6] ASoC: codecs: msm8916-wcd-analog: add MBHC support | From | Srinivas Kandagatla <> | Date | Fri, 28 Jul 2017 15:12:06 +0100 |
| |
On 28/07/17 14:51, Mark Brown wrote: > On Wed, Jul 26, 2017 at 02:35:11AM +0200, srinivas.kandagatla@linaro.org 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.
>
| |