[PATCH 0/4] Fix headphone detection on Acer Chromebook 13

[PATCH 0/4] Fix headphone detection on Acer Chromebook 13

[Jonathan Tinkham]

Headphone output works, but headphone detection does not. The problem
is improper inversion of the jack gpio, and improper naming of the
DAPM switch. With these changes, headphone insertion and removal is
picked up by userspace, and the switch properly toggled.

Jonathan Tinkham (4):
  soc/sound: tegra_max98090: do not invert headphone jack
  sound/soc: tegra_max98090: rename headphone jack DAPM
  sound/soc: tegra_max98090: document headphone jack rename
  soc/sound: tegra_max98090: update dts affected by rename

 .../devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt  |  6 +++---
 arch/arm/boot/dts/tegra124-nyan.dtsi                           |  4 ++--
 arch/arm/boot/dts/tegra124-venice2.dts                         |  4 ++--
 sound/soc/tegra/tegra_max98090.c                               | 10 +++++-----
 4 files changed, 12 insertions(+), 12 deletions(-)

-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jonathan Tinkham]

The headphone jack should not be inverted

Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 sound/soc/tegra/tegra_max98090.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c
index 902da36..b373d06 100644
--- a/sound/soc/tegra/tegra_max98090.c
+++ b/sound/soc/tegra/tegra_max98090.c
@@ -110,7 +110,7 @@ static struct snd_soc_jack_gpio tegra_max98090_hp_jack_gpio = {
 	.name = "Headphone detection",
 	.report = SND_JACK_HEADPHONE,
 	.debounce_time = 150,
-	.invert = 1,
+	.invert = 0,
 };
 
 static struct snd_soc_jack tegra_max98090_mic_jack;
-- 
2.7.0

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

[Stephen Warren]

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jonathan Tinkham]

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

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

[Dylan Reid]

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?

>
> 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
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Warren]

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.

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

[Jonathan Tinkham]

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?

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

[Stephen Warren]

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".

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

On Thu, Feb 04, 2016 at 12:05:50PM -0700, Jonathan Tinkham wrote:

> 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).

That's *not* ABI documentation, that's a random example of some kernel
internal strings.

> 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.

A property would be nicer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Stephen Warren]

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jonathan Tinkham]

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"?

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

[Stephen Warren]

On 02/12/2016 07:10 PM, Jonathan Tinkham wrote:
> 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"?

The default (if no property is present in the DT) should indeed match 
whatever the current behaviour is in order to maintain compatibility.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] Fix headphone detection on Acer Chromebook 13

[Jonathan Tinkham]

Add a new DT property to disable inversion of headphone detect GPIO.
This fixes the headphone detection on the CB5-311.

Jonathan Tinkham (3):
  dt/bindings: Add 'nvidia,hd-no-invert' option
  sound/soc/tegra: tegra_max98090: add 'nvidia,hd-no-invert' option
  ARM: tegra: Do not invert nyan headphone detection signal

 .../devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt        | 1 +
 arch/arm/boot/dts/tegra124-nyan.dtsi                                 | 1 +
 sound/soc/tegra/tegra_max98090.c                                     | 5 +++++
 3 files changed, 7 insertions(+)

-- 
2.7.1

[PATCH 1/3] dt/bindings: Add 'nvidia,hd-no-invert' option

[Jonathan Tinkham]

Document an option to not invert the headphone detect GPIO signal

Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt b/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt
index c3495be..5dac704 100644
--- a/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt
+++ b/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt
@@ -26,6 +26,7 @@ Required properties:
 
 Optional properties:
 - nvidia,hp-det-gpios : The GPIO that detect headphones are plugged in
+- nvidia,hd-no-invert : Do not invert headphone detect signal
 - nvidia,mic-det-gpios : The GPIO that detect microphones are plugged in
 
 Example:
-- 
2.7.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] dt/bindings: Add 'nvidia,hd-no-invert' option

[Stephen Warren]

On 02/21/2016 11:24 PM, Jonathan Tinkham wrote:
> Document an option to not invert the headphone detect GPIO signal
>

> diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt b/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt

>   Optional properties:
>   - nvidia,hp-det-gpios : The GPIO that detect headphones are plugged in
> +- nvidia,hd-no-invert : Do not invert headphone detect signal

We should use "hp" not "hd" to be consistent with other properties.

