Re: [PATCH v2 11/16] iio: adc: ad7768-1: Add VCM output support

[David Lechner <dlechner@baylibre.com>]

On 1/27/25 9:13 AM, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> The VCM output voltage can be used as a common-mode voltage within the
> amplifier preconditioning circuits external to the AD7768-1.
> 
> This change allows the user to configure VCM output trough a devicetree
> attribute.
> 
> Co-developed-by: Jonathan Santos <Jonathan.Santos@analog.com>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>

Correct order according to [1]:

Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
Co-developed-by: Jonathan Santos <Jonathan.Santos@analog.com>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>


[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

> ---
> v2 Changes:
> * VCM output support is now defined by a devicetree property, instead of 
>   and IIO attribute.
> ---
>  drivers/iio/adc/ad7768-1.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 8487b9a06609..c540583808c2 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -24,6 +24,8 @@
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/trigger_consumer.h>
>  
> +#include <dt-bindings/iio/adc/adi,ad7768-1.h>
> +
>  /* AD7768 registers definition */
>  #define AD7768_REG_CHIP_TYPE		0x3
>  #define AD7768_REG_PROD_ID_L		0x4
> @@ -347,6 +349,11 @@ static int ad7768_set_freq(struct ad7768_state *st,
>  	return 0;
>  }
>  
> +static int ad7768_set_vcm_output(struct ad7768_state *st, unsigned int mode)
> +{

Is setting CHOP_FREQUENCY bit to 0 intentional here? Could use a comment or
use regmap_update_bits() + FIELD_PREP() instead.

> +	return regmap_write(st->regmap, AD7768_REG_ANALOG2, mode);
> +}

IMHO, one-line helper function just makes more code to read, so would be
simpler without it.

> +
>  static ssize_t ad7768_sampling_freq_avail(struct device *dev,
>  					  struct device_attribute *attr,
>  					  char *buf)
> @@ -628,6 +635,7 @@ static int ad7768_probe(struct spi_device *spi)
>  {
>  	struct ad7768_state *st;
>  	struct iio_dev *indio_dev;
> +	u32 val;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> @@ -688,6 +696,18 @@ static int ad7768_probe(struct spi_device *spi)
>  	indio_dev->info = &ad7768_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> +	ret = device_property_read_u32(&spi->dev, "adi,vcm-output", &val);
> +	if (!ret) {
> +		if (val > AD7768_VCM_OUTPUT_OFF) {
> +			dev_err(&spi->dev, "Invalid VCM output value\n");
> +			return -EINVAL;

			return dev_err_probe(...);

> +		}
> +
> +		ret = ad7768_set_vcm_output(st, val);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = ad7768_setup(st);
>  	if (ret < 0) {
>  		dev_err(&spi->dev, "AD7768 setup failed\n");