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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 03/08/17 13:53, Damien Riegel wrote:
> On Thu, Aug 03, 2017 at 12:11:04PM +0100, Srinivas Kandagatla wrote:
>>
>>
>> On 03/08/17 00:33, Damien Riegel wrote:
>>> Hi Srinivas,
>>>
>>>
>>> On Wed, Aug 02, 2017 at 07:09:28PM +0200, srinivas.kandagatla@linaro.org wrote:
>>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>>
>>>> MBHC (MultiButton Headset Control) support is available in pm8921 in two
>>>> blocks, one to detect mechanical headset insertion and removal and other
>>>> block to support headset type detection and 5 button detection and othe
>>>> features like impedance calculation.
>>>>
>>>> This patch adds support to:
>>>> 1> Support to NC and NO type of headset Jacks.
>>>> 2> Mechanical insertion and detection of headset jack.
>>>> 3> Detect a 3 pole Headphone and a 4 pole Headset.
>>>> 4> Detect 5 buttons.
>>>>
>>>> Tested it on DB410c with Audio Mezz board with 4 pole and 3 pole
>>>> headset/headphones.
>>>
>>
>>> Good news! This version has better results. I still have an issue where
>>> a headphone is reported as a headset when I do this sequence:
>>
>> Hi Damien,
>>
>> Thanks for testing..
>>
>>
>>> 	1. connect headset
>>> 	2. disconnect headset
>>> 	3. connect headphone
>>
>>> Following headphone connections/disconnections are reported correctly.
>>> Note that I don't press the media key, it's wrongly detected when I pull
>>> off the cable.
>>
>> If you do step 3 slowly there is a chance that we get KEY_MEDIA events, as
>> headphone is a 3 pole, during hp inserting we would end up shorting the MIC
>> and GND lines on the connector, as 3-pole connectors have longer GND
>> pole/connector length, this action is same as pressing MEDIA KEY.
>>
>> The logic in the driver uses this MEDIA KEY press release as advantage to
>> detect the accessory type before hs insertion, but this logic could break if
>> we are inserting headphone slowly, which is not a normal usecase.
>>
>> Any solution to this issue can be always break if we are doing slow
>> insertion of hs or we endup making the driver complex. I will think about
>> this once again and see how other drivers handle this usecase.
> 
> Understood, and you're right we should keep this driver as simple as
> possible.
> 
> I get the KEY_MEDIA event when doing step 2 (disconnecting headset), and
> I understand why it's doing that (and I think it's okay if we don't
> filter out that spurious event), but we should at least get a KEY_MEDIA
> release event at the same time as SW_{MICROPHONE,HEADPHONE}_INSERT
> removal. It doesn't really matter if I disconnect it fast or slowly,
> sometimes I'll get the KEY_MEDIA, some other times I won't.

I will see if I can fix this in a much better way and let you know.

> 
> Shouldn't mbhc_btn0_pressed be reset to false when physical removal is
> detected? I think that would solve the issue of headphone being detected
> as headset.
> 
Yes, I have considered that change to my next version.

> 
> Regards,
>