Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[Andy Shevchenko <andy@kernel.org>]

On Mon, Oct 14, 2024 at 12:40:40PM +0300, Antoniu Miclaus wrote:
> Add support for the AD485X a fully buffered, 8-channel simultaneous
> sampling, 16/20-bit, 1 MSPS data acquisition system (DAS) with
> differential, wide common-mode range inputs.

...

> +config AD4851
> +	tristate "Analog Device AD4851 DAS Driver"
> +	depends on SPI
> +	select REGMAP_SPI
> +	select IIO_BACKEND
> +	help
> +	  Say yes here to build support for Analog Devices AD4851, AD4852,
> +	  AD4853, AD4854, AD4855, AD4856, AD4857, AD4858, AD4858I high speed
> +	  data acquisition system (DAS).

I think I already commented on this... Anyway, it's much better to support when
this list is broke down on per device per line. In such a case it's less churn
if we need to remove or add an entry in the future.

> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad4851.

Also, with all these devices to be supported why not ad485x as the name of
the driver? Is it a preference by the IIO subsystem?

...

> +#include <asm/unaligned.h>

linux/unaligned nowadays (I learnt it quite recently).
(It requires v6.12-rc2).

...

> +struct ad4851_chip_info {

Have you run `pahole`? It seems you may reduce the memory footprint of this
structure.

> +	const char *name;
> +	unsigned int product_id;
> +	const unsigned int (*scale_table)[2];
> +	int num_scales;
> +	const int *offset_table;
> +	int num_offset;
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
> +	unsigned long throughput;
> +	unsigned int resolution;
> +};

...

> +static const int ad4851_oversampling_ratios[] = {
> +	1,
> +	2,
> +	4,
> +	8,
> +	16,
> +	32,
> +	64,
> +	128,
> +	256,
> +	512,
> +	1024,
> +	2048,
> +	4096,
> +	8192,
> +	16384,
> +	32768,
> +	65536,

I believe you can compact them to be 4 or 8 per line

	1, 2, 4, 8, 16, 32, 64, 128,			/* 0-7 */
	256, 512, 1024, 2048, 4096, 8192, 16384, 32768,	/* 8-15 */
	65536,						/* 16 */

> +};

...

> +static int ad4851_osr_to_regval(int ratio)
> +{
> +	int i;
> +
> +	for (i = 1; i < ARRAY_SIZE(ad4851_oversampling_ratios); i++)
> +		if (ratio == ad4851_oversampling_ratios[i])
> +			return i - 1;
> +
> +	return -EINVAL;
> +}
> +
> +static int ad4851_set_oversampling_ratio(struct ad4851_state *st,
> +					 const struct iio_chan_spec *chan,
> +					 unsigned int osr)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	if (osr == 1) {
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_EN_MSK, 0);
> +		if (ret)
> +			return ret;
> +	} else {

0 is listed here. Is it a problem?

> +		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_EN_MSK, AD4851_OS_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		val = ad4851_osr_to_regval(osr);
> +		if (val < 0)
> +			return -EINVAL;
> +
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_RATIO_MSK, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (chan->scan_type.realbits) {
> +	case 20:
> +		switch (osr) {
> +		case 1:
> +			val = 20;
> +			break;
> +		default:

Ditto.

> +			val = 24;
> +			break;
> +		}
> +		break;
> +	case 16:
> +		val = 16;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = iio_backend_data_size_set(st->back, val);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> +				  AD4851_PACKET_FORMAT_MASK, (osr == 1) ? 0 : 1);

I would do it with a conditional

	if (osr ...)
		return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
					  AD4851_PACKET_FORMAT_MASK, 0);

	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
				  AD4851_PACKET_FORMAT_MASK, 1);

But looking at the above I would split this to three functions, that outer will
look like

int ...(...)
{
	if (osr ...)
		return _osr_X(...);
	return _osr_Y(...);
}

> +}

...

> +static int ad4851_find_opt(bool *field, u32 size, u32 *ret_start)
> +{
> +	unsigned int i, cnt = 0, max_cnt = 0, max_start = 0;
> +	int start;
> +
> +	for (i = 0, start = -1; i < size; i++) {
> +		if (field[i] == 0) {
> +			if (start == -1)
> +				start = i;
> +			cnt++;
> +		} else {
> +			if (cnt > max_cnt) {
> +				max_cnt = cnt;
> +				max_start = start;
> +			}
> +			start = -1;
> +			cnt = 0;
> +		}
> +	}

This magic has to be commented... I have a déjà vu that I have commented on all
this, but it hasn't been addressed!

> +	if (cnt > max_cnt) {
> +		max_cnt = cnt;
> +		max_start = start;
> +	}
> +
> +	if (!max_cnt)
> +		return -ENOENT;
> +
> +	*ret_start = max_start;
> +
> +	return max_cnt;
> +}

Also the cover letter is missing.

I would recommend you to use my "smart" script [1] for sending series, it has
some good heuristics on whom to include into the email thread and handles
missed cover letters for the series.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko