[PATCH v2 0/2] iio: adc: ad9467: fixes for ad9434

[PATCH v2 0/2] iio: adc: ad9467: fixes for ad9434

[Tomas Melin]

Add support for setting offset range (calibration) for the ad9434
and fixup vref mask handling.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
Changes in v2:
Updates according to received feedback:
- embed ad9434 channel description instead of resorting to macro
- change INFO_OFFSET to INFO_CALIBBIAS
- keep offset value untouched in case of error
- drop length from avail_range
- Link to v1: https://lore.kernel.org/r/20251201-ad9434-fixes-v1-0-54a9ca2ac514@vaisala.com

---
Tomas Melin (2):
      iio: adc: ad9467: fix ad9434 vref mask
      iio: adc: ad9467: support write/read offset

 drivers/iio/adc/ad9467.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)
---
base-commit: 9b9e43704d2b05514aeeaea36311addba2c72408
change-id: 20251201-ad9434-fixes-6dfdc86fb881

Best regards,
-- 
Tomas Melin <tomas.melin@vaisala.com>

[PATCH v2 1/2] iio: adc: ad9467: fix ad9434 vref mask

[Tomas Melin]

The mask setting is 5 bits wide for the ad9434
(ref. data sheet register 0x18 FLEX_VREF). Apparently the settings
from ad9265 were copied by mistake when support for the device was added
to the driver.

Fixes: 4606d0f4b05f ("iio: adc: ad9467: add support for AD9434 high-speed ADC")
Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/iio/adc/ad9467.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index f7a9f46ea0dc405e25f312197df4b2131871b4bc..2d8f8da3671dac61994a1864a82cdbef7f54c1af 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -95,7 +95,7 @@
 
 #define CHIPID_AD9434			0x6A
 #define AD9434_DEF_OUTPUT_MODE		0x00
-#define AD9434_REG_VREF_MASK		0xC0
+#define AD9434_REG_VREF_MASK		GENMASK(4, 0)
 
 /*
  * Analog Devices AD9467 16-Bit, 200/250 MSPS ADC

-- 
2.47.3

[PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Tomas Melin]

Support configuring output calibration value. Among the devices
currently supported by this driver, this setting is specific to
ad9434. The offset can be used to calibrate the output against
a known input. The register is called offset, but the procedure
is best mapped internally with calibbias operation.

Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
---
 drivers/iio/adc/ad9467.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 2d8f8da3671dac61994a1864a82cdbef7f54c1af..c3cf7ae977d4279ce5e80a7c956c3844483eb8bd 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -145,6 +145,7 @@ struct ad9467_chip_info {
 	unsigned int num_lanes;
 	unsigned int dco_en;
 	unsigned int test_points;
+	const int *offset_range;
 	/* data clock output */
 	bool has_dco;
 	bool has_dco_invert;
@@ -234,6 +235,10 @@ static int ad9467_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 	return 0;
 }
 
+static const int ad9434_offset_range[] = {
+	-128, 1, 127,
+};
+
 static const unsigned int ad9265_scale_table[][2] = {
 	{1250, 0x00}, {1500, 0x40}, {1750, 0x80}, {2000, 0xC0},
 };
