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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 10 Nov 2025 09:45:40 -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.
> 
> 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.

'maybe' isn't usually a justification for a split.  If that code
was on list even as an RFC before merging  I'd be fine with this, but if it is
something we might never see upstream, then squash the abstractions for
now. Those then end up being introduced as a precursor part of the patch
set that gives them a reason to exist.

> 
> Add basic support for AD4134 that allows single-shot ADC sample read.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
A few other comments inline,

Thanks, J
> diff --git a/drivers/iio/adc/ad4134-common.c b/drivers/iio/adc/ad4134-common.c
> new file mode 100644
> index 000000000000..05332a640926
> --- /dev/null
> +++ b/drivers/iio/adc/ad4134-common.c

> +
> +static const char *const ad4134_clk_sel[] = {
> +	"xtal1-xtal2", "clkin"
> +};
> +
> +static int ad4134_clock_select(struct ad4134_state *st)
> +{
> +	struct device *dev = st->dev;
> +	struct clk *sys_clk;
> +	int ret;
> +
> +	ret = device_property_match_property_string(dev, "clock-names",
> +						    ad4134_clk_sel,
> +						    ARRAY_SIZE(ad4134_clk_sel));
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to find external clock\n");
> +
> +	sys_clk = devm_clk_get_enabled(dev, ad4134_clk_sel[ret]);
> +	if (IS_ERR(sys_clk))
> +		return dev_err_probe(dev, PTR_ERR(sys_clk),
> +				     "failed to get %s external clock\n",
> +				     ad4134_clk_sel[ret]);
This is a somewhat unusual approach. More common to just trying getting
an optional clock and if that fails try the other one.

devm_clk_get_optional_enabled()


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

> diff --git a/drivers/iio/adc/ad4134-common.h b/drivers/iio/adc/ad4134-common.h
> new file mode 100644
> index 000000000000..c0a553d827c9
> --- /dev/null
> +++ b/drivers/iio/adc/ad4134-common.h

> +
> +#define AD4134_CH_VREG(x)			((x) + 0x50) /* chanX virtual register */
> +#define AD4134_VREG_CH(x)			((x) - 0x50) /* chan of virtual reg X */

Add a comment or two on what virtual registers are for.

> +struct iio_scan_type ad4134_scan_types[] = {
> +	AD4134_SCAN_TYPE(16, 16),
> +	AD4134_SCAN_TYPE(16, 24),

There are no buffer in here so can type ends up meaning little.
If this eventually doesn't become useful, storage bits must be a power of 2 * 8
So can't be 24.  

> +	AD4134_SCAN_TYPE(24, 24),
> +	AD4134_SCAN_TYPE(24, 32),
> +};
> +
> +#define AD4134_CHANNEL(_index) {						\
> +	.type = IIO_VOLTAGE,							\
> +	.indexed = 1,								\
> +	.channel = (_index),							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),				\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),			\
> +	.scan_index = (_index),							\
> +	.has_ext_scan_type = 1,							\
> +	.ext_scan_type = ad4134_scan_types,					\
> +	.num_ext_scan_type = ARRAY_SIZE(ad4134_scan_types)			\
> +}

> diff --git a/drivers/iio/adc/ad4134-spi.c b/drivers/iio/adc/ad4134-spi.c
> new file mode 100644
> index 000000000000..7d0749e5c084
> --- /dev/null
> +++ b/drivers/iio/adc/ad4134-spi.c
> @@ -0,0 +1,287 @@

> +
> +#include "ad4134-common.h"

> +static int ad4134_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct ad4134_state *st = context;
> +	struct spi_device *spi = to_spi_device(st->dev);
> +	struct spi_transfer xfer = {
> +		.tx_buf = st->tx_buf,
> +		.rx_buf = st->rx_buf,
> +		.len = AD4134_SPI_MAX_XFER_LEN,
> +	};
> +	int ret;
> +
> +	ad4134_prepare_spi_tx_buf(reg, val, st->tx_buf);
> +
> +	ret = spi_sync_transfer(spi, &xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (st->rx_buf[2] != st->tx_buf[2])
> +		dev_dbg(st->dev, "reg write CRC check failed\n");
> +
> +	return 0;
> +}
> +
> +static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
> +			    unsigned int *val)
> +{
> +	struct spi_device *spi = to_spi_device(st->dev);
> +	struct iio_scan_type *scan_type = &ad4134_scan_types[st->current_scan_type];
> +	unsigned int i;
> +	int ret;
> +
> +	/*
> +	 * Data from all four channels is serialized and output on SDO. Read
> +	 * them all but keep only the requested data.

I'm failing to spot this mode described on the datasheet.  Could you
provide a reference section?

> +	 */
> +	for (i = 0; i < ARRAY_SIZE(ad4134_chan_set); i++) {
> +		ret = spi_write_then_read(spi, NULL, 0, st->rx_buf,
> +					  BITS_TO_BYTES(scan_type->storagebits));
> +		if (ret)
> +			return ret;
> +
> +		if (i != AD4134_VREG_CH(reg))
> +			continue;
> +
> +		if (scan_type->realbits == 16)
> +			*val = get_unaligned_be16(st->rx_buf);
> +		else
> +			*val = get_unaligned_be24(st->rx_buf);
> +
> +		*val >>= scan_type->shift;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad4134_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct ad4134_state *st = context;
> +	struct spi_device *spi = to_spi_device(st->dev);
> +	struct spi_transfer xfer = {
> +		.tx_buf = st->tx_buf,
> +		.rx_buf = st->rx_buf,
> +		.len = AD4134_SPI_MAX_XFER_LEN,
> +	};
> +	unsigned int inst;
> +	int ret;
> +
> +	if (reg >= AD4134_CH_VREG(0))
> +		return ad4134_data_read(st, reg, val);

If you are going down this path the xfer isn't used.  To avoid that being
a little confusing I'd factor out the rest of this function into a helper

> +
> +	inst = AD4134_REG_READ_MASK | reg;
> +	ad4134_prepare_spi_tx_buf(inst, 0, st->tx_buf);
> +
> +	ret = spi_sync_transfer(spi, &xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	*val = st->rx_buf[1];
> +
> +	/* Check CRC */
> +	if (st->rx_buf[2] != st->tx_buf[2])
> +		dev_dbg(st->dev, "reg read CRC check failed\n");
> +
> +	return 0;
> +}


> +
> +static const struct ad4134_bus_info ad4134_min_io_bus_info = {

given it's a mix of bus specific and other stuff, I'm not sure 
that calling this bus_info makes sense.  Maybe just ad4134_info?

> +	.chip_info = &ad4134_chip_info,
> +	.bops = &ad4134_min_io_bops,
> +};