diff mbox series

[18/48] ARM: pxa: hx4700: use gpio descriptors for audio

Message ID 20220419163810.2118169-19-arnd@kernel.org
State New
Headers show
Series ARM: PXA multiplatform support | expand

Commit Message

Arnd Bergmann April 19, 2022, 4:37 p.m. UTC
From: Arnd Bergmann <arnd@arndb.de>

The audio driver should not use a hardwired gpio number
from the header. Change it to use a lookup table.

Cc: Philipp Zabel <philipp.zabel@gmail.com>
Cc: Paul Parsons <lost.distance@yahoo.com>
Acked-by: Mark Brown <broonie@kernel.org>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: alsa-devel@alsa-project.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/mach-pxa/hx4700-pcmcia.c             |  2 +-
 arch/arm/mach-pxa/hx4700.c                    | 16 ++++++++-
 arch/arm/mach-pxa/{include/mach => }/hx4700.h |  2 +-
 sound/soc/pxa/hx4700.c                        | 34 ++++++++-----------
 4 files changed, 31 insertions(+), 23 deletions(-)
 rename arch/arm/mach-pxa/{include/mach => }/hx4700.h (99%)

Comments

Linus Walleij May 1, 2022, 9:41 p.m. UTC | #1
On Tue, Apr 19, 2022 at 6:41 PM Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
>
> The audio driver should not use a hardwired gpio number
> from the header. Change it to use a lookup table.
>
> Cc: Philipp Zabel <philipp.zabel@gmail.com>
> Cc: Paul Parsons <lost.distance@yahoo.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
> Cc: alsa-devel@alsa-project.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

