staging: iio: ad7780: Add gain & filter gpio support

PATCH In review Giuliano Belinassi 4 participants 7 messages Nov 21, 2018 – Dec 1, 2018

Subsystems (4)

Staging IIO Analog Devices IIO drivers Staging - industrial io

lore

Share & tools

lore

Giuliano Belinassi

author

2018-11-21 18:04 UTC #0 lore raw

PATCH 1/1 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.

Signed-off-by Giuliano Belinassi <[email protected]>

---
 drivers/staging/iio/adc/ad7780.c       | 61 ++++++++++++++++++++++++--
 include/linux/iio/adc/ad_sigma_delta.h |  5 +++
 2 files changed, 62 insertions(+), 4 deletions(-)

drivers/staging/iio/adc/ad7780.c +57 -4

index c4a85789c2db..69794f06dbcd 100644
@@ -39,6 +39,9 @@
 #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
 #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
 
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1
+
 struct ad7780_chip_info {
 	struct iio_chan_spec	channel;
 	unsigned int		pattern_mask;

@@ -50,6 +53,8 @@ struct ad7780_state {
 	const struct ad7780_chip_info	*chip_info;
 	struct regulator		*reg;
 	struct gpio_desc		*powerdown_gpio;
+	struct gpio_desc		*gain_gpio;
+	struct gpio_desc		*filter_gpio;
 	unsigned int	gain;
 
 	struct ad_sigma_delta sd;

@@ -115,18 +120,51 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int ad7780_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long m)
+{
+	struct ad7780_state *st = iio_priv(indio_dev);
+
+	if (m != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	if (st->chip_info->is_ad778x) {
+		switch(val) {
+		case AD7780_GAIN_GPIO:
+			gpiod_set_value(st->gain_gpio, val2);
+		break;
+		case AD7780_FILTER_GPIO:
+			gpiod_set_value(st->filter_gpio, val2);
+		break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 				     unsigned int raw_sample)
 {
 	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
 	const struct ad7780_chip_info *chip_info = st->chip_info;
+	int val;
 
 	if ((raw_sample & AD7780_ERR) ||
 	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
 		return -EIO;
 
 	if (chip_info->is_ad778x) {
-		if (raw_sample & AD7780_GAIN)
+		val = raw_sample & AD7780_GAIN;
+
+		if (val != gpiod_get_value(st->gain_gpio))
+			return -EIO;
+
+		if (val)
 			st->gain = 1;
 		else
 			st->gain = 128;

@@ -141,18 +179,20 @@ static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
 	.has_registers = false,
 };
 
-#define AD7780_CHANNEL(bits, wordsize) \
+#define AD7170_CHANNEL(bits, wordsize) \
 	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
+#define AD7780_CHANNEL(bits, wordsize) \
+	AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
 
 static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 	[ID_AD7170] = {
-		.channel = AD7780_CHANNEL(12, 24),
+		.channel = AD7170_CHANNEL(12, 24),
 		.pattern = AD7170_PATTERN,
 		.pattern_mask = AD7170_PATTERN_MASK,
 		.is_ad778x = false,
 	},
 	[ID_AD7171] = {
-		.channel = AD7780_CHANNEL(16, 24),
+		.channel = AD7170_CHANNEL(16, 24),
 		.pattern = AD7170_PATTERN,
 		.pattern_mask = AD7170_PATTERN_MASK,
 		.is_ad778x = false,

@@ -173,6 +213,7 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 
 static const struct iio_info ad7780_info = {
 	.read_raw = ad7780_read_raw,
+	.write_raw = ad7780_write_raw,
 };
 
 static int ad7780_probe(struct spi_device *spi)

@@ -222,6 +263,18 @@ static int ad7780_probe(struct spi_device *spi)
 		goto error_disable_reg;
 	}
 
+	if (st->chip_info->is_ad778x) {
+		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
+							"gain",
+							GPIOD_OUT_HIGH);
+		if (IS_ERR(st->gain_gpio)) {
+			ret = PTR_ERR(st->gain_gpio);
+			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
+				ret);
+			goto error_disable_reg;
+		}
+	}
+
 	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
 	if (ret)
 		goto error_disable_reg;

include/linux/iio/adc/ad_sigma_delta.h +5 -0

index 730ead1a46df..6cadab6fd5fd 100644
@@ -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

Popa, Stefan Serban

2018-11-22 11:01 UTC #1 lore raw

Quoted 2 lines · Giuliano Belinassi

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

Hey,

Comments inline.

Quoted 41 lines

Signed-off-by Giuliano Belinassi <[email protected]>
---
drivers/staging/iio/adc/ad7780.c | 61 ++++++++++++++++++++++++-- include/linux/iio/adc/ad_sigma_delta.h | 5 +++
 2 files changed, 62 insertions(+), 4 deletions(-)
drivers/staging/iio/adc/ad7780.c +0 -0
index c4a85789c2db..69794f06dbcd 100644
@@ -39,6 +39,9 @@
  #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
  #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 |
 AD7170_PAT2)
drivers/staging/iio/adc/ad7780.c +3 -0
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1
+
struct ad7780_chip_info {
struct iio_chan_spec channel;
unsigned int pattern_mask;
drivers/staging/iio/adc/ad7780.c +2 -0
@@ -50,6 +53,8 @@ struct ad7780_state {
  	const struct ad7780_chip_info	*chip_info;
  	struct regulator		*reg;
  	struct gpio_desc		*powerdown_gpio;
+	struct gpio_desc		*gain_gpio;
+	struct gpio_desc		*filter_gpio;
  	unsigned int	gain;
struct ad_sigma_delta sd;
drivers/staging/iio/adc/ad7780.c +0 -0
@@ -115,18 +120,51 @@ static int ad7780_read_raw(struct iio_dev
 *indio_dev,
  	return -EINVAL;
  }
drivers/staging/iio/adc/ad7780.c +4 -0
+static int ad7780_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,

drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 136

Quoted 10 lines

drivers/staging/iio/adc/ad7780.c +10 -0

+			    long m)
+{
+	struct ad7780_state *st = iio_priv(indio_dev);
+
+	if (m != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	if (st->chip_info->is_ad778x) {
+		switch(val) {
+		case AD7780_GAIN_GPIO:

I think that instead of setting the gain directly, we should use the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datasheet there is a formula from which the output code can be calculated:
Code = 2^(N − 1)
× [(AIN × Gain /VREF) + 1]. So, by setting the scale from user space, the driver can calculate the correct gain by using the formula above. Also, it would be useful to introduce scale available.
Furthermore, there is a new
ad7124 adc driver which does this exact thing. Take a look here: https://gi
thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.c#L337.

drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 139

Quoted 3 lines

drivers/staging/iio/adc/ad7780.c +3 -0

+			gpiod_set_value(st->gain_gpio, val2);
+		break;
+		case AD7780_FILTER_GPIO:

The attribute that should be used to configure the filter gpio is IIO_CHAN_INFO_SAMP_FREQ. So, we should have 10 Hz and 16.7 Hz available sampling frequencies. If from user space the 10 Hz sampling freq is requested, then we set the FILTER pin high, while for 16.7 Hz the FILTER pin will be low.

Quoted 106 lines

drivers/staging/iio/adc/ad7780.c +10 -0
+			gpiod_set_value(st->filter_gpio, val2);
+		break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta, unsigned int raw_sample) {
struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta); const struct ad7780_chip_info *chip_info = st->chip_info;
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 +6 -1
-		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;
drivers/staging/iio/adc/ad7780.c +0 -0
@@ -141,18 +179,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 struct ad7780_chip_info ad7780_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 +213,7 @@ static const struct ad7780_chip_info
 ad7780_chip_info_tbl[] = {
static const struct iio_info ad7780_info = {
.read_raw = ad7780_read_raw,
drivers/staging/iio/adc/ad7780.c +1 -0
+	.write_raw = ad7780_write_raw,
};

static int ad7780_probe(struct spi_device *spi)
drivers/staging/iio/adc/ad7780.c +0 -0
@@ -222,6 +263,18 @@ static int ad7780_probe(struct spi_device *spi)
  		goto error_disable_reg;
  	}
drivers/staging/iio/adc/ad7780.c +7 -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)) {
+			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)
goto error_disable_reg;
include/linux/iio/adc/ad_sigma_delta.h +0 -0
index 730ead1a46df..6cadab6fd5fd 100644
@@ -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))
+
#define AD_SD_TEMP_CHANNEL(_si, _address, _bits, _storagebits, _shift) \
__AD_SD_CHANNEL(_si, 0, -1, _address, _bits, \
_storagebits, _shift, NULL, IIO_TEMP, \

Jonathan Cameron

maintainer

2018-11-25 11:08 UTC #2 lore raw

Quoted 69 lines · Stefan Serban

Quoted 2 lines · Giuliano Belinassi
Previously, the AD7780 driver only supported gpio for the 'powerdown' pin. This commit adds suppport for the 'gain' and 'filter' pin.
Hey,
Comments inline.
Quoted 41 lines
Signed-off-by Giuliano Belinassi <[email protected]>
---
drivers/staging/iio/adc/ad7780.c | 61 ++++++++++++++++++++++++-- include/linux/iio/adc/ad_sigma_delta.h | 5 +++
 2 files changed, 62 insertions(+), 4 deletions(-)
drivers/staging/iio/adc/ad7780.c +0 -0
index c4a85789c2db..69794f06dbcd 100644
@@ -39,6 +39,9 @@
  #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
  #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 |
 AD7170_PAT2)
drivers/staging/iio/adc/ad7780.c +3 -0
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1
+
struct ad7780_chip_info {
struct iio_chan_spec channel;
unsigned int pattern_mask;
drivers/staging/iio/adc/ad7780.c +2 -0
@@ -50,6 +53,8 @@ struct ad7780_state {
  	const struct ad7780_chip_info	*chip_info;
  	struct regulator		*reg;
  	struct gpio_desc		*powerdown_gpio;
+	struct gpio_desc		*gain_gpio;
+	struct gpio_desc		*filter_gpio;
  	unsigned int	gain;
struct ad_sigma_delta sd;
drivers/staging/iio/adc/ad7780.c +0 -0
@@ -115,18 +120,51 @@ static int ad7780_read_raw(struct iio_dev
 *indio_dev,
  	return -EINVAL;
  }
drivers/staging/iio/adc/ad7780.c +4 -0
+static int ad7780_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 136
Quoted 10 lines
drivers/staging/iio/adc/ad7780.c +10 -0
+			    long m)
+{
+	struct ad7780_state *st = iio_priv(indio_dev);
+
+	if (m != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	if (st->chip_info->is_ad778x) {
+		switch(val) {
+		case AD7780_GAIN_GPIO:  
I think that instead of setting the gain directly, we should use the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datasheet there is a formula from which the output code can be calculated:
Code = 2^(N − 1)
× [(AIN × Gain /VREF) + 1]. So, by setting the scale from user space, the driver can calculate the correct gain by using the formula above. Also, it would be useful to introduce scale available.
Furthermore, there is a new
ad7124 adc driver which does this exact thing. Take a look here: https://gi
thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.c#L337.

Quoted 9 lines

drivers/staging/iio/adc/ad7780.c static int ad7780_read_raw(struct iio_dev *indio_dev, · line 139
Quoted 3 lines
drivers/staging/iio/adc/ad7780.c +3 -0
+			gpiod_set_value(st->gain_gpio, val2);
+		break;
+		case AD7780_FILTER_GPIO:  
The attribute that should be used to configure the filter gpio is IIO_CHAN_INFO_SAMP_FREQ. So, we should have 10 Hz and 16.7 Hz available sampling frequencies. If from user space the 10 Hz sampling freq is requested, then we set the FILTER pin high, while for 16.7 Hz the FILTER pin will be low.

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

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

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

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

Thanks,

Jonathan

Quoted 107 lines

Quoted 107 lines
drivers/staging/iio/adc/ad7780.c +10 -0
+			gpiod_set_value(st->filter_gpio, val2);
+		break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta, unsigned int raw_sample) {
struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta); const struct ad7780_chip_info *chip_info = st->chip_info;
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 +6 -1
-		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;
drivers/staging/iio/adc/ad7780.c +0 -0
@@ -141,18 +179,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 struct ad7780_chip_info ad7780_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 +213,7 @@ static const struct ad7780_chip_info
 ad7780_chip_info_tbl[] = {
static const struct iio_info ad7780_info = {
.read_raw = ad7780_read_raw,
drivers/staging/iio/adc/ad7780.c +1 -0
+	.write_raw = ad7780_write_raw,
};

static int ad7780_probe(struct spi_device *spi)
drivers/staging/iio/adc/ad7780.c +0 -0
@@ -222,6 +263,18 @@ static int ad7780_probe(struct spi_device *spi)
  		goto error_disable_reg;
  	}
drivers/staging/iio/adc/ad7780.c +7 -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)) {
+			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)
goto error_disable_reg;
include/linux/iio/adc/ad_sigma_delta.h +0 -0
index 730ead1a46df..6cadab6fd5fd 100644
@@ -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))
+
#define AD_SD_TEMP_CHANNEL(_si, _address, _bits, _storagebits, _shift) \
__AD_SD_CHANNEL(_si, 0, -1, _address, _bits, \
_storagebits, _shift, NULL, IIO_TEMP,

Giuliano Belinassi

author

2018-11-26 19:24 UTC #3 lore raw

Hi, thank you for the review

Quoted 12 lines

Quoted 10 lines · Stefan Serban
I think that instead of setting the gain directly, we should use the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datasheet there is a formula from which the output code can be calculated:
Code = 2^(N − 1)
× [(AIN × Gain /VREF) + 1]. So, by setting the scale from user space, the driver can calculate the correct gain by using the formula above. Also, it would be useful to introduce scale available.
Furthermore, there is a new
ad7124 adc driver which does this exact thing. Take a look here: https://gi
thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.c#L337.

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

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

Thank you

Popa, Stefan Serban

2018-11-27 11:11 UTC #4 lore raw

On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote:
Hi, please see bellow

Quoted 21 lines

Hi, thank you for the review
Quoted 4 lines
Quoted 1 line · Stefan Serban
Quoted 15 lines
Quoted 7 lines
I think that instead of setting the gain directly, we should use the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datasheet there
is a formula from which the output code can be calculated:
Code = 2^(N − 1)
× [(AIN × Gain /VREF) + 1]. So, by setting the scale from user space, the
Quoted 8 lines
driver can calculate the correct gain by using the formula above. Also, it
would be useful to introduce scale available.
Furthermore, there is a new
ad7124 adc driver which does this exact thing. Take a look here: http s://gi thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.c# L337.

Quoted 2 lines

We have some questions about the code you provided to us:
1-) What is exactly the inputs for the write_raw function?

In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case. Setting the scale from user space looks something like this:
root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale . Furthermore, in your write_raw() function, val=0 and val2=298. Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate the gain = Vref / (full_scale_voltage * scale). We only support two gains (1 and 128), so we need to determine which one fits better with the desired scale. Finally, all we have left to do is to set the gain.

Quoted 3 lines

2-) Could you give more details about the math around lines 346-348? Is it correct to assume that the multiplication at line 346 won't overflow? (vref is an uint)

It is correct that Vref is in microvolts, so for example, Vref of 2.5V = 2500000000uV. It won't overflow since we use the Vref as nominator, while full_scale_voltage and scale are the denominators.

Quoted 5 lines

And regarding our code:
1-) The val in our write_raw function should be, in case of GAIN, a number that best approximate the actual gain value of 1 or 128? For instance, if the user inputs 126, we should default to 128?

We should not allow the the user to input the gain, he needs to input the scale (see the mail from Jonathan and the above explanation). However, if the calculated gain is one that is not supported, such as 126, we will set the closest matching value, in this case 128.

Quoted 2 lines

2-) In the case of FILTER, is it the same? Is the user sending the value in mHz (milihertz)?

Yes, it is the same with the FILTER. You need to add
a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user space, the input value should be in Hz, something like this:
root:/sys/bus/iio/devices/iio:device0> echo 10 > in_voltage_sampling_frequency

Quoted 3 lines

Thank you

Ardelean, Alexandru

2018-11-29 12:19 UTC #5 lore raw

Quoted 3 lines · Stefan Serban

On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote:
Hi, please see bellow

One note from me here.

Quoted 39 lines

Quoted 26 lines
Hi, thank you for the review
Quoted 15 lines
Quoted 12 lines · Stefan Serban
I think that instead of setting the gain directly, we should use the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datasheet
there
is a formula from which the output code can be calculated:
Code = 2^(N − 1)
× [(AIN × Gain /VREF) + 1]. So, by setting the scale from user space,
the
driver can calculate the correct gain by using the formula above. Also, it
Quoted 8 lines
Quoted 8 lines
would be useful to introduce scale available. Furthermore, there is a new
ad7124 adc driver which does this exact thing. Take a look here:
http
s://gi thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124. c#
L337.
Quoted 2 lines
We have some questions about the code you provided to us:
1-) What is exactly the inputs for the write_raw function?
In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case. Setting the scale from user space looks something like this:
root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale . Furthermore, in your write_raw() function, val=0 and val2=298. Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate the gain = Vref / (full_scale_voltage * scale). We only support two gains (1 and 128), so we need to determine which one fits better with the desired
scale. Finally, all we have left to do is to set the gain.

