| Message ID | 1456247985-5563-2-git-send-email-sctincman@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
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! -- 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
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 linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- 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
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. -- 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
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;
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@gmail.com> --- sound/soc/tegra/tegra_max98090.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)