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

[Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 02/08/2016 11:59 AM, Stephen Warren wrote:
> On 02/04/2016 12:05 PM, Jonathan Tinkham 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.
>>
>> Something like:
>> - "compatible = nvidia,tegra-audio-max98090" implements old inversion
>> behavior and leaves the switch as "Headphones" to avoid breaking 
>> older DTs
>> - "compatible = nvidia,tegra-{venice2,nyan}-audio-max98090" that
>> separates the inversion behavior and also introduces the rename
>
> It'd be preferable to key this off an separate flag rather than the 
> compatible value. That would more directly represent the data in 
> question, and allow any future boards to be added without having to 
> edit the driver to know whether those new boards neded HP DET 
> inversion or not.
>
> However, do note that each of the 3 boards using this binding has a 
> board-specific compatible value present already:
>
> nvidia,tegra-audio-max98090-venice2
> nvidia,tegra-audio-max98090-nyan-blaze
> nvidia,tegra-audio-max98090-nyan-big

Indeed, I noticed those compatible strings after I sent that message.

A property seems the more ideal/desired way to go. However, to avoid 
breaking older DTs, does that mean it must be implemented as assuming 
inversion is default, and set nyan and other boards to "hd-invert = 0"?