Quoted 8 lines

Quoted 3 lines
2-) Could you give more details about the math around lines 346-348?
Is it correct to assume that the multiplication at line 346 won't overflow? (vref is an uint)
It is correct that Vref is in microvolts, so for example, Vref of 2.5V = 2500000000uV. It won't overflow since we use the Vref as nominator, while full_scale_voltage and scale are the denominators.

[Regarding the AD7124] I guess I should be noted that the code can overflow, but in this case it shouldn't, because (according to the device datasheet) the maximum VREF can be 3600 mV.
A user could specify (in the devicetree) something larger than 4300 mV (and that would overflow) but that would also make the readings useless as the external VREF would be invalid ; and there is also the risk of frying the chip in that case [something you really can't protect the user against].

The internal VREF however is 2500 mV, so things should be fine from that point of view.

Typically, in datasheets (at least from Analog Devices) it's good to take a look at the specifications sections.
[For AD7124] You will see that the internal VREF [page 8] is 2.5V (with approximation of +/-0.2%) and for external reference it goes all the way up to AVDD, which has typical values of 2.9V - 3.6V.
So, for u32 this code should be fine and not overflow.

One small thing that can be confusing about that code in AD7124 is that it gets multiplied with 1000000LL (which is signed long-long), but that should be fine, since the operation should be converted to u32 (unsigned int) representation [by being assigned to vref], which ends up being fine in the end.

Quoted 23 lines

Quoted 5 lines
And regarding our code:
1-) The val in our write_raw function should be, in case of GAIN, a
number that best approximate the actual gain value of 1 or 128? For instance, if the user inputs 126, we should default to 128?
We should not allow the the user to input the gain, he needs to input the scale (see the mail from Jonathan and the above explanation). However, if the calculated gain is one that is not supported, such as 126, we will set
the closest matching value, in this case 128.
Quoted 2 lines
2-) In the case of FILTER, is it the same? Is the user sending the
value in mHz (milihertz)?
Yes, it is the same with the FILTER. You need to add
a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user space, the input value should be in Hz, something like this:
root:/sys/bus/iio/devices/iio:device0> echo 10 > in_voltage_sampling_frequency
Quoted 2 lines
Thank you

