linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] staging: iio: ad7780: Add gain & filter gpio support
@ 2018-11-21 18:04 Giuliano Belinassi
  2018-11-22 11:01 ` Popa, Stefan Serban
  0 siblings, 1 reply; 8+ messages in thread
From: Giuliano Belinassi @ 2018-11-21 18:04 UTC (permalink / raw)
  To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh,
	alexandru.Ardelean, stefan.popa
  Cc: linux-iio, devel, linux-kernel, kernel-usp

Previously, the AD7780 driver only supported gpio for the 'powerdown'
pin. This commit adds suppport for the 'gain' and 'filter' pin.

Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
 drivers/staging/iio/adc/ad7780.c       | 61 ++++++++++++++++++++++++--
 include/linux/iio/adc/ad_sigma_delta.h |  5 +++
 2 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index c4a85789c2db..69794f06dbcd 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -39,6 +39,9 @@
 #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
 #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
 
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1
+
 struct ad7780_chip_info {
 	struct iio_chan_spec	channel;
 	unsigned int		pattern_mask;
@@ -50,6 +53,8 @@ struct ad7780_state {
 	const struct ad7780_chip_info	*chip_info;
 	struct regulator		*reg;
 	struct gpio_desc		*powerdown_gpio;
+	struct gpio_desc		*gain_gpio;
+	struct gpio_desc		*filter_gpio;
 	unsigned int	gain;
 
 	struct ad_sigma_delta sd;
@@ -115,18 +120,51 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int ad7780_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long m)
+{
+	struct ad7780_state *st = iio_priv(indio_dev);
+
+	if (m != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	if (st->chip_info->is_ad778x) {
+		switch(val) {
+		case AD7780_GAIN_GPIO:
+			gpiod_set_value(st->gain_gpio, val2);
+		break;
+		case AD7780_FILTER_GPIO:
+			gpiod_set_value(st->filter_gpio, val2);
+		break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 				     unsigned int raw_sample)
 {
 	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
 	const struct ad7780_chip_info *chip_info = st->chip_info;
+	int val;
 
 	if ((raw_sample & AD7780_ERR) ||
 	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
 		return -EIO;
 
 	if (chip_info->is_ad778x) {
-		if (raw_sample & AD7780_GAIN)
+		val = raw_sample & AD7780_GAIN;
+
+		if (val != gpiod_get_value(st->gain_gpio))
+			return -EIO;
+
+		if (val)
 			st->gain = 1;
 		else
 			st->gain = 128;
@@ -141,18 +179,20 @@ static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
 	.has_registers = false,
 };
 
-#define AD7780_CHANNEL(bits, wordsize) \
+#define AD7170_CHANNEL(bits, wordsize) \
 	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
+#define AD7780_CHANNEL(bits, wordsize) \
+	AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
 
 static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 	[ID_AD7170] = {
-		.channel = AD7780_CHANNEL(12, 24),
+		.channel = AD7170_CHANNEL(12, 24),
 		.pattern = AD7170_PATTERN,
 		.pattern_mask = AD7170_PATTERN_MASK,
 		.is_ad778x = false,
 	},
 	[ID_AD7171] = {
-		.channel = AD7780_CHANNEL(16, 24),
+		.channel = AD7170_CHANNEL(16, 24),
 		.pattern = AD7170_PATTERN,
 		.pattern_mask = AD7170_PATTERN_MASK,
 		.is_ad778x = false,
@@ -173,6 +213,7 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 
 static const struct iio_info ad7780_info = {
 	.read_raw = ad7780_read_raw,
+	.write_raw = ad7780_write_raw,
 };
 
 static int ad7780_probe(struct spi_device *spi)
@@ -222,6 +263,18 @@ static int ad7780_probe(struct spi_device *spi)
 		goto error_disable_reg;
 	}
 
+	if (st->chip_info->is_ad778x) {
+		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
+							"gain",
+							GPIOD_OUT_HIGH);
+		if (IS_ERR(st->gain_gpio)) {
+			ret = PTR_ERR(st->gain_gpio);
+			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
+				ret);
+			goto error_disable_reg;
+		}
+	}
+
 	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
 	if (ret)
 		goto error_disable_reg;
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index 730ead1a46df..6cadab6fd5fd 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -173,6 +173,11 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig);
 	__AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
 		_storagebits, _shift, NULL, IIO_VOLTAGE, 0)
 
+#define AD_SD_CHANNEL_GAIN_FILTER(_si, _channel, _address, _bits, \
+	_storagebits, _shift) \
+	__AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
+		_storagebits, _shift, NULL, IIO_VOLTAGE, BIT(IIO_CHAN_INFO_RAW))
+
 #define AD_SD_TEMP_CHANNEL(_si, _address, _bits, _storagebits, _shift) \
 	__AD_SD_CHANNEL(_si, 0, -1, _address, _bits, \
 		_storagebits, _shift, NULL, IIO_TEMP, \
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
  2018-11-21 18:04 [PATCH] staging: iio: ad7780: Add gain & filter gpio support Giuliano Belinassi
@ 2018-11-22 11:01 ` Popa, Stefan Serban
  2018-11-25 11:08   ` Jonathan Cameron
  0 siblings, 1 reply; 8+ messages in thread
From: Popa, Stefan Serban @ 2018-11-22 11:01 UTC (permalink / raw)
  To: Ardelean, Alexandru, lars, knaack.h, jic23, Hennerich, Michael,
	giuliano.belinassi, pmeerw, gregkh
  Cc: linux-kernel, linux-iio, devel, kernel-usp

T24gTWksIDIwMTgtMTEtMjEgYXQgMTY6MDQgLTAyMDAsIEdpdWxpYW5vIEJlbGluYXNzaSB3cm90
ZToNCj4gUHJldmlvdXNseSwgdGhlIEFENzc4MCBkcml2ZXIgb25seSBzdXBwb3J0ZWQgZ3BpbyBm
b3IgdGhlICdwb3dlcmRvd24nDQo+IHBpbi4gVGhpcyBjb21taXQgYWRkcyBzdXBwcG9ydCBmb3Ig
dGhlICdnYWluJyBhbmQgJ2ZpbHRlcicgcGluLg0KSGV5LA0KDQpDb21tZW50cyBpbmxpbmUuDQo+
IA0KPiBTaWduZWQtb2ZmLWJ5OiBHaXVsaWFubyBCZWxpbmFzc2kgPGdpdWxpYW5vLmJlbGluYXNz
aUB1c3AuYnI+DQo+IC0tLQ0KPiDCoGRyaXZlcnMvc3RhZ2luZy9paW8vYWRjL2FkNzc4MC5jwqDC
oMKgwqDCoMKgwqB8IDYxICsrKysrKysrKysrKysrKysrKysrKysrKy0tDQo+IMKgaW5jbHVkZS9s
aW51eC9paW8vYWRjL2FkX3NpZ21hX2RlbHRhLmggfMKgwqA1ICsrKw0KPiDCoDIgZmlsZXMgY2hh
bmdlZCwgNjIgaW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQg
YS9kcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiBiL2RyaXZlcnMvc3RhZ2luZy9p
aW8vYWRjL2FkNzc4MC5jDQo+IGluZGV4IGM0YTg1Nzg5YzJkYi4uNjk3OTRmMDZkYmNkIDEwMDY0
NA0KPiAtLS0gYS9kcml2ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiArKysgYi9kcml2
ZXJzL3N0YWdpbmcvaWlvL2FkYy9hZDc3ODAuYw0KPiBAQCAtMzksNiArMzksOSBAQA0KPiDCoCNk
ZWZpbmUgQUQ3MTcwX1BBVFRFUk4JCShBRDc3ODBfUEFUMCB8IEFENzE3MF9QQVQyKQ0KPiDCoCNk
ZWZpbmUgQUQ3MTcwX1BBVFRFUk5fTUFTSwkoQUQ3NzgwX1BBVDAgfCBBRDc3ODBfUEFUMSB8DQo+
IEFENzE3MF9QQVQyKQ0KPiDCoA0KPiArI2RlZmluZSBBRDc3ODBfR0FJTl9HUElPCTANCj4gKyNk
ZWZpbmUgQUQ3NzgwX0ZJTFRFUl9HUElPCTENCj4gKw0KPiDCoHN0cnVjdCBhZDc3ODBfY2hpcF9p
bmZvIHsNCj4gwqAJc3RydWN0IGlpb19jaGFuX3NwZWMJY2hhbm5lbDsNCj4gwqAJdW5zaWduZWQg
aW50CQlwYXR0ZXJuX21hc2s7DQo+IEBAIC01MCw2ICs1Myw4IEBAIHN0cnVjdCBhZDc3ODBfc3Rh
dGUgew0KPiDCoAljb25zdCBzdHJ1Y3QgYWQ3NzgwX2NoaXBfaW5mbwkqY2hpcF9pbmZvOw0KPiDC
oAlzdHJ1Y3QgcmVndWxhdG9yCQkqcmVnOw0KPiDCoAlzdHJ1Y3QgZ3Bpb19kZXNjCQkqcG93ZXJk
b3duX2dwaW87DQo+ICsJc3RydWN0IGdwaW9fZGVzYwkJKmdhaW5fZ3BpbzsNCj4gKwlzdHJ1Y3Qg
Z3Bpb19kZXNjCQkqZmlsdGVyX2dwaW87DQo+IMKgCXVuc2lnbmVkIGludAlnYWluOw0KPiDCoA0K
PiDCoAlzdHJ1Y3QgYWRfc2lnbWFfZGVsdGEgc2Q7DQo+IEBAIC0xMTUsMTggKzEyMCw1MSBAQCBz
dGF0aWMgaW50IGFkNzc4MF9yZWFkX3JhdyhzdHJ1Y3QgaWlvX2Rldg0KPiAqaW5kaW9fZGV2LA0K
PiDCoAlyZXR1cm4gLUVJTlZBTDsNCj4gwqB9DQo+IMKgDQo+ICtzdGF0aWMgaW50IGFkNzc4MF93
cml0ZV9yYXcoc3RydWN0IGlpb19kZXYgKmluZGlvX2RldiwNCj4gKwkJCcKgwqDCoMKgc3RydWN0
IGlpb19jaGFuX3NwZWMgY29uc3QgKmNoYW4sDQo+ICsJCQnCoMKgwqDCoGludCB2YWwsDQo+ICsJ
CQnCoMKgwqDCoGludCB2YWwyLA0KPiArCQkJwqDCoMKgwqBsb25nIG0pDQo+ICt7DQo+ICsJc3Ry
dWN0IGFkNzc4MF9zdGF0ZSAqc3QgPSBpaW9fcHJpdihpbmRpb19kZXYpOw0KPiArDQo+ICsJaWYg
KG0gIT0gSUlPX0NIQU5fSU5GT19SQVcpDQo+ICsJCXJldHVybiAtRUlOVkFMOw0KPiArDQo+ICsJ
aWYgKHN0LT5jaGlwX2luZm8tPmlzX2FkNzc4eCkgew0KPiArCQlzd2l0Y2godmFsKSB7DQo+ICsJ
CWNhc2UgQUQ3NzgwX0dBSU5fR1BJTzoNCg0KSSB0aGluayB0aGF0IGluc3RlYWQgb2Ygc2V0dGlu
ZyB0aGUgZ2FpbiBkaXJlY3RseSwgd2Ugc2hvdWxkIHVzZQ0KdGhlwqBJSU9fQ0hBTl9JTkZPX1ND
QUxFIGF0dHJpYnV0ZS4gQXQgcGFnZSAxMiBvZiB0aGUgYWQ3NzgwIGRhdGFzaGVldCB0aGVyZQ0K
aXMgYSBmb3JtdWxhIGZyb20gd2hpY2ggdGhlIG91dHB1dCBjb2RlIGNhbiBiZSBjYWxjdWxhdGVk
Og0KQ29kZSA9IDJeKE4g4oiSIDEpDQrDlyBbKEFJTiDDlyBHYWluIC9WUkVGKSArIDFdLiBTbywg
Ynkgc2V0dGluZyB0aGUgc2NhbGUgZnJvbSB1c2VyIHNwYWNlLCB0aGUNCmRyaXZlciBjYW4gY2Fs
Y3VsYXRlIHRoZSBjb3JyZWN0IGdhaW4gYnkgdXNpbmcgdGhlIGZvcm11bGEgYWJvdmUuIEFsc28s
IGl0DQp3b3VsZCBiZSB1c2VmdWwgdG8gaW50cm9kdWNlIHNjYWxlIGF2YWlsYWJsZS4NCkZ1cnRo
ZXJtb3JlLCB0aGVyZSBpcyBhIG5ldw0KYWQ3MTI0IGFkYyBkcml2ZXIgd2hpY2ggZG9lcyB0aGlz
IGV4YWN0IHRoaW5nLiBUYWtlIGEgbG9vayBoZXJlOsKgaHR0cHM6Ly9naQ0KdGh1Yi5jb20vYW5h
bG9nZGV2aWNlc2luYy9saW51eC9ibG9iL21hc3Rlci9kcml2ZXJzL2lpby9hZGMvYWQ3MTI0LmMj
TDMzNy4NCg0KPiArCQkJZ3Bpb2Rfc2V0X3ZhbHVlKHN0LT5nYWluX2dwaW8sIHZhbDIpOw0KPiAr
CQlicmVhazsNCj4gKwkJY2FzZSBBRDc3ODBfRklMVEVSX0dQSU86DQoNClRoZSBhdHRyaWJ1dGUg
dGhhdCBzaG91bGQgYmUgdXNlZCB0byBjb25maWd1cmUgdGhlIGZpbHRlciBncGlvIGlzDQpJSU9f
Q0hBTl9JTkZPX1NBTVBfRlJFUS4gU28sIHdlIHNob3VsZCBoYXZlIDEwIEh6IGFuZCAxNi43IEh6
IGF2YWlsYWJsZQ0Kc2FtcGxpbmcgZnJlcXVlbmNpZXMuIElmIGZyb20gdXNlciBzcGFjZSB0aGUg
MTAgSHogc2FtcGxpbmcgZnJlcSBpcw0KcmVxdWVzdGVkLCB0aGVuIHdlIHNldCB0aGUgRklMVEVS
IHBpbiBoaWdoLCB3aGlsZSBmb3IgMTYuNyBIeiB0aGUgRklMVEVSDQpwaW4gd2lsbCBiZSBsb3cu
DQoNCj4gKwkJCWdwaW9kX3NldF92YWx1ZShzdC0+ZmlsdGVyX2dwaW8sIHZhbDIpOw0KPiArCQli
cmVhazsNCj4gKwkJZGVmYXVsdDoNCj4gKwkJCXJldHVybiAtRUlOVkFMOw0KPiArCQl9DQo+ICsJ
fQ0KPiArDQo+ICsJcmV0dXJuIDA7DQo+ICt9DQo+ICsNCj4gwqBzdGF0aWMgaW50IGFkNzc4MF9w
b3N0cHJvY2Vzc19zYW1wbGUoc3RydWN0IGFkX3NpZ21hX2RlbHRhICpzaWdtYV9kZWx0YSwNCj4g
wqAJCQkJwqDCoMKgwqDCoHVuc2lnbmVkIGludCByYXdfc2FtcGxlKQ0KPiDCoHsNCj4gwqAJc3Ry
dWN0IGFkNzc4MF9zdGF0ZSAqc3QgPSBhZF9zaWdtYV9kZWx0YV90b19hZDc3ODAoc2lnbWFfZGVs
dGEpOw0KPiDCoAljb25zdCBzdHJ1Y3QgYWQ3NzgwX2NoaXBfaW5mbyAqY2hpcF9pbmZvID0gc3Qt
PmNoaXBfaW5mbzsNCj4gKwlpbnQgdmFsOw0KPiDCoA0KPiDCoAlpZiAoKHJhd19zYW1wbGUgJiBB
RDc3ODBfRVJSKSB8fA0KPiDCoAnCoMKgwqDCoCgocmF3X3NhbXBsZSAmIGNoaXBfaW5mby0+cGF0
dGVybl9tYXNrKSAhPSBjaGlwX2luZm8tDQo+ID5wYXR0ZXJuKSkNCj4gwqAJCXJldHVybiAtRUlP
Ow0KPiDCoA0KPiDCoAlpZiAoY2hpcF9pbmZvLT5pc19hZDc3OHgpIHsNCj4gLQkJaWYgKHJhd19z
YW1wbGUgJiBBRDc3ODBfR0FJTikNCj4gKwkJdmFsID0gcmF3X3NhbXBsZSAmIEFENzc4MF9HQUlO
Ow0KPiArDQo+ICsJCWlmICh2YWwgIT0gZ3Bpb2RfZ2V0X3ZhbHVlKHN0LT5nYWluX2dwaW8pKQ0K
PiArCQkJcmV0dXJuIC1FSU87DQo+ICsNCj4gKwkJaWYgKHZhbCkNCj4gwqAJCQlzdC0+Z2FpbiA9
IDE7DQo+IMKgCQllbHNlDQo+IMKgCQkJc3QtPmdhaW4gPSAxMjg7DQo+IEBAIC0xNDEsMTggKzE3
OSwyMCBAQCBzdGF0aWMgY29uc3Qgc3RydWN0IGFkX3NpZ21hX2RlbHRhX2luZm8NCj4gYWQ3Nzgw
X3NpZ21hX2RlbHRhX2luZm8gPSB7DQo+IMKgCS5oYXNfcmVnaXN0ZXJzID0gZmFsc2UsDQo+IMKg
fTsNCj4gwqANCj4gLSNkZWZpbmUgQUQ3NzgwX0NIQU5ORUwoYml0cywgd29yZHNpemUpIFwNCj4g
KyNkZWZpbmUgQUQ3MTcwX0NIQU5ORUwoYml0cywgd29yZHNpemUpIFwNCj4gwqAJQURfU0RfQ0hB
Tk5FTF9OT19TQU1QX0ZSRVEoMSwgMCwgMCwgYml0cywgMzIsIHdvcmRzaXplIC0gYml0cykNCj4g
KyNkZWZpbmUgQUQ3NzgwX0NIQU5ORUwoYml0cywgd29yZHNpemUpIFwNCj4gKwlBRF9TRF9DSEFO
TkVMX0dBSU5fRklMVEVSKDEsIDAsIDAsIGJpdHMsIDMyLCB3b3Jkc2l6ZSAtIGJpdHMpDQo+IMKg
DQo+IMKgc3RhdGljIGNvbnN0IHN0cnVjdCBhZDc3ODBfY2hpcF9pbmZvIGFkNzc4MF9jaGlwX2lu
Zm9fdGJsW10gPSB7DQo+IMKgCVtJRF9BRDcxNzBdID0gew0KPiAtCQkuY2hhbm5lbCA9IEFENzc4
MF9DSEFOTkVMKDEyLCAyNCksDQo+ICsJCS5jaGFubmVsID0gQUQ3MTcwX0NIQU5ORUwoMTIsIDI0
KSwNCj4gwqAJCS5wYXR0ZXJuID0gQUQ3MTcwX1BBVFRFUk4sDQo+IMKgCQkucGF0dGVybl9tYXNr
ID0gQUQ3MTcwX1BBVFRFUk5fTUFTSywNCj4gwqAJCS5pc19hZDc3OHggPSBmYWxzZSwNCj4gwqAJ
fSwNCj4gwqAJW0lEX0FENzE3MV0gPSB7DQo+IC0JCS5jaGFubmVsID0gQUQ3NzgwX0NIQU5ORUwo
MTYsIDI0KSwNCj4gKwkJLmNoYW5uZWwgPSBBRDcxNzBfQ0hBTk5FTCgxNiwgMjQpLA0KPiDCoAkJ
LnBhdHRlcm4gPSBBRDcxNzBfUEFUVEVSTiwNCj4gwqAJCS5wYXR0ZXJuX21hc2sgPSBBRDcxNzBf
UEFUVEVSTl9NQVNLLA0KPiDCoAkJLmlzX2FkNzc4eCA9IGZhbHNlLA0KPiBAQCAtMTczLDYgKzIx
Myw3IEBAIHN0YXRpYyBjb25zdCBzdHJ1Y3QgYWQ3NzgwX2NoaXBfaW5mbw0KPiBhZDc3ODBfY2hp
cF9pbmZvX3RibFtdID0gew0KPiDCoA0KPiDCoHN0YXRpYyBjb25zdCBzdHJ1Y3QgaWlvX2luZm8g
YWQ3NzgwX2luZm8gPSB7DQo+IMKgCS5yZWFkX3JhdyA9IGFkNzc4MF9yZWFkX3JhdywNCj4gKwku
d3JpdGVfcmF3ID0gYWQ3NzgwX3dyaXRlX3JhdywNCj4gwqB9Ow0KPiDCoA0KPiDCoHN0YXRpYyBp
bnQgYWQ3NzgwX3Byb2JlKHN0cnVjdCBzcGlfZGV2aWNlICpzcGkpDQo+IEBAIC0yMjIsNiArMjYz
LDE4IEBAIHN0YXRpYyBpbnQgYWQ3NzgwX3Byb2JlKHN0cnVjdCBzcGlfZGV2aWNlICpzcGkpDQo+
IMKgCQlnb3RvIGVycm9yX2Rpc2FibGVfcmVnOw0KPiDCoAl9DQo+IMKgDQo+ICsJaWYgKHN0LT5j
aGlwX2luZm8tPmlzX2FkNzc4eCkgew0KPiArCQlzdC0+Z2Fpbl9ncGlvID0gZGV2bV9ncGlvZF9n
ZXRfb3B0aW9uYWwoJnNwaS0+ZGV2LA0KPiArCQkJCQkJCSJnYWluIiwNCj4gKwkJCQkJCQlHUElP
RF9PVVRfSElHSCk7DQo+ICsJCWlmIChJU19FUlIoc3QtPmdhaW5fZ3BpbykpIHsNCj4gKwkJCXJl
dCA9IFBUUl9FUlIoc3QtPmdhaW5fZ3Bpbyk7DQo+ICsJCQlkZXZfZXJyKCZzcGktPmRldiwgIkZh
aWxlZCB0byByZXF1ZXN0IGdhaW4gR1BJTzoNCj4gJWRcbiIsDQo+ICsJCQkJcmV0KTsNCj4gKwkJ
CWdvdG8gZXJyb3JfZGlzYWJsZV9yZWc7DQo+ICsJCX0NCj4gKwl9DQo+ICsNCj4gwqAJcmV0ID0g
YWRfc2Rfc2V0dXBfYnVmZmVyX2FuZF90cmlnZ2VyKGluZGlvX2Rldik7DQo+IMKgCWlmIChyZXQp
DQo+IMKgCQlnb3RvIGVycm9yX2Rpc2FibGVfcmVnOw0KPiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9s
aW51eC9paW8vYWRjL2FkX3NpZ21hX2RlbHRhLmgNCj4gYi9pbmNsdWRlL2xpbnV4L2lpby9hZGMv
YWRfc2lnbWFfZGVsdGEuaA0KPiBpbmRleCA3MzBlYWQxYTQ2ZGYuLjZjYWRhYjZmZDVmZCAxMDA2
NDQNCj4gLS0tIGEvaW5jbHVkZS9saW51eC9paW8vYWRjL2FkX3NpZ21hX2RlbHRhLmgNCj4gKysr
IGIvaW5jbHVkZS9saW51eC9paW8vYWRjL2FkX3NpZ21hX2RlbHRhLmgNCj4gQEAgLTE3Myw2ICsx
NzMsMTEgQEAgaW50IGFkX3NkX3ZhbGlkYXRlX3RyaWdnZXIoc3RydWN0IGlpb19kZXYNCj4gKmlu
ZGlvX2Rldiwgc3RydWN0IGlpb190cmlnZ2VyICp0cmlnKTsNCj4gwqAJX19BRF9TRF9DSEFOTkVM
KF9zaSwgX2NoYW5uZWwsIC0xLCBfYWRkcmVzcywgX2JpdHMsIFwNCj4gwqAJCV9zdG9yYWdlYml0
cywgX3NoaWZ0LCBOVUxMLCBJSU9fVk9MVEFHRSwgMCkNCj4gwqANCj4gKyNkZWZpbmUgQURfU0Rf
Q0hBTk5FTF9HQUlOX0ZJTFRFUihfc2ksIF9jaGFubmVsLCBfYWRkcmVzcywgX2JpdHMsIFwNCj4g
Kwlfc3RvcmFnZWJpdHMsIF9zaGlmdCkgXA0KPiArCV9fQURfU0RfQ0hBTk5FTChfc2ksIF9jaGFu
bmVsLCAtMSwgX2FkZHJlc3MsIF9iaXRzLCBcDQo+ICsJCV9zdG9yYWdlYml0cywgX3NoaWZ0LCBO
VUxMLCBJSU9fVk9MVEFHRSwNCj4gQklUKElJT19DSEFOX0lORk9fUkFXKSkNCj4gKw0KPiDCoCNk
ZWZpbmUgQURfU0RfVEVNUF9DSEFOTkVMKF9zaSwgX2FkZHJlc3MsIF9iaXRzLCBfc3RvcmFnZWJp
dHMsIF9zaGlmdCkgXA0KPiDCoAlfX0FEX1NEX0NIQU5ORUwoX3NpLCAwLCAtMSwgX2FkZHJlc3Ms
IF9iaXRzLCBcDQo+IMKgCQlfc3RvcmFnZWJpdHMsIF9zaGlmdCwgTlVMTCwgSUlPX1RFTVAsIFw=

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
  2018-11-22 11:01 ` Popa, Stefan Serban
