| Message ID | 20220309104104.3605112-1-john@metanate.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: tas5805m: fix pdn polarity | expand |
On Wed, Mar 09, 2022 at 10:41:04AM +0000, John Keeping wrote:
> Fixes: ec45268467f4 ("ASoC: add support for TAS5805M digital amplifier")
Please don't insert fixes tags unless you're actually fixing a bug,
stylistic improvements like this - people try to use the tag for
backporting bug fixes and so on so extra tags create noise for them.
On Wed, Mar 09, 2022 at 12:12:47PM +0000, Mark Brown wrote: > On Wed, Mar 09, 2022 at 10:41:04AM +0000, John Keeping wrote: > > > Fixes: ec45268467f4 ("ASoC: add support for TAS5805M digital amplifier") > > Please don't insert fixes tags unless you're actually fixing a bug, > stylistic improvements like this - people try to use the tag for > backporting bug fixes and so on so extra tags create noise for them. This isn't purely stylistic - it affects the interpretation of pdn-gpios in the device tree so that it matches all of the other bindings that use this property: active means PDN asserted. There are currently no in-tree users of the binding and the patch adding it is queued for the next merge window.
On Wed, Mar 09, 2022 at 12:34:28PM +0000, John Keeping wrote: > On Wed, Mar 09, 2022 at 12:12:47PM +0000, Mark Brown wrote: > > On Wed, Mar 09, 2022 at 10:41:04AM +0000, John Keeping wrote: > > > > > Fixes: ec45268467f4 ("ASoC: add support for TAS5805M digital amplifier") > > > > Please don't insert fixes tags unless you're actually fixing a bug, > > stylistic improvements like this - people try to use the tag for > > backporting bug fixes and so on so extra tags create noise for them. > This isn't purely stylistic - it affects the interpretation of pdn-gpios > in the device tree so that it matches all of the other bindings that use > this property: active means PDN asserted. So there's some functional change? That's not at all clear from either the patch description or the code - it's flipping both the ACTIVE mode for the GPIO and all the values set which should result in no visible change. If there is a functional problem that is being fixed the description needs to be clear as to what that is.
diff --git a/sound/soc/codecs/tas5805m.c b/sound/soc/codecs/tas5805m.c index fa0e81ec875a..12146a860ef8 100644 --- a/sound/soc/codecs/tas5805m.c +++ b/sound/soc/codecs/tas5805m.c @@ -155,7 +155,7 @@ static const uint32_t tas5805m_volume[] = { struct tas5805m_priv { struct regulator *pvdd; - struct gpio_desc *gpio_pdn_n; + struct gpio_desc *gpio_pdn; uint8_t *dsp_cfg_data; int dsp_cfg_len; @@ -444,11 +444,11 @@ static int tas5805m_i2c_probe(struct i2c_client *i2c) dev_set_drvdata(dev, tas5805m); tas5805m->regmap = regmap; - tas5805m->gpio_pdn_n = devm_gpiod_get(dev, "pdn", GPIOD_OUT_LOW); - if (IS_ERR(tas5805m->gpio_pdn_n)) { + tas5805m->gpio_pdn = devm_gpiod_get(dev, "pdn", GPIOD_OUT_HIGH); + if (IS_ERR(tas5805m->gpio_pdn)) { dev_err(dev, "error requesting PDN gpio: %ld\n", - PTR_ERR(tas5805m->gpio_pdn_n)); - return PTR_ERR(tas5805m->gpio_pdn_n); + PTR_ERR(tas5805m->gpio_pdn)); + return PTR_ERR(tas5805m->gpio_pdn); } /* This configuration must be generated by PPC3. The file loaded @@ -505,7 +505,7 @@ static int tas5805m_i2c_probe(struct i2c_client *i2c) } usleep_range(100000, 150000); - gpiod_set_value(tas5805m->gpio_pdn_n, 1); + gpiod_set_value(tas5805m->gpio_pdn, 0); usleep_range(10000, 15000); /* Don't register through devm. We need to be able to unregister @@ -515,7 +515,7 @@ static int tas5805m_i2c_probe(struct i2c_client *i2c) &tas5805m_dai, 1); if (ret < 0) { dev_err(dev, "unable to register codec: %d\n", ret); - gpiod_set_value(tas5805m->gpio_pdn_n, 0); + gpiod_set_value(tas5805m->gpio_pdn, 1); regulator_disable(tas5805m->pvdd); return ret; } @@ -529,7 +529,7 @@ static int tas5805m_i2c_remove(struct i2c_client *i2c) struct tas5805m_priv *tas5805m = dev_get_drvdata(dev); snd_soc_unregister_component(dev); - gpiod_set_value(tas5805m->gpio_pdn_n, 0); + gpiod_set_value(tas5805m->gpio_pdn, 1); usleep_range(10000, 15000); regulator_disable(tas5805m->pvdd); return 0;
The binding defines the GPIO as "pdn-gpios" so when the GPIO is active the expectation is that the power down signal is asserted, but the driver swaps this to represent the inverted logic of the electrical signal. The GPIO_ACTIVE_HIGH/LOW flags should be used to identify the inverted logic here with the driver treating power down as active when the mapped GPIO value is 1. Fixes: ec45268467f4 ("ASoC: add support for TAS5805M digital amplifier") Signed-off-by: John Keeping <john@metanate.com> --- sound/soc/codecs/tas5805m.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)