The binding defines the GPIO as "pdn-gpios" so when the GPIO is active
the expectation is that the power down signal is asserted and this is
how all other drivers using this GPIO name interpret the value.
But the tas5805m driver inverts the sense from the normal expectation so
when the powerdown GPIO is logically asserted the chip is running.
This is a new driver that is not yet in a released kernel and has no
in-tree users of the binding so fix the sense of the GPIO so that
logically asserted means that the device is powered down.
Rename the variable to match so that the compiler will catch any places
that should have been updated but have been missed.
Fixes: ec45268467f4 ("ASoC: add support for TAS5805M digital amplifier")
Signed-off-by: John Keeping <[email protected]>
---
v2:
- Rewrite commit message to make it more obvious that this is a change
to the interpretation of the GPIO in the binding
sound/soc/codecs/tas5805m.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
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;
--
2.35.1
On Wed, Mar 09, 2022 at 01:56:49PM +0000, John Keeping wrote:
> The binding defines the GPIO as "pdn-gpios" so when the GPIO is active
> the expectation is that the power down signal is asserted and this is
> how all other drivers using this GPIO name interpret the value.
> But the tas5805m driver inverts the sense from the normal expectation so
> when the powerdown GPIO is logically asserted the chip is running.
> This is a new driver that is not yet in a released kernel and has no
> in-tree users of the binding so fix the sense of the GPIO so that
> logically asserted means that the device is powered down.
> - Rewrite commit message to make it more obvious that this is a change
> to the interpretation of the GPIO in the binding
I'm still not seeing the functional change here. The actual state of
the GPIO is identical in both cases, all that's changing is the logical
view internally to the kernel.
On Wed, Mar 09, 2022 at 03:56:12PM +0000, Mark Brown wrote:
> On Wed, Mar 09, 2022 at 01:56:49PM +0000, John Keeping wrote:
>
> > The binding defines the GPIO as "pdn-gpios" so when the GPIO is active
> > the expectation is that the power down signal is asserted and this is
> > how all other drivers using this GPIO name interpret the value.
>
> > But the tas5805m driver inverts the sense from the normal expectation so
> > when the powerdown GPIO is logically asserted the chip is running.
>
> > This is a new driver that is not yet in a released kernel and has no
> > in-tree users of the binding so fix the sense of the GPIO so that
> > logically asserted means that the device is powered down.
>
> > - Rewrite commit message to make it more obvious that this is a change
> > to the interpretation of the GPIO in the binding
>
> I'm still not seeing the functional change here. The actual state of
> the GPIO is identical in both cases, all that's changing is the logical
> view internally to the kernel.
Ah, sorry, I'm considering it functional since it changes the device
tree ABI.
Used with the same device tree with, say, GPIO_ACTIVE_HIGH the physical
state of the GPIO will change as a result of this patch and the device
tree needs to be updated to use GPIO_ACTIVE_LOW.
On Wed, Mar 09, 2022 at 04:19:31PM +0000, John Keeping wrote:
> On Wed, Mar 09, 2022 at 03:56:12PM +0000, Mark Brown wrote:
> > I'm still not seeing the functional change here. The actual state of
> > the GPIO is identical in both cases, all that's changing is the logical
> > view internally to the kernel.
> Ah, sorry, I'm considering it functional since it changes the device
> tree ABI.
> Used with the same device tree with, say, GPIO_ACTIVE_HIGH the physical
> state of the GPIO will change as a result of this patch and the device
> tree needs to be updated to use GPIO_ACTIVE_LOW.
I think the device tree binding needs to be clarified here to be
explicit about this since there's obviously some room for user confusion
here. We can probably get away with a change at this point since it's
not hit a release but we do need to try to avoid the situation where any
other implementations use active high polarity for the bindings.
On Wed, Mar 09, 2022 at 08:16:07PM +0000, John Keeping wrote:
> On Wed, Mar 09, 2022 at 04:28:30PM +0000, Mark Brown wrote:
> > I think the device tree binding needs to be clarified here to be
> > explicit about this since there's obviously some room for user confusion
> > here. We can probably get away with a change at this point since it's
> > not hit a release but we do need to try to avoid the situation where any
> > other implementations use active high polarity for the bindings.
> Taking a quick survey of the other devices that have a pdn-gpios
> property:
> - tvp5150 is correct with the driver setting 0 to make the device active
> - tas571x also sets 0 to make the device active
> - ak4375 uses the opposite sense setting PDN = 1 to make the device
> active; this has no in-tree users and was merged as part of v5.17-rc1
> so it's not in a released kernel yet
Sure, I still think it would be good to update the binding document to
clarify things as part of your patch - the binding currently just has it
as the "pdn" pin not the /pdn pin or anything.
On Wed, Mar 09, 2022 at 04:28:30PM +0000, Mark Brown wrote:
> On Wed, Mar 09, 2022 at 04:19:31PM +0000, John Keeping wrote:
> > On Wed, Mar 09, 2022 at 03:56:12PM +0000, Mark Brown wrote:
>
> > > I'm still not seeing the functional change here. The actual state of
> > > the GPIO is identical in both cases, all that's changing is the logical
> > > view internally to the kernel.
>
> > Ah, sorry, I'm considering it functional since it changes the device
> > tree ABI.
>
> > Used with the same device tree with, say, GPIO_ACTIVE_HIGH the physical
> > state of the GPIO will change as a result of this patch and the device
> > tree needs to be updated to use GPIO_ACTIVE_LOW.
>
> I think the device tree binding needs to be clarified here to be
> explicit about this since there's obviously some room for user confusion
> here. We can probably get away with a change at this point since it's
> not hit a release but we do need to try to avoid the situation where any
> other implementations use active high polarity for the bindings.
Taking a quick survey of the other devices that have a pdn-gpios
property:
- tvp5150 is correct with the driver setting 0 to make the device active
- tas571x also sets 0 to make the device active
- ak4375 uses the opposite sense setting PDN = 1 to make the device
active; this has no in-tree users and was merged as part of v5.17-rc1
so it's not in a released kernel yet
On Wed, Mar 09, 2022 at 09:55:40PM +0000, Mark Brown wrote:
> On Wed, Mar 09, 2022 at 08:16:07PM +0000, John Keeping wrote:
> > On Wed, Mar 09, 2022 at 04:28:30PM +0000, Mark Brown wrote:
>
> > > I think the device tree binding needs to be clarified here to be
> > > explicit about this since there's obviously some room for user confusion
> > > here. We can probably get away with a change at this point since it's
> > > not hit a release but we do need to try to avoid the situation where any
> > > other implementations use active high polarity for the bindings.
>
> > Taking a quick survey of the other devices that have a pdn-gpios
> > property:
>
> > - tvp5150 is correct with the driver setting 0 to make the device active
>
> > - tas571x also sets 0 to make the device active
>
> > - ak4375 uses the opposite sense setting PDN = 1 to make the device
> > active; this has no in-tree users and was merged as part of v5.17-rc1
> > so it's not in a released kernel yet
>
> Sure, I still think it would be good to update the binding document to
> clarify things as part of your patch - the binding currently just has it
> as the "pdn" pin not the /pdn pin or anything.
I've been thinking about this but I can't really think what to say.
tas571x's binding says:
GPIO specifier for the TAS571x's active low powerdown line
Is that the sort of wording you have in mind?
To me it seems like a general principle that the GPIO_ACTIVE_{HIGH,LOW}
flags should be used to indicate how the pin works so that the driver
consistently uses logical levels regardless of how the hardware is
wired.
From the driver point of view pdn-gpios is effectively reset-gpios by
another name and it's pretty consistent that setting a reset GPIO to 1
means the device is inaccessible.
Maybe this just means I'm approaching this "down" from the software
abstraction more than "up" from the hardware.
On Thu, Mar 10, 2022 at 08:09:42PM +0000, John Keeping wrote:
> On Wed, Mar 09, 2022 at 09:55:40PM +0000, Mark Brown wrote:
> > Sure, I still think it would be good to update the binding document to
> > clarify things as part of your patch - the binding currently just has it
> > as the "pdn" pin not the /pdn pin or anything.
> I've been thinking about this but I can't really think what to say.
> tas571x's binding says:
> GPIO specifier for the TAS571x's active low powerdown line
> Is that the sort of wording you have in mind?
Something along those lines, probably also mention something about the
flag. If the DT has to specify the polarity for things to work
(basically, if it's not active high) then that should be in the binding.
> To me it seems like a general principle that the GPIO_ACTIVE_{HIGH,LOW}
> flags should be used to indicate how the pin works so that the driver
> consistently uses logical levels regardless of how the hardware is
> wired.
Every layer that introduces an inversion is a source of confusion - if
the physical signal needs to be set low but the code in the driver sets
the signal high that's a surprise for people, and generally if the user
needs to specify that the polarity is inverted every time they bind the
device that's a gotcha people won't tend to expect.
> Maybe this just means I'm approaching this "down" from the software
> abstraction more than "up" from the hardware.
Think about it more as being about making it easier to follow what the
physical state of the GPIO is.