[PATCH v3 0/3] staging: iio: ad7780: correct driver read

[PATCH v3 0/3] staging: iio: ad7780: correct driver read

[Renato Lui Geh]

The purpose of this series is to correct two issues in the driver's raw
read function and remove unnecessary struct fields.

Changelog:
*v2
	- separated original patch into two patches
	  (https://marc.info/?l=linux-iio&m=154047435605492)
*v3
	- reordered patches so that fixes go first
	- removed unnecessary initialization
	- removed unnecessary voltage field variable
	- dropped reading voltage on probe
	- returns -EINVAL error on null voltage

Renato Lui Geh (3):
  staging: iio: ad7780: fix offset read value
  staging: iio: ad7780: update voltage on read
  staging: iio: ad7780: remove unnecessary stashed voltage value

 drivers/staging/iio/adc/ad7780.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.19.1

[PATCH v3 1/3] staging: iio: ad7780: fix offset read value

[Renato Lui Geh]

Variable val subtracted an uninitialized value on IIO_CHAN_INFO_OFFSET.
This was fixed by assigning the correct value instead.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
 drivers/staging/iio/adc/ad7780.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index b67412db0318..91e016d534ed 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -96,7 +96,7 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
 		*val2 = chan->scan_type.realbits - 1;
 		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
-		*val -= (1 << (chan->scan_type.realbits - 1));
+		*val = -(1 << (chan->scan_type.realbits - 1));
 		return IIO_VAL_INT;
 	}
 
-- 
2.19.1

[PATCH v3 2/3] staging: iio: ad7780: update voltage on read

[Renato Lui Geh]

The ad7780 driver previously did not read the correct device output, as
it read an outdated value set at initialization. It now updates its
voltage on read.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
Changes in v3:
	- removed initialization (int voltage_uv = 0)
	- returns error when voltage_uv is null

 drivers/staging/iio/adc/ad7780.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 91e016d534ed..f2a11e9424cd 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
 			   long m)
 {
 	struct ad7780_state *st = iio_priv(indio_dev);
+	int voltage_uv;
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
 		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
 	case IIO_CHAN_INFO_SCALE:
-		*val = st->int_vref_mv * st->gain;
+		voltage_uv = regulator_get_voltage(st->reg);
+		if (!voltage_uv)
+			return -EINVAL;
+		*val = (voltage_uv / 1000) * st->gain;
 		*val2 = chan->scan_type.realbits - 1;
 		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
-- 
2.19.1

[PATCH v3 3/3] staging: iio: ad7780: remove unnecessary stashed voltage value

[Renato Lui Geh]

This patch removes the unnecessary field int_vref_mv in ad7780_state
referring to the device's voltage.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
Changes in v3:
	- removed unnecessary int_vref_mv from ad7780_state

 drivers/staging/iio/adc/ad7780.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index f2a11e9424cd..f250370efcf7 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -42,7 +42,6 @@ struct ad7780_state {
 	struct regulator		*reg;
 	struct gpio_desc		*powerdown_gpio;
 	unsigned int	gain;
-	u16				int_vref_mv;
 
 	struct ad_sigma_delta sd;
 };
@@ -190,9 +189,7 @@ static int ad7780_probe(struct spi_device *spi)
 	st->chip_info =
 		&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
 
-	if (voltage_uv)
-		st->int_vref_mv = voltage_uv / 1000;
-	else
+	if (!voltage_uv)
 		dev_warn(&spi->dev, "Reference voltage unspecified\n");
 
 	spi_set_drvdata(spi, indio_dev);
-- 
2.19.1

Re: [PATCH v3 1/3] staging: iio: ad7780: fix offset read value

[Ardelean, Alexandru]

Good catch.

Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> Variable val subtracted an uninitialized value on IIO_CHAN_INFO_OFFSET.
> This was fixed by assigning the correct value instead.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7780.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index b67412db0318..91e016d534ed 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -96,7 +96,7 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>  		*val2 = chan->scan_type.realbits - 1;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  	case IIO_CHAN_INFO_OFFSET:
> -		*val -= (1 << (chan->scan_type.realbits - 1));
> +		*val = -(1 << (chan->scan_type.realbits - 1));
>  		return IIO_VAL_INT;
>  	}
>  

