[PATCH v2 0/3] 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 --- Changes in v2: - Rebased onto v6.11-rc1 - Link to v1: https://lore.kernel.org/r/20240701-asoc-tas-gpios-v1-0-d69ec5d79939@linaro.o...
--- Linus Walleij (3): 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 | 1 - sound/soc/codecs/tas2764.c | 1 - sound/soc/codecs/tas2770.c | 1 - sound/soc/codecs/tas2780.c | 1 - sound/soc/codecs/tas2781-comlib.c | 3 --- sound/soc/codecs/tas2781-fmwlib.c | 1 - sound/soc/codecs/tas2781-i2c.c | 26 ++++---------------------- 9 files changed, 6 insertions(+), 37 deletions(-) --- base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b change-id: 20240701-asoc-tas-gpios-5c051d80d768
Best regards,
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 18161d02a96f..dbda552398b5 100644 --- a/include/sound/tas2781.h +++ b/include/sound/tas2781.h @@ -81,11 +81,6 @@ struct tasdevice { bool is_loaderr; };
-struct tasdevice_irqinfo { - int irq_gpio; - int irq; -}; - struct calidata { unsigned char *data; unsigned long total_sz; @@ -93,7 +88,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; @@ -115,6 +109,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 49bd7097d892..8a7fe48043d2 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -814,7 +814,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 1fbf4560f5cc..28d8b4d7b985 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> @@ -411,8 +410,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 8f9a3ae7153e..f3a7605f0710 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 e79d613745b4..fdf0840ac6c7 100644 --- a/sound/soc/codecs/tas2781-i2c.c +++ b/sound/soc/codecs/tas2781-i2c.c @@ -22,7 +22,6 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_address.h> -#include <linux/of_gpio.h> #include <linux/of_irq.h> #include <linux/regmap.h> #include <linux/slab.h> @@ -757,7 +756,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, @@ -772,7 +771,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 if (IS_ENABLED(CONFIG_OF)) { struct device_node *np = tas_priv->dev->of_node; @@ -784,7 +783,7 @@ static void tasdevice_parse_dt(struct tasdevice_priv *tas_priv) dev_addrs[ndev++] = addr; }
- tas_priv->irq_info.irq_gpio = of_irq_get(np, 0); + tas_priv->irq = of_irq_get(np, 0); } else { ndev = 1; dev_addrs[0] = client->addr; @@ -800,23 +799,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)
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 fdf0840ac6c7..bac5ea6d99b9 100644 --- a/sound/soc/codecs/tas2781-i2c.c +++ b/sound/soc/codecs/tas2781-i2c.c @@ -793,7 +793,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 | 1 - sound/soc/codecs/tas2764.c | 1 - sound/soc/codecs/tas2770.c | 1 - sound/soc/codecs/tas2780.c | 1 - 4 files changed, 4 deletions(-)
diff --git a/sound/soc/codecs/tas2552.c b/sound/soc/codecs/tas2552.c index 9e68afc09897..684d52ec6600 100644 --- a/sound/soc/codecs/tas2552.c +++ b/sound/soc/codecs/tas2552.c @@ -12,7 +12,6 @@ #include <linux/errno.h> #include <linux/device.h> #include <linux/i2c.h> -#include <linux/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 5eaddf07aadc..d482cd194c08 100644 --- a/sound/soc/codecs/tas2764.c +++ b/sound/soc/codecs/tas2764.c @@ -10,7 +10,6 @@ #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> diff --git a/sound/soc/codecs/tas2770.c b/sound/soc/codecs/tas2770.c index 5601fba17c96..9f93b230652a 100644 --- a/sound/soc/codecs/tas2770.c +++ b/sound/soc/codecs/tas2770.c @@ -14,7 +14,6 @@ #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> diff --git a/sound/soc/codecs/tas2780.c b/sound/soc/codecs/tas2780.c index 6902bfef185b..a1963415c931 100644 --- a/sound/soc/codecs/tas2780.c +++ b/sound/soc/codecs/tas2780.c @@ -7,7 +7,6 @@ #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>
On Wed, 07 Aug 2024 17:02:31 +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/3] ASoC: tas2781-i2c: Drop weird GPIO code commit: c2c0b67dca3cb3b3cea0dd60075a1c5ba77e2fcd [2/3] ASoC: tas2781-i2c: Get the right GPIO line commit: 1c4b509edad15192bfb64c81d3c305bbae8070db [3/3] ASoC: tas*: Drop unused GPIO includes commit: caab9a1cbb9a9fca24ceabeef57b3764d861ad32
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