| 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: <aRHk6KZqQP-PAFuY@smile.fi.intel.com>
Date: Mon, 10 Nov 2025 15:13:12 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
jic23@...nel.org, nuno.sa@...log.com, dlechner@...libre.com,
andy@...nel.org, Michael.Hennerich@...log.com, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, corbet@....net,
cosmin.tanislav@...log.com, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v1 2/3] iio: adc: Initial support for AD4134
On Mon, Nov 10, 2025 at 09:45:40AM -0300, Marcelo Schmitt wrote:
> AD4134 is a 24-bit, 4-channel, simultaneous sampling, precision
> analog-to-digital converter (ADC). The device can be managed through SPI or
> direct control of pin logical levels (pin control mode). The AD4134 design
> also features a dedicated bus for ADC sample data output. Though, this
> initial driver for AD4134 only supports usual SPI connections.
>
> The different wiring configurations will likely require distinct software
> to handle.
> So, the code specific to SPI is enclosed in ad4134-spi.c, while
> functionality that may be useful to all wiring configuration is set into
> ad4134-common.h and ad4134-common.c.
This part is good for comment or cover letter, I dunno what it gives to the Git
history. Perhaps you want to rephrase it somehow?
> Add basic support for AD4134 that allows single-shot ADC sample read.
Below is my review based on the assumption that there is a good justification
for a brand new driver.
...
> +obj-$(CONFIG_AD4134_SPI) += ad4134-spi.o
This can be split also to the separate patch.
...
Please, follow IWYU principle, many are missing here.
+ array_size.h
+ bitfield.h
> +#include <linux/clk.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
I don't see the use of this, rather dev_printk.h should be put here.
> +#include <linux/err.h>
+ export.h
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
+ types.h
...
> +static const char * const ad4143_regulator_names[] = {
> + "avdd5", "dvdd5", "iovdd", "refin", /* Required supplies */
> + "avdd1v8", "dvdd1v8", "clkvdd", /* Required if ldoin not provided */
> + "ldoin",
Make them equal in count (2 lines by 4 in each sounds really good choice
to me).
> +};
...
> +static const char *const ad4134_clk_sel[] = {
> + "xtal1-xtal2", "clkin"
Leave trailing comma here.
> +};
...
> +#define __DRIVERS_IIO_ADC_AD4134_COMMON_H__
Do we need DRIVERS_ part?
...
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/compiler_attributes.h>
No need when we have types.h listed.
> +#include <linux/crc8.h>
Is this being used?
> +#include <linux/iio/iio.h>
+ regmap.h
> +#include <linux/units.h>
> +#include <linux/types.h>
Again, follow IWYU.
...
> +#define AD4134_RESET_TIME_US (10 * MICRO)
We have USEC_PER_SEC (include time.h for that).
...
> +#define AD4134_EXT_CLOCK_MHZ (48 * MEGA)
We have HZ_PER_MHZ.
...
> +#define AD4134_SCAN_TYPE(_realbits, _storebits) { \
> + .sign = 's', \
> + .realbits = (_realbits), \
> + .storagebits = (_storebits), \
> + .shift = ((_storebits) - (_realbits)), \
> + .endianness = IIO_BE \
Missing comma, this might make an addition churn in the future changes.
> +}
...
> +struct device;
What about struct gpio_desc?
...
> +#endif /* __DRIVERS_IIO_ADC_AD4134_COMMON_H__ */
...
+ array_size.h
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
> +#include <linux/crc8.h>
> +#include <linux/device.h>
Is it being used? Perhaps dev_printk.h is enough?
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +#include <linux/module.h>
+ mod_devicetable.h
> +#include <linux/spi/spi.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
...
> +static int ad4134_calc_spi_crc(u8 inst, u8 data)
> +{
> + u8 buf[] = {inst, data};
Better
u8 buf[] = { inst, data };
> + return crc8(ad4134_spi_crc_table, buf, ARRAY_SIZE(buf),
> + AD4134_SPI_CRC_INIT_VALUE);
> +}
...
> +static const struct spi_device_id ad4134_id[] = {
> + { "ad4134", (kernel_ulong_t)&ad4134_min_io_bus_info },
> + { },
No comma for the terminator entry. It's even inconsistent with the below ID
table.
> +};
> +MODULE_DEVICE_TABLE(spi, ad4134_id);
> +
> +static const struct of_device_id ad4134_of_match[] = {
> + { .compatible = "adi,ad4134", .data = &ad4134_min_io_bus_info },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad4134_of_match);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists