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

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

[Renato Lui Geh]

This series of patches adds user input to ad7780 'gain' & 'filter' gpio
pins, moves regulator initialization to after gpio initialization to
maintain consistency between probe and remove, adds SPDX to the driver's
license, and moves the ad7780 to the mainline.

Renato Lui Geh (4):
  staging: iio: ad7780: add gain & filter gpio support
  staging: iio: ad7780: move regulator to after GPIO init
  staging: iio: ad7780: add SPDX identifier
  staging: iio: ad7780: moving ad7780 out of staging

Changelog:
*v3
	- SPDX and regulator init as patches
	- Renamed filter to odr and ad778x_filter to ad778x_odr_avail
	- Removed unnecessary regulator disabling
	- Removed unnecessary AD_SD_CHANNEL macro
	- Changed unsigned int to unsigned long long to avoid overflow

 drivers/iio/adc/Kconfig          |  13 ++
 drivers/iio/adc/Makefile         |   1 +
 drivers/iio/adc/ad7780.c         | 359 +++++++++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig  |  13 --
 drivers/staging/iio/adc/Makefile |   1 -
 drivers/staging/iio/adc/ad7780.c | 277 ------------------------
 6 files changed, 373 insertions(+), 291 deletions(-)
 create mode 100644 drivers/iio/adc/ad7780.c
 delete mode 100644 drivers/staging/iio/adc/ad7780.c

-- 
2.20.1

[PATCH v3 1/4] staging: iio: ad7780: add gain & filter gpio support

[Renato Lui Geh]

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

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
Changes in v3:
	- Renamed ad7780_chip_info's filter to odr
	- Renamed ad778x_filter to ad778x_odr_avail
	- Changed vref variable from unsigned int to unsigned long long to
	  avoid overflow
	- Removed unnecessary AD_SD_CHANNEL macro

 drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index c4a85789c2db..6e4357800d31 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -39,6 +39,15 @@
 #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
+
+static const unsigned int ad778x_gain[2]      = { 1, 128 };
+static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
+
 struct ad7780_chip_info {
 	struct iio_chan_spec	channel;
 	unsigned int		pattern_mask;
@@ -50,7 +59,11 @@ struct ad7780_state {
 	const struct ad7780_chip_info	*chip_info;
 	struct regulator		*reg;
 	struct gpio_desc		*powerdown_gpio;
-	unsigned int	gain;
+	struct gpio_desc		*gain_gpio;
+	struct gpio_desc		*filter_gpio;
+	unsigned int			gain;
+	unsigned int			odr;
+	unsigned int			int_vref_mv;
 
 	struct ad_sigma_delta sd;
 };
@@ -104,17 +117,65 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
 		voltage_uv = regulator_get_voltage(st->reg);
 		if (voltage_uv < 0)
 			return voltage_uv;
-		*val = (voltage_uv / 1000) * st->gain;
+		voltage_uv /= 1000;
+		*val = voltage_uv * st->gain;
 		*val2 = chan->scan_type.realbits - 1;
+		st->int_vref_mv = voltage_uv;
 		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
 		*val = -(1 << (chan->scan_type.realbits - 1));
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->odr;
+		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;
+	unsigned long long vref;
+	unsigned int full_scale, gain;
+
+	if (!chip_info->is_ad778x)
+		return 0;
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val != 0)
+			return -EINVAL;
+
+		vref = st->int_vref_mv * 1000000LL;
+		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
+		gain = DIV_ROUND_CLOSEST(vref, full_scale);
+		gain = DIV_ROUND_CLOSEST(gain, val2);
+		st->gain = gain;
+		if (gain < AD7780_GAIN_MIDPOINT)
+			gain = 0;
+		else
+			gain = 1;
+		gpiod_set_value(st->gain_gpio, gain);
+	break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
+			val = 0;
+		else
+			val = 1;
+		st->odr = ad778x_odr_avail[val];
+		gpiod_set_value(st->filter_gpio, val);
+	break;
+	}
+
+	return 0;
+}
+
 static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 				     unsigned int raw_sample)
 {
@@ -126,10 +187,8 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 		return -EIO;
 
 	if (chip_info->is_ad778x) {
-		if (raw_sample & AD7780_GAIN)
-			st->gain = 1;
-		else
-			st->gain = 128;
+		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
+		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
 	}
 
 	return 0;
@@ -173,6 +232,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 +282,29 @@ 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;
+		}
+
+		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
+							  "filter",
+							  GPIOD_OUT_HIGH);
+		if (IS_ERR(st->filter_gpio)) {
+			ret = PTR_ERR(st->filter_gpio);
+			dev_err(&spi->dev,
+				"Failed to request filter GPIO: %d\n",
+				ret);
+			goto error_disable_reg;
+		}
+	}
+
 	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
 	if (ret)
 		goto error_disable_reg;
-- 
2.20.1

Re: [PATCH v3 1/4] staging: iio: ad7780: add gain & filter gpio support

[Peter Meerwald-Stadler]

On Tue, 5 Feb 2019, Renato Lui Geh wrote:

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

comments below
 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> ---
> Changes in v3:
> 	- Renamed ad7780_chip_info's filter to odr
> 	- Renamed ad778x_filter to ad778x_odr_avail
> 	- Changed vref variable from unsigned int to unsigned long long to
> 	  avoid overflow
> 	- Removed unnecessary AD_SD_CHANNEL macro
> 
> drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--
> 1 file changed, 89 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c4a85789c2db..6e4357800d31 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -39,6 +39,15 @@
> #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
> +
> +static const unsigned int ad778x_gain[2]      = { 1, 128 };
> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
> +
> struct ad7780_chip_info {
> 	struct iio_chan_spec	channel;
> 	unsigned int		pattern_mask;
> @@ -50,7 +59,11 @@ struct ad7780_state {
> 	const struct ad7780_chip_info	*chip_info;
> 	struct regulator		*reg;
> 	struct gpio_desc		*powerdown_gpio;
> -	unsigned int	gain;
> +	struct gpio_desc		*gain_gpio;
> +	struct gpio_desc		*filter_gpio;
> +	unsigned int			gain;
> +	unsigned int			odr;
> +	unsigned int			int_vref_mv;
> 
> 	struct ad_sigma_delta sd;
> };
> @@ -104,17 +117,65 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> 		voltage_uv = regulator_get_voltage(st->reg);
> 		if (voltage_uv < 0)
> 			return voltage_uv;
> -		*val = (voltage_uv / 1000) * st->gain;
> +		voltage_uv /= 1000;
> +		*val = voltage_uv * st->gain;
> 		*val2 = chan->scan_type.realbits - 1;
> +		st->int_vref_mv = voltage_uv;
> 		return IIO_VAL_FRACTIONAL_LOG2;
> 	case IIO_CHAN_INFO_OFFSET:
> 		*val = -(1 << (chan->scan_type.realbits - 1));
> 		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:

was this missing before?
this is not covered by the patch description and seems unrelated
needs to be added to the channel spec?

> +		*val = st->odr;
> +		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;
> +	unsigned long long vref;
> +	unsigned int full_scale, gain;
> +
> +	if (!chip_info->is_ad778x)
> +		return 0;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		vref = st->int_vref_mv * 1000000LL;
> +		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
> +		gain = DIV_ROUND_CLOSEST(vref, full_scale);
> +		gain = DIV_ROUND_CLOSEST(gain, val2);
> +		st->gain = gain;
> +		if (gain < AD7780_GAIN_MIDPOINT)
> +			gain = 0;
> +		else
> +			gain = 1;
> +		gpiod_set_value(st->gain_gpio, gain);
> +	break;

indentation with the other, previous statements

> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)