Why can't we use the GPIO specifier's ACTIVE_LOW/ACTIVE_HIGH flag to 
represent the polarity of this signal?

Re: [PATCH 1/3] dt/bindings: Add 'nvidia,hd-no-invert' option

[Jonathan Tinkham]

Oh! I see now. That should actually work, will test quickly and see.

On 02/22/2016 12:48 PM, Stephen Warren wrote:
> On 02/21/2016 11:24 PM, Jonathan Tinkham wrote:
>> Document an option to not invert the headphone detect GPIO signal
>>
>
>> diff --git 
>> a/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt 
>> b/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt 
>>
>
>>   Optional properties:
>>   - nvidia,hp-det-gpios : The GPIO that detect headphones are plugged in
>> +- nvidia,hd-no-invert : Do not invert headphone detect signal
>
> We should use "hp" not "hd" to be consistent with other properties.
>
> Why can't we use the GPIO specifier's ACTIVE_LOW/ACTIVE_HIGH flag to 
> represent the polarity of this signal?

[PATCH] sound/soc/tegra: tegra_max98090: fix hp detect on Chromebook 13

[Jonathan Tinkham]

It appears the driver ignores the GPIO flag for the headphone detect pin. By
using this, in a manner similar to tegra_rt5640, the invert flag can be set
appropriately without changing the DTs.

Jonathan Tinkham (1):
  sound/soc/tegra: tegra_max98090: Invert headphone by GPIO flag

 sound/soc/tegra/tegra_max98090.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.7.1

[PATCH] sound/soc/tegra: tegra_max98090: Invert headphone by GPIO flag

[Jonathan Tinkham]

Set the invert property for the headphone jack depending on the GPIO flag in the
device-tree. This is similar to what is done for tegra_rt5640.

Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 sound/soc/tegra/tegra_max98090.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c
index 902da36..49b95c7 100644
--- a/sound/soc/tegra/tegra_max98090.c
+++ b/sound/soc/tegra/tegra_max98090.c
@@ -42,6 +42,7 @@
 struct tegra_max98090 {
 	struct tegra_asoc_utils_data util_data;
 	int gpio_hp_det;
+	enum of_gpio_flags gpio_hp_det_flags;
 	int gpio_mic_det;
 };
 
@@ -155,6 +156,7 @@ static int tegra_max98090_asoc_init(struct snd_soc_pcm_runtime *rtd)
 				      ARRAY_SIZE(tegra_max98090_hp_jack_pins));
 
 		tegra_max98090_hp_jack_gpio.gpio = machine->gpio_hp_det;
+		tegra_max98090_hp_jack_gpio.invert = (machine->gpio_hp_det_flags & OF_GPIO_ACTIVE_LOW);
 		snd_soc_jack_add_gpios(&tegra_max98090_hp_jack,
 					1,
 					&tegra_max98090_hp_jack_gpio);
@@ -234,7 +236,8 @@ static int tegra_max98090_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, card);
 	snd_soc_card_set_drvdata(card, machine);
 
-	machine->gpio_hp_det = of_get_named_gpio(np, "nvidia,hp-det-gpios", 0);
+	machine->gpio_hp_det = of_get_named_gpio_flags(
+                np, "nvidia,hp-det-gpios", 0, &machine->gpio_hp_det_flags);
 	if (machine->gpio_hp_det == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
-- 
2.7.1

Re: [PATCH] sound/soc/tegra: tegra_max98090: Invert headphone by GPIO flag

[Stephen Warren]

On 02/23/2016 10:19 AM, Jonathan Tinkham wrote:

 > Subject: sound/soc/tegra: tegra_max98090: Invert headphone by GPIO
 > flag

Mark has mentioned quite a few times that this patch subject is 
incorrect. ASoC patch subjects should start with "ASoC: ". You can see 
this by running:

git log -- sound/soc/tegra/

I'd suggest the following:

AsoC: tegra_max98090: honor headphone detect GPIO polarity

> Set the invert property for the headphone jack depending on the GPIO flag in the
> device-tree. This is similar to what is done for tegra_rt5640.
>

> diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c

> @@ -155,6 +156,7 @@ static int tegra_max98090_asoc_init(struct snd_soc_pcm_runtime *rtd)
>   				      ARRAY_SIZE(tegra_max98090_hp_jack_pins));
>
>   		tegra_max98090_hp_jack_gpio.gpio = machine->gpio_hp_det;
> +		tegra_max98090_hp_jack_gpio.invert = (machine->gpio_hp_det_flags & OF_GPIO_ACTIVE_LOW);

