[PATCH 0/2] staging: iio: ad7780: move out of staging

[PATCH 0/2] staging: iio: ad7780: move out of staging

[Giuliano Belinassi]

This series of patches add user input to ad7780 'gain' & 'filter' gpio
pins. Also, it moves the ad7780 out of staging to the main tree.

Giuliano Belinassi (2):
  staging: iio: ad7780: Add gain & filter gpio support
  staging: iio: ad7780: Moving ad7780 out of staging

 drivers/iio/adc/Kconfig                |  13 +
 drivers/iio/adc/Makefile               |   1 +
 drivers/iio/adc/ad7780.c               | 347 +++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig        |  13 -
 drivers/staging/iio/adc/Makefile       |   1 -
 drivers/staging/iio/adc/ad7780.c       | 277 --------------------
 include/linux/iio/adc/ad_sigma_delta.h |   5 +
 7 files changed, 366 insertions(+), 291 deletions(-)
 create mode 100644 drivers/iio/adc/ad7780.c
 delete mode 100644 drivers/staging/iio/adc/ad7780.c

-- 
2.19.1

[PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support

[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 <giuliano.belinassi@usp.br>
---
Changes in v2:
    - Now this patch is part of the patchset that aims to remove ad7780
out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2
    - Also, now it reads voltage and filter values from the userspace
instead of gpio pin states.

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

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

Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support

[Popa, Stefan Serban]

On Mi, 2018-11-28 at 16:15 -0200, Giuliano Belinassi wrote:
> Previously, the AD7780 driver only supported gpio for the 'powerdown'
> pin. This commit adds suppport for the 'gain' and 'filter' pin.
> 
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> ---
> Changes in v2:
>     - Now this patch is part of the patchset that aims to remove ad7780
> out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2
>     - Also, now it reads voltage and filter values from the userspace
> instead of gpio pin states.

Hello,
Please see bellow.

> 
>  drivers/staging/iio/adc/ad7780.c       | 78 ++++++++++++++++++++++++--
>  include/linux/iio/adc/ad_sigma_delta.h |  5 ++
>  2 files changed, 79 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c4a85789c2db..05979a79fda3 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -39,6 +39,12 @@
>  #define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
>  #define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 |
> AD7170_PAT2)
>  
> +#define AD7780_GAIN_GPIO	0
> +#define AD7780_FILTER_GPIO	1
> +
> +#define AD7780_GAIN_MIDPOINT	64
> +#define AD7780_FILTER_MIDPOINT	13350
> +
>  struct ad7780_chip_info {
>  	struct iio_chan_spec	channel;
>  	unsigned int		pattern_mask;
> @@ -50,6 +56,8 @@ struct ad7780_state {
>  	const struct ad7780_chip_info	*chip_info;
>  	struct regulator		*reg;
>  	struct gpio_desc		*powerdown_gpio;
> +	struct gpio_desc		*gain_gpio;
> +	struct gpio_desc		*filter_gpio;
>  	unsigned int	gain;
>  
>  	struct ad_sigma_delta sd;
> @@ -115,18 +123,65 @@ static int ad7780_read_raw(struct iio_dev
> *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static int ad7780_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val,
> +			    int val2,
> +			    long m)
> +{
> +	struct ad7780_state *st = iio_priv(indio_dev);
> +	const struct ad7780_chip_info *chip_info = st->chip_info;
> +	int uvref, gain;
> +	unsigned int full_scale;
> +
> +	if (!chip_info->is_ad778x)
> +		return 0;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		uvref = regulator_get_voltage(st->reg);

regulator_get_voltage() has already been called in the probe function and
the result is stored in st->int_vref_mv.
My suggestion would be to use a local vref variable declared as unsigned
int. It is my fault that I haven't explained correctly in the previous
email, but you need to multiply vref_mv with 1000000LL in order to get the
right precision: vref = st->int_vref_mv * 1000000LL. Afterwards you will be
able to perform the divisions.
> +
> +		if (uvref < 0)
> +			return uvref;
> +
> +		full_scale = 1 << (chip_info->channel.scan_type.realbits 
> - 1);
> +		gain = DIV_ROUND_CLOSEST(uvref, full_scale);
> +		gain = DIV_ROUND_CLOSEST(gain, val2);
> +
> +		gpiod_set_value(st->gain_gpio, gain <
> AD7780_GAIN_MIDPOINT ? 0 : 1);

Once the gain is set, you can store it in st->gain variable.

> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (val2 != 0)
> +			return -EINVAL;
> +
> +		gpiod_set_value(st->filter_gpio, val <
> AD7780_FILTER_MIDPOINT ? 0 : 1);

This is probably fine, although I am not a big fan of the ternary operator.
A simple if else statement would do. However, I don't feel strongly about
it, so feel free to disagree. 

> +	break;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>  				     unsigned int raw_sample)
>  {
>  	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
>  	const struct ad7780_chip_info *chip_info = st->chip_info;
> +	int val;
>  
>  	if ((raw_sample & AD7780_ERR) ||
>  	    ((raw_sample & chip_info->pattern_mask) != chip_info-
> >pattern))
>  		return -EIO;
>  
>  	if (chip_info->is_ad778x) {
> -		if (raw_sample & AD7780_GAIN)
> +		val = raw_sample & AD7780_GAIN;
> +
> +		if (val != gpiod_get_value(st->gain_gpio))
> +			return -EIO;

It is not obvious to me what is the point of this check. Maybe you can add
a comment?

> +
> +		if (val)
>  			st->gain = 1;
>  		else
>  			st->gain = 128;

Do we still need this? I am not convinced. 

> @@ -141,18 +196,20 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
>  	.has_registers = false,
>  };
>  
> -#define AD7780_CHANNEL(bits, wordsize) \
> +#define AD7170_CHANNEL(bits, wordsize) \
>  	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> +#define AD7780_CHANNEL(bits, wordsize) \
> +	AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
>  
>  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
>  	[ID_AD7170] = {
> -		.channel = AD7780_CHANNEL(12, 24),
> +		.channel = AD7170_CHANNEL(12, 24),
>  		.pattern = AD7170_PATTERN,
>  		.pattern_mask = AD7170_PATTERN_MASK,
>  		.is_ad778x = false,
>  	},
>  	[ID_AD7171] = {
> -		.channel = AD7780_CHANNEL(16, 24),
> +		.channel = AD7170_CHANNEL(16, 24),
>  		.pattern = AD7170_PATTERN,
>  		.pattern_mask = AD7170_PATTERN_MASK,
>  		.is_ad778x = false,
> @@ -173,6 +230,7 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
>  
>  static const struct iio_info ad7780_info = {
>  	.read_raw = ad7780_read_raw,
> +	.write_raw = ad7780_write_raw,
>  };
>  
>  static int ad7780_probe(struct spi_device *spi)
> @@ -222,6 +280,18 @@ static int ad7780_probe(struct spi_device *spi)
>  		goto error_disable_reg;
>  	}
>  
> +	if (st->chip_info->is_ad778x) {
> +		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
> +							"gain",
> +							GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->gain_gpio)) {

if the GPIO is optional, then we should continue in case of -ENODEV.

Shouldn't we have a devm_gpiod_get_optional() call also for filter_gpio?

> +			ret = PTR_ERR(st->gain_gpio);
> +			dev_err(&spi->dev, "Failed to request gain GPIO:
> %d\n",
> +				ret);
> +			goto error_disable_reg;
> +		}
> +	}
> +
>  	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>  	if (ret)
>  		goto error_disable_reg;
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h
> b/include/linux/iio/adc/ad_sigma_delta.h
> index 730ead1a46df..6cadab6fd5fd 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -173,6 +173,11 @@ int ad_sd_validate_trigger(struct iio_dev
> *indio_dev, struct iio_trigger *trig);
>  	__AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
>  		_storagebits, _shift, NULL, IIO_VOLTAGE, 0)
>  
> +#define AD_SD_CHANNEL_GAIN_FILTER(_si, _channel, _address, _bits, \
> +	_storagebits, _shift) \
> +	__AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
> +		_storagebits, _shift, NULL, IIO_VOLTAGE,
> BIT(IIO_CHAN_INFO_RAW))
> +
>  #define AD_SD_TEMP_CHANNEL(_si, _address, _bits, _storagebits, _shift) \
>  	__AD_SD_CHANNEL(_si, 0, -1, _address, _bits, \
>  		_storagebits, _shift, NULL, IIO_TEMP, \

Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support

[Giuliano Augusto Faulin Belinassi]

Hi

On Thu, Nov 29, 2018 at 9:18 AM Popa, Stefan Serban
<StefanSerban.Popa@analog.com> wrote:
>
> On Mi, 2018-11-28 at 16:15 -0200, Giuliano Belinassi wrote:
> > Previously, the AD7780 driver only supported gpio for the 'powerdown'
> > pin. This commit adds suppport for the 'gain' and 'filter' pin.
> >
> > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > ---
> > Changes in v2:
> >     - Now this patch is part of the patchset that aims to remove ad7780
> > out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2
> >     - Also, now it reads voltage and filter values from the userspace
> > instead of gpio pin states.
>
> Hello,
> Please see bellow.
>
> >
> >  drivers/staging/iio/adc/ad7780.c       | 78 ++++++++++++++++++++++++--
> >  include/linux/iio/adc/ad_sigma_delta.h |  5 ++
> >  2 files changed, 79 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index c4a85789c2db..05979a79fda3 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -39,6 +39,12 @@
> >  #define AD7170_PATTERN               (AD7780_PAT0 | AD7170_PAT2)
> >  #define AD7170_PATTERN_MASK  (AD7780_PAT0 | AD7780_PAT1 |
> > AD7170_PAT2)
> >
> > +#define AD7780_GAIN_GPIO     0
> > +#define AD7780_FILTER_GPIO   1
> > +
> > +#define AD7780_GAIN_MIDPOINT 64
> > +#define AD7780_FILTER_MIDPOINT       13350
> > +
> >  struct ad7780_chip_info {
> >       struct iio_chan_spec    channel;
> >       unsigned int            pattern_mask;
> > @@ -50,6 +56,8 @@ struct ad7780_state {
> >       const struct ad7780_chip_info   *chip_info;
> >       struct regulator                *reg;
> >       struct gpio_desc                *powerdown_gpio;
> > +     struct gpio_desc                *gain_gpio;
> > +     struct gpio_desc                *filter_gpio;
> >       unsigned int    gain;
> >
> >       struct ad_sigma_delta sd;
> > @@ -115,18 +123,65 @@ static int ad7780_read_raw(struct iio_dev
> > *indio_dev,
> >       return -EINVAL;
> >  }
> >
> > +static int ad7780_write_raw(struct iio_dev *indio_dev,
> > +                         struct iio_chan_spec const *chan,
> > +                         int val,
> > +                         int val2,
> > +                         long m)
> > +{
> > +     struct ad7780_state *st = iio_priv(indio_dev);
> > +     const struct ad7780_chip_info *chip_info = st->chip_info;
> > +     int uvref, gain;
> > +     unsigned int full_scale;
> > +
> > +     if (!chip_info->is_ad778x)
> > +             return 0;
> > +
> > +     switch (m) {
> > +     case IIO_CHAN_INFO_SCALE:
> > +             if (val != 0)
> > +                     return -EINVAL;
> > +
> > +             uvref = regulator_get_voltage(st->reg);
>
> regulator_get_voltage() has already been called in the probe function and
> the result is stored in st->int_vref_mv.

This was removed in commit  9eae69ddbc4717a0bd702eddac76c7848773bf71
because the value was not being updated. But I agree if the vref
voltage is not going to change at all after the initialization, then
this value should be kept in memory.

> My suggestion would be to use a local vref variable declared as unsigned
> int. It is my fault that I haven't explained correctly in the previous
> email, but you need to multiply vref_mv with 1000000LL in order to get the
> right precision: vref = st->int_vref_mv * 1000000LL. Afterwards you will be
> able to perform the divisions.

Thanks for this info! :-)
Shouldn't we store this in uV (microVolts)? This will yield a more
accurate result after the multiplication.

> > +
> > +             if (uvref < 0)
> > +                     return uvref;
> > +
> > +             full_scale = 1 << (chip_info->channel.scan_type.realbits
> > - 1);
> > +             gain = DIV_ROUND_CLOSEST(uvref, full_scale);
> > +             gain = DIV_ROUND_CLOSEST(gain, val2);
> > +
> > +             gpiod_set_value(st->gain_gpio, gain <
> > AD7780_GAIN_MIDPOINT ? 0 : 1);
>
> Once the gain is set, you can store it in st->gain variable.

Yes, we forgot it.

>
> > +     case IIO_CHAN_INFO_SAMP_FREQ:
> > +             if (val2 != 0)
> > +                     return -EINVAL;
> > +
> > +             gpiod_set_value(st->filter_gpio, val <
> > AD7780_FILTER_MIDPOINT ? 0 : 1);
>
> This is probably fine, although I am not a big fan of the ternary operator.
> A simple if else statement would do. However, I don't feel strongly about
> it, so feel free to disagree.
>
> > +     break;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> >                                    unsigned int raw_sample)
> >  {
> >       struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
> >       const struct ad7780_chip_info *chip_info = st->chip_info;
> > +     int val;
> >
> >       if ((raw_sample & AD7780_ERR) ||
> >           ((raw_sample & chip_info->pattern_mask) != chip_info-
> > >pattern))
> >               return -EIO;
> >
> >       if (chip_info->is_ad778x) {
> > -             if (raw_sample & AD7780_GAIN)
> > +             val = raw_sample & AD7780_GAIN;
> > +
> > +             if (val != gpiod_get_value(st->gain_gpio))
> > +                     return -EIO;
>
> It is not obvious to me what is the point of this check. Maybe you can add
> a comment?

It seems to be a redundancy check. It is getting the 32-bits
raw_output, getting the bit that represents the GAIN value and
checking if the pin is set accordingly (see Figure 22 of datasheet,
page 13). Is this correct? If yes we add a comment explaining this.

>
> > +
> > +             if (val)
> >                       st->gain = 1;
> >               else
> >                       st->gain = 128;
>
> Do we still need this? I am not convinced.
No, I don't think so. Thanks for pointing this out :-)

>
> > @@ -141,18 +196,20 @@ static const struct ad_sigma_delta_info
> > ad7780_sigma_delta_info = {
> >       .has_registers = false,
> >  };
> >
> > -#define AD7780_CHANNEL(bits, wordsize) \
> > +#define AD7170_CHANNEL(bits, wordsize) \
> >       AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> > +#define AD7780_CHANNEL(bits, wordsize) \
> > +     AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
> >
> >  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> >       [ID_AD7170] = {
> > -             .channel = AD7780_CHANNEL(12, 24),
> > +             .channel = AD7170_CHANNEL(12, 24),
> >               .pattern = AD7170_PATTERN,
> >               .pattern_mask = AD7170_PATTERN_MASK,
> >               .is_ad778x = false,
> >       },
> >       [ID_AD7171] = {
> > -             .channel = AD7780_CHANNEL(16, 24),
> > +             .channel = AD7170_CHANNEL(16, 24),
> >               .pattern = AD7170_PATTERN,
> >               .pattern_mask = AD7170_PATTERN_MASK,
> >               .is_ad778x = false,
> > @@ -173,6 +230,7 @@ static const struct ad7780_chip_info
> > ad7780_chip_info_tbl[] = {
> >
> >  static const struct iio_info ad7780_info = {
> >       .read_raw = ad7780_read_raw,
> > +     .write_raw = ad7780_write_raw,
> >  };
> >
> >  static int ad7780_probe(struct spi_device *spi)
> > @@ -222,6 +280,18 @@ static int ad7780_probe(struct spi_device *spi)
> >               goto error_disable_reg;
> >       }
> >
> > +     if (st->chip_info->is_ad778x) {
> > +             st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
> > +                                                     "gain",
> > +                                                     GPIOD_OUT_HIGH);
> > +             if (IS_ERR(st->gain_gpio)) {
>
> if the GPIO is optional, then we should continue in case of -ENODEV.
>
> Shouldn't we have a devm_gpiod_get_optional() call also for filter_gpio?
>
> > +                     ret = PTR_ERR(st->gain_gpio);
> > +                     dev_err(&spi->dev, "Failed to request gain GPIO:
> > %d\n",
> > +                             ret);
> > +                     goto error_disable_reg;
> > +             }
> > +     }
> > +
> >       ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> >       if (ret)
> >               goto error_disable_reg;
> > diff --git a/include/linux/iio/adc/ad_sigma_delta.h
> > b/include/linux/iio/adc/ad_sigma_delta.h
> > index 730ead1a46df..6cadab6fd5fd 100644
> > --- a/include/linux/iio/adc/ad_sigma_delta.h
> > +++ b/include/linux/iio/adc/ad_sigma_delta.h
> > @@ -173,6 +173,11 @@ int ad_sd_validate_trigger(struct iio_dev
> > *indio_dev, struct iio_trigger *trig);
> >       __AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
> >               _storagebits, _shift, NULL, IIO_VOLTAGE, 0)
> >
> > +#define AD_SD_CHANNEL_GAIN_FILTER(_si, _channel, _address, _bits, \
> > +     _storagebits, _shift) \
> > +     __AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
> > +             _storagebits, _shift, NULL, IIO_VOLTAGE,
> > BIT(IIO_CHAN_INFO_RAW))
> > +
> >  #define AD_SD_TEMP_CHANNEL(_si, _address, _bits, _storagebits, _shift) \
> >       __AD_SD_CHANNEL(_si, 0, -1, _address, _bits, \
> >               _storagebits, _shift, NULL, IIO_TEMP, \
>
> --
> You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
> To post to this group, send email to kernel-usp@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/1543490289.11186.22.camel%40analog.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support

[Jonathan Cameron]

On Thu, 29 Nov 2018 11:02:46 -0200
Giuliano Augusto Faulin Belinassi <giuliano.belinassi@usp.br> wrote:

> Hi

A few follow ups from me having read the result in patch 2.

Jonathan
> 
> On Thu, Nov 29, 2018 at 9:18 AM Popa, Stefan Serban
> <StefanSerban.Popa@analog.com> wrote:
> >
> > On Mi, 2018-11-28 at 16:15 -0200, Giuliano Belinassi wrote:  
> > > Previously, the AD7780 driver only supported gpio for the 'powerdown'
> > > pin. This commit adds suppport for the 'gain' and 'filter' pin.
> > >
> > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> > > ---
> > > Changes in v2:
> > >     - Now this patch is part of the patchset that aims to remove ad7780
> > > out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2
> > >     - Also, now it reads voltage and filter values from the userspace
> > > instead of gpio pin states.  
> >
> > Hello,
> > Please see bellow.
> >  
> > >
> > >  drivers/staging/iio/adc/ad7780.c       | 78 ++++++++++++++++++++++++--
> > >  include/linux/iio/adc/ad_sigma_delta.h |  5 ++
> > >  2 files changed, 79 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index c4a85789c2db..05979a79fda3 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -39,6 +39,12 @@
> > >  #define AD7170_PATTERN               (AD7780_PAT0 | AD7170_PAT2)
> > >  #define AD7170_PATTERN_MASK  (AD7780_PAT0 | AD7780_PAT1 |
> > > AD7170_PAT2)
> > >
> > > +#define AD7780_GAIN_GPIO     0
> > > +#define AD7780_FILTER_GPIO   1
> > > +
> > > +#define AD7780_GAIN_MIDPOINT 64
> > > +#define AD7780_FILTER_MIDPOINT       13350
> > > +
> > >  struct ad7780_chip_info {
> > >       struct iio_chan_spec    channel;
> > >       unsigned int            pattern_mask;
> > > @@ -50,6 +56,8 @@ struct ad7780_state {
> > >       const struct ad7780_chip_info   *chip_info;
> > >       struct regulator                *reg;
> > >       struct gpio_desc                *powerdown_gpio;
> > > +     struct gpio_desc                *gain_gpio;
> > > +     struct gpio_desc                *filter_gpio;
> > >       unsigned int    gain;
> > >
> > >       struct ad_sigma_delta sd;
> > > @@ -115,18 +123,65 @@ static int ad7780_read_raw(struct iio_dev
> > > *indio_dev,
> > >       return -EINVAL;
> > >  }
> > >
> > > +static int ad7780_write_raw(struct iio_dev *indio_dev,
> > > +                         struct iio_chan_spec const *chan,
> > > +                         int val,
> > > +                         int val2,
> > > +                         long m)
> > > +{
> > > +     struct ad7780_state *st = iio_priv(indio_dev);
> > > +     const struct ad7780_chip_info *chip_info = st->chip_info;
> > > +     int uvref, gain;
> > > +     unsigned int full_scale;
> > > +
> > > +     if (!chip_info->is_ad778x)
> > > +             return 0;
> > > +
> > > +     switch (m) {
> > > +     case IIO_CHAN_INFO_SCALE:
> > > +             if (val != 0)
> > > +                     return -EINVAL;
> > > +
> > > +             uvref = regulator_get_voltage(st->reg);  
> >
> > regulator_get_voltage() has already been called in the probe function and
> > the result is stored in st->int_vref_mv.  
> 
> This was removed in commit  9eae69ddbc4717a0bd702eddac76c7848773bf71
> because the value was not being updated. But I agree if the vref
> voltage is not going to change at all after the initialization, then
> this value should be kept in memory.
> 
> > My suggestion would be to use a local vref variable declared as unsigned
> > int. It is my fault that I haven't explained correctly in the previous
> > email, but you need to multiply vref_mv with 1000000LL in order to get the
> > right precision: vref = st->int_vref_mv * 1000000LL. Afterwards you will be
> > able to perform the divisions.  
> 
> Thanks for this info! :-)
> Shouldn't we store this in uV (microVolts)? This will yield a more
> accurate result after the multiplication.
> 
> > > +
> > > +             if (uvref < 0)
> > > +                     return uvref;
> > > +
> > > +             full_scale = 1 << (chip_info->channel.scan_type.realbits
> > > - 1);
> > > +             gain = DIV_ROUND_CLOSEST(uvref, full_scale);
> > > +             gain = DIV_ROUND_CLOSEST(gain, val2);
> > > +
> > > +             gpiod_set_value(st->gain_gpio, gain <
> > > AD7780_GAIN_MIDPOINT ? 0 : 1);  
> >
> > Once the gain is set, you can store it in st->gain variable.  
> 
> Yes, we forgot it.
> 
> >  
> > > +     case IIO_CHAN_INFO_SAMP_FREQ:
> > > +             if (val2 != 0)
> > > +                     return -EINVAL;
comment I raised in patch 2 about the odd preciseness of insisting
no decimal places, but matching any value based on a threshold on the
whole number part.

I'd also expect to see read_raw support for this.

> > > +
> > > +             gpiod_set_value(st->filter_gpio, val <
> > > AD7780_FILTER_MIDPOINT ? 0 : 1);  
> >
> > This is probably fine, although I am not a big fan of the ternary operator.
> > A simple if else statement would do. However, I don't feel strongly about
> > it, so feel free to disagree.
> >  
> > > +     break;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> > >                                    unsigned int raw_sample)
> > >  {
> > >       struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
> > >       const struct ad7780_chip_info *chip_info = st->chip_info;
> > > +     int val;
> > >
> > >       if ((raw_sample & AD7780_ERR) ||
> > >           ((raw_sample & chip_info->pattern_mask) != chip_info-  
> > > >pattern))  
> > >               return -EIO;
> > >
> > >       if (chip_info->is_ad778x) {
> > > -             if (raw_sample & AD7780_GAIN)
> > > +             val = raw_sample & AD7780_GAIN;
> > > +
> > > +             if (val != gpiod_get_value(st->gain_gpio))
> > > +                     return -EIO;  
> >
> > It is not obvious to me what is the point of this check. Maybe you can add
> > a comment?  
> 
> It seems to be a redundancy check. It is getting the 32-bits
> raw_output, getting the bit that represents the GAIN value and
> checking if the pin is set accordingly (see Figure 22 of datasheet,
> page 13). Is this correct? If yes we add a comment explaining this.
> 
> >  
> > > +
> > > +             if (val)
> > >                       st->gain = 1;
> > >               else
> > >                       st->gain = 128;  
> >
> > Do we still need this? I am not convinced.  
> No, I don't think so. Thanks for pointing this out :-)
> 
> >  
> > > @@ -141,18 +196,20 @@ static const struct ad_sigma_delta_info
> > > ad7780_sigma_delta_info = {
> > >       .has_registers = false,
> > >  };
> > >
> > > -#define AD7780_CHANNEL(bits, wordsize) \
> > > +#define AD7170_CHANNEL(bits, wordsize) \
> > >       AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> > > +#define AD7780_CHANNEL(bits, wordsize) \
> > > +     AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
> > >
> > >  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > >       [ID_AD7170] = {
> > > -             .channel = AD7780_CHANNEL(12, 24),
> > > +             .channel = AD7170_CHANNEL(12, 24),
> > >               .pattern = AD7170_PATTERN,
> > >               .pattern_mask = AD7170_PATTERN_MASK,
> > >               .is_ad778x = false,
> > >       },
> > >       [ID_AD7171] = {
> > > -             .channel = AD7780_CHANNEL(16, 24),
> > > +             .channel = AD7170_CHANNEL(16, 24),
> > >               .pattern = AD7170_PATTERN,
> > >               .pattern_mask = AD7170_PATTERN_MASK,
> > >               .is_ad778x = false,
> > > @@ -173,6 +230,7 @@ static const struct ad7780_chip_info
> > > ad7780_chip_info_tbl[] = {
> > >
> > >  static const struct iio_info ad7780_info = {
> > >       .read_raw = ad7780_read_raw,
> > > +     .write_raw = ad7780_write_raw,
> > >  };
> > >
> > >  static int ad7780_probe(struct spi_device *spi)
> > > @@ -222,6 +280,18 @@ static int ad7780_probe(struct spi_device *spi)
> > >               goto error_disable_reg;
> > >       }
> > >
> > > +     if (st->chip_info->is_ad778x) {
> > > +             st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
> > > +                                                     "gain",
> > > +                                                     GPIOD_OUT_HIGH);
> > > +             if (IS_ERR(st->gain_gpio)) {  
> >
> > if the GPIO is optional, then we should continue in case of -ENODEV.
> >
> > Shouldn't we have a devm_gpiod_get_optional() call also for filter_gpio?
I had to check this one...

 * This is equivalent to gpiod_get(), except that when no GPIO was assigned to
 * the requested function it will return NULL. This is convenient for drivers
 * that need to handle optional GPIOs.

So nope, it shouldn't return -ENODEV;  unlike the clock equivalent which
IIRC does...

> >  
> > > +                     ret = PTR_ERR(st->gain_gpio);
> > > +                     dev_err(&spi->dev, "Failed to request gain GPIO:
> > > %d\n",
> > > +                             ret);
> > > +                     goto error_disable_reg;
> > > +             }
> > > +     }
> > > +
> > >       ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> > >       if (ret)
> > >               goto error_disable_reg;
> > > diff --git a/include/linux/iio/adc/ad_sigma_delta.h
> > > b/include/linux/iio/adc/ad_sigma_delta.h
> > > index 730ead1a46df..6cadab6fd5fd 100644
> > > --- a/include/linux/iio/adc/ad_sigma_delta.h
> > > +++ b/include/linux/iio/adc/ad_sigma_delta.h
> > > @@ -173,6 +173,11 @@ int ad_sd_validate_trigger(struct iio_dev
> > > *indio_dev, struct iio_trigger *trig);
> > >       __AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
> > >               _storagebits, _shift, NULL, IIO_VOLTAGE, 0)
> > >
> > > +#define AD_SD_CHANNEL_GAIN_FILTER(_si, _channel, _address, _bits, \
> > > +     _storagebits, _shift) \
> > > +     __AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
> > > +             _storagebits, _shift, NULL, IIO_VOLTAGE,
> > > BIT(IIO_CHAN_INFO_RAW))
> > > +
> > >  #define AD_SD_TEMP_CHANNEL(_si, _address, _bits, _storagebits, _shift) \
> > >       __AD_SD_CHANNEL(_si, 0, -1, _address, _bits, \
> > >               _storagebits, _shift, NULL, IIO_TEMP, \  
> >
> > --
> > You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
> > To post to this group, send email to kernel-usp@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/1543490289.11186.22.camel%40analog.com.
> > For more options, visit https://groups.google.com/d/optout.  

Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support

[Renato Lui Geh]

Hi,

Sorry for the (extremely) late reply.

Comments inline.

Renato

On 12/01, Jonathan Cameron wrote:
>On Thu, 29 Nov 2018 11:02:46 -0200
>Giuliano Augusto Faulin Belinassi <giuliano.belinassi@usp.br> wrote:
>
>> Hi
>
>A few follow ups from me having read the result in patch 2.
>
>Jonathan
>>
>> On Thu, Nov 29, 2018 at 9:18 AM Popa, Stefan Serban
>> <StefanSerban.Popa@analog.com> wrote:
>> >
>> > On Mi, 2018-11-28 at 16:15 -0200, Giuliano Belinassi wrote:
>> > > Previously, the AD7780 driver only supported gpio for the 'powerdown'
>> > > pin. This commit adds suppport for the 'gain' and 'filter' pin.
>> > >
>> > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
>> > > ---
>> > > Changes in v2:
>> > >     - Now this patch is part of the patchset that aims to remove ad7780
>> > > out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2
>> > >     - Also, now it reads voltage and filter values from the userspace
>> > > instead of gpio pin states.
>> >
>> > Hello,
>> > Please see bellow.
>> >
>> > >
>> > >  drivers/staging/iio/adc/ad7780.c       | 78 ++++++++++++++++++++++++--
>> > >  include/linux/iio/adc/ad_sigma_delta.h |  5 ++
>> > >  2 files changed, 79 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/drivers/staging/iio/adc/ad7780.c
>> > > b/drivers/staging/iio/adc/ad7780.c
>> > > index c4a85789c2db..05979a79fda3 100644
>> > > --- a/drivers/staging/iio/adc/ad7780.c
>> > > +++ b/drivers/staging/iio/adc/ad7780.c
>> > > @@ -39,6 +39,12 @@
>> > >  #define AD7170_PATTERN               (AD7780_PAT0 | AD7170_PAT2)
>> > >  #define AD7170_PATTERN_MASK  (AD7780_PAT0 | AD7780_PAT1 |
>> > > AD7170_PAT2)
>> > >
>> > > +#define AD7780_GAIN_GPIO     0
>> > > +#define AD7780_FILTER_GPIO   1
>> > > +
>> > > +#define AD7780_GAIN_MIDPOINT 64
>> > > +#define AD7780_FILTER_MIDPOINT       13350
>> > > +
>> > >  struct ad7780_chip_info {
>> > >       struct iio_chan_spec    channel;
>> > >       unsigned int            pattern_mask;
>> > > @@ -50,6 +56,8 @@ struct ad7780_state {
>> > >       const struct ad7780_chip_info   *chip_info;
>> > >       struct regulator                *reg;
>> > >       struct gpio_desc                *powerdown_gpio;
>> > > +     struct gpio_desc                *gain_gpio;
>> > > +     struct gpio_desc                *filter_gpio;
>> > >       unsigned int    gain;
>> > >
>> > >       struct ad_sigma_delta sd;
>> > > @@ -115,18 +123,65 @@ static int ad7780_read_raw(struct iio_dev
>> > > *indio_dev,
>> > >       return -EINVAL;
>> > >  }
>> > >
>> > > +static int ad7780_write_raw(struct iio_dev *indio_dev,
>> > > +                         struct iio_chan_spec const *chan,
>> > > +                         int val,
>> > > +                         int val2,
>> > > +                         long m)
>> > > +{
>> > > +     struct ad7780_state *st = iio_priv(indio_dev);
>> > > +     const struct ad7780_chip_info *chip_info = st->chip_info;
>> > > +     int uvref, gain;
>> > > +     unsigned int full_scale;
>> > > +
>> > > +     if (!chip_info->is_ad778x)
>> > > +             return 0;
>> > > +
>> > > +     switch (m) {
>> > > +     case IIO_CHAN_INFO_SCALE:
>> > > +             if (val != 0)
>> > > +                     return -EINVAL;
>> > > +
>> > > +             uvref = regulator_get_voltage(st->reg);
>> >
>> > regulator_get_voltage() has already been called in the probe function and
>> > the result is stored in st->int_vref_mv.
>>
>> This was removed in commit  9eae69ddbc4717a0bd702eddac76c7848773bf71
>> because the value was not being updated. But I agree if the vref
>> voltage is not going to change at all after the initialization, then
>> this value should be kept in memory.

Why wouldn't the vref voltage not change after initialization? Shouldn't
we keep reading and updating this in read_raw?
>>
>> > My suggestion would be to use a local vref variable declared as unsigned
>> > int. It is my fault that I haven't explained correctly in the previous
>> > email, but you need to multiply vref_mv with 1000000LL in order to get the
>> > right precision: vref = st->int_vref_mv * 1000000LL. Afterwards you will be
>> > able to perform the divisions.

Shouldn't we read vref inside read_raw in order to get up-to-date
readings on voltage values? Or should we keep reading from a cached
value?
>>
>> Thanks for this info! :-)
>> Shouldn't we store this in uV (microVolts)? This will yield a more
>> accurate result after the multiplication.
>>
>> > > +
>> > > +             if (uvref < 0)
>> > > +                     return uvref;
>> > > +
>> > > +             full_scale = 1 << (chip_info->channel.scan_type.realbits
>> > > - 1);
>> > > +             gain = DIV_ROUND_CLOSEST(uvref, full_scale);
>> > > +             gain = DIV_ROUND_CLOSEST(gain, val2);
>> > > +
>> > > +             gpiod_set_value(st->gain_gpio, gain <
>> > > AD7780_GAIN_MIDPOINT ? 0 : 1);
>> >
>> > Once the gain is set, you can store it in st->gain variable.
>>
>> Yes, we forgot it.
>>
>> >
>> > > +     case IIO_CHAN_INFO_SAMP_FREQ:
>> > > +             if (val2 != 0)
>> > > +                     return -EINVAL;
>comment I raised in patch 2 about the odd preciseness of insisting
>no decimal places, but matching any value based on a threshold on the
>whole number part.

I see. I thought the filter input was in mHz. So should I compare
1000*val with AD7780_FILTER_MIDPOINT?
>
>I'd also expect to see read_raw support for this.
>
>> > > +
>> > > +             gpiod_set_value(st->filter_gpio, val <
>> > > AD7780_FILTER_MIDPOINT ? 0 : 1);
>> >
>> > This is probably fine, although I am not a big fan of the ternary operator.
>> > A simple if else statement would do. However, I don't feel strongly about
>> > it, so feel free to disagree.
>> >
>> > > +     break;
>> > > +     }
>> > > +
>> > > +     return 0;
>> > > +}
>> > > +
>> > >  static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>> > >                                    unsigned int raw_sample)
>> > >  {
>> > >       struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
>> > >       const struct ad7780_chip_info *chip_info = st->chip_info;
>> > > +     int val;
>> > >
>> > >       if ((raw_sample & AD7780_ERR) ||
>> > >           ((raw_sample & chip_info->pattern_mask) != chip_info-
>> > > >pattern))
>> > >               return -EIO;
>> > >
>> > >       if (chip_info->is_ad778x) {
>> > > -             if (raw_sample & AD7780_GAIN)
>> > > +             val = raw_sample & AD7780_GAIN;
>> > > +
>> > > +             if (val != gpiod_get_value(st->gain_gpio))
>> > > +                     return -EIO;
>> >
>> > It is not obvious to me what is the point of this check. Maybe you can add
>> > a comment?

If I'm not mistaken, we had agreed earlier to remove this altogether, as
having a redundancy check could potentially slow down reading.
>>
>> It seems to be a redundancy check. It is getting the 32-bits
>> raw_output, getting the bit that represents the GAIN value and
>> checking if the pin is set accordingly (see Figure 22 of datasheet,
>> page 13). Is this correct? If yes we add a comment explaining this.
>>
>> >
>> > > +
>> > > +             if (val)
>> > >                       st->gain = 1;
>> > >               else
>> > >                       st->gain = 128;
>> >
>> > Do we still need this? I am not convinced.
>> No, I don't think so. Thanks for pointing this out :-)
>>
>> >
>> > > @@ -141,18 +196,20 @@ static const struct ad_sigma_delta_info
>> > > ad7780_sigma_delta_info = {
>> > >       .has_registers = false,
>> > >  };
>> > >
>> > > -#define AD7780_CHANNEL(bits, wordsize) \
>> > > +#define AD7170_CHANNEL(bits, wordsize) \
>> > >       AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
>> > > +#define AD7780_CHANNEL(bits, wordsize) \
>> > > +     AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
>> > >
>> > >  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
>> > >       [ID_AD7170] = {
>> > > -             .channel = AD7780_CHANNEL(12, 24),
>> > > +             .channel = AD7170_CHANNEL(12, 24),
>> > >               .pattern = AD7170_PATTERN,
>> > >               .pattern_mask = AD7170_PATTERN_MASK,
>> > >               .is_ad778x = false,
>> > >       },
>> > >       [ID_AD7171] = {
>> > > -             .channel = AD7780_CHANNEL(16, 24),
>> > > +             .channel = AD7170_CHANNEL(16, 24),
>> > >               .pattern = AD7170_PATTERN,
>> > >               .pattern_mask = AD7170_PATTERN_MASK,
>> > >               .is_ad778x = false,
>> > > @@ -173,6 +230,7 @@ static const struct ad7780_chip_info
>> > > ad7780_chip_info_tbl[] = {
>> > >
>> > >  static const struct iio_info ad7780_info = {
>> > >       .read_raw = ad7780_read_raw,
>> > > +     .write_raw = ad7780_write_raw,
>> > >  };
>> > >
>> > >  static int ad7780_probe(struct spi_device *spi)
>> > > @@ -222,6 +280,18 @@ static int ad7780_probe(struct spi_device *spi)
>> > >               goto error_disable_reg;
>> > >       }
>> > >
>> > > +     if (st->chip_info->is_ad778x) {
>> > > +             st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
>> > > +                                                     "gain",
>> > > +                                                     GPIOD_OUT_HIGH);
>> > > +             if (IS_ERR(st->gain_gpio)) {
>> >
>> > if the GPIO is optional, then we should continue in case of -ENODEV.
>> >
>> > Shouldn't we have a devm_gpiod_get_optional() call also for filter_gpio?
>I had to check this one...
>
> * This is equivalent to gpiod_get(), except that when no GPIO was assigned to
> * the requested function it will return NULL. This is convenient for drivers
> * that need to handle optional GPIOs.
>
>So nope, it shouldn't return -ENODEV;  unlike the clock equivalent which
>IIRC does...
>
>> >
>> > > +                     ret = PTR_ERR(st->gain_gpio);
>> > > +                     dev_err(&spi->dev, "Failed to request gain GPIO:
>> > > %d\n",
>> > > +                             ret);
>> > > +                     goto error_disable_reg;
>> > > +             }
>> > > +     }
>> > > +
>> > >       ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>> > >       if (ret)
>> > >               goto error_disable_reg;
>> > > diff --git a/include/linux/iio/adc/ad_sigma_delta.h
>> > > b/include/linux/iio/adc/ad_sigma_delta.h
>> > > index 730ead1a46df..6cadab6fd5fd 100644
>> > > --- a/include/linux/iio/adc/ad_sigma_delta.h
>> > > +++ b/include/linux/iio/adc/ad_sigma_delta.h
>> > > @@ -173,6 +173,11 @@ int ad_sd_validate_trigger(struct iio_dev
>> > > *indio_dev, struct iio_trigger *trig);
>> > >       __AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
>> > >               _storagebits, _shift, NULL, IIO_VOLTAGE, 0)
>> > >
>> > > +#define AD_SD_CHANNEL_GAIN_FILTER(_si, _channel, _address, _bits, \
>> > > +     _storagebits, _shift) \
>> > > +     __AD_SD_CHANNEL(_si, _channel, -1, _address, _bits, \
>> > > +             _storagebits, _shift, NULL, IIO_VOLTAGE,
>> > > BIT(IIO_CHAN_INFO_RAW))
>> > > +
>> > >  #define AD_SD_TEMP_CHANNEL(_si, _address, _bits, _storagebits, _shift) \
>> > >       __AD_SD_CHANNEL(_si, 0, -1, _address, _bits, \
>> > >               _storagebits, _shift, NULL, IIO_TEMP, \
>> >
>> > --
>> > You received this message because you are subscribed to the Google Groups "Kernel USP" group.
>> > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@googlegroups.com.
>> > To post to this group, send email to kernel-usp@googlegroups.com.
>> > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/1543490289.11186.22.camel%40analog.com.
>> > For more options, visit https://groups.google.com/d/optout.
>

Re: [PATCH 1/2] staging: iio: ad7780: Add gain & filter gpio support

[Jonathan Cameron]

On Fri, 18 Jan 2019 18:19:40 -0200
Renato Lui Geh <renatogeh@gmail.com> wrote:

> Hi,
> 
> Sorry for the (extremely) late reply.
> 
> Comments inline.
> 
> Renato
> 
> On 12/01, Jonathan Cameron wrote:
> >On Thu, 29 Nov 2018 11:02:46 -0200
> >Giuliano Augusto Faulin Belinassi <giuliano.belinassi@usp.br> wrote:
> >  
> >> Hi  
> >
> >A few follow ups from me having read the result in patch 2.
> >
> >Jonathan  
> >>
> >> On Thu, Nov 29, 2018 at 9:18 AM Popa, Stefan Serban
> >> <StefanSerban.Popa@analog.com> wrote:  
> >> >
> >> > On Mi, 2018-11-28 at 16:15 -0200, Giuliano Belinassi wrote:  
> >> > > Previously, the AD7780 driver only supported gpio for the 'powerdown'
> >> > > pin. This commit adds suppport for the 'gain' and 'filter' pin.
> >> > >
> >> > > Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> >> > > ---
> >> > > Changes in v2:
> >> > >     - Now this patch is part of the patchset that aims to remove ad7780
> >> > > out of staging. https://marc.info/?l=linux-iio&m=154282349808890&w=2
> >> > >     - Also, now it reads voltage and filter values from the userspace
> >> > > instead of gpio pin states.  
> >> >
> >> > Hello,
> >> > Please see bellow.
> >> >  
> >> > >
> >> > >  drivers/staging/iio/adc/ad7780.c       | 78 ++++++++++++++++++++++++--
> >> > >  include/linux/iio/adc/ad_sigma_delta.h |  5 ++
> >> > >  2 files changed, 79 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> >> > > b/drivers/staging/iio/adc/ad7780.c
> >> > > index c4a85789c2db..05979a79fda3 100644
> >> > > --- a/drivers/staging/iio/adc/ad7780.c
> >> > > +++ b/drivers/staging/iio/adc/ad7780.c
> >> > > @@ -39,6 +39,12 @@
> >> > >  #define AD7170_PATTERN               (AD7780_PAT0 | AD7170_PAT2)
> >> > >  #define AD7170_PATTERN_MASK  (AD7780_PAT0 | AD7780_PAT1 |
> >> > > AD7170_PAT2)
> >> > >
> >> > > +#define AD7780_GAIN_GPIO     0
> >> > > +#define AD7780_FILTER_GPIO   1
> >> > > +
> >> > > +#define AD7780_GAIN_MIDPOINT 64
> >> > > +#define AD7780_FILTER_MIDPOINT       13350
> >> > > +
> >> > >  struct ad7780_chip_info {
> >> > >       struct iio_chan_spec    channel;
> >> > >       unsigned int            pattern_mask;
> >> > > @@ -50,6 +56,8 @@ struct ad7780_state {
> >> > >       const struct ad7780_chip_info   *chip_info;
> >> > >       struct regulator                *reg;
> >> > >       struct gpio_desc                *powerdown_gpio;
> >> > > +     struct gpio_desc                *gain_gpio;
> >> > > +     struct gpio_desc                *filter_gpio;
> >> > >       unsigned int    gain;
> >> > >
> >> > >       struct ad_sigma_delta sd;
> >> > > @@ -115,18 +123,65 @@ static int ad7780_read_raw(struct iio_dev
> >> > > *indio_dev,
> >> > >       return -EINVAL;
> >> > >  }
> >> > >
> >> > > +static int ad7780_write_raw(struct iio_dev *indio_dev,
> >> > > +                         struct iio_chan_spec const *chan,
> >> > > +                         int val,
> >> > > +                         int val2,
> >> > > +                         long m)
> >> > > +{
> >> > > +     struct ad7780_state *st = iio_priv(indio_dev);
> >> > > +     const struct ad7780_chip_info *chip_info = st->chip_info;
> >> > > +     int uvref, gain;
> >> > > +     unsigned int full_scale;
> >> > > +
> >> > > +     if (!chip_info->is_ad778x)
> >> > > +             return 0;
> >> > > +
> >> > > +     switch (m) {
> >> > > +     case IIO_CHAN_INFO_SCALE:
> >> > > +             if (val != 0)
> >> > > +                     return -EINVAL;
> >> > > +
> >> > > +             uvref = regulator_get_voltage(st->reg);  
> >> >
> >> > regulator_get_voltage() has already been called in the probe function and
> >> > the result is stored in st->int_vref_mv.  
> >>
> >> This was removed in commit  9eae69ddbc4717a0bd702eddac76c7848773bf71
> >> because the value was not being updated. But I agree if the vref
> >> voltage is not going to change at all after the initialization, then
> >> this value should be kept in memory.  
> 
> Why wouldn't the vref voltage not change after initialization? Shouldn't
> we keep reading and updating this in read_raw?

It may well change so bbest to check it.

> >>  
> >> > My suggestion would be to use a local vref variable declared as unsigned
> >> > int. It is my fault that I haven't explained correctly in the previous
> >> > email, but you need to multiply vref_mv with 1000000LL in order to get the
> >> > right precision: vref = st->int_vref_mv * 1000000LL. Afterwards you will be
> >> > able to perform the divisions.  
> 
> Shouldn't we read vref inside read_raw in order to get up-to-date
> readings on voltage values? Or should we keep reading from a cached
> value?

I agree.  We don't want to do it in a fast path as it 'probably' won't
change after the initial boot is done without some deliberate intervention
but fine to read it whenever we are dealing with anything other than
reading the ADC value (so reading the scale or similar).


> >>
> >> Thanks for this info! :-)
> >> Shouldn't we store this in uV (microVolts)? This will yield a more
> >> accurate result after the multiplication.
> >>  
> >> > > +
> >> > > +             if (uvref < 0)
> >> > > +                     return uvref;
> >> > > +
> >> > > +             full_scale = 1 << (chip_info->channel.scan_type.realbits
> >> > > - 1);
> >> > > +             gain = DIV_ROUND_CLOSEST(uvref, full_scale);
> >> > > +             gain = DIV_ROUND_CLOSEST(gain, val2);
> >> > > +
> >> > > +             gpiod_set_value(st->gain_gpio, gain <
> >> > > AD7780_GAIN_MIDPOINT ? 0 : 1);  
> >> >
> >> > Once the gain is set, you can store it in st->gain variable.  
> >>
> >> Yes, we forgot it.
> >>  
> >> >  
> >> > > +     case IIO_CHAN_INFO_SAMP_FREQ:
> >> > > +             if (val2 != 0)
> >> > > +                     return -EINVAL;  
> >comment I raised in patch 2 about the odd preciseness of insisting
> >no decimal places, but matching any value based on a threshold on the
> >whole number part.  
> 
> I see. I thought the filter input was in mHz. So should I compare
> 1000*val with AD7780_FILTER_MIDPOINT?

Not totally sure what I was going on about here..
I think I was just raising that you should use 1000*val + val2/1000 to do
the rounding with the decimal part taken into account.

> >
> >I'd also expect to see read_raw support for this.
> >  
> >> > > +
> >> > > +             gpiod_set_value(st->filter_gpio, val <
> >> > > AD7780_FILTER_MIDPOINT ? 0 : 1);  
> >> >
> >> > This is probably fine, although I am not a big fan of the ternary operator.
> >> > A simple if else statement would do. However, I don't feel strongly about
> >> > it, so feel free to disagree.
> >> >  
> >> > > +     break;
> >> > > +     }
> >> > > +
> >> > > +     return 0;
> >> > > +}
> >> > > +
> >> > >  static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> >> > >                                    unsigned int raw_sample)
> >> > >  {
> >> > >       struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
> >> > >       const struct ad7780_chip_info *chip_info = st->chip_info;
> >> > > +     int val;
> >> > >
> >> > >       if ((raw_sample & AD7780_ERR) ||
> >> > >           ((raw_sample & chip_info->pattern_mask) != chip_info-  
> >> > > >pattern))  
> >> > >               return -EIO;
> >> > >
> >> > >       if (chip_info->is_ad778x) {
> >> > > -             if (raw_sample & AD7780_GAIN)
> >> > > +             val = raw_sample & AD7780_GAIN;
> >> > > +
> >> > > +             if (val != gpiod_get_value(st->gain_gpio))
> >> > > +                     return -EIO;  
> >> >
> >> > It is not obvious to me what is the point of this check. Maybe you can add
> >> > a comment?  
> 
> If I'm not mistaken, we had agreed earlier to remove this altogether, as
> having a redundancy check could potentially slow down reading.

Works for me.

> >>
> >> It seems to be a redundancy check. It is getting the 32-bits
> >> raw_output, getting the bit that represents the GAIN value and
> >> checking if the pin is set accordingly (see Figure 22 of datasheet,
> >> page 13). Is this correct? If yes we add a comment explaining this.
> >>  
> >> >  
...

[PATCH 2/2] staging: iio: ad7780: Moving ad7780 out of staging

[Giuliano Belinassi]

Move ad7780 sigma-delta adc out of staging to the main tree

Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
 drivers/iio/adc/Kconfig          |  13 ++
 drivers/iio/adc/Makefile         |   1 +
 drivers/iio/adc/ad7780.c         | 347 +++++++++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig  |  13 --
 drivers/staging/iio/adc/Makefile |   1 -
 drivers/staging/iio/adc/ad7780.c | 347 -------------------------------
 6 files changed, 361 insertions(+), 361 deletions(-)
 create mode 100644 drivers/iio/adc/ad7780.c
 delete mode 100644 drivers/staging/iio/adc/ad7780.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 575bf69fea57..e517425e364d 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -92,6 +92,19 @@ config AD7793
 	  To compile this driver as a module, choose M here: the
 	  module will be called AD7793.
 
+config AD7780
+	tristate "Analog Devices AD7780 and similar ADCs driver"
+	depends on SPI
+	depends on GPIOLIB || COMPILE_TEST
+	select AD_SIGMA_DELTA
+	help
+	  Say yes here to build support for Analog Devices AD7170, AD7171,
+	  AD7780 and AD7781 SPI analog to digital converters (ADC).
+	  If unsure, say N (but it's safe to say "Y").
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7780.
+
 config AD7887
 	tristate "Analog Devices AD7887 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 91ba28aeb150..3301ca10b385 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_AD7298) += ad7298.o
 obj-$(CONFIG_AD7923) += ad7923.o
 obj-$(CONFIG_AD7476) += ad7476.o
 obj-$(CONFIG_AD7766) += ad7766.o
+obj-$(CONFIG_AD7780) += ad7780.o
 obj-$(CONFIG_AD7791) += ad7791.o
 obj-$(CONFIG_AD7793) += ad7793.o
 obj-$(CONFIG_AD7887) += ad7887.o
diff --git a/drivers/iio/adc/ad7780.c b/drivers/iio/adc/ad7780.c
new file mode 100644
index 000000000000..05979a79fda3
--- /dev/null
+++ b/drivers/iio/adc/ad7780.c
@@ -0,0 +1,347 @@
+/*
+ * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/spi/spi.h>
+#include <linux/regulator/consumer.h>
+#include <linux/err.h>
+#include <linux/sched.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/adc/ad_sigma_delta.h>
+
+#define AD7780_RDY		BIT(7)
+#define AD7780_FILTER		BIT(6)
+#define AD7780_ERR		BIT(5)
+#define AD7780_ID1		BIT(4)
+#define AD7780_ID0		BIT(3)
+#define AD7780_GAIN		BIT(2)
+#define AD7780_PAT1		BIT(1)
+#define AD7780_PAT0		BIT(0)
+
+#define AD7780_PATTERN		(AD7780_PAT0)
+#define AD7780_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1)
+
+#define AD7170_PAT2		BIT(2)
+
+#define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
+#define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
+
+#define AD7780_GAIN_GPIO	0
+#define AD7780_FILTER_GPIO	1
+
+#define AD7780_GAIN_MIDPOINT	64
+#define AD7780_FILTER_MIDPOINT	13350
+
+struct ad7780_chip_info {
+	struct iio_chan_spec	channel;
+	unsigned int		pattern_mask;
+	unsigned int		pattern;
+	bool			is_ad778x;
+};
+
+struct ad7780_state {
+	const struct ad7780_chip_info	*chip_info;
+	struct regulator		*reg;
+	struct gpio_desc		*powerdown_gpio;
+	struct gpio_desc		*gain_gpio;
+	struct gpio_desc		*filter_gpio;
+	unsigned int	gain;
+
+	struct ad_sigma_delta sd;
+};
+
+enum ad7780_supported_device_ids {
+	ID_AD7170,
+	ID_AD7171,
+	ID_AD7780,
+	ID_AD7781,
+};
+
+static struct ad7780_state *ad_sigma_delta_to_ad7780(struct ad_sigma_delta *sd)
+{
+	return container_of(sd, struct ad7780_state, sd);
+}
+
+static int ad7780_set_mode(struct ad_sigma_delta *sigma_delta,
+			   enum ad_sigma_delta_mode mode)
+{
+	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
+	unsigned int val;
+
+	switch (mode) {
+	case AD_SD_MODE_SINGLE:
+	case AD_SD_MODE_CONTINUOUS:
+		val = 1;
+		break;
+	default:
+		val = 0;
+		break;
+	}
+
+	gpiod_set_value(st->powerdown_gpio, val);
+
+	return 0;
+}
+
+static int ad7780_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long m)
+{
+	struct ad7780_state *st = iio_priv(indio_dev);
+	int voltage_uv;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
+	case IIO_CHAN_INFO_SCALE:
+		voltage_uv = regulator_get_voltage(st->reg);
+		if (voltage_uv < 0)
+			return voltage_uv;
+		*val = (voltage_uv / 1000) * st->gain;
+		*val2 = chan->scan_type.realbits - 1;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = -(1 << (chan->scan_type.realbits - 1));
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static int ad7780_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long m)
+{
+	struct ad7780_state *st = iio_priv(indio_dev);
+	const struct ad7780_chip_info *chip_info = st->chip_info;
+	int uvref, gain;
+	unsigned int full_scale;
+
+	if (!chip_info->is_ad778x)
+		return 0;
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val != 0)
+			return -EINVAL;
+
+		uvref = regulator_get_voltage(st->reg);
+
+		if (uvref < 0)
+			return uvref;
+
+		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
+		gain = DIV_ROUND_CLOSEST(uvref, full_scale);
+		gain = DIV_ROUND_CLOSEST(gain, val2);
+
+		gpiod_set_value(st->gain_gpio, gain < AD7780_GAIN_MIDPOINT ? 0 : 1);
+	break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (val2 != 0)
+			return -EINVAL;
+
+		gpiod_set_value(st->filter_gpio, val < AD7780_FILTER_MIDPOINT ? 0 : 1);
+	break;
+	}
+
+	return 0;
+}
+
+static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
+				     unsigned int raw_sample)
+{
+	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
+	const struct ad7780_chip_info *chip_info = st->chip_info;
+	int val;
+
+	if ((raw_sample & AD7780_ERR) ||
+	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
+		return -EIO;
+
+	if (chip_info->is_ad778x) {
+		val = raw_sample & AD7780_GAIN;
+
+		if (val != gpiod_get_value(st->gain_gpio))
+			return -EIO;
+
+		if (val)
+			st->gain = 1;
+		else
+			st->gain = 128;
+	}
+
+	return 0;
+}
+
+static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
+	.set_mode = ad7780_set_mode,
+	.postprocess_sample = ad7780_postprocess_sample,
+	.has_registers = false,
+};
+
+#define AD7170_CHANNEL(bits, wordsize) \
+	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
+#define AD7780_CHANNEL(bits, wordsize) \
+	AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
+
+static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
+	[ID_AD7170] = {
+		.channel = AD7170_CHANNEL(12, 24),
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7171] = {
+		.channel = AD7170_CHANNEL(16, 24),
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7780] = {
+		.channel = AD7780_CHANNEL(24, 32),
+		.pattern = AD7780_PATTERN,
+		.pattern_mask = AD7780_PATTERN_MASK,
+		.is_ad778x = true,
+	},
+	[ID_AD7781] = {
+		.channel = AD7780_CHANNEL(20, 32),
+		.pattern = AD7780_PATTERN,
+		.pattern_mask = AD7780_PATTERN_MASK,
+		.is_ad778x = true,
+	},
+};
+
+static const struct iio_info ad7780_info = {
+	.read_raw = ad7780_read_raw,
+	.write_raw = ad7780_write_raw,
+};
+
+static int ad7780_probe(struct spi_device *spi)
+{
+	struct ad7780_state *st;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->gain = 1;
+
+	ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
+
+	st->reg = devm_regulator_get(&spi->dev, "avdd");
+	if (IS_ERR(st->reg))
+		return PTR_ERR(st->reg);
+
+	ret = regulator_enable(st->reg);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
+		return ret;
+	}
+
+	st->chip_info =
+		&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
+
+	spi_set_drvdata(spi, indio_dev);
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = &st->chip_info->channel;
+	indio_dev->num_channels = 1;
+	indio_dev->info = &ad7780_info;
+
+	st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev,
+						     "powerdown",
+						     GPIOD_OUT_LOW);
+	if (IS_ERR(st->powerdown_gpio)) {
+		ret = PTR_ERR(st->powerdown_gpio);
+		dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
+			ret);
+		goto error_disable_reg;
+	}
+
+	if (st->chip_info->is_ad778x) {
+		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
+							"gain",
+							GPIOD_OUT_HIGH);
+		if (IS_ERR(st->gain_gpio)) {
+			ret = PTR_ERR(st->gain_gpio);
+			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
+				ret);
+			goto error_disable_reg;
+		}
+	}
+
+	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
+	if (ret)
+		goto error_disable_reg;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto error_cleanup_buffer_and_trigger;
+
+	return 0;
+
+error_cleanup_buffer_and_trigger:
+	ad_sd_cleanup_buffer_and_trigger(indio_dev);
+error_disable_reg:
+	regulator_disable(st->reg);
+
+	return ret;
+}
+
+static int ad7780_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct ad7780_state *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	ad_sd_cleanup_buffer_and_trigger(indio_dev);
+
+	regulator_disable(st->reg);
+
+	return 0;
+}
+
+static const struct spi_device_id ad7780_id[] = {
+	{"ad7170", ID_AD7170},
+	{"ad7171", ID_AD7171},
+	{"ad7780", ID_AD7780},
+	{"ad7781", ID_AD7781},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad7780_id);
+
+static struct spi_driver ad7780_driver = {
+	.driver = {
+		.name	= "ad7780",
+	},
+	.probe		= ad7780_probe,
+	.remove		= ad7780_remove,
+	.id_table	= ad7780_id,
+};
+module_spi_driver(ad7780_driver);
+
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index fc23059f1673..4cd0a13a588c 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -37,19 +37,6 @@ config AD7606_IFACE_SPI
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7606_spi.
 
-config AD7780
-	tristate "Analog Devices AD7780 and similar ADCs driver"
-	depends on SPI
-	depends on GPIOLIB || COMPILE_TEST
-	select AD_SIGMA_DELTA
-	help
-	  Say yes here to build support for Analog Devices AD7170, AD7171,
-	  AD7780 and AD7781 SPI analog to digital converters (ADC).
-	  If unsure, say N (but it's safe to say "Y").
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called ad7780.
-
 config AD7816
 	tristate "Analog Devices AD7816/7/8 temperature sensor and ADC driver"
 	depends on SPI
diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
index ebe83c1ad362..344002d87c78 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -7,7 +7,6 @@ obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
 obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
 obj-$(CONFIG_AD7606) += ad7606.o
 
-obj-$(CONFIG_AD7780) += ad7780.o
 obj-$(CONFIG_AD7816) += ad7816.o
 obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7280) += ad7280a.o
diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
deleted file mode 100644
index 05979a79fda3..000000000000
--- a/drivers/staging/iio/adc/ad7780.c
+++ /dev/null
@@ -1,347 +0,0 @@
-/*
- * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
- *
- * Copyright 2011 Analog Devices Inc.
- *
- * Licensed under the GPL-2.
- */
-
-#include <linux/interrupt.h>
-#include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/sysfs.h>
-#include <linux/spi/spi.h>
-#include <linux/regulator/consumer.h>
-#include <linux/err.h>
-#include <linux/sched.h>
-#include <linux/gpio/consumer.h>
-#include <linux/module.h>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-#include <linux/iio/adc/ad_sigma_delta.h>
-
-#define AD7780_RDY		BIT(7)
-#define AD7780_FILTER		BIT(6)
-#define AD7780_ERR		BIT(5)
-#define AD7780_ID1		BIT(4)
-#define AD7780_ID0		BIT(3)
-#define AD7780_GAIN		BIT(2)
-#define AD7780_PAT1		BIT(1)
-#define AD7780_PAT0		BIT(0)
-
-#define AD7780_PATTERN		(AD7780_PAT0)
-#define AD7780_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1)
-
-#define AD7170_PAT2		BIT(2)
-
-#define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
-#define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
-
-#define AD7780_GAIN_GPIO	0
-#define AD7780_FILTER_GPIO	1
-
-#define AD7780_GAIN_MIDPOINT	64
-#define AD7780_FILTER_MIDPOINT	13350
-
-struct ad7780_chip_info {
-	struct iio_chan_spec	channel;
-	unsigned int		pattern_mask;
-	unsigned int		pattern;
-	bool			is_ad778x;
-};
-
-struct ad7780_state {
-	const struct ad7780_chip_info	*chip_info;
-	struct regulator		*reg;
-	struct gpio_desc		*powerdown_gpio;
-	struct gpio_desc		*gain_gpio;
-	struct gpio_desc		*filter_gpio;
-	unsigned int	gain;
-
-	struct ad_sigma_delta sd;
-};
-
-enum ad7780_supported_device_ids {
-	ID_AD7170,
-	ID_AD7171,
-	ID_AD7780,
-	ID_AD7781,
-};
-
-static struct ad7780_state *ad_sigma_delta_to_ad7780(struct ad_sigma_delta *sd)
-{
-	return container_of(sd, struct ad7780_state, sd);
-}
-
-static int ad7780_set_mode(struct ad_sigma_delta *sigma_delta,
-			   enum ad_sigma_delta_mode mode)
-{
-	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
-	unsigned int val;
-
-	switch (mode) {
-	case AD_SD_MODE_SINGLE:
-	case AD_SD_MODE_CONTINUOUS:
-		val = 1;
-		break;
-	default:
-		val = 0;
-		break;
-	}
-
-	gpiod_set_value(st->powerdown_gpio, val);
-
-	return 0;
-}
-
-static int ad7780_read_raw(struct iio_dev *indio_dev,
-			   struct iio_chan_spec const *chan,
-			   int *val,
-			   int *val2,
-			   long m)
-{
-	struct ad7780_state *st = iio_priv(indio_dev);
-	int voltage_uv;
-
-	switch (m) {
-	case IIO_CHAN_INFO_RAW:
-		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
-	case IIO_CHAN_INFO_SCALE:
-		voltage_uv = regulator_get_voltage(st->reg);
-		if (voltage_uv < 0)
-			return voltage_uv;
-		*val = (voltage_uv / 1000) * st->gain;
-		*val2 = chan->scan_type.realbits - 1;
-		return IIO_VAL_FRACTIONAL_LOG2;
-	case IIO_CHAN_INFO_OFFSET:
-		*val = -(1 << (chan->scan_type.realbits - 1));
-		return IIO_VAL_INT;
-	}
-
-	return -EINVAL;
-}
-
-static int ad7780_write_raw(struct iio_dev *indio_dev,
-			    struct iio_chan_spec const *chan,
-			    int val,
-			    int val2,
-			    long m)
-{
-	struct ad7780_state *st = iio_priv(indio_dev);
-	const struct ad7780_chip_info *chip_info = st->chip_info;
-	int uvref, gain;
-	unsigned int full_scale;
-
-	if (!chip_info->is_ad778x)
-		return 0;
-
-	switch (m) {
-	case IIO_CHAN_INFO_SCALE:
-		if (val != 0)
-			return -EINVAL;
-
-		uvref = regulator_get_voltage(st->reg);
-
-		if (uvref < 0)
-			return uvref;
-
-		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
-		gain = DIV_ROUND_CLOSEST(uvref, full_scale);
-		gain = DIV_ROUND_CLOSEST(gain, val2);
-
-		gpiod_set_value(st->gain_gpio, gain < AD7780_GAIN_MIDPOINT ? 0 : 1);
-	break;
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		if (val2 != 0)
-			return -EINVAL;
-
-		gpiod_set_value(st->filter_gpio, val < AD7780_FILTER_MIDPOINT ? 0 : 1);
-	break;
-	}
-
-	return 0;
-}
-
-static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
-				     unsigned int raw_sample)
-{
-	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
-	const struct ad7780_chip_info *chip_info = st->chip_info;
-	int val;
-
-	if ((raw_sample & AD7780_ERR) ||
-	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
-		return -EIO;
-
-	if (chip_info->is_ad778x) {
-		val = raw_sample & AD7780_GAIN;
-
-		if (val != gpiod_get_value(st->gain_gpio))
-			return -EIO;
-
-		if (val)
-			st->gain = 1;
-		else
-			st->gain = 128;
-	}
-
-	return 0;
-}
-
-static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
-	.set_mode = ad7780_set_mode,
-	.postprocess_sample = ad7780_postprocess_sample,
-	.has_registers = false,
-};
-
-#define AD7170_CHANNEL(bits, wordsize) \
-	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
-#define AD7780_CHANNEL(bits, wordsize) \
-	AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
-
-static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
-	[ID_AD7170] = {
-		.channel = AD7170_CHANNEL(12, 24),
-		.pattern = AD7170_PATTERN,
-		.pattern_mask = AD7170_PATTERN_MASK,
-		.is_ad778x = false,
-	},
-	[ID_AD7171] = {
-		.channel = AD7170_CHANNEL(16, 24),
-		.pattern = AD7170_PATTERN,
-		.pattern_mask = AD7170_PATTERN_MASK,
-		.is_ad778x = false,
-	},
-	[ID_AD7780] = {
-		.channel = AD7780_CHANNEL(24, 32),
-		.pattern = AD7780_PATTERN,
-		.pattern_mask = AD7780_PATTERN_MASK,
-		.is_ad778x = true,
-	},
-	[ID_AD7781] = {
-		.channel = AD7780_CHANNEL(20, 32),
-		.pattern = AD7780_PATTERN,
-		.pattern_mask = AD7780_PATTERN_MASK,
-		.is_ad778x = true,
-	},
-};
-
-static const struct iio_info ad7780_info = {
-	.read_raw = ad7780_read_raw,
-	.write_raw = ad7780_write_raw,
-};
-
-static int ad7780_probe(struct spi_device *spi)
-{
-	struct ad7780_state *st;
-	struct iio_dev *indio_dev;
-	int ret;
-
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
-	if (!indio_dev)
-		return -ENOMEM;
-
-	st = iio_priv(indio_dev);
-	st->gain = 1;
-
-	ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
-
-	st->reg = devm_regulator_get(&spi->dev, "avdd");
-	if (IS_ERR(st->reg))
-		return PTR_ERR(st->reg);
-
-	ret = regulator_enable(st->reg);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
-		return ret;
-	}
-
-	st->chip_info =
-		&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
-
-	spi_set_drvdata(spi, indio_dev);
-
-	indio_dev->dev.parent = &spi->dev;
-	indio_dev->name = spi_get_device_id(spi)->name;
-	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = &st->chip_info->channel;
-	indio_dev->num_channels = 1;
-	indio_dev->info = &ad7780_info;
-
-	st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev,
-						     "powerdown",
-						     GPIOD_OUT_LOW);
-	if (IS_ERR(st->powerdown_gpio)) {
-		ret = PTR_ERR(st->powerdown_gpio);
-		dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
-			ret);
-		goto error_disable_reg;
-	}
-
-	if (st->chip_info->is_ad778x) {
-		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
-							"gain",
-							GPIOD_OUT_HIGH);
-		if (IS_ERR(st->gain_gpio)) {
-			ret = PTR_ERR(st->gain_gpio);
-			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
-				ret);
-			goto error_disable_reg;
-		}
-	}
-
-	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
-	if (ret)
-		goto error_disable_reg;
-
-	ret = iio_device_register(indio_dev);
-	if (ret)
-		goto error_cleanup_buffer_and_trigger;
-
-	return 0;
-
-error_cleanup_buffer_and_trigger:
-	ad_sd_cleanup_buffer_and_trigger(indio_dev);
-error_disable_reg:
-	regulator_disable(st->reg);
-
-	return ret;
-}
-
-static int ad7780_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct ad7780_state *st = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	ad_sd_cleanup_buffer_and_trigger(indio_dev);
-
-	regulator_disable(st->reg);
-
-	return 0;
-}
-
-static const struct spi_device_id ad7780_id[] = {
-	{"ad7170", ID_AD7170},
-	{"ad7171", ID_AD7171},
-	{"ad7780", ID_AD7780},
-	{"ad7781", ID_AD7781},
-	{}
-};
-MODULE_DEVICE_TABLE(spi, ad7780_id);
-
-static struct spi_driver ad7780_driver = {
-	.driver = {
-		.name	= "ad7780",
-	},
-	.probe		= ad7780_probe,
-	.remove		= ad7780_remove,
-	.id_table	= ad7780_id,
-};
-module_spi_driver(ad7780_driver);
-
-MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
-MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs");
-MODULE_LICENSE("GPL v2");
-- 
2.19.1

