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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 2 Dec 2025 17:55:21 -0300
Marcelo Schmitt <marcelo.schmitt@analog.com> 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.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
Hi Marcelo

Nice and clean which makes for pleasant reviewing :)
A few minor comments inline.

Jonathan

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> new file mode 100644
> index 000000000000..7158eefcd877
> --- /dev/null
> +++ b/drivers/iio/adc/ad4134.c


> +static int ad4134_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long info)
> +{
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		gpiod_set_value_cansleep(st->odr_gpio, 1);
> +		fsleep(1);
Any section reference for required pulse width that you can add as a comment here?

It's useful if people end up wondering if the pulse is long enough if they have
problems with a board.

> +		gpiod_set_value_cansleep(st->odr_gpio, 0);
> +		ret = regmap_read(st->regmap, AD4134_CH_VREG(chan->channel), val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->refin_mv;
> +		*val2 = AD4134_CHAN_PRECISION_BITS - 1;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static const char * const ad4143_regulator_names[] = {
> +	"avdd5", "dvdd5", "iovdd", "refin",
> +	"avdd1v8", "dvdd1v8", "clkvdd",	"ldoin",
> +};
> +
> +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);

Why list names of regulators in that array that you don't use? I'd call it
ad4143_required_regulator_names and then you can use ARRAY_SIZE() to replace
that 3.

> +	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 / (MICRO / MILLI);
> +
> +	/*
> +	 * 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)
> +		return 0;
> +

For these 3 you can use a second array of names and devm_regulator_bulk_get_enable()
Finding a short descriptive name for that array might be tricky however.

> +	ret = devm_regulator_get_enable(dev, "avdd1v8");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to enable avdd1v8 supply\n");
> +
> +	ret = devm_regulator_get_enable(dev, "dvdd1v8");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to enable dvdd1v8 supply\n");
> +
> +	ret = devm_regulator_get_enable(dev, "clkvdd");
> +	if (ret)
> +		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;
> +
> +	/*
> +	 * AD4134 requires one external clock source and only one external clock
> +	 * source can be provided at a time. Try get a crystal provided clock.
> +	 * If that fails, try to get a CMOS clock.
> +	 */
> +	sys_clk = devm_clk_get_optional_enabled(dev, "xtal1-xtal2");

I should have noticed this in the binding review.  Why do we need to call out which
xtal pins?  Previous cases of this have just used the name "xtal" for the clock
type selection.  Maybe there was some discussion I missed.

> +	if (IS_ERR_OR_NULL(sys_clk)) {
> +		if (PTR_ERR(sys_clk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		/* Try the CMOS clock */
> +		sys_clk = devm_clk_get_enabled(dev, "clkin");
> +		if (IS_ERR(sys_clk)) {
> +			if (PTR_ERR(sys_clk) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +
> +			return dev_err_probe(dev, PTR_ERR(sys_clk),
> +					     "failed to get external clock\n");
> +		}
> +	}
> +
> +	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;
> +}