Did you run checkpatch? It should complain about > 80 columns, and I 
suspect about the unnecessary brackets around that expression. In fact, 
checkpatch indicates quite a few other warnings and errors.

The logic in this patch looks OK. Do the relevant DT files all have the 
correct GPIO flags already? That'd be nice!

Re: [PATCH] sound/soc/tegra: tegra_max98090: Invert headphone by GPIO flag

[Jonathan Tinkham]

On 02/23/2016 10:35 AM, Stephen Warren wrote:
> On 02/23/2016 10:19 AM, Jonathan Tinkham wrote:
>
> > Subject: sound/soc/tegra: tegra_max98090: Invert headphone by GPIO
> > flag
>
> Mark has mentioned quite a few times that this patch subject is 
> incorrect. ASoC patch subjects should start with "ASoC: ". You can see 
> this by running:
>
> git log -- sound/soc/tegra/
>
> I'd suggest the following:
>
> AsoC: tegra_max98090: honor headphone detect GPIO polarity
>
My apologies, I finally grok what he was saying. Thank you.
>> Set the invert property for the headphone jack depending on the GPIO 
>> flag in the
>> device-tree. This is similar to what is done for tegra_rt5640.
>>
>
>> diff --git a/sound/soc/tegra/tegra_max98090.c 
>> b/sound/soc/tegra/tegra_max98090.c
>
>> @@ -155,6 +156,7 @@ static int tegra_max98090_asoc_init(struct 
>> snd_soc_pcm_runtime *rtd)
>> ARRAY_SIZE(tegra_max98090_hp_jack_pins));
>>
>>           tegra_max98090_hp_jack_gpio.gpio = machine->gpio_hp_det;
>> +        tegra_max98090_hp_jack_gpio.invert = 
>> (machine->gpio_hp_det_flags & OF_GPIO_ACTIVE_LOW);
>
> Did you run checkpatch? It should complain about > 80 columns, and I 
> suspect about the unnecessary brackets around that expression. In 
> fact, checkpatch indicates quite a few other warnings and errors.
>
> The logic in this patch looks OK. Do the relevant DT files all have 
> the correct GPIO flags already? That'd be nice!

The only board is the venice2, which doesn't have a 'nvidia,hp-det-gpio' 
entry at all (how did this even work before?)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sound/soc/tegra: tegra_max98090: Invert headphone by GPIO flag

[Stephen Warren]

On 02/24/2016 09:42 AM, Jonathan Tinkham wrote:
> On 02/23/2016 10:35 AM, Stephen Warren wrote:
>> On 02/23/2016 10:19 AM, Jonathan Tinkham wrote:
>>
>> > Subject: sound/soc/tegra: tegra_max98090: Invert headphone by GPIO
>> > flag
>>
>> Mark has mentioned quite a few times that this patch subject is
>> incorrect. ASoC patch subjects should start with "ASoC: ". You can see
>> this by running:
>>
>> git log -- sound/soc/tegra/
>>
>> I'd suggest the following:
>>
>> AsoC: tegra_max98090: honor headphone detect GPIO polarity
>>
> My apologies, I finally grok what he was saying. Thank you.
>>> Set the invert property for the headphone jack depending on the GPIO
>>> flag in the
>>> device-tree. This is similar to what is done for tegra_rt5640.
>>>
>>
>>> diff --git a/sound/soc/tegra/tegra_max98090.c
>>> b/sound/soc/tegra/tegra_max98090.c
>>
>>> @@ -155,6 +156,7 @@ static int tegra_max98090_asoc_init(struct
>>> snd_soc_pcm_runtime *rtd)
>>> ARRAY_SIZE(tegra_max98090_hp_jack_pins));
>>>
>>>           tegra_max98090_hp_jack_gpio.gpio = machine->gpio_hp_det;
>>> +        tegra_max98090_hp_jack_gpio.invert =
>>> (machine->gpio_hp_det_flags & OF_GPIO_ACTIVE_LOW);
>>
>> Did you run checkpatch? It should complain about > 80 columns, and I
>> suspect about the unnecessary brackets around that expression. In
>> fact, checkpatch indicates quite a few other warnings and errors.
>>
>> The logic in this patch looks OK. Do the relevant DT files all have
>> the correct GPIO flags already? That'd be nice!
>
> The only board is the venice2, which doesn't have a 'nvidia,hp-det-gpio'
> entry at all (how did this even work before?)