space around operators missing

> +			val = 0;
> +		else
> +			val = 1;
> +		st->odr = ad778x_odr_avail[val];
> +		gpiod_set_value(st->filter_gpio, val);
> +	break;
> +	}
> +
> +	return 0;
> +}
> +
> static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> 				     unsigned int raw_sample)
> {
> @@ -126,10 +187,8 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
> 		return -EIO;
> 
> 	if (chip_info->is_ad778x) {
> -		if (raw_sample & AD7780_GAIN)
> -			st->gain = 1;
> -		else
> -			st->gain = 128;
> +		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
> +		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
> 	}
> 
> 	return 0;
> @@ -173,6 +232,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 +282,29 @@ 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;
> +		}
> +
> +		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> +							  "filter",
> +							  GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->filter_gpio)) {
> +			ret = PTR_ERR(st->filter_gpio);
> +			dev_err(&spi->dev,
> +				"Failed to request filter GPIO: %d\n",
> +				ret);
> +			goto error_disable_reg;
> +		}
> +	}
> +
> 	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> 	if (ret)
> 		goto error_disable_reg;
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Re: [PATCH v3 1/4] staging: iio: ad7780: add gain & filter gpio support

[Renato Lui Geh]

Hi Peter,

Thank you for the review! Comments inline.

Renato

On 02/05, Peter Meerwald-Stadler wrote:
>On Tue, 5 Feb 2019, Renato Lui Geh wrote:
>
>> Previously, the AD7780 driver only supported gpio for the 'powerdown'
>> pin. This commit adds suppport for the 'gain' and 'filter' pin.
>
>comments below
>
>> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
>> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
>> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
>> ---
>> Changes in v3:
>> 	- Renamed ad7780_chip_info's filter to odr
>> 	- Renamed ad778x_filter to ad778x_odr_avail
>> 	- Changed vref variable from unsigned int to unsigned long long to
>> 	  avoid overflow
>> 	- Removed unnecessary AD_SD_CHANNEL macro
>>
>> drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--
>> 1 file changed, 89 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/iio/adc/ad7780.c
>> b/drivers/staging/iio/adc/ad7780.c
>> index c4a85789c2db..6e4357800d31 100644
>> --- a/drivers/staging/iio/adc/ad7780.c
>> +++ b/drivers/staging/iio/adc/ad7780.c
>> @@ -39,6 +39,15 @@
>> #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
>> +
>> +static const unsigned int ad778x_gain[2]      = { 1, 128 };
>> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
>> +
>> struct ad7780_chip_info {
>> 	struct iio_chan_spec	channel;
>> 	unsigned int		pattern_mask;
>> @@ -50,7 +59,11 @@ struct ad7780_state {
>> 	const struct ad7780_chip_info	*chip_info;
>> 	struct regulator		*reg;
>> 	struct gpio_desc		*powerdown_gpio;
>> -	unsigned int	gain;
>> +	struct gpio_desc		*gain_gpio;
>> +	struct gpio_desc		*filter_gpio;
>> +	unsigned int			gain;
>> +	unsigned int			odr;
>> +	unsigned int			int_vref_mv;
>>
>> 	struct ad_sigma_delta sd;
>> };
>> @@ -104,17 +117,65 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>> 		voltage_uv = regulator_get_voltage(st->reg);
>> 		if (voltage_uv < 0)
>> 			return voltage_uv;
>> -		*val = (voltage_uv / 1000) * st->gain;
>> +		voltage_uv /= 1000;
>> +		*val = voltage_uv * st->gain;
>> 		*val2 = chan->scan_type.realbits - 1;
>> +		st->int_vref_mv = voltage_uv;
>> 		return IIO_VAL_FRACTIONAL_LOG2;
>> 	case IIO_CHAN_INFO_OFFSET:
>> 		*val = -(1 << (chan->scan_type.realbits - 1));
>> 		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>
>was this missing before?
>this is not covered by the patch description and seems unrelated
>needs to be added to the channel spec?

Yes, this is a new feature. Should I have this chunk sent as a separate
patch?

What is the channel spec? How would I add this to the channel spec?

>
>> +		*val = st->odr;
>> +		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;
>> +	unsigned long long vref;
>> +	unsigned int full_scale, gain;
>> +
>> +	if (!chip_info->is_ad778x)
>> +		return 0;
>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		if (val != 0)
>> +			return -EINVAL;
>> +
>> +		vref = st->int_vref_mv * 1000000LL;
>> +		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
>> +		gain = DIV_ROUND_CLOSEST(vref, full_scale);
>> +		gain = DIV_ROUND_CLOSEST(gain, val2);
>> +		st->gain = gain;
>> +		if (gain < AD7780_GAIN_MIDPOINT)
>> +			gain = 0;
>> +		else
>> +			gain = 1;
>> +		gpiod_set_value(st->gain_gpio, gain);
>> +	break;
>
>indentation with the other, previous statements
>
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
>
>space around operators missing
>
>> +			val = 0;
>> +		else
>> +			val = 1;
>> +		st->odr = ad778x_odr_avail[val];
>> +		gpiod_set_value(st->filter_gpio, val);
>> +	break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>> 				     unsigned int raw_sample)
>> {
>> @@ -126,10 +187,8 @@ static int ad7780_postprocess_sample(struct
>> ad_sigma_delta *sigma_delta,
>> 		return -EIO;
>>
>> 	if (chip_info->is_ad778x) {
>> -		if (raw_sample & AD7780_GAIN)
>> -			st->gain = 1;
>> -		else
>> -			st->gain = 128;
>> +		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
>> +		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
>> 	}
>>
>> 	return 0;
>> @@ -173,6 +232,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 +282,29 @@ 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;
>> +		}
>> +
>> +		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
>> +							  "filter",
>> +							  GPIOD_OUT_HIGH);
>> +		if (IS_ERR(st->filter_gpio)) {
>> +			ret = PTR_ERR(st->filter_gpio);
>> +			dev_err(&spi->dev,
>> +				"Failed to request filter GPIO: %d\n",
>> +				ret);
>> +			goto error_disable_reg;
>> +		}
>> +	}
>> +
>> 	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>> 	if (ret)
>> 		goto error_disable_reg;
>>
>
>-- 
>
>Peter Meerwald-Stadler
>Mobile: +43 664 24 44 418

Re: [PATCH v3 1/4] staging: iio: ad7780: add gain & filter gpio support

[Jonathan Cameron]

On Tue, 5 Feb 2019 15:13:00 -0200
Renato Lui Geh <renatogeh@gmail.com> 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: Renato Lui Geh <renatogeh@gmail.com>
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
Comments inline.

> ---
> Changes in v3:
> 	- Renamed ad7780_chip_info's filter to odr
> 	- Renamed ad778x_filter to ad778x_odr_avail
> 	- Changed vref variable from unsigned int to unsigned long long to
> 	  avoid overflow
> 	- Removed unnecessary AD_SD_CHANNEL macro
> 
>  drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--
>  1 file changed, 89 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
> index c4a85789c2db..6e4357800d31 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -39,6 +39,15 @@
>  #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
What are these for?

