| 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
| ||
|
Message-ID: <Zw0ZM0vQXJep3dFJ@smile.fi.intel.com>
Date: Mon, 14 Oct 2024 16:14:27 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Nuno Sa <nuno.sa@...log.com>,
Olivier Moysan <olivier.moysan@...s.st.com>,
Uwe Kleine-König <ukleinek@...nel.org>,
David Lechner <dlechner@...libre.com>,
Marcelo Schmitt <marcelo.schmitt@...log.com>,
Ivan Mikhaylov <fr0st61te@...il.com>,
Marius Cristea <marius.cristea@...rochip.com>,
Dumitru Ceclan <mitrutzceclan@...il.com>,
João Paulo Gonçalves <joao.goncalves@...adex.com>,
Alisa-Dariana Roman <alisadariana@...il.com>,
Mike Looijmans <mike.looijmans@...ic.nl>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Sergiu Cuciurean <sergiu.cuciurean@...log.com>,
Dragos Bogdan <dragos.bogdan@...log.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pwm@...r.kernel.org
Subject: Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver
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
Powered by blists - more mailing lists