Skip to contentSkip to conversation

staging: iio: ad7780: correct driver read

PATCH In review Renato Lui Geh 3 participants 9 messages Nov 5, 2018 – Nov 16, 2018

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

v4 cover staging: iio: ad7780: correct driver read

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(-)
Signature · 1 line
2.19.1
#1 lore raw

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

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
Changes in v4:
- returns error when voltage_uv is negative

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

1 file changed, 5 insertions(+), 1 deletion(-)
drivers/staging/iio/adc/ad7780.c +0 -0
index b67412db0318..c7cb05cedbbc 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 < 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:
Signature · 1 line
2.19.1
#2 lore raw

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

T24gTW9uLCAyMDE4LTExLTA1IGF0IDE3OjE0IC0wMjAwLCBSZW5hdG8gTHVpIEdlaCB3cm90ZToN Cj4gVGhlIGFkNzc4MCBkcml2ZXIgcHJldmlvdXNseSBkaWQgbm90IHJlYWQgdGhlIGNvcnJlY3Qg ZGV2aWNlIG91dHB1dCwgYXMNCj4gaXQgcmVhZCBhbiBvdXRkYXRlZCB2YWx1ZSBzZXQgYXQgaW5p dGlhbGl6YXRpb24uIEl0IG5vdyB1cGRhdGVzIGl0cw0KPiB2b2x0YWdlIG9uIHJlYWQuDQo+IA0K DQpMb29rcyBnb29kIGZyb20gbXkgc2lkZS4NCg0KQWxleA0KDQo+IFNpZ25lZC1vZmYtYnk6IFJl bmF0byBMdWkgR2VoIDxyZW5hdG9nZWhAZ21haWwuY29tPg0KPiAtLS0NCj4gQ2hhbmdlcyBpbiB2 MzoNCj4gCS0gcmVtb3ZlZCBpbml0aWFsaXphdGlvbiAoaW50IHZvbHRhZ2VfdXYgPSAwKQ0KPiAJ LSByZXR1cm5zIGVycm9yIHdoZW4gdm9sdGFnZV91diBpcyBudWxsDQo+IENoYW5nZXMgaW4gdjQ6 DQo+IAktIHJldHVybnMgZXJyb3Igd2hlbiB2b2x0YWdlX3V2IGlzIG5lZ2F0aXZlDQo+IA0KPiAg ZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMgfCA2ICsrKysrLQ0KPiAgMSBmaWxlIGNo YW5nZWQsIDUgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQ0KPiANCj4gZGlmZiAtLWdpdCBh L2RyaXZlcnMvc3RhZ2luZy9paW8vYWRjL2FkNzc4MC5jDQo+IGIvZHJpdmVycy9zdGFnaW5nL2lp by9hZGMvYWQ3NzgwLmMNCj4gaW5kZXggYjY3NDEyZGIwMzE4Li5jN2NiMDVjZWRiYmMgMTAwNjQ0 DQo+IC0tLSBhL2RyaXZlcnMvc3RhZ2luZy9paW8vYWRjL2FkNzc4MC5jDQo+ICsrKyBiL2RyaXZl cnMvc3RhZ2luZy9paW8vYWRjL2FkNzc4MC5jDQo+IEBAIC04NywxMiArODcsMTYgQEAgc3RhdGlj IGludCBhZDc3ODBfcmVhZF9yYXcoc3RydWN0IGlpb19kZXYgKmluZGlvX2RldiwNCj4gIAkJCSAg IGxvbmcgbSkNCj4gIHsNCj4gIAlzdHJ1Y3QgYWQ3NzgwX3N0YXRlICpzdCA9IGlpb19wcml2KGlu ZGlvX2Rldik7DQo+ICsJaW50IHZvbHRhZ2VfdXY7DQo+ICANCj4gIAlzd2l0Y2ggKG0pIHsNCj4g IAljYXNlIElJT19DSEFOX0lORk9fUkFXOg0KPiAgCQlyZXR1cm4gYWRfc2lnbWFfZGVsdGFfc2lu Z2xlX2NvbnZlcnNpb24oaW5kaW9fZGV2LCBjaGFuLA0KPiB2YWwpOw0KPiAgCWNhc2UgSUlPX0NI QU5fSU5GT19TQ0FMRToNCj4gLQkJKnZhbCA9IHN0LT5pbnRfdnJlZl9tdiAqIHN0LT5nYWluOw0K PiArCQl2b2x0YWdlX3V2ID0gcmVndWxhdG9yX2dldF92b2x0YWdlKHN0LT5yZWcpOw0KPiArCQlp ZiAodm9sdGFnZV91diA8IDApDQo+ICsJCQlyZXR1cm4gdm9sdGFnZV91djsNCj4gKwkJKnZhbCA9 ICh2b2x0YWdlX3V2IC8gMTAwMCkgKiBzdC0+Z2FpbjsNCj4gIAkJKnZhbDIgPSBjaGFuLT5zY2Fu X3R5cGUucmVhbGJpdHMgLSAxOw0KPiAgCQlyZXR1cm4gSUlPX1ZBTF9GUkFDVElPTkFMX0xPRzI7 DQo+ICAJY2FzZSBJSU9fQ0hBTl9JTkZPX09GRlNFVDoNCg==

