[v2] iio: adc: ad9467: fixes for ad9434

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

Posted by Tomas Melin 2 months, 1 week ago

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

Posted by Andy Shevchenko 2 months, 1 week ago

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 2/2] iio: adc: ad9467: support write/read offset

Posted by Tomas Melin 2 months, 1 week ago


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

Posted by Tomas Melin 2 months, 1 week ago


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

Posted by Andy Shevchenko 2 months, 1 week ago

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

Posted by Jonathan Cameron 2 months ago

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,
> > >>> +		},
> > >>> +	},
> > >>>  };  
>

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

Posted by Nuno Sá 2 months, 1 week ago

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 2/2] iio: adc: ad9467: support write/read offset

Posted by Andy Shevchenko 2 months, 1 week ago

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

Posted by Nuno Sá 2 months, 1 week ago

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 2/2] iio: adc: ad9467: support write/read offset

Posted by Tomas Melin 2 months, 1 week ago

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

Posted by Nuno Sá 2 months, 1 week ago

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

Posted by David Lechner 2 months, 1 week ago

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

Posted by Tomas Melin 2 months, 1 week ago


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