[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 1/3] staging: iio: ad7780: fix offset read value

[Ardelean, Alexandru]

R29vZCBjYXRjaC4NCg0KQWNrZWQtYnk6IEFsZXhhbmRydSBBcmRlbGVhbiA8YWxleGFuZHJ1LmFy
ZGVsZWFuQGFuYWxvZy5jb20+DQoNCk9uIFRodSwgMjAxOC0xMS0wMSBhdCAxMTo0MyAtMDMwMCwg
UmVuYXRvIEx1aSBHZWggd3JvdGU6DQo+IFZhcmlhYmxlIHZhbCBzdWJ0cmFjdGVkIGFuIHVuaW5p
dGlhbGl6ZWQgdmFsdWUgb24gSUlPX0NIQU5fSU5GT19PRkZTRVQuDQo+IFRoaXMgd2FzIGZpeGVk
IGJ5IGFzc2lnbmluZyB0aGUgY29ycmVjdCB2YWx1ZSBpbnN0ZWFkLg0KPiANCj4gU2lnbmVkLW9m
Zi1ieTogUmVuYXRvIEx1aSBHZWggPHJlbmF0b2dlaEBnbWFpbC5jb20+DQo+IC0tLQ0KPiAgZHJp
dmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMgfCAyICstDQo+ICAxIGZpbGUgY2hhbmdlZCwg
MSBpbnNlcnRpb24oKyksIDEgZGVsZXRpb24oLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9kcml2ZXJz
L3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiBiL2RyaXZlcnMvc3RhZ2luZy9paW8vYWRjL2Fk
Nzc4MC5jDQo+IGluZGV4IGI2NzQxMmRiMDMxOC4uOTFlMDE2ZDUzNGVkIDEwMDY0NA0KPiAtLS0g
YS9kcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiArKysgYi9kcml2ZXJzL3N0YWdp
bmcvaWlvL2FkYy9hZDc3ODAuYw0KPiBAQCAtOTYsNyArOTYsNyBAQCBzdGF0aWMgaW50IGFkNzc4
MF9yZWFkX3JhdyhzdHJ1Y3QgaWlvX2RldiAqaW5kaW9fZGV2LA0KPiAgCQkqdmFsMiA9IGNoYW4t
PnNjYW5fdHlwZS5yZWFsYml0cyAtIDE7DQo+ICAJCXJldHVybiBJSU9fVkFMX0ZSQUNUSU9OQUxf
TE9HMjsNCj4gIAljYXNlIElJT19DSEFOX0lORk9fT0ZGU0VUOg0KPiAtCQkqdmFsIC09ICgxIDw8
IChjaGFuLT5zY2FuX3R5cGUucmVhbGJpdHMgLSAxKSk7DQo+ICsJCSp2YWwgPSAtKDEgPDwgKGNo
YW4tPnNjYW5fdHlwZS5yZWFsYml0cyAtIDEpKTsNCj4gIAkJcmV0dXJuIElJT19WQUxfSU5UOw0K
PiAgCX0NCj4gIA0K

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 2/3] staging: iio: ad7780: update voltage on read

[Ardelean, Alexandru]

