diff mbox

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

Message ID 1454563862-1971-2-git-send-email-sctincman@gmail.com
State Superseded, archived
Headers show

Commit Message

Jonathan Tinkham Feb. 4, 2016, 5:30 a.m. UTC
The headphone jack should not be inverted

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

Comments

Stephen Warren Feb. 4, 2016, 4:36 p.m. UTC | #1
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 linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Tinkham Feb. 4, 2016, 7:05 p.m. UTC | #2
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


--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dylan Reid Feb. 4, 2016, 7:17 p.m. UTC | #3
On Thu, Feb 4, 2016 at 11:05 AM, Jonathan Tinkham <sctincman@gmail.com> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Feb. 4, 2016, 7:37 p.m. UTC | #4
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.
Stephen Warren Feb. 8, 2016, 6:56 p.m. UTC | #5
On 02/04/2016 12:17 PM, Dylan Reid wrote:
> On Thu, Feb 4, 2016 at 11:05 AM, Jonathan Tinkham <sctincman@gmail.com> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Feb. 8, 2016, 6:59 p.m. UTC | #6
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 linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Tinkham Feb. 13, 2016, 2:10 a.m. UTC | #7
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"?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Tinkham Feb. 13, 2016, 2:12 a.m. UTC | #8
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@gmail.com> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Feb. 16, 2016, 9:41 p.m. UTC | #9
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 linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Feb. 16, 2016, 9:43 p.m. UTC | #10
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@gmail.com> 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".
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Tinkham Feb. 22, 2016, 6:24 a.m. UTC | #11
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(+)
Mark Brown Feb. 22, 2016, 9:46 a.m. UTC | #12
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.
diff mbox

Patch

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;