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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 28 Jul 2017 15:12:06 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     Banajit Goswami <bgoswami@...eaurora.org>,
        alsa-devel@...a-project.org, Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Patrick Lai <plai@...eaurora.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 5/6] ASoC: codecs: msm8916-wcd-analog: add MBHC support



On 28/07/17 14:51, Mark Brown wrote:
> On Wed, Jul 26, 2017 at 02:35:11AM +0200, srinivas.kandagatla@...aro.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.



>