lkml.org 
[lkml]   [2017]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 5/6] ASoC: codecs: msm8916-wcd-analog: add MBHC support
From
Date


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.



>

\
 
 \ /
  Last update: 2017-07-28 16:12    [W:0.287 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site