> +
> +#define AD7780_GAIN_MIDPOINT	64
> +#define AD7780_FILTER_MIDPOINT	13350
> +
> +static const unsigned int ad778x_gain[2]      = { 1, 128 };
> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
> +
>  struct ad7780_chip_info {
>  	struct iio_chan_spec	channel;
>  	unsigned int		pattern_mask;
> @@ -50,7 +59,11 @@ struct ad7780_state {
>  	const struct ad7780_chip_info	*chip_info;
>  	struct regulator		*reg;
>  	struct gpio_desc		*powerdown_gpio;
> -	unsigned int	gain;
> +	struct gpio_desc		*gain_gpio;
> +	struct gpio_desc		*filter_gpio;
> +	unsigned int			gain;
> +	unsigned int			odr;
> +	unsigned int			int_vref_mv;
>  
>  	struct ad_sigma_delta sd;
>  };
> @@ -104,17 +117,65 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>  		voltage_uv = regulator_get_voltage(st->reg);
>  		if (voltage_uv < 0)
>  			return voltage_uv;
> -		*val = (voltage_uv / 1000) * st->gain;
> +		voltage_uv /= 1000;
> +		*val = voltage_uv * st->gain;
>  		*val2 = chan->scan_type.realbits - 1;
> +		st->int_vref_mv = voltage_uv;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  	case IIO_CHAN_INFO_OFFSET:
>  		*val = -(1 << (chan->scan_type.realbits - 1));
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->odr;
> +		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;
> +	unsigned long long vref;
> +	unsigned int full_scale, gain;
> +
> +	if (!chip_info->is_ad778x)
> +		return 0;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		vref = st->int_vref_mv * 1000000LL;
> +		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
> +		gain = DIV_ROUND_CLOSEST(vref, full_scale);
> +		gain = DIV_ROUND_CLOSEST(gain, val2);
> +		st->gain = gain;
> +		if (gain < AD7780_GAIN_MIDPOINT)
> +			gain = 0;
> +		else
> +			gain = 1;
> +		gpiod_set_value(st->gain_gpio, gain);
> +	break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
> +			val = 0;
> +		else
> +			val = 1;
> +		st->odr = ad778x_odr_avail[val];
> +		gpiod_set_value(st->filter_gpio, val);
> +	break;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>  				     unsigned int raw_sample)
>  {
> @@ -126,10 +187,8 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>  		return -EIO;
>  
>  	if (chip_info->is_ad778x) {
> -		if (raw_sample & AD7780_GAIN)
> -			st->gain = 1;
> -		else
> -			st->gain = 128;
> +		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
> +		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
>  	}
>  
>  	return 0;
> @@ -173,6 +232,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 +282,29 @@ 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",

These are not particularly standard names (basically not "reset"),
so they should be vendor prefixed, so that people know to go
look at the device specific binding.

> +							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;
> +		}
> +
> +		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> +							  "filter",
> +							  GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->filter_gpio)) {
> +			ret = PTR_ERR(st->filter_gpio);
> +			dev_err(&spi->dev,
> +				"Failed to request filter GPIO: %d\n",
> +				ret);
> +			goto error_disable_reg;
> +		}
> +	}
> +
>  	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>  	if (ret)
>  		goto error_disable_reg;

Re: [PATCH v3 1/4] staging: iio: ad7780: add gain & filter gpio support

[Renato Lui Geh]

Hi Jonathan,

Thanks for the review. Comments inline.

Renato

On 02/09, Jonathan Cameron wrote:
>On Tue, 5 Feb 2019 15:13:00 -0200
>Renato Lui Geh <renatogeh@gmail.com> 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: Renato Lui Geh <renatogeh@gmail.com>
>> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
>> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
>Comments inline.
>
>> ---
>> Changes in v3:
>> 	- Renamed ad7780_chip_info's filter to odr
>> 	- Renamed ad778x_filter to ad778x_odr_avail
>> 	- Changed vref variable from unsigned int to unsigned long long to
>> 	  avoid overflow
>> 	- Removed unnecessary AD_SD_CHANNEL macro
>>
>>  drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--
>>  1 file changed, 89 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
>> index c4a85789c2db..6e4357800d31 100644
>> --- a/drivers/staging/iio/adc/ad7780.c
>> +++ b/drivers/staging/iio/adc/ad7780.c
>> @@ -39,6 +39,15 @@
>>  #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
>What are these for?

Sorry about that. That's leftover from a previous attempt.
>
>> +
>> +#define AD7780_GAIN_MIDPOINT	64
>> +#define AD7780_FILTER_MIDPOINT	13350
>> +
>> +static const unsigned int ad778x_gain[2]      = { 1, 128 };
>> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
>> +
>>  struct ad7780_chip_info {
>>  	struct iio_chan_spec	channel;
>>  	unsigned int		pattern_mask;
>> @@ -50,7 +59,11 @@ struct ad7780_state {
>>  	const struct ad7780_chip_info	*chip_info;
>>  	struct regulator		*reg;
>>  	struct gpio_desc		*powerdown_gpio;
>> -	unsigned int	gain;
>> +	struct gpio_desc		*gain_gpio;
>> +	struct gpio_desc		*filter_gpio;
>> +	unsigned int			gain;
>> +	unsigned int			odr;
>> +	unsigned int			int_vref_mv;
>>
>>  	struct ad_sigma_delta sd;
>>  };
>> @@ -104,17 +117,65 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>>  		voltage_uv = regulator_get_voltage(st->reg);
>>  		if (voltage_uv < 0)
>>  			return voltage_uv;
>> -		*val = (voltage_uv / 1000) * st->gain;
>> +		voltage_uv /= 1000;
>> +		*val = voltage_uv * st->gain;
>>  		*val2 = chan->scan_type.realbits - 1;
>> +		st->int_vref_mv = voltage_uv;
>>  		return IIO_VAL_FRACTIONAL_LOG2;
>>  	case IIO_CHAN_INFO_OFFSET:
>>  		*val = -(1 << (chan->scan_type.realbits - 1));
>>  		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*val = st->odr;
>> +		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;
>> +	unsigned long long vref;
>> +	unsigned int full_scale, gain;
>> +
>> +	if (!chip_info->is_ad778x)
>> +		return 0;
>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		if (val != 0)
>> +			return -EINVAL;
>> +
>> +		vref = st->int_vref_mv * 1000000LL;
>> +		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
>> +		gain = DIV_ROUND_CLOSEST(vref, full_scale);
>> +		gain = DIV_ROUND_CLOSEST(gain, val2);
>> +		st->gain = gain;
>> +		if (gain < AD7780_GAIN_MIDPOINT)
>> +			gain = 0;
>> +		else
>> +			gain = 1;
>> +		gpiod_set_value(st->gain_gpio, gain);
>> +	break;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
>> +			val = 0;
>> +		else
>> +			val = 1;
>> +		st->odr = ad778x_odr_avail[val];
>> +		gpiod_set_value(st->filter_gpio, val);
>> +	break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>>  				     unsigned int raw_sample)
>>  {
>> @@ -126,10 +187,8 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
>>  		return -EIO;
>>
>>  	if (chip_info->is_ad778x) {
>> -		if (raw_sample & AD7780_GAIN)
>> -			st->gain = 1;
>> -		else
>> -			st->gain = 128;
>> +		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
>> +		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
>>  	}
>>
>>  	return 0;
>> @@ -173,6 +232,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 +282,29 @@ 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",
>
>These are not particularly standard names (basically not "reset"),
>so they should be vendor prefixed, so that people know to go
>look at the device specific binding.

I see. Should they be something like "adi,gain" and "adi,filter"? Am I
correct to assume that I'll have to somehow mention these in the
dt-binding?
>
>> +							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;
>> +		}
>> +
>> +		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
>> +							  "filter",
>> +							  GPIOD_OUT_HIGH);
>> +		if (IS_ERR(st->filter_gpio)) {
>> +			ret = PTR_ERR(st->filter_gpio);
>> +			dev_err(&spi->dev,
>> +				"Failed to request filter GPIO: %d\n",
>> +				ret);
>> +			goto error_disable_reg;
>> +		}
>> +	}
>> +
>>  	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>>  	if (ret)
>>  		goto error_disable_reg;
>