I mean: Is ACTIVE_LOW/ACTIVE_HIGH flag in the existing 
nvidia,hp-det-gpios already correctly set?

Re: [PATCH] sound/soc/tegra: tegra_max98090: Invert headphone by GPIO flag

[Jonathan Tinkham]

On 02/24/2016 09:45 AM, Stephen Warren wrote:
> On 02/24/2016 09:42 AM, Jonathan Tinkham wrote:
>> On 02/23/2016 10:35 AM, Stephen Warren wrote:
>>> On 02/23/2016 10:19 AM, Jonathan Tinkham wrote:
>>>
>>> > Subject: sound/soc/tegra: tegra_max98090: Invert headphone by GPIO
>>> > flag
>>>
>>> Mark has mentioned quite a few times that this patch subject is
>>> incorrect. ASoC patch subjects should start with "ASoC: ". You can see
>>> this by running:
>>>
>>> git log -- sound/soc/tegra/
>>>
>>> I'd suggest the following:
>>>
>>> AsoC: tegra_max98090: honor headphone detect GPIO polarity
>>>
>> My apologies, I finally grok what he was saying. Thank you.
>>>> Set the invert property for the headphone jack depending on the GPIO
>>>> flag in the
>>>> device-tree. This is similar to what is done for tegra_rt5640.
>>>>
>>>
>>>> diff --git a/sound/soc/tegra/tegra_max98090.c
>>>> b/sound/soc/tegra/tegra_max98090.c
>>>
>>>> @@ -155,6 +156,7 @@ static int tegra_max98090_asoc_init(struct
>>>> snd_soc_pcm_runtime *rtd)
>>>> ARRAY_SIZE(tegra_max98090_hp_jack_pins));
>>>>
>>>>           tegra_max98090_hp_jack_gpio.gpio = machine->gpio_hp_det;
>>>> +        tegra_max98090_hp_jack_gpio.invert =
>>>> (machine->gpio_hp_det_flags & OF_GPIO_ACTIVE_LOW);
>>>
>>> Did you run checkpatch? It should complain about > 80 columns, and I
>>> suspect about the unnecessary brackets around that expression. In
>>> fact, checkpatch indicates quite a few other warnings and errors.
>>>
>>> The logic in this patch looks OK. Do the relevant DT files all have
>>> the correct GPIO flags already? That'd be nice!
>>
>> The only board is the venice2, which doesn't have a 'nvidia,hp-det-gpio'
>> entry at all (how did this even work before?)
>
> I mean: Is ACTIVE_LOW/ACTIVE_HIGH flag in the existing 
> nvidia,hp-det-gpios already correctly set?

Yes. I also have cleaned up the warnings/errors from checkpatch, and can 
resubmit if acceptable.

[PATCH] AsoC: tegra_max98090: honor headphone detect GPIO polarity

[Jonathan Tinkham]

Set the invert property for the headphone jack depending on the GPIO polarity
in the device-tree.

Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 sound/soc/tegra/tegra_max98090.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c
index 902da36..da4e8d1 100644
--- a/sound/soc/tegra/tegra_max98090.c
+++ b/sound/soc/tegra/tegra_max98090.c
@@ -42,6 +42,7 @@
 struct tegra_max98090 {
 	struct tegra_asoc_utils_data util_data;
 	int gpio_hp_det;
+	enum of_gpio_flags gpio_hp_det_flags;
 	int gpio_mic_det;
 };
 
@@ -155,6 +156,8 @@ static int tegra_max98090_asoc_init(struct snd_soc_pcm_runtime *rtd)
 				      ARRAY_SIZE(tegra_max98090_hp_jack_pins));
 
 		tegra_max98090_hp_jack_gpio.gpio = machine->gpio_hp_det;
+		tegra_max98090_hp_jack_gpio.invert =
+			machine->gpio_hp_det_flags & OF_GPIO_ACTIVE_LOW;
 		snd_soc_jack_add_gpios(&tegra_max98090_hp_jack,
 					1,
 					&tegra_max98090_hp_jack_gpio);
