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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8edcbe15-b3cf-4259-9d07-87c07f1f400e@vaisala.com>
Date: Tue, 2 Dec 2025 16:52:01 +0200
From: Tomas Melin <tomas.melin@...sala.com>
To: Nuno Sá <noname.nuno@...il.com>,
 Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Nuno Sa <nuno.sa@...log.com>, Jonathan Cameron <jic23@...nel.org>,
 David Lechner <dlechner@...libre.com>, Andy Shevchenko <andy@...nel.org>,
 Alexandru Ardelean <alexandru.ardelean@...log.com>
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: adc: ad9467: support write/read offset

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