| Message ID | 20201209104649.5794-1-lars@metafoo.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: ad5504: Fix setting power-down state | expand |
On Wed, 9 Dec 2020 11:46:49 +0100 Lars-Peter Clausen <lars@metafoo.de> wrote: > The power-down mask of the ad5504 is actually a power-up mask. Meaning if > a bit is set the corresponding channel is powered up and if it is not set > the channel is powered down. > > The driver currently has this the wrong way around, resulting in the > channel being powered up when requested to be powered down and vice versa. > > Fixes: 3bbbf150ffde ("staging:iio:dac:ad5504: Use strtobool for boolean values") > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > --- > Stumbled upon this while looking at something else. It is untested, but I > think it should be right. Alex can you double check? I read the datasheet as saying this fix is right but will wait for a confirmation from Alex. Thanks, Jonathan > --- > drivers/iio/dac/ad5504.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c > index 28921b62e642..e9297c25d4ef 100644 > --- a/drivers/iio/dac/ad5504.c > +++ b/drivers/iio/dac/ad5504.c > @@ -187,9 +187,9 @@ static ssize_t ad5504_write_dac_powerdown(struct iio_dev *indio_dev, > return ret; > > if (pwr_down) > - st->pwr_down_mask |= (1 << chan->channel); > - else > st->pwr_down_mask &= ~(1 << chan->channel); > + else > + st->pwr_down_mask |= (1 << chan->channel); > > ret = ad5504_spi_write(st, AD5504_ADDR_CTRL, > AD5504_DAC_PWRDWN_MODE(st->pwr_down_mode) |
On Sun, Dec 13, 2020 at 8:59 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 9 Dec 2020 11:46:49 +0100 > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > The power-down mask of the ad5504 is actually a power-up mask. Meaning if > > a bit is set the corresponding channel is powered up and if it is not set > > the channel is powered down. > > > > The driver currently has this the wrong way around, resulting in the > > channel being powered up when requested to be powered down and vice versa. > > > > Fixes: 3bbbf150ffde ("staging:iio:dac:ad5504: Use strtobool for boolean values") > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > > --- > > Stumbled upon this while looking at something else. It is untested, but I > > think it should be right. Alex can you double check? > > I read the datasheet as saying this fix is right but will wait for > a confirmation from Alex. Oh. I see why the initial confusion existed. Was confused a bit myself. The thermal shutdown follows the initial/unchanged logic. While the actual powerdown channel bits are inverted. Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > Thanks, > > Jonathan > > > --- > > drivers/iio/dac/ad5504.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c > > index 28921b62e642..e9297c25d4ef 100644 > > --- a/drivers/iio/dac/ad5504.c > > +++ b/drivers/iio/dac/ad5504.c > > @@ -187,9 +187,9 @@ static ssize_t ad5504_write_dac_powerdown(struct iio_dev *indio_dev, > > return ret; > > > > if (pwr_down) > > - st->pwr_down_mask |= (1 << chan->channel); > > - else > > st->pwr_down_mask &= ~(1 << chan->channel); > > + else > > + st->pwr_down_mask |= (1 << chan->channel); > > > > ret = ad5504_spi_write(st, AD5504_ADDR_CTRL, > > AD5504_DAC_PWRDWN_MODE(st->pwr_down_mode) | >
On Mon, 14 Dec 2020 09:23:20 +0200 Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > On Sun, Dec 13, 2020 at 8:59 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Wed, 9 Dec 2020 11:46:49 +0100 > > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > > The power-down mask of the ad5504 is actually a power-up mask. Meaning if > > > a bit is set the corresponding channel is powered up and if it is not set > > > the channel is powered down. > > > > > > The driver currently has this the wrong way around, resulting in the > > > channel being powered up when requested to be powered down and vice versa. > > > > > > Fixes: 3bbbf150ffde ("staging:iio:dac:ad5504: Use strtobool for boolean values") > > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > > > --- > > > Stumbled upon this while looking at something else. It is untested, but I > > > think it should be right. Alex can you double check? > > > > I read the datasheet as saying this fix is right but will wait for > > a confirmation from Alex. > > Oh. > I see why the initial confusion existed. > Was confused a bit myself. > The thermal shutdown follows the initial/unchanged logic. > While the actual powerdown channel bits are inverted. > > Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Applied to the fixes-togreg branch of iio.git and marked for stable. Thanks, Jonathan > > > > > > Thanks, > > > > Jonathan > > > > > --- > > > drivers/iio/dac/ad5504.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c > > > index 28921b62e642..e9297c25d4ef 100644 > > > --- a/drivers/iio/dac/ad5504.c > > > +++ b/drivers/iio/dac/ad5504.c > > > @@ -187,9 +187,9 @@ static ssize_t ad5504_write_dac_powerdown(struct iio_dev *indio_dev, > > > return ret; > > > > > > if (pwr_down) > > > - st->pwr_down_mask |= (1 << chan->channel); > > > - else > > > st->pwr_down_mask &= ~(1 << chan->channel); > > > + else > > > + st->pwr_down_mask |= (1 << chan->channel); > > > > > > ret = ad5504_spi_write(st, AD5504_ADDR_CTRL, > > > AD5504_DAC_PWRDWN_MODE(st->pwr_down_mode) | > >
diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c index 28921b62e642..e9297c25d4ef 100644 --- a/drivers/iio/dac/ad5504.c +++ b/drivers/iio/dac/ad5504.c @@ -187,9 +187,9 @@ static ssize_t ad5504_write_dac_powerdown(struct iio_dev *indio_dev, return ret; if (pwr_down) - st->pwr_down_mask |= (1 << chan->channel); - else st->pwr_down_mask &= ~(1 << chan->channel); + else + st->pwr_down_mask |= (1 << chan->channel); ret = ad5504_spi_write(st, AD5504_ADDR_CTRL, AD5504_DAC_PWRDWN_MODE(st->pwr_down_mode) |
The power-down mask of the ad5504 is actually a power-up mask. Meaning if a bit is set the corresponding channel is powered up and if it is not set the channel is powered down. The driver currently has this the wrong way around, resulting in the channel being powered up when requested to be powered down and vice versa. Fixes: 3bbbf150ffde ("staging:iio:dac:ad5504: Use strtobool for boolean values") Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> --- Stumbled upon this while looking at something else. It is untested, but I think it should be right. Alex can you double check? --- drivers/iio/dac/ad5504.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)