@@ -234,7 +237,8 @@ static int tegra_max98090_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, card);
 	snd_soc_card_set_drvdata(card, machine);
 
-	machine->gpio_hp_det = of_get_named_gpio(np, "nvidia,hp-det-gpios", 0);
+	machine->gpio_hp_det = of_get_named_gpio_flags(
+		np, "nvidia,hp-det-gpios", 0, &machine->gpio_hp_det_flags);
 	if (machine->gpio_hp_det == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
-- 
2.7.2

Re: [PATCH] AsoC: tegra_max98090: honor headphone detect GPIO polarity

[Stephen Warren]

On 03/02/2016 10:43 AM, Jonathan Tinkham wrote:
> Set the invert property for the headphone jack depending on the GPIO polarity
> in the device-tree.
>
> Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---

I think this is an updatd version of a previous patch, so it should say 
"[PATCH V2]" in the subject, and have a description of the changes you 
made between the versions here, below the --- line.

> diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c
> index 902da36..da4e8d1 100644
> --- a/sound/soc/tegra/tegra_max98090.c
> +++ b/sound/soc/tegra/tegra_max98090.c
> @@ -42,6 +42,7 @@
>   struct tegra_max98090 {
>   	struct tegra_asoc_utils_data util_data;
>   	int gpio_hp_det;
> +	enum of_gpio_flags gpio_hp_det_flags;
>   	int gpio_mic_det;
>   };
>
> @@ -155,6 +156,8 @@ static int tegra_max98090_asoc_init(struct snd_soc_pcm_runtime *rtd)
>   				      ARRAY_SIZE(tegra_max98090_hp_jack_pins));
>
>   		tegra_max98090_hp_jack_gpio.gpio = machine->gpio_hp_det;
> +		tegra_max98090_hp_jack_gpio.invert =
> +			machine->gpio_hp_det_flags & OF_GPIO_ACTIVE_LOW;

So now, this .invert flag is always set directly based on the GPIO 
flags. That's good.

I believe this means you should remove the .invert assignment from 
static struct snd_soc_jack_gpio tegra_max98090_hp_jack_gpio too, since 
it's always over-written, so doesn't need a default value.

Question: Should we make the same change for DT property 
nvidia,mic-det-gpios and value tegra_max98090_mic_jack_gpio.invert? 
tegra124-nyan.dts currently says GPIO_ACTIVE_HIGH for both hp-det-gpios 
and mic-det-gpios. I don't know if the DT is correct for both of those 
(and hence making the same change for the mic-det-gpios would break 
Nyan) or not.

Re: [PATCH] AsoC: tegra_max98090: honor headphone detect GPIO polarity

[Jonathan Tinkham]

On 03/03/2016 11:37 AM, Stephen Warren wrote:
> On 03/02/2016 10:43 AM, Jonathan Tinkham wrote:
>> Set the invert property for the headphone jack depending on the GPIO 
>> polarity
>> in the device-tree.
>>
>> Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>
> I think this is an updatd version of a previous patch, so it should 
> say "[PATCH V2]" in the subject, and have a description of the changes 
> you made between the versions here, below the --- line.

Ah yes, my apologies. For the next version should I increment that to v3 
or v2 then?
>
>> diff --git a/sound/soc/tegra/tegra_max98090.c 
>> b/sound/soc/tegra/tegra_max98090.c
>> index 902da36..da4e8d1 100644
>> --- a/sound/soc/tegra/tegra_max98090.c
>> +++ b/sound/soc/tegra/tegra_max98090.c
>> @@ -42,6 +42,7 @@
>>   struct tegra_max98090 {
>>       struct tegra_asoc_utils_data util_data;
>>       int gpio_hp_det;
>> +    enum of_gpio_flags gpio_hp_det_flags;
>>       int gpio_mic_det;
>>   };
>>
>> @@ -155,6 +156,8 @@ static int tegra_max98090_asoc_init(struct 
>> snd_soc_pcm_runtime *rtd)
>> ARRAY_SIZE(tegra_max98090_hp_jack_pins));
>>
>>           tegra_max98090_hp_jack_gpio.gpio = machine->gpio_hp_det;
>> +        tegra_max98090_hp_jack_gpio.invert =
>> +            machine->gpio_hp_det_flags & OF_GPIO_ACTIVE_LOW;
>
> So now, this .invert flag is always set directly based on the GPIO 
> flags. That's good.
>
> I believe this means you should remove the .invert assignment from 
> static struct snd_soc_jack_gpio tegra_max98090_hp_jack_gpio too, since 
> it's always over-written, so doesn't need a default value.
>
> Question: Should we make the same change for DT property 
> nvidia,mic-det-gpios and value tegra_max98090_mic_jack_gpio.invert? 
> tegra124-nyan.dts currently says GPIO_ACTIVE_HIGH for both 
> hp-det-gpios and mic-det-gpios. I don't know if the DT is correct for 
> both of those (and hence making the same change for the mic-det-gpios 
> would break Nyan) or not.
Hmmm, it is indeed the same logic. I will implement and test on nyan.