@@ -298,7 +303,24 @@ static void __ad9467_get_scale(struct ad9467_state *st, int index,
 }
 
 static const struct iio_chan_spec ad9434_channels[] = {
-	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_shared_by_type =
+		BIT(IIO_CHAN_INFO_SCALE) |
+		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+		BIT(IIO_CHAN_INFO_CALIBBIAS),
+		.info_mask_shared_by_type_available =
+		BIT(IIO_CHAN_INFO_SCALE) |
+		BIT(IIO_CHAN_INFO_CALIBBIAS),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 12,
+			.storagebits = 16,
+		},
+	},
 };
 
 static const struct iio_chan_spec ad9467_channels[] = {
@@ -367,6 +389,7 @@ static const struct ad9467_chip_info ad9434_chip_tbl = {
 	.default_output_mode = AD9434_DEF_OUTPUT_MODE,
 	.vref_mask = AD9434_REG_VREF_MASK,
 	.num_lanes = 6,
+	.offset_range = ad9434_offset_range,
 };
 
 static const struct ad9467_chip_info ad9265_chip_tbl = {
@@ -499,6 +522,33 @@ static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
 	return -EINVAL;
 }
 
+static int ad9467_get_offset(struct ad9467_state *st, int *val)
+{
+	int ret;
+
+	ret = ad9467_spi_read(st, AN877_ADC_REG_OFFSET);
+	if (ret < 0)
+		return ret;
+	*val = ret;
+
+	return IIO_VAL_INT;
+}
+
+static int ad9467_set_offset(struct ad9467_state *st, int val)
+{
+	int ret;
+
+	if (val < st->info->offset_range[0] || val > st->info->offset_range[2])
+		return -EINVAL;
+
+	ret = ad9467_spi_write(st, AN877_ADC_REG_OFFSET, val);
+	if (ret < 0)
+		return ret;
+	/* Sync registers */
+	return ad9467_spi_write(st, AN877_ADC_REG_TRANSFER,
+				AN877_ADC_TRANSFER_SYNC);
+}
+
 static int ad9467_outputmode_set(struct ad9467_state *st, unsigned int mode)
 {
 	int ret;
@@ -802,6 +852,8 @@ static int ad9467_read_raw(struct iio_dev *indio_dev,
 	struct ad9467_state *st = iio_priv(indio_dev);
 
 	switch (m) {
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return ad9467_get_offset(st, val);
 	case IIO_CHAN_INFO_SCALE:
 		return ad9467_get_scale(st, val, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
@@ -836,6 +888,8 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
 	int ret;
 
 	switch (mask) {
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return ad9467_set_offset(st, val);
 	case IIO_CHAN_INFO_SCALE:
 		return ad9467_set_scale(st, val, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
@@ -874,6 +928,10 @@ static int ad9467_read_avail(struct iio_dev *indio_dev,
 	const struct ad9467_chip_info *info = st->info;
 
 	switch (mask) {
+	case IIO_CHAN_INFO_CALIBBIAS:
+		*type = IIO_VAL_INT;
+		*vals = info->offset_range;
+		return IIO_AVAIL_RANGE;
 	case IIO_CHAN_INFO_SCALE:
 		*vals = (const int *)st->scales;
 		*type = IIO_VAL_INT_PLUS_MICRO;

-- 
2.47.3

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Nuno Sá]

On Tue, 2025-12-02 at 12:53 +0000, Tomas Melin wrote:
> Support configuring output calibration value. Among the devices
> currently supported by this driver, this setting is specific to
> ad9434. The offset can be used to calibrate the output against
> a known input. The register is called offset, but the procedure
> is best mapped internally with calibbias operation.
> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  drivers/iio/adc/ad9467.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 2d8f8da3671dac61994a1864a82cdbef7f54c1af..c3cf7ae977d4279ce5e80a7c956c3844483eb8bd 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -145,6 +145,7 @@ struct ad9467_chip_info {
>  	unsigned int num_lanes;
>  	unsigned int dco_en;
>  	unsigned int test_points;
> +	const int *offset_range;
>  	/* data clock output */
>  	bool has_dco;
>  	bool has_dco_invert;
> @@ -234,6 +235,10 @@ static int ad9467_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>  	return 0;
>  }
>  
> +static const int ad9434_offset_range[] = {
> +	-128, 1, 127,
> +};
> +
>  static const unsigned int ad9265_scale_table[][2] = {
>  	{1250, 0x00}, {1500, 0x40}, {1750, 0x80}, {2000, 0xC0},
>  };
> @@ -298,7 +303,24 @@ static void __ad9467_get_scale(struct ad9467_state *st, int index,
>  }
>  
>  static const struct iio_chan_spec ad9434_channels[] = {
> -	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
> +	{
> +		.type = IIO_VOLTAGE,
> +		.indexed = 1,
> +		.channel = 0,
> +		.info_mask_shared_by_type =
> +		BIT(IIO_CHAN_INFO_SCALE) |
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
> +		.info_mask_shared_by_type_available =
> +		BIT(IIO_CHAN_INFO_SCALE) |
> +		BIT(IIO_CHAN_INFO_CALIBBIAS),

Odd style for info_mask_shared_by_type_available and info_mask_shared_by_type. Seems we have
more line breaks than needed.


> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 12,
> +			.storagebits = 16,
> +		},
> +	},
>  };
>  
>  static const struct iio_chan_spec ad9467_channels[] = {
> @@ -367,6 +389,7 @@ static const struct ad9467_chip_info ad9434_chip_tbl = {
>  	.default_output_mode = AD9434_DEF_OUTPUT_MODE,
>  	.vref_mask = AD9434_REG_VREF_MASK,
>  	.num_lanes = 6,
> +	.offset_range = ad9434_offset_range,
>  };
>  
>  static const struct ad9467_chip_info ad9265_chip_tbl = {
> @@ -499,6 +522,33 @@ static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
>  	return -EINVAL;
>  }
>  
> +static int ad9467_get_offset(struct ad9467_state *st, int *val)
> +{
> +	int ret;
> +
> +	ret = ad9467_spi_read(st, AN877_ADC_REG_OFFSET);
> +	if (ret < 0)
> +		return ret;
> +	*val = ret;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad9467_set_offset(struct ad9467_state *st, int val)
> +{
> +	int ret;
> +
> +	if (val < st->info->offset_range[0] || val > st->info->offset_range[2])
> +		return -EINVAL;
> +
> +	ret = ad9467_spi_write(st, AN877_ADC_REG_OFFSET, val);
> +	if (ret < 0)
> +		return ret;
> +	/* Sync registers */

I think this is not what David meant by adding a comment. IMHO, the comment as-is does not
bring any added value.

- Nuno Sá

Re: [PATCH v2 1/2] iio: adc: ad9467: fix ad9434 vref mask

[Nuno Sá]

On Tue, 2025-12-02 at 12:53 +0000, Tomas Melin wrote:
> The mask setting is 5 bits wide for the ad9434
> (ref. data sheet register 0x18 FLEX_VREF). Apparently the settings
> from ad9265 were copied by mistake when support for the device was added
> to the driver.
> 
> Fixes: 4606d0f4b05f ("iio: adc: ad9467: add support for AD9434 high-speed ADC")
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
>  drivers/iio/adc/ad9467.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index f7a9f46ea0dc405e25f312197df4b2131871b4bc..2d8f8da3671dac61994a1864a82cdbef7f54c1af 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -95,7 +95,7 @@
>  
>  #define CHIPID_AD9434			0x6A
>  #define AD9434_DEF_OUTPUT_MODE		0x00
> -#define AD9434_REG_VREF_MASK		0xC0
> +#define AD9434_REG_VREF_MASK		GENMASK(4, 0)

In theory we should fix it using the same mask style as the other variants and then moving all to
GENMASK(). However, I do not feel strong about it and I can send a follow up. Hence:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

>  
>  /*
>   * Analog Devices AD9467 16-Bit, 200/250 MSPS ADC

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Andy Shevchenko]

On Tue, Dec 02, 2025 at 12:53:09PM +0000, Tomas Melin wrote:
> Support configuring output calibration value. Among the devices
> currently supported by this driver, this setting is specific to
> ad9434. The offset can be used to calibrate the output against
> a known input. The register is called offset, but the procedure
> is best mapped internally with calibbias operation.

...

>  static const struct iio_chan_spec ad9434_channels[] = {
> -	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
> +	{
> +		.type = IIO_VOLTAGE,
> +		.indexed = 1,
> +		.channel = 0,
> +		.info_mask_shared_by_type =
> +		BIT(IIO_CHAN_INFO_SCALE) |
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +		BIT(IIO_CHAN_INFO_CALIBBIAS),

Wrong indentation.

> +		.info_mask_shared_by_type_available =
> +		BIT(IIO_CHAN_INFO_SCALE) |
> +		BIT(IIO_CHAN_INFO_CALIBBIAS),

Ditto.

> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 12,
> +			.storagebits = 16,
> +		},
> +	},
>  };

I'm not sure about macro-less approach here, I think that we want more
consistency and hence before doing this change probably we want to clean up
the existing macro, then split it to two, and add another one here based on
the low-level, which was split in the previous clean up.

...

> +	return ad9467_spi_write(st, AN877_ADC_REG_TRANSFER,
> +				AN877_ADC_TRANSFER_SYNC);

I would make it one line, despite on being 85 characters long.
But it's up to you and maintainers.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] iio: adc: ad9467: fix ad9434 vref mask

[Andy Shevchenko]

On Tue, Dec 02, 2025 at 12:53:08PM +0000, Tomas Melin wrote:
> The mask setting is 5 bits wide for the ad9434
> (ref. data sheet register 0x18 FLEX_VREF). Apparently the settings
> from ad9265 were copied by mistake when support for the device was added
> to the driver.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Tomas Melin]

Hi,

On 02/12/2025 15:47, Nuno Sá wrote:
> On Tue, 2025-12-02 at 12:53 +0000, Tomas Melin wrote:

>>  static const struct iio_chan_spec ad9434_channels[] = {
>> -	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
>> +	{
>> +		.type = IIO_VOLTAGE,
>> +		.indexed = 1,
>> +		.channel = 0,
>> +		.info_mask_shared_by_type =
>> +		BIT(IIO_CHAN_INFO_SCALE) |
>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
>> +		.info_mask_shared_by_type_available =
>> +		BIT(IIO_CHAN_INFO_SCALE) |
>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
> 
> Odd style for info_mask_shared_by_type_available and info_mask_shared_by_type. Seems we have
> more line breaks than needed.
> 
Looking at existing code, there seems to many different ways to indent
these kind of lines. Can you please provide your preferred style?


> 
>> +		.scan_index = 0,
>> +		.scan_type = {
>> +			.sign = 's',
>> +			.realbits = 12,
>> +			.storagebits = 16,
>> +		},
>> +	},
>>  };
>>  
>>  static const struct iio_chan_spec ad9467_channels[] = {
>> @@ -367,6 +389,7 @@ static const struct ad9467_chip_info ad9434_chip_tbl = {
>>  	.default_output_mode = AD9434_DEF_OUTPUT_MODE,
>>  	.vref_mask = AD9434_REG_VREF_MASK,
>>  	.num_lanes = 6,
>> +	.offset_range = ad9434_offset_range,
>>  };
>>  
>>  static const struct ad9467_chip_info ad9265_chip_tbl = {
>> @@ -499,6 +522,33 @@ static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
>>  	return -EINVAL;
>>  }
>>  
>> +static int ad9467_get_offset(struct ad9467_state *st, int *val)
>> +{
>> +	int ret;
>> +
>> +	ret = ad9467_spi_read(st, AN877_ADC_REG_OFFSET);
>> +	if (ret < 0)
>> +		return ret;
>> +	*val = ret;
>> +
>> +	return IIO_VAL_INT;
>> +}
>> +
>> +static int ad9467_set_offset(struct ad9467_state *st, int val)
>> +{
>> +	int ret;
>> +
>> +	if (val < st->info->offset_range[0] || val > st->info->offset_range[2])
>> +		return -EINVAL;
>> +
>> +	ret = ad9467_spi_write(st, AN877_ADC_REG_OFFSET, val);
>> +	if (ret < 0)
>> +		return ret;
>> +	/* Sync registers */
> 
> I think this is not what David meant by adding a comment. IMHO, the comment as-is does not
> bring any added value.
The sync operation is needed in several places and is not commented in
other locations either. Do you prefer no comment or even more elaborate
comment for this particular sync operation?

Thanks,
Tomas


> 
> - Nuno Sá
> 

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Tomas Melin]

On 02/12/2025 16:11, Andy Shevchenko wrote:
> On Tue, Dec 02, 2025 at 12:53:09PM +0000, Tomas Melin wrote:
>> Support configuring output calibration value. Among the devices
>> currently supported by this driver, this setting is specific to
>> ad9434. The offset can be used to calibrate the output against
>> a known input. The register is called offset, but the procedure
>> is best mapped internally with calibbias operation.
> 
> ...
> 
>>  static const struct iio_chan_spec ad9434_channels[] = {
>> -	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
>> +	{
>> +		.type = IIO_VOLTAGE,
>> +		.indexed = 1,
>> +		.channel = 0,
>> +		.info_mask_shared_by_type =
>> +		BIT(IIO_CHAN_INFO_SCALE) |
>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
> 
> Wrong indentation.

Can you please provide example of your preferred indentation for this
particular case? This is used in several places around the code and
seemed like one of the more readable.

> 
>> +		.info_mask_shared_by_type_available =
>> +		BIT(IIO_CHAN_INFO_SCALE) |
>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
> 
> Ditto.
> 
>> +		.scan_index = 0,
>> +		.scan_type = {
>> +			.sign = 's',
>> +			.realbits = 12,
>> +			.storagebits = 16,
>> +		},
>> +	},
>>  };
> 
> I'm not sure about macro-less approach here, I think that we want more
> consistency and hence before doing this change probably we want to clean up
> the existing macro, then split it to two, and add another one here based on
> the low-level, which was split in the previous clean up.

As mentioned, this is only needed for a single channel, and since it is
different than the other, it needs to be separated. Do You think we
actually need another macro for this?

> 
> ...
> 
>> +	return ad9467_spi_write(st, AN877_ADC_REG_TRANSFER,
>> +				AN877_ADC_TRANSFER_SYNC);
> 
> I would make it one line, despite on being 85 characters long.
> But it's up to you and maintainers.
I would like to not fight against checkpatch here.

> 

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Nuno Sá]

On Tue, 2025-12-02 at 16:52 +0200, Tomas Melin wrote:
> Hi,
> 
> On 02/12/2025 15:47, Nuno Sá wrote:
> > On Tue, 2025-12-02 at 12:53 +0000, Tomas Melin wrote:
> 
> > >  static const struct iio_chan_spec ad9434_channels[] = {
> > > -	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
> > > +	{
> > > +		.type = IIO_VOLTAGE,
> > > +		.indexed = 1,
> > > +		.channel = 0,
> > > +		.info_mask_shared_by_type =
> > > +		BIT(IIO_CHAN_INFO_SCALE) |
> > > +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > > +		BIT(IIO_CHAN_INFO_CALIBBIAS),
> > > +		.info_mask_shared_by_type_available =
> > > +		BIT(IIO_CHAN_INFO_SCALE) |
> > > +		BIT(IIO_CHAN_INFO_CALIBBIAS),
> > 
> > Odd style for info_mask_shared_by_type_available and info_mask_shared_by_type. Seems we have
> > more line breaks than needed.
> > 
> Looking at existing code, there seems to many different ways to indent
> these kind of lines. Can you please provide your preferred style?
> 

Looking at the same driver I would expect something like:

https://elixir.bootlin.com/linux/v6.18/source/drivers/iio/adc/ad9467.c#L289

So, just break the line when the col limit is reached.

> 
> > 
> > > +		.scan_index = 0,
> > > +		.scan_type = {
> > > +			.sign = 's',
> > > +			.realbits = 12,
> > > +			.storagebits = 16,
> > > +		},
> > > +	},
> > >  };
> > >  
> > >  static const struct iio_chan_spec ad9467_channels[] = {
> > > @@ -367,6 +389,7 @@ static const struct ad9467_chip_info ad9434_chip_tbl = {
> > >  	.default_output_mode = AD9434_DEF_OUTPUT_MODE,
> > >  	.vref_mask = AD9434_REG_VREF_MASK,
> > >  	.num_lanes = 6,
> > > +	.offset_range = ad9434_offset_range,
> > >  };
> > >  
> > >  static const struct ad9467_chip_info ad9265_chip_tbl = {
> > > @@ -499,6 +522,33 @@ static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
> > >  	return -EINVAL;
> > >  }
> > >  
> > > +static int ad9467_get_offset(struct ad9467_state *st, int *val)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = ad9467_spi_read(st, AN877_ADC_REG_OFFSET);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	*val = ret;
> > > +
> > > +	return IIO_VAL_INT;
> > > +}
> > > +
> > > +static int ad9467_set_offset(struct ad9467_state *st, int val)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (val < st->info->offset_range[0] || val > st->info->offset_range[2])
> > > +		return -EINVAL;
> > > +
> > > +	ret = ad9467_spi_write(st, AN877_ADC_REG_OFFSET, val);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	/* Sync registers */
> > 
> > I think this is not what David meant by adding a comment. IMHO, the comment as-is does not
> > bring any added value.
> The sync operation is needed in several places and is not commented in
> other locations either. Do you prefer no comment or even more elaborate
> comment for this particular sync operation?
> 

I know. I'm just stating the comment, as is, does not bring much value. But I was not the one asking
for it so I guess you should ask David :)

- Nuno Sá

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[David Lechner]

On 12/2/25 9:05 AM, Nuno Sá wrote:
> On Tue, 2025-12-02 at 16:52 +0200, Tomas Melin wrote:
>> Hi,
>>
>> On 02/12/2025 15:47, Nuno Sá wrote:
>>> On Tue, 2025-12-02 at 12:53 +0000, Tomas Melin wrote:
>>
>>>>  static const struct iio_chan_spec ad9434_channels[] = {
>>>> -	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
>>>> +	{
>>>> +		.type = IIO_VOLTAGE,
>>>> +		.indexed = 1,
>>>> +		.channel = 0,
>>>> +		.info_mask_shared_by_type =
>>>> +		BIT(IIO_CHAN_INFO_SCALE) |
>>>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
>>>> +		.info_mask_shared_by_type_available =
>>>> +		BIT(IIO_CHAN_INFO_SCALE) |
>>>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
>>>
>>> Odd style for info_mask_shared_by_type_available and info_mask_shared_by_type. Seems we have
>>> more line breaks than needed.
>>>
>> Looking at existing code, there seems to many different ways to indent
>> these kind of lines. Can you please provide your preferred style?
>>
> 
> Looking at the same driver I would expect something like:
> 
> https://elixir.bootlin.com/linux/v6.18/source/drivers/iio/adc/ad9467.c#L289
> 
> So, just break the line when the col limit is reached.
> 
>>
>>>
>>>> +		.scan_index = 0,
>>>> +		.scan_type = {
>>>> +			.sign = 's',
>>>> +			.realbits = 12,
>>>> +			.storagebits = 16,
>>>> +		},
>>>> +	},
>>>>  };
>>>>  
>>>>  static const struct iio_chan_spec ad9467_channels[] = {
>>>> @@ -367,6 +389,7 @@ static const struct ad9467_chip_info ad9434_chip_tbl = {
>>>>  	.default_output_mode = AD9434_DEF_OUTPUT_MODE,
>>>>  	.vref_mask = AD9434_REG_VREF_MASK,
>>>>  	.num_lanes = 6,
>>>> +	.offset_range = ad9434_offset_range,
>>>>  };
>>>>  
>>>>  static const struct ad9467_chip_info ad9265_chip_tbl = {
>>>> @@ -499,6 +522,33 @@ static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
>>>>  	return -EINVAL;
>>>>  }
>>>>  
>>>> +static int ad9467_get_offset(struct ad9467_state *st, int *val)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = ad9467_spi_read(st, AN877_ADC_REG_OFFSET);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +	*val = ret;
>>>> +
>>>> +	return IIO_VAL_INT;
>>>> +}
>>>> +
>>>> +static int ad9467_set_offset(struct ad9467_state *st, int val)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	if (val < st->info->offset_range[0] || val > st->info->offset_range[2])
>>>> +		return -EINVAL;
>>>> +
>>>> +	ret = ad9467_spi_write(st, AN877_ADC_REG_OFFSET, val);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +	/* Sync registers */
>>>
>>> I think this is not what David meant by adding a comment. IMHO, the comment as-is does not
>>> bring any added value.
>> The sync operation is needed in several places and is not commented in
>> other locations either. Do you prefer no comment or even more elaborate
>> comment for this particular sync operation?
>>
> 
> I know. I'm just stating the comment, as is, does not bring much value. But I was not the one asking
> for it so I guess you should ask David :)
> 
> - Nuno Sá

I did not look at the rest of the driver before. I guess the
fact that it does the sync after every register write makes it
clear enough that this is just a thing you have to do. So I'm
OK with leaving out the comment.

What I was asking for though is _why_ do we need to do that?

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Nuno Sá]

On Tue, 2025-12-02 at 17:01 +0200, Tomas Melin wrote:
> 
> 
> On 02/12/2025 16:11, Andy Shevchenko wrote:
> > On Tue, Dec 02, 2025 at 12:53:09PM +0000, Tomas Melin wrote:
> > > Support configuring output calibration value. Among the devices
> > > currently supported by this driver, this setting is specific to
> > > ad9434. The offset can be used to calibrate the output against
> > > a known input. The register is called offset, but the procedure
> > > is best mapped internally with calibbias operation.
> > 
> > ...
> > 
> > >  static const struct iio_chan_spec ad9434_channels[] = {
> > > -	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
> > > +	{
> > > +		.type = IIO_VOLTAGE,
> > > +		.indexed = 1,
> > > +		.channel = 0,
> > > +		.info_mask_shared_by_type =
> > > +		BIT(IIO_CHAN_INFO_SCALE) |
> > > +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > > +		BIT(IIO_CHAN_INFO_CALIBBIAS),
> > 
> > Wrong indentation.
> 
> Can you please provide example of your preferred indentation for this
> particular case? This is used in several places around the code and
> seemed like one of the more readable.
> 
> > 
> > > +		.info_mask_shared_by_type_available =
> > > +		BIT(IIO_CHAN_INFO_SCALE) |
> > > +		BIT(IIO_CHAN_INFO_CALIBBIAS),
> > 
> > Ditto.
> > 
> > > +		.scan_index = 0,
> > > +		.scan_type = {
> > > +			.sign = 's',
> > > +			.realbits = 12,
> > > +			.storagebits = 16,
> > > +		},
> > > +	},
> > >  };
> > 
> > I'm not sure about macro-less approach here, I think that we want more
> > consistency and hence before doing this change probably we want to clean up
> > the existing macro, then split it to two, and add another one here based on
> > the low-level, which was split in the previous clean up.
> 
> As mentioned, this is only needed for a single channel, and since it is
> different than the other, it needs to be separated. Do You think we
> actually need another macro for this?
> 
> > 
> > ...
> > 
> > > +	return ad9467_spi_write(st, AN877_ADC_REG_TRANSFER,
> > > +				AN877_ADC_TRANSFER_SYNC);
> > 
> > I would make it one line, despite on being 85 characters long.
> > But it's up to you and maintainers.
> I would like to not fight against checkpatch here.
> 
> > 

AFAIK, Jonathan policy is that 80 column limit is still the preferred limit unless readability is
hurt. So I would say the line break here is up to the IIO policy.

- Nuno Sá

Re: [PATCH v2 0/2] iio: adc: ad9467: fixes for ad9434

[David Lechner]

On 12/2/25 6:53 AM, Tomas Melin wrote:
> Add support for setting offset range (calibration) for the ad9434
> and fixup vref mask handling.
> 
> Signed-off-by: Tomas Melin <tomas.melin@vaisala.com>
> ---
> Changes in v2:
> Updates according to received feedback:
> - embed ad9434 channel description instead of resorting to macro
> - change INFO_OFFSET to INFO_CALIBBIAS
> - keep offset value untouched in case of error
> - drop length from avail_range
> - Link to v1: https://lore.kernel.org/r/20251201-ad9434-fixes-v1-0-54a9ca2ac514@vaisala.com
> 
> ---
> Tomas Melin (2):
>       iio: adc: ad9467: fix ad9434 vref mask
>       iio: adc: ad9467: support write/read offset
> 
>  drivers/iio/adc/ad9467.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 60 insertions(+), 2 deletions(-)
> ---
> base-commit: 9b9e43704d2b05514aeeaea36311addba2c72408
> change-id: 20251201-ad9434-fixes-6dfdc86fb881
> 
> Best regards,

Reviewed-by: David Lechner <dlechner@baylibre.com>

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Andy Shevchenko]

On Tue, Dec 2, 2025 at 6:08 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Tue, 2025-12-02 at 17:01 +0200, Tomas Melin wrote:
> > On 02/12/2025 16:11, Andy Shevchenko wrote:
> > > On Tue, Dec 02, 2025 at 12:53:09PM +0000, Tomas Melin wrote:

...

> > > > + return ad9467_spi_write(st, AN877_ADC_REG_TRANSFER,
> > > > +                         AN877_ADC_TRANSFER_SYNC);
> > >
> > > I would make it one line, despite on being 85 characters long.
> > > But it's up to you and maintainers.
> > I would like to not fight against checkpatch here.

Checkpatch doesn't complain about this unless you specifically tell it
to do so with --strict.

> AFAIK, Jonathan policy is that 80 column limit is still the preferred limit unless readability is
> hurt. So I would say the line break here is up to the IIO policy.

Right, that's why I wrote the second sentence there :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Tomas Melin]

On 02/12/2025 17:28, David Lechner wrote:
> On 12/2/25 9:05 AM, Nuno Sá wrote:
>> On Tue, 2025-12-02 at 16:52 +0200, Tomas Melin wrote:

>>>>> +
>>>>> +static int ad9467_set_offset(struct ad9467_state *st, int val)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	if (val < st->info->offset_range[0] || val > st->info->offset_range[2])
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	ret = ad9467_spi_write(st, AN877_ADC_REG_OFFSET, val);
>>>>> +	if (ret < 0)
>>>>> +		return ret;
>>>>> +	/* Sync registers */
>>>>
>>>> I think this is not what David meant by adding a comment. IMHO, the comment as-is does not
>>>> bring any added value.
>>> The sync operation is needed in several places and is not commented in
>>> other locations either. Do you prefer no comment or even more elaborate
>>> comment for this particular sync operation?
>>>
>>
>> I know. I'm just stating the comment, as is, does not bring much value. But I was not the one asking
>> for it so I guess you should ask David :)
>>
>> - Nuno Sá
> 
> I did not look at the rest of the driver before. I guess the
> fact that it does the sync after every register write makes it
> clear enough that this is just a thing you have to do. So I'm
> OK with leaving out the comment.
Thanks for the clarification. I will remove the commment for v3.

> 
> What I was asking for though is _why_ do we need to do that?

Addresses in range 0x8-0x2A require a sync operation to transfer the
data into use. So it's a kind of latching.

Thanks,
Tomas

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Tomas Melin]

On 02/12/2025 17:01, Tomas Melin wrote:
> 
> 
> On 02/12/2025 16:11, Andy Shevchenko wrote:
>> On Tue, Dec 02, 2025 at 12:53:09PM +0000, Tomas Melin wrote:
>>> Support configuring output calibration value. Among the devices
>>> currently supported by this driver, this setting is specific to
>>> ad9434. The offset can be used to calibrate the output against
>>> a known input. The register is called offset, but the procedure
>>> is best mapped internally with calibbias operation.
>>
>> ...
>>
>>>  static const struct iio_chan_spec ad9434_channels[] = {
>>> -	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
>>> +	{
>>> +		.type = IIO_VOLTAGE,
>>> +		.indexed = 1,
>>> +		.channel = 0,
>>> +		.info_mask_shared_by_type =
>>> +		BIT(IIO_CHAN_INFO_SCALE) |
>>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
>>
>> Wrong indentation.
> 
> Can you please provide example of your preferred indentation for this
> particular case? This is used in several places around the code and
> seemed like one of the more readable.

Would this be the preferred indentation?

{
	.type = IIO_VOLTAGE,
	.indexed = 1,
	.channel = 0,
	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
				    BIT(IIO_CHAN_INFO_SAMP_FREQ) |
				    BIT(IIO_CHAN_INFO_CALIBBIAS),
	.info_mask_shared_by_type_available =
		BIT(IIO_CHAN_INFO_SCALE) |
		BIT(IIO_CHAN_INFO_CALIBBIAS),
	.scan_index = 0,
	.scan_type = {
		.sign = 's',
		.realbits = 12,
		.storagebits = 16,
	},
},


BR,
Tomas

> 
>>
>>> +		.info_mask_shared_by_type_available =
>>> +		BIT(IIO_CHAN_INFO_SCALE) |
>>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
>>
>> Ditto.
>>
>>> +		.scan_index = 0,
>>> +		.scan_type = {
>>> +			.sign = 's',
>>> +			.realbits = 12,
>>> +			.storagebits = 16,
>>> +		},
>>> +	},
>>>  };
>>
>> I'm not sure about macro-less approach here, I think that we want more
>> consistency and hence before doing this change probably we want to clean up
>> the existing macro, then split it to two, and add another one here based on
>> the low-level, which was split in the previous clean up.
> 
> As mentioned, this is only needed for a single channel, and since it is
> different than the other, it needs to be separated. Do You think we
> actually need another macro for this?
> 
>>
>> ...
>>
>>> +	return ad9467_spi_write(st, AN877_ADC_REG_TRANSFER,
>>> +				AN877_ADC_TRANSFER_SYNC);
>>
>> I would make it one line, despite on being 85 characters long.
>> But it's up to you and maintainers.
> I would like to not fight against checkpatch here.
> 
>>
> 
> 

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Andy Shevchenko]

On Wed, Dec 03, 2025 at 09:28:23AM +0200, Tomas Melin wrote:
> On 02/12/2025 17:01, Tomas Melin wrote:
> > On 02/12/2025 16:11, Andy Shevchenko wrote:
> >> On Tue, Dec 02, 2025 at 12:53:09PM +0000, Tomas Melin wrote:

...

> >>>  static const struct iio_chan_spec ad9434_channels[] = {
> >>> -	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
> >>> +	{
> >>> +		.type = IIO_VOLTAGE,
> >>> +		.indexed = 1,
> >>> +		.channel = 0,
> >>> +		.info_mask_shared_by_type =
> >>> +		BIT(IIO_CHAN_INFO_SCALE) |
> >>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> >>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
> >>
> >> Wrong indentation.
> > 
> > Can you please provide example of your preferred indentation for this
> > particular case? This is used in several places around the code and
> > seemed like one of the more readable.
> 
> Would this be the preferred indentation?

Almost LGTM, thanks.

> {
> 	.type = IIO_VOLTAGE,
> 	.indexed = 1,
> 	.channel = 0,
> 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> 				    BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> 				    BIT(IIO_CHAN_INFO_CALIBBIAS),
> 	.info_mask_shared_by_type_available =
> 		BIT(IIO_CHAN_INFO_SCALE) |
> 		BIT(IIO_CHAN_INFO_CALIBBIAS),

	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE) |
					      BIT(IIO_CHAN_INFO_CALIBBIAS),

It's still less than 80.

_OR_

rake the style consistent with the second one

	.info_mask_shared_by_type =
		BIT(IIO_CHAN_INFO_SCALE) |
		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
		BIT(IIO_CHAN_INFO_CALIBBIAS),

But I dunno which one is preferred. These two are fine with me.

> 	.scan_index = 0,
> 	.scan_type = {
> 		.sign = 's',
> 		.realbits = 12,
> 		.storagebits = 16,
> 	},
> },

> >>> +		.info_mask_shared_by_type_available =
> >>> +		BIT(IIO_CHAN_INFO_SCALE) |
> >>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),
> >>
> >> Ditto.
> >>
> >>> +		.scan_index = 0,
> >>> +		.scan_type = {
> >>> +			.sign = 's',
> >>> +			.realbits = 12,
> >>> +			.storagebits = 16,
> >>> +		},
> >>> +	},
> >>>  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