Re: [PATCH v3 1/4] staging: iio: ad7780: add gain & filter gpio support

[Jonathan Cameron]

On Thu, 14 Feb 2019 18:31:12 -0200
Renato Lui Geh <renatogeh@gmail.com> wrote:

> Hi Jonathan,
> 
> Thanks for the review. Comments inline.
> 
> Renato
> 
> On 02/09, Jonathan Cameron wrote:
> >On Tue, 5 Feb 2019 15:13:00 -0200
> >Renato Lui Geh <renatogeh@gmail.com> 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: Renato Lui Geh <renatogeh@gmail.com>
> >> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> >> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>  
> >Comments inline.
> >  
> >> ---
> >> Changes in v3:
> >> 	- Renamed ad7780_chip_info's filter to odr
> >> 	- Renamed ad778x_filter to ad778x_odr_avail
> >> 	- Changed vref variable from unsigned int to unsigned long long to
> >> 	  avoid overflow
> >> 	- Removed unnecessary AD_SD_CHANNEL macro
> >>
> >>  drivers/staging/iio/adc/ad7780.c | 95 ++++++++++++++++++++++++++++++--
> >>  1 file changed, 89 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
> >> index c4a85789c2db..6e4357800d31 100644
> >> --- a/drivers/staging/iio/adc/ad7780.c
> >> +++ b/drivers/staging/iio/adc/ad7780.c
> >> @@ -39,6 +39,15 @@
> >>  #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  
> >What are these for?  
> 
> Sorry about that. That's leftover from a previous attempt.
> >  
> >> +
> >> +#define AD7780_GAIN_MIDPOINT	64
> >> +#define AD7780_FILTER_MIDPOINT	13350
> >> +
> >> +static const unsigned int ad778x_gain[2]      = { 1, 128 };
> >> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
> >> +
> >>  struct ad7780_chip_info {
> >>  	struct iio_chan_spec	channel;
> >>  	unsigned int		pattern_mask;
> >> @@ -50,7 +59,11 @@ struct ad7780_state {
> >>  	const struct ad7780_chip_info	*chip_info;
> >>  	struct regulator		*reg;
> >>  	struct gpio_desc		*powerdown_gpio;
> >> -	unsigned int	gain;
> >> +	struct gpio_desc		*gain_gpio;
> >> +	struct gpio_desc		*filter_gpio;
> >> +	unsigned int			gain;
> >> +	unsigned int			odr;
> >> +	unsigned int			int_vref_mv;
> >>
> >>  	struct ad_sigma_delta sd;
> >>  };
> >> @@ -104,17 +117,65 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> >>  		voltage_uv = regulator_get_voltage(st->reg);
> >>  		if (voltage_uv < 0)
> >>  			return voltage_uv;
> >> -		*val = (voltage_uv / 1000) * st->gain;
> >> +		voltage_uv /= 1000;
> >> +		*val = voltage_uv * st->gain;
> >>  		*val2 = chan->scan_type.realbits - 1;
> >> +		st->int_vref_mv = voltage_uv;
> >>  		return IIO_VAL_FRACTIONAL_LOG2;
> >>  	case IIO_CHAN_INFO_OFFSET:
> >>  		*val = -(1 << (chan->scan_type.realbits - 1));
> >>  		return IIO_VAL_INT;
> >> +	case IIO_CHAN_INFO_SAMP_FREQ:
> >> +		*val = st->odr;
> >> +		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;
> >> +	unsigned long long vref;
> >> +	unsigned int full_scale, gain;
> >> +
> >> +	if (!chip_info->is_ad778x)
> >> +		return 0;
> >> +
> >> +	switch (m) {
> >> +	case IIO_CHAN_INFO_SCALE:
> >> +		if (val != 0)
> >> +			return -EINVAL;
> >> +
> >> +		vref = st->int_vref_mv * 1000000LL;
> >> +		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
> >> +		gain = DIV_ROUND_CLOSEST(vref, full_scale);
> >> +		gain = DIV_ROUND_CLOSEST(gain, val2);
> >> +		st->gain = gain;
> >> +		if (gain < AD7780_GAIN_MIDPOINT)
> >> +			gain = 0;
> >> +		else
> >> +			gain = 1;
> >> +		gpiod_set_value(st->gain_gpio, gain);
> >> +	break;
> >> +	case IIO_CHAN_INFO_SAMP_FREQ:
> >> +		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
> >> +			val = 0;
> >> +		else
> >> +			val = 1;
> >> +		st->odr = ad778x_odr_avail[val];
> >> +		gpiod_set_value(st->filter_gpio, val);
> >> +	break;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> >>  				     unsigned int raw_sample)
> >>  {
> >> @@ -126,10 +187,8 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
> >>  		return -EIO;
> >>
> >>  	if (chip_info->is_ad778x) {
> >> -		if (raw_sample & AD7780_GAIN)
> >> -			st->gain = 1;
> >> -		else
> >> -			st->gain = 128;
> >> +		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
> >> +		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
> >>  	}
> >>
> >>  	return 0;
> >> @@ -173,6 +232,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 +282,29 @@ 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",  
> >
> >These are not particularly standard names (basically not "reset"),
> >so they should be vendor prefixed, so that people know to go
> >look at the device specific binding.  
> 
> I see. Should they be something like "adi,gain" and "adi,filter"? Am I
> correct to assume that I'll have to somehow mention these in the
> dt-binding?
yes and yes - name is just adi,gain-gpios rather than gain-gpios.

Take a look at the other drivers doing the same thing.  We used to
be more lax on this so there are drivers without the prefixes, but
can't fix them now.

Jonathan

> >  
> >> +							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;
> >> +		}
> >> +
> >> +		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> >> +							  "filter",
> >> +							  GPIOD_OUT_HIGH);
> >> +		if (IS_ERR(st->filter_gpio)) {
> >> +			ret = PTR_ERR(st->filter_gpio);
> >> +			dev_err(&spi->dev,
> >> +				"Failed to request filter GPIO: %d\n",
> >> +				ret);
> >> +			goto error_disable_reg;
> >> +		}
> >> +	}
> >> +
> >>  	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> >>  	if (ret)
> >>  		goto error_disable_reg;  
> >  

[PATCH v3 2/4] staging: iio: ad7780: move regulator to after GPIO init

[Renato Lui Geh]

To maintain consistency between ad7780_probe and ad7780_remove orders,
regulator initialization has been moved to after GPIO initializations.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
 drivers/staging/iio/adc/ad7780.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 6e4357800d31..7804cd2b273e 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -250,16 +250,6 @@ static int ad7780_probe(struct spi_device *spi)
 
 	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];
 
@@ -279,7 +269,7 @@ static int ad7780_probe(struct spi_device *spi)
 		ret = PTR_ERR(st->powerdown_gpio);
 		dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
 			ret);
-		goto error_disable_reg;
+		return ret;
 	}
 
 	if (st->chip_info->is_ad778x) {
@@ -290,7 +280,7 @@ static int ad7780_probe(struct spi_device *spi)
 			ret = PTR_ERR(st->gain_gpio);
 			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
 				ret);
-			goto error_disable_reg;
+			return ret;
 		}
 
 		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
@@ -301,10 +291,20 @@ static int ad7780_probe(struct spi_device *spi)
 			dev_err(&spi->dev,
 				"Failed to request filter GPIO: %d\n",
 				ret);
-			goto error_disable_reg;
+			return ret;
 		}
 	}
 
+	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;
+	}
+
 	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
 	if (ret)
 		goto error_disable_reg;
-- 
2.20.1

Re: [PATCH v3 2/4] staging: iio: ad7780: move regulator to after GPIO init

[Jonathan Cameron]

On Tue, 5 Feb 2019 15:13:21 -0200
Renato Lui Geh <renatogeh@gmail.com> wrote:

> To maintain consistency between ad7780_probe and ad7780_remove orders,
> regulator initialization has been moved to after GPIO initializations.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
This looks fine, will pick up with the earlier patches when ready.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/adc/ad7780.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
> index 6e4357800d31..7804cd2b273e 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -250,16 +250,6 @@ static int ad7780_probe(struct spi_device *spi)
>  
>  	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];
>  
> @@ -279,7 +269,7 @@ static int ad7780_probe(struct spi_device *spi)
>  		ret = PTR_ERR(st->powerdown_gpio);
>  		dev_err(&spi->dev, "Failed to request powerdown GPIO: %d\n",
>  			ret);
> -		goto error_disable_reg;
> +		return ret;
>  	}
>  
>  	if (st->chip_info->is_ad778x) {
> @@ -290,7 +280,7 @@ static int ad7780_probe(struct spi_device *spi)
>  			ret = PTR_ERR(st->gain_gpio);
>  			dev_err(&spi->dev, "Failed to request gain GPIO: %d\n",
>  				ret);
> -			goto error_disable_reg;
> +			return ret;
>  		}
>  
>  		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> @@ -301,10 +291,20 @@ static int ad7780_probe(struct spi_device *spi)
>  			dev_err(&spi->dev,
>  				"Failed to request filter GPIO: %d\n",
>  				ret);
> -			goto error_disable_reg;
> +			return ret;
>  		}
>  	}
>  
> +	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;
> +	}
> +
>  	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>  	if (ret)
>  		goto error_disable_reg;

[PATCH v3 3/4] staging: iio: ad7780: add SPDX identifier

[Renato Lui Geh]

Add SPDX identifier (GPL-2.0) to the AD7780 driver.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
---
 drivers/staging/iio/adc/ad7780.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 7804cd2b273e..163e3c983598 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -1,9 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
  *
  * Copyright 2011 Analog Devices Inc.
- *
- * Licensed under the GPL-2.
  */
 
 #include <linux/interrupt.h>
-- 
2.20.1

[PATCH v3 4/4] staging: iio: ad7780: moving ad7780 out of staging

[Renato Lui Geh]

Move ad7780 ADC driver out of staging and into the mainline.

The ad7780 is a sigma-delta analog to digital converter. This driver provides
reading voltage values and status bits from both the ad778x and ad717x series.
Its interface also allows writing on the FILTER and GAIN GPIO pins on the
ad778x.

Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
Changes in v3:
	- Changes unrelated to moving the driver to main tree were resent as
	  individual patches

 drivers/iio/adc/Kconfig          |  13 ++
 drivers/iio/adc/Makefile         |   1 +
 drivers/iio/adc/ad7780.c         | 359 +++++++++++++++++++++++++++++++
 drivers/staging/iio/adc/Kconfig  |  13 --
 drivers/staging/iio/adc/Makefile |   1 -
 drivers/staging/iio/adc/ad7780.c | 359 -------------------------------
 6 files changed, 373 insertions(+), 373 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 f3cc7a31bce5..2cdee166d0e9 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -108,6 +108,19 @@ config AD7766
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad7766.
 
+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 AD7791
 	tristate "Analog Devices AD7791 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ea5031348052..b48852157115 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
 obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
 obj-$(CONFIG_AD7606) += ad7606.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..163e3c983598
--- /dev/null
+++ b/drivers/iio/adc/ad7780.c
@@ -0,0 +1,359 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc.
+ */
+
+#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
+
+static const unsigned int ad778x_gain[2]      = { 1, 128 };
+static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
+
+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;
+	unsigned int			odr;
+	unsigned int			int_vref_mv;
+
+	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;
+		voltage_uv /= 1000;
+		*val = voltage_uv * st->gain;
+		*val2 = chan->scan_type.realbits - 1;
+		st->int_vref_mv = voltage_uv;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = -(1 << (chan->scan_type.realbits - 1));
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->odr;
+		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;
+	unsigned long long vref;
+	unsigned int full_scale, gain;
+
+	if (!chip_info->is_ad778x)
+		return 0;
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val != 0)
+			return -EINVAL;
+
+		vref = st->int_vref_mv * 1000000LL;
+		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
+		gain = DIV_ROUND_CLOSEST(vref, full_scale);
+		gain = DIV_ROUND_CLOSEST(gain, val2);
+		st->gain = gain;
+		if (gain < AD7780_GAIN_MIDPOINT)
+			gain = 0;
+		else
+			gain = 1;
+		gpiod_set_value(st->gain_gpio, gain);
+	break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
+			val = 0;
+		else
+			val = 1;
+		st->odr = ad778x_odr_avail[val];
+		gpiod_set_value(st->filter_gpio, val);
+	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;
+
+	if ((raw_sample & AD7780_ERR) ||
+	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
+		return -EIO;
+
+	if (chip_info->is_ad778x) {
+		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
+		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
+	}
+
+	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 AD7780_CHANNEL(bits, wordsize) \
+	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
+
+static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
+	[ID_AD7170] = {
+		.channel = AD7780_CHANNEL(12, 24),
+		.pattern = AD7170_PATTERN,
+		.pattern_mask = AD7170_PATTERN_MASK,
+		.is_ad778x = false,
+	},
+	[ID_AD7171] = {
+		.channel = AD7780_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->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);
+		return ret;
+	}
+
+	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);
+			return ret;
+		}
+
+		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
+							  "filter",
+							  GPIOD_OUT_HIGH);
+		if (IS_ERR(st->filter_gpio)) {
+			ret = PTR_ERR(st->filter_gpio);
+			dev_err(&spi->dev,
+				"Failed to request filter GPIO: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	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;
+	}
+
+	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 7a93d3a5c113..404a53c743a6 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -3,19 +3,6 @@
 #
 menu "Analog to digital converters"
 
-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 7a421088ff82..4b76769b32bc 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -3,7 +3,6 @@
 # Makefile for industrial I/O ADC drivers
 #
 
-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 163e3c983598..000000000000
--- a/drivers/staging/iio/adc/ad7780.c
+++ /dev/null
@@ -1,359 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
- *
- * Copyright 2011 Analog Devices Inc.
- */
-
-#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
-
-static const unsigned int ad778x_gain[2]      = { 1, 128 };
-static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
-
-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;
-	unsigned int			odr;
-	unsigned int			int_vref_mv;
-
-	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;
-		voltage_uv /= 1000;
-		*val = voltage_uv * st->gain;
-		*val2 = chan->scan_type.realbits - 1;
-		st->int_vref_mv = voltage_uv;
-		return IIO_VAL_FRACTIONAL_LOG2;
-	case IIO_CHAN_INFO_OFFSET:
-		*val = -(1 << (chan->scan_type.realbits - 1));
-		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		*val = st->odr;
-		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;
-	unsigned long long vref;
-	unsigned int full_scale, gain;
-
-	if (!chip_info->is_ad778x)
-		return 0;
-
-	switch (m) {
-	case IIO_CHAN_INFO_SCALE:
-		if (val != 0)
-			return -EINVAL;
-
-		vref = st->int_vref_mv * 1000000LL;
-		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
-		gain = DIV_ROUND_CLOSEST(vref, full_scale);
-		gain = DIV_ROUND_CLOSEST(gain, val2);
-		st->gain = gain;
-		if (gain < AD7780_GAIN_MIDPOINT)
-			gain = 0;
-		else
-			gain = 1;
-		gpiod_set_value(st->gain_gpio, gain);
-	break;
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
-			val = 0;
-		else
-			val = 1;
-		st->odr = ad778x_odr_avail[val];
-		gpiod_set_value(st->filter_gpio, val);
-	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;
-
-	if ((raw_sample & AD7780_ERR) ||
-	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
-		return -EIO;
-
-	if (chip_info->is_ad778x) {
-		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
-		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
-	}
-
-	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 AD7780_CHANNEL(bits, wordsize) \
-	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
-
-static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
-	[ID_AD7170] = {
-		.channel = AD7780_CHANNEL(12, 24),
-		.pattern = AD7170_PATTERN,
-		.pattern_mask = AD7170_PATTERN_MASK,
-		.is_ad778x = false,
-	},
-	[ID_AD7171] = {
-		.channel = AD7780_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->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);
-		return ret;
-	}
-
-	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);
-			return ret;
-		}
-
-		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
-							  "filter",
-							  GPIOD_OUT_HIGH);
-		if (IS_ERR(st->filter_gpio)) {
-			ret = PTR_ERR(st->filter_gpio);
-			dev_err(&spi->dev,
-				"Failed to request filter GPIO: %d\n",
-				ret);
-			return ret;
-		}
-	}
-
-	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;
-	}
-
-	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.20.1