maintainer
#3 lore raw

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

Quoted 6 lines · Alexandru
Quoted 4 lines · 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.

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.

Quoted 1 line

Looks good from my side.

Ack?

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

Thanks,

Jonathan

Quoted 38 lines

Alex

Quoted 35 lines

---
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(-)
drivers/staging/iio/adc/ad7780.c +5 -1
index b67412db0318..c7cb05cedbbc 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 < 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:

#4 lore raw

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

T24gU3VuLCAyMDE4LTExLTExIGF0IDE0OjMwICswMDAwLCBKb25hdGhhbiBDYW1lcm9uIHdyb3Rl Og0KPiBPbiBUdWUsIDYgTm92IDIwMTggMDk6MjQ6NDQgKzAwMDANCj4gIkFyZGVsZWFuLCBBbGV4 YW5kcnUiIDxhbGV4YW5kcnUuQXJkZWxlYW5AYW5hbG9nLmNvbT4gd3JvdGU6DQo+IA0KPiA+IE9u IE1vbiwgMjAxOC0xMS0wNSBhdCAxNzoxNCAtMDIwMCwgUmVuYXRvIEx1aSBHZWggd3JvdGU6DQo+ ID4gPiBUaGUgYWQ3NzgwIGRyaXZlciBwcmV2aW91c2x5IGRpZCBub3QgcmVhZCB0aGUgY29ycmVj dCBkZXZpY2Ugb3V0cHV0LA0KPiA+ID4gYXMNCj4gPiA+IGl0IHJlYWQgYW4gb3V0ZGF0ZWQgdmFs dWUgc2V0IGF0IGluaXRpYWxpemF0aW9uLiBJdCBub3cgdXBkYXRlcyBpdHMNCj4gPiA+IHZvbHRh Z2Ugb24gcmVhZC4NCj4gPiA+ICAgDQo+IA0KPiBBcHBsaWVkIHRvIHRoZSB0b2dyZWcgYnJhbmNo IG9mIGlpby5naXQgYW5kIHB1c2hlZCBvdXQgYXMgdGVzdGluZyBmb3INCj4gdGhlIGF1dG9idWls ZGVycyB0byBjaGVjayBpdC4gIEknbSBub3QgdGhhdCBmdXNzZWQgYWJvdXQgcHVzaGluZyB0aGlz DQo+IG9uZSBhcyBhIGZpeCwgYXMgbW9zdCBvZiB0aGUgdGltZSB0aGVzZSB0aGluZ3MgYXJlIG9u IGZpeGVkIHJlZ3VsYXRvcnMNCj4gYW55d2F5LCBidXQgaXQgaXMgbmljZSB0byBkbyBpdCByaWdo dC4NCj4gDQo+ID4gTG9va3MgZ29vZCBmcm9tIG15IHNpZGUuDQo+IA0KPiBBY2s/DQoNCkFja2Vk LWJ5OiBBbGV4YW5kcnUgQXJkZWxlYW4gPGFsZXhhbmRydS5hcmRlbGVhbkBhbmFsb2cuY29tPg0K DQo+IA0KPiBNdWNoIHByZWZlcnJlZCBpZiB5b3UgYXJlIHdpbGxpbmcgdG8gZ2l2ZSBhIGZvcm1h bCBhY2tub3dsZWRnZW1lbnQuDQo+IA0KPiBUaGFua3MsDQo+IA0KPiBKb25hdGhhbg0KPiANCj4g DQo+ID4gDQo+ID4gQWxleA0KPiA+IA0KPiA+ID4gU2lnbmVkLW9mZi1ieTogUmVuYXRvIEx1aSBH ZWggPHJlbmF0b2dlaEBnbWFpbC5jb20+DQo+ID4gPiAtLS0NCj4gPiA+IENoYW5nZXMgaW4gdjM6 DQo+ID4gPiAJLSByZW1vdmVkIGluaXRpYWxpemF0aW9uIChpbnQgdm9sdGFnZV91diA9IDApDQo+ ID4gPiAJLSByZXR1cm5zIGVycm9yIHdoZW4gdm9sdGFnZV91diBpcyBudWxsDQo+ID4gPiBDaGFu Z2VzIGluIHY0Og0KPiA+ID4gCS0gcmV0dXJucyBlcnJvciB3aGVuIHZvbHRhZ2VfdXYgaXMgbmVn YXRpdmUNCj4gPiA+IA0KPiA+ID4gIGRyaXZlcnMvc3RhZ2luZy9paW8vYWRjL2FkNzc4MC5jIHwg NiArKysrKy0NCj4gPiA+ICAxIGZpbGUgY2hhbmdlZCwgNSBpbnNlcnRpb25zKCspLCAxIGRlbGV0 aW9uKC0pDQo+ID4gPiANCj4gPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3N0YWdpbmcvaWlvL2Fk Yy9hZDc3ODAuYw0KPiA+ID4gYi9kcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiA+ ID4gaW5kZXggYjY3NDEyZGIwMzE4Li5jN2NiMDVjZWRiYmMgMTAwNjQ0DQo+ID4gPiAtLS0gYS9k cml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiA+ID4gKysrIGIvZHJpdmVycy9zdGFn aW5nL2lpby9hZGMvYWQ3NzgwLmMNCj4gPiA+IEBAIC04NywxMiArODcsMTYgQEAgc3RhdGljIGlu dCBhZDc3ODBfcmVhZF9yYXcoc3RydWN0IGlpb19kZXYNCj4gPiA+ICppbmRpb19kZXYsDQo+ID4g PiAgCQkJICAgbG9uZyBtKQ0KPiA+ID4gIHsNCj4gPiA+ICAJc3RydWN0IGFkNzc4MF9zdGF0ZSAq c3QgPSBpaW9fcHJpdihpbmRpb19kZXYpOw0KPiA+ID4gKwlpbnQgdm9sdGFnZV91djsNCj4gPiA+ ICANCj4gPiA+ICAJc3dpdGNoIChtKSB7DQo+ID4gPiAgCWNhc2UgSUlPX0NIQU5fSU5GT19SQVc6 DQo+ID4gPiAgCQlyZXR1cm4gYWRfc2lnbWFfZGVsdGFfc2luZ2xlX2NvbnZlcnNpb24oaW5kaW9f ZGV2LCBjaGFuLA0KPiA+ID4gdmFsKTsNCj4gPiA+ICAJY2FzZSBJSU9fQ0hBTl9JTkZPX1NDQUxF Og0KPiA+ID4gLQkJKnZhbCA9IHN0LT5pbnRfdnJlZl9tdiAqIHN0LT5nYWluOw0KPiA+ID4gKwkJ dm9sdGFnZV91diA9IHJlZ3VsYXRvcl9nZXRfdm9sdGFnZShzdC0+cmVnKTsNCj4gPiA+ICsJCWlm ICh2b2x0YWdlX3V2IDwgMCkNCj4gPiA+ICsJCQlyZXR1cm4gdm9sdGFnZV91djsNCj4gPiA+ICsJ CSp2YWwgPSAodm9sdGFnZV91diAvIDEwMDApICogc3QtPmdhaW47DQo+ID4gPiAgCQkqdmFsMiA9 IGNoYW4tPnNjYW5fdHlwZS5yZWFsYml0cyAtIDE7DQo+ID4gPiAgCQlyZXR1cm4gSUlPX1ZBTF9G UkFDVElPTkFMX0xPRzI7DQo+ID4gPiAgCWNhc2UgSUlPX0NIQU5fSU5GT19PRkZTRVQ6ICANCj4g DQo+IA0K