T24gVGh1LCAyMDE4LTExLTAxIGF0IDExOjQzIC0wMzAwLCBSZW5hdG8gTHVpIEdlaCB3cm90ZToN
Cj4gVGhlIGFkNzc4MCBkcml2ZXIgcHJldmlvdXNseSBkaWQgbm90IHJlYWQgdGhlIGNvcnJlY3Qg
ZGV2aWNlIG91dHB1dCwgYXMNCj4gaXQgcmVhZCBhbiBvdXRkYXRlZCB2YWx1ZSBzZXQgYXQgaW5p
dGlhbGl6YXRpb24uIEl0IG5vdyB1cGRhdGVzIGl0cw0KPiB2b2x0YWdlIG9uIHJlYWQuDQo+IA0K
PiBTaWduZWQtb2ZmLWJ5OiBSZW5hdG8gTHVpIEdlaCA8cmVuYXRvZ2VoQGdtYWlsLmNvbT4NCj4g
LS0tDQo+IENoYW5nZXMgaW4gdjM6DQo+IAktIHJlbW92ZWQgaW5pdGlhbGl6YXRpb24gKGludCB2
b2x0YWdlX3V2ID0gMCkNCj4gCS0gcmV0dXJucyBlcnJvciB3aGVuIHZvbHRhZ2VfdXYgaXMgbnVs
bA0KPiANCj4gIGRyaXZlcnMvc3RhZ2luZy9paW8vYWRjL2FkNzc4MC5jIHwgNiArKysrKy0NCj4g
IDEgZmlsZSBjaGFuZ2VkLCA1IGluc2VydGlvbnMoKyksIDEgZGVsZXRpb24oLSkNCj4gDQo+IGRp
ZmYgLS1naXQgYS9kcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiBiL2RyaXZlcnMv
c3RhZ2luZy9paW8vYWRjL2FkNzc4MC5jDQo+IGluZGV4IDkxZTAxNmQ1MzRlZC4uZjJhMTFlOTQy
NGNkIDEwMDY0NA0KPiAtLS0gYS9kcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiAr
KysgYi9kcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiBAQCAtODcsMTIgKzg3LDE2
IEBAIHN0YXRpYyBpbnQgYWQ3NzgwX3JlYWRfcmF3KHN0cnVjdCBpaW9fZGV2ICppbmRpb19kZXYs
DQo+ICAJCQkgICBsb25nIG0pDQo+ICB7DQo+ICAJc3RydWN0IGFkNzc4MF9zdGF0ZSAqc3QgPSBp
aW9fcHJpdihpbmRpb19kZXYpOw0KPiArCWludCB2b2x0YWdlX3V2Ow0KPiAgDQo+ICAJc3dpdGNo
IChtKSB7DQo+ICAJY2FzZSBJSU9fQ0hBTl9JTkZPX1JBVzoNCj4gIAkJcmV0dXJuIGFkX3NpZ21h
X2RlbHRhX3NpbmdsZV9jb252ZXJzaW9uKGluZGlvX2RldiwgY2hhbiwNCj4gdmFsKTsNCj4gIAlj
YXNlIElJT19DSEFOX0lORk9fU0NBTEU6DQo+IC0JCSp2YWwgPSBzdC0+aW50X3ZyZWZfbXYgKiBz
dC0+Z2FpbjsNCj4gKwkJdm9sdGFnZV91diA9IHJlZ3VsYXRvcl9nZXRfdm9sdGFnZShzdC0+cmVn
KTsNCj4gKwkJaWYgKCF2b2x0YWdlX3V2KQ0KDQpUaGlzIGxvb2tzIHdyb25nLg0KSSBhZG1pdCB0
aGlzIHdhcyBkb25lIGluIHRoZSBzYW1lIHdheSBpbiB0aGUgcHJvYmUgZnVuY3Rpb24sIGJ1dCB0
aGF0IGxvb2tzDQphIGJpdCB3cm9uZyBhcyB3ZWxsLg0KDQpUeXBpY2FsbHksIHRoZSByZXR1cm4g
dmFsdWUgb2YgYHJlZ3VsYXRvcl9nZXRfdm9sdGFnZSgpYCB3b3VsZCBnZXQgY2hlY2tlZA0Kd2l0
aDoNCg0KcmV0ID0gcmVndWxhdG9yX2dldF92b2x0YWdlKHN0LT5yZWcpOw0KaWYgKHJldCA8IDAp
DQogICByZXR1cm4gcmV0Ow0KKnZhbCA9IHJldCAvIDEwMDA7DQoNClNvLCBuZWdhdGl2ZSB2YWx1
ZXMgYXJlIGVycm9ycyBhbmQgemVybyAmIHBvc2l0aXZlIHZhbHVlcyBhcmUgdmFsaWQgdm9sdGFn
ZQ0KdmFsdWVzLg0KDQo+ICsJCQlyZXR1cm4gLUVJTlZBTDsNCj4gKwkJKnZhbCA9ICh2b2x0YWdl
X3V2IC8gMTAwMCkgKiBzdC0+Z2FpbjsNCj4gIAkJKnZhbDIgPSBjaGFuLT5zY2FuX3R5cGUucmVh
bGJpdHMgLSAxOw0KPiAgCQlyZXR1cm4gSUlPX1ZBTF9GUkFDVElPTkFMX0xPRzI7DQo+ICAJY2Fz
ZSBJSU9fQ0hBTl9JTkZPX09GRlNFVDoNCg==

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 3/3] staging: iio: ad7780: remove unnecessary stashed voltage value

[Ardelean, Alexandru]