@ 2018-11-25 11:08   ` Jonathan Cameron
  2018-11-26 19:24     ` Giuliano Belinassi
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2018-11-25 11:08 UTC (permalink / raw)
  To: Popa, Stefan Serban
  Cc: Ardelean, Alexandru, lars, knaack.h, Hennerich, Michael,
	giuliano.belinassi, pmeerw, gregkh, linux-kernel, linux-iio,
	devel, kernel-usp

On Thu, 22 Nov 2018 11:01:00 +0000
"Popa, Stefan Serban" <StefanSerban.Popa@analog.com> wrote:

> On Mi, 2018-11-21 at 16:04 -0200, Giuliano Belinassi wrote:
> > Previously, the AD7780 driver only supported gpio for the 'powerdown'
> > pin. This commit adds suppport for the 'gain' and 'filter' pin. =20
> Hey,
>=20
> Comments inline.
> >=20
> > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > ---
> > =C2=A0drivers/staging/iio/adc/ad7780.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0| 61 ++++++++++++++++++++++++--
> > =C2=A0include/linux/iio/adc/ad_sigma_delta.h |=C2=A0=C2=A05 +++
> > =C2=A02 files changed, 62 insertions(+), 4 deletions(-)
> >=20
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index c4a85789c2db..69794f06dbcd 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -39,6 +39,9 @@
> > =C2=A0#define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
> > =C2=A0#define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 |
> > AD7170_PAT2)
> > =C2=A0
> > +#define AD7780_GAIN_GPIO	0
> > +#define AD7780_FILTER_GPIO	1
> > +
> > =C2=A0struct ad7780_chip_info {
> > =C2=A0	struct iio_chan_spec	channel;
> > =C2=A0	unsigned int		pattern_mask;
> > @@ -50,6 +53,8 @@ struct ad7780_state {
> > =C2=A0	const struct ad7780_chip_info	*chip_info;
> > =C2=A0	struct regulator		*reg;
> > =C2=A0	struct gpio_desc		*powerdown_gpio;
> > +	struct gpio_desc		*gain_gpio;
> > +	struct gpio_desc		*filter_gpio;
> > =C2=A0	unsigned int	gain;
> > =C2=A0
> > =C2=A0	struct ad_sigma_delta sd;
> > @@ -115,18 +120,51 @@ static int ad7780_read_raw(struct iio_dev
> > *indio_dev,
> > =C2=A0	return -EINVAL;
> > =C2=A0}
> > =C2=A0
> > +static int ad7780_write_raw(struct iio_dev *indio_dev,
> > +			=C2=A0=C2=A0=C2=A0=C2=A0struct iio_chan_spec const *chan,
> > +			=C2=A0=C2=A0=C2=A0=C2=A0int val,
> > +			=C2=A0=C2=A0=C2=A0=C2=A0int val2,
> > +			=C2=A0=C2=A0=C2=A0=C2=A0long m)
> > +{
> > +	struct ad7780_state *st =3D iio_priv(indio_dev);
> > +
> > +	if (m !=3D IIO_CHAN_INFO_RAW)
> > +		return -EINVAL;
> > +
> > +	if (st->chip_info->is_ad778x) {
> > +		switch(val) {
> > +		case AD7780_GAIN_GPIO: =20
>=20
> I think that instead of setting the gain directly, we should use
> the=C2=A0IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datashee=
t there
> is a formula from which the output code can be calculated:
> Code =3D 2^(N =E2=88=92 1)
> =C3=97 [(AIN =C3=97 Gain /VREF) + 1]. So, by setting the scale from user =
space, the
> driver can calculate the correct gain by using the formula above. Also, it
> would be useful to introduce scale available.
> Furthermore, there is a new
> ad7124 adc driver which does this exact thing. Take a look here:=C2=A0htt=
ps://gi
> thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.c#L337.
>=20
> > +			gpiod_set_value(st->gain_gpio, val2);
> > +		break;
> > +		case AD7780_FILTER_GPIO: =20
>=20
> The attribute that should be used to configure the filter gpio is
> IIO_CHAN_INFO_SAMP_FREQ. So, we should have 10 Hz and 16.7 Hz available
> sampling frequencies. If from user space the 10 Hz sampling freq is
> requested, then we set the FILTER pin high, while for 16.7 Hz the FILTER
> pin will be low.

Absolutely agreed with Stefan here.  If it had been decoupled from sampling
frequency (sometimes they are) then we have specific controls for filters
as well.  Here it directly effects the sampling frequency.

Please in future avoid any driver specific control like you have here.
I haven't really worked out what the interface is beyond some sort
of bitmap passed through a write to a magic channel?

If there isn't an existing interface in IIO for what you want to do
please propose one rather than doing something that will only work
with a particular userspace.  One of the primary purposes of having
a subsystem is to standardise interfaces.  This definitely doesn't
do that!

Will be good to have the support along the lines Stefan suggested though!

Thanks,

Jonathan



>=20
> > +			gpiod_set_value(st->filter_gpio, val2);
> > +		break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > =C2=A0static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma=
_delta,
> > =C2=A0				=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0unsigned int raw_sample)
> > =C2=A0{
> > =C2=A0	struct ad7780_state *st =3D ad_sigma_delta_to_ad7780(sigma_delta=
);
> > =C2=A0	const struct ad7780_chip_info *chip_info =3D st->chip_info;
> > +	int val;
> > =C2=A0
> > =C2=A0	if ((raw_sample & AD7780_ERR) ||
> > =C2=A0	=C2=A0=C2=A0=C2=A0=C2=A0((raw_sample & chip_info->pattern_mask) =
!=3D chip_info- =20
> > >pattern)) =20
> > =C2=A0		return -EIO;
> > =C2=A0
> > =C2=A0	if (chip_info->is_ad778x) {
> > -		if (raw_sample & AD7780_GAIN)
> > +		val =3D raw_sample & AD7780_GAIN;
> > +
> > +		if (val !=3D gpiod_get_value(st->gain_gpio))
> > +			return -EIO;
> > +
> > +		if (val)
> > =C2=A0			st->gain =3D 1;
> > =C2=A0		else
> > =C2=A0			st->gain =3D 128;
> > @@ -141,18 +179,20 @@ static const struct ad_sigma_delta_info
> > ad7780_sigma_delta_info =3D {
> > =C2=A0	.has_registers =3D false,
> > =C2=A0};
> > =C2=A0
> > -#define AD7780_CHANNEL(bits, wordsize) \
> > +#define AD7170_CHANNEL(bits, wordsize) \
> > =C2=A0	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> > +#define AD7780_CHANNEL(bits, wordsize) \
> > +	AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
> > =C2=A0
> > =C2=A0static const struct ad7780_chip_info ad7780_chip_info_tbl[] =3D {
> > =C2=A0	[ID_AD7170] =3D {
> > -		.channel =3D AD7780_CHANNEL(12, 24),
> > +		.channel =3D AD7170_CHANNEL(12, 24),
> > =C2=A0		.pattern =3D AD7170_PATTERN,
> > =C2=A0		.pattern_mask =3D AD7170_PATTERN_MASK,
> > =C2=A0		.is_ad778x =3D false,
> > =C2=A0	},
> > =C2=A0	[ID_AD7171] =3D {
> > -		.channel =3D AD7780_CHANNEL(16, 24),
> > +		.channel =3D AD7170_CHANNEL(16, 24),
> > =C2=A0		.pattern =3D AD7170_PATTERN,
> > =C2=A0		.pattern_mask =3D AD7170_PATTERN_MASK,
> > =C2=A0		.is_ad778x =3D false,
> > @@ -173,6 +213,7 @@ static const struct ad7780_chip_info
> > ad7780_chip_info_tbl[] =3D {
> > =C2=A0
> > =C2=A0static const struct iio_info ad7780_info =3D {
> > =C2=A0	.read_raw =3D ad7780_read_raw,
> > +	.write_raw =3D ad7780_write_raw,
> > =C2=A0};
> > =C2=A0
> > =C2=A0static int ad7780_probe(struct spi_device *spi)
> > @@ -222,6 +263,18 @@ static int ad7780_probe(struct spi_device *spi)
> > =C2=A0		goto error_disable_reg;
> > =C2=A0	}
> > =C2=A0
> > +	if (st->chip_info->is_ad778x) {
> > +		st->gain_gpio =3D devm_gpiod_get_optional(&spi->dev,
> > +							"gain",
> > +							GPIOD_OUT_HIGH);
> > +		if (IS_ERR(st->gain_gpio)) {
> > +			ret =3D PTR_ERR(st->gain_gpio);
> > +			dev_err(&spi->dev, "Failed to request gain GPIO:
> > %d\n",
> > +				ret);
> > +			goto error_disable_reg;
> > +		}
> > +	}
> > +
> > =C2=A0	ret =3D ad_sd_setup_buffer_and_trigger(indio_dev);
> > =C2=A0	if (ret)
> > =C2=A0		goto error_disable_reg;
> > diff --git a/include/linux/iio/adc/ad_sigma_delta.h
> > b/include/linux/iio/adc/ad_sigma_delta.h
> > index 730ead1a46df..6cadab6fd5fd 100644
> > --- a/include/linux/iio/adc/ad_sigma_delta.h
> > +++ b/include/linux/iio/adc/ad_sigma_delta.h
> > @@ -173,6 +173,11 @@ int ad_sd_validate_trigger(struct iio_dev
> > *indio_dev, struct iio_trigger *trig);
> > =C2=A0	__AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
> > =C2=A0		_storagebits, _shift, NULL, IIO_VOLTAGE, 0)
> > =C2=A0
> > +#define AD_SD_CHANNEL_GAIN_FILTER(_si, _channel, _address, _bits, \
> > +	_storagebits, _shift) \
> > +	__AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
> > +		_storagebits, _shift, NULL, IIO_VOLTAGE,
> > BIT(IIO_CHAN_INFO_RAW))
> > +
> > =C2=A0#define AD_SD_TEMP_CHANNEL(_si, _address, _bits, _storagebits, _s=
hift) \
> > =C2=A0	__AD_SD_CHANNEL(_si, 0, -1, _address, _bits, \
> > =C2=A0		_storagebits, _shift, NULL, IIO_TEMP,  =20

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
  2018-11-25 11:08   ` Jonathan Cameron
@ 2018-11-26 19:24     ` Giuliano Belinassi
  2018-11-27 11:11       ` Popa, Stefan Serban
  0 siblings, 1 reply; 8+ messages in thread
From: Giuliano Belinassi @ 2018-11-26 19:24 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: StefanSerban.Popa, Ardelean, Alexandru, Lars-Peter Clausen,
	Hartmut Knaack, Michael Hennerich, Peter Meerwald-Stadler,
	Greg Kroah-Hartman, linux-kernel, linux-iio, devel, kernel-usp

Hi, thank you for the review

> On Thu, 22 Nov 2018 11:01:00 +0000
> "Popa, Stefan Serban" <StefanSerban.Popa@analog.com> wrote:
> > I think that instead of setting the gain directly, we should use
> > the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datasheet t=
here
> > is a formula from which the output code can be calculated:
> > Code =3D 2^(N =E2=88=92 1)
> > =C3=97 [(AIN =C3=97 Gain /VREF) + 1]. So, by setting the scale from use=
r space, the
> > driver can calculate the correct gain by using the formula above. Also,=
 it
> > would be useful to introduce scale available.
> > Furthermore, there is a new
> > ad7124 adc driver which does this exact thing. Take a look here: https:=
//gi
> > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.c#L3=
37.

We have some questions about the code you provided to us:
  1-) What is exactly the inputs for the write_raw function?
  2-) Could you give more details about the math around lines 346-348?
Is it correct to assume that the multiplication at line 346 won't
overflow? (vref is an uint)

And regarding our code:
  1-) The val in our write_raw function should be, in case of GAIN, a
number that best approximate the actual gain value of 1 or 128? For
instance, if the user inputs 126, we should default to 128?
  2-) In the case of FILTER, is it the same? Is the user sending the
value in mHz (milihertz)?

Thank you

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
  2018-11-26 19:24     ` Giuliano Belinassi
@ 2018-11-27 11:11       ` Popa, Stefan Serban
  2018-11-29 12:19         ` Ardelean, Alexandru
  0 siblings, 1 reply; 8+ messages in thread
From: Popa, Stefan Serban @ 2018-11-27 11:11 UTC (permalink / raw)
  To: giuliano.belinassi, jic23
  Cc: kernel-usp, linux-kernel, lars, knaack.h, Ardelean, Alexandru,
	Hennerich, Michael, linux-iio, devel, pmeerw, gregkh, Popa,
	Stefan Serban

T24gTHUsIDIwMTgtMTEtMjYgYXQgMTc6MjQgLTAyMDAsIEdpdWxpYW5vIEJlbGluYXNzaSB3cm90
ZToNCkhpLCBwbGVhc2Ugc2VlIGJlbGxvdw0KDQo+IEhpLCB0aGFuayB5b3UgZm9yIHRoZSByZXZp
ZXcNCj4gDQo+ID4gDQo+ID4gT24gVGh1LCAyMiBOb3YgMjAxOCAxMTowMTowMCArMDAwMA0KPiA+
ICJQb3BhLCBTdGVmYW4gU2VyYmFuIiA8U3RlZmFuU2VyYmFuLlBvcGFAYW5hbG9nLmNvbT4gd3Jv
dGU6DQo+ID4gPiANCj4gPiA+IEkgdGhpbmsgdGhhdCBpbnN0ZWFkIG9mIHNldHRpbmcgdGhlIGdh
aW4gZGlyZWN0bHksIHdlIHNob3VsZCB1c2UNCj4gPiA+IHRoZSBJSU9fQ0hBTl9JTkZPX1NDQUxF
IGF0dHJpYnV0ZS4gQXQgcGFnZSAxMiBvZiB0aGUgYWQ3NzgwIGRhdGFzaGVldA0KPiA+ID4gdGhl
cmUNCj4gPiA+IGlzIGEgZm9ybXVsYSBmcm9tIHdoaWNoIHRoZSBvdXRwdXQgY29kZSBjYW4gYmUg
Y2FsY3VsYXRlZDoNCj4gPiA+IENvZGUgPSAyXihOIOKIkiAxKQ0KPiA+ID4gw5cgWyhBSU4gw5cg
R2FpbiAvVlJFRikgKyAxXS4gU28sIGJ5IHNldHRpbmcgdGhlIHNjYWxlIGZyb20gdXNlciBzcGFj
ZSwNCj4gPiA+IHRoZQ0KPiA+ID4gZHJpdmVyIGNhbiBjYWxjdWxhdGUgdGhlIGNvcnJlY3QgZ2Fp
biBieSB1c2luZyB0aGUgZm9ybXVsYSBhYm92ZS4NCj4gPiA+IEFsc28sIGl0DQo+ID4gPiB3b3Vs
ZCBiZSB1c2VmdWwgdG8gaW50cm9kdWNlIHNjYWxlIGF2YWlsYWJsZS4NCj4gPiA+IEZ1cnRoZXJt
b3JlLCB0aGVyZSBpcyBhIG5ldw0KPiA+ID4gYWQ3MTI0IGFkYyBkcml2ZXIgd2hpY2ggZG9lcyB0
aGlzIGV4YWN0IHRoaW5nLiBUYWtlIGEgbG9vayBoZXJlOiBodHRwDQo+ID4gPiBzOi8vZ2kNCj4g
PiA+IHRodWIuY29tL2FuYWxvZ2RldmljZXNpbmMvbGludXgvYmxvYi9tYXN0ZXIvZHJpdmVycy9p
aW8vYWRjL2FkNzEyNC5jIw0KPiA+ID4gTDMzNy4NCj4gV2UgaGF2ZSBzb21lIHF1ZXN0aW9ucyBh
Ym91dCB0aGUgY29kZSB5b3UgcHJvdmlkZWQgdG8gdXM6DQo+IMKgIDEtKSBXaGF0IGlzIGV4YWN0
bHkgdGhlIGlucHV0cyBmb3IgdGhlIHdyaXRlX3JhdyBmdW5jdGlvbj8NCg0KSW4geW91ciB3cml0
ZV9yYXcoKSBmdW5jdGlvbiB5b3UgbmVlZCB0byBhZGQgSUlPX0NIQU5fSU5GT19TQ0FMRSBjYXNl
Lg0KU2V0dGluZyB0aGUgc2NhbGUgZnJvbSB1c2VyIHNwYWNlIGxvb2tzIHNvbWV0aGluZyBsaWtl
IHRoaXM6DQpyb290Oi9zeXMvYnVzL2lpby9kZXZpY2VzL2lpbzpkZXZpY2UwPiBlY2hvIDAuMDAw
Mjk4ID4gaW5fdm9sdGFnZV9zY2FsZSAuDQpGdXJ0aGVybW9yZSwgaW4geW91ciB3cml0ZV9yYXco
KSBmdW5jdGlvbiwgdmFsPTAgYW5kIHZhbDI9Mjk4Lg0KS25vd2luZyB0aGF0IGZ1bGxfc2NhbGVf
dm9sdGFnZSA9IFZyZWYgLyAoZ2FpbiAqIHNjYWxlKSwgd2UgY2FuIGNhbGN1bGF0ZQ0KdGhlIGdh
aW4gPSBWcmVmIC8gKGZ1bGxfc2NhbGVfdm9sdGFnZSAqIHNjYWxlKS4gV2Ugb25seSBzdXBwb3J0
IHR3byBnYWlucw0KKDEgYW5kIDEyOCksIHNvIHdlIG5lZWQgdG8gZGV0ZXJtaW5lIHdoaWNoIG9u
ZSBmaXRzIGJldHRlciB3aXRoIHRoZSBkZXNpcmVkDQpzY2FsZS4gRmluYWxseSwgYWxsIHdlIGhh
dmUgbGVmdCB0byBkbyBpcyB0byBzZXQgdGhlIGdhaW4uwqANCsKgDQo+IMKgIDItKSBDb3VsZCB5
b3UgZ2l2ZSBtb3JlIGRldGFpbHMgYWJvdXQgdGhlIG1hdGggYXJvdW5kIGxpbmVzIDM0Ni0zNDg/
DQo+IElzIGl0IGNvcnJlY3QgdG8gYXNzdW1lIHRoYXQgdGhlIG11bHRpcGxpY2F0aW9uIGF0IGxp
bmUgMzQ2IHdvbid0DQo+IG92ZXJmbG93PyAodnJlZiBpcyBhbiB1aW50KQ0KDQpJdCBpcyBjb3Jy
ZWN0IHRoYXQgVnJlZiBpcyBpbiBtaWNyb3ZvbHRzLCBzbyBmb3IgZXhhbXBsZSwgVnJlZiBvZiAy
LjVWIMKgPQ0KMjUwMDAwMDAwMHVWLiBJdCB3b24ndCBvdmVyZmxvdyBzaW5jZSB3ZSB1c2UgdGhl
IFZyZWYgYXMgbm9taW5hdG9yLCB3aGlsZQ0KZnVsbF9zY2FsZV92b2x0YWdlIGFuZCBzY2FsZSBh
cmUgdGhlIGRlbm9taW5hdG9ycy4NCg0KPiANCj4gQW5kIHJlZ2FyZGluZyBvdXIgY29kZToNCj4g
wqAgMS0pIFRoZSB2YWwgaW4gb3VyIHdyaXRlX3JhdyBmdW5jdGlvbiBzaG91bGQgYmUsIGluIGNh
c2Ugb2YgR0FJTiwgYQ0KPiBudW1iZXIgdGhhdCBiZXN0IGFwcHJveGltYXRlIHRoZSBhY3R1YWwg
Z2FpbiB2YWx1ZSBvZiAxIG9yIDEyOD8gRm9yDQo+IGluc3RhbmNlLCBpZiB0aGUgdXNlciBpbnB1
dHMgMTI2LCB3ZSBzaG91bGQgZGVmYXVsdCB0byAxMjg/DQoNCldlIHNob3VsZCBub3QgYWxsb3cg
dGhlIHRoZSB1c2VyIHRvIGlucHV0IHRoZSBnYWluLCBoZSBuZWVkcyB0byBpbnB1dCB0aGUNCnNj
YWxlIChzZWUgdGhlIG1haWwgZnJvbSBKb25hdGhhbiBhbmQgdGhlIGFib3ZlIGV4cGxhbmF0aW9u
KS4gSG93ZXZlciwgaWYNCnRoZSBjYWxjdWxhdGVkIGdhaW4gaXMgb25lIHRoYXQgaXMgbm90IHN1
cHBvcnRlZCwgc3VjaCBhcyAxMjYsIHdlIHdpbGwgc2V0DQp0aGUgY2xvc2VzdCBtYXRjaGluZyB2
YWx1ZSwgaW4gdGhpcyBjYXNlIDEyOC4NCg0KPiDCoCAyLSkgSW4gdGhlIGNhc2Ugb2YgRklMVEVS
LCBpcyBpdCB0aGUgc2FtZT8gSXMgdGhlIHVzZXIgc2VuZGluZyB0aGUNCj4gdmFsdWUgaW4gbUh6
IChtaWxpaGVydHopPw0KDQpZZXMsIGl0IGlzIHRoZSBzYW1lIHdpdGggdGhlIEZJTFRFUi4gWW91
IG5lZWQgdG8gYWRkDQphwqBJSU9fQ0hBTl9JTkZPX1NBTVBfRlJFUSBjYXNlIGluIHlvdXIgd3Jp
dGVfcmF3KCkgZnVuY3Rpb24uIEZyb20gdXNlcg0Kc3BhY2UsIHRoZSBpbnB1dCB2YWx1ZSBzaG91
bGQgYmUgaW4gSHosIHNvbWV0aGluZyBsaWtlIHRoaXM6DQpyb290Oi9zeXMvYnVzL2lpby9kZXZp
Y2VzL2lpbzpkZXZpY2UwPiBlY2hvIDEwID4NCmluX3ZvbHRhZ2Vfc2FtcGxpbmdfZnJlcXVlbmN5
DQoNCj4gDQo+IFRoYW5rIHlvdQ0KPiA=

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
  2018-11-27 11:11       ` Popa, Stefan Serban
@ 2018-11-29 12:19         ` Ardelean, Alexandru
  2018-12-01 15:17           ` Jonathan Cameron
  0 siblings, 1 reply; 8+ messages in thread
From: Ardelean, Alexandru @ 2018-11-29 12:19 UTC (permalink / raw)
  To: giuliano.belinassi, jic23, Popa, Stefan Serban
  Cc: kernel-usp, linux-kernel, lars, knaack.h, Hennerich, Michael,
	linux-iio, devel, pmeerw, gregkh

T24gVHVlLCAyMDE4LTExLTI3IGF0IDA2OjExIC0wNTAwLCBQb3BhLCBTdGVmYW4gU2VyYmFuIHdy
b3RlOg0KPiBPbiBMdSwgMjAxOC0xMS0yNiBhdCAxNzoyNCAtMDIwMCwgR2l1bGlhbm8gQmVsaW5h
c3NpIHdyb3RlOg0KPiBIaSwgcGxlYXNlIHNlZSBiZWxsb3cNCj4gDQoNCk9uZSBub3RlIGZyb20g
bWUgaGVyZS4NCg0KPiA+IEhpLCB0aGFuayB5b3UgZm9yIHRoZSByZXZpZXcNCj4gPiANCj4gPiA+
IA0KPiA+ID4gT24gVGh1LCAyMiBOb3YgMjAxOCAxMTowMTowMCArMDAwMA0KPiA+ID4gIlBvcGEs
IFN0ZWZhbiBTZXJiYW4iIDxTdGVmYW5TZXJiYW4uUG9wYUBhbmFsb2cuY29tPiB3cm90ZToNCj4g
PiA+ID4gDQo+ID4gPiA+IEkgdGhpbmsgdGhhdCBpbnN0ZWFkIG9mIHNldHRpbmcgdGhlIGdhaW4g
ZGlyZWN0bHksIHdlIHNob3VsZCB1c2UNCj4gPiA+ID4gdGhlIElJT19DSEFOX0lORk9fU0NBTEUg
YXR0cmlidXRlLiBBdCBwYWdlIDEyIG9mIHRoZSBhZDc3ODANCj4gPiA+ID4gZGF0YXNoZWV0DQo+
ID4gPiA+IHRoZXJlDQo+ID4gPiA+IGlzIGEgZm9ybXVsYSBmcm9tIHdoaWNoIHRoZSBvdXRwdXQg
Y29kZSBjYW4gYmUgY2FsY3VsYXRlZDoNCj4gPiA+ID4gQ29kZSA9IDJeKE4g4oiSIDEpDQo+ID4g
PiA+IMOXIFsoQUlOIMOXIEdhaW4gL1ZSRUYpICsgMV0uIFNvLCBieSBzZXR0aW5nIHRoZSBzY2Fs
ZSBmcm9tIHVzZXINCj4gPiA+ID4gc3BhY2UsDQo+ID4gPiA+IHRoZQ0KPiA+ID4gPiBkcml2ZXIg
Y2FuIGNhbGN1bGF0ZSB0aGUgY29ycmVjdCBnYWluIGJ5IHVzaW5nIHRoZSBmb3JtdWxhIGFib3Zl
Lg0KPiA+ID4gPiBBbHNvLCBpdA0KPiA+ID4gPiB3b3VsZCBiZSB1c2VmdWwgdG8gaW50cm9kdWNl
IHNjYWxlIGF2YWlsYWJsZS4NCj4gPiA+ID4gRnVydGhlcm1vcmUsIHRoZXJlIGlzIGEgbmV3DQo+
ID4gPiA+IGFkNzEyNCBhZGMgZHJpdmVyIHdoaWNoIGRvZXMgdGhpcyBleGFjdCB0aGluZy4gVGFr
ZSBhIGxvb2sgaGVyZToNCj4gPiA+ID4gaHR0cA0KPiA+ID4gPiBzOi8vZ2kNCj4gPiA+ID4gdGh1
Yi5jb20vYW5hbG9nZGV2aWNlc2luYy9saW51eC9ibG9iL21hc3Rlci9kcml2ZXJzL2lpby9hZGMv
YWQ3MTI0Lg0KPiA+ID4gPiBjIw0KPiA+ID4gPiBMMzM3Lg0KPiA+IA0KPiA+IFdlIGhhdmUgc29t
ZSBxdWVzdGlvbnMgYWJvdXQgdGhlIGNvZGUgeW91IHByb3ZpZGVkIHRvIHVzOg0KPiA+ICAgMS0p
IFdoYXQgaXMgZXhhY3RseSB0aGUgaW5wdXRzIGZvciB0aGUgd3JpdGVfcmF3IGZ1bmN0aW9uPw0K
PiANCj4gSW4geW91ciB3cml0ZV9yYXcoKSBmdW5jdGlvbiB5b3UgbmVlZCB0byBhZGQgSUlPX0NI
QU5fSU5GT19TQ0FMRSBjYXNlLg0KPiBTZXR0aW5nIHRoZSBzY2FsZSBmcm9tIHVzZXIgc3BhY2Ug
bG9va3Mgc29tZXRoaW5nIGxpa2UgdGhpczoNCj4gcm9vdDovc3lzL2J1cy9paW8vZGV2aWNlcy9p
aW86ZGV2aWNlMD4gZWNobyAwLjAwMDI5OCA+IGluX3ZvbHRhZ2Vfc2NhbGUgLg0KPiBGdXJ0aGVy
bW9yZSwgaW4geW91ciB3cml0ZV9yYXcoKSBmdW5jdGlvbiwgdmFsPTAgYW5kIHZhbDI9Mjk4Lg0K
PiBLbm93aW5nIHRoYXQgZnVsbF9zY2FsZV92b2x0YWdlID0gVnJlZiAvIChnYWluICogc2NhbGUp
LCB3ZSBjYW4gY2FsY3VsYXRlDQo+IHRoZSBnYWluID0gVnJlZiAvIChmdWxsX3NjYWxlX3ZvbHRh
Z2UgKiBzY2FsZSkuIFdlIG9ubHkgc3VwcG9ydCB0d28gZ2FpbnMNCj4gKDEgYW5kIDEyOCksIHNv
IHdlIG5lZWQgdG8gZGV0ZXJtaW5lIHdoaWNoIG9uZSBmaXRzIGJldHRlciB3aXRoIHRoZQ0KPiBk
ZXNpcmVkDQo+IHNjYWxlLiBGaW5hbGx5LCBhbGwgd2UgaGF2ZSBsZWZ0IHRvIGRvIGlzIHRvIHNl
dCB0aGUgZ2Fpbi4gDQo+ICANCj4gPiAgIDItKSBDb3VsZCB5b3UgZ2l2ZSBtb3JlIGRldGFpbHMg
YWJvdXQgdGhlIG1hdGggYXJvdW5kIGxpbmVzIDM0Ni0zNDg/DQo+ID4gSXMgaXQgY29ycmVjdCB0
byBhc3N1bWUgdGhhdCB0aGUgbXVsdGlwbGljYXRpb24gYXQgbGluZSAzNDYgd29uJ3QNCj4gPiBv
dmVyZmxvdz8gKHZyZWYgaXMgYW4gdWludCkNCj4gDQo+IEl0IGlzIGNvcnJlY3QgdGhhdCBWcmVm
IGlzIGluIG1pY3Jvdm9sdHMsIHNvIGZvciBleGFtcGxlLCBWcmVmIG9mIDIuNVYgID0NCj4gMjUw
MDAwMDAwMHVWLiBJdCB3b24ndCBvdmVyZmxvdyBzaW5jZSB3ZSB1c2UgdGhlIFZyZWYgYXMgbm9t
aW5hdG9yLCB3aGlsZQ0KPiBmdWxsX3NjYWxlX3ZvbHRhZ2UgYW5kIHNjYWxlIGFyZSB0aGUgZGVu
b21pbmF0b3JzLg0KPiANCg0KW1JlZ2FyZGluZyB0aGUgQUQ3MTI0XSBJIGd1ZXNzIEkgc2hvdWxk
IGJlIG5vdGVkIHRoYXQgdGhlIGNvZGUgY2FuDQpvdmVyZmxvdywgYnV0IGluIHRoaXMgY2FzZSBp
dCBzaG91bGRuJ3QsIGJlY2F1c2UgKGFjY29yZGluZyB0byB0aGUgZGV2aWNlDQpkYXRhc2hlZXQp
IHRoZSBtYXhpbXVtIFZSRUYgY2FuIGJlIDM2MDAgbVYuDQpBIHVzZXIgY291bGQgc3BlY2lmeSAo
aW4gdGhlIGRldmljZXRyZWUpIHNvbWV0aGluZyBsYXJnZXIgdGhhbiA0MzAwIG1WIChhbmQNCnRo
YXQgd291bGQgb3ZlcmZsb3cpIGJ1dCB0aGF0IHdvdWxkIGFsc28gbWFrZSB0aGUgcmVhZGluZ3Mg
dXNlbGVzcyBhcyB0aGUNCmV4dGVybmFsIFZSRUYgd291bGQgYmUgaW52YWxpZCA7IGFuZCB0aGVy
ZSBpcyBhbHNvIHRoZSByaXNrIG9mIGZyeWluZyB0aGUNCmNoaXAgaW4gdGhhdCBjYXNlIFtzb21l
dGhpbmcgeW91IHJlYWxseSBjYW4ndCBwcm90ZWN0IHRoZSB1c2VyIGFnYWluc3RdLg0KDQpUaGUg
aW50ZXJuYWwgVlJFRiBob3dldmVyIGlzIDI1MDAgbVYsIHNvIHRoaW5ncyBzaG91bGQgYmUgZmlu
ZSBmcm9tIHRoYXQNCnBvaW50IG9mIHZpZXcuDQoNClR5cGljYWxseSwgaW4gZGF0YXNoZWV0cyAo
YXQgbGVhc3QgZnJvbSBBbmFsb2cgRGV2aWNlcykgaXQncyBnb29kIHRvIHRha2UgYQ0KbG9vayBh
dCB0aGUgc3BlY2lmaWNhdGlvbnMgc2VjdGlvbnMuDQpbRm9yIEFENzEyNF0gWW91IHdpbGwgc2Vl
IHRoYXQgdGhlIGludGVybmFsIFZSRUYgW3BhZ2UgOF0gaXMgMi41ViAod2l0aA0KYXBwcm94aW1h
dGlvbiBvZiArLy0wLjIlKSBhbmQgZm9yIGV4dGVybmFsIHJlZmVyZW5jZSBpdCBnb2VzIGFsbCB0
aGUgd2F5IHVwDQp0byBBVkRELCB3aGljaCBoYXMgdHlwaWNhbCB2YWx1ZXMgb2YgMi45ViAtIDMu
NlYuDQpTbywgZm9yIHUzMiB0aGlzIGNvZGUgc2hvdWxkIGJlIGZpbmUgYW5kIG5vdCBvdmVyZmxv
dy4NCg0KT25lIHNtYWxsIHRoaW5nIHRoYXQgY2FuIGJlIGNvbmZ1c2luZyBhYm91dCB0aGF0IGNv
ZGUgaW4gQUQ3MTI0IGlzIHRoYXQgaXQNCmdldHMgbXVsdGlwbGllZCB3aXRoIDEwMDAwMDBMTCAo
d2hpY2ggaXMgc2lnbmVkIGxvbmctbG9uZyksIGJ1dCB0aGF0IHNob3VsZA0KYmUgZmluZSwgc2lu
Y2UgdGhlIG9wZXJhdGlvbiBzaG91bGQgYmUgY29udmVydGVkIHRvIHUzMiAodW5zaWduZWQgaW50
KQ0KcmVwcmVzZW50YXRpb24gW2J5IGJlaW5nIGFzc2lnbmVkIHRvIHZyZWZdLCB3aGljaCBlbmRz
IHVwIGJlaW5nIGZpbmUgaW4gdGhlDQplbmQuDQoNCj4gPiANCj4gPiBBbmQgcmVnYXJkaW5nIG91
ciBjb2RlOg0KPiA+ICAgMS0pIFRoZSB2YWwgaW4gb3VyIHdyaXRlX3JhdyBmdW5jdGlvbiBzaG91
bGQgYmUsIGluIGNhc2Ugb2YgR0FJTiwgYQ0KPiA+IG51bWJlciB0aGF0IGJlc3QgYXBwcm94aW1h
dGUgdGhlIGFjdHVhbCBnYWluIHZhbHVlIG9mIDEgb3IgMTI4PyBGb3INCj4gPiBpbnN0YW5jZSwg
aWYgdGhlIHVzZXIgaW5wdXRzIDEyNiwgd2Ugc2hvdWxkIGRlZmF1bHQgdG8gMTI4Pw0KPiANCj4g
V2Ugc2hvdWxkIG5vdCBhbGxvdyB0aGUgdGhlIHVzZXIgdG8gaW5wdXQgdGhlIGdhaW4sIGhlIG5l
ZWRzIHRvIGlucHV0IHRoZQ0KPiBzY2FsZSAoc2VlIHRoZSBtYWlsIGZyb20gSm9uYXRoYW4gYW5k
IHRoZSBhYm92ZSBleHBsYW5hdGlvbikuIEhvd2V2ZXIsIGlmDQo+IHRoZSBjYWxjdWxhdGVkIGdh
aW4gaXMgb25lIHRoYXQgaXMgbm90IHN1cHBvcnRlZCwgc3VjaCBhcyAxMjYsIHdlIHdpbGwNCj4g
c2V0DQo+IHRoZSBjbG9zZXN0IG1hdGNoaW5nIHZhbHVlLCBpbiB0aGlzIGNhc2UgMTI4Lg0KPiAN
Cj4gPiAgIDItKSBJbiB0aGUgY2FzZSBvZiBGSUxURVIsIGlzIGl0IHRoZSBzYW1lPyBJcyB0aGUg
dXNlciBzZW5kaW5nIHRoZQ0KPiA+IHZhbHVlIGluIG1IeiAobWlsaWhlcnR6KT8NCj4gDQo+IFll
cywgaXQgaXMgdGhlIHNhbWUgd2l0aCB0aGUgRklMVEVSLiBZb3UgbmVlZCB0byBhZGQNCj4gYSBJ
SU9fQ0hBTl9JTkZPX1NBTVBfRlJFUSBjYXNlIGluIHlvdXIgd3JpdGVfcmF3KCkgZnVuY3Rpb24u
IEZyb20gdXNlcg0KPiBzcGFjZSwgdGhlIGlucHV0IHZhbHVlIHNob3VsZCBiZSBpbiBIeiwgc29t
ZXRoaW5nIGxpa2UgdGhpczoNCj4gcm9vdDovc3lzL2J1cy9paW8vZGV2aWNlcy9paW86ZGV2aWNl
MD4gZWNobyAxMCA+DQo+IGluX3ZvbHRhZ2Vfc2FtcGxpbmdfZnJlcXVlbmN5DQo+IA0KPiA+IA0K
PiA+IFRoYW5rIHlvdQ0K

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
  2018-11-29 12:19         ` Ardelean, Alexandru
@ 2018-12-01 15:17           ` Jonathan Cameron
  2018-12-01 15:17             ` Jonathan Cameron
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2018-12-01 15:17 UTC (permalink / raw)
  To: Ardelean, Alexandru
  Cc: giuliano.belinassi, Popa, Stefan Serban, kernel-usp,
	linux-kernel, lars, knaack.h, Hennerich, Michael, linux-iio,
	devel, pmeerw, gregkh

On Thu, 29 Nov 2018 12:19:08 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Tue, 2018-11-27 at 06:11 -0500, Popa, Stefan Serban wrote:
> > On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote:
> > Hi, please see bellow
> >  =20
>=20
> One note from me here.
>=20
> > > Hi, thank you for the review
> > >  =20
> > > >=20
> > > > On Thu, 22 Nov 2018 11:01:00 +0000
> > > > "Popa, Stefan Serban" <StefanSerban.Popa@analog.com> wrote: =20
> > > > >=20
> > > > > I think that instead of setting the gain directly, we should use
> > > > > the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780
> > > > > datasheet
> > > > > there
> > > > > is a formula from which the output code can be calculated:
> > > > > Code =3D 2^(N =E2=88=92 1)
> > > > > =C3=97 [(AIN =C3=97 Gain /VREF) + 1]. So, by setting the scale fr=
om user
> > > > > space,
> > > > > the
> > > > > driver can calculate the correct gain by using the formula above.
> > > > > Also, it
> > > > > would be useful to introduce scale available.
> > > > > Furthermore, there is a new
> > > > > ad7124 adc driver which does this exact thing. Take a look here:
> > > > > http
> > > > > s://gi
> > > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad712=
4.
> > > > > c#
> > > > > L337. =20
> > >=20
> > > We have some questions about the code you provided to us:
> > >   1-) What is exactly the inputs for the write_raw function? =20
> >=20
> > In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case.
> > Setting the scale from user space looks something like this: =20
> > root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale=
 . =20
> > Furthermore, in your write_raw() function, val=3D0 and val2=3D298.
> > Knowing that full_scale_voltage =3D Vref / (gain * scale), we can calcu=
late
> > the gain =3D Vref / (full_scale_voltage * scale). We only support two g=
ains
> > (1 and 128), so we need to determine which one fits better with the
> > desired
> > scale. Finally, all we have left to do is to set the gain.=20
> >   =20
> > >   2-) Could you give more details about the math around lines 346-348?
> > > Is it correct to assume that the multiplication at line 346 won't
> > > overflow? (vref is an uint) =20
> >=20
> > It is correct that Vref is in microvolts, so for example, Vref of 2.5V =
 =3D
> > 2500000000uV. It won't overflow since we use the Vref as nominator, whi=
le
> > full_scale_voltage and scale are the denominators.
> >  =20
>=20
> [Regarding the AD7124] I guess I should be noted that the code can
> overflow, but in this case it shouldn't, because (according to the device
> datasheet) the maximum VREF can be 3600 mV.
> A user could specify (in the devicetree) something larger than 4300 mV (a=
nd
> that would overflow) but that would also make the readings useless as the
> external VREF would be invalid ; and there is also the risk of frying the
> chip in that case [something you really can't protect the user against].

Thanks Alex and Stefan for great detail here (glad you got to this before
I did ;)  One thing to add though is that if we have constraints like
this that would mess up the readings, it is sensible to check them
in the driver and at the very least put out a loud warning about the
problem, possibly even refuse to probe the driver.

The number of times I've spent hours chasing configuration problems
only to discover I was doing something stupid and the kernel silently
carried on regardlessly make me very keen on such commonsense protections.

I'm not going to review every driver to ensure they do this, but I'm keen
on them being checked where a possible problem has been identified!

Jonathan
>=20
> The internal VREF however is 2500 mV, so things should be fine from that
> point of view.
>=20
> Typically, in datasheets (at least from Analog Devices) it's good to take=
 a
> look at the specifications sections.
> [For AD7124] You will see that the internal VREF [page 8] is 2.5V (with
> approximation of +/-0.2%) and for external reference it goes all the way =
up
> to AVDD, which has typical values of 2.9V - 3.6V.
> So, for u32 this code should be fine and not overflow.
>=20
> One small thing that can be confusing about that code in AD7124 is that it
> gets multiplied with 1000000LL (which is signed long-long), but that shou=
ld
> be fine, since the operation should be converted to u32 (unsigned int)
> representation [by being assigned to vref], which ends up being fine in t=
he
> end.
>=20
> > >=20
> > > And regarding our code:
> > >   1-) The val in our write_raw function should be, in case of GAIN, a
> > > number that best approximate the actual gain value of 1 or 128? For
> > > instance, if the user inputs 126, we should default to 128? =20
> >=20
> > We should not allow the the user to input the gain, he needs to input t=
he
> > scale (see the mail from Jonathan and the above explanation). However, =
if
> > the calculated gain is one that is not supported, such as 126, we will
> > set
> > the closest matching value, in this case 128.
> >  =20
> > >   2-) In the case of FILTER, is it the same? Is the user sending the
> > > value in mHz (milihertz)? =20
> >=20
> > Yes, it is the same with the FILTER. You need to add
> > a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user
> > space, the input value should be in Hz, something like this: =20
> > root:/sys/bus/iio/devices/iio:device0> echo 10 > =20
> > in_voltage_sampling_frequency
> >  =20
> > >=20
> > > Thank you =20

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support
  2018-12-01 15:17           ` Jonathan Cameron
@ 2018-12-01 15:17             ` Jonathan Cameron
  0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2018-12-01 15:17 UTC (permalink / raw)
  To: Ardelean, Alexandru
  Cc: giuliano.belinassi, Popa, Stefan Serban, kernel-usp,
	linux-kernel, lars, knaack.h, Hennerich, Michael, linux-iio,
	devel, pmeerw, gregkh

On Thu, 29 Nov 2018 12:19:08 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Tue, 2018-11-27 at 06:11 -0500, Popa, Stefan Serban wrote:
> > On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote:
> > Hi, please see bellow
> >   
> 
> One note from me here.
> 
> > > Hi, thank you for the review
> > >   
> > > > 
> > > > On Thu, 22 Nov 2018 11:01:00 +0000
> > > > "Popa, Stefan Serban" <StefanSerban.Popa@analog.com> wrote:  
> > > > > 
> > > > > I think that instead of setting the gain directly, we should use
> > > > > the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780
> > > > > datasheet
> > > > > there
> > > > > is a formula from which the output code can be calculated:
> > > > > Code = 2^(N − 1)
> > > > > × [(AIN × Gain /VREF) + 1]. So, by setting the scale from user
> > > > > space,
> > > > > the
> > > > > driver can calculate the correct gain by using the formula above.
> > > > > Also, it
> > > > > would be useful to introduce scale available.
> > > > > Furthermore, there is a new
> > > > > ad7124 adc driver which does this exact thing. Take a look here:
> > > > > http
> > > > > s://gi
> > > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.
> > > > > c#
> > > > > L337.  
> > > 
> > > We have some questions about the code you provided to us:
> > >   1-) What is exactly the inputs for the write_raw function?  
> > 
> > In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case.
> > Setting the scale from user space looks something like this:  
> > root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale .  
> > Furthermore, in your write_raw() function, val=0 and val2=298.
> > Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate
> > the gain = Vref / (full_scale_voltage * scale). We only support two gains
> > (1 and 128), so we need to determine which one fits better with the
> > desired
> > scale. Finally, all we have left to do is to set the gain. 
> >    
> > >   2-) Could you give more details about the math around lines 346-348?
> > > Is it correct to assume that the multiplication at line 346 won't
> > > overflow? (vref is an uint)  
> > 
> > It is correct that Vref is in microvolts, so for example, Vref of 2.5V  =
> > 2500000000uV. It won't overflow since we use the Vref as nominator, while
> > full_scale_voltage and scale are the denominators.
> >   
> 
> [Regarding the AD7124] I guess I should be noted that the code can
> overflow, but in this case it shouldn't, because (according to the device
> datasheet) the maximum VREF can be 3600 mV.
> A user could specify (in the devicetree) something larger than 4300 mV (and
> that would overflow) but that would also make the readings useless as the
> external VREF would be invalid ; and there is also the risk of frying the
> chip in that case [something you really can't protect the user against].

Thanks Alex and Stefan for great detail here (glad you got to this before
I did ;)  One thing to add though is that if we have constraints like
this that would mess up the readings, it is sensible to check them
in the driver and at the very least put out a loud warning about the
problem, possibly even refuse to probe the driver.

The number of times I've spent hours chasing configuration problems
only to discover I was doing something stupid and the kernel silently
carried on regardlessly make me very keen on such commonsense protections.

I'm not going to review every driver to ensure they do this, but I'm keen
on them being checked where a possible problem has been identified!

Jonathan
> 
> The internal VREF however is 2500 mV, so things should be fine from that
> point of view.
> 
> Typically, in datasheets (at least from Analog Devices) it's good to take a
> look at the specifications sections.
> [For AD7124] You will see that the internal VREF [page 8] is 2.5V (with
> approximation of +/-0.2%) and for external reference it goes all the way up
> to AVDD, which has typical values of 2.9V - 3.6V.
> So, for u32 this code should be fine and not overflow.
> 
> One small thing that can be confusing about that code in AD7124 is that it
> gets multiplied with 1000000LL (which is signed long-long), but that should
> be fine, since the operation should be converted to u32 (unsigned int)
> representation [by being assigned to vref], which ends up being fine in the
> end.
> 
> > > 
> > > And regarding our code:
> > >   1-) The val in our write_raw function should be, in case of GAIN, a
> > > number that best approximate the actual gain value of 1 or 128? For
> > > instance, if the user inputs 126, we should default to 128?  
> > 
> > We should not allow the the user to input the gain, he needs to input the
> > scale (see the mail from Jonathan and the above explanation). However, if
> > the calculated gain is one that is not supported, such as 126, we will
> > set
> > the closest matching value, in this case 128.
> >   
> > >   2-) In the case of FILTER, is it the same? Is the user sending the
> > > value in mHz (milihertz)?  
> > 
> > Yes, it is the same with the FILTER. You need to add
> > a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user
> > space, the input value should be in Hz, something like this:  
> > root:/sys/bus/iio/devices/iio:device0> echo 10 >  
> > in_voltage_sampling_frequency
> >   
> > > 
> > > Thank you  


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-12-02  2:30 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-21 18:04 [PATCH] staging: iio: ad7780: Add gain & filter gpio support Giuliano Belinassi
2018-11-22 11:01 ` Popa, Stefan Serban
2018-11-25 11:08   ` Jonathan Cameron
2018-11-26 19:24     ` Giuliano Belinassi
2018-11-27 11:11       ` Popa, Stefan Serban
2018-11-29 12:19         ` Ardelean, Alexandru
2018-12-01 15:17           ` Jonathan Cameron
2018-12-01 15:17             ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).