Re: [PATCH] AsoC: tegra_max98090: honor headphone detect GPIO polarity

[Stephen Warren]

On 03/04/2016 10:42 AM, Jonathan Tinkham wrote:
>
>
> On 03/03/2016 11:37 AM, Stephen Warren wrote:
>> On 03/02/2016 10:43 AM, Jonathan Tinkham wrote:
>>> Set the invert property for the headphone jack depending on the GPIO
>>> polarity
>>> in the device-tree.
>>>
>>> Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>
>> I think this is an updatd version of a previous patch, so it should
>> say "[PATCH V2]" in the subject, and have a description of the changes
>> you made between the versions here, below the --- line.
>
> Ah yes, my apologies. For the next version should I increment that to v3
> or v2 then?

May as well make it V3 since that's what it is:-)

Re: [PATCH] sound/soc/tegra: tegra_max98090: fix hp detect on Chromebook 13

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

On Tue, Feb 23, 2016 at 10:19:44AM -0700, Jonathan Tinkham wrote:

> Jonathan Tinkham (1):
>   sound/soc/tegra: tegra_max98090: Invert headphone by GPIO flag

I have *repeatedly* reminded you to use subject lines reflecting the
style for the subsystem and you continue to ignore me.  Please don't
ignore review comments, people are generally making them for a reason
and are likely to have the same concerns if issues remain unaddressed.
Having to repeat the same comments can get repetitive and make people
question the value of time spent reviewing.  If you disagree with the
concerns so that the reviewer can understand your decisions.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] sound/soc/tegra: tegra_max98090: fix hp detect on Chromebook 13

[Jonathan Tinkham]

On 02/23/2016 08:25 PM, Mark Brown wrote:
> On Tue, Feb 23, 2016 at 10:19:44AM -0700, Jonathan Tinkham wrote:
>
>> Jonathan Tinkham (1):
>>    sound/soc/tegra: tegra_max98090: Invert headphone by GPIO flag
> I have *repeatedly* reminded you to use subject lines reflecting the
> style for the subsystem and you continue to ignore me.  Please don't
> ignore review comments, people are generally making them for a reason
> and are likely to have the same concerns if issues remain unaddressed.
> Having to repeat the same comments can get repetitive and make people
> question the value of time spent reviewing.  If you disagree with the
> concerns so that the reviewer can understand your decisions.
Yes, my apologies. I continually misunderstood your comments

[PATCH 2/3] sound/soc/tegra: tegra_max98090: add 'nvidia,hd-no-invert' option

[Jonathan Tinkham]

