[PATCH v4 0/2] staging: iio: ad7780: correct driver read

[PATCH v4 0/2] staging: iio: ad7780: correct driver read

[Renato Lui Geh]

The purpose of this series is to correct an issue in the driver's raw
read function and remove an unnecessary struct field.

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
*v4
	- removed voltage reading from probe
	- fixed voltage error handling

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

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

-- 
2.19.1

[PATCH v4 1/2] 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
Changes in v4:
	- returns error when voltage_uv is negative

 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 b67412db0318..c7cb05cedbbc 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 < 0)
+			return voltage_uv;
+		*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 v4 2/2] 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
Changes in v4:
	- removed voltage reading on probe

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

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index c7cb05cedbbc..52a914360574 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;
 };
@@ -165,7 +164,7 @@ static int ad7780_probe(struct spi_device *spi)
 {
 	struct ad7780_state *st;
 	struct iio_dev *indio_dev;
-	int ret, voltage_uv = 0;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
@@ -185,16 +184,10 @@ static int ad7780_probe(struct spi_device *spi)
 		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
 		return ret;
 	}
-	voltage_uv = regulator_get_voltage(st->reg);
 
 	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
-		dev_warn(&spi->dev, "Reference voltage unspecified\n");
-
 	spi_set_drvdata(spi, indio_dev);
 
 	indio_dev->dev.parent = &spi->dev;
-- 
2.19.1

Re: [PATCH v4 1/2] staging: iio: ad7780: update voltage on read

[Ardelean, Alexandru]

On Mon, 2018-11-05 at 17:14 -0200, 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.
> 

Looks good from my side.

Alex

> 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
> Changes in v4:
> 	- returns error when voltage_uv is negative
> 
>  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 b67412db0318..c7cb05cedbbc 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 < 0)
> +			return voltage_uv;
> +		*val = (voltage_uv / 1000) * st->gain;
>  		*val2 = chan->scan_type.realbits - 1;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  	case IIO_CHAN_INFO_OFFSET:

Re: [PATCH v4 2/2] staging: iio: ad7780: remove unnecessary stashed voltage value

[Ardelean, Alexandru]

On Mon, 2018-11-05 at 17:16 -0200, Renato Lui Geh wrote:
> This patch removes the unnecessary field int_vref_mv in ad7780_state
> referring to the device's voltage.
> 

Looks good from my side.

Alex

> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> ---
> Changes in v3:
> 	- removed unnecessary int_vref_mv from ad7780_state
> Changes in v4:
> 	- removed voltage reading on probe
> 
>  drivers/staging/iio/adc/ad7780.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c7cb05cedbbc..52a914360574 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;
>  };
> @@ -165,7 +164,7 @@ static int ad7780_probe(struct spi_device *spi)
>  {
>  	struct ad7780_state *st;
>  	struct iio_dev *indio_dev;
> -	int ret, voltage_uv = 0;
> +	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>  	if (!indio_dev)
> @@ -185,16 +184,10 @@ static int ad7780_probe(struct spi_device *spi)
>  		dev_err(&spi->dev, "Failed to enable specified AVdd
> supply\n");
>  		return ret;
>  	}
> -	voltage_uv = regulator_get_voltage(st->reg);
>  
>  	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
> -		dev_warn(&spi->dev, "Reference voltage unspecified\n");
> -
>  	spi_set_drvdata(spi, indio_dev);
>  
>  	indio_dev->dev.parent = &spi->dev;

Re: [PATCH v4 1/2] staging: iio: ad7780: update voltage on read

[Jonathan Cameron]

On Tue, 6 Nov 2018 09:24:44 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Mon, 2018-11-05 at 17:14 -0200, 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.
> >   
> 
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to check it.  I'm not that fussed about pushing this
one as a fix, as most of the time these things are on fixed regulators
anyway, but it is nice to do it right.

> Looks good from my side.
Ack?

Much preferred if you are willing to give a formal acknowledgement.

Thanks,

Jonathan


> 
> Alex
> 
> > 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
> > Changes in v4:
> > 	- returns error when voltage_uv is negative
> > 
> >  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 b67412db0318..c7cb05cedbbc 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 < 0)
> > +			return voltage_uv;
> > +		*val = (voltage_uv / 1000) * st->gain;
> >  		*val2 = chan->scan_type.realbits - 1;
> >  		return IIO_VAL_FRACTIONAL_LOG2;
> >  	case IIO_CHAN_INFO_OFFSET:  

