linux-kernel - Re: [PATCH v4 4/4] iio: adc: update ad7779 to use IIO backend

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: <20250825111550.2af1ed7b@jic23-huawei>
Date: Mon, 25 Aug 2025 11:15:50 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Ioana Risteiu <Ioana.Risteiu@...log.com>, Lars-Peter Clausen
 <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, David
 Lechner <dlechner@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Ramona Nechita <ramona.nechita@...log.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 4/4] iio: adc: update ad7779 to use IIO backend

On Wed, 20 Aug 2025 17:18:05 +0300
Andy Shevchenko <andriy.shevchenko@...el.com> wrote:

> On Wed, Aug 20, 2025 at 03:02:45PM +0300, Ioana Risteiu wrote:
> > Add a new functionality to ad7779 driver that streams data through data
> > output interface using IIO backend interface.  

A few follow ups from me.

Jonathan

> ...
> 
> > +static int ad7779_set_data_lines(struct iio_dev *indio_dev,
> > +				 unsigned int num_lanes)
> > +{
> > +	struct ad7779_state *st = iio_priv(indio_dev);  
> 
> > +	int ret = -EINVAL;  
> 
> In general the split assignment is easier to maintain and less prone to subtle
> errors. In this case it's even worse as it's not needed...
> 
> > +	if (num_lanes != AD7779_1LINE &&
> > +		num_lanes != AD7779_2LINES &&
> > +		num_lanes != AD7779_4LINES)
> > +		return ret;  
> 
> ...just return the error code directly here.

Those enum values in general seem like a bad idea.  Just use numbers.
If the enum was used as a type it would be a different situation but here it
is used as a glorified way to give the 1 lane to the value 1 etc which just
makes it harder to read.


> 
> > +	ret = ad7779_set_sampling_frequency(st, num_lanes * AD7779_DEFAULT_SAMPLING_1LINE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = iio_backend_num_lanes_set(st->back, num_lanes);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT,
> > +				    AD7779_DOUT_FORMAT_MSK,
> > +				    FIELD_PREP(AD7779_DOUT_FORMAT_MSK, 2 - ilog2(num_lanes)));
> > +}  
> 
> ...
> 
> > +static int ad7779_setup_channels(struct iio_dev *indio_dev, const struct ad7779_state *st)
> > +{
> > +	struct iio_chan_spec *channels;
> > +	struct device *dev = &st->spi->dev;
> > +
> > +	channels = devm_kmemdup_array(dev, st->chip_info->channels,
> > +					ARRAY_SIZE(ad7779_channels),
> > +					sizeof(*channels), GFP_KERNEL);  
> 
> Indentation...
> 
> > +	if (!channels)
> > +		return -ENOMEM;
> > +
> > +	for (int i = 0; i < ARRAY_SIZE(ad7779_channels); i++)  
> 
> Why signed iterator?
> 
> > +		channels[i].scan_type.endianness = IIO_CPU;
> > +
> > +	indio_dev->channels = channels;
> > +
> > +	return 0;
> > +}  
> 
> ...
> 
> > +static int ad7779_setup_backend(struct ad7779_state *st, struct iio_dev *indio_dev)
> > +{
> > +	struct device *dev = &st->spi->dev;  
> 
> > +	int ret = -EINVAL;  
> 
> Why?!
> 
> > +	int num_lanes;  
> 
> Can it be negatie?

Good spot.

Even beyond sign it needs to be a u32 given how it is used.

> 
> > +	indio_dev->info = &ad7779_info_data;
> > +
> > +	ret = ad7779_setup_channels(indio_dev, st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->back = devm_iio_backend_get(dev, NULL);
> > +	if (IS_ERR(st->back))
> > +		return dev_err_probe(dev, PTR_ERR(st->back),
> > +				     "failed to get iio backend");
> > +
> > +	ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_iio_backend_enable(dev, st->back);
> > +	if (ret)
> > +		return ret;
> > +
> > +	num_lanes = 4;
> > +	ret = device_property_read_u32(dev, "adi,num-lanes", &num_lanes);
> > +	if (ret && ret != -EINVAL)
> > +		return ret;
> > +
> > +	return ad7779_set_data_lines(indio_dev, num_lanes);
> > +}  
>