Re: [PATCH v3 4/4] staging: iio: ad7780: moving ad7780 out of staging

[Jonathan Cameron]

On Tue, 5 Feb 2019 15:14:03 -0200
Renato Lui Geh <renatogeh@gmail.com> wrote:

> Move ad7780 ADC driver out of staging and into the mainline.
> 
> The ad7780 is a sigma-delta analog to digital converter. This driver provides
> reading voltage values and status bits from both the ad778x and ad717x series.
> Its interface also allows writing on the FILTER and GAIN GPIO pins on the
> ad778x.
> 
> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>

This needs a device tree binding doc which should be reviewed before we move
the driver out of staging.  Make sure to cc the dt-binding maintainers and
list.  Doesn't really matter if that patch is before or after this one
in the series but needs to be in the same series.

There are a few more minor tidy ups that would be nice to have inline
given you are doing a v4. Stuff like this could have been cleaned up
after moving out of staging (nothing wrong with improving non staging
drivers after all) but always better to do it whilst we remember!

> ---
> Changes in v3:
> 	- Changes unrelated to moving the driver to main tree were resent as
> 	  individual patches
> 
>  drivers/iio/adc/Kconfig          |  13 ++
>  drivers/iio/adc/Makefile         |   1 +
>  drivers/iio/adc/ad7780.c         | 359 +++++++++++++++++++++++++++++++
>  drivers/staging/iio/adc/Kconfig  |  13 --
>  drivers/staging/iio/adc/Makefile |   1 -
>  drivers/staging/iio/adc/ad7780.c | 359 -------------------------------
>  6 files changed, 373 insertions(+), 373 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 f3cc7a31bce5..2cdee166d0e9 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -108,6 +108,19 @@ config AD7766
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad7766.
>  
> +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").

I wouldn't bother with this statement, doesn't add any real info!

> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad7780.
> +
>  config AD7791
>  	tristate "Analog Devices AD7791 ADC driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ea5031348052..b48852157115 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
>  obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
>  obj-$(CONFIG_AD7606) += ad7606.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..163e3c983598
> --- /dev/null
> +++ b/drivers/iio/adc/ad7780.c
> @@ -0,0 +1,359 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.

I think you have done more than enough to this driver to add
an additional copyright line if you want to!

> + */
> +
> +#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)
These two bits of pattern don't really add anything. I'd drop them in
favour of something like

#define AD7780_PATTERN_GOOD 1
#define AD7780_PATTERN_MASK GENMASK(1, 0)

Same for ID for that matter.  These aren't one bit fields, so we shouldn't
ever present them as such (though the datasheet confusingly sort of does
so!)

> +
> +#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)
I'd use a value for the pattern directly and 
GENMASK for the mask.

> +
> +#define AD7780_GAIN_GPIO	0
> +#define AD7780_FILTER_GPIO	1
> +
> +#define AD7780_GAIN_MIDPOINT	64
> +#define AD7780_FILTER_MIDPOINT	13350
> +
> +static const unsigned int ad778x_gain[2]      = { 1, 128 };
> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
> +
> +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;
> +	unsigned int			odr;
> +	unsigned int			int_vref_mv;
> +
> +	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;
> +		voltage_uv /= 1000;
> +		*val = voltage_uv * st->gain;
> +		*val2 = chan->scan_type.realbits - 1;
> +		st->int_vref_mv = voltage_uv;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -(1 << (chan->scan_type.realbits - 1));
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->odr;
> +		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;
> +	unsigned long long vref;
> +	unsigned int full_scale, gain;
> +
> +	if (!chip_info->is_ad778x)
> +		return 0;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		vref = st->int_vref_mv * 1000000LL;
> +		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
> +		gain = DIV_ROUND_CLOSEST(vref, full_scale);
> +		gain = DIV_ROUND_CLOSEST(gain, val2);
> +		st->gain = gain;
> +		if (gain < AD7780_GAIN_MIDPOINT)
> +			gain = 0;
> +		else
> +			gain = 1;
> +		gpiod_set_value(st->gain_gpio, gain);
> +	break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
> +			val = 0;
> +		else
> +			val = 1;
> +		st->odr = ad778x_odr_avail[val];
> +		gpiod_set_value(st->filter_gpio, val);
> +	break;
We'll get a warning here due to the lack of a default handler.
It's pointless except to suppress the warning, but best to add one.

> +	}
> +
> +	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;
> +
> +	if ((raw_sample & AD7780_ERR) ||
> +	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> +		return -EIO;
> +
> +	if (chip_info->is_ad778x) {
> +		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
> +		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
> +	}
> +
> +	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 AD7780_CHANNEL(bits, wordsize) \
> +	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> +
> +static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> +	[ID_AD7170] = {
> +		.channel = AD7780_CHANNEL(12, 24),
> +		.pattern = AD7170_PATTERN,
> +		.pattern_mask = AD7170_PATTERN_MASK,
> +		.is_ad778x = false,
> +	},
> +	[ID_AD7171] = {
> +		.channel = AD7780_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->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);
> +		return ret;
> +	}
> +
> +	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);
> +			return ret;
> +		}
> +
> +		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> +							  "filter",
> +							  GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->filter_gpio)) {
> +			ret = PTR_ERR(st->filter_gpio);
> +			dev_err(&spi->dev,
> +				"Failed to request filter GPIO: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	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;
> +	}
> +
> +	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");

Re: [PATCH v3 4/4] staging: iio: ad7780: moving ad7780 out of staging

[Renato Lui Geh]

On 02/09, Jonathan Cameron wrote:
>On Tue, 5 Feb 2019 15:14:03 -0200
>Renato Lui Geh <renatogeh@gmail.com> wrote:
>
>> Move ad7780 ADC driver out of staging and into the mainline.
>>
>> The ad7780 is a sigma-delta analog to digital converter. This driver provides
>> reading voltage values and status bits from both the ad778x and ad717x series.
>> Its interface also allows writing on the FILTER and GAIN GPIO pins on the
>> ad778x.
>>
>> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
>> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
>> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
>
>This needs a device tree binding doc which should be reviewed before we move
>the driver out of staging.  Make sure to cc the dt-binding maintainers and
>list.  Doesn't really matter if that patch is before or after this one
>in the series but needs to be in the same series.

Ok! I see that some Analog dt-bindings are prefixed by adi and some are
not. Should I follow any naming standard?
>
>There are a few more minor tidy ups that would be nice to have inline
>given you are doing a v4. Stuff like this could have been cleaned up
>after moving out of staging (nothing wrong with improving non staging
>drivers after all) but always better to do it whilst we remember!
>
>> ---
>> Changes in v3:
>> 	- Changes unrelated to moving the driver to main tree were resent as
>> 	  individual patches
>>
>>  drivers/iio/adc/Kconfig          |  13 ++
>>  drivers/iio/adc/Makefile         |   1 +
>>  drivers/iio/adc/ad7780.c         | 359 +++++++++++++++++++++++++++++++
>>  drivers/staging/iio/adc/Kconfig  |  13 --
>>  drivers/staging/iio/adc/Makefile |   1 -
>>  drivers/staging/iio/adc/ad7780.c | 359 -------------------------------
>>  6 files changed, 373 insertions(+), 373 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 f3cc7a31bce5..2cdee166d0e9 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -108,6 +108,19 @@ config AD7766
>>  	  To compile this driver as a module, choose M here: the module will be
>>  	  called ad7766.
>>
>> +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").
>
>I wouldn't bother with this statement, doesn't add any real info!
>
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called ad7780.
>> +
>>  config AD7791
>>  	tristate "Analog Devices AD7791 ADC driver"
>>  	depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index ea5031348052..b48852157115 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
>>  obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
>>  obj-$(CONFIG_AD7606) += ad7606.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..163e3c983598
>> --- /dev/null
>> +++ b/drivers/iio/adc/ad7780.c
>> @@ -0,0 +1,359 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
>> + *
>> + * Copyright 2011 Analog Devices Inc.
>
>I think you have done more than enough to this driver to add
>an additional copyright line if you want to!

Oh wow, that'd be awesome! Thanks! Should I just add my full name there?
>
>> + */
>> +
>> +#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)
>These two bits of pattern don't really add anything. I'd drop them in
>favour of something like
>
>#define AD7780_PATTERN_GOOD 1
>#define AD7780_PATTERN_MASK GENMASK(1, 0)
>
>Same for ID for that matter.  These aren't one bit fields, so we shouldn't
>ever present them as such (though the datasheet confusingly sort of does
>so!)
>
>> +
>> +#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)
>I'd use a value for the pattern directly and
>GENMASK for the mask.
>
>> +
>> +#define AD7780_GAIN_GPIO	0
>> +#define AD7780_FILTER_GPIO	1
>> +
>> +#define AD7780_GAIN_MIDPOINT	64
>> +#define AD7780_FILTER_MIDPOINT	13350
>> +
>> +static const unsigned int ad778x_gain[2]      = { 1, 128 };
>> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
>> +
>> +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;
>> +	unsigned int			odr;
>> +	unsigned int			int_vref_mv;
>> +
>> +	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;
>> +		voltage_uv /= 1000;
>> +		*val = voltage_uv * st->gain;
>> +		*val2 = chan->scan_type.realbits - 1;
>> +		st->int_vref_mv = voltage_uv;
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		*val = -(1 << (chan->scan_type.realbits - 1));
>> +		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*val = st->odr;
>> +		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;
>> +	unsigned long long vref;
>> +	unsigned int full_scale, gain;
>> +
>> +	if (!chip_info->is_ad778x)
>> +		return 0;
>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		if (val != 0)
>> +			return -EINVAL;
>> +
>> +		vref = st->int_vref_mv * 1000000LL;
>> +		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
>> +		gain = DIV_ROUND_CLOSEST(vref, full_scale);
>> +		gain = DIV_ROUND_CLOSEST(gain, val2);
>> +		st->gain = gain;
>> +		if (gain < AD7780_GAIN_MIDPOINT)
>> +			gain = 0;
>> +		else
>> +			gain = 1;
>> +		gpiod_set_value(st->gain_gpio, gain);
>> +	break;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
>> +			val = 0;
>> +		else
>> +			val = 1;
>> +		st->odr = ad778x_odr_avail[val];
>> +		gpiod_set_value(st->filter_gpio, val);
>> +	break;
>We'll get a warning here due to the lack of a default handler.
>It's pointless except to suppress the warning, but best to add one.
>
>> +	}
>> +
>> +	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;
>> +
>> +	if ((raw_sample & AD7780_ERR) ||
>> +	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
>> +		return -EIO;
>> +
>> +	if (chip_info->is_ad778x) {
>> +		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
>> +		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
>> +	}
>> +
>> +	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 AD7780_CHANNEL(bits, wordsize) \
>> +	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
>> +
>> +static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
>> +	[ID_AD7170] = {
>> +		.channel = AD7780_CHANNEL(12, 24),
>> +		.pattern = AD7170_PATTERN,
>> +		.pattern_mask = AD7170_PATTERN_MASK,
>> +		.is_ad778x = false,
>> +	},
>> +	[ID_AD7171] = {
>> +		.channel = AD7780_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->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);
>> +		return ret;
>> +	}
>> +
>> +	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);
>> +			return ret;
>> +		}
>> +
>> +		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
>> +							  "filter",
>> +							  GPIOD_OUT_HIGH);
>> +		if (IS_ERR(st->filter_gpio)) {
>> +			ret = PTR_ERR(st->filter_gpio);
>> +			dev_err(&spi->dev,
>> +				"Failed to request filter GPIO: %d\n",
>> +				ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	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;
>> +	}
>> +
>> +	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");

Re: [PATCH v3 4/4] staging: iio: ad7780: moving ad7780 out of staging

[Jonathan Cameron]

On Thu, 14 Feb 2019 18:48:15 -0200
Renato Lui Geh <renatogeh@gmail.com> wrote:

> On 02/09, Jonathan Cameron wrote:
> >On Tue, 5 Feb 2019 15:14:03 -0200
> >Renato Lui Geh <renatogeh@gmail.com> wrote:
> >  
> >> Move ad7780 ADC driver out of staging and into the mainline.
> >>
> >> The ad7780 is a sigma-delta analog to digital converter. This driver provides
> >> reading voltage values and status bits from both the ad778x and ad717x series.
> >> Its interface also allows writing on the FILTER and GAIN GPIO pins on the
> >> ad778x.
> >>
> >> Signed-off-by: Renato Lui Geh <renatogeh@gmail.com>
> >> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> >> Co-developed-by: Giuliano Belinassi <giuliano.belinassi@usp.br>  
> >
> >This needs a device tree binding doc which should be reviewed before we move
> >the driver out of staging.  Make sure to cc the dt-binding maintainers and
> >list.  Doesn't really matter if that patch is before or after this one
> >in the series but needs to be in the same series.  
> 
> Ok! I see that some Analog dt-bindings are prefixed by adi and some are
> not. Should I follow any naming standard?

Yeah, comes of history and the fact that we can't 'fix' a binding that
is in the wild if the driver is already out of staging.  Even in the
staging case we won't generally do it if we know of real devices out
there using the defacto binding.

They should be prefixed if they aren't 'generic'. Exactly where that lines
tends to be a little unclear unfortunately.

> >
> >There are a few more minor tidy ups that would be nice to have inline
> >given you are doing a v4. Stuff like this could have been cleaned up
> >after moving out of staging (nothing wrong with improving non staging
> >drivers after all) but always better to do it whilst we remember!
> >  
> >> ---
> >> Changes in v3:
> >> 	- Changes unrelated to moving the driver to main tree were resent as
> >> 	  individual patches
> >>
> >>  drivers/iio/adc/Kconfig          |  13 ++
> >>  drivers/iio/adc/Makefile         |   1 +
> >>  drivers/iio/adc/ad7780.c         | 359 +++++++++++++++++++++++++++++++
> >>  drivers/staging/iio/adc/Kconfig  |  13 --
> >>  drivers/staging/iio/adc/Makefile |   1 -
> >>  drivers/staging/iio/adc/ad7780.c | 359 -------------------------------
> >>  6 files changed, 373 insertions(+), 373 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 f3cc7a31bce5..2cdee166d0e9 100644
> >> --- a/drivers/iio/adc/Kconfig
> >> +++ b/drivers/iio/adc/Kconfig
> >> @@ -108,6 +108,19 @@ config AD7766
> >>  	  To compile this driver as a module, choose M here: the module will be
> >>  	  called ad7766.
> >>
> >> +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").  
> >
> >I wouldn't bother with this statement, doesn't add any real info!
> >  
> >> +
> >> +	  To compile this driver as a module, choose M here: the
> >> +	  module will be called ad7780.
> >> +
> >>  config AD7791
> >>  	tristate "Analog Devices AD7791 ADC driver"
> >>  	depends on SPI
> >> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >> index ea5031348052..b48852157115 100644
> >> --- a/drivers/iio/adc/Makefile
> >> +++ b/drivers/iio/adc/Makefile
> >> @@ -15,6 +15,7 @@ obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
> >>  obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
> >>  obj-$(CONFIG_AD7606) += ad7606.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..163e3c983598
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/ad7780.c
> >> @@ -0,0 +1,359 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * AD7170/AD7171 and AD7780/AD7781 SPI ADC driver
> >> + *
> >> + * Copyright 2011 Analog Devices Inc.  
> >
> >I think you have done more than enough to this driver to add
> >an additional copyright line if you want to!  
> 
> Oh wow, that'd be awesome! Thanks! Should I just add my full name there?

Yes.

> >  
> >> + */
> >> +
> >> +#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)  
> >These two bits of pattern don't really add anything. I'd drop them in
> >favour of something like
> >
> >#define AD7780_PATTERN_GOOD 1
> >#define AD7780_PATTERN_MASK GENMASK(1, 0)
> >
> >Same for ID for that matter.  These aren't one bit fields, so we shouldn't
> >ever present them as such (though the datasheet confusingly sort of does
> >so!)
> >  
> >> +
> >> +#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)  
> >I'd use a value for the pattern directly and
> >GENMASK for the mask.
> >  
> >> +
> >> +#define AD7780_GAIN_GPIO	0
> >> +#define AD7780_FILTER_GPIO	1
> >> +
> >> +#define AD7780_GAIN_MIDPOINT	64
> >> +#define AD7780_FILTER_MIDPOINT	13350
> >> +
> >> +static const unsigned int ad778x_gain[2]      = { 1, 128 };
> >> +static const unsigned int ad778x_odr_avail[2] = { 10000, 16700 };
> >> +
> >> +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;
> >> +	unsigned int			odr;
> >> +	unsigned int			int_vref_mv;
> >> +
> >> +	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;
> >> +		voltage_uv /= 1000;
> >> +		*val = voltage_uv * st->gain;
> >> +		*val2 = chan->scan_type.realbits - 1;
> >> +		st->int_vref_mv = voltage_uv;
> >> +		return IIO_VAL_FRACTIONAL_LOG2;
> >> +	case IIO_CHAN_INFO_OFFSET:
> >> +		*val = -(1 << (chan->scan_type.realbits - 1));
> >> +		return IIO_VAL_INT;
> >> +	case IIO_CHAN_INFO_SAMP_FREQ:
> >> +		*val = st->odr;
> >> +		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;
> >> +	unsigned long long vref;
> >> +	unsigned int full_scale, gain;
> >> +
> >> +	if (!chip_info->is_ad778x)
> >> +		return 0;
> >> +
> >> +	switch (m) {
> >> +	case IIO_CHAN_INFO_SCALE:
> >> +		if (val != 0)
> >> +			return -EINVAL;
> >> +
> >> +		vref = st->int_vref_mv * 1000000LL;
> >> +		full_scale = 1 << (chip_info->channel.scan_type.realbits - 1);
> >> +		gain = DIV_ROUND_CLOSEST(vref, full_scale);
> >> +		gain = DIV_ROUND_CLOSEST(gain, val2);
> >> +		st->gain = gain;
> >> +		if (gain < AD7780_GAIN_MIDPOINT)
> >> +			gain = 0;
> >> +		else
> >> +			gain = 1;
> >> +		gpiod_set_value(st->gain_gpio, gain);
> >> +	break;
> >> +	case IIO_CHAN_INFO_SAMP_FREQ:
> >> +		if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
> >> +			val = 0;
> >> +		else
> >> +			val = 1;
> >> +		st->odr = ad778x_odr_avail[val];
> >> +		gpiod_set_value(st->filter_gpio, val);
> >> +	break;  
> >We'll get a warning here due to the lack of a default handler.
> >It's pointless except to suppress the warning, but best to add one.
> >  
> >> +	}
> >> +
> >> +	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;
> >> +
> >> +	if ((raw_sample & AD7780_ERR) ||
> >> +	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> >> +		return -EIO;
> >> +
> >> +	if (chip_info->is_ad778x) {
> >> +		st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
> >> +		st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
> >> +	}
> >> +
> >> +	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 AD7780_CHANNEL(bits, wordsize) \
> >> +	AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> >> +
> >> +static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> >> +	[ID_AD7170] = {
> >> +		.channel = AD7780_CHANNEL(12, 24),
> >> +		.pattern = AD7170_PATTERN,
> >> +		.pattern_mask = AD7170_PATTERN_MASK,
> >> +		.is_ad778x = false,
> >> +	},
> >> +	[ID_AD7171] = {
> >> +		.channel = AD7780_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->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);
> >> +		return ret;
> >> +	}
> >> +
> >> +	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);
> >> +			return ret;
> >> +		}
> >> +
> >> +		st->filter_gpio = devm_gpiod_get_optional(&spi->dev,
> >> +							  "filter",
> >> +							  GPIOD_OUT_HIGH);
> >> +		if (IS_ERR(st->filter_gpio)) {
> >> +			ret = PTR_ERR(st->filter_gpio);
> >> +			dev_err(&spi->dev,
> >> +				"Failed to request filter GPIO: %d\n",
> >> +				ret);
> >> +			return ret;
> >> +		}
> >> +	}
> >> +
> >> +	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;
> >> +	}
> >> +
> >> +	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");