maintainer
#5 lore raw

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

Quoted 16 lines · Alexandru
Quoted 15 lines · Jonathan Cameron
Quoted 6 lines · Alexandru
Quoted 5 lines · 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.

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.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

Looks good from my side.

Ack?

Added to both in this series.

Thanks,

Jonathan

Quoted 50 lines
Quoted 50 lines

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

Thanks,

Jonathan

Quoted 39 lines

Alex

Quoted 36 lines

---
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(-)
drivers/staging/iio/adc/ad7780.c +5 -1
index b67412db0318..c7cb05cedbbc 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 < 0)
+			return voltage_uv;
+		*val = (voltage_uv / 1000) * st->gain;
 		*val2 = chan->scan_type.realbits - 1;
drivers/staging/iio/adc/ad7780.c +0 -0
 		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:    
#6 lore raw

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

In reply to Renato Lui Geh

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
Changes in v4:
- removed voltage reading on probe

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

1 file changed, 1 insertion(+), 8 deletions(-)
drivers/staging/iio/adc/ad7780.c +0 -0
index c7cb05cedbbc..52a914360574 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;
};
@@ -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;
Signature · 1 line
2.19.1
#7 lore raw

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

Quoted 3 lines · Renato Lui Geh

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

Looks good from my side.

Alex

Quoted 50 lines

---
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(-)
drivers/staging/iio/adc/ad7780.c +1 -8
index c7cb05cedbbc..52a914360574 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;
 };
@@ -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;

maintainer
#8 lore raw

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

Quoted 6 lines · Alexandru
Quoted 3 lines · Renato Lui Geh

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

Quoted 53 lines

Alex

Quoted 50 lines

---
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(-)
drivers/staging/iio/adc/ad7780.c +1 -8
index c7cb05cedbbc..52a914360574 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;
 };
@@ -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;