[Jonathan Cameron]

On Wed, 3 Dec 2025 11:04:08 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Dec 03, 2025 at 09:28:23AM +0200, Tomas Melin wrote:
> > On 02/12/2025 17:01, Tomas Melin wrote:  
> > > On 02/12/2025 16:11, Andy Shevchenko wrote:  
> > >> On Tue, Dec 02, 2025 at 12:53:09PM +0000, Tomas Melin wrote:  
> 
> ...
> 
> > >>>  static const struct iio_chan_spec ad9434_channels[] = {
> > >>> -	AD9467_CHAN(0, BIT(IIO_CHAN_INFO_SCALE), 0, 12, 's'),
> > >>> +	{
> > >>> +		.type = IIO_VOLTAGE,
> > >>> +		.indexed = 1,
> > >>> +		.channel = 0,
> > >>> +		.info_mask_shared_by_type =
> > >>> +		BIT(IIO_CHAN_INFO_SCALE) |
> > >>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > >>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),  
> > >>
> > >> Wrong indentation.  
> > > 
> > > Can you please provide example of your preferred indentation for this
> > > particular case? This is used in several places around the code and
> > > seemed like one of the more readable.  
> > 
> > Would this be the preferred indentation?  
> 
> Almost LGTM, thanks.
> 
> > {
> > 	.type = IIO_VOLTAGE,
> > 	.indexed = 1,
> > 	.channel = 0,
> > 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> > 				    BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > 				    BIT(IIO_CHAN_INFO_CALIBBIAS),
> > 	.info_mask_shared_by_type_available =
> > 		BIT(IIO_CHAN_INFO_SCALE) |
> > 		BIT(IIO_CHAN_INFO_CALIBBIAS),  
> 
> 	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE) |
> 					      BIT(IIO_CHAN_INFO_CALIBBIAS),
> 
> It's still less than 80.
> 
> _OR_
> 
> rake the style consistent with the second one
> 
> 	.info_mask_shared_by_type =
> 		BIT(IIO_CHAN_INFO_SCALE) |
> 		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> 		BIT(IIO_CHAN_INFO_CALIBBIAS),
> 
> But I dunno which one is preferred. These two are fine with me.

For the record, either of these is fine with me too. 
Pick one option and use it consistently for similar elements.

> 
> > 	.scan_index = 0,
> > 	.scan_type = {
> > 		.sign = 's',
> > 		.realbits = 12,
> > 		.storagebits = 16,
> > 	},
> > },  
> 
> > >>> +		.info_mask_shared_by_type_available =
> > >>> +		BIT(IIO_CHAN_INFO_SCALE) |
> > >>> +		BIT(IIO_CHAN_INFO_CALIBBIAS),  
> > >>
> > >> Ditto.
> > >>  
> > >>> +		.scan_index = 0,
> > >>> +		.scan_type = {
> > >>> +			.sign = 's',
> > >>> +			.realbits = 12,
> > >>> +			.storagebits = 16,
> > >>> +		},
> > >>> +	},
> > >>>  };  
>