Re: [PATCH v3 2/3] staging: iio: ad7780: update voltage on read

[Ardelean, Alexandru]

On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> The ad7780 driver previously did not read the correct device output, as
> it read an outdated value set at initialization. It now updates its
> voltage on read.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> ---
> Changes in v3:
> 	- removed initialization (int voltage_uv = 0)
> 	- returns error when voltage_uv is null
> 
>  drivers/staging/iio/adc/ad7780.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..f2a11e9424cd 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>  			   long m)
>  {
>  	struct ad7780_state *st = iio_priv(indio_dev);
> +	int voltage_uv;
>  
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
>  		return ad_sigma_delta_single_conversion(indio_dev, chan,
> val);
>  	case IIO_CHAN_INFO_SCALE:
> -		*val = st->int_vref_mv * st->gain;
> +		voltage_uv = regulator_get_voltage(st->reg);
> +		if (!voltage_uv)

This looks wrong.
I admit this was done in the same way in the probe function, but that looks
a bit wrong as well.

Typically, the return value of `regulator_get_voltage()` would get checked
with:

ret = regulator_get_voltage(st->reg);
if (ret < 0)
   return ret;
*val = ret / 1000;

So, negative values are errors and zero & positive values are valid voltage
values.

> +			return -EINVAL;
> +		*val = (voltage_uv / 1000) * st->gain;
>  		*val2 = chan->scan_type.realbits - 1;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  	case IIO_CHAN_INFO_OFFSET:

Re: [PATCH v3 3/3] staging: iio: ad7780: remove unnecessary stashed voltage value

[Ardelean, Alexandru]

On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> This patch removes the unnecessary field int_vref_mv in ad7780_state
> referring to the device's voltage.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> ---
> Changes in v3:
> 	- removed unnecessary int_vref_mv from ad7780_state
> 
>  drivers/staging/iio/adc/ad7780.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index f2a11e9424cd..f250370efcf7 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -42,7 +42,6 @@ struct ad7780_state {
>  	struct regulator		*reg;
>  	struct gpio_desc		*powerdown_gpio;
>  	unsigned int	gain;
> -	u16				int_vref_mv;
>  
>  	struct ad_sigma_delta sd;
>  };
> @@ -190,9 +189,7 @@ static int ad7780_probe(struct spi_device *spi)
>  	st->chip_info =
>  		&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
>  
> -	if (voltage_uv)
> -		st->int_vref_mv = voltage_uv / 1000;
> -	else
> +	if (!voltage_uv)
>  		dev_warn(&spi->dev, "Reference voltage unspecified\n");

I think you could remove this altogether, and also remove the entire
`voltage_uv = regulator_get_voltage(st->reg);` assignment.

It doesn't make much sense to read the voltage here, since it won't be used
in the probe part anymore.

>  
>  	spi_set_drvdata(spi, indio_dev);

Re: [PATCH v3 1/3] staging: iio: ad7780: fix offset read value

[Jonathan Cameron]

On Thu, 1 Nov 2018 15:02:32 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> Good catch.
> 
> Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
On the basis this has been broken for a long time, and you are clearly
doing other nearby not fix work, I'm going to take this through the togreg
tree rather than via the quicker fix path.  It makes my life
easier :)

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> 
> On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> > Variable val subtracted an uninitialized value on IIO_CHAN_INFO_OFFSET.
> > This was fixed by assigning the correct value instead.
> > 
> > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > ---
> >  drivers/staging/iio/adc/ad7780.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index b67412db0318..91e016d534ed 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -96,7 +96,7 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> >  		*val2 = chan->scan_type.realbits - 1;
> >  		return IIO_VAL_FRACTIONAL_LOG2;
> >  	case IIO_CHAN_INFO_OFFSET:
> > -		*val -= (1 << (chan->scan_type.realbits - 1));
> > +		*val = -(1 << (chan->scan_type.realbits - 1));
> >  		return IIO_VAL_INT;
> >  	}
> >    