(...)
> +static struct gpiod_lookup_table hx4700_audio_gpio_table = {
> +       .dev_id = "hx4700-audio",
> +       .table = {
> +               GPIO_LOOKUP("gpio-pxa", GPIO75_HX4700_EARPHONE_nDET,
> +                           "earphone-ndet", GPIO_ACTIVE_HIGH),

This looks wrong. The n in nDET in the end of the name of the GPIO line
means active low does it not?

What I usually do when I see this is to properly set it to
GPIO_ACTIVE_LOW in the descriptor table, then invert the logic
where it's getting used.

Also rename to earphone-det instead of -ndet

> +               GPIO_LOOKUP("gpio-pxa", GPIO92_HX4700_HP_DRIVER,
> +                           "hp-driver", GPIO_ACTIVE_HIGH),
> +               GPIO_LOOKUP("gpio-pxa", GPIO107_HX4700_SPK_nSD,
> +                           "spk-nsd", GPIO_ACTIVE_HIGH),

Same here. Rename spk-sd

Yours,
Linus Walleij
Linus Walleij May 4, 2022, 9:59 p.m. UTC | #2
On Mon, May 2, 2022 at 9:08 AM Arnd Bergmann <arnd@kernel.org> wrote:
> On Sun, May 1, 2022 at 11:41 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > (...)
> > > +static struct gpiod_lookup_table hx4700_audio_gpio_table = {
> > > +       .dev_id = "hx4700-audio",
> > > +       .table = {
> > > +               GPIO_LOOKUP("gpio-pxa", GPIO75_HX4700_EARPHONE_nDET,
> > > +                           "earphone-ndet", GPIO_ACTIVE_HIGH),
> >
> > This looks wrong. The n in nDET in the end of the name of the GPIO line
> > means active low does it not?
> >
> > What I usually do when I see this is to properly set it to
> > GPIO_ACTIVE_LOW in the descriptor table, then invert the logic
> > where it's getting used.
> >
> > Also rename to earphone-det instead of -ndet
>
> Thanks for taking a look! I changed it now, but I don't know if
> I got the correct number of inversions in the end. How does this look?

Looks wrong, you can just invert the argument to any statement of set_value()
after tagging respective line as active low. Then gpilob will do a second
inversion.

> +               GPIO_LOOKUP("gpio-pxa", GPIO75_HX4700_EARPHONE_nDET,
> +                           "earphone-det", GPIO_ACTIVE_LOW),
> +               GPIO_LOOKUP("gpio-pxa", GPIO107_HX4700_SPK_nSD,
> +                           "spk-sd", GPIO_ACTIVE_LOW),

So those two have switched polarity.

> @@ -81,14 +79,14 @@ static const struct snd_soc_ops hx4700_ops = {
>  static int hx4700_spk_power(struct snd_soc_dapm_widget *w,
>                             struct snd_kcontrol *k, int event)
>  {
> -       gpio_set_value(GPIO107_HX4700_SPK_nSD, !!SND_SOC_DAPM_EVENT_ON(event));
> +       gpiod_set_value(gpiod_spk_sd, !!SND_SOC_DAPM_EVENT_ON(event));

Thus drop one ! in front of the expression, just !SND_SOC_DAPM_EVENT_ON(event)

> -       gpio_set_value(GPIO92_HX4700_HP_DRIVER, !!SND_SOC_DAPM_EVENT_ON(event));
> +       gpiod_set_value(gpiod_hp_driver, !!SND_SOC_DAPM_EVENT_ON(event));

But not this.

> +       gpiod_spk_sd = devm_gpiod_get(&pdev->dev, "spk-sd", GPIOD_OUT_LOW);

These initial values don't seem to be set in the old code you could
just use GPIOD_ASIS as flag to make sure the new code behaves
the same.

Yours,
Linus Walleij
Arnd Bergmann May 5, 2022, 6:04 a.m. UTC | #3
On Wed, May 4, 2022 at 11:59 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, May 2, 2022 at 9:08 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > On Sun, May 1, 2022 at 11:41 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > >
> > > (...)
> > > > +static struct gpiod_lookup_table hx4700_audio_gpio_table = {
> > > > +       .dev_id = "hx4700-audio",
> > > > +       .table = {
> > > > +               GPIO_LOOKUP("gpio-pxa", GPIO75_HX4700_EARPHONE_nDET,
> > > > +                           "earphone-ndet", GPIO_ACTIVE_HIGH),
> > >
> > > This looks wrong. The n in nDET in the end of the name of the GPIO line
> > > means active low does it not?
> > >
> > > What I usually do when I see this is to properly set it to
> > > GPIO_ACTIVE_LOW in the descriptor table, then invert the logic
> > > where it's getting used.
> > >
> > > Also rename to earphone-det instead of -ndet
> >
> > Thanks for taking a look! I changed it now, but I don't know if
> > I got the correct number of inversions in the end. How does this look?
>
> Looks wrong, you can just invert the argument to any statement of set_value()
> after tagging respective line as active low. Then gpilob will do a second
> inversion.
>
> > +               GPIO_LOOKUP("gpio-pxa", GPIO75_HX4700_EARPHONE_nDET,
> > +                           "earphone-det", GPIO_ACTIVE_LOW),
> > +               GPIO_LOOKUP("gpio-pxa", GPIO107_HX4700_SPK_nSD,
> > +                           "spk-sd", GPIO_ACTIVE_LOW),
>
> So those two have switched polarity.
>
> > @@ -81,14 +79,14 @@ static const struct snd_soc_ops hx4700_ops = {
> >  static int hx4700_spk_power(struct snd_soc_dapm_widget *w,
> >                             struct snd_kcontrol *k, int event)
> >  {
> > -       gpio_set_value(GPIO107_HX4700_SPK_nSD, !!SND_SOC_DAPM_EVENT_ON(event));
> > +       gpiod_set_value(gpiod_spk_sd, !!SND_SOC_DAPM_EVENT_ON(event));
>
> Thus drop one ! in front of the expression, just !SND_SOC_DAPM_EVENT_ON(event)

Ok, done. But I still leave the extra 'invert=1' in hs_jack_pin[], right?

/* Headphones jack detection DAPM pin */
static struct snd_soc_jack_pin hs_jack_pin[] = {
        {
                .pin    = "Headphone Jack",
                .mask   = SND_JACK_HEADPHONE,
        },
        {
                .pin    = "Speaker",
                /* disable speaker when hp jack is inserted */
                .mask   = SND_JACK_HEADPHONE,
                .invert = 1,
        },
};

> > +       gpiod_spk_sd = devm_gpiod_get(&pdev->dev, "spk-sd", GPIOD_OUT_LOW);
>
> These initial values don't seem to be set in the old code you could
> just use GPIOD_ASIS as flag to make sure the new code behaves
> the same.

Ok.

        Arnd
Mark Brown May 5, 2022, 2:39 p.m. UTC | #4
On Thu, May 05, 2022 at 04:33:06PM +0200, Linus Walleij wrote:
> On Thu, May 5, 2022 at 8:04 AM Arnd Bergmann <arnd@kernel.org> wrote:

> > /* Headphones jack detection DAPM pin */
> > static struct snd_soc_jack_pin hs_jack_pin[] = {
> >         {
> >                 .pin    = "Headphone Jack",
> >                 .mask   = SND_JACK_HEADPHONE,
> >         },
> >         {
> >                 .pin    = "Speaker",
> >                 /* disable speaker when hp jack is inserted */
> >                 .mask   = SND_JACK_HEADPHONE,
> >                 .invert = 1,
> >         },

> Hm some ASoC thingie. No idea what that is, but I suppose another
> place where a subsystem for legacy reasons try to do the gpiolib
> inversion on it's own accord. That one isn't flagged as active low in the
> descriptor so it's fine I guess.

It's saying that when the headphone is inserted the headphone output
should be enabled and the speaker output should be disabled, and vice
versa.

> Possible this should be fixed in ASoC to rely on gpiolib but we can't
> fix the entire world.

I don't think so.
Mark Brown May 5, 2022, 3:04 p.m. UTC | #5
On Thu, May 05, 2022 at 04:59:35PM +0200, Arnd Bergmann wrote:
> On Thu, May 5, 2022 at 4:39 PM Mark Brown <broonie@kernel.org> wrote:
> > On Thu, May 05, 2022 at 04:33:06PM +0200, Linus Walleij wrote:
> > > On Thu, May 5, 2022 at 8:04 AM Arnd Bergmann <arnd@kernel.org> wrote:

> > > > static struct snd_soc_jack_pin hs_jack_pin[] = {
> > > >         {
> > > >                 .pin    = "Headphone Jack",
> > > >                 .mask   = SND_JACK_HEADPHONE,
> > > >         },
> > > >         {
> > > >                 .pin    = "Speaker",
> > > >                 /* disable speaker when hp jack is inserted */
> > > >                 .mask   = SND_JACK_HEADPHONE,
> > > >                 .invert = 1,
> > > >         },

> > > Hm some ASoC thingie. No idea what that is, but I suppose another
> > > place where a subsystem for legacy reasons try to do the gpiolib
> > > inversion on it's own accord. That one isn't flagged as active low in the
> > > descriptor so it's fine I guess.

> > It's saying that when the headphone is inserted the headphone output
> > should be enabled and the speaker output should be disabled, and vice
> > versa.

> Ok, that sounds like I should remove the flag here if I declare the
> GPIO line as GPIO_ACTIVE_LOW instead of GPIO_ACTIVE_HIGH, right?

If you change the sense of the GPIO you'll need to flip the invert to
the headphone instead of the speaker - whichever way round the GPIO
sense is each of the pins should be taking the opposite sense from the
GPIO state to the other.
diff mbox series

Patch

diff --git a/arch/arm/mach-pxa/hx4700-pcmcia.c b/arch/arm/mach-pxa/hx4700-pcmcia.c
index e8acbfc9ef6c..e2331dfe427d 100644
--- a/arch/arm/mach-pxa/hx4700-pcmcia.c
+++ b/arch/arm/mach-pxa/hx4700-pcmcia.c
@@ -10,7 +10,7 @@ 
 #include <linux/irq.h>
 
 #include <asm/mach-types.h>
-#include <mach/hx4700.h>
+#include "hx4700.h"
 
 #include <pcmcia/soc_common.h>
 
diff --git a/arch/arm/mach-pxa/hx4700.c b/arch/arm/mach-pxa/hx4700.c
index 140a44cb2989..2b7f37172725 100644
--- a/arch/arm/mach-pxa/hx4700.c
+++ b/arch/arm/mach-pxa/hx4700.c
@@ -41,7 +41,7 @@ 
 
 #include "pxa27x.h"
 #include "addr-map.h"
-#include <mach/hx4700.h>
+#include "hx4700.h"
 #include <linux/platform_data/irda-pxaficp.h>
 
 #include <sound/ak4641.h>
@@ -834,6 +834,19 @@  static struct i2c_board_info i2c_board_info[] __initdata = {
 	},
 };
 
+static struct gpiod_lookup_table hx4700_audio_gpio_table = {
+	.dev_id = "hx4700-audio",
+	.table = {
+		GPIO_LOOKUP("gpio-pxa", GPIO75_HX4700_EARPHONE_nDET,
+			    "earphone-ndet", GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP("gpio-pxa", GPIO92_HX4700_HP_DRIVER,
+			    "hp-driver", GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP("gpio-pxa", GPIO107_HX4700_SPK_nSD,
+			    "spk-nsd", GPIO_ACTIVE_HIGH),
+		{ },
+	},
+};
+
 static struct platform_device audio = {
 	.name	= "hx4700-audio",
 	.id	= -1,
@@ -895,6 +908,7 @@  static void __init hx4700_init(void)
 
 	gpiod_add_lookup_table(&bq24022_gpiod_table);
 	gpiod_add_lookup_table(&gpio_vbus_gpiod_table);
+	gpiod_add_lookup_table(&hx4700_audio_gpio_table);
 	platform_add_devices(devices, ARRAY_SIZE(devices));
 	pwm_add_table(hx4700_pwm_lookup, ARRAY_SIZE(hx4700_pwm_lookup));
 
diff --git a/arch/arm/mach-pxa/include/mach/hx4700.h b/arch/arm/mach-pxa/hx4700.h
similarity index 99%
rename from arch/arm/mach-pxa/include/mach/hx4700.h
rename to arch/arm/mach-pxa/hx4700.h
index 0c30e6d9c660..ce2db33989e1 100644
--- a/arch/arm/mach-pxa/include/mach/hx4700.h
+++ b/arch/arm/mach-pxa/hx4700.h
@@ -10,7 +10,7 @@ 
 
 #include <linux/gpio.h>
 #include <linux/mfd/asic3.h>
-#include "irqs.h" /* PXA_NR_BUILTIN_GPIO */
+#include <mach/irqs.h> /* PXA_NR_BUILTIN_GPIO */
 
 #define HX4700_ASIC3_GPIO_BASE	PXA_NR_BUILTIN_GPIO
 #define HX4700_EGPIO_BASE	(HX4700_ASIC3_GPIO_BASE + ASIC3_NUM_GPIOS)
diff --git a/sound/soc/pxa/hx4700.c b/sound/soc/pxa/hx4700.c
index 7334fac758de..e70dc38d9892 100644
--- a/sound/soc/pxa/hx4700.c
+++ b/sound/soc/pxa/hx4700.c
@@ -10,7 +10,7 @@ 
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 
 #include <sound/core.h>
 #include <sound/jack.h>
@@ -18,10 +18,10 @@ 
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 
-#include <mach/hx4700.h>
 #include <asm/mach-types.h>
 #include "pxa2xx-i2s.h"
 
+static struct gpio_desc *gpiod_hp_driver, *gpiod_spk_nsd;
 static struct snd_soc_jack hs_jack;
 
 /* Headphones jack detection DAPM pin */
@@ -40,9 +40,8 @@  static struct snd_soc_jack_pin hs_jack_pin[] = {
 
 /* Headphones jack detection GPIO */
 static struct snd_soc_jack_gpio hs_jack_gpio = {
-	.gpio		= GPIO75_HX4700_EARPHONE_nDET,
 	.invert		= true,
-	.name		= "hp-gpio",
+	.name		= "earphone-ndet",
 	.report		= SND_JACK_HEADPHONE,
 	.debounce_time	= 200,
 };
@@ -81,14 +80,14 @@  static const struct snd_soc_ops hx4700_ops = {
 static int hx4700_spk_power(struct snd_soc_dapm_widget *w,
 			    struct snd_kcontrol *k, int event)
 {
-	gpio_set_value(GPIO107_HX4700_SPK_nSD, !!SND_SOC_DAPM_EVENT_ON(event));
+	gpiod_set_value(gpiod_spk_nsd, !!SND_SOC_DAPM_EVENT_ON(event));
 	return 0;
 }
 
 static int hx4700_hp_power(struct snd_soc_dapm_widget *w,
 			   struct snd_kcontrol *k, int event)
 {
-	gpio_set_value(GPIO92_HX4700_HP_DRIVER, !!SND_SOC_DAPM_EVENT_ON(event));
+	gpiod_set_value(gpiod_hp_driver, !!SND_SOC_DAPM_EVENT_ON(event));
 	return 0;
 }
 
@@ -162,11 +161,6 @@  static struct snd_soc_card snd_soc_card_hx4700 = {
 	.fully_routed		= true,
 };
 
-static struct gpio hx4700_audio_gpios[] = {
-	{ GPIO107_HX4700_SPK_nSD, GPIOF_OUT_INIT_HIGH, "SPK_POWER" },
-	{ GPIO92_HX4700_HP_DRIVER, GPIOF_OUT_INIT_LOW, "EP_POWER" },
-};
-
 static int hx4700_audio_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -174,26 +168,26 @@  static int hx4700_audio_probe(struct platform_device *pdev)
 	if (!machine_is_h4700())
 		return -ENODEV;
 
-	ret = gpio_request_array(hx4700_audio_gpios,
-				ARRAY_SIZE(hx4700_audio_gpios));
+	gpiod_hp_driver = devm_gpiod_get(&pdev->dev, "hp-driver", GPIOD_OUT_HIGH);
+	ret = PTR_ERR_OR_ZERO(gpiod_hp_driver);
+	if (ret)
+		return ret;
+	gpiod_spk_nsd = devm_gpiod_get(&pdev->dev, "spk-nsd", GPIOD_OUT_HIGH);
+	ret = PTR_ERR_OR_ZERO(gpiod_spk_nsd);
 	if (ret)
 		return ret;
 
+	hs_jack_gpio.gpiod_dev = &pdev->dev;
 	snd_soc_card_hx4700.dev = &pdev->dev;
 	ret = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_hx4700);
-	if (ret)
-		gpio_free_array(hx4700_audio_gpios,
-				ARRAY_SIZE(hx4700_audio_gpios));
 
 	return ret;
 }
 
 static int hx4700_audio_remove(struct platform_device *pdev)
 {
-	gpio_set_value(GPIO92_HX4700_HP_DRIVER, 0);
-	gpio_set_value(GPIO107_HX4700_SPK_nSD, 0);
-
-	gpio_free_array(hx4700_audio_gpios, ARRAY_SIZE(hx4700_audio_gpios));
+	gpiod_set_value(gpiod_hp_driver, 0);
+	gpiod_set_value(gpiod_spk_nsd, 0);
 	return 0;
 }