Add a device-tree property that does not invert the headphone
detect signal

Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 sound/soc/tegra/tegra_max98090.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c
index 902da36..73beba6 100644
--- a/sound/soc/tegra/tegra_max98090.c
+++ b/sound/soc/tegra/tegra_max98090.c
@@ -238,6 +238,11 @@ static int tegra_max98090_probe(struct platform_device *pdev)
 	if (machine->gpio_hp_det == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
+	if (of_property_read_bool(np, "nvidia,hd-no-invert"))
+		tegra_max98090_hp_jack_gpio.invert = 0;
+	else
+		tegra_max98090_hp_jack_gpio.invert = 1;
+
 	machine->gpio_mic_det =
 			of_get_named_gpio(np, "nvidia,mic-det-gpios", 0);
 	if (machine->gpio_mic_det == -EPROBE_DEFER)
-- 
2.7.1

[PATCH 3/3] ARM: tegra: Do not invert nyan headphone detection signal

[Jonathan Tinkham]

Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/tegra124-nyan.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/tegra124-nyan.dtsi b/arch/arm/boot/dts/tegra124-nyan.dtsi
index ec1aa64..1e6e547 100644
--- a/arch/arm/boot/dts/tegra124-nyan.dtsi
+++ b/arch/arm/boot/dts/tegra124-nyan.dtsi
@@ -694,6 +694,7 @@
 		clock-names = "pll_a", "pll_a_out0", "mclk";
 
 		nvidia,hp-det-gpios = <&gpio TEGRA_GPIO(I, 7) GPIO_ACTIVE_HIGH>;
+		nvidia,hd-no-invert;
 		nvidia,mic-det-gpios =
 				<&gpio TEGRA_GPIO(R, 7) GPIO_ACTIVE_HIGH>;
 	};
-- 
2.7.1

Re: [PATCH 0/3] Fix headphone detection on Acer Chromebook 13

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

On Sun, Feb 21, 2016 at 11:24:10PM -0700, Jonathan Tinkham wrote:
> Add a new DT property to disable inversion of headphone detect GPIO.
> This fixes the headphone detection on the CB5-311.
> 
> Jonathan Tinkham (3):
>   dt/bindings: Add 'nvidia,hd-no-invert' option
>   sound/soc/tegra: tegra_max98090: add 'nvidia,hd-no-invert' option
>   ARM: tegra: Do not invert nyan headphone detection signal

As I said in reply to your earlier series:

| Please try to use subject lines reflecting the style for the subsystem -
| this makes it easier for people to spot relevant content.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 2/4] sound/soc: tegra_max98090: rename headphone jack DAPM

[Jonathan Tinkham]

The correct DAPM name for a headphone jack is "Headphone Jack"
This is also what userspace expects, and this fixes headphone detection.

Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 sound/soc/tegra/tegra_max98090.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/tegra/tegra_max98090.c b/sound/soc/tegra/tegra_max98090.c
index b373d06..a015d1b 100644
--- a/sound/soc/tegra/tegra_max98090.c
+++ b/sound/soc/tegra/tegra_max98090.c
@@ -101,7 +101,7 @@ static struct snd_soc_jack tegra_max98090_hp_jack;
 
 static struct snd_soc_jack_pin tegra_max98090_hp_jack_pins[] = {
 	{
-		.pin = "Headphones",
+		.pin = "Headphone Jack",
 		.mask = SND_JACK_HEADPHONE,
 	},
 };