Re: [PATCH v3 2/3] staging: iio: ad7780: update voltage on read

[Jonathan Cameron]

On Thu, 1 Nov 2018 15:20:55 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> > The ad7780 driver previously did not read the correct device output, as
> > it read an outdated value set at initialization. It now updates its
> > voltage on read.
> > 
> > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > ---
> > Changes in v3:
> > 	- removed initialization (int voltage_uv = 0)
> > 	- returns error when voltage_uv is null
> > 
> >  drivers/staging/iio/adc/ad7780.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index 91e016d534ed..f2a11e9424cd 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> >  			   long m)
> >  {
> >  	struct ad7780_state *st = iio_priv(indio_dev);
> > +	int voltage_uv;
> >  
> >  	switch (m) {
> >  	case IIO_CHAN_INFO_RAW:
> >  		return ad_sigma_delta_single_conversion(indio_dev, chan,
> > val);
> >  	case IIO_CHAN_INFO_SCALE:
> > -		*val = st->int_vref_mv * st->gain;
> > +		voltage_uv = regulator_get_voltage(st->reg);
> > +		if (!voltage_uv)  
> 
> This looks wrong.
> I admit this was done in the same way in the probe function, but that looks
> a bit wrong as well.
> 
> Typically, the return value of `regulator_get_voltage()` would get checked
> with:
> 
> ret = regulator_get_voltage(st->reg);
> if (ret < 0)
>    return ret;
> *val = ret / 1000;
> 
> So, negative values are errors and zero & positive values are valid voltage
> values.
This one is a stylistic choice for readability. I ever so slightly
prefer how Alex has it but don't care enough that I'd have commented on it ;)

However, nice to tidy up as you'll be respinning patch 3 anyway!

Thanks,

Jonathan

> 
> > +			return -EINVAL;
> > +		*val = (voltage_uv / 1000) * st->gain;
> >  		*val2 = chan->scan_type.realbits - 1;
> >  		return IIO_VAL_FRACTIONAL_LOG2;
> >  	case IIO_CHAN_INFO_OFFSET:  

Re: [PATCH v3 3/3] staging: iio: ad7780: remove unnecessary stashed voltage value

[Jonathan Cameron]

On Thu, 1 Nov 2018 15:28:19 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> > This patch removes the unnecessary field int_vref_mv in ad7780_state
> > referring to the device's voltage.
> > 
> > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > ---
> > Changes in v3:
> > 	- removed unnecessary int_vref_mv from ad7780_state
> > 
> >  drivers/staging/iio/adc/ad7780.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index f2a11e9424cd..f250370efcf7 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -42,7 +42,6 @@ struct ad7780_state {
> >  	struct regulator		*reg;
> >  	struct gpio_desc		*powerdown_gpio;
> >  	unsigned int	gain;
> > -	u16				int_vref_mv;
> >  
> >  	struct ad_sigma_delta sd;
> >  };
> > @@ -190,9 +189,7 @@ static int ad7780_probe(struct spi_device *spi)
> >  	st->chip_info =
> >  		&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
> >  
> > -	if (voltage_uv)
> > -		st->int_vref_mv = voltage_uv / 1000;
> > -	else
> > +	if (!voltage_uv)
> >  		dev_warn(&spi->dev, "Reference voltage unspecified\n");  
> 
> I think you could remove this altogether, and also remove the entire
> `voltage_uv = regulator_get_voltage(st->reg);` assignment.
> 
> It doesn't make much sense to read the voltage here, since it won't be used
> in the probe part anymore.
> 
Absolutely agree on this. There is no point in reading here at all.

Jonathan
> >  
> >  	spi_set_drvdata(spi, indio_dev);  

Re: [PATCH v3 1/3] staging: iio: ad7780: fix offset read value

[Renato Lui Geh]

Hi,

>On Thu, 1 Nov 2018 15:02:32 +0000
>"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
>
>> Good catch.

That was actually Jonathan's catch. :)

>> Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

I read up on Acked-by on the kernel docs, as I didn't know exactly what
it meant, but I'm not so sure on how to proceed once the patch has been
acked.  For future patches, do I add this Acked-by line on the patch's
message body? Or is this just an informal way of approval?

>On the basis this has been broken for a long time, and you are clearly
>doing other nearby not fix work, I'm going to take this through the togreg
>tree rather than via the quicker fix path.  It makes my life
>easier :)
>
>Applied to the togreg branch of iio.git and pushed out as testing for
>the autobuilders to play with it.

