[PATCH] iio: ad5504: Fix setting power-down state

[PATCH] iio: ad5504: Fix setting power-down state

[Lars-Peter Clausen]

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(-)

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) |
-- 
2.20.1

Re: [PATCH] iio: ad5504: Fix setting power-down state

[Jonathan Cameron]

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) |

Re: [PATCH] iio: ad5504: Fix setting power-down state

[Alexandru Ardelean]

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) |
>

Re: [PATCH] iio: ad5504: Fix setting power-down state

[Jonathan Cameron]

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) |  
> >