Jonathan Cameron

maintainer

2018-12-01 15:17 UTC #6 lore raw

Quoted 55 lines · Alexandru

Quoted 3 lines · Stefan Serban
On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote:
Hi, please see bellow
One note from me here.
Quoted 39 lines
Quoted 26 lines
Hi, thank you for the review
Quoted 15 lines
Quoted 12 lines · Stefan Serban
I think that instead of setting the gain directly, we should use the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780 datasheet
there
is a formula from which the output code can be calculated:
Code = 2^(N − 1)
× [(AIN × Gain /VREF) + 1]. So, by setting the scale from user space,
the
driver can calculate the correct gain by using the formula above. Also, it
Quoted 8 lines
Quoted 8 lines
would be useful to introduce scale available. Furthermore, there is a new
ad7124 adc driver which does this exact thing. Take a look here:
http
s://gi thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124. c#
L337.
Quoted 2 lines
We have some questions about the code you provided to us:
1-) What is exactly the inputs for the write_raw function?
In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case. Setting the scale from user space looks something like this:
root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale . Furthermore, in your write_raw() function, val=0 and val2=298. Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate the gain = Vref / (full_scale_voltage * scale). We only support two gains (1 and 128), so we need to determine which one fits better with the desired
scale. Finally, all we have left to do is to set the gain.
Quoted 8 lines
Quoted 3 lines
2-) Could you give more details about the math around lines 346-348?
Is it correct to assume that the multiplication at line 346 won't overflow? (vref is an uint)
It is correct that Vref is in microvolts, so for example, Vref of 2.5V = 2500000000uV. It won't overflow since we use the Vref as nominator, while full_scale_voltage and scale are the denominators.

Quoted 7 lines

[Regarding the AD7124] I guess I should be noted that the code can overflow, but in this case it shouldn't, because (according to the device datasheet) the maximum VREF can be 3600 mV.
A user could specify (in the devicetree) something larger than 4300 mV (and that would overflow) but that would also make the readings useless as the external VREF would be invalid ; and there is also the risk of frying the chip in that case [something you really can't protect the user against].

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

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

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

Jonathan

Quoted 40 lines

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