* [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@metafoo.de, knaack.h@gmx.de, jic23@kernel.org, Hennerich, Michael, giuliano.belinassi@gmail.com, pmeerw@pmeerw.net, gregkh@linuxfoundation.org Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, devel@driverdev.osuosl.org, kernel-usp@googlegroups.com 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@metafoo.de, knaack.h@gmx.de, Hennerich, Michael, giuliano.belinassi@gmail.com, pmeerw@pmeerw.net, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, devel@driverdev.osuosl.org, kernel-usp@googlegroups.com 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@gmail.com, jic23@kernel.org Cc: kernel-usp@googlegroups.com, linux-kernel@vger.kernel.org, lars@metafoo.de, knaack.h@gmx.de, Ardelean, Alexandru, Hennerich, Michael, linux-iio@vger.kernel.org, devel@driverdev.osuosl.org, pmeerw@pmeerw.net, gregkh@linuxfoundation.org, 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@gmail.com, jic23@kernel.org, Popa, Stefan Serban Cc: kernel-usp@googlegroups.com, linux-kernel@vger.kernel.org, lars@metafoo.de, knaack.h@gmx.de, Hennerich, Michael, linux-iio@vger.kernel.org, devel@driverdev.osuosl.org, pmeerw@pmeerw.net, gregkh@linuxfoundation.org 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@gmail.com, Popa, Stefan Serban, kernel-usp@googlegroups.com, linux-kernel@vger.kernel.org, lars@metafoo.de, knaack.h@gmx.de, Hennerich, Michael, linux-iio@vger.kernel.org, devel@driverdev.osuosl.org, pmeerw@pmeerw.net, gregkh@linuxfoundation.org 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@gmail.com, Popa, Stefan Serban, kernel-usp@googlegroups.com, linux-kernel@vger.kernel.org, lars@metafoo.de, knaack.h@gmx.de, Hennerich, Michael, linux-iio@vger.kernel.org, devel@driverdev.osuosl.org, pmeerw@pmeerw.net, gregkh@linuxfoundation.org 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).