@@ -130,14 +130,14 @@ static struct snd_soc_jack_gpio tegra_max98090_mic_jack_gpio = {
 };
 
 static const struct snd_soc_dapm_widget tegra_max98090_dapm_widgets[] = {
-	SND_SOC_DAPM_HP("Headphones", NULL),
+	SND_SOC_DAPM_HP("Headphone Jack", NULL),
 	SND_SOC_DAPM_SPK("Speakers", NULL),
 	SND_SOC_DAPM_MIC("Mic Jack", NULL),
 	SND_SOC_DAPM_MIC("Int Mic", NULL),
 };
 
 static const struct snd_kcontrol_new tegra_max98090_controls[] = {
-	SOC_DAPM_PIN_SWITCH("Headphones"),
+	SOC_DAPM_PIN_SWITCH("Headphone Jack"),
 	SOC_DAPM_PIN_SWITCH("Speakers"),
 	SOC_DAPM_PIN_SWITCH("Mic Jack"),
 	SOC_DAPM_PIN_SWITCH("Int Mic"),
@@ -148,7 +148,7 @@ static int tegra_max98090_asoc_init(struct snd_soc_pcm_runtime *rtd)
 	struct tegra_max98090 *machine = snd_soc_card_get_drvdata(rtd->card);
 
 	if (gpio_is_valid(machine->gpio_hp_det)) {
-		snd_soc_card_jack_new(rtd->card, "Headphones",
+		snd_soc_card_jack_new(rtd->card, "Headphone Jack",
 				      SND_JACK_HEADPHONE,
 				      &tegra_max98090_hp_jack,
 				      tegra_max98090_hp_jack_pins,
-- 
2.7.0

Re: [PATCH 2/4] sound/soc: tegra_max98090: rename headphone jack DAPM

[Stephen Warren]

On 02/03/2016 10:31 PM, Jonathan Tinkham wrote:
> The correct DAPM name for a headphone jack is "Headphone Jack"
> This is also what userspace expects, and this fixes headphone detection.

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?

[PATCH 3/4] sound/soc: tegra_max98090: document headphone jack rename

[Jonathan Tinkham]

Update device-tree bindings to reflect the rename of the board's
headphone jack.

Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt       | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt b/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt
index c3495be..7ad911f 100644
--- a/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt
+++ b/Documentation/devicetree/bindings/sound/nvidia,tegra-audio-max98090.txt
@@ -15,7 +15,7 @@ Required properties:
   sinks are the MAX98090's pins (as documented in its binding), and the jacks
   on the board:
 
-  * Headphones
+  * Headphone Jack
   * Speakers
   * Mic Jack
   * Int Mic
@@ -36,8 +36,8 @@ sound {
 	nvidia,model = "NVIDIA Tegra Venice2";
 
 	nvidia,audio-routing =
-		"Headphones", "HPR",
-		"Headphones", "HPL",
+		"Headphone Jack", "HPR",
+		"Headphone Jack", "HPL",
 		"Speakers", "SPKR",
 		"Speakers", "SPKL",
 		"Mic Jack", "MICBIAS",
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] sound/soc: tegra_max98090: document headphone jack rename

[Stephen Warren]

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.

Re: [PATCH 3/4] sound/soc: tegra_max98090: document headphone jack rename

[Dylan Reid]

On Thu, Feb 4, 2016 at 8:39 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 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.
>

These values are also present in the UCM config for the boards present
in alsa-lib/conf/ucm/GoogleNyan

It's better not to rely on drivers using a "standard" name, better to
let the UCM file abstract it away.

[PATCH 4/4] soc/sound: tegra_max98090: update dts affected by rename

[Jonathan Tinkham]

Update the DTs that are affected by the DAPM headphone jack rename

Signed-off-by: Jonathan Tinkham <sctincman-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/tegra124-nyan.dtsi   | 4 ++--
 arch/arm/boot/dts/tegra124-venice2.dts | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/tegra124-nyan.dtsi b/arch/arm/boot/dts/tegra124-nyan.dtsi
index ec1aa64..000b1f6 100644
--- a/arch/arm/boot/dts/tegra124-nyan.dtsi
+++ b/arch/arm/boot/dts/tegra124-nyan.dtsi
@@ -676,8 +676,8 @@
 
 	sound {
 		nvidia,audio-routing =
-			"Headphones", "HPR",
-			"Headphones", "HPL",
+			"Headphone Jack", "HPR",
+			"Headphone Jack", "HPL",
 			"Speakers", "SPKR",
 			"Speakers", "SPKL",
 			"Mic Jack", "MICBIAS",
diff --git a/arch/arm/boot/dts/tegra124-venice2.dts b/arch/arm/boot/dts/tegra124-venice2.dts
index cfbdf42..c535d55 100644
--- a/arch/arm/boot/dts/tegra124-venice2.dts
+++ b/arch/arm/boot/dts/tegra124-venice2.dts
@@ -1147,8 +1147,8 @@
 		nvidia,model = "NVIDIA Tegra Venice2";
 
 		nvidia,audio-routing =
-			"Headphones", "HPR",
-			"Headphones", "HPL",
+			"Headphone Jack", "HPR",
+			"Headphone Jack", "HPL",
 			"Speakers", "SPKR",
 			"Speakers", "SPKL",
 			"Mic Jack", "MICBIAS",
-- 
2.7.0

Re: [PATCH 0/4] Fix headphone detection on Acer Chromebook 13

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On Wed, Feb 03, 2016 at 10:30:58PM -0700, Jonathan Tinkham wrote:

> Jonathan Tinkham (4):
>   soc/sound: tegra_max98090: do not invert headphone jack
>   sound/soc: tegra_max98090: rename headphone jack DAPM
>   sound/soc: tegra_max98090: document headphone jack rename
>   soc/sound: tegra_max98090: update dts affected by rename

Please try to use subject lines reflecting the style for the subsystem -
this makes it easier for people to spot relevant content.  The above
isn't even internally consistent :(

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]