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