Re: [PATCH v4 2/2] staging: iio: ad7780: remove unnecessary stashed voltage value

[Jonathan Cameron]

On Tue, 6 Nov 2018 09:25:27 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Mon, 2018-11-05 at 17:16 -0200, Renato Lui Geh wrote:
> > This patch removes the unnecessary field int_vref_mv in ad7780_state
> > referring to the device's voltage.
> >   
> 
> Looks good from my side.
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> 
> Alex
> 
> > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> > ---
> > Changes in v3:
> > 	- removed unnecessary int_vref_mv from ad7780_state
> > Changes in v4:
> > 	- removed voltage reading on probe
> > 
> >  drivers/staging/iio/adc/ad7780.c | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index c7cb05cedbbc..52a914360574 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;
> >  };
> > @@ -165,7 +164,7 @@ static int ad7780_probe(struct spi_device *spi)
> >  {
> >  	struct ad7780_state *st;
> >  	struct iio_dev *indio_dev;
> > -	int ret, voltage_uv = 0;
> > +	int ret;
> >  
> >  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> >  	if (!indio_dev)
> > @@ -185,16 +184,10 @@ static int ad7780_probe(struct spi_device *spi)
> >  		dev_err(&spi->dev, "Failed to enable specified AVdd
> > supply\n");
> >  		return ret;
> >  	}
> > -	voltage_uv = regulator_get_voltage(st->reg);
> >  
> >  	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
> > -		dev_warn(&spi->dev, "Reference voltage unspecified\n");
> > -
> >  	spi_set_drvdata(spi, indio_dev);
> >  
> >  	indio_dev->dev.parent = &spi->dev;  

Re: [PATCH v4 1/2] staging: iio: ad7780: update voltage on read

[Ardelean, Alexandru]

On Sun, 2018-11-11 at 14:30 +0000, Jonathan Cameron wrote:
> On Tue, 6 Nov 2018 09:24:44 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Mon, 2018-11-05 at 17:14 -0200, 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.
> > >   
> 
> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to check it.  I'm not that fussed about pushing this
> one as a fix, as most of the time these things are on fixed regulators
> anyway, but it is nice to do it right.
> 
> > Looks good from my side.
> 
> Ack?

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

> 
> Much preferred if you are willing to give a formal acknowledgement.
> 
> Thanks,
> 
> Jonathan
> 
> 
> > 
> > Alex
> > 
> > > 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
> > > Changes in v4:
> > > 	- returns error when voltage_uv is negative
> > > 
> > >  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 b67412db0318..c7cb05cedbbc 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 < 0)
> > > +			return voltage_uv;
> > > +		*val = (voltage_uv / 1000) * st->gain;
> > >  		*val2 = chan->scan_type.realbits - 1;
> > >  		return IIO_VAL_FRACTIONAL_LOG2;
> > >  	case IIO_CHAN_INFO_OFFSET:  
> 
> 

Re: [PATCH v4 1/2] staging: iio: ad7780: update voltage on read

[Jonathan Cameron]

On Mon, 12 Nov 2018 07:57:24 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2018-11-11 at 14:30 +0000, Jonathan Cameron wrote:
> > On Tue, 6 Nov 2018 09:24:44 +0000
> > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> >   
> > > On Mon, 2018-11-05 at 17:14 -0200, 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.
> > > >     
> > 
> > Applied to the togreg branch of iio.git and pushed out as testing for
> > the autobuilders to check it.  I'm not that fussed about pushing this
> > one as a fix, as most of the time these things are on fixed regulators
> > anyway, but it is nice to do it right.
> >   
> > > Looks good from my side.  
> > 
> > Ack?  
> 
> Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Added to both in this series.

Thanks,

Jonathan

> 
> > 
> > Much preferred if you are willing to give a formal acknowledgement.
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> >   
> > > 
> > > Alex
> > >   
> > > > 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
> > > > Changes in v4:
> > > > 	- returns error when voltage_uv is negative
> > > > 
> > > >  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 b67412db0318..c7cb05cedbbc 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 < 0)
> > > > +			return voltage_uv;
> > > > +		*val = (voltage_uv / 1000) * st->gain;
> > > >  		*val2 = chan->scan_type.realbits - 1;
> > > >  		return IIO_VAL_FRACTIONAL_LOG2;
> > > >  	case IIO_CHAN_INFO_OFFSET:    
> > 
> >