* [PATCH 0/2] staging: iio: ad7780: move out of staging @ 2018-11-28 18:15 Giuliano Belinassi 2018-11-28 18:15 ` [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support Giuliano Belinassi 2018-11-28 18:16 ` [PATCH 2/2] staging: iio: ad7780: Moving ad7780 out of staging Giuliano Belinassi 0 siblings, 2 replies; 10+ messages in thread From: Giuliano Belinassi @ 2018-11-28 18:15 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, stefan.popa, alexandru.Ardelean, renatogeh Cc: linux-iio, devel, linux-kernel, kernel-usp This series of patches add user input to ad7780 'gain' & 'filter' gpio pins. Also, it moves the ad7780 out of staging to the main tree. Giuliano Belinassi (2): staging: iio: ad7780: Add gain & filter gpio support staging: iio: ad7780: Moving ad7780 out of staging drivers/iio/adc/Kconfig | 13 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ad7780.c | 347 +++++++++++++++++++++++++ drivers/staging/iio/adc/Kconfig | 13 - drivers/staging/iio/adc/Makefile | 1 - drivers/staging/iio/adc/ad7780.c | 277 -------------------- include/linux/iio/adc/ad_sigma_delta.h | 5 + 7 files changed, 366 insertions(+), 291 deletions(-) create mode 100644 drivers/iio/adc/ad7780.c delete mode 100644 drivers/staging/iio/adc/ad7780.c -- 2.19.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support 2018-11-28 18:15 [PATCH 0/2] staging: iio: ad7780: move out of staging Giuliano Belinassi @ 2018-11-28 18:15 ` Giuliano Belinassi 2018-11-29 11:18 ` Popa, Stefan Serban 2018-11-28 18:16 ` [PATCH 2/2] staging: iio: ad7780: Moving ad7780 out of staging Giuliano Belinassi 1 sibling, 1 reply; 10+ messages in thread From: Giuliano Belinassi @ 2018-11-28 18:15 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, stefan.popa, alexandru.Ardelean, renatogeh 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> --- Changes in v2: - Now this patch is part of the patchset that aims to remove ad7780 out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2 - Also, now it reads voltage and filter values from the userspace instead of gpio pin states. drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++-- include/linux/iio/adc/ad_sigma_delta.h | 5 ++ 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c index c4a85789c2db..05979a79fda3 100644 --- a/drivers/staging/iio/adc/ad7780.c +++ b/drivers/staging/iio/adc/ad7780.c @@ -39,6 +39,12 @@ #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 + +#define AD7780_GAIN_MIDPOINT 64 +#define AD7780_FILTER_MIDPOINT 13350 + struct ad7780_chip_info { struct iio_chan_spec channel; unsigned int pattern_mask; @@ -50,6 +56,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 +123,65 @@ 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); + const struct ad7780_chip_info *chip_info = st->chip_info; + int uvref, gain; + unsigned int full_scale; + + if (!chip_info->is_ad778x) + return 0; + + switch (m) { + case IIO_CHAN_INFO_SCALE: + if (val != 0) + return -EINVAL; + + uvref = regulator_get_voltage(st->reg); + + if (uvref < 0) + return uvref; + + full_scale = 1 << (chip_info->channel.scan_type.realbits - 1); + gain = DIV_ROUND_CLOSEST(uvref, full_scale); + gain = DIV_ROUND_CLOSEST(gain, val2); + + gpiod_set_value(st->gain_gpio, gain < AD7780_GAIN_MIDPOINT ? 0 : 1); + break; + case IIO_CHAN_INFO_SAMP_FREQ: + if (val2 != 0) + return -EINVAL; + + gpiod_set_value(st->filter_gpio, val < AD7780_FILTER_MIDPOINT ? 0 : 1); + break; + } + + 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 +196,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 +230,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 +280,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] 10+ messages in thread
* Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support 2018-11-28 18:15 ` [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support Giuliano Belinassi @ 2018-11-29 11:18 ` Popa, Stefan Serban 0 siblings, 0 replies; 10+ messages in thread From: Popa, Stefan Serban @ 2018-11-29 11:18 UTC (permalink / raw) To: Ardelean, Alexandru, lars, knaack.h, jic23, Hennerich, Michael, renatogeh, giuliano.belinassi, pmeerw, gregkh Cc: linux-kernel, linux-iio, devel, kernel-usp On Mi, 2018-11-28 at 16:15 -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. > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br> > --- > Changes in v2: > - Now this patch is part of the patchset that aims to remove ad7780 > out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2 > - Also, now it reads voltage and filter values from the userspace > instead of gpio pin states. Hello, Please see bellow. > > drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++-- > include/linux/iio/adc/ad_sigma_delta.h | 5 ++ > 2 files changed, 79 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7780.c > b/drivers/staging/iio/adc/ad7780.c > index c4a85789c2db..05979a79fda3 100644 > --- a/drivers/staging/iio/adc/ad7780.c > +++ b/drivers/staging/iio/adc/ad7780.c > @@ -39,6 +39,12 @@ > #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 > + > +#define AD7780_GAIN_MIDPOINT 64 > +#define AD7780_FILTER_MIDPOINT 13350 > + > struct ad7780_chip_info { > struct iio_chan_spec channel; > unsigned int pattern_mask; > @@ -50,6 +56,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 +123,65 @@ 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); > + const struct ad7780_chip_info *chip_info = st->chip_info; > + int uvref, gain; > + unsigned int full_scale; > + > + if (!chip_info->is_ad778x) > + return 0; > + > + switch (m) { > + case IIO_CHAN_INFO_SCALE: > + if (val != 0) > + return -EINVAL; > + > + uvref = regulator_get_voltage(st->reg); regulator_get_voltage() has already been called in the probe function and the result is stored in st->int_vref_mv. My suggestion would be to use a local vref variable declared as unsigned int. It is my fault that I haven't explained correctly in the previous email, but you need to multiply vref_mv with 1000000LL in order to get the right precision: vref = st->int_vref_mv * 1000000LL. Afterwards you will be able to perform the divisions. > + > + if (uvref < 0) > + return uvref; > + > + full_scale = 1 << (chip_info->channel.scan_type.realbits > - 1); > + gain = DIV_ROUND_CLOSEST(uvref, full_scale); > + gain = DIV_ROUND_CLOSEST(gain, val2); > + > + gpiod_set_value(st->gain_gpio, gain < > AD7780_GAIN_MIDPOINT ? 0 : 1); Once the gain is set, you can store it in st->gain variable. > + case IIO_CHAN_INFO_SAMP_FREQ: > + if (val2 != 0) > + return -EINVAL; > + > + gpiod_set_value(st->filter_gpio, val < > AD7780_FILTER_MIDPOINT ? 0 : 1); This is probably fine, although I am not a big fan of the ternary operator. A simple if else statement would do. However, I don't feel strongly about it, so feel free to disagree. > + break; > + } > + > + 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; It is not obvious to me what is the point of this check. Maybe you can add a comment? > + > + if (val) > st->gain = 1; > else > st->gain = 128; Do we still need this? I am not convinced. > @@ -141,18 +196,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 +230,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 +280,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)) { if the GPIO is optional, then we should continue in case of -ENODEV. Shouldn't we have a devm_gpiod_get_optional() call also for filter_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, \ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support @ 2018-11-29 11:18 ` Popa, Stefan Serban 0 siblings, 0 replies; 10+ messages in thread From: Popa, Stefan Serban @ 2018-11-29 11:18 UTC (permalink / raw) To: Ardelean, Alexandru, lars, knaack.h, jic23, Hennerich, Michael, renatogeh, giuliano.belinassi, pmeerw, gregkh Cc: linux-kernel, linux-iio, devel, kernel-usp T24gTWksIDIwMTgtMTEtMjggYXQgMTY6MTUgLTAyMDAsIEdpdWxpYW5vIEJlbGluYXNzaSB3cm90 ZToNCj4gUHJldmlvdXNseSwgdGhlIEFENzc4MCBkcml2ZXIgb25seSBzdXBwb3J0ZWQgZ3BpbyBm b3IgdGhlICdwb3dlcmRvd24nDQo+IHBpbi4gVGhpcyBjb21taXQgYWRkcyBzdXBwcG9ydCBmb3Ig dGhlICdnYWluJyBhbmQgJ2ZpbHRlcicgcGluLg0KPiANCj4gU2lnbmVkLW9mZi1ieTogR2l1bGlh bm8gQmVsaW5hc3NpIDxnaXVsaWFuby5iZWxpbmFzc2lAdXNwLmJyPg0KPiAtLS0NCj4gQ2hhbmdl cyBpbiB2MjoNCj4gwqDCoMKgwqAtIE5vdyB0aGlzIHBhdGNoIGlzIHBhcnQgb2YgdGhlIHBhdGNo c2V0IHRoYXQgYWltcyB0byByZW1vdmUgYWQ3NzgwDQo+IG91dCBvZiBzdGFnaW5nLiBodHRwczov L21hcmMuaW5mby8/bD1saW51eC1paW8mbT0xNTQyODIzNDk4MDg4OTAmdz0yDQo+IMKgwqDCoMKg LSBBbHNvLCBub3cgaXQgcmVhZHMgdm9sdGFnZSBhbmQgZmlsdGVyIHZhbHVlcyBmcm9tIHRoZSB1 c2Vyc3BhY2UNCj4gaW5zdGVhZCBvZiBncGlvIHBpbiBzdGF0ZXMuDQoNCkhlbGxvLA0KUGxlYXNl IHNlZSBiZWxsb3cuDQoNCj4gDQo+IMKgZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmPC oMKgwqDCoMKgwqDCoHwgNzggKysrKysrKysrKysrKysrKysrKysrKysrLS0NCj4gwqBpbmNsdWRl L2xpbnV4L2lpby9hZGMvYWRfc2lnbWFfZGVsdGEuaCB8wqDCoDUgKysNCj4gwqAyIGZpbGVzIGNo YW5nZWQsIDc5IGluc2VydGlvbnMoKyksIDQgZGVsZXRpb25zKC0pDQo+IA0KPiBkaWZmIC0tZ2l0 IGEvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMNCj4gYi9kcml2ZXJzL3N0YWdpbmcv aWlvL2FkYy9hZDc3ODAuYw0KPiBpbmRleCBjNGE4NTc4OWMyZGIuLjA1OTc5YTc5ZmRhMyAxMDA2 NDQNCj4gLS0tIGEvZHJpdmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMNCj4gKysrIGIvZHJp dmVycy9zdGFnaW5nL2lpby9hZGMvYWQ3NzgwLmMNCj4gQEAgLTM5LDYgKzM5LDEyIEBADQo+IMKg I2RlZmluZSBBRDcxNzBfUEFUVEVSTgkJKEFENzc4MF9QQVQwIHwgQUQ3MTcwX1BBVDIpDQo+IMKg I2RlZmluZSBBRDcxNzBfUEFUVEVSTl9NQVNLCShBRDc3ODBfUEFUMCB8IEFENzc4MF9QQVQxIHwN Cj4gQUQ3MTcwX1BBVDIpDQo+IMKgDQo+ICsjZGVmaW5lIEFENzc4MF9HQUlOX0dQSU8JMA0KPiAr I2RlZmluZSBBRDc3ODBfRklMVEVSX0dQSU8JMQ0KPiArDQo+ICsjZGVmaW5lIEFENzc4MF9HQUlO X01JRFBPSU5UCTY0DQo+ICsjZGVmaW5lIEFENzc4MF9GSUxURVJfTUlEUE9JTlQJMTMzNTANCj4g Kw0KPiDCoHN0cnVjdCBhZDc3ODBfY2hpcF9pbmZvIHsNCj4gwqAJc3RydWN0IGlpb19jaGFuX3Nw ZWMJY2hhbm5lbDsNCj4gwqAJdW5zaWduZWQgaW50CQlwYXR0ZXJuX21hc2s7DQo+IEBAIC01MCw2 ICs1Niw4IEBAIHN0cnVjdCBhZDc3ODBfc3RhdGUgew0KPiDCoAljb25zdCBzdHJ1Y3QgYWQ3Nzgw X2NoaXBfaW5mbwkqY2hpcF9pbmZvOw0KPiDCoAlzdHJ1Y3QgcmVndWxhdG9yCQkqcmVnOw0KPiDC oAlzdHJ1Y3QgZ3Bpb19kZXNjCQkqcG93ZXJkb3duX2dwaW87DQo+ICsJc3RydWN0IGdwaW9fZGVz YwkJKmdhaW5fZ3BpbzsNCj4gKwlzdHJ1Y3QgZ3Bpb19kZXNjCQkqZmlsdGVyX2dwaW87DQo+IMKg CXVuc2lnbmVkIGludAlnYWluOw0KPiDCoA0KPiDCoAlzdHJ1Y3QgYWRfc2lnbWFfZGVsdGEgc2Q7 DQo+IEBAIC0xMTUsMTggKzEyMyw2NSBAQCBzdGF0aWMgaW50IGFkNzc4MF9yZWFkX3JhdyhzdHJ1 Y3QgaWlvX2Rldg0KPiAqaW5kaW9fZGV2LA0KPiDCoAlyZXR1cm4gLUVJTlZBTDsNCj4gwqB9DQo+ IMKgDQo+ICtzdGF0aWMgaW50IGFkNzc4MF93cml0ZV9yYXcoc3RydWN0IGlpb19kZXYgKmluZGlv X2RldiwNCj4gKwkJCcKgwqDCoMKgc3RydWN0IGlpb19jaGFuX3NwZWMgY29uc3QgKmNoYW4sDQo+ ICsJCQnCoMKgwqDCoGludCB2YWwsDQo+ICsJCQnCoMKgwqDCoGludCB2YWwyLA0KPiArCQkJwqDC oMKgwqBsb25nIG0pDQo+ICt7DQo+ICsJc3RydWN0IGFkNzc4MF9zdGF0ZSAqc3QgPSBpaW9fcHJp dihpbmRpb19kZXYpOw0KPiArCWNvbnN0IHN0cnVjdCBhZDc3ODBfY2hpcF9pbmZvICpjaGlwX2lu Zm8gPSBzdC0+Y2hpcF9pbmZvOw0KPiArCWludCB1dnJlZiwgZ2FpbjsNCj4gKwl1bnNpZ25lZCBp bnQgZnVsbF9zY2FsZTsNCj4gKw0KPiArCWlmICghY2hpcF9pbmZvLT5pc19hZDc3OHgpDQo+ICsJ CXJldHVybiAwOw0KPiArDQo+ICsJc3dpdGNoIChtKSB7DQo+ICsJY2FzZSBJSU9fQ0hBTl9JTkZP X1NDQUxFOg0KPiArCQlpZiAodmFsICE9IDApDQo+ICsJCQlyZXR1cm4gLUVJTlZBTDsNCj4gKw0K PiArCQl1dnJlZiA9IHJlZ3VsYXRvcl9nZXRfdm9sdGFnZShzdC0+cmVnKTsNCg0KcmVndWxhdG9y X2dldF92b2x0YWdlKCkgaGFzIGFscmVhZHkgYmVlbiBjYWxsZWQgaW4gdGhlIHByb2JlIGZ1bmN0 aW9uIGFuZA0KdGhlIHJlc3VsdCBpcyBzdG9yZWQgaW7CoHN0LT5pbnRfdnJlZl9tdi4NCk15IHN1 Z2dlc3Rpb24gd291bGQgYmUgdG8gdXNlIGEgbG9jYWwgdnJlZiB2YXJpYWJsZSBkZWNsYXJlZCBh cyB1bnNpZ25lZA0KaW50LiBJdCBpcyBteSBmYXVsdCB0aGF0IEkgaGF2ZW4ndCBleHBsYWluZWQg Y29ycmVjdGx5IGluIHRoZSBwcmV2aW91cw0KZW1haWwsIGJ1dCB5b3UgbmVlZCB0byBtdWx0aXBs eSB2cmVmX212IHdpdGjCoDEwMDAwMDBMTCBpbiBvcmRlciB0byBnZXQgdGhlDQpyaWdodCBwcmVj aXNpb246IHZyZWYgPSBzdC0+aW50X3ZyZWZfbXYgKiAxMDAwMDAwTEwuIEFmdGVyd2FyZHMgeW91 IHdpbGwgYmUNCmFibGUgdG8gcGVyZm9ybSB0aGUgZGl2aXNpb25zLg0KPiArDQo+ICsJCWlmICh1 dnJlZiA8IDApDQo+ICsJCQlyZXR1cm4gdXZyZWY7DQo+ICsNCj4gKwkJZnVsbF9zY2FsZSA9IDEg PDwgKGNoaXBfaW5mby0+Y2hhbm5lbC5zY2FuX3R5cGUucmVhbGJpdHMgDQo+IC0gMSk7DQo+ICsJ CWdhaW4gPSBESVZfUk9VTkRfQ0xPU0VTVCh1dnJlZiwgZnVsbF9zY2FsZSk7DQo+ICsJCWdhaW4g PSBESVZfUk9VTkRfQ0xPU0VTVChnYWluLCB2YWwyKTsNCj4gKw0KPiArCQlncGlvZF9zZXRfdmFs dWUoc3QtPmdhaW5fZ3BpbywgZ2FpbiA8DQo+IEFENzc4MF9HQUlOX01JRFBPSU5UID8gMCA6IDEp Ow0KDQpPbmNlIHRoZSBnYWluIGlzIHNldCwgeW91IGNhbiBzdG9yZSBpdCBpbiBzdC0+Z2FpbiB2 YXJpYWJsZS4NCg0KPiArCWNhc2UgSUlPX0NIQU5fSU5GT19TQU1QX0ZSRVE6DQo+ICsJCWlmICh2 YWwyICE9IDApDQo+ICsJCQlyZXR1cm4gLUVJTlZBTDsNCj4gKw0KPiArCQlncGlvZF9zZXRfdmFs dWUoc3QtPmZpbHRlcl9ncGlvLCB2YWwgPA0KPiBBRDc3ODBfRklMVEVSX01JRFBPSU5UID8gMCA6 IDEpOw0KDQpUaGlzIGlzIHByb2JhYmx5IGZpbmUsIGFsdGhvdWdoIEkgYW0gbm90IGEgYmlnIGZh biBvZiB0aGUgdGVybmFyeSBvcGVyYXRvci4NCkEgc2ltcGxlIGlmIGVsc2Ugc3RhdGVtZW50IHdv dWxkIGRvLiBIb3dldmVyLCBJIGRvbid0IGZlZWwgc3Ryb25nbHkgYWJvdXQNCml0LCBzbyBmZWVs IGZyZWUgdG8gZGlzYWdyZWUuwqANCg0KPiArCWJyZWFrOw0KPiArCX0NCj4gKw0KPiArCXJldHVy biAwOw0KPiArfQ0KPiArDQo+IMKgc3RhdGljIGludCBhZDc3ODBfcG9zdHByb2Nlc3Nfc2FtcGxl KHN0cnVjdCBhZF9zaWdtYV9kZWx0YSAqc2lnbWFfZGVsdGEsDQo+IMKgCQkJCcKgwqDCoMKgwqB1 bnNpZ25lZCBpbnQgcmF3X3NhbXBsZSkNCj4gwqB7DQo+IMKgCXN0cnVjdCBhZDc3ODBfc3RhdGUg KnN0ID0gYWRfc2lnbWFfZGVsdGFfdG9fYWQ3NzgwKHNpZ21hX2RlbHRhKTsNCj4gwqAJY29uc3Qg c3RydWN0IGFkNzc4MF9jaGlwX2luZm8gKmNoaXBfaW5mbyA9IHN0LT5jaGlwX2luZm87DQo+ICsJ aW50IHZhbDsNCj4gwqANCj4gwqAJaWYgKChyYXdfc2FtcGxlICYgQUQ3NzgwX0VSUikgfHwNCj4g wqAJwqDCoMKgwqAoKHJhd19zYW1wbGUgJiBjaGlwX2luZm8tPnBhdHRlcm5fbWFzaykgIT0gY2hp cF9pbmZvLQ0KPiA+cGF0dGVybikpDQo+IMKgCQlyZXR1cm4gLUVJTzsNCj4gwqANCj4gwqAJaWYg KGNoaXBfaW5mby0+aXNfYWQ3Nzh4KSB7DQo+IC0JCWlmIChyYXdfc2FtcGxlICYgQUQ3NzgwX0dB SU4pDQo+ICsJCXZhbCA9IHJhd19zYW1wbGUgJiBBRDc3ODBfR0FJTjsNCj4gKw0KPiArCQlpZiAo dmFsICE9IGdwaW9kX2dldF92YWx1ZShzdC0+Z2Fpbl9ncGlvKSkNCj4gKwkJCXJldHVybiAtRUlP Ow0KDQpJdCBpcyBub3Qgb2J2aW91cyB0byBtZSB3aGF0IGlzIHRoZSBwb2ludCBvZiB0aGlzIGNo ZWNrLiBNYXliZSB5b3UgY2FuIGFkZA0KYSBjb21tZW50Pw0KDQo+ICsNCj4gKwkJaWYgKHZhbCkN Cj4gwqAJCQlzdC0+Z2FpbiA9IDE7DQo+IMKgCQllbHNlDQo+IMKgCQkJc3QtPmdhaW4gPSAxMjg7 DQoNCkRvIHdlIHN0aWxsIG5lZWQgdGhpcz8gSSBhbSBub3QgY29udmluY2VkLsKgDQoNCj4gQEAg LTE0MSwxOCArMTk2LDIwIEBAIHN0YXRpYyBjb25zdCBzdHJ1Y3QgYWRfc2lnbWFfZGVsdGFfaW5m bw0KPiBhZDc3ODBfc2lnbWFfZGVsdGFfaW5mbyA9IHsNCj4gwqAJLmhhc19yZWdpc3RlcnMgPSBm YWxzZSwNCj4gwqB9Ow0KPiDCoA0KPiAtI2RlZmluZSBBRDc3ODBfQ0hBTk5FTChiaXRzLCB3b3Jk c2l6ZSkgXA0KPiArI2RlZmluZSBBRDcxNzBfQ0hBTk5FTChiaXRzLCB3b3Jkc2l6ZSkgXA0KPiDC oAlBRF9TRF9DSEFOTkVMX05PX1NBTVBfRlJFUSgxLCAwLCAwLCBiaXRzLCAzMiwgd29yZHNpemUg LSBiaXRzKQ0KPiArI2RlZmluZSBBRDc3ODBfQ0hBTk5FTChiaXRzLCB3b3Jkc2l6ZSkgXA0KPiAr CUFEX1NEX0NIQU5ORUxfR0FJTl9GSUxURVIoMSwgMCwgMCwgYml0cywgMzIsIHdvcmRzaXplIC0g Yml0cykNCj4gwqANCj4gwqBzdGF0aWMgY29uc3Qgc3RydWN0IGFkNzc4MF9jaGlwX2luZm8gYWQ3 NzgwX2NoaXBfaW5mb190YmxbXSA9IHsNCj4gwqAJW0lEX0FENzE3MF0gPSB7DQo+IC0JCS5jaGFu bmVsID0gQUQ3NzgwX0NIQU5ORUwoMTIsIDI0KSwNCj4gKwkJLmNoYW5uZWwgPSBBRDcxNzBfQ0hB Tk5FTCgxMiwgMjQpLA0KPiDCoAkJLnBhdHRlcm4gPSBBRDcxNzBfUEFUVEVSTiwNCj4gwqAJCS5w YXR0ZXJuX21hc2sgPSBBRDcxNzBfUEFUVEVSTl9NQVNLLA0KPiDCoAkJLmlzX2FkNzc4eCA9IGZh bHNlLA0KPiDCoAl9LA0KPiDCoAlbSURfQUQ3MTcxXSA9IHsNCj4gLQkJLmNoYW5uZWwgPSBBRDc3 ODBfQ0hBTk5FTCgxNiwgMjQpLA0KPiArCQkuY2hhbm5lbCA9IEFENzE3MF9DSEFOTkVMKDE2LCAy NCksDQo+IMKgCQkucGF0dGVybiA9IEFENzE3MF9QQVRURVJOLA0KPiDCoAkJLnBhdHRlcm5fbWFz ayA9IEFENzE3MF9QQVRURVJOX01BU0ssDQo+IMKgCQkuaXNfYWQ3Nzh4ID0gZmFsc2UsDQo+IEBA IC0xNzMsNiArMjMwLDcgQEAgc3RhdGljIGNvbnN0IHN0cnVjdCBhZDc3ODBfY2hpcF9pbmZvDQo+ IGFkNzc4MF9jaGlwX2luZm9fdGJsW10gPSB7DQo+IMKgDQo+IMKgc3RhdGljIGNvbnN0IHN0cnVj dCBpaW9faW5mbyBhZDc3ODBfaW5mbyA9IHsNCj4gwqAJLnJlYWRfcmF3ID0gYWQ3NzgwX3JlYWRf cmF3LA0KPiArCS53cml0ZV9yYXcgPSBhZDc3ODBfd3JpdGVfcmF3LA0KPiDCoH07DQo+IMKgDQo+ IMKgc3RhdGljIGludCBhZDc3ODBfcHJvYmUoc3RydWN0IHNwaV9kZXZpY2UgKnNwaSkNCj4gQEAg LTIyMiw2ICsyODAsMTggQEAgc3RhdGljIGludCBhZDc3ODBfcHJvYmUoc3RydWN0IHNwaV9kZXZp Y2UgKnNwaSkNCj4gwqAJCWdvdG8gZXJyb3JfZGlzYWJsZV9yZWc7DQo+IMKgCX0NCj4gwqANCj4g KwlpZiAoc3QtPmNoaXBfaW5mby0+aXNfYWQ3Nzh4KSB7DQo+ICsJCXN0LT5nYWluX2dwaW8gPSBk ZXZtX2dwaW9kX2dldF9vcHRpb25hbCgmc3BpLT5kZXYsDQo+ICsJCQkJCQkJImdhaW4iLA0KPiAr CQkJCQkJCUdQSU9EX09VVF9ISUdIKTsNCj4gKwkJaWYgKElTX0VSUihzdC0+Z2Fpbl9ncGlvKSkg ew0KDQppZiB0aGUgR1BJTyBpcyBvcHRpb25hbCwgdGhlbiB3ZSBzaG91bGQgY29udGludWUgaW4g Y2FzZSBvZsKgLUVOT0RFVi4NCg0KU2hvdWxkbid0IHdlIGhhdmUgYcKgZGV2bV9ncGlvZF9nZXRf b3B0aW9uYWwoKSBjYWxsIGFsc28gZm9yIGZpbHRlcl9ncGlvPw0KDQo+ICsJCQlyZXQgPSBQVFJf RVJSKHN0LT5nYWluX2dwaW8pOw0KPiArCQkJZGV2X2Vycigmc3BpLT5kZXYsICJGYWlsZWQgdG8g cmVxdWVzdCBnYWluIEdQSU86DQo+ICVkXG4iLA0KPiArCQkJCXJldCk7DQo+ICsJCQlnb3RvIGVy cm9yX2Rpc2FibGVfcmVnOw0KPiArCQl9DQo+ICsJfQ0KPiArDQo+IMKgCXJldCA9IGFkX3NkX3Nl dHVwX2J1ZmZlcl9hbmRfdHJpZ2dlcihpbmRpb19kZXYpOw0KPiDCoAlpZiAocmV0KQ0KPiDCoAkJ Z290byBlcnJvcl9kaXNhYmxlX3JlZzsNCj4gZGlmZiAtLWdpdCBhL2luY2x1ZGUvbGludXgvaWlv L2FkYy9hZF9zaWdtYV9kZWx0YS5oDQo+IGIvaW5jbHVkZS9saW51eC9paW8vYWRjL2FkX3NpZ21h X2RlbHRhLmgNCj4gaW5kZXggNzMwZWFkMWE0NmRmLi42Y2FkYWI2ZmQ1ZmQgMTAwNjQ0DQo+IC0t LSBhL2luY2x1ZGUvbGludXgvaWlvL2FkYy9hZF9zaWdtYV9kZWx0YS5oDQo+ICsrKyBiL2luY2x1 ZGUvbGludXgvaWlvL2FkYy9hZF9zaWdtYV9kZWx0YS5oDQo+IEBAIC0xNzMsNiArMTczLDExIEBA IGludCBhZF9zZF92YWxpZGF0ZV90cmlnZ2VyKHN0cnVjdCBpaW9fZGV2DQo+ICppbmRpb19kZXYs IHN0cnVjdCBpaW9fdHJpZ2dlciAqdHJpZyk7DQo+IMKgCV9fQURfU0RfQ0hBTk5FTChfc2ksIF9j aGFubmVsLCAtMSwgX2FkZHJlc3MsIF9iaXRzLCBcDQo+IMKgCQlfc3RvcmFnZWJpdHMsIF9zaGlm dCwgTlVMTCwgSUlPX1ZPTFRBR0UsIDApDQo+IMKgDQo+ICsjZGVmaW5lIEFEX1NEX0NIQU5ORUxf R0FJTl9GSUxURVIoX3NpLCBfY2hhbm5lbCwgX2FkZHJlc3MsIF9iaXRzLCBcDQo+ICsJX3N0b3Jh Z2ViaXRzLCBfc2hpZnQpIFwNCj4gKwlfX0FEX1NEX0NIQU5ORUwoX3NpLCBfY2hhbm5lbCwgLTEs IF9hZGRyZXNzLCBfYml0cywgXA0KPiArCQlfc3RvcmFnZWJpdHMsIF9zaGlmdCwgTlVMTCwgSUlP X1ZPTFRBR0UsDQo+IEJJVChJSU9fQ0hBTl9JTkZPX1JBVykpDQo+ICsNCj4gwqAjZGVmaW5lIEFE X1NEX1RFTVBfQ0hBTk5FTChfc2ksIF9hZGRyZXNzLCBfYml0cywgX3N0b3JhZ2ViaXRzLCBfc2hp ZnQpIFwNCj4gwqAJX19BRF9TRF9DSEFOTkVMKF9zaSwgMCwgLTEsIF9hZGRyZXNzLCBfYml0cywg XA0KPiDCoAkJX3N0b3JhZ2ViaXRzLCBfc2hpZnQsIE5VTEwsIElJT19URU1QLCBc ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support 2018-11-29 11:18 ` Popa, Stefan Serban (?) @ 2018-11-29 13:02 ` Giuliano Augusto Faulin Belinassi 2018-12-01 17:23 ` Jonathan Cameron -1 siblings, 1 reply; 10+ messages in thread From: Giuliano Augusto Faulin Belinassi @ 2018-11-29 13:02 UTC (permalink / raw) To: StefanSerban.Popa Cc: alexandru.Ardelean, lars, knaack.h, jic23, Michael.Hennerich, Renato Geh, giuliano.belinassi, Peter Meerwald-Stadler, gregkh, linux-kernel, linux-iio, devel, kernel-usp Hi On Thu, Nov 29, 2018 at 9:18 AM Popa, Stefan Serban <StefanSerban.Popa@analog.com> wrote: > > On Mi, 2018-11-28 at 16:15 -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. > > > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br> > > --- > > Changes in v2: > > - Now this patch is part of the patchset that aims to remove ad7780 > > out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2 > > - Also, now it reads voltage and filter values from the userspace > > instead of gpio pin states. > > Hello, > Please see bellow. > > > > > drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++-- > > include/linux/iio/adc/ad_sigma_delta.h | 5 ++ > > 2 files changed, 79 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/iio/adc/ad7780.c > > b/drivers/staging/iio/adc/ad7780.c > > index c4a85789c2db..05979a79fda3 100644 > > --- a/drivers/staging/iio/adc/ad7780.c > > +++ b/drivers/staging/iio/adc/ad7780.c > > @@ -39,6 +39,12 @@ > > #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 > > + > > +#define AD7780_GAIN_MIDPOINT 64 > > +#define AD7780_FILTER_MIDPOINT 13350 > > + > > struct ad7780_chip_info { > > struct iio_chan_spec channel; > > unsigned int pattern_mask; > > @@ -50,6 +56,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 +123,65 @@ 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); > > + const struct ad7780_chip_info *chip_info = st->chip_info; > > + int uvref, gain; > > + unsigned int full_scale; > > + > > + if (!chip_info->is_ad778x) > > + return 0; > > + > > + switch (m) { > > + case IIO_CHAN_INFO_SCALE: > > + if (val != 0) > > + return -EINVAL; > > + > > + uvref = regulator_get_voltage(st->reg); > > regulator_get_voltage() has already been called in the probe function and > the result is stored in st->int_vref_mv. This was removed in commit 9eae69ddbc4717a0bd702eddac76c7848773bf71 because the value was not being updated. But I agree if the vref voltage is not going to change at all after the initialization, then this value should be kept in memory. > My suggestion would be to use a local vref variable declared as unsigned > int. It is my fault that I haven't explained correctly in the previous > email, but you need to multiply vref_mv with 1000000LL in order to get the > right precision: vref = st->int_vref_mv * 1000000LL. Afterwards you will be > able to perform the divisions. Thanks for this info! :-) Shouldn't we store this in uV (microVolts)? This will yield a more accurate result after the multiplication. > > + > > + if (uvref < 0) > > + return uvref; > > + > > + full_scale = 1 << (chip_info->channel.scan_type.realbits > > - 1); > > + gain = DIV_ROUND_CLOSEST(uvref, full_scale); > > + gain = DIV_ROUND_CLOSEST(gain, val2); > > + > > + gpiod_set_value(st->gain_gpio, gain < > > AD7780_GAIN_MIDPOINT ? 0 : 1); > > Once the gain is set, you can store it in st->gain variable. Yes, we forgot it. > > > + case IIO_CHAN_INFO_SAMP_FREQ: > > + if (val2 != 0) > > + return -EINVAL; > > + > > + gpiod_set_value(st->filter_gpio, val < > > AD7780_FILTER_MIDPOINT ? 0 : 1); > > This is probably fine, although I am not a big fan of the ternary operator. > A simple if else statement would do. However, I don't feel strongly about > it, so feel free to disagree. > > > + break; > > + } > > + > > + 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; > > It is not obvious to me what is the point of this check. Maybe you can add > a comment? It seems to be a redundancy check. It is getting the 32-bits raw_output, getting the bit that represents the GAIN value and checking if the pin is set accordingly (see Figure 22 of datasheet, page 13). Is this correct? If yes we add a comment explaining this. > > > + > > + if (val) > > st->gain = 1; > > else > > st->gain = 128; > > Do we still need this? I am not convinced. No, I don't think so. Thanks for pointing this out :-) > > > @@ -141,18 +196,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 +230,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 +280,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)) { > > if the GPIO is optional, then we should continue in case of -ENODEV. > > Shouldn't we have a devm_gpiod_get_optional() call also for filter_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, \ > > -- > You received this message because you are subscribed to the Google Groups "Kernel USP" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com. > To post to this group, send email to kernel-usp@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/1543490289.11186.22.camel%40analog.com. > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support 2018-11-29 13:02 ` Giuliano Augusto Faulin Belinassi @ 2018-12-01 17:23 ` Jonathan Cameron 2019-01-18 20:19 ` Renato Lui Geh 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Cameron @ 2018-12-01 17:23 UTC (permalink / raw) To: Giuliano Augusto Faulin Belinassi Cc: StefanSerban.Popa, alexandru.Ardelean, lars, knaack.h, Michael.Hennerich, Renato Geh, giuliano.belinassi, Peter Meerwald-Stadler, gregkh, linux-kernel, linux-iio, devel, kernel-usp On Thu, 29 Nov 2018 11:02:46 -0200 Giuliano Augusto Faulin Belinassi <giuliano.belinassi@usp.br> wrote: > Hi A few follow ups from me having read the result in patch 2. Jonathan > > On Thu, Nov 29, 2018 at 9:18 AM Popa, Stefan Serban > <StefanSerban.Popa@analog.com> wrote: > > > > On Mi, 2018-11-28 at 16:15 -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. > > > > > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br> > > > --- > > > Changes in v2: > > > - Now this patch is part of the patchset that aims to remove ad7780 > > > out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2 > > > - Also, now it reads voltage and filter values from the userspace > > > instead of gpio pin states. > > > > Hello, > > Please see bellow. > > > > > > > > drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++-- > > > include/linux/iio/adc/ad_sigma_delta.h | 5 ++ > > > 2 files changed, 79 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/staging/iio/adc/ad7780.c > > > b/drivers/staging/iio/adc/ad7780.c > > > index c4a85789c2db..05979a79fda3 100644 > > > --- a/drivers/staging/iio/adc/ad7780.c > > > +++ b/drivers/staging/iio/adc/ad7780.c > > > @@ -39,6 +39,12 @@ > > > #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 > > > + > > > +#define AD7780_GAIN_MIDPOINT 64 > > > +#define AD7780_FILTER_MIDPOINT 13350 > > > + > > > struct ad7780_chip_info { > > > struct iio_chan_spec channel; > > > unsigned int pattern_mask; > > > @@ -50,6 +56,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 +123,65 @@ 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); > > > + const struct ad7780_chip_info *chip_info = st->chip_info; > > > + int uvref, gain; > > > + unsigned int full_scale; > > > + > > > + if (!chip_info->is_ad778x) > > > + return 0; > > > + > > > + switch (m) { > > > + case IIO_CHAN_INFO_SCALE: > > > + if (val != 0) > > > + return -EINVAL; > > > + > > > + uvref = regulator_get_voltage(st->reg); > > > > regulator_get_voltage() has already been called in the probe function and > > the result is stored in st->int_vref_mv. > > This was removed in commit 9eae69ddbc4717a0bd702eddac76c7848773bf71 > because the value was not being updated. But I agree if the vref > voltage is not going to change at all after the initialization, then > this value should be kept in memory. > > > My suggestion would be to use a local vref variable declared as unsigned > > int. It is my fault that I haven't explained correctly in the previous > > email, but you need to multiply vref_mv with 1000000LL in order to get the > > right precision: vref = st->int_vref_mv * 1000000LL. Afterwards you will be > > able to perform the divisions. > > Thanks for this info! :-) > Shouldn't we store this in uV (microVolts)? This will yield a more > accurate result after the multiplication. > > > > + > > > + if (uvref < 0) > > > + return uvref; > > > + > > > + full_scale = 1 << (chip_info->channel.scan_type.realbits > > > - 1); > > > + gain = DIV_ROUND_CLOSEST(uvref, full_scale); > > > + gain = DIV_ROUND_CLOSEST(gain, val2); > > > + > > > + gpiod_set_value(st->gain_gpio, gain < > > > AD7780_GAIN_MIDPOINT ? 0 : 1); > > > > Once the gain is set, you can store it in st->gain variable. > > Yes, we forgot it. > > > > > > + case IIO_CHAN_INFO_SAMP_FREQ: > > > + if (val2 != 0) > > > + return -EINVAL; comment I raised in patch 2 about the odd preciseness of insisting no decimal places, but matching any value based on a threshold on the whole number part. I'd also expect to see read_raw support for this. > > > + > > > + gpiod_set_value(st->filter_gpio, val < > > > AD7780_FILTER_MIDPOINT ? 0 : 1); > > > > This is probably fine, although I am not a big fan of the ternary operator. > > A simple if else statement would do. However, I don't feel strongly about > > it, so feel free to disagree. > > > > > + break; > > > + } > > > + > > > + 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; > > > > It is not obvious to me what is the point of this check. Maybe you can add > > a comment? > > It seems to be a redundancy check. It is getting the 32-bits > raw_output, getting the bit that represents the GAIN value and > checking if the pin is set accordingly (see Figure 22 of datasheet, > page 13). Is this correct? If yes we add a comment explaining this. > > > > > > + > > > + if (val) > > > st->gain = 1; > > > else > > > st->gain = 128; > > > > Do we still need this? I am not convinced. > No, I don't think so. Thanks for pointing this out :-) > > > > > > @@ -141,18 +196,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 +230,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 +280,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)) { > > > > if the GPIO is optional, then we should continue in case of -ENODEV. > > > > Shouldn't we have a devm_gpiod_get_optional() call also for filter_gpio? I had to check this one... * This is equivalent to gpiod_get(), except that when no GPIO was assigned to * the requested function it will return NULL. This is convenient for drivers * that need to handle optional GPIOs. So nope, it shouldn't return -ENODEV; unlike the clock equivalent which IIRC does... > > > > > + 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, \ > > > > -- > > You received this message because you are subscribed to the Google Groups "Kernel USP" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com. > > To post to this group, send email to kernel-usp@googlegroups.com. > > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/1543490289.11186.22.camel%40analog.com. > > For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support 2018-12-01 17:23 ` Jonathan Cameron @ 2019-01-18 20:19 ` Renato Lui Geh 2019-01-19 16:11 ` Jonathan Cameron 0 siblings, 1 reply; 10+ messages in thread From: Renato Lui Geh @ 2019-01-18 20:19 UTC (permalink / raw) To: Jonathan Cameron Cc: Giuliano Augusto Faulin Belinassi, StefanSerban.Popa, alexandru.Ardelean, lars, knaack.h, Michael.Hennerich, Renato Geh, giuliano.belinassi, Peter Meerwald-Stadler, gregkh, linux-kernel, linux-iio, devel, kernel-usp Hi, Sorry for the (extremely) late reply. Comments inline. Renato On 12/01, Jonathan Cameron wrote: >On Thu, 29 Nov 2018 11:02:46 -0200 >Giuliano Augusto Faulin Belinassi <giuliano.belinassi@usp.br> wrote: > >> Hi > >A few follow ups from me having read the result in patch 2. > >Jonathan >> >> On Thu, Nov 29, 2018 at 9:18 AM Popa, Stefan Serban >> <StefanSerban.Popa@analog.com> wrote: >> > >> > On Mi, 2018-11-28 at 16:15 -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. >> > > >> > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br> >> > > --- >> > > Changes in v2: >> > > - Now this patch is part of the patchset that aims to remove ad7780 >> > > out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2 >> > > - Also, now it reads voltage and filter values from the userspace >> > > instead of gpio pin states. >> > >> > Hello, >> > Please see bellow. >> > >> > > >> > > drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++-- >> > > include/linux/iio/adc/ad_sigma_delta.h | 5 ++ >> > > 2 files changed, 79 insertions(+), 4 deletions(-) >> > > >> > > diff --git a/drivers/staging/iio/adc/ad7780.c >> > > b/drivers/staging/iio/adc/ad7780.c >> > > index c4a85789c2db..05979a79fda3 100644 >> > > --- a/drivers/staging/iio/adc/ad7780.c >> > > +++ b/drivers/staging/iio/adc/ad7780.c >> > > @@ -39,6 +39,12 @@ >> > > #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 >> > > + >> > > +#define AD7780_GAIN_MIDPOINT 64 >> > > +#define AD7780_FILTER_MIDPOINT 13350 >> > > + >> > > struct ad7780_chip_info { >> > > struct iio_chan_spec channel; >> > > unsigned int pattern_mask; >> > > @@ -50,6 +56,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 +123,65 @@ 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); >> > > + const struct ad7780_chip_info *chip_info = st->chip_info; >> > > + int uvref, gain; >> > > + unsigned int full_scale; >> > > + >> > > + if (!chip_info->is_ad778x) >> > > + return 0; >> > > + >> > > + switch (m) { >> > > + case IIO_CHAN_INFO_SCALE: >> > > + if (val != 0) >> > > + return -EINVAL; >> > > + >> > > + uvref = regulator_get_voltage(st->reg); >> > >> > regulator_get_voltage() has already been called in the probe function and >> > the result is stored in st->int_vref_mv. >> >> This was removed in commit 9eae69ddbc4717a0bd702eddac76c7848773bf71 >> because the value was not being updated. But I agree if the vref >> voltage is not going to change at all after the initialization, then >> this value should be kept in memory. Why wouldn't the vref voltage not change after initialization? Shouldn't we keep reading and updating this in read_raw? >> >> > My suggestion would be to use a local vref variable declared as unsigned >> > int. It is my fault that I haven't explained correctly in the previous >> > email, but you need to multiply vref_mv with 1000000LL in order to get the >> > right precision: vref = st->int_vref_mv * 1000000LL. Afterwards you will be >> > able to perform the divisions. Shouldn't we read vref inside read_raw in order to get up-to-date readings on voltage values? Or should we keep reading from a cached value? >> >> Thanks for this info! :-) >> Shouldn't we store this in uV (microVolts)? This will yield a more >> accurate result after the multiplication. >> >> > > + >> > > + if (uvref < 0) >> > > + return uvref; >> > > + >> > > + full_scale = 1 << (chip_info->channel.scan_type.realbits >> > > - 1); >> > > + gain = DIV_ROUND_CLOSEST(uvref, full_scale); >> > > + gain = DIV_ROUND_CLOSEST(gain, val2); >> > > + >> > > + gpiod_set_value(st->gain_gpio, gain < >> > > AD7780_GAIN_MIDPOINT ? 0 : 1); >> > >> > Once the gain is set, you can store it in st->gain variable. >> >> Yes, we forgot it. >> >> > >> > > + case IIO_CHAN_INFO_SAMP_FREQ: >> > > + if (val2 != 0) >> > > + return -EINVAL; >comment I raised in patch 2 about the odd preciseness of insisting >no decimal places, but matching any value based on a threshold on the >whole number part. I see. I thought the filter input was in mHz. So should I compare 1000*val with AD7780_FILTER_MIDPOINT? > >I'd also expect to see read_raw support for this. > >> > > + >> > > + gpiod_set_value(st->filter_gpio, val < >> > > AD7780_FILTER_MIDPOINT ? 0 : 1); >> > >> > This is probably fine, although I am not a big fan of the ternary operator. >> > A simple if else statement would do. However, I don't feel strongly about >> > it, so feel free to disagree. >> > >> > > + break; >> > > + } >> > > + >> > > + 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; >> > >> > It is not obvious to me what is the point of this check. Maybe you can add >> > a comment? If I'm not mistaken, we had agreed earlier to remove this altogether, as having a redundancy check could potentially slow down reading. >> >> It seems to be a redundancy check. It is getting the 32-bits >> raw_output, getting the bit that represents the GAIN value and >> checking if the pin is set accordingly (see Figure 22 of datasheet, >> page 13). Is this correct? If yes we add a comment explaining this. >> >> > >> > > + >> > > + if (val) >> > > st->gain = 1; >> > > else >> > > st->gain = 128; >> > >> > Do we still need this? I am not convinced. >> No, I don't think so. Thanks for pointing this out :-) >> >> > >> > > @@ -141,18 +196,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 +230,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 +280,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)) { >> > >> > if the GPIO is optional, then we should continue in case of -ENODEV. >> > >> > Shouldn't we have a devm_gpiod_get_optional() call also for filter_gpio? >I had to check this one... > > * This is equivalent to gpiod_get(), except that when no GPIO was assigned to > * the requested function it will return NULL. This is convenient for drivers > * that need to handle optional GPIOs. > >So nope, it shouldn't return -ENODEV; unlike the clock equivalent which >IIRC does... > >> > >> > > + 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, \ >> > >> > -- >> > You received this message because you are subscribed to the Google Groups "Kernel USP" group. >> > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com. >> > To post to this group, send email to kernel-usp@googlegroups.com. >> > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/1543490289.11186.22.camel%40analog.com. >> > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support 2019-01-18 20:19 ` Renato Lui Geh @ 2019-01-19 16:11 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2019-01-19 16:11 UTC (permalink / raw) To: Renato Lui Geh Cc: Giuliano Augusto Faulin Belinassi, StefanSerban.Popa, alexandru.Ardelean, lars, knaack.h, Michael.Hennerich, giuliano.belinassi, Peter Meerwald-Stadler, gregkh, linux-kernel, linux-iio, devel, kernel-usp On Fri, 18 Jan 2019 18:19:40 -0200 Renato Lui Geh <renatogeh@gmail.com> wrote: > Hi, > > Sorry for the (extremely) late reply. > > Comments inline. > > Renato > > On 12/01, Jonathan Cameron wrote: > >On Thu, 29 Nov 2018 11:02:46 -0200 > >Giuliano Augusto Faulin Belinassi <giuliano.belinassi@usp.br> wrote: > > > >> Hi > > > >A few follow ups from me having read the result in patch 2. > > > >Jonathan > >> > >> On Thu, Nov 29, 2018 at 9:18 AM Popa, Stefan Serban > >> <StefanSerban.Popa@analog.com> wrote: > >> > > >> > On Mi, 2018-11-28 at 16:15 -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. > >> > > > >> > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br> > >> > > --- > >> > > Changes in v2: > >> > > - Now this patch is part of the patchset that aims to remove ad7780 > >> > > out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2 > >> > > - Also, now it reads voltage and filter values from the userspace > >> > > instead of gpio pin states. > >> > > >> > Hello, > >> > Please see bellow. > >> > > >> > > > >> > > drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++-- > >> > > include/linux/iio/adc/ad_sigma_delta.h | 5 ++ > >> > > 2 files changed, 79 insertions(+), 4 deletions(-) > >> > > > >> > > diff --git a/drivers/staging/iio/adc/ad7780.c > >> > > b/drivers/staging/iio/adc/ad7780.c > >> > > index c4a85789c2db..05979a79fda3 100644 > >> > > --- a/drivers/staging/iio/adc/ad7780.c > >> > > +++ b/drivers/staging/iio/adc/ad7780.c > >> > > @@ -39,6 +39,12 @@ > >> > > #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 > >> > > + > >> > > +#define AD7780_GAIN_MIDPOINT 64 > >> > > +#define AD7780_FILTER_MIDPOINT 13350 > >> > > + > >> > > struct ad7780_chip_info { > >> > > struct iio_chan_spec channel; > >> > > unsigned int pattern_mask; > >> > > @@ -50,6 +56,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 +123,65 @@ 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); > >> > > + const struct ad7780_chip_info *chip_info = st->chip_info; > >> > > + int uvref, gain; > >> > > + unsigned int full_scale; > >> > > + > >> > > + if (!chip_info->is_ad778x) > >> > > + return 0; > >> > > + > >> > > + switch (m) { > >> > > + case IIO_CHAN_INFO_SCALE: > >> > > + if (val != 0) > >> > > + return -EINVAL; > >> > > + > >> > > + uvref = regulator_get_voltage(st->reg); > >> > > >> > regulator_get_voltage() has already been called in the probe function and > >> > the result is stored in st->int_vref_mv. > >> > >> This was removed in commit 9eae69ddbc4717a0bd702eddac76c7848773bf71 > >> because the value was not being updated. But I agree if the vref > >> voltage is not going to change at all after the initialization, then > >> this value should be kept in memory. > > Why wouldn't the vref voltage not change after initialization? Shouldn't > we keep reading and updating this in read_raw? It may well change so bbest to check it. > >> > >> > My suggestion would be to use a local vref variable declared as unsigned > >> > int. It is my fault that I haven't explained correctly in the previous > >> > email, but you need to multiply vref_mv with 1000000LL in order to get the > >> > right precision: vref = st->int_vref_mv * 1000000LL. Afterwards you will be > >> > able to perform the divisions. > > Shouldn't we read vref inside read_raw in order to get up-to-date > readings on voltage values? Or should we keep reading from a cached > value? I agree. We don't want to do it in a fast path as it 'probably' won't change after the initial boot is done without some deliberate intervention but fine to read it whenever we are dealing with anything other than reading the ADC value (so reading the scale or similar). > >> > >> Thanks for this info! :-) > >> Shouldn't we store this in uV (microVolts)? This will yield a more > >> accurate result after the multiplication. > >> > >> > > + > >> > > + if (uvref < 0) > >> > > + return uvref; > >> > > + > >> > > + full_scale = 1 << (chip_info->channel.scan_type.realbits > >> > > - 1); > >> > > + gain = DIV_ROUND_CLOSEST(uvref, full_scale); > >> > > + gain = DIV_ROUND_CLOSEST(gain, val2); > >> > > + > >> > > + gpiod_set_value(st->gain_gpio, gain < > >> > > AD7780_GAIN_MIDPOINT ? 0 : 1); > >> > > >> > Once the gain is set, you can store it in st->gain variable. > >> > >> Yes, we forgot it. > >> > >> > > >> > > + case IIO_CHAN_INFO_SAMP_FREQ: > >> > > + if (val2 != 0) > >> > > + return -EINVAL; > >comment I raised in patch 2 about the odd preciseness of insisting > >no decimal places, but matching any value based on a threshold on the > >whole number part. > > I see. I thought the filter input was in mHz. So should I compare > 1000*val with AD7780_FILTER_MIDPOINT? Not totally sure what I was going on about here.. I think I was just raising that you should use 1000*val + val2/1000 to do the rounding with the decimal part taken into account. > > > >I'd also expect to see read_raw support for this. > > > >> > > + > >> > > + gpiod_set_value(st->filter_gpio, val < > >> > > AD7780_FILTER_MIDPOINT ? 0 : 1); > >> > > >> > This is probably fine, although I am not a big fan of the ternary operator. > >> > A simple if else statement would do. However, I don't feel strongly about > >> > it, so feel free to disagree. > >> > > >> > > + break; > >> > > + } > >> > > + > >> > > + 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; > >> > > >> > It is not obvious to me what is the point of this check. Maybe you can add > >> > a comment? > > If I'm not mistaken, we had agreed earlier to remove this altogether, as > having a redundancy check could potentially slow down reading. Works for me. > >> > >> It seems to be a redundancy check. It is getting the 32-bits > >> raw_output, getting the bit that represents the GAIN value and > >> checking if the pin is set accordingly (see Figure 22 of datasheet, > >> page 13). Is this correct? If yes we add a comment explaining this. > >> > >> > ... ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] staging: iio: ad7780: Moving ad7780 out of staging 2018-11-28 18:15 [PATCH 0/2] staging: iio: ad7780: move out of staging Giuliano Belinassi 2018-11-28 18:15 ` [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support Giuliano Belinassi @ 2018-11-28 18:16 ` Giuliano Belinassi 2018-12-01 17:20 ` Jonathan Cameron 1 sibling, 1 reply; 10+ messages in thread From: Giuliano Belinassi @ 2018-11-28 18:16 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, stefan.popa, alexandru.Ardelean, renatogeh Cc: linux-iio, devel, linux-kernel, kernel-usp Move ad7780 sigma-delta adc out of staging to the main tree Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com> --- drivers/iio/adc/Kconfig | 13 ++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ad7780.c | 347 +++++++++++++++++++++++++++++++ drivers/staging/iio/adc/Kconfig | 13 -- drivers/staging/iio/adc/Makefile | 1 - drivers/staging/iio/adc/ad7780.c | 347 ------------------------------- 6 files changed, 361 insertions(+), 361 deletions(-) create mode 100644 drivers/iio/adc/ad7780.c delete mode 100644 drivers/staging/iio/adc/ad7780.c diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 575bf69fea57..e517425e364d 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -92,6 +92,19 @@ config AD7793 To compile this driver as a module, choose M here: the module will be called AD7793. +config AD7780 + tristate "Analog Devices AD7780 and similar ADCs driver" + depends on SPI + depends on GPIOLIB || COMPILE_TEST + select AD_SIGMA_DELTA + help + Say yes here to build support for Analog Devices AD7170, AD7171, + AD7780 and AD7781 SPI analog to digital converters (ADC). + If unsure, say N (but it's safe to say "Y"). + + To compile this driver as a module, choose M here: the + module will be called ad7780. + config AD7887 tristate "Analog Devices AD7887 ADC driver" depends on SPI diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 91ba28aeb150..3301ca10b385 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_AD7298) += ad7298.o obj-$(CONFIG_AD7923) += ad7923.o obj-$(CONFIG_AD7476) += ad7476.o obj-$(CONFIG_AD7766) += ad7766.o +obj-$(CONFIG_AD7780) += ad7780.o obj-$(CONFIG_AD7791) += ad7791.o obj-$(CONFIG_AD7793) += ad7793.o obj-$(CONFIG_AD7887) += ad7887.o diff --git a/drivers/iio/adc/ad7780.c b/drivers/iio/adc/ad7780.c new file mode 100644 index 000000000000..05979a79fda3 --- /dev/null +++ b/drivers/iio/adc/ad7780.c @@ -0,0 +1,347 @@ +/* + * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver + * + * Copyright 2011 Analog Devices Inc. + * + * Licensed under the GPL-2. + */ + +#include <linux/interrupt.h> +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/sysfs.h> +#include <linux/spi/spi.h> +#include <linux/regulator/consumer.h> +#include <linux/err.h> +#include <linux/sched.h> +#include <linux/gpio/consumer.h> +#include <linux/module.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/adc/ad_sigma_delta.h> + +#define AD7780_RDY BIT(7) +#define AD7780_FILTER BIT(6) +#define AD7780_ERR BIT(5) +#define AD7780_ID1 BIT(4) +#define AD7780_ID0 BIT(3) +#define AD7780_GAIN BIT(2) +#define AD7780_PAT1 BIT(1) +#define AD7780_PAT0 BIT(0) + +#define AD7780_PATTERN (AD7780_PAT0) +#define AD7780_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1) + +#define AD7170_PAT2 BIT(2) + +#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 + +#define AD7780_GAIN_MIDPOINT 64 +#define AD7780_FILTER_MIDPOINT 13350 + +struct ad7780_chip_info { + struct iio_chan_spec channel; + unsigned int pattern_mask; + unsigned int pattern; + bool is_ad778x; +}; + +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; +}; + +enum ad7780_supported_device_ids { + ID_AD7170, + ID_AD7171, + ID_AD7780, + ID_AD7781, +}; + +static struct ad7780_state *ad_sigma_delta_to_ad7780(struct ad_sigma_delta *sd) +{ + return container_of(sd, struct ad7780_state, sd); +} + +static int ad7780_set_mode(struct ad_sigma_delta *sigma_delta, + enum ad_sigma_delta_mode mode) +{ + struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta); + unsigned int val; + + switch (mode) { + case AD_SD_MODE_SINGLE: + case AD_SD_MODE_CONTINUOUS: + val = 1; + break; + default: + val = 0; + break; + } + + gpiod_set_value(st->powerdown_gpio, val); + + return 0; +} + +static int ad7780_read_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); + int voltage_uv; + + switch (m) { + case IIO_CHAN_INFO_RAW: + return ad_sigma_delta_single_conversion(indio_dev, chan, val); + case IIO_CHAN_INFO_SCALE: + voltage_uv = regulator_get_voltage(st->reg); + if (voltage_uv < 0) + return voltage_uv; + *val = (voltage_uv / 1000) * st->gain; + *val2 = chan->scan_type.realbits - 1; + return IIO_VAL_FRACTIONAL_LOG2; + case IIO_CHAN_INFO_OFFSET: + *val = -(1 << (chan->scan_type.realbits - 1)); + return IIO_VAL_INT; + } + + 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); + const struct ad7780_chip_info *chip_info = st->chip_info; + int uvref, gain; + unsigned int full_scale; + + if (!chip_info->is_ad778x) + return 0; + + switch (m) { + case IIO_CHAN_INFO_SCALE: + if (val != 0) + return -EINVAL; + + uvref = regulator_get_voltage(st->reg); + + if (uvref < 0) + return uvref; + + full_scale = 1 << (chip_info->channel.scan_type.realbits - 1); + gain = DIV_ROUND_CLOSEST(uvref, full_scale); + gain = DIV_ROUND_CLOSEST(gain, val2); + + gpiod_set_value(st->gain_gpio, gain < AD7780_GAIN_MIDPOINT ? 0 : 1); + break; + case IIO_CHAN_INFO_SAMP_FREQ: + if (val2 != 0) + return -EINVAL; + + gpiod_set_value(st->filter_gpio, val < AD7780_FILTER_MIDPOINT ? 0 : 1); + break; + } + + 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) { + val = raw_sample & AD7780_GAIN; + + if (val != gpiod_get_value(st->gain_gpio)) + return -EIO; + + if (val) + st->gain = 1; + else + st->gain = 128; + } + + return 0; +} + +static const struct ad_sigma_delta_info ad7780_sigma_delta_info = { + .set_mode = ad7780_set_mode, + .postprocess_sample = ad7780_postprocess_sample, + .has_registers = false, +}; + +#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 = AD7170_CHANNEL(12, 24), + .pattern = AD7170_PATTERN, + .pattern_mask = AD7170_PATTERN_MASK, + .is_ad778x = false, + }, + [ID_AD7171] = { + .channel = AD7170_CHANNEL(16, 24), + .pattern = AD7170_PATTERN, + .pattern_mask = AD7170_PATTERN_MASK, + .is_ad778x = false, + }, + [ID_AD7780] = { + .channel = AD7780_CHANNEL(24, 32), + .pattern = AD7780_PATTERN, + .pattern_mask = AD7780_PATTERN_MASK, + .is_ad778x = true, + }, + [ID_AD7781] = { + .channel = AD7780_CHANNEL(20, 32), + .pattern = AD7780_PATTERN, + .pattern_mask = AD7780_PATTERN_MASK, + .is_ad778x = true, + }, +}; + +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) +{ + struct ad7780_state *st; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); + if (!indio_dev) + return -ENOMEM; + + st = iio_priv(indio_dev); + st->gain = 1; + + ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info); + + st->reg = devm_regulator_get(&spi->dev, "avdd"); + if (IS_ERR(st->reg)) + return PTR_ERR(st->reg); + + ret = regulator_enable(st->reg); + if (ret) { + dev_err(&spi->dev, "Failed to enable specified AVdd supply\n"); + return ret; + } + + st->chip_info = + &ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data]; + + spi_set_drvdata(spi, indio_dev); + + indio_dev->dev.parent = &spi->dev; + indio_dev->name = spi_get_device_id(spi)->name; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = &st->chip_info->channel; + indio_dev->num_channels = 1; + indio_dev->info = &ad7780_info; + + st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev, + "powerdown", + GPIOD_OUT_LOW); + if (IS_ERR(st->powerdown_gpio)) { + ret = PTR_ERR(st->powerdown_gpio); + dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n", + ret); + 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; + + ret = iio_device_register(indio_dev); + if (ret) + goto error_cleanup_buffer_and_trigger; + + return 0; + +error_cleanup_buffer_and_trigger: + ad_sd_cleanup_buffer_and_trigger(indio_dev); +error_disable_reg: + regulator_disable(st->reg); + + return ret; +} + +static int ad7780_remove(struct spi_device *spi) +{ + struct iio_dev *indio_dev = spi_get_drvdata(spi); + struct ad7780_state *st = iio_priv(indio_dev); + + iio_device_unregister(indio_dev); + ad_sd_cleanup_buffer_and_trigger(indio_dev); + + regulator_disable(st->reg); + + return 0; +} + +static const struct spi_device_id ad7780_id[] = { + {"ad7170", ID_AD7170}, + {"ad7171", ID_AD7171}, + {"ad7780", ID_AD7780}, + {"ad7781", ID_AD7781}, + {} +}; +MODULE_DEVICE_TABLE(spi, ad7780_id); + +static struct spi_driver ad7780_driver = { + .driver = { + .name = "ad7780", + }, + .probe = ad7780_probe, + .remove = ad7780_remove, + .id_table = ad7780_id, +}; +module_spi_driver(ad7780_driver); + +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); +MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig index fc23059f1673..4cd0a13a588c 100644 --- a/drivers/staging/iio/adc/Kconfig +++ b/drivers/staging/iio/adc/Kconfig @@ -37,19 +37,6 @@ config AD7606_IFACE_SPI To compile this driver as a module, choose M here: the module will be called ad7606_spi. -config AD7780 - tristate "Analog Devices AD7780 and similar ADCs driver" - depends on SPI - depends on GPIOLIB || COMPILE_TEST - select AD_SIGMA_DELTA - help - Say yes here to build support for Analog Devices AD7170, AD7171, - AD7780 and AD7781 SPI analog to digital converters (ADC). - If unsure, say N (but it's safe to say "Y"). - - To compile this driver as a module, choose M here: the - module will be called ad7780. - config AD7816 tristate "Analog Devices AD7816/7/8 temperature sensor and ADC driver" depends on SPI diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile index ebe83c1ad362..344002d87c78 100644 --- a/drivers/staging/iio/adc/Makefile +++ b/drivers/staging/iio/adc/Makefile @@ -7,7 +7,6 @@ obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o obj-$(CONFIG_AD7606) += ad7606.o -obj-$(CONFIG_AD7780) += ad7780.o obj-$(CONFIG_AD7816) += ad7816.o obj-$(CONFIG_AD7192) += ad7192.o obj-$(CONFIG_AD7280) += ad7280a.o diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c deleted file mode 100644 index 05979a79fda3..000000000000 --- a/drivers/staging/iio/adc/ad7780.c +++ /dev/null @@ -1,347 +0,0 @@ -/* - * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver - * - * Copyright 2011 Analog Devices Inc. - * - * Licensed under the GPL-2. - */ - -#include <linux/interrupt.h> -#include <linux/device.h> -#include <linux/kernel.h> -#include <linux/slab.h> -#include <linux/sysfs.h> -#include <linux/spi/spi.h> -#include <linux/regulator/consumer.h> -#include <linux/err.h> -#include <linux/sched.h> -#include <linux/gpio/consumer.h> -#include <linux/module.h> - -#include <linux/iio/iio.h> -#include <linux/iio/sysfs.h> -#include <linux/iio/adc/ad_sigma_delta.h> - -#define AD7780_RDY BIT(7) -#define AD7780_FILTER BIT(6) -#define AD7780_ERR BIT(5) -#define AD7780_ID1 BIT(4) -#define AD7780_ID0 BIT(3) -#define AD7780_GAIN BIT(2) -#define AD7780_PAT1 BIT(1) -#define AD7780_PAT0 BIT(0) - -#define AD7780_PATTERN (AD7780_PAT0) -#define AD7780_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1) - -#define AD7170_PAT2 BIT(2) - -#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 - -#define AD7780_GAIN_MIDPOINT 64 -#define AD7780_FILTER_MIDPOINT 13350 - -struct ad7780_chip_info { - struct iio_chan_spec channel; - unsigned int pattern_mask; - unsigned int pattern; - bool is_ad778x; -}; - -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; -}; - -enum ad7780_supported_device_ids { - ID_AD7170, - ID_AD7171, - ID_AD7780, - ID_AD7781, -}; - -static struct ad7780_state *ad_sigma_delta_to_ad7780(struct ad_sigma_delta *sd) -{ - return container_of(sd, struct ad7780_state, sd); -} - -static int ad7780_set_mode(struct ad_sigma_delta *sigma_delta, - enum ad_sigma_delta_mode mode) -{ - struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta); - unsigned int val; - - switch (mode) { - case AD_SD_MODE_SINGLE: - case AD_SD_MODE_CONTINUOUS: - val = 1; - break; - default: - val = 0; - break; - } - - gpiod_set_value(st->powerdown_gpio, val); - - return 0; -} - -static int ad7780_read_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); - int voltage_uv; - - switch (m) { - case IIO_CHAN_INFO_RAW: - return ad_sigma_delta_single_conversion(indio_dev, chan, val); - case IIO_CHAN_INFO_SCALE: - voltage_uv = regulator_get_voltage(st->reg); - if (voltage_uv < 0) - return voltage_uv; - *val = (voltage_uv / 1000) * st->gain; - *val2 = chan->scan_type.realbits - 1; - return IIO_VAL_FRACTIONAL_LOG2; - case IIO_CHAN_INFO_OFFSET: - *val = -(1 << (chan->scan_type.realbits - 1)); - return IIO_VAL_INT; - } - - 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); - const struct ad7780_chip_info *chip_info = st->chip_info; - int uvref, gain; - unsigned int full_scale; - - if (!chip_info->is_ad778x) - return 0; - - switch (m) { - case IIO_CHAN_INFO_SCALE: - if (val != 0) - return -EINVAL; - - uvref = regulator_get_voltage(st->reg); - - if (uvref < 0) - return uvref; - - full_scale = 1 << (chip_info->channel.scan_type.realbits - 1); - gain = DIV_ROUND_CLOSEST(uvref, full_scale); - gain = DIV_ROUND_CLOSEST(gain, val2); - - gpiod_set_value(st->gain_gpio, gain < AD7780_GAIN_MIDPOINT ? 0 : 1); - break; - case IIO_CHAN_INFO_SAMP_FREQ: - if (val2 != 0) - return -EINVAL; - - gpiod_set_value(st->filter_gpio, val < AD7780_FILTER_MIDPOINT ? 0 : 1); - break; - } - - 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) { - val = raw_sample & AD7780_GAIN; - - if (val != gpiod_get_value(st->gain_gpio)) - return -EIO; - - if (val) - st->gain = 1; - else - st->gain = 128; - } - - return 0; -} - -static const struct ad_sigma_delta_info ad7780_sigma_delta_info = { - .set_mode = ad7780_set_mode, - .postprocess_sample = ad7780_postprocess_sample, - .has_registers = false, -}; - -#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 = AD7170_CHANNEL(12, 24), - .pattern = AD7170_PATTERN, - .pattern_mask = AD7170_PATTERN_MASK, - .is_ad778x = false, - }, - [ID_AD7171] = { - .channel = AD7170_CHANNEL(16, 24), - .pattern = AD7170_PATTERN, - .pattern_mask = AD7170_PATTERN_MASK, - .is_ad778x = false, - }, - [ID_AD7780] = { - .channel = AD7780_CHANNEL(24, 32), - .pattern = AD7780_PATTERN, - .pattern_mask = AD7780_PATTERN_MASK, - .is_ad778x = true, - }, - [ID_AD7781] = { - .channel = AD7780_CHANNEL(20, 32), - .pattern = AD7780_PATTERN, - .pattern_mask = AD7780_PATTERN_MASK, - .is_ad778x = true, - }, -}; - -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) -{ - struct ad7780_state *st; - struct iio_dev *indio_dev; - int ret; - - indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); - if (!indio_dev) - return -ENOMEM; - - st = iio_priv(indio_dev); - st->gain = 1; - - ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info); - - st->reg = devm_regulator_get(&spi->dev, "avdd"); - if (IS_ERR(st->reg)) - return PTR_ERR(st->reg); - - ret = regulator_enable(st->reg); - if (ret) { - dev_err(&spi->dev, "Failed to enable specified AVdd supply\n"); - return ret; - } - - st->chip_info = - &ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data]; - - spi_set_drvdata(spi, indio_dev); - - indio_dev->dev.parent = &spi->dev; - indio_dev->name = spi_get_device_id(spi)->name; - indio_dev->modes = INDIO_DIRECT_MODE; - indio_dev->channels = &st->chip_info->channel; - indio_dev->num_channels = 1; - indio_dev->info = &ad7780_info; - - st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev, - "powerdown", - GPIOD_OUT_LOW); - if (IS_ERR(st->powerdown_gpio)) { - ret = PTR_ERR(st->powerdown_gpio); - dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n", - ret); - 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; - - ret = iio_device_register(indio_dev); - if (ret) - goto error_cleanup_buffer_and_trigger; - - return 0; - -error_cleanup_buffer_and_trigger: - ad_sd_cleanup_buffer_and_trigger(indio_dev); -error_disable_reg: - regulator_disable(st->reg); - - return ret; -} - -static int ad7780_remove(struct spi_device *spi) -{ - struct iio_dev *indio_dev = spi_get_drvdata(spi); - struct ad7780_state *st = iio_priv(indio_dev); - - iio_device_unregister(indio_dev); - ad_sd_cleanup_buffer_and_trigger(indio_dev); - - regulator_disable(st->reg); - - return 0; -} - -static const struct spi_device_id ad7780_id[] = { - {"ad7170", ID_AD7170}, - {"ad7171", ID_AD7171}, - {"ad7780", ID_AD7780}, - {"ad7781", ID_AD7781}, - {} -}; -MODULE_DEVICE_TABLE(spi, ad7780_id); - -static struct spi_driver ad7780_driver = { - .driver = { - .name = "ad7780", - }, - .probe = ad7780_probe, - .remove = ad7780_remove, - .id_table = ad7780_id, -}; -module_spi_driver(ad7780_driver); - -MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); -MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs"); -MODULE_LICENSE("GPL v2"); -- 2.19.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] staging: iio: ad7780: Moving ad7780 out of staging 2018-11-28 18:16 ` [PATCH 2/2] staging: iio: ad7780: Moving ad7780 out of staging Giuliano Belinassi @ 2018-12-01 17:20 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2018-12-01 17:20 UTC (permalink / raw) To: Giuliano Belinassi Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, stefan.popa, alexandru.Ardelean, renatogeh, linux-iio, devel, linux-kernel, kernel-usp On Wed, 28 Nov 2018 16:16:34 -0200 Giuliano Belinassi <giuliano.belinassi@gmail.com> wrote: > Move ad7780 sigma-delta adc out of staging to the main tree Please add a few details here on what the device is and what interfaces are provided. It's nice for anyone whose first encounter with this driver is this patch (as they don't look at activity in staging). > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br> > Signed-off-by: Renato Lui Geh <renatogeh@gmail.com> Look pretty good. A few small things inline once patch 1 is sorted. I'd also see if you can get agreement for SPDX from one of the AD reviewers (they are normally fine with this I think and can go ask Michael if they want to be sure and Michael isn't following the thread) Note some of the comments are actually on patch 1 but visible here when placed in the surrounding code. I'll try to mention any in that thread as well in a minute... Jonathan > --- > drivers/iio/adc/Kconfig | 13 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ad7780.c | 347 +++++++++++++++++++++++++++++++ > drivers/staging/iio/adc/Kconfig | 13 -- > drivers/staging/iio/adc/Makefile | 1 - > drivers/staging/iio/adc/ad7780.c | 347 ------------------------------- > 6 files changed, 361 insertions(+), 361 deletions(-) > create mode 100644 drivers/iio/adc/ad7780.c > delete mode 100644 drivers/staging/iio/adc/ad7780.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 575bf69fea57..e517425e364d 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -92,6 +92,19 @@ config AD7793 > To compile this driver as a module, choose M here: the > module will be called AD7793. > > +config AD7780 > + tristate "Analog Devices AD7780 and similar ADCs driver" > + depends on SPI > + depends on GPIOLIB || COMPILE_TEST > + select AD_SIGMA_DELTA > + help > + Say yes here to build support for Analog Devices AD7170, AD7171, > + AD7780 and AD7781 SPI analog to digital converters (ADC). > + If unsure, say N (but it's safe to say "Y"). > + > + To compile this driver as a module, choose M here: the > + module will be called ad7780. > + > config AD7887 > tristate "Analog Devices AD7887 ADC driver" > depends on SPI > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 91ba28aeb150..3301ca10b385 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -11,6 +11,7 @@ obj-$(CONFIG_AD7298) += ad7298.o > obj-$(CONFIG_AD7923) += ad7923.o > obj-$(CONFIG_AD7476) += ad7476.o > obj-$(CONFIG_AD7766) += ad7766.o > +obj-$(CONFIG_AD7780) += ad7780.o > obj-$(CONFIG_AD7791) += ad7791.o > obj-$(CONFIG_AD7793) += ad7793.o > obj-$(CONFIG_AD7887) += ad7887.o > diff --git a/drivers/iio/adc/ad7780.c b/drivers/iio/adc/ad7780.c > new file mode 100644 > index 000000000000..05979a79fda3 > --- /dev/null > +++ b/drivers/iio/adc/ad7780.c > @@ -0,0 +1,347 @@ > +/* > + * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver > + * > + * Copyright 2011 Analog Devices Inc. > + * > + * Licensed under the GPL-2. Definitely time to look at moving to SPDX as various AD reviewers looking at this anyway so should be fine to get their ack! > + */ > + > +#include <linux/interrupt.h> > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/sysfs.h> > +#include <linux/spi/spi.h> > +#include <linux/regulator/consumer.h> > +#include <linux/err.h> > +#include <linux/sched.h> > +#include <linux/gpio/consumer.h> > +#include <linux/module.h> > + > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/adc/ad_sigma_delta.h> > + > +#define AD7780_RDY BIT(7) > +#define AD7780_FILTER BIT(6) > +#define AD7780_ERR BIT(5) > +#define AD7780_ID1 BIT(4) > +#define AD7780_ID0 BIT(3) > +#define AD7780_GAIN BIT(2) > +#define AD7780_PAT1 BIT(1) > +#define AD7780_PAT0 BIT(0) > + > +#define AD7780_PATTERN (AD7780_PAT0) > +#define AD7780_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1) > + > +#define AD7170_PAT2 BIT(2) > + > +#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 > + > +#define AD7780_GAIN_MIDPOINT 64 > +#define AD7780_FILTER_MIDPOINT 13350 > + > +struct ad7780_chip_info { > + struct iio_chan_spec channel; > + unsigned int pattern_mask; > + unsigned int pattern; > + bool is_ad778x; > +}; > + > +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; > +}; > + > +enum ad7780_supported_device_ids { > + ID_AD7170, > + ID_AD7171, > + ID_AD7780, > + ID_AD7781, > +}; > + > +static struct ad7780_state *ad_sigma_delta_to_ad7780(struct ad_sigma_delta *sd) > +{ > + return container_of(sd, struct ad7780_state, sd); > +} > + > +static int ad7780_set_mode(struct ad_sigma_delta *sigma_delta, > + enum ad_sigma_delta_mode mode) > +{ > + struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta); > + unsigned int val; > + > + switch (mode) { > + case AD_SD_MODE_SINGLE: > + case AD_SD_MODE_CONTINUOUS: > + val = 1; > + break; > + default: > + val = 0; > + break; > + } > + > + gpiod_set_value(st->powerdown_gpio, val); > + > + return 0; > +} > + > +static int ad7780_read_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); > + int voltage_uv; > + > + switch (m) { > + case IIO_CHAN_INFO_RAW: > + return ad_sigma_delta_single_conversion(indio_dev, chan, val); > + case IIO_CHAN_INFO_SCALE: > + voltage_uv = regulator_get_voltage(st->reg); > + if (voltage_uv < 0) > + return voltage_uv; > + *val = (voltage_uv / 1000) * st->gain; > + *val2 = chan->scan_type.realbits - 1; > + return IIO_VAL_FRACTIONAL_LOG2; > + case IIO_CHAN_INFO_OFFSET: > + *val = -(1 << (chan->scan_type.realbits - 1)); > + return IIO_VAL_INT; > + } We should be able to read back the current sampling frequency as well (from a cached value). > + > + 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); > + const struct ad7780_chip_info *chip_info = st->chip_info; > + int uvref, gain; > + unsigned int full_scale; > + > + if (!chip_info->is_ad778x) > + return 0; > + > + switch (m) { > + case IIO_CHAN_INFO_SCALE: > + if (val != 0) > + return -EINVAL; > + > + uvref = regulator_get_voltage(st->reg); > + > + if (uvref < 0) > + return uvref; > + > + full_scale = 1 << (chip_info->channel.scan_type.realbits - 1); > + gain = DIV_ROUND_CLOSEST(uvref, full_scale); > + gain = DIV_ROUND_CLOSEST(gain, val2); > + > + gpiod_set_value(st->gain_gpio, gain < AD7780_GAIN_MIDPOINT ? 0 : 1); > + break; > + case IIO_CHAN_INFO_SAMP_FREQ: > + if (val2 != 0) It's a bit odd to do a combination of a precise match (nothing after decimal point) but then use just a threshold for the integer part... If you are going to do block this then insist on a precise match for the filter value below and provide an available attribute to give the two possible values. > + return -EINVAL; > + > + gpiod_set_value(st->filter_gpio, val < AD7780_FILTER_MIDPOINT ? 0 : 1); > + break; > + } > + > + 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) { > + val = raw_sample & AD7780_GAIN; > + > + if (val != gpiod_get_value(st->gain_gpio)) > + return -EIO; > + > + if (val) > + st->gain = 1; > + else > + st->gain = 128; There are outstanding comments on this code in patch 1, I won't comment on it here. > + } > + > + return 0; > +} > + > +static const struct ad_sigma_delta_info ad7780_sigma_delta_info = { > + .set_mode = ad7780_set_mode, > + .postprocess_sample = ad7780_postprocess_sample, > + .has_registers = false, > +}; > + > +#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 = AD7170_CHANNEL(12, 24), > + .pattern = AD7170_PATTERN, > + .pattern_mask = AD7170_PATTERN_MASK, > + .is_ad778x = false, > + }, > + [ID_AD7171] = { > + .channel = AD7170_CHANNEL(16, 24), > + .pattern = AD7170_PATTERN, > + .pattern_mask = AD7170_PATTERN_MASK, > + .is_ad778x = false, > + }, > + [ID_AD7780] = { > + .channel = AD7780_CHANNEL(24, 32), > + .pattern = AD7780_PATTERN, > + .pattern_mask = AD7780_PATTERN_MASK, > + .is_ad778x = true, > + }, > + [ID_AD7781] = { > + .channel = AD7780_CHANNEL(20, 32), > + .pattern = AD7780_PATTERN, > + .pattern_mask = AD7780_PATTERN_MASK, > + .is_ad778x = true, > + }, > +}; > + > +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) > +{ > + struct ad7780_state *st; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); > + if (!indio_dev) > + return -ENOMEM; > + > + st = iio_priv(indio_dev); > + st->gain = 1; > + > + ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info); > + > + st->reg = devm_regulator_get(&spi->dev, "avdd"); > + if (IS_ERR(st->reg)) > + return PTR_ERR(st->reg); > + > + ret = regulator_enable(st->reg); Hmm. This is obviously not an actual bug, but from my 'obviously correct' mental filter is tripping over the fact the remove ordering is not the precise opposite of the probe order. As devm managed cleanup occurs after remove finishes, the two gpios below are freed after the regulator disable, whereas for precise reverse order they would be before it. The easiest 'fix' for this would be to just move this regulator enable until after we have grabbed the gpios. I don't think that makes any real difference as we use neither of them in this function anyway. Another option would be to use devm_add_action_or_reset to do the unwinding of this regulator_enable so that it gets cleaned up in the devm unwind rather than by hand. > + if (ret) { > + dev_err(&spi->dev, "Failed to enable specified AVdd supply\n"); > + return ret; > + } > + > + st->chip_info = > + &ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data]; > + > + spi_set_drvdata(spi, indio_dev); > + > + indio_dev->dev.parent = &spi->dev; > + indio_dev->name = spi_get_device_id(spi)->name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = &st->chip_info->channel; > + indio_dev->num_channels = 1; > + indio_dev->info = &ad7780_info; > + > + st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev, > + "powerdown", > + GPIOD_OUT_LOW); > + if (IS_ERR(st->powerdown_gpio)) { > + ret = PTR_ERR(st->powerdown_gpio); > + dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n", > + ret); > + 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; > + > + ret = iio_device_register(indio_dev); > + if (ret) > + goto error_cleanup_buffer_and_trigger; > + > + return 0; > + > +error_cleanup_buffer_and_trigger: > + ad_sd_cleanup_buffer_and_trigger(indio_dev); > +error_disable_reg: > + regulator_disable(st->reg); > + > + return ret; > +} > + > +static int ad7780_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct ad7780_state *st = iio_priv(indio_dev); > + > + iio_device_unregister(indio_dev); > + ad_sd_cleanup_buffer_and_trigger(indio_dev); > + > + regulator_disable(st->reg); > + > + return 0; > +} > + > +static const struct spi_device_id ad7780_id[] = { > + {"ad7170", ID_AD7170}, > + {"ad7171", ID_AD7171}, > + {"ad7780", ID_AD7780}, > + {"ad7781", ID_AD7781}, > + {} > +}; > +MODULE_DEVICE_TABLE(spi, ad7780_id); > + > +static struct spi_driver ad7780_driver = { > + .driver = { > + .name = "ad7780", > + }, > + .probe = ad7780_probe, > + .remove = ad7780_remove, > + .id_table = ad7780_id, > +}; > +module_spi_driver(ad7780_driver); > + > +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); > +MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs"); > +MODULE_LICENSE("GPL v2"); <cut removed code> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-01-19 16:11 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-28 18:15 [PATCH 0/2] staging: iio: ad7780: move out of staging Giuliano Belinassi 2018-11-28 18:15 ` [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support Giuliano Belinassi 2018-11-29 11:18 ` Popa, Stefan Serban 2018-11-29 11:18 ` Popa, Stefan Serban 2018-11-29 13:02 ` Giuliano Augusto Faulin Belinassi 2018-12-01 17:23 ` Jonathan Cameron 2019-01-18 20:19 ` Renato Lui Geh 2019-01-19 16:11 ` Jonathan Cameron 2018-11-28 18:16 ` [PATCH 2/2] staging: iio: ad7780: Moving ad7780 out of staging Giuliano Belinassi 2018-12-01 17:20 ` Jonathan Cameron
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.