Re: [PATCH 1/4] soc/sound: tegra_max98090: do not invert headphone jack

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 02/12/2016 07:12 PM, Jonathan Tinkham wrote:
> On 02/08/2016 11:56 AM, Stephen Warren wrote:
>> On 02/04/2016 12:17 PM, Dylan Reid wrote:
>>> On Thu, Feb 4, 2016 at 11:05 AM, Jonathan Tinkham
>>> <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On 02/04/2016 09:36 AM, Stephen Warren wrote:
>>>>>
>>>>> On 02/03/2016 10:30 PM, Jonathan Tinkham wrote:
>>>>>>
>>>>>> The headphone jack should not be inverted
>>>>>
>>>>>
>>>>> Have you tested this on Venice2 as well as Nyan? I'm pretty sure
>>>>> Venice2
>>>>> was tested when this driver was written, and whoever added Nyan
>>>>> support to
>>>>> the kernel simply assumed it would work. As such, my suspicion is
>>>>> that this
>>>>> series will break Venice2 even as it fixes Nyan.
>>>>
>>>>
>>>> I have not tested this on Venice2, only on Nyan. That seems like a
>>>> plausible
>>>> cause and reasonable suspicion.
>>>>
>>>>> Why doesn't user-space expect what the kernel actually implements? The
>>>>> kernel should be defining the control naming.
>>>>>
>>>>> Which user-space are you using specifically, and which part of it
>>>>> expects
>>>>> particular naming?
>>>>>
>>>>> Perhaps this series needs to be parametrized based on a flag in DT,
>>>>> rather
>>>>> than switching the hard-coded values, so that only Venice2 can be
>>>>> affected?
>>>>
>>>>
>>>> Specifically pulse-audio and alsa under Arch Linux.
>>>>
>>>> I was referencing 'Documentation/sound/alsa/soc/dapm.txt' with
>>>> regards to
>>>> control names. While it is possible to add another entry into the
>>>> user-space
>>>> configuration, I took this documentation as a definition of kernel
>>>> control
>>>> naming schemes, and thought the driver had used a non-standard
>>>> naming scheme
>>>> (or at least, not a consistent one).
>>>>
>>>>> On 02/03/2016 10:31 PM, Jonathan Tinkham wrote:
>>>>>>
>>>>>> Update device-tree bindings to reflect the rename of the board's
>>>>>> headphone jack.
>>>>>
>>>>>
>>>>> This looks like an incompatible change to the DT. While you've
>>>>> fixed the
>>>>> DT, which will fix new installations, old DTs now won't work. This
>>>>> breaks DT
>>>>> ABI. Any DT change needs to be backwards compatible, i.e. the old name
>>>>> should still work and be documented as a legacy value.
>>>>
>>>>
>>>> I see that now. If the inversion behavior differs between venice2
>>>> and nyan,
>>>> then another compatible string would need to be added anyways,
>>>> correct? As
>>>> you mentioned above, this might need to be done anyways for the rename.
>>>
>>> Venice2 and both nyan platforms do have different polarity of HP detect.
>>>
>>> For some boards we have an hd-invert property in DT.
>>>
>>> Would setting hp-det-gpio to active low in the pinmux achieve the
>>> same thing?
>>
>> I don't believe we have a pinmux setting for this.
>>
>> However, there's a GPIO_ACTIVE_HIGH/LOW flag that could be used in DT
>> property nvidia,hp-det-gpios's flags field to indicate the polarity.
>> That should work, but indeed you could use a separate hp-dt-invert
>> property if not.
>
> Yeah, I could find other things that have an "invert" property, but none
> for headphone/mic detect pins.
>
> I'm not sure setting the GPIO_ACTIVE_HIGH/LOW flag would work. Doesn't
> it depend on how the pin is physically wired?

The flag value used has to match the circuit design, yes. However, I 
don't think there's any requirement that the circuit work any particular 
way for the software to rely on using such a flag.

Logically, there's a "headphone present" signal, and SW needs to 
determine the value of that signal. The "present" value could be 
represented by either a low or a high state in the circuit, depending on 
how the circuit is implemented. The ACTIVE_HIGH/LOW flag simply 
indicates which voltage level is associated with the semantic meaning on 
"headphone present".