linux-kernel - Re: [PATCH v2 3/3] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aJO8thsrIgS3YGj2@smile.fi.intel.com>
Date: Wed, 6 Aug 2025 23:36:06 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Ioana Risteiu <Ioana.Risteiu@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Jonathan Cameron <jic23@...nel.org>,
	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 v2 3/3] iio: adc: update ad7779 to use IIO backend

On Wed, Aug 06, 2025 at 10:25:00PM +0300, Ioana Risteiu wrote:
> Add a new functionality to ad7779 driver that streams data through data
> output interface using IIO backend interface.

...

> +enum ad7779_data_lines {
> +	AD7779_4LINES = 4,
> +	AD7779_2LINES = 2,
> +	AD7779_1LINE = 1,

Reversed ordering in enum is not a thing I see everyday in C, maybe a bit
confusing to the reader.

> +};

Seems like it's 1:1, why enum at all?

...

> -#define AD777x_CHAN_S(index, _ext_info)					\
> +#define AD777X_CHAN_S(index, _ext_info)					\
>  	{								\
>  		.type = IIO_VOLTAGE,					\
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE)  |	\

Unneeded change.

...

> -#define AD777x_CHAN_NO_FILTER_S(index)					\
> -	AD777x_CHAN_S(index, NULL)
> +#define AD777X_CHAN_NO_FILTER_S(index)					\
> +	AD777X_CHAN_S(index, NULL)

Why?!

> +#define AD777X_CHAN_FILTER_S(index)					\
> +	AD777X_CHAN_S(index, ad7779_ext_filter)
>  
> -#define AD777x_CHAN_FILTER_S(index)					\
> -	AD777x_CHAN_S(index, ad7779_ext_filter)

Ditto.

>  static const struct iio_chan_spec ad7779_channels[] = {
> -	AD777x_CHAN_NO_FILTER_S(0),
> -	AD777x_CHAN_NO_FILTER_S(1),
> -	AD777x_CHAN_NO_FILTER_S(2),
> -	AD777x_CHAN_NO_FILTER_S(3),
> -	AD777x_CHAN_NO_FILTER_S(4),
> -	AD777x_CHAN_NO_FILTER_S(5),
> -	AD777x_CHAN_NO_FILTER_S(6),
> -	AD777x_CHAN_NO_FILTER_S(7),
> +	AD777X_CHAN_NO_FILTER_S(0),
> +	AD777X_CHAN_NO_FILTER_S(1),
> +	AD777X_CHAN_NO_FILTER_S(2),
> +	AD777X_CHAN_NO_FILTER_S(3),
> +	AD777X_CHAN_NO_FILTER_S(4),
> +	AD777X_CHAN_NO_FILTER_S(5),
> +	AD777X_CHAN_NO_FILTER_S(6),
> +	AD777X_CHAN_NO_FILTER_S(7),
>  	IIO_CHAN_SOFT_TIMESTAMP(8),
>  };

Ditto.

>  static const struct iio_chan_spec ad7779_channels_filter[] = {
> -	AD777x_CHAN_FILTER_S(0),
> -	AD777x_CHAN_FILTER_S(1),
> -	AD777x_CHAN_FILTER_S(2),
> -	AD777x_CHAN_FILTER_S(3),
> -	AD777x_CHAN_FILTER_S(4),
> -	AD777x_CHAN_FILTER_S(5),
> -	AD777x_CHAN_FILTER_S(6),
> -	AD777x_CHAN_FILTER_S(7),
> +	AD777X_CHAN_FILTER_S(0),
> +	AD777X_CHAN_FILTER_S(1),
> +	AD777X_CHAN_FILTER_S(2),
> +	AD777X_CHAN_FILTER_S(3),
> +	AD777X_CHAN_FILTER_S(4),
> +	AD777X_CHAN_FILTER_S(5),
> +	AD777X_CHAN_FILTER_S(6),
> +	AD777X_CHAN_FILTER_S(7),
>  	IIO_CHAN_SOFT_TIMESTAMP(8),
>  };

Ditto.

...

> +static int ad7779_conf_channels(struct iio_dev *indio_dev, const struct ad7779_state *st)
> +{
> +	struct iio_chan_spec *channels;
> +	struct device *dev = &st->spi->dev;
> +	int num_channels = ARRAY_SIZE(ad7779_channels);
> +
> +	channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);

Doesn't sound like a right place for devm. Is this function called only at probe stage?

> +	if (!channels)
> +		return -ENOMEM;
> +
> +	memcpy(channels, st->chip_info->channels, num_channels * sizeof(struct iio_chan_spec));

kmemdup_array()?

> +	for (int i = 0; i < 8; i++)

Why i is signed?
What is the magic 8? ARRAY_SIZE()?

> +		channels[i].scan_type.endianness = IIO_CPU;
> +
> +	indio_dev->channels = channels;
> +
> +	return 0;
> +}

...

> +	if (ret)
> +		return ret;
> +
> +	return 0;

return ...


...

> +	st->back = devm_iio_backend_get(dev, NULL);
> +	if (IS_ERR(st->back)) {
> +		dev_err_probe(dev, ret, "failed to get iio backend");

Huh?!

> +		return PTR_ERR(st->back);

Pattern is

		return dev_err_probe(...);

> +	}

-- 
With Best Regards,
Andy Shevchenko