linux-kernel - Re: [PATCH v3 11/11] iio: adc: ad4080: add driver support

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] [day] [month] [year] [list]
Message-ID: <c6e1c555-37c0-484c-a955-39163cd148bb@baylibre.com>
Date: Wed, 30 Apr 2025 09:11:56 -0500
From: David Lechner <dlechner@...libre.com>
To: "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>
Cc: "jic23@...nel.org" <jic23@...nel.org>, "robh@...nel.org"
 <robh@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
 "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 11/11] iio: adc: ad4080: add driver support

On 4/30/25 7:31 AM, Miclaus, Antoniu wrote:
> ...
>>> +	unsigned int num_channels;
>>> +};
>>
>> I guess this is preparing the driver to support more than one chip?
>>
> Yes. It is stated also in the cover letter.
>>> +
>>> +struct ad4080_state {
>>> +	struct spi_device		*spi;
>>
>> It looks like this is only ever used to get &spi->dev. We could drop this and
>> get dev from regmap instead.
> How can I get the dev from regmap?

struct device *dev = regmap_get_device(regmap);

>>> +	struct regmap			*regmap;
>>> +	struct clk			*clk;
>>> +	struct iio_backend		*back;
>>> +	const struct ad4080_chip_info	*info;
>>> +	/*
>>> +	 * Synchronize access to members the of driver state, and ensure
>>> +	 * atomicity of consecutive regmap operations.
>>> +	 */
>>> +	struct mutex			lock;
>>> +	unsigned int			num_lanes;
>>> +	unsigned int			dec_rate;
>>> +	enum ad4080_filter_type		filter_type;
>>> +	bool				lvds_cnv_en;
>>> +};
>>> +
>>> +static const struct regmap_config ad4080_regmap_config = {
>>> +	.reg_bits = 16,
>>> +	.val_bits = 8,
>>> +	.read_flag_mask = BIT(7),
>>> +	.max_register = 0x29,
>>> +};
>>> +
>>> +static int ad4080_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>>> +			     unsigned int writeval, unsigned int *readval)
>>> +{
>>> +	struct ad4080_state *st = iio_priv(indio_dev);
>>> +
>>
>> Missing guard(mutex)(&st->lock); ?
> Aren't regmap operations thread safe? (own internal locking).

For single operations, yes. But I assumed that you added the lock so that when
functions that do multiple regmap read/writes don't have another thread doing
a different regmap operation in the middle.

However, it looks like ad4080_lvds_sync_write() is currently the only function
like this and it is only called during probe. So it seems like the extra mutex
lock isn't currently needed and could be removed from the driver entirely.

>>> +	if (readval)
>>> +		return regmap_read(st->regmap, reg, readval);
>>> +
>>> +	return regmap_write(st->regmap, reg, writeval);
>>> +}


...

>>> +
>>> +static int ad4080_set_dec_rate(struct iio_dev *dev,
>>> +			       const struct iio_chan_spec *chan,
>>> +			       unsigned int mode)
>>> +{
>>> +	struct ad4080_state *st = iio_priv(dev);
>>> +	int ret;
>>> +
>>
>> Don't we need to check for < 2 as well?
>>
>>> +	if (st->filter_type >= SINC_5 && mode >= 512)
>>> +		return -EINVAL;
>>> +
>>> +	guard(mutex)(&st->lock);
>>> +	ret = regmap_update_bits(st->regmap, AD4080_REG_FILTER_CONFIG,
>>> +
>> AD4080_FILTER_CONFIG_SINC_DEC_RATE_MSK,
>>> +
>> FIELD_PREP(AD4080_FILTER_CONFIG_SINC_DEC_RATE_MSK,
>>> +					    (ilog2(mode) - 1)));
>>
>> Otherwise ilog2(mode) - 1 could be < 0.
>>
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	st->dec_rate = mode;
>>
>> This saves the value the user entered, not what the hardware is actually doing.
>> It should be saving the power of 2 value instead.
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int ad4080_read_raw(struct iio_dev *indio_dev,
>>> +			   struct iio_chan_spec const *chan,
>>> +			   int *val, int *val2, long m)
>>> +{
>>> +	struct ad4080_state *st = iio_priv(indio_dev);
>>> +	int dec_rate;
>>> +
>>> +	switch (m) {
>>> +	case IIO_CHAN_INFO_SCALE:
>>> +		return ad4080_get_scale(st, val, val2);
>>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>>> +		if (st->filter_type == SINC_5_COMP)
>>> +			dec_rate = st->dec_rate * 2;
>>> +		else
>>> +			dec_rate = st->dec_rate;
>>
>> As a concequence of the above, this will return incorrect information if the
>> user didn't enter an exact value.
> Isn't the user constrained by the ad4080_read_avail for entering the dec_rate values?
> The user both writes and reads the actual decimation rate value. The conversions are done inside the functions.

Yes, the oversampling_ratio attribute is calling ad4080_get_dec_rate(), so
that one is OK, but this is the sampling_frequency attribute. Currently
st->dec_rate holds the user-requested value and isn't necessarily the same as
the value that would be returned by ad4080_get_dec_rate(indio_dev, chan).

If we dropped st->dec_rate and used ad4080_get_dec_rate(indio_dev, chan) here
too, that would be an acceptable solution too.

>>
>>> +		if (st->filter_type)
>>> +			*val = DIV_ROUND_CLOSEST(clk_get_rate(st->clk),
>> dec_rate);
>>> +		else
>>> +			*val = clk_get_rate(st->clk);
>>> +		return IIO_VAL_INT;
>>> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>> +		*val = ad4080_get_dec_rate(indio_dev, chan);
>>> +		return IIO_VAL_INT;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +}

...

>>> +static const struct iio_chan_spec ad4080_channels[] = {
>>
>> Array with one element doesn't make sense. It can just be a single struct.
> Isn't indio_dev->channels expecting an array?

No, it expects a pointer. So &ad4080_channel; can be used to get a pointer to
a single struct instance.