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

[Tomas Melin <tomas.melin@vaisala.com>]

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á
>