staging: iio: ad7780: move out of staging
Subsystems (5)
cover staging: iio: ad7780: move out of staging
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
Signature · 1 line
2.19.1
PATCH 1/2 staging: iio: ad7780: Add gain & filter gpio support
Previously, the AD7780 driver only supported gpio for the 'powerdown' pin. This commit adds suppport for the 'gain' and 'filter' pin.
---
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(-)
drivers/staging/iio/adc/ad7780.c +74 -4
@@ -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;
include/linux/iio/adc/ad_sigma_delta.h +5 -0
@@ -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, \
Signature · 1 line
2.19.1
PATCH 2/2 staging: iio: ad7780: Moving ad7780 out of staging
In reply to Giuliano BelinassiMove ad7780 sigma-delta adc out of staging to the main tree
--- 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
drivers/iio/adc/Kconfig +13 -0
@@ -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
drivers/iio/adc/Makefile +1 -0
@@ -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
drivers/iio/adc/ad7780.c new file +347 -0
347 lines not on this page · raw message
drivers/staging/iio/adc/Kconfig +0 -13
@@ -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
drivers/staging/iio/adc/Makefile +0 -1
@@ -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
drivers/staging/iio/adc/ad7780.c deleted +0 -347
347 lines not on this page · raw message
Signature · 1 line
2.19.1
Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support
In reply to Giuliano BelinassiQuoted 10 lines · Giuliano Belinassi
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 <[email protected]>---
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.
Quoted 48 lines
drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++--
include/linux/iio/adc/ad_sigma_delta.h| 5 ++2 files changed, 79 insertions(+), 4 deletions(-)drivers/staging/iio/adc/ad7780.c +0
-0@@ -39,6 +39,12 @@ #define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2) #define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)drivers/staging/iio/adc/ad7780.c +6
-0+#define AD7780_GAIN_GPIO 0 +#define AD7780_FILTER_GPIO 1 + +#define AD7780_GAIN_MIDPOINT 64 +#define AD7780_FILTER_MIDPOINT 13350 +struct
ad7780_chip_info{
structiio_chan_specchannel;
unsigned intpattern_mask;drivers/staging/iio/adc/ad7780.c +2
-0@@ -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;
structad_sigma_deltasd;drivers/staging/iio/adc/ad7780.c +0
-0@@ -115,18 +123,65 @@ static int ad7780_read_raw(struct iio_dev *indio_dev, return -EINVAL; }drivers/staging/iio/adc/ad7780.c +10
-0+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;
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 145
Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +10
-0+ +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.
Quoted 11 lines
drivers/staging/iio/adc/ad7780.c +9
-1+ +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.
Quoted 6 lines
drivers/staging/iio/adc/ad7780.c +5
-0+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.
Quoted 19 lines
drivers/staging/iio/adc/ad7780.c +6
-0+break ; + } + +return 0 ; +} +static int
ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta, unsigned int raw_sample){
structad7780_state*st =ad_sigma_delta_to_ad7780(sigma_delta); const structad7780_chip_info*chip_info=st->chip_info;drivers/staging/iio/adc/ad7780.c +1
-0+int val;
if ((raw_sample&AD7780_ERR) ||
((raw_sample &chip_info->pattern_mask) != chip_info-Quoted 1 line
pattern))
return
-EIO;
if (chip_info->is_ad778x) {
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 182
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +4
-1-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?
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +2
-0+ +if (val)
st->gain= 1;
else
st->gain= 128;
Do we still need this? I am not convinced.
Quoted 38 lines
drivers/staging/iio/adc/ad7780.c +0
-0@@ -141,18 +196,20 @@ static const struct ad_sigma_delta_info ad7780_sigma_delta_info = { .has_registers = false, };drivers/staging/iio/adc/ad7780.c +1
-1-#define AD7780_CHANNEL (bits, wordsize) \ +#define AD7170_CHANNEL (bits, wordsize) \
AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)drivers/staging/iio/adc/ad7780.c +2
-0+#define AD7780_CHANNEL (bits, wordsize) \ + AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
static const structad7780_chip_infoad7780_chip_info_tbl[] = { [ID_AD7170] = {drivers/staging/iio/adc/ad7780.c +1
-1- .channel = AD7780_CHANNEL(12, 24), + .channel = AD7170_CHANNEL(12, 24),.pattern =
AD7170_PATTERN, .pattern_mask =AD7170_PATTERN_MASK, .is_ad778x = false,
},
[ID_AD7171] = {drivers/staging/iio/adc/ad7780.c +1
-1- .channel = AD7780_CHANNEL(16, 24), + .channel = AD7170_CHANNEL(16, 24),.pattern =
AD7170_PATTERN, .pattern_mask =AD7170_PATTERN_MASK, .is_ad778x = false,drivers/staging/iio/adc/ad7780.c +0
-0@@ -173,6 +230,7 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
static const structiio_infoad7780_info= {
.read_raw =ad7780_read_raw,drivers/staging/iio/adc/ad7780.c +1
-0+ .write_raw = ad7780_write_raw,};
static intad7780_probe(struct spi_device *spi)drivers/staging/iio/adc/ad7780.c +0
-0@@ -222,6 +280,18 @@ static int ad7780_probe(struct spi_device *spi) goto error_disable_reg; }
drivers/staging/iio/adc/ad7780.c static int ad7780_probe(struct spi_device *spi) · line 287
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +5
-0+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?
Quoted 30 lines
drivers/staging/iio/adc/ad7780.c +2
-0+ ret =PTR_ERR (st->gain_gpio); + dev_err(&spi->dev, "Failed to request gain GPIO:%d\n",
drivers/staging/iio/adc/ad7780.c +5
-0+ ret); + goto error_disable_reg; + } + } +ret =
ad_sd_setup_buffer_and_trigger(indio_dev); if (ret)
gotoerror_disable_reg;include/linux/iio/adc/ad_sigma_delta.h +0
-0@@ -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)include/linux/iio/adc/ad_sigma_delta.h +4
-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))
+
#defineAD_SD_TEMP_CHANNEL(_si, _address, _bits, _storagebits, _shift)\
__AD_SD_CHANNEL(_si, 0, -1,_address,_bits, \
_storagebits,_shift, NULL,IIO_TEMP, \
Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support
Hi
Quoted 69 lines · Stefan Serban
Quoted 10 lines · Giuliano Belinassi
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 <[email protected]>---
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.Quoted 53 lines
drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++-- include/linux/iio/adc/ad_sigma_delta.h | 5 ++ 2 files changed, 79 insertions(+), 4 deletions(-)drivers/staging/iio/adc/ad7780.c +6
-0@@ -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;drivers/staging/iio/adc/ad7780.c +17
-0@@ -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) {
Quoted 8 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 145
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +5
-0+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 inst->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.
Quoted 5 lines
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_mvwith 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.
Quoted 5 lines
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +5
-0+ +if (uvref <0 ) +return uvref; + + full_scale = 1 << (chip_info->channel.scan_type.realbits
Quoted 8 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev,
Quoted 6 lines
drivers/staging/iio/adc/ad7780.c +4
-1- 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->gainvariable.
Yes, we forgot it.
Quoted 37 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev,
Quoted 7 lines
drivers/staging/iio/adc/ad7780.c +5
-0+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.
Quoted 14 lines
drivers/staging/iio/adc/ad7780.c +7
-0+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) ||drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 182
Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +4
-1((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 ;
Quoted 2 lines
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.
Quoted 8 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 187
Quoted 6 lines
drivers/staging/iio/adc/ad7780.c +2
-0+ +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 :-)
Quoted 86 lines
Quoted 36 lines
drivers/staging/iio/adc/ad7780.c +5
-3@@ -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,drivers/staging/iio/adc/ad7780.c +1
-0@@ -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)drivers/staging/iio/adc/ad7780.c static int ad7780_probe(struct spi_device *spi) · line 287
Quoted 8 lines
drivers/staging/iio/adc/ad7780.c +5
-0@@ -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 forfilter_gpio?Quoted 28 lines
drivers/staging/iio/adc/ad7780.c +7
-0+ 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;include/linux/iio/adc/ad_sigma_delta.h +5
-0@@ -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) \Quoted 2 lines
__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 [email protected]. To post to this group, send email to [email protected].
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.
Jonathan Cameron
maintainerRe: [PATCH 2/2] staging: iio: ad7780: Moving ad7780 out of staging
In reply to Giuliano BelinassiQuoted 1 line · On Wed, 28 Nov 2018 16:16:34 -0200 Giuliano Belinassi…
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).
Quoted 3 lines
Signed-off-by Giuliano Belinassi <[email protected]>Signed-off-by Renato Lui Geh <[email protected]>
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
Quoted 53 lines
--- 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.cdrivers/iio/adc/Kconfig +13
-0@@ -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 SPIdrivers/iio/adc/Makefile +1
-0@@ -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.odrivers/iio/adc/ad7780.c new file +0
-0
drivers/iio/adc/ad7780.c line 6
Quoted 6 lines
drivers/iio/adc/ad7780.c new file +6
-0@@ -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!
Quoted 105 lines
drivers/iio/adc/ad7780.c new file +105
-0+*/ + +#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 :
drivers/iio/adc/ad7780.c line 121
Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10
-0+ 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).
Quoted 26 lines
drivers/iio/adc/ad7780.c new file +26
-0+ +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 )
drivers/iio/adc/ad7780.c line 157
Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10
-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.
Quoted 20 lines
drivers/iio/adc/ad7780.c new file +20
-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 ; +
drivers/iio/adc/ad7780.c line 187
Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10
-0+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.
Quoted 58 lines
drivers/iio/adc/ad7780.c new file +58
-0+ } + +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 ; +
drivers/iio/adc/ad7780.c line 255
Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10
-0+ 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.
Quoted 82 lines
drivers/iio/adc/ad7780.c new file +82
-0+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" ,
drivers/iio/adc/ad7780.c line 347
Quoted 10 lines
drivers/iio/adc/ad7780.c new file +10
-0+ }, + .probe = ad7780_probe, + .remove = ad7780_remove, + .id_table = ad7780_id, +}; +module_spi_driver (ad7780_driver); + +MODULE_AUTHOR ("Michael Hennerich <[email protected]>" ); +MODULE_DESCRIPTION ("Analog Devices AD7780 and similar ADCs" ); +MODULE_LICENSE ("GPL v2" );
<cut removed code>
Jonathan Cameron
maintainerRe: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support
In reply to Giuliano Augusto Faulin BelinassiQuoted 1 line · On Thu, 29 Nov 2018 11:02:46 -0200 Giuliano Augusto Faulin Belinassi…
Hi
A few follow ups from me having read the result in patch 2.
Jonathan
Quoted 110 lines
Quoted 69 lines · Stefan Serban
Quoted 10 lines · Giuliano Belinassi
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 <[email protected]>---
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.Quoted 53 lines
drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++-- include/linux/iio/adc/ad_sigma_delta.h | 5 ++ 2 files changed, 79 insertions(+), 4 deletions(-)drivers/staging/iio/adc/ad7780.c +6
-0@@ -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;drivers/staging/iio/adc/ad7780.c +17
-0@@ -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) {Quoted 8 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 145
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +5
-0+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 inst->int_vref_mv.This was removed in commit
9eae69ddbc4717a0bd702eddac76c7848773bf71because 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.Quoted 5 lines
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_mvwith 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.Quoted 5 lines
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +5
-0+ +if (uvref <0 ) +return uvref; + + full_scale = 1 << (chip_info->channel.scan_type.realbitsQuoted 8 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev,
Quoted 6 lines
drivers/staging/iio/adc/ad7780.c +4
-1- 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->gainvariable.
Quoted 6 lines
Yes, we forgot it.
Quoted 4 lines
Quoted 4 lines
drivers/staging/iio/adc/ad7780.c +3
-0+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.
Quoted 42 lines
Quoted 33 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev · line 162
Quoted 3 lines
drivers/staging/iio/adc/ad7780.c +2
-0+ +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.
Quoted 14 lines
drivers/staging/iio/adc/ad7780.c +7
-0+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) ||drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 182
Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +4
-1((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 ;Quoted 2 lines
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.Quoted 1 line
Quoted 57 lines
Quoted 7 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 187
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +2
-0+ +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 :-)
Quoted 48 lines
Quoted 36 lines
drivers/staging/iio/adc/ad7780.c +5
-3@@ -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,drivers/staging/iio/adc/ad7780.c +1
-0@@ -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)drivers/staging/iio/adc/ad7780.c static int ad7780_probe(struct spi_device *spi) · line 287
Quoted 8 lines
drivers/staging/iio/adc/ad7780.c +5
-0@@ -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 forfilter_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...
Quoted 38 lines
Quoted 38 lines
Quoted 29 lines
drivers/staging/iio/adc/ad7780.c +7
-0+ 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;include/linux/iio/adc/ad_sigma_delta.h +5
-0@@ -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) \Quoted 2 lines
__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 [email protected]. To post to this group, send email to [email protected].
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.
Renato Lui Geh
maintainerRe: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support
Hi,
Sorry for the (extremely) late reply.
Comments inline.
Renato
Quoted 93 lines · Jonathan Cameron
Quoted 1 line · On Thu, 29 Nov 2018 11:02:46 -0200 Giuliano Augusto Faulin Belinassi…
Hi
A few follow ups from me having read the result in patch 2.
Jonathan
Quoted 85 lines
Quoted 69 lines · Stefan Serban
Quoted 10 lines · Giuliano Belinassi
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 <[email protected]>---
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.Quoted 53 lines
drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++-- include/linux/iio/adc/ad_sigma_delta.h | 5 ++ 2 files changed, 79 insertions(+), 4 deletions(-)drivers/staging/iio/adc/ad7780.c +6
-0@@ -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;drivers/staging/iio/adc/ad7780.c +17
-0@@ -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) {Quoted 8 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 145
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +5
-0+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 inst->int_vref_mv.This was removed in commit
9eae69ddbc4717a0bd702eddac76c7848773bf71because 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?
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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_mvwith 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?
Quoted 22 lines
Quoted 22 lines
Thanks for this info! :-)
Shouldn't we store this in uV (microVolts)? This will yield a more accurate result after the multiplication.Quoted 5 lines
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +5
-0+ +if (uvref <0 ) +return uvref; + + full_scale = 1 << (chip_info->channel.scan_type.realbitsQuoted 8 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev,
Quoted 6 lines
drivers/staging/iio/adc/ad7780.c +4
-1- 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->gainvariable.Yes, we forgot it.
Quoted 1 line
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 158
Quoted 3 lines
drivers/staging/iio/adc/ad7780.c +3
-0+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?
Quoted 38 lines
I'd also expect to see
read_rawsupport for this.Quoted 35 lines
Quoted 35 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev · line 162
Quoted 3 lines
drivers/staging/iio/adc/ad7780.c +2
-0+ +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.
Quoted 14 lines
drivers/staging/iio/adc/ad7780.c +7
-0+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) ||drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 182
Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +4
-1((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.
Quoted 112 lines
Quoted 7 lines
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.Quoted 1 line
Quoted 57 lines
Quoted 7 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 187
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +2
-0+ +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 :-)
Quoted 48 lines
Quoted 36 lines
drivers/staging/iio/adc/ad7780.c +5
-3@@ -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,drivers/staging/iio/adc/ad7780.c +1
-0@@ -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)drivers/staging/iio/adc/ad7780.c static int ad7780_probe(struct spi_device *spi) · line 287
Quoted 8 lines
drivers/staging/iio/adc/ad7780.c +5
-0@@ -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 forfilter_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...Quoted 38 lines
Quoted 38 lines
Quoted 29 lines
drivers/staging/iio/adc/ad7780.c +7
-0+ 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;include/linux/iio/adc/ad_sigma_delta.h +5
-0@@ -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) \Quoted 2 lines
__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 [email protected]. To post to this group, send email to [email protected].
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.
Jonathan Cameron
maintainerRe: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support
Quoted 98 lines · On Fri, 18 Jan 2019 18:19:40 -0200 Renato Lui Geh <[email protected]> wrote:
Hi,
Sorry for the (extremely) late reply.
Comments inline.
Renato
Quoted 89 lines · Jonathan Cameron
Quoted 1 line · On Thu, 29 Nov 2018 11:02:46 -0200 Giuliano Augusto Faulin Belinassi…
Hi
A few follow ups from me having read the result in patch 2.
Jonathan
Quoted 81 lines
Quoted 77 lines · Stefan Serban
Quoted 10 lines · Giuliano Belinassi
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 <[email protected]>---
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.Quoted 48 lines
drivers/staging/iio/adc/ad7780.c | 78 ++++++++++++++++++++++++-- include/linux/iio/adc/ad_sigma_delta.h | 5 ++ 2 files changed, 79 insertions(+), 4 deletions(-)drivers/staging/iio/adc/ad7780.c +6
-0@@ -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;drivers/staging/iio/adc/ad7780.c +12
-0@@ -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;drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev · line 146
Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +10
-0+ +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 inst->int_vref_mv.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
This was removed in commit
9eae69ddbc4717a0bd702eddac76c7848773bf71because 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.
Quoted 10 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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_mvwith 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_rawin 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).
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
Thanks for this info! :-)
Shouldn't we store this in uV (microVolts)? This will yield a more accurate result after the multiplication.Quoted 5 lines
Quoted 5 lines
drivers/staging/iio/adc/ad7780.c +5
-0+ +if (uvref <0 ) +return uvref; + + full_scale = 1 << (chip_info->channel.scan_type.realbitsQuoted 8 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev,
Quoted 6 lines
drivers/staging/iio/adc/ad7780.c +4
-1- 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->gainvariable.Yes, we forgot it.
Quoted 4 lines
Quoted 4 lines
drivers/staging/iio/adc/ad7780.c +3
-0+case IIO_CHAN_INFO_SAMP_FREQ : +if (val2 !=0 ) +return -EINVAL ;
Quoted 6 lines
Quoted 3 lines
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.
Quoted 36 lines
Quoted 36 lines
I'd also expect to see
read_rawsupport for this.Quoted 33 lines
Quoted 33 lines
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev · line 162
Quoted 3 lines
drivers/staging/iio/adc/ad7780.c +2
-0+ +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.
Quoted 24 lines
drivers/staging/iio/adc/ad7780.c +11
-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 ;
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
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.Quoted 1 line
...