Re: [PATCH v1 2/3] iio: adc: Initial support for AD4134

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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