| Message ID | 20240618-cleanup-ad7606-v1-9-f1854d5c779d@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7606: Improvements | expand |
On Tue, 18 Jun 2024 14:02:41 +0000 Guillaume Stols <gstols@baylibre.com> wrote: > The binding's documentation specifies that "As the line is active low, it > should be marked GPIO_ACTIVE_LOW". However, in the driver, it was handled > the opposite way. This commit sets the driver's behaviour in sync with the > documentation > > Fixes: 722407a4e8c0 ("staging:iio:ad7606: Use GPIO descriptor API") > Signed-off-by: Guillaume Stols <gstols@baylibre.com> This sound dangerous. If anyone is using the driver before this an it's working they indeed have the pin inverted wrt to the docs, but as it works for them this will be a regression. So messy corner - do we fix the docs or the driver? I'm not sure which is more painful. In theory the DT binding might be in use by another OS or similar which might have a non broken driver, but I suspect it isn't. Whereas perhaps the driver as it stands is in use on Linux. AD folk: You will get the support calls, do you want to risk them or should we change the docs (and maybe add a note on it being 'odd' wrt to the documentation as we are treating it as an active !standy pin) > --- > drivers/iio/adc/ad7606.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c > index 502344e019e0..05addea105f0 100644 > --- a/drivers/iio/adc/ad7606.c > +++ b/drivers/iio/adc/ad7606.c > @@ -438,7 +438,7 @@ static int ad7606_request_gpios(struct ad7606_state *st) > return PTR_ERR(st->gpio_range); > > st->gpio_standby = devm_gpiod_get_optional(dev, "standby", > - GPIOD_OUT_HIGH); > + GPIOD_OUT_LOW); > if (IS_ERR(st->gpio_standby)) > return PTR_ERR(st->gpio_standby); > > @@ -681,7 +681,7 @@ static int ad7606_suspend(struct device *dev) > > if (st->gpio_standby) { > gpiod_set_value(st->gpio_range, 1); > - gpiod_set_value(st->gpio_standby, 0); > + gpiod_set_value(st->gpio_standby, 1); > } > > return 0; >
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c index 502344e019e0..05addea105f0 100644 --- a/drivers/iio/adc/ad7606.c +++ b/drivers/iio/adc/ad7606.c @@ -438,7 +438,7 @@ static int ad7606_request_gpios(struct ad7606_state *st) return PTR_ERR(st->gpio_range); st->gpio_standby = devm_gpiod_get_optional(dev, "standby", - GPIOD_OUT_HIGH); + GPIOD_OUT_LOW); if (IS_ERR(st->gpio_standby)) return PTR_ERR(st->gpio_standby); @@ -681,7 +681,7 @@ static int ad7606_suspend(struct device *dev) if (st->gpio_standby) { gpiod_set_value(st->gpio_range, 1); - gpiod_set_value(st->gpio_standby, 0); + gpiod_set_value(st->gpio_standby, 1); } return 0;
The binding's documentation specifies that "As the line is active low, it should be marked GPIO_ACTIVE_LOW". However, in the driver, it was handled the opposite way. This commit sets the driver's behaviour in sync with the documentation Fixes: 722407a4e8c0 ("staging:iio:ad7606: Use GPIO descriptor API") Signed-off-by: Guillaume Stols <gstols@baylibre.com> --- drivers/iio/adc/ad7606.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)