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

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

On Tue, Nov 18, 2025 at 02:32:43PM -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.
> 
> Add basic support for AD4134 that enables single-shot ADC sample read.

Thanks for an update! My comments below.

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/bits.h>

bitops.h implies bits.h and the latter can be omitted. But it's up to you, I'm
fine with the current way.

> +#include <linux/clk.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>

...

> +struct ad4134_state {

> +	struct spi_device *spi;
> +	struct device *dev;
> +	struct regmap *regmap;

We have a duplication here, I believe. Either struct device or struct regmap
may be dropped. If it's not the case, the fields needs a good description /
justification.

> +	unsigned long sys_clk_hz;
> +	struct gpio_desc *odr_gpio;
> +	int refin_mv;
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the transfer buffers
> +	 * to live in their own cache lines.
> +	 */
> +	u8 rx_buf[AD4134_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> +	u8 tx_buf[AD4134_SPI_MAX_XFER_LEN];
> +};

...

> +static int ad4134_regulator_setup(struct ad4134_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	bool use_internal_ldo_regulator;
> +	int ret;
> +
> +	/* Required regulators */
> +	ret = devm_regulator_bulk_get_enable(dev, 3, ad4143_regulator_names);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable power supplies\n");
> +
> +	/* Required regulator that we need to read the voltage */
> +	ret = devm_regulator_get_enable_read_voltage(dev, "refin");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to get REFIN voltage.\n");

> +	st->refin_mv = ret / MILLI;

I think the divisor should be (MICRO / MILLI). This what Jonathan suggested
last time I remember (in some other reviews).

> +	/*
> +	 * If ldoin is not provided, then avdd1v8, dvdd1v8, and clkvdd are
> +	 * required.
> +	 */
> +	ret = devm_regulator_get_enable_optional(dev, "ldoin");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to enable ldoin supply\n");
> +
> +	use_internal_ldo_regulator = ret == 0;

> +	if (!use_internal_ldo_regulator) {

Can it be

	if (use_internal_ldo_regulator)
		return 0;

with the dropped indentation level for the below?


> +		ret = devm_regulator_get_enable(dev, "avdd1v8");
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to enable avdd1v8 supply\n");
> +
> +		ret = devm_regulator_get_enable(dev, "dvdd1v8");
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to enable dvdd1v8 supply\n");
> +
> +		ret = devm_regulator_get_enable(dev, "clkvdd");
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to enable clkvdd supply\n");
> +	}
> +
> +	return 0;
> +}

...

> +static int ad4134_clock_select(struct ad4134_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct clk *sys_clk;
> +	int ret;

> +	sys_clk = devm_clk_get_optional_enabled(dev, "xtal1-xtal2");

> +	if (IS_ERR_OR_NULL(sys_clk)) {

I don't understand the choice of _optional() with the _NULL here.
Also note, we missed the deferred probe case here.

Was it me who suggested this? :-)

One solution may be to check the clock name presence in the clock-names or
whatever the name of that DT property before trying to get this clock.

> +		ret = PTR_ERR_OR_ZERO(sys_clk);
> +		sys_clk = devm_clk_get_enabled(dev, "clkin");
> +		if (IS_ERR(sys_clk))

Yeah, v1 might look better, but I don't remember by heart.

> +			return dev_err_probe(dev, PTR_ERR(sys_clk),
> +					     "failed to get xtal1-xtal2: %d, clkin: %ld\n",
> +					     ret, PTR_ERR(sys_clk));

No need to have a duplicated error code being printed.

> +	}
> +
> +	st->sys_clk_hz = clk_get_rate(sys_clk);
> +	if (st->sys_clk_hz != AD4134_EXT_CLOCK_MHZ)
> +		dev_warn(dev, "invalid external clock frequency %lu\n",
> +			 st->sys_clk_hz);
> +
> +	return 0;
> +}

...

> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(reset_gpio),
> +				     "failed to find reset GPIO\n");
> +
> +	if (reset_gpio) {
> +		fsleep(AD4134_RESET_TIME_US);

> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +	}

Can we use reset-gpio driver instead of custom implementation?

-- 
With Best Regards,
Andy Shevchenko