So this means I should skip this patch on v4, right?

Thanks,
Renato

Re: [PATCH v3 2/3] staging: iio: ad7780: update voltage on read

[Renato Lui Geh]

On Thu, 1 Nov 2018 15:20:55 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
>
> This looks wrong.
> I admit this was done in the same way in the probe function, but that looks
> a bit wrong as well.
>
> Typically, the return value of `regulator_get_voltage()` would get checked
> with:
>
> ret = regulator_get_voltage(st->reg);
> if (ret < 0)
>    return ret;
> *val = ret / 1000;
>
> So, negative values are errors and zero & positive values are valid voltage
> values.

I see. So -EINVAL is only used when sent the wrong info type?

Re: [PATCH v3 2/3] staging: iio: ad7780: update voltage on read

[Jonathan Cameron]

On Sat, 3 Nov 2018 13:06:19 -0300
Renato Lui Geh <renatogeh@gmail.com> wrote:

> On Thu, 1 Nov 2018 15:20:55 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> >
> > This looks wrong.
> > I admit this was done in the same way in the probe function, but that looks
> > a bit wrong as well.
> >
> > Typically, the return value of `regulator_get_voltage()` would get checked
> > with:
> >
> > ret = regulator_get_voltage(st->reg);
> > if (ret < 0)
> >    return ret;
> > *val = ret / 1000;
> >
> > So, negative values are errors and zero & positive values are valid voltage
> > values.  
> 
> I see. So -EINVAL is only used when sent the wrong info type?
yes. I actually misread what was there and thought we were just talking
about using a ret variable rather than returning the error via your
local variable.

Definitely want to pass on the error from regulator_get_voltage as
it may have more meaning than a simple -EINVAL.

Thanks,

Jonathan

Re: [PATCH v3 1/3] staging: iio: ad7780: fix offset read value

[Jonathan Cameron]

On Sat, 3 Nov 2018 12:59:16 -0300
Renato Lui Geh <renatogeh@gmail.com> wrote:

> Hi,
> 
> >On Thu, 1 Nov 2018 15:02:32 +0000
> >"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> >  
> >> Good catch.  
> 
> That was actually Jonathan's catch. :)
> 
> >> Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> 
> I read up on Acked-by on the kernel docs, as I didn't know exactly what
> it meant, but I'm not so sure on how to proceed once the patch has been
> acked.  For future patches, do I add this Acked-by line on the patch's
> message body? Or is this just an informal way of approval?
It's formal.  You put the line directly below your Signed-off-by: line
if you are resending.  If I pick up the patch I paste it in there
whilst applying.

> 
> >On the basis this has been broken for a long time, and you are clearly
> >doing other nearby not fix work, I'm going to take this through the togreg
> >tree rather than via the quicker fix path.  It makes my life
> >easier :)
> >
> >Applied to the togreg branch of iio.git and pushed out as testing for
> >the autobuilders to play with it.  
> 
> So this means I should skip this patch on v4, right?
Yes. Already in the tree so don't send it again.

Thanks

Jonathan

> 
> Thanks,
> Renato
>