staging: iio: ad7780: correct driver read

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

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

---
drivers/staging/iio/adc/ad7780.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

index b67412db0318..91e016d534ed 100644
@@ -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

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.

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

index 91e016d534ed..f2a11e9424cd 100644
@@ -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

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

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

index f2a11e9424cd..f250370efcf7 100644
@@ -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

Good catch.

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

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

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

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

Absolutely agree on this. There is no point in reading here at all.

Jonathan

Hi,

That was actually Jonathan's catch. :)

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?

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

Thanks,
Renato

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

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.

Yes. Already in the tree so don't send it again.

Thanks

Jonathan