Re: [PATCH 2/2] staging: iio: ad7780: Moving ad7780 out of staging

[Jonathan Cameron]

On Wed, 28 Nov 2018 16:16:34 -0200
Giuliano Belinassi <giuliano.belinassi@gmail.com> wrote:

> Move ad7780 sigma-delta adc out of staging to the main tree
Please add a few details here on what the device is and what interfaces
are provided.  It's nice for anyone whose first encounter with this
driver is this patch (as they don't look at activity in staging).
> 
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>

Look pretty good.  A few small things inline once patch 1 is sorted.
I'd also see if you can get agreement for SPDX from one of the AD
reviewers (they are normally fine with this I think and can
go ask Michael if they want to be sure and Michael isn't following the
thread)

Note some of the comments are actually on patch 1 but visible here
when placed in the surrounding code.  I'll try to mention any in
that thread as well in a minute...


Jonathan

> ---
>  drivers/iio/adc/Kconfig          |  13 ++
>  drivers/iio/adc/Makefile         |   1 +
>  drivers/iio/adc/ad7780.c         | 347 +++++++++++++++++++++++++++++++
>  drivers/staging/iio/adc/Kconfig  |  13 --
>  drivers/staging/iio/adc/Makefile |   1 -
>  drivers/staging/iio/adc/ad7780.c | 347 -------------------------------
>  6 files changed, 361 insertions(+), 361 deletions(-)
>  create mode 100644 drivers/iio/adc/ad7780.c
>  delete mode 100644 drivers/staging/iio/adc/ad7780.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 575bf69fea57..e517425e364d 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -92,6 +92,19 @@ config AD7793
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called AD7793.
>  
> +config AD7780
> +	tristate "Analog Devices AD7780 and similar ADCs driver"
> +	depends on SPI
> +	depends on GPIOLIB || COMPILE_TEST
> +	select AD_SIGMA_DELTA
> +	help
> +	  Say yes here to build support for Analog Devices AD7170, AD7171,
> +	  AD7780 and AD7781 SPI analog to digital converters (ADC).
> +	  If unsure, say N (but it's safe to say "Y").
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad7780.
> +
>  config AD7887
>  	tristate "Analog Devices AD7887 ADC driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 91ba28aeb150..3301ca10b385 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_AD7298) += ad7298.o
>  obj-$(CONFIG_AD7923) += ad7923.o
>  obj-$(CONFIG_AD7476) += ad7476.o
>  obj-$(CONFIG_AD7766) += ad7766.o
> +obj-$(CONFIG_AD7780) += ad7780.o
>  obj-$(CONFIG_AD7791) += ad7791.o
>  obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7887) += ad7887.o
> diff --git a/drivers/iio/adc/ad7780.c b/drivers/iio/adc/ad7780.c
> new file mode 100644
> index 000000000000..05979a79fda3
> --- /dev/null
> +++ b/drivers/iio/adc/ad7780.c
> @@ -0,0 +1,347 @@
> +/*
> + * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.

Definitely time to look at moving to SPDX as various AD reviewers
looking at this anyway so should be fine to get their ack!

> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/err.h>
> +#include <linux/sched.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/adc/ad_sigma_delta.h>
> +
> +#define AD7780_RDY		BIT(7)
> +#define AD7780_FILTER		BIT(6)
> +#define AD7780_ERR		BIT(5)
> +#define AD7780_ID1		BIT(4)
> +#define AD7780_ID0		BIT(3)
> +#define AD7780_GAIN		BIT(2)
> +#define AD7780_PAT1		BIT(1)
> +#define AD7780_PAT0		BIT(0)
> +
> +#define AD7780_PATTERN		(AD7780_PAT0)
> +#define AD7780_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1)
> +
> +#define AD7170_PAT2		BIT(2)
> +
> +#define AD7170_PATTERN		(AD7780_PAT0 | AD7170_PAT2)
> +#define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> +
> +#define AD7780_GAIN_GPIO	0
> +#define AD7780_FILTER_GPIO	1
> +
> +#define AD7780_GAIN_MIDPOINT	64
> +#define AD7780_FILTER_MIDPOINT	13350
> +
> +struct ad7780_chip_info {
> +	struct iio_chan_spec	channel;
> +	unsigned int		pattern_mask;
> +	unsigned int		pattern;
> +	bool			is_ad778x;
> +};
> +
> +struct ad7780_state {
> +	const struct ad7780_chip_info	*chip_info;
> +	struct regulator		*reg;
> +	struct gpio_desc		*powerdown_gpio;
> +	struct gpio_desc		*gain_gpio;
> +	struct gpio_desc		*filter_gpio;
> +	unsigned int	gain;
> +
> +	struct ad_sigma_delta sd;
> +};
> +
> +enum ad7780_supported_device_ids {
> +	ID_AD7170,
> +	ID_AD7171,
> +	ID_AD7780,
> +	ID_AD7781,
> +};
> +
> +static struct ad7780_state *ad_sigma_delta_to_ad7780(struct ad_sigma_delta *sd)
> +{
> +	return container_of(sd, struct ad7780_state, sd);
> +}
> +
> +static int ad7780_set_mode(struct ad_sigma_delta *sigma_delta,
> +			   enum ad_sigma_delta_mode mode)
> +{
> +	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
> +	unsigned int val;
> +
> +	switch (mode) {
> +	case AD_SD_MODE_SINGLE:
> +	case AD_SD_MODE_CONTINUOUS:
> +		val = 1;
> +		break;
> +	default:
> +		val = 0;
> +		break;
> +	}
> +
> +	gpiod_set_value(st->powerdown_gpio, val);
> +
> +	return 0;
> +}
> +
> +static int ad7780_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long m)
> +{
> +	struct ad7780_state *st = iio_priv(indio_dev);
> +	int voltage_uv;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		voltage_uv = regulator_get_voltage(st->reg);
> +		if (voltage_uv < 0)
> +			return voltage_uv;
> +		*val = (voltage_uv / 1000) * st->gain;
> +		*val2 = chan->scan_type.realbits - 1;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -(1 << (chan->scan_type.realbits - 1));
> +		return IIO_VAL_INT;
> +	}

We should be able to read back the current sampling frequency as well
(from a cached value).

> +
> +	return -EINVAL;
> +}
> +
> +static int ad7780_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val,
> +			    int val2,
> +			    long m)
> +{
> +	struct ad7780_state *st = iio_priv(indio_dev);
> +	const struct ad7780_chip_info *chip_info = st->chip_info;
> +	int uvref, gain;
> +	unsigned int full_scale;
> +
> +	if (!chip_info->is_ad778x)
> +		return 0;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		uvref = regulator_get_voltage(st->reg);
> +
> +		if (uvref < 0)
> +			return uvref;
> +
> +		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
> +		gain = DIV_ROUND_CLOSEST(uvref, full_scale);
> +		gain = DIV_ROUND_CLOSEST(gain, val2);
> +
> +		gpiod_set_value(st->gain_gpio, gain < AD7780_GAIN_MIDPOINT ? 0 : 1);
> +	break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (val2 != 0)

It's a bit odd to do a combination of a precise match (nothing after decimal
point) but then use just a threshold for the integer part... 

If you are going to do block this then insist on a precise match for the
filter value below and provide an available attribute to give the two possible values.

> +			return -EINVAL;
> +
> +		gpiod_set_value(st->filter_gpio, val < AD7780_FILTER_MIDPOINT ? 0 : 1);
> +	break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> +				     unsigned int raw_sample)
> +{
> +	struct ad7780_state *st = ad_sigma_delta_to_ad7780(sigma_delta);
> +	const struct ad7780_chip_info *chip_info = st->chip_info;
> +	int val;
> +
> +	if ((raw_sample & AD7780_ERR) ||
> +	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> +		return -EIO;
> +
> +	if (chip_info->is_ad778x) {
> +		val = raw_sample & AD7780_GAIN;
> +
> +		if (val != gpiod_get_value(st->gain_gpio))
> +			return -EIO;
> +
> +		if (val)
> +			st->gain = 1;
> +		else
> +			st->gain = 128;

There are outstanding comments on this code in patch 1, I won't comment on
it here.

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
> +	.set_mode = ad7780_set_mode,
> +	.postprocess_sample = ad7780_postprocess_sample,
> +	.has_registers = false,
> +};
> +
> +#define AD7170_CHANNEL(bits, wordsize) \
> +	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> +#define AD7780_CHANNEL(bits, wordsize) \
> +	AD_SD_CHANNEL_GAIN_FILTER(1, 0, 0, bits, 32, wordsize - bits)
> +
> +static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> +	[ID_AD7170] = {
> +		.channel = AD7170_CHANNEL(12, 24),
> +		.pattern = AD7170_PATTERN,
> +		.pattern_mask = AD7170_PATTERN_MASK,
> +		.is_ad778x = false,
> +	},
> +	[ID_AD7171] = {
> +		.channel = AD7170_CHANNEL(16, 24),
> +		.pattern = AD7170_PATTERN,
> +		.pattern_mask = AD7170_PATTERN_MASK,
> +		.is_ad778x = false,
> +	},
> +	[ID_AD7780] = {
> +		.channel = AD7780_CHANNEL(24, 32),
> +		.pattern = AD7780_PATTERN,
> +		.pattern_mask = AD7780_PATTERN_MASK,
> +		.is_ad778x = true,
> +	},
> +	[ID_AD7781] = {
> +		.channel = AD7780_CHANNEL(20, 32),
> +		.pattern = AD7780_PATTERN,
> +		.pattern_mask = AD7780_PATTERN_MASK,
> +		.is_ad778x = true,
> +	},
> +};
> +
> +static const struct iio_info ad7780_info = {
> +	.read_raw = ad7780_read_raw,
> +	.write_raw = ad7780_write_raw,
> +};
> +
> +static int ad7780_probe(struct spi_device *spi)
> +{
> +	struct ad7780_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->gain = 1;
> +
> +	ad_sd_init(&st->sd, indio_dev, spi, &ad7780_sigma_delta_info);
> +
> +	st->reg = devm_regulator_get(&spi->dev, "avdd");
> +	if (IS_ERR(st->reg))
> +		return PTR_ERR(st->reg);
> +
> +	ret = regulator_enable(st->reg);

Hmm.  This is obviously not an actual bug, but from my 'obviously correct'
mental filter is tripping over the fact the remove ordering is not
the precise opposite of the probe order.  As devm managed cleanup occurs
after remove finishes, the two gpios below are freed after the regulator
disable, whereas for precise reverse order they would be before it.

The easiest 'fix' for this would be to just move this regulator enable
until after we have grabbed the gpios.  I don't think that makes any
real difference as we use neither of them in this function anyway.

Another option would be to use devm_add_action_or_reset to do
the unwinding of this regulator_enable so that it gets cleaned up
in the devm unwind rather than by hand.

> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
> +		return ret;
> +	}
> +
> +	st->chip_info =
> +		&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &st->chip_info->channel;
> +	indio_dev->num_channels = 1;
> +	indio_dev->info = &ad7780_info;
> +
> +	st->powerdown_gpio = devm_gpiod_get_optional(&spi->dev,
> +						     "powerdown",
> +						     GPIOD_OUT_LOW);
> +	if (IS_ERR(st->powerdown_gpio)) {
> +		ret = PTR_ERR(st->powerdown_gpio);
> +		dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
> +			ret);
> +		goto error_disable_reg;
> +	}
> +
> +	if (st->chip_info->is_ad778x) {
> +		st->gain_gpio = devm_gpiod_get_optional(&spi->dev,
> +							"gain",
> +							GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->gain_gpio)) {
> +			ret = PTR_ERR(st->gain_gpio);
> +			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
> +				ret);
> +			goto error_disable_reg;
> +		}
> +	}
> +
> +	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto error_cleanup_buffer_and_trigger;
> +
> +	return 0;
> +
> +error_cleanup_buffer_and_trigger:
> +	ad_sd_cleanup_buffer_and_trigger(indio_dev);
> +error_disable_reg:
> +	regulator_disable(st->reg);
> +
> +	return ret;
> +}
> +
> +static int ad7780_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct ad7780_state *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	ad_sd_cleanup_buffer_and_trigger(indio_dev);
> +
> +	regulator_disable(st->reg);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id ad7780_id[] = {
> +	{"ad7170", ID_AD7170},
> +	{"ad7171", ID_AD7171},
> +	{"ad7780", ID_AD7780},
> +	{"ad7781", ID_AD7781},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, ad7780_id);
> +
> +static struct spi_driver ad7780_driver = {
> +	.driver = {
> +		.name	= "ad7780",
> +	},
> +	.probe		= ad7780_probe,
> +	.remove		= ad7780_remove,
> +	.id_table	= ad7780_id,
> +};
> +module_spi_driver(ad7780_driver);
> +
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices AD7780 and similar ADCs");
> +MODULE_LICENSE("GPL v2");

<cut removed code>