[PATCH 0/4] ASoC: tas*: Fix up GPIO usage
The TI TAS drivers use some legacy GPIO code and headers, this series fixes it up.
The TAS2781 is a special case since it adds a handful of lines of deviating code to reconfigure a GPIO line for IRQ mode and then never actually use the IRQ obtained in the code. Is the line used by autonomous hardware? I'm puzzled by this.
Anyways the patch suggest how to solve this properly by fixing the parent irqchip and I'm happy to help.
Signed-off-by: Linus Walleij linus.walleij@linaro.org --- Linus Walleij (4): ASoC: tas5086: Convert to GPIO descriptors ASoC: tas2781-i2c: Drop weird GPIO code ASoC: tas2781-i2c: Get the right GPIO line ASoC: tas*: Drop unused GPIO includes
include/sound/tas2781.h | 7 +------ sound/pci/hda/tas2781_hda_i2c.c | 2 +- sound/soc/codecs/tas2552.c | 2 -- sound/soc/codecs/tas2764.c | 2 -- sound/soc/codecs/tas2770.c | 2 -- sound/soc/codecs/tas2780.c | 2 -- sound/soc/codecs/tas2781-comlib.c | 3 --- sound/soc/codecs/tas2781-fmwlib.c | 1 - sound/soc/codecs/tas2781-i2c.c | 26 ++++---------------------- sound/soc/codecs/tas5086.c | 27 ++++++++++++--------------- 10 files changed, 18 insertions(+), 56 deletions(-) --- base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0 change-id: 20240701-asoc-tas-gpios-5c051d80d768
Best regards,
Switch the driver to use GPIO descriptors.
Notice that we let the gpiolib handle line inversion for the active low reset line (nreset !reset).
There are no upstream device trees using the tas5086 compatible string, if there were, we would need to ascertain that they all set the GPIO_ACTIVE_LOW flag on their GPIO lines.
Signed-off-by: Linus Walleij linus.walleij@linaro.org --- sound/soc/codecs/tas5086.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/sound/soc/codecs/tas5086.c b/sound/soc/codecs/tas5086.c index 6d45df3b9ba4..4bc1fdd232bb 100644 --- a/sound/soc/codecs/tas5086.c +++ b/sound/soc/codecs/tas5086.c @@ -24,14 +24,13 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/delay.h> -#include <linux/gpio.h> +#include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> #include <linux/of.h> #include <linux/of_device.h> -#include <linux/of_gpio.h> #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> @@ -246,7 +245,7 @@ struct tas5086_private { /* Current sample rate for de-emphasis control */ int rate; /* GPIO driving Reset pin, if any */ - int gpio_nreset; + struct gpio_desc *reset; struct regulator_bulk_data supplies[ARRAY_SIZE(supply_names)]; };
@@ -462,11 +461,11 @@ static int tas5086_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
static void tas5086_reset(struct tas5086_private *priv) { - if (gpio_is_valid(priv->gpio_nreset)) { + if (priv->reset) { /* Reset codec - minimum assertion time is 400ns */ - gpio_direction_output(priv->gpio_nreset, 0); + gpiod_direction_output(priv->reset, 1); udelay(1); - gpio_set_value(priv->gpio_nreset, 1); + gpiod_set_value(priv->reset, 0);
/* Codec needs ~15ms to wake up */ msleep(15); @@ -867,9 +866,9 @@ static void tas5086_remove(struct snd_soc_component *component) { struct tas5086_private *priv = snd_soc_component_get_drvdata(component);
- if (gpio_is_valid(priv->gpio_nreset)) + if (priv->reset) /* Set codec to the reset state */ - gpio_set_value(priv->gpio_nreset, 0); + gpiod_set_value(priv->reset, 1);
regulator_bulk_disable(ARRAY_SIZE(priv->supplies), priv->supplies); }; @@ -914,7 +913,6 @@ static int tas5086_i2c_probe(struct i2c_client *i2c) { struct tas5086_private *priv; struct device *dev = &i2c->dev; - int gpio_nreset = -EINVAL; int i, ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); @@ -940,12 +938,11 @@ static int tas5086_i2c_probe(struct i2c_client *i2c)
i2c_set_clientdata(i2c, priv);
- gpio_nreset = of_get_named_gpio(dev->of_node, "reset-gpio", 0); - if (gpio_is_valid(gpio_nreset)) - if (devm_gpio_request(dev, gpio_nreset, "TAS5086 Reset")) - gpio_nreset = -EINVAL; - - priv->gpio_nreset = gpio_nreset; + /* Request line asserted */ + priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(priv->reset)) + return PTR_ERR(priv->reset); + gpiod_set_consumer_name(priv->reset, "TAS5086 Reset");
ret = regulator_bulk_enable(ARRAY_SIZE(priv->supplies), priv->supplies); if (ret < 0) {
The tas2781-i2c driver gets an IRQ from either ACPI or device tree, then proceeds to check if the IRQ has a corresponding GPIO and in case it does enforce the GPIO as input and set a label on it.
This is abuse of the API:
- First we cannot guarantee that the numberspaces of the GPIOs and the IRQs are the same, i.e that an IRQ number corresponds to a GPIO number like that.
- Second, GPIO chips and IRQ chips should be treated as orthogonal APIs, the irqchip needs to ascertain that the backing GPIO line is set to input etc just using the irqchip.
- Third it is using the legacy <linux/gpio.h> API which should not be used in new code yet this was added just a year ago.
Delete the offending code.
If this creates problems the GPIO and irqchip maintainers can help to fix the issues.
It *should* not create any problems, because the irq isn't used anywhere in the driver, it's just obtained and then left unused.
Fixes: ef3bcde75d06 ("ASoC: tas2781: Add tas2781 driver") Signed-off-by: Linus Walleij linus.walleij@linaro.org --- include/sound/tas2781.h | 7 +------ sound/pci/hda/tas2781_hda_i2c.c | 2 +- sound/soc/codecs/tas2781-comlib.c | 3 --- sound/soc/codecs/tas2781-fmwlib.c | 1 - sound/soc/codecs/tas2781-i2c.c | 24 +++--------------------- 5 files changed, 5 insertions(+), 32 deletions(-)
diff --git a/include/sound/tas2781.h b/include/sound/tas2781.h index 99ca3e401fd1..6f6e3e2f652c 100644 --- a/include/sound/tas2781.h +++ b/include/sound/tas2781.h @@ -80,11 +80,6 @@ struct tasdevice { bool is_loaderr; };
-struct tasdevice_irqinfo { - int irq_gpio; - int irq; -}; - struct calidata { unsigned char *data; unsigned long total_sz; @@ -92,7 +87,6 @@ struct calidata {
struct tasdevice_priv { struct tasdevice tasdevice[TASDEVICE_MAX_CHANNELS]; - struct tasdevice_irqinfo irq_info; struct tasdevice_rca rcabin; struct calidata cali_data; struct tasdevice_fw *fmw; @@ -113,6 +107,7 @@ struct tasdevice_priv { unsigned int chip_id; unsigned int sysclk;
+ int irq; int cur_prog; int cur_conf; int fw_state; diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 75f7674c66ee..c5ace7216ecb 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -818,7 +818,7 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt) } else return -ENODEV;
- tas_hda->priv->irq_info.irq = clt->irq; + tas_hda->priv->irq = clt->irq; ret = tas2781_read_acpi(tas_hda->priv, device_name); if (ret) return dev_err_probe(tas_hda->dev, ret, diff --git a/sound/soc/codecs/tas2781-comlib.c b/sound/soc/codecs/tas2781-comlib.c index 3aa81514dad7..0444cf90c511 100644 --- a/sound/soc/codecs/tas2781-comlib.c +++ b/sound/soc/codecs/tas2781-comlib.c @@ -14,7 +14,6 @@ #include <linux/interrupt.h> #include <linux/module.h> #include <linux/of.h> -#include <linux/of_gpio.h> #include <linux/of_irq.h> #include <linux/regmap.h> #include <linux/slab.h> @@ -406,8 +405,6 @@ EXPORT_SYMBOL_GPL(tasdevice_dsp_remove);
void tasdevice_remove(struct tasdevice_priv *tas_priv) { - if (gpio_is_valid(tas_priv->irq_info.irq_gpio)) - gpio_free(tas_priv->irq_info.irq_gpio); mutex_destroy(&tas_priv->codec_lock); } EXPORT_SYMBOL_GPL(tasdevice_remove); diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c index 265a8ca25cbb..d6afab542da7 100644 --- a/sound/soc/codecs/tas2781-fmwlib.c +++ b/sound/soc/codecs/tas2781-fmwlib.c @@ -13,7 +13,6 @@ #include <linux/interrupt.h> #include <linux/module.h> #include <linux/of.h> -#include <linux/of_gpio.h> #include <linux/of_irq.h> #include <linux/regmap.h> #include <linux/slab.h> diff --git a/sound/soc/codecs/tas2781-i2c.c b/sound/soc/codecs/tas2781-i2c.c index 9350972dfefe..1542915b83a2 100644 --- a/sound/soc/codecs/tas2781-i2c.c +++ b/sound/soc/codecs/tas2781-i2c.c @@ -21,7 +21,6 @@ #include <linux/interrupt.h> #include <linux/module.h> #include <linux/of.h> -#include <linux/of_gpio.h> #include <linux/of_irq.h> #include <linux/regmap.h> #include <linux/slab.h> @@ -605,7 +604,7 @@ static void tasdevice_parse_dt(struct tasdevice_priv *tas_priv) { struct i2c_client *client = (struct i2c_client *)tas_priv->client; unsigned int dev_addrs[TASDEVICE_MAX_CHANNELS]; - int rc, i, ndev = 0; + int i, ndev = 0;
if (tas_priv->isacpi) { ndev = device_property_read_u32_array(&client->dev, @@ -620,7 +619,7 @@ static void tasdevice_parse_dt(struct tasdevice_priv *tas_priv) "ti,audio-slots", dev_addrs, ndev); }
- tas_priv->irq_info.irq_gpio = + tas_priv->irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(&client->dev), 0); } else { struct device_node *np = tas_priv->dev->of_node; @@ -648,7 +647,7 @@ static void tasdevice_parse_dt(struct tasdevice_priv *tas_priv) ndev = 1; dev_addrs[0] = client->addr; #endif - tas_priv->irq_info.irq_gpio = of_irq_get(np, 0); + tas_priv->irq = of_irq_get(np, 0); } tas_priv->ndev = ndev; for (i = 0; i < ndev; i++) @@ -661,23 +660,6 @@ static void tasdevice_parse_dt(struct tasdevice_priv *tas_priv) __func__);
strcpy(tas_priv->dev_name, tasdevice_id[tas_priv->chip_id].name); - - if (gpio_is_valid(tas_priv->irq_info.irq_gpio)) { - rc = gpio_request(tas_priv->irq_info.irq_gpio, - "AUDEV-IRQ"); - if (!rc) { - gpio_direction_input( - tas_priv->irq_info.irq_gpio); - - tas_priv->irq_info.irq = - gpio_to_irq(tas_priv->irq_info.irq_gpio); - } else - dev_err(tas_priv->dev, "%s: GPIO %d request error\n", - __func__, tas_priv->irq_info.irq_gpio); - } else - dev_err(tas_priv->dev, - "Looking up irq-gpio property failed %d\n", - tas_priv->irq_info.irq_gpio); }
static int tasdevice_i2c_probe(struct i2c_client *i2c)
On Mon, Jul 01, 2024 at 09:02:13AM +0200, Linus Walleij wrote:
The tas2781-i2c driver gets an IRQ from either ACPI or device tree, then proceeds to check if the IRQ has a corresponding GPIO and in case it does enforce the GPIO as input and set a label on it.
This doesn't apply against current code, please check and resend.
The code is obtaining a GPIO reset using the reset GPIO name "reset-gpios", but the gpiolib is already adding the suffix "-gpios" to anything passed to this function and will be looking for "reset-gpios-gpios" which is most certainly not what the author desired.
Fix it up.
Fixes: ef3bcde75d06 ("ASoC: tas2781: Add tas2781 driver") Signed-off-by: Linus Walleij linus.walleij@linaro.org --- sound/soc/codecs/tas2781-i2c.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tas2781-i2c.c b/sound/soc/codecs/tas2781-i2c.c index 1542915b83a2..256f2e8f1ba9 100644 --- a/sound/soc/codecs/tas2781-i2c.c +++ b/sound/soc/codecs/tas2781-i2c.c @@ -654,7 +654,7 @@ static void tasdevice_parse_dt(struct tasdevice_priv *tas_priv) tas_priv->tasdevice[i].dev_addr = dev_addrs[i];
tas_priv->reset = devm_gpiod_get_optional(&client->dev, - "reset-gpios", GPIOD_OUT_HIGH); + "reset", GPIOD_OUT_HIGH); if (IS_ERR(tas_priv->reset)) dev_err(tas_priv->dev, "%s Can't get reset GPIO\n", __func__);
These drivers all use <linux/gpio/consumer.h> and has no business including the legacy headers <linux/gpio.h> or <linux/of_gpio.h>. Drop the surplus includes.
Signed-off-by: Linus Walleij linus.walleij@linaro.org --- sound/soc/codecs/tas2552.c | 2 -- sound/soc/codecs/tas2764.c | 2 -- sound/soc/codecs/tas2770.c | 2 -- sound/soc/codecs/tas2780.c | 2 -- 4 files changed, 8 deletions(-)
diff --git a/sound/soc/codecs/tas2552.c b/sound/soc/codecs/tas2552.c index a7ed59ec49a6..684d52ec6600 100644 --- a/sound/soc/codecs/tas2552.c +++ b/sound/soc/codecs/tas2552.c @@ -12,8 +12,6 @@ #include <linux/errno.h> #include <linux/device.h> #include <linux/i2c.h> -#include <linux/gpio.h> -#include <linux/of_gpio.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> #include <linux/slab.h> diff --git a/sound/soc/codecs/tas2764.c b/sound/soc/codecs/tas2764.c index 1dc719d726ab..d482cd194c08 100644 --- a/sound/soc/codecs/tas2764.c +++ b/sound/soc/codecs/tas2764.c @@ -10,12 +10,10 @@ #include <linux/delay.h> #include <linux/pm.h> #include <linux/i2c.h> -#include <linux/gpio.h> #include <linux/gpio/consumer.h> #include <linux/regulator/consumer.h> #include <linux/regmap.h> #include <linux/of.h> -#include <linux/of_gpio.h> #include <linux/slab.h> #include <sound/soc.h> #include <sound/pcm.h> diff --git a/sound/soc/codecs/tas2770.c b/sound/soc/codecs/tas2770.c index 67bc1c8b0131..9f93b230652a 100644 --- a/sound/soc/codecs/tas2770.c +++ b/sound/soc/codecs/tas2770.c @@ -14,13 +14,11 @@ #include <linux/delay.h> #include <linux/pm.h> #include <linux/i2c.h> -#include <linux/gpio.h> #include <linux/gpio/consumer.h> #include <linux/regulator/consumer.h> #include <linux/firmware.h> #include <linux/regmap.h> #include <linux/of.h> -#include <linux/of_gpio.h> #include <linux/slab.h> #include <sound/soc.h> #include <sound/pcm.h> diff --git a/sound/soc/codecs/tas2780.c b/sound/soc/codecs/tas2780.c index a18ccf5fb7ad..a1963415c931 100644 --- a/sound/soc/codecs/tas2780.c +++ b/sound/soc/codecs/tas2780.c @@ -7,11 +7,9 @@ #include <linux/err.h> #include <linux/pm.h> #include <linux/i2c.h> -#include <linux/gpio.h> #include <linux/gpio/consumer.h> #include <linux/regmap.h> #include <linux/of.h> -#include <linux/of_gpio.h> #include <sound/soc.h> #include <sound/pcm.h> #include <sound/pcm_params.h>
On Mon, 01 Jul 2024 09:02:11 +0200, Linus Walleij wrote:
The TI TAS drivers use some legacy GPIO code and headers, this series fixes it up.
The TAS2781 is a special case since it adds a handful of lines of deviating code to reconfigure a GPIO line for IRQ mode and then never actually use the IRQ obtained in the code. Is the line used by autonomous hardware? I'm puzzled by this.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/4] ASoC: tas5086: Convert to GPIO descriptors commit: 3b628e617b21144579bbca806a26737e8485d93a
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Linus Walleij -
Mark Brown