T24gVGh1LCAyMDE4LTExLTAxIGF0IDExOjQzIC0wMzAwLCBSZW5hdG8gTHVpIEdlaCB3cm90ZToN
Cj4gVGhpcyBwYXRjaCByZW1vdmVzIHRoZSB1bm5lY2Vzc2FyeSBmaWVsZCBpbnRfdnJlZl9tdiBp
biBhZDc3ODBfc3RhdGUNCj4gcmVmZXJyaW5nIHRvIHRoZSBkZXZpY2UncyB2b2x0YWdlLg0KPiAN
Cj4gU2lnbmVkLW9mZi1ieTogUmVuYXRvIEx1aSBHZWggPHJlbmF0b2dlaEBnbWFpbC5jb20+DQo+
IC0tLQ0KPiBDaGFuZ2VzIGluIHYzOg0KPiAJLSByZW1vdmVkIHVubmVjZXNzYXJ5IGludF92cmVm
X212IGZyb20gYWQ3NzgwX3N0YXRlDQo+IA0KPiAgZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3
NzgwLmMgfCA1ICstLS0tDQo+ICAxIGZpbGUgY2hhbmdlZCwgMSBpbnNlcnRpb24oKyksIDQgZGVs
ZXRpb25zKC0pDQo+IA0KPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3
NzgwLmMNCj4gYi9kcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiBpbmRleCBmMmEx
MWU5NDI0Y2QuLmYyNTAzNzBlZmNmNyAxMDA2NDQNCj4gLS0tIGEvZHJpdmVycy9zdGFnaW5nL2lp
by9hZGMvYWQ3NzgwLmMNCj4gKysrIGIvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMN
Cj4gQEAgLTQyLDcgKzQyLDYgQEAgc3RydWN0IGFkNzc4MF9zdGF0ZSB7DQo+ICAJc3RydWN0IHJl
Z3VsYXRvcgkJKnJlZzsNCj4gIAlzdHJ1Y3QgZ3Bpb19kZXNjCQkqcG93ZXJkb3duX2dwaW87DQo+
ICAJdW5zaWduZWQgaW50CWdhaW47DQo+IC0JdTE2CQkJCWludF92cmVmX212Ow0KPiAgDQo+ICAJ
c3RydWN0IGFkX3NpZ21hX2RlbHRhIHNkOw0KPiAgfTsNCj4gQEAgLTE5MCw5ICsxODksNyBAQCBz
dGF0aWMgaW50IGFkNzc4MF9wcm9iZShzdHJ1Y3Qgc3BpX2RldmljZSAqc3BpKQ0KPiAgCXN0LT5j
aGlwX2luZm8gPQ0KPiAgCQkmYWQ3NzgwX2NoaXBfaW5mb190Ymxbc3BpX2dldF9kZXZpY2VfaWQo
c3BpKS0+ZHJpdmVyX2RhdGFdOw0KPiAgDQo+IC0JaWYgKHZvbHRhZ2VfdXYpDQo+IC0JCXN0LT5p
bnRfdnJlZl9tdiA9IHZvbHRhZ2VfdXYgLyAxMDAwOw0KPiAtCWVsc2UNCj4gKwlpZiAoIXZvbHRh
Z2VfdXYpDQo+ICAJCWRldl93YXJuKCZzcGktPmRldiwgIlJlZmVyZW5jZSB2b2x0YWdlIHVuc3Bl
Y2lmaWVkXG4iKTsNCg0KSSB0aGluayB5b3UgY291bGQgcmVtb3ZlIHRoaXMgYWx0b2dldGhlciwg
YW5kIGFsc28gcmVtb3ZlIHRoZSBlbnRpcmUNCmB2b2x0YWdlX3V2ID0gcmVndWxhdG9yX2dldF92
b2x0YWdlKHN0LT5yZWcpO2AgYXNzaWdubWVudC4NCg0KSXQgZG9lc24ndCBtYWtlIG11Y2ggc2Vu
c2UgdG8gcmVhZCB0aGUgdm9sdGFnZSBoZXJlLCBzaW5jZSBpdCB3b24ndCBiZSB1c2VkDQppbiB0
aGUgcHJvYmUgcGFydCBhbnltb3JlLg0KDQo+ICANCj4gIAlzcGlfc2V0X2RydmRhdGEoc3BpLCBp
bmRpb19kZXYpOw0K

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
>