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

[David Lechner <dlechner@baylibre.com>]

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?