| Message ID | 1412198720-2326-1-git-send-email-dgreid@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3fe240326cc395c66eda0518b1945ea505afd1fc |
| Headers | show |
On Wed, Oct 01, 2014 at 02:25:20PM -0700, Dylan Reid wrote: > Allow Headphone and Microphone jack detect gpios to be specified in > device tree. This will allow a few systems including rk3288_max98090 > to use simple-card instead of having their own board file. Applied, thanks.
On 10/01/2014 11:25 PM, Dylan Reid wrote: > diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt > index c2e9841..72d94b7 100644 > --- a/Documentation/devicetree/bindings/sound/simple-card.txt > +++ b/Documentation/devicetree/bindings/sound/simple-card.txt > @@ -17,6 +17,10 @@ Optional properties: > source. > - simple-audio-card,mclk-fs : Multiplication factor between stream rate and codec > mclk. > +- simple-audio-card,hp_det_gpio : Reference to GPIO that signals when > + headphones are attached. > +- simple-audio-card,mic_det_gpio : Reference to GPIO that signals when The code has this correct, the "_"s should be "-"s. - Lars
On Thu, Oct 2, 2014 at 9:25 AM, Lars-Peter Clausen <lars@metafoo.de> wrote: > On 10/01/2014 11:25 PM, Dylan Reid wrote: >> >> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt >> b/Documentation/devicetree/bindings/sound/simple-card.txt >> index c2e9841..72d94b7 100644 >> --- a/Documentation/devicetree/bindings/sound/simple-card.txt >> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt >> @@ -17,6 +17,10 @@ Optional properties: >> source. >> - simple-audio-card,mclk-fs : Multiplication factor between >> stream rate and codec >> mclk. >> +- simple-audio-card,hp_det_gpio : Reference to GPIO that >> signals when >> + headphones are attached. >> +- simple-audio-card,mic_det_gpio : Reference to GPIO that signals >> when > > > The code has this correct, the "_"s should be "-"s. Indeed, my fault. Sorry about that. I will post a patch once the first one propagates. > > - Lars >
Hi Mark, Dylan, On Thu, Oct 2, 2014 at 5:53 PM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Oct 01, 2014 at 02:25:20PM -0700, Dylan Reid wrote: >> Allow Headphone and Microphone jack detect gpios to be specified in >> device tree. This will allow a few systems including rk3288_max98090 >> to use simple-card instead of having their own board file. > > Applied, thanks. Unfortunately there's no equivalent code for platform data, and the uninitialized default of 0 for gpio_hp_det and gpio_mic_det doesn't play well with asm-generic's gpio_is_valid(): static inline bool gpio_is_valid(int number) { return number >= 0 && number < ARCH_NR_GPIOS; } Hence on r8a7740/armadillo-legacy, which uses platform devices instead of DT: sh-mobile-hdmi sh-mobile-hdmi: SH Mobile HDMI Audio Codec sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Headphones sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Mic Jack After that the kernel log is spammed ca. 7 times per second with: sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Headphones Reverting commit 3fe240326cc395c66 ("ASoC: simple-card: Add mic and hp detect gpios.") fixes this. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Oct 07, 2014 at 02:32:57PM +0200, Geert Uytterhoeven wrote: > Hence on r8a7740/armadillo-legacy, which uses platform devices instead of DT: > sh-mobile-hdmi sh-mobile-hdmi: SH Mobile HDMI Audio Codec > sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Headphones > sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Mic Jack > After that the kernel log is spammed ca. 7 times per second with: > sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Headphones > Reverting commit 3fe240326cc395c66 ("ASoC: simple-card: Add mic and > hp detect gpios.") fixes this. The fix here is to not allow 0 as a GPIO in the core code (which should've been there already).
(re-added some context, CC Linus, Alexandre, linux-gpio) On Tue, Oct 7, 2014 at 2:38 PM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Oct 07, 2014 at 02:32:57PM +0200, Geert Uytterhoeven wrote: >> Unfortunately there's no equivalent code for platform data, and the >> uninitialized default of 0 for gpio_hp_det and gpio_mic_det doesn't >> play well with asm-generic's gpio_is_valid(): >> >> static inline bool gpio_is_valid(int number) >> { >> return number >= 0 && number < ARCH_NR_GPIOS; >> } >> >> Hence on r8a7740/armadillo-legacy, which uses platform devices instead of DT: > >> sh-mobile-hdmi sh-mobile-hdmi: SH Mobile HDMI Audio Codec >> sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Headphones >> sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Mic Jack > >> After that the kernel log is spammed ca. 7 times per second with: > >> sh-mobile-hdmi sh-mobile-hdmi: ASoC: DAPM unknown pin Headphones > >> Reverting commit 3fe240326cc395c66 ("ASoC: simple-card: Add mic and >> hp detect gpios.") fixes this. > > The fix here is to not allow 0 as a GPIO in the core code (which > should've been there already). Unfortunately it's not there. And it's not as simple as changing the definition of gpio_is_valid() (crash in gpio_get_value()): gpiochip_add: GPIOs 0..211 (r8a7740_pfc) failed to register sh-pfc pfc-r8a7740: failed to init GPIO chip, ignoring... sh-pfc pfc-r8a7740: r8a7740_pfc support registered Unable to handle kernel NULL pointer dereference at virtual address 0000004c Quoting Linus (https://lkml.org/lkml/2014/9/4/464): "Fixing the old global GPIO numberspace API is a waste of time IMO". Hence I've just sent a patch to initialize the GPIO numbers with -ENOENT. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Oct 07, 2014 at 03:10:01PM +0200, Geert Uytterhoeven wrote: > On Tue, Oct 7, 2014 at 2:38 PM, Mark Brown <broonie@kernel.org> wrote: > > The fix here is to not allow 0 as a GPIO in the core code (which > > should've been there already). > Unfortunately it's not there. > And it's not as simple as changing the definition of gpio_is_valid() > (crash in gpio_get_value()): > gpiochip_add: GPIOs 0..211 (r8a7740_pfc) failed to register > sh-pfc pfc-r8a7740: failed to init GPIO chip, ignoring... > sh-pfc pfc-r8a7740: r8a7740_pfc support registered > Unable to handle kernel NULL pointer dereference at virtual address 0000004c Right, obviously it's not going to work if the platform actually uses 0 as a valid GPIO. > Quoting Linus (https://lkml.org/lkml/2014/9/4/464): > "Fixing the old global GPIO numberspace API is a waste of time IMO". > Hence I've just sent a patch to initialize the GPIO numbers with -ENOENT. I do think it's worth renumbering the platforms since it's *relatively* little work per platform compared to completing the gpiod transition.
On Wed, Oct 8, 2014 at 1:36 AM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Oct 07, 2014 at 03:10:01PM +0200, Geert Uytterhoeven wrote: >> On Tue, Oct 7, 2014 at 2:38 PM, Mark Brown <broonie@kernel.org> wrote: > >> > The fix here is to not allow 0 as a GPIO in the core code (which >> > should've been there already). > >> Unfortunately it's not there. > >> And it's not as simple as changing the definition of gpio_is_valid() >> (crash in gpio_get_value()): > >> gpiochip_add: GPIOs 0..211 (r8a7740_pfc) failed to register >> sh-pfc pfc-r8a7740: failed to init GPIO chip, ignoring... >> sh-pfc pfc-r8a7740: r8a7740_pfc support registered >> Unable to handle kernel NULL pointer dereference at virtual address 0000004c > > Right, obviously it's not going to work if the platform actually uses 0 > as a valid GPIO. > >> Quoting Linus (https://lkml.org/lkml/2014/9/4/464): >> "Fixing the old global GPIO numberspace API is a waste of time IMO". > >> Hence I've just sent a patch to initialize the GPIO numbers with -ENOENT. > > I do think it's worth renumbering the platforms since it's *relatively* > little work per platform compared to completing the gpiod transition. But transition to gpiod is the way to ultimately fix this issue, as well as many others. Not to mention that renumbering GPIOs will certainly make a few users of the GPIO sysfs (another abomination, agreed) unhappy. I can only recommend switching drivers to gpiod when such issues are spotted.
On Wed, Oct 8, 2014 at 9:05 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Wed, Oct 8, 2014 at 1:36 AM, Mark Brown <broonie@kernel.org> wrote: >> On Tue, Oct 07, 2014 at 03:10:01PM +0200, Geert Uytterhoeven wrote: >> Right, obviously it's not going to work if the platform actually uses 0 >> as a valid GPIO. >> >>> Quoting Linus (https://lkml.org/lkml/2014/9/4/464): >>> "Fixing the old global GPIO numberspace API is a waste of time IMO". >> >>> Hence I've just sent a patch to initialize the GPIO numbers with -ENOENT. >> >> I do think it's worth renumbering the platforms since it's *relatively* >> little work per platform compared to completing the gpiod transition. > > But transition to gpiod is the way to ultimately fix this issue, as > well as many others. Not to mention that renumbering GPIOs will > certainly make a few users of the GPIO sysfs (another abomination, > agreed) unhappy. I can only recommend switching drivers to gpiod when > such issues are spotted. Yeah that is another issue ... we end up in catch 22 situations like that, renumber the GPIOs, OK, then we break the ABI. Admittedly that "ABI" is something people break all the time, /sys/*gpioN just isnt what it should be, not stable at all. I'm not against renumbering GPIO if it's minor effort, but if it start to consume hundreds of hours and regressions and what not, that time is better spent focusing on the gpiod transition. Yours, Linus Walleij
On Wed, Oct 08, 2014 at 10:50:00AM +0200, Linus Walleij wrote: > On Wed, Oct 8, 2014 at 9:05 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > > But transition to gpiod is the way to ultimately fix this issue, as > > well as many others. Not to mention that renumbering GPIOs will > > certainly make a few users of the GPIO sysfs (another abomination, > > agreed) unhappy. I can only recommend switching drivers to gpiod when > > such issues are spotted. > Yeah that is another issue ... we end up in catch 22 situations like > that, renumber the GPIOs, OK, then we break the ABI. > Admittedly that "ABI" is something people break all the time, > /sys/*gpioN just isnt what it should be, not stable at all. > I'm not against renumbering GPIO if it's minor effort, but if it > start to consume hundreds of hours and regressions and what not, > that time is better spent focusing on the gpiod transition. My guess is that it's relatively little work for most platforms - with the systems I've done enough work on to notice everything is keyed off a few defines in the header file. Things tend to be worse in out of tree code but mainline's generally been pretty good.
? 10/02/2014 05:25 AM, Dylan Reid ??: > Allow Headphone and Microphone jack detect gpios to be specified in > device tree. This will allow a few systems including rk3288_max98090 > to use simple-card instead of having their own board file. > > Signed-off-by: Dylan Reid <dgreid@chromium.org> > --- > .../devicetree/bindings/sound/simple-card.txt | 4 ++ > sound/soc/generic/simple-card.c | 73 ++++++++++++++++++++++ > 2 files changed, 77 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt > index c2e9841..72d94b7 100644 > --- a/Documentation/devicetree/bindings/sound/simple-card.txt > +++ b/Documentation/devicetree/bindings/sound/simple-card.txt > @@ -17,6 +17,10 @@ Optional properties: > source. > - simple-audio-card,mclk-fs : Multiplication factor between stream rate and codec > mclk. > +- simple-audio-card,hp_det_gpio : Reference to GPIO that signals when > + headphones are attached. > +- simple-audio-card,mic_det_gpio : Reference to GPIO that signals when > + a microphone is attached. > > Optional subnodes: > > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c > index 709ce67..fcb431f 100644 > --- a/sound/soc/generic/simple-card.c > +++ b/sound/soc/generic/simple-card.c > @@ -10,10 +10,13 @@ > */ > #include <linux/clk.h> > #include <linux/device.h> > +#include <linux/gpio.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_gpio.h> > #include <linux/platform_device.h> > #include <linux/string.h> > +#include <sound/jack.h> > #include <sound/simple_card.h> > #include <sound/soc-dai.h> > #include <sound/soc.h> > @@ -25,6 +28,8 @@ struct simple_card_data { > struct asoc_simple_dai codec_dai; > } *dai_props; > unsigned int mclk_fs; > + int gpio_hp_det; > + int gpio_mic_det; > struct snd_soc_dai_link dai_link[]; /* dynamically allocated */ > }; > > @@ -54,6 +59,32 @@ static struct snd_soc_ops asoc_simple_card_ops = { > .hw_params = asoc_simple_card_hw_params, > }; > > +static struct snd_soc_jack simple_card_hp_jack; > +static struct snd_soc_jack_pin simple_card_hp_jack_pins[] = { > + { > + .pin = "Headphones", > + .mask = SND_JACK_HEADPHONE, > + }, > +}; > +static struct snd_soc_jack_gpio simple_card_hp_jack_gpio = { > + .name = "Headphone detection", > + .report = SND_JACK_HEADPHONE, > + .debounce_time = 150, I think some board needs "invert" trigger level, i.e need to add "invert" property in dt ? Pinky board do need it. > +}; > + > +static struct snd_soc_jack simple_card_mic_jack; > +static struct snd_soc_jack_pin simple_card_mic_jack_pins[] = { > + { > + .pin = "Mic Jack", > + .mask = SND_JACK_MICROPHONE, > + }, > +}; > +static struct snd_soc_jack_gpio simple_card_mic_jack_gpio = { > + .name = "Mic detection", > + .report = SND_JACK_MICROPHONE, > + .debounce_time = 150, > +}; > + > static int __asoc_simple_card_dai_init(struct snd_soc_dai *dai, > struct asoc_simple_dai *set) > { > @@ -109,6 +140,28 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd) > if (ret < 0) > return ret; > > + if (gpio_is_valid(priv->gpio_hp_det)) { > + snd_soc_jack_new(codec->codec, "Headphones", SND_JACK_HEADPHONE, > + &simple_card_hp_jack); > + snd_soc_jack_add_pins(&simple_card_hp_jack, > + ARRAY_SIZE(simple_card_hp_jack_pins), > + simple_card_hp_jack_pins); > + > + simple_card_hp_jack_gpio.gpio = priv->gpio_hp_det; > + snd_soc_jack_add_gpios(&simple_card_hp_jack, 1, > + &simple_card_hp_jack_gpio); > + } > + > + if (gpio_is_valid(priv->gpio_mic_det)) { > + snd_soc_jack_new(codec->codec, "Mic Jack", SND_JACK_MICROPHONE, > + &simple_card_mic_jack); > + snd_soc_jack_add_pins(&simple_card_mic_jack, > + ARRAY_SIZE(simple_card_mic_jack_pins), > + simple_card_mic_jack_pins); > + simple_card_mic_jack_gpio.gpio = priv->gpio_mic_det; > + snd_soc_jack_add_gpios(&simple_card_mic_jack, 1, > + &simple_card_mic_jack_gpio); > + } > return 0; > } > > @@ -383,6 +436,16 @@ static int asoc_simple_card_parse_of(struct device_node *node, > return ret; > } > > + priv->gpio_hp_det = of_get_named_gpio(node, > + "simple-audio-card,hp-det-gpio", 0); > + if (priv->gpio_hp_det == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + priv->gpio_mic_det = of_get_named_gpio(node, > + "simple-audio-card,mic-det-gpio", 0); > + if (priv->gpio_mic_det == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > if (!priv->snd_card.name) > priv->snd_card.name = priv->snd_card.dai_link->name; > > @@ -502,6 +565,16 @@ err: > > static int asoc_simple_card_remove(struct platform_device *pdev) > { > + struct snd_soc_card *card = platform_get_drvdata(pdev); > + struct simple_card_data *priv = snd_soc_card_get_drvdata(card); > + > + if (gpio_is_valid(priv->gpio_hp_det)) > + snd_soc_jack_free_gpios(&simple_card_hp_jack, 1, > + &simple_card_hp_jack_gpio); > + if (gpio_is_valid(priv->gpio_mic_det)) > + snd_soc_jack_free_gpios(&simple_card_mic_jack, 1, > + &simple_card_mic_jack_gpio); > + > return asoc_simple_card_unref(pdev); > } >
diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt index c2e9841..72d94b7 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.txt +++ b/Documentation/devicetree/bindings/sound/simple-card.txt @@ -17,6 +17,10 @@ Optional properties: source. - simple-audio-card,mclk-fs : Multiplication factor between stream rate and codec mclk. +- simple-audio-card,hp_det_gpio : Reference to GPIO that signals when + headphones are attached. +- simple-audio-card,mic_det_gpio : Reference to GPIO that signals when + a microphone is attached. Optional subnodes: diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 709ce67..fcb431f 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -10,10 +10,13 @@ */ #include <linux/clk.h> #include <linux/device.h> +#include <linux/gpio.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_gpio.h> #include <linux/platform_device.h> #include <linux/string.h> +#include <sound/jack.h> #include <sound/simple_card.h> #include <sound/soc-dai.h> #include <sound/soc.h> @@ -25,6 +28,8 @@ struct simple_card_data { struct asoc_simple_dai codec_dai; } *dai_props; unsigned int mclk_fs; + int gpio_hp_det; + int gpio_mic_det; struct snd_soc_dai_link dai_link[]; /* dynamically allocated */ }; @@ -54,6 +59,32 @@ static struct snd_soc_ops asoc_simple_card_ops = { .hw_params = asoc_simple_card_hw_params, }; +static struct snd_soc_jack simple_card_hp_jack; +static struct snd_soc_jack_pin simple_card_hp_jack_pins[] = { + { + .pin = "Headphones", + .mask = SND_JACK_HEADPHONE, + }, +}; +static struct snd_soc_jack_gpio simple_card_hp_jack_gpio = { + .name = "Headphone detection", + .report = SND_JACK_HEADPHONE, + .debounce_time = 150, +}; + +static struct snd_soc_jack simple_card_mic_jack; +static struct snd_soc_jack_pin simple_card_mic_jack_pins[] = { + { + .pin = "Mic Jack", + .mask = SND_JACK_MICROPHONE, + }, +}; +static struct snd_soc_jack_gpio simple_card_mic_jack_gpio = { + .name = "Mic detection", + .report = SND_JACK_MICROPHONE, + .debounce_time = 150, +}; + static int __asoc_simple_card_dai_init(struct snd_soc_dai *dai, struct asoc_simple_dai *set) { @@ -109,6 +140,28 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd) if (ret < 0) return ret; + if (gpio_is_valid(priv->gpio_hp_det)) { + snd_soc_jack_new(codec->codec, "Headphones", SND_JACK_HEADPHONE, + &simple_card_hp_jack); + snd_soc_jack_add_pins(&simple_card_hp_jack, + ARRAY_SIZE(simple_card_hp_jack_pins), + simple_card_hp_jack_pins); + + simple_card_hp_jack_gpio.gpio = priv->gpio_hp_det; + snd_soc_jack_add_gpios(&simple_card_hp_jack, 1, + &simple_card_hp_jack_gpio); + } + + if (gpio_is_valid(priv->gpio_mic_det)) { + snd_soc_jack_new(codec->codec, "Mic Jack", SND_JACK_MICROPHONE, + &simple_card_mic_jack); + snd_soc_jack_add_pins(&simple_card_mic_jack, + ARRAY_SIZE(simple_card_mic_jack_pins), + simple_card_mic_jack_pins); + simple_card_mic_jack_gpio.gpio = priv->gpio_mic_det; + snd_soc_jack_add_gpios(&simple_card_mic_jack, 1, + &simple_card_mic_jack_gpio); + } return 0; } @@ -383,6 +436,16 @@ static int asoc_simple_card_parse_of(struct device_node *node, return ret; } + priv->gpio_hp_det = of_get_named_gpio(node, + "simple-audio-card,hp-det-gpio", 0); + if (priv->gpio_hp_det == -EPROBE_DEFER) + return -EPROBE_DEFER; + + priv->gpio_mic_det = of_get_named_gpio(node, + "simple-audio-card,mic-det-gpio", 0); + if (priv->gpio_mic_det == -EPROBE_DEFER) + return -EPROBE_DEFER; + if (!priv->snd_card.name) priv->snd_card.name = priv->snd_card.dai_link->name; @@ -502,6 +565,16 @@ err: static int asoc_simple_card_remove(struct platform_device *pdev) { + struct snd_soc_card *card = platform_get_drvdata(pdev); + struct simple_card_data *priv = snd_soc_card_get_drvdata(card); + + if (gpio_is_valid(priv->gpio_hp_det)) + snd_soc_jack_free_gpios(&simple_card_hp_jack, 1, + &simple_card_hp_jack_gpio); + if (gpio_is_valid(priv->gpio_mic_det)) + snd_soc_jack_free_gpios(&simple_card_mic_jack, 1, + &simple_card_mic_jack_gpio); + return asoc_simple_card_unref(pdev); }
Allow Headphone and Microphone jack detect gpios to be specified in device tree. This will allow a few systems including rk3288_max98090 to use simple-card instead of having their own board file. Signed-off-by: Dylan Reid <dgreid@chromium.org> --- .../devicetree/bindings/sound/simple-card.txt | 4 ++ sound/soc/generic/simple-card.c | 73 ++++++++++++++++++++++ 2 files changed, 77 insertions(+)