[v2,5/5] iio: adc: add support for ad4052

The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit,
successive approximation register (SAR) analog-to-digital converter (ADC)
that enables low-power, high-density data acquisition solutions without
sacrificing precision.
This ADC offers a unique balance of performance and power efficiency,
plus innovative features for seamlessly switching between high-resolution
and low-power modes tailored to the immediate needs of the system.
The AD4052/AD4058/AD4050/AD4056 are ideal for battery-powered,
compact data acquisition and edge sensing applications.

Signed-off-by: Jorge Marques <jorge.marques@analog.com>
---
 MAINTAINERS              |    1 +
 drivers/iio/adc/Kconfig  |   14 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad4052.c | 1425 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1441 insertions(+)

On Tue, Apr 22, 2025 at 01:34:50PM +0200, Jorge Marques wrote:
> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit,
> successive approximation register (SAR) analog-to-digital converter (ADC)
> that enables low-power, high-density data acquisition solutions without
> sacrificing precision.
> This ADC offers a unique balance of performance and power efficiency,
> plus innovative features for seamlessly switching between high-resolution
> and low-power modes tailored to the immediate needs of the system.
> The AD4052/AD4058/AD4050/AD4056 are ideal for battery-powered,
> compact data acquisition and edge sensing applications.

...

+ array_size.h

> +#include <linux/bitfield.h>

+ bitops.h
+ completion.h

> +#include <linux/delay.h>

+ dev_printk.h
+ err.h

> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/buffer-dmaengine.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/interrupt.h>

+ jiffies.h
+ math.h

> +#include <linux/pm_runtime.h>

+ property.h

> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/offload/consumer.h>
> +#include <linux/spi/offload/provider.h>

+ string.h
+ types.h

+ asm/byteorder.h

...

> +#define AD4052_FS(g)		((&ad4052_conversion_freqs[AD4052_FS_OFFSET(g)]))

Why double parentheses? What does this mean?

...

> +#define AD4052_FS_LEN(g)	(ARRAY_SIZE(ad4052_conversion_freqs) - (AD4052_FS_OFFSET(g)))

Too many parentheses.

...

> +static const struct iio_event_spec ad4052_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE)

Leave trailing comma.

> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> +				      BIT(IIO_EV_INFO_HYSTERESIS)

Ditto.

> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> +				      BIT(IIO_EV_INFO_HYSTERESIS)

Ditto.

> +	}
> +};

...

> +static const char *const ad4052_conversion_freqs[] = {
> +	"2000000", "1000000", "300000", "100000", "33300",
> +	"10000", "3000", "500", "333", "250", "200",
> +	"166", "140", "124", "111",

Better to format with equal amount of members per line (usually power-of-two)
with a comment.

	"2000000", "1000000", "300000", "100000",	/*  0 -  3 */
	"33300", "10000", "3000", "500",		/*  4 -  7 */
	"333", "250", "200", "166",			/*  8 - 11 */
	"140", "124", "111",				/* 12 - 15 */

And why these are string literals?

> +};

...

> +static ssize_t ad4052_events_frequency_show(struct device *dev,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
> +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%s\n", ad4052_conversion_freqs[st->events_frequency]);

You should use sysfs_emit() from sysfs.h.

> +}

...

> +static ssize_t ad4052_events_frequency_available_show(struct device *dev,
> +						      struct device_attribute *attr,
> +						      char *buf)
> +{
> +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> +	int len = 0;
> +
> +	for (u8 i = AD4052_FS_OFFSET(st->grade);
> +	     i < AD4052_FS_LEN(st->grade); i++)
> +		len += sprintf(buf + len, "%s ", ad4052_conversion_freqs[i]);
> +
> +	return sprintf(buf + len, "\n") + len;

sysfs_emit_at(). Use of sprintf() is quite wrong here even if don't care about
sysfs_emit*() APIs.

> +}

> +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> +		       ad4052_events_frequency_show,
> +		       ad4052_events_frequency_store, 0);
> +
> +static IIO_DEVICE_ATTR(sampling_frequency_available, 0444,
> +		       ad4052_events_frequency_available_show,
> +		       NULL, 0);

Please, move each of them closer to the callback. Also, why not
IIO_DEVICE_ATTR_RO() to begin with?

...

> +static struct attribute *ad4052_event_attributes[] = {
> +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL,

No comma in terminator entry.

> +};

...

> +static void ad4052_update_xfer_raw(struct iio_dev *indio_dev,
> +				   struct iio_chan_spec const *chan)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +	struct spi_transfer *xfer = &st->xfer;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);

> +

Unneeded blank line.

> +	if (IS_ERR(scan_type))
> +		return;
> +
> +	xfer->bits_per_word = scan_type->realbits;
> +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
> +}
> +
> +static int ad4052_update_xfer_offload(struct iio_dev *indio_dev,
> +				      struct iio_chan_spec const *chan)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +	struct spi_transfer *xfer = &st->xfer;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +

Ditto.

> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	xfer = &st->offload_xfer;
> +	xfer->bits_per_word = scan_type->realbits;
> +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
> +
> +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> +	st->offload_msg.offload = st->offload;
> +
> +	return spi_optimize_message(st->spi, &st->offload_msg);
> +}

...

> +static int ad4052_set_oversampling_ratio(struct iio_dev *indio_dev,
> +					 const struct iio_chan_spec *chan,
> +					 unsigned int val)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if ((val) < 1 || (val) > BIT(st->chip->max_avg + 1))

Too many parentheses.

> +		return -EINVAL;
> +
> +	/* 1 disables oversampling */
> +	if (val == 1) {
> +		st->mode = AD4052_SAMPLE_MODE;
> +	} else {
> +		val = ilog2(val);
> +		st->mode = AD4052_BURST_AVERAGING_MODE;
> +		ret = regmap_write(st->regmap, AD4052_REG_AVG_CONFIG, val - 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ad4052_update_xfer_raw(indio_dev, chan);
> +
> +	return 0;
> +}

...

> +static int ad4052_get_oversampling_ratio(struct ad4052_state *st,
> +					 unsigned int *val)
> +{
> +	int ret;
> +
> +	if (st->mode == AD4052_SAMPLE_MODE) {
> +		*val = 1;
> +		return 0;
> +	}
> +
> +	ret = regmap_read(st->regmap, AD4052_REG_AVG_CONFIG, val);
> +	if (ret)
> +		return ret;
> +
> +	*val = BIT(*val + 1);

Please, introduce a local variable and use it. This one looks bad because it
will write into output knowing when it's an error case.

> +	return 0;
> +}

...

> +static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
> +{
> +	struct pwm_state pwm_st;
> +
> +	if (freq <= 0 || freq > AD4052_MAX_RATE(st->grade))

in_range() from minmax.h?

> +		return -EINVAL;
> +
> +	pwm_get_state(st->cnv_pwm, &pwm_st);
> +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> +	return pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
> +}

...

> +static int ad4052_soft_reset(struct ad4052_state *st)
> +{
> +	int ret;
> +
> +	memset(st->buf_reset_pattern, 0xFF, sizeof(st->buf_reset_pattern));
> +	for (int i = 0; i < 3; i++)
> +		st->buf_reset_pattern[6 * (i + 1) - 1] = 0xFE;

Only three times and simple oneliner, can we unroll the loop and show
the indices explicitly? It will help a lot in understanding what the actual
pattern is.

> +	ret = spi_write(st->spi, st->buf_reset_pattern,
> +			sizeof(st->buf_reset_pattern));
> +	if (ret)
> +		return ret;
> +
> +	/* Wait AD4052 reset delay */
> +	fsleep(5000);
> +
> +	return 0;
> +}

...

> +static int ad4052_setup(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);

> +

Unneeded blank line.

> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	u8 val = FIELD_PREP(AD4052_GP_CONF_MODE_MSK_0, AD4052_GP_INTR) |
> +		 FIELD_PREP(AD4052_GP_CONF_MODE_MSK_1, AD4052_GP_DRDY);
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONF,
> +				 AD4052_GP_CONF_MODE_MSK_1 | AD4052_GP_CONF_MODE_MSK_0,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(AD4052_INTR_CONF_EN_MSK_0, (AD4052_INTR_EN_EITHER)) |
> +	      FIELD_PREP(AD4052_INTR_CONF_EN_MSK_1, (AD4052_INTR_EN_NEITHER));
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_INTR_CONF,
> +				 AD4052_INTR_CONF_EN_MSK_0 | AD4052_INTR_CONF_EN_MSK_1,
> +				 val);
> +	if (ret)
> +		return ret;

> +	val = 0;
> +	if (scan_type->sign == 's')
> +		val |= AD4052_ADC_MODES_DATA_FORMAT;
> +
> +	st->data_format = val;

Why not simply:

	if (scan_type->sign == 's')
		st->data_format = val | AD4052_ADC_MODES_DATA_FORMAT;
	else
		st->data_format = 0;

?

> +	if (st->grade == AD4052_500KSPS) {
> +		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
> +				   FIELD_PREP(AD4052_TIMER_CONFIG_FS_MASK,
> +					      AD4052_TIMER_CONFIG_300KSPS));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regmap_write(st->regmap, AD4052_REG_ADC_MODES, val);
> +}

...

> +static int ad4052_request_irq(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;

> +	int ret = 0;

Why an assignment?

> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> +	if (ret <= 0)
> +		return ret ? ret : -EINVAL;
> +
> +	ret = devm_request_threaded_irq(dev, ret, NULL,
> +					ad4052_irq_handler_thresh,
> +					IRQF_ONESHOT, indio_dev->name,
> +					indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
> +	if (ret <= 0)

Ahy comparison to 0?

> +		return ret ? ret : -EINVAL;

This is not needed in such a form. Please, read the above API's doc and act
accordingly.

> +	st->gp1_irq = ret;
> +	return devm_request_threaded_irq(dev, ret, NULL,
> +					 ad4052_irq_handler_drdy,
> +					 IRQF_ONESHOT, indio_dev->name,
> +					 st);
> +}

...

> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
> +{
> +	struct spi_device *spi = st->spi;
> +	int ret;

> +	struct spi_transfer t_cnv = {
> +		.len = 0

Missing comma, but...

> +	};

This all is not needed, just make it

	struct spi_transfer t_cnv = {};

> +	reinit_completion(&st->completion);
> +
> +	if (st->cnv_gp) {
> +		gpiod_set_value_cansleep(st->cnv_gp, 1);

No delay?

> +		gpiod_set_value_cansleep(st->cnv_gp, 0);
> +	} else {
> +		ret = spi_sync_transfer(spi, &t_cnv, 1);
> +		if (ret)
> +			return ret;
> +	}
> +	/*
> +	 * Single sample read should be used only for oversampling and
> +	 * sampling frequency pairs that take less than 1 sec.
> +	 */
> +	ret = wait_for_completion_timeout(&st->completion,
> +					  msecs_to_jiffies(1000));

Where msec_to_jiffies() is defined?

> +	if (!ret)
> +		return -ETIMEDOUT;
> +
> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (st->xfer.len == 2) {
> +		*val = be16_to_cpu(st->d16);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 15);

Where sign_extend32() is defined?

> +	} else {
> +		*val = be32_to_cpu(st->d32);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 23);
> +	}
> +
> +	return ret;
> +}

...

> +static int ad4052_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {

> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;

Inconsistent approach, use the same goto.

> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad4052_read_chan_raw(indio_dev, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ad4052_get_oversampling_ratio(st, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> +		if (ret)
> +			goto out_release;
> +
> +		if (!pwm_st.enabled)
> +			pwm_get_state(st->cnv_pwm, &pwm_st);
> +
> +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> +
> +		ret = IIO_VAL_INT;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;

You may have a great deal of reducing or at least making this readable if split
to two, one is claim_direct wrapped.

> +}
> +
> +static int ad4052_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val,
> +			    int val2, long info)

Ditto for the above function. At least try and see the result. I believe that
it might even shrink number of LoCs.

...

> +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> +{
> +	int ret;
> +
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +	ret = ad4052_exit_command(st);

> +

Unneeded blank line.

> +	if (ret)
> +		pm_runtime_resume_and_get(&st->spi->dev);
> +
> +	return ret;
> +}

...

> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	u8 reg, size = 1;

Make size assignment explicit in each case, it will help a lot for both:
maintaining in long term and reading the code.

> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (dir == IIO_EV_DIR_RISING)
> +			reg = AD4052_REG_MAX_LIMIT;
> +		else
> +			reg = AD4052_REG_MIN_LIMIT;
> +		size = 2;
> +		break;
> +	case IIO_EV_INFO_HYSTERESIS:
> +		if (dir == IIO_EV_DIR_RISING)
> +			reg = AD4052_REG_MAX_HYST;
> +		else
> +			reg = AD4052_REG_MIN_HYST;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto out_release;
> +	}
> +
> +	ret = regmap_bulk_read(st->regmap, reg, &st->d32, size);
> +	if (ret)
> +		goto out_release;
> +
> +	if (reg == AD4052_REG_MAX_LIMIT || reg == AD4052_REG_MIN_LIMIT) {
> +		*val = be16_to_cpu(st->d16);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 11);

Better pattern is to use local variable and assign if and only if the function
returns success.

> +	} else {
> +		*val = be32_to_cpu(st->d32);
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret ? ret : IIO_VAL_INT;

Again, try with a wrapper.

> +}

...

> +static int ad4052_write_event_value(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir,
> +				    enum iio_event_info info, int val,
> +				    int val2)

Same comments as per previous one.

> +static int ad4052_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	pwm_disable(st->cnv_pwm);
> +
> +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +	spi_unoptimize_message(&st->offload_msg);
> +	enable_irq(st->gp1_irq);
> +
> +	ret = ad4052_exit_command(st);

You leave IRQ enabled even in error case, is it on purpose?

> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +	return ret;
> +}

...

> +static int ad4052_request_offload(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;
> +	struct dma_chan *rx_dma;
> +	struct spi_offload_trigger_info trigger_info = {
> +		.fwnode = dev_fwnode(dev),
> +		.ops = &ad4052_offload_trigger_ops,
> +		.priv = st,
> +	};
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	indio_dev->setup_ops = &ad4052_buffer_setup_ops;
> +
> +	ret = devm_spi_offload_trigger_register(dev, &trigger_info);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register offload trigger\n");

One line?

> +	st->offload_trigger = devm_spi_offload_trigger_get(dev, st->offload,
> +							   SPI_OFFLOAD_TRIGGER_DATA_READY);
> +	if (IS_ERR(st->offload_trigger))
> +		return PTR_ERR(st->offload_trigger);
> +
> +	st->cnv_pwm = devm_pwm_get(dev, NULL);
> +	if (IS_ERR(st->cnv_pwm))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_pwm),
> +				     "failed to get CNV PWM\n");

Can be one line (you already have above 91 character long line).

> +	pwm_init_state(st->cnv_pwm, &pwm_st);
> +
> +	pwm_st.enabled = false;
> +	pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2;

> +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> +					 AD4052_MAX_RATE(st->grade));

It can be one line (83 characters).


> +	ret = pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to apply CNV PWM\n");
> +
> +	ret = devm_add_action_or_reset(dev, ad4052_pwm_disable, st->cnv_pwm);
> +	if (ret)
> +		return ret;
> +
> +	rx_dma = devm_spi_offload_rx_stream_request_dma_chan(dev, st->offload);
> +	if (IS_ERR(rx_dma))
> +		return PTR_ERR(rx_dma);
> +
> +	return devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev, rx_dma,
> +							   IIO_BUFFER_DIRECTION_IN);
> +}

...

> +static int ad4052_probe(struct spi_device *spi)
> +{
> +	const struct ad4052_chip_info *chip;
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4052_state *st;

> +	int ret = 0;

What is this for?

> +	chip = spi_get_device_match_data(spi);
> +	if (!chip)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "Could not find chip info data\n");
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +	spi_set_drvdata(spi, st);
> +	init_completion(&st->completion);
> +
> +	st->regmap = devm_regmap_init_spi(spi, &ad4052_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Failed to initialize regmap\n");
> +
> +	st->mode = AD4052_SAMPLE_MODE;
> +	st->wait_event = false;
> +	st->chip = chip;
> +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
> +	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
> +	st->events_frequency = AD4052_FS_OFFSET(st->grade);
> +
> +	st->cnv_gp = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->cnv_gp))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_gp),
> +				     "Failed to get cnv gpio\n");
> +
> +	indio_dev->modes = INDIO_BUFFER_HARDWARE | INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = 1;
> +	indio_dev->info = &ad4052_info;
> +	indio_dev->name = chip->name;
> +
> +	st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config);
> +	if (IS_ERR(st->offload))
> +		return PTR_ERR(st->offload);

> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "Failed to get offload\n");

Huh?! Leftover?

> +	if (ret == -ENODEV) {
> +		st->offload_trigger = NULL;
> +		indio_dev->channels = chip->channels;

How this is not a dead code, please?

> +	} else {
> +		indio_dev->channels = chip->offload_channels;
> +		ret = ad4052_request_offload(indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to configure offload\n");
> +	}
> +
> +	st->xfer.rx_buf = &st->d32;
> +
> +	ret = ad4052_soft_reset(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "AD4052 failed to soft reset\n");
> +
> +	ret = ad4052_check_ids(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "AD4052 fields assertions failed\n");
> +
> +	ret = ad4052_setup(indio_dev, indio_dev->channels);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> +			   AD4052_DEVICE_STATUS_DEVICE_RESET);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_request_irq(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ad4052_update_xfer_raw(indio_dev, indio_dev->channels);
> +
> +	pm_runtime_set_active(dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable pm_runtime\n");
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

...

> +static int ad4052_runtime_resume(struct device *dev)
> +{
> +	struct ad4052_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
> +	return ret;

Redundant local variable.

> +}

On 4/22/25 6:34 AM, Jorge Marques wrote:
> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit,
> successive approximation register (SAR) analog-to-digital converter (ADC)
> that enables low-power, high-density data acquisition solutions without
> sacrificing precision.
> This ADC offers a unique balance of performance and power efficiency,
> plus innovative features for seamlessly switching between high-resolution
> and low-power modes tailored to the immediate needs of the system.
> The AD4052/AD4058/AD4050/AD4056 are ideal for battery-powered,
> compact data acquisition and edge sensing applications.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> ---
>  MAINTAINERS              |    1 +
>  drivers/iio/adc/Kconfig  |   14 +
>  drivers/iio/adc/Makefile |    1 +
>  drivers/iio/adc/ad4052.c | 1425 ++++++++++++++++++++++++++++++++++++++++++++++

This patch is way too big, so I didn't review most of it yet. But time to call
it quits for today. In the future, it would be a lot easier for reviewers if
you can split things into multiple patches instead of implementing all of the
features at once. E.g. start with just a basic driver, then a patch to add
oversampling support, then another patch to add SPI offload support. 500 lines
is a more manageable size for review.

...

> +static int ad4052_update_xfer_offload(struct iio_dev *indio_dev,
> +				      struct iio_chan_spec const *chan)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +	struct spi_transfer *xfer = &st->xfer;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	xfer = &st->offload_xfer;
> +	xfer->bits_per_word = scan_type->realbits;
> +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);

This doesn't work for oversampling. realbits may be 16 while storagebits is 32.
But the SPI controller needs to know how many realbits-sized words to read.

So this should be 

	xfer->len = BITS_TO_BYTES(scan_type->realbits);



> +
> +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> +	st->offload_msg.offload = st->offload;
> +
> +	return spi_optimize_message(st->spi, &st->offload_msg);

I know it is like this in a few other drivers already, but I don't like having
spi_optimize_message() in this funtion because it makes it really easy to
forget to do have balanced calls to spi_unoptimize_message().

> +}
> +

...

> +static const struct iio_buffer_setup_ops ad4052_buffer_setup_ops = {
> +	.postenable = &ad4052_buffer_postenable,
> +	.predisable = &ad4052_buffer_predisable,
> +};

Would be nice to add "offload" to the name of this struct and the callbacks
to make it clear that these are only for the SPI offload use case.

...

> +
> +static bool ad4052_offload_trigger_match(struct spi_offload_trigger *trigger,
> +					 enum spi_offload_trigger_type type,
> +					 u64 *args, u32 nargs)
> +{

We should be checking the args here according to what I suggested in my reply
to the devicetree bindings patch. Right now it is assuming that we are only
using this for SPI offload and that the pin used is GP1 and the event is data
read. We should at least verify that the args match those assumptions.

For bonus points, we could implement allowing GPO as well.

> +	return type == SPI_OFFLOAD_TRIGGER_DATA_READY;
> +}
> +
> +static const struct spi_offload_trigger_ops ad4052_offload_trigger_ops = {
> +	.match = ad4052_offload_trigger_match,
> +};
> +
> +static int ad4052_request_offload(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;
> +	struct dma_chan *rx_dma;
> +	struct spi_offload_trigger_info trigger_info = {
> +		.fwnode = dev_fwnode(dev),
> +		.ops = &ad4052_offload_trigger_ops,
> +		.priv = st,
> +	};
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	indio_dev->setup_ops = &ad4052_buffer_setup_ops;
> +
> +	ret = devm_spi_offload_trigger_register(dev, &trigger_info);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register offload trigger\n");

Strictly speaking, the trigger-source provider is indendant of using it for
SPI offload. I guess this is fine here for now though.

> +
> +	st->offload_trigger = devm_spi_offload_trigger_get(dev, st->offload,
> +							   SPI_OFFLOAD_TRIGGER_DATA_READY);
> +	if (IS_ERR(st->offload_trigger))
> +		return PTR_ERR(st->offload_trigger);
> +
> +	st->cnv_pwm = devm_pwm_get(dev, NULL);
> +	if (IS_ERR(st->cnv_pwm))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_pwm),
> +				     "failed to get CNV PWM\n");
> +
> +	pwm_init_state(st->cnv_pwm, &pwm_st);
> +
> +	pwm_st.enabled = false;
> +	pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2;
> +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> +					 AD4052_MAX_RATE(st->grade));
> +
> +	ret = pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to apply CNV PWM\n");
> +
> +	ret = devm_add_action_or_reset(dev, ad4052_pwm_disable, st->cnv_pwm);
> +	if (ret)
> +		return ret;
> +
> +	rx_dma = devm_spi_offload_rx_stream_request_dma_chan(dev, st->offload);
> +	if (IS_ERR(rx_dma))
> +		return PTR_ERR(rx_dma);
> +
> +	return devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev, rx_dma,
> +							   IIO_BUFFER_DIRECTION_IN);
> +}
> +
> +static int ad4052_probe(struct spi_device *spi)
> +{
> +	const struct ad4052_chip_info *chip;
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4052_state *st;
> +	int ret = 0;
> +
> +	chip = spi_get_device_match_data(spi);
> +	if (!chip)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "Could not find chip info data\n");
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +	spi_set_drvdata(spi, st);
> +	init_completion(&st->completion);
> +
> +	st->regmap = devm_regmap_init_spi(spi, &ad4052_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Failed to initialize regmap\n");
> +
> +	st->mode = AD4052_SAMPLE_MODE;
> +	st->wait_event = false;
> +	st->chip = chip;
> +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
> +	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
> +	st->events_frequency = AD4052_FS_OFFSET(st->grade);

Somewhere around here, we should be turning on the power supplies. Also, it
looks like we need some special handling to get the reference volage. If there
is a supply connected to REF, use that, if not, use VDD which requires writing
to a register to let the chip know.

> +
> +	st->cnv_gp = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->cnv_gp))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_gp),
> +				     "Failed to get cnv gpio\n");
> +
> +	indio_dev->modes = INDIO_BUFFER_HARDWARE | INDIO_DIRECT_MODE;

INDIO_BUFFER_HARDWARE should not be set here. If using SPI offload,
devm_iio_dmaengine_buffer_setup_with_handle() will add it automatically.
For non-SPI-offload operation, it should not be set.

> +	indio_dev->num_channels = 1;
> +	indio_dev->info = &ad4052_info;
> +	indio_dev->name = chip->name;
> +
> +	st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config);

This

> +	if (IS_ERR(st->offload))
> +		return PTR_ERR(st->offload);

should be

	ret = PTR_ERR_OR_ZERO(st->offload);

> +
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "Failed to get offload\n");
> +
> +	if (ret == -ENODEV) {
> +		st->offload_trigger = NULL;
> +		indio_dev->channels = chip->channels;
> +	} else {
> +		indio_dev->channels = chip->offload_channels;
> +		ret = ad4052_request_offload(indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to configure offload\n");
> +	}
> +
> +	st->xfer.rx_buf = &st->d32;

I don't think we want this set globally. I.e. it doesn't make sense for SPI
offload xfers.

> +
> +	ret = ad4052_soft_reset(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "AD4052 failed to soft reset\n");
> +
> +	ret = ad4052_check_ids(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "AD4052 fields assertions failed\n");
> +
> +	ret = ad4052_setup(indio_dev, indio_dev->channels);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> +			   AD4052_DEVICE_STATUS_DEVICE_RESET);

Why not include this in ad4052_setup() or even ad4052_soft_reset()?

> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_request_irq(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ad4052_update_xfer_raw(indio_dev, indio_dev->channels);
> +
> +	pm_runtime_set_active(dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable pm_runtime\n");
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static int ad4052_runtime_suspend(struct device *dev)
> +{
> +	struct ad4052_state *st = dev_get_drvdata(dev);
> +
> +	return regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> +			    FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK,
> +				       AD4052_DEVICE_CONFIG_LOW_POWER_MODE));
> +}
> +
> +static int ad4052_runtime_resume(struct device *dev)
> +{
> +	struct ad4052_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));

regmap_clear_bits() would be shorter if there isn't going to be a macro to
explain the meaning of 0.

> +	return ret;
> +}
> +

On Tue, 22 Apr 2025 13:34:50 +0200
Jorge Marques <jorge.marques@analog.com> wrote:

> The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit,
> successive approximation register (SAR) analog-to-digital converter (ADC)
> that enables low-power, high-density data acquisition solutions without
> sacrificing precision.
> This ADC offers a unique balance of performance and power efficiency,
> plus innovative features for seamlessly switching between high-resolution
> and low-power modes tailored to the immediate needs of the system.
> The AD4052/AD4058/AD4050/AD4056 are ideal for battery-powered,
> compact data acquisition and edge sensing applications.
> 
> Signed-off-by: Jorge Marques <jorge.marques@analog.com>
Hi Jorge,

A few additional comments from me.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..f412f0884bab4f500091f6c7ca761967c8f6e3b6
> --- /dev/null
> +++ b/drivers/iio/adc/ad4052.c
> @@ -0,0 +1,1425 @@


> +static int ad4052_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad4052_read_chan_raw(indio_dev, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ad4052_get_oversampling_ratio(st, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> +		if (ret)
> +			goto out_release;
> +
> +		if (!pwm_st.enabled)
> +			pwm_get_state(st->cnv_pwm, &pwm_st);
> +
> +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> +
> +		ret = IIO_VAL_INT;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}


> +static int ad4052_write_event_value(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir,
> +				    enum iio_event_info info, int val,
> +				    int val2)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +	u8 reg, size = 1;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;

Sometimes it is worth a n internal function factored out in cases
like this to allow direct returns in error cases with the release
always happening before we check if the inner function failed.

That suggestion applies in other places in this code.
> +
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT) {
> +				if (val > 2047 || val < -2048)
> +					goto out_release;
> +			} else if (val > 4095 || val < 0) {
> +				goto out_release;
> +			}
> +			if (dir == IIO_EV_DIR_RISING)
> +				reg = AD4052_REG_MAX_LIMIT;
> +			else
> +				reg = AD4052_REG_MIN_LIMIT;
> +			size = 2;
> +			st->d16 = cpu_to_be16(val);
> +			break;
> +		case IIO_EV_INFO_HYSTERESIS:
> +			if (val & BIT(7))
> +				goto out_release;
> +			if (dir == IIO_EV_DIR_RISING)
> +				reg = AD4052_REG_MAX_HYST;
> +			else
> +				reg = AD4052_REG_MIN_HYST;
> +			st->d16 = cpu_to_be16(val >> 8);
> +			break;
> +		default:
> +			goto out_release;
> +		}
> +		break;
> +	default:
> +		goto out_release;
> +	}
> +
> +	ret = regmap_bulk_write(st->regmap, reg, &st->d16, size);
> +
> +out_release:
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}
> +
> +static int ad4052_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct spi_offload_trigger_config config = {
> +		.type = SPI_OFFLOAD_TRIGGER_DATA_READY,
> +	};
> +	int ret;
> +
> +	if (st->wait_event)
> +		return -EBUSY;
> +
> +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_set_operation_mode(st, st->mode);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = ad4052_update_xfer_offload(indio_dev, indio_dev->channels);
> +	if (ret)
> +		goto out_error;
> +
> +	/* SPI Offload handles the data ready irq */
> +	disable_irq(st->gp1_irq);
> +
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &config);
> +	if (ret)
> +		goto out_offload_error;
> +
> +	ret = pwm_enable(st->cnv_pwm);
> +	if (ret)
> +		goto out_pwm_error;
> +
> +	return 0;
> +
> +out_pwm_error:
> +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +out_offload_error:
> +	enable_irq(st->gp1_irq);
> +	spi_unoptimize_message(&st->offload_msg);
> +	ad4052_exit_command(st);

What is this matching to?  Feels like it would be set_operation_mode()
but I may be wrong on that.  If it is then you need another label
to call only this update_xfer_offload fails.

> +out_error:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +	return ret;
> +}

> +static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +				     unsigned int writeval, unsigned int *readval)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
Probably use a goto to release this in only one place.

> +		return -EBUSY;
> +	}
> +
> +	if (readval)
> +		ret = regmap_read(st->regmap, reg, readval);
> +	else
> +		ret = regmap_write(st->regmap, reg, writeval);
> +
> +	iio_device_release_direct(indio_dev);
> +	return ret;
> +}
> +
> +static int ad4052_get_current_scan_type(const struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +
> +	if (iio_buffer_enabled(indio_dev))

This is the bit I'm not really following. Why is the enabling or not of
the buffer related to whether offload is going on?


> +		return st->mode == AD4052_BURST_AVERAGING_MODE ?
> +				   AD4052_SCAN_TYPE_OFFLOAD_BURST_AVG :
> +				   AD4052_SCAN_TYPE_OFFLOAD_SAMPLE;
> +
> +	return st->mode == AD4052_BURST_AVERAGING_MODE ?
> +			   AD4052_SCAN_TYPE_BURST_AVG :
> +			   AD4052_SCAN_TYPE_SAMPLE;
> +}


> +static int ad4052_probe(struct spi_device *spi)
> +{
...

> +
> +	st->mode = AD4052_SAMPLE_MODE;
> +	st->wait_event = false;
> +	st->chip = chip;
> +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;

That feels like it should be encoded directly in chip.  Basing it
on prod_id seems liable to bite us at somepoint in the future.




> +
> +static int ad4052_runtime_resume(struct device *dev)
> +{
> +	struct ad4052_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
> +	return ret;

	return regmap_write();
and no need for the local variable ret.

> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(ad4052_pm_ops, ad4052_runtime_suspend,
> +				 ad4052_runtime_resume, NULL);

Hi Andy, thank you for the review.
I just have comments on
The ad4052_conversion_freqs string literal [#f0].
iio_device_claim_direct_scoped (that could be
used to organize some parts of the code, but can't because the macro was
removed) [#f1].
And why there is no delay [#f2].

On Tue, Apr 22, 2025 at 07:33:07PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 22, 2025 at 01:34:50PM +0200, Jorge Marques wrote:
> > The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit,
> > successive approximation register (SAR) analog-to-digital converter (ADC)
> > that enables low-power, high-density data acquisition solutions without
> > sacrificing precision.
> > This ADC offers a unique balance of performance and power efficiency,
> > plus innovative features for seamlessly switching between high-resolution
> > and low-power modes tailored to the immediate needs of the system.
> > The AD4052/AD4058/AD4050/AD4056 are ideal for battery-powered,
> > compact data acquisition and edge sensing applications.
> 
> ...
> 
> + array_size.h
> 
> > +#include <linux/bitfield.h>
> 
> + bitops.h
> + completion.h
> 
> > +#include <linux/delay.h>
> 
> + dev_printk.h
> + err.h
> 
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/buffer-dmaengine.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/interrupt.h>
> 
> + jiffies.h
> + math.h
> 
> > +#include <linux/pm_runtime.h>
> 
> + property.h
> 
> > +#include <linux/pwm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/offload/consumer.h>
> > +#include <linux/spi/offload/provider.h>
> 
> + string.h
> + types.h
> 
> + asm/byteorder.h

Ack.

> 
> ...
> 
> > +#define AD4052_FS(g)		((&ad4052_conversion_freqs[AD4052_FS_OFFSET(g)]))
> 
> Why double parentheses? What does this mean?

Dropped.

> 
> ...
> 
> > +#define AD4052_FS_LEN(g)	(ARRAY_SIZE(ad4052_conversion_freqs) - (AD4052_FS_OFFSET(g)))
> 
> Too many parentheses.
> 

Dropped.

> ...
> 
> > +static const struct iio_event_spec ad4052_events[] = {
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_EITHER,
> > +		.mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE)
> 
> Leave trailing comma.

Ack.
> 
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_RISING,
> > +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> > +				      BIT(IIO_EV_INFO_HYSTERESIS)
> 
> Ditto.
Ack.
> 
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> > +				      BIT(IIO_EV_INFO_HYSTERESIS)
> 
> Ditto.
Ack.
> 
> > +	}
> > +};
> 
> ...
> 
> > +static const char *const ad4052_conversion_freqs[] = {
> > +	"2000000", "1000000", "300000", "100000", "33300",
> > +	"10000", "3000", "500", "333", "250", "200",
> > +	"166", "140", "124", "111",
> 
> Better to format with equal amount of members per line (usually power-of-two)
> with a comment.
> 
> 	"2000000", "1000000", "300000", "100000",	/*  0 -  3 */
> 	"33300", "10000", "3000", "500",		/*  4 -  7 */
> 	"333", "250", "200", "166",			/*  8 - 11 */
> 	"140", "124", "111",				/* 12 - 15 */
> 
> And why these are string literals?
> 

Ack.
The string literals are because this array is also used in the
iio_enum struct that takes string literals.
[#f0]

> > +};
> 
> ...
> 
> > +static ssize_t ad4052_events_frequency_show(struct device *dev,
> > +					    struct device_attribute *attr,
> > +					    char *buf)
> > +{
> > +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> > +
> > +	return sprintf(buf, "%s\n", ad4052_conversion_freqs[st->events_frequency]);
> 
> You should use sysfs_emit() from sysfs.h.
> 

Ack.

> > +}
> 
> ...
> 
> > +static ssize_t ad4052_events_frequency_available_show(struct device *dev,
> > +						      struct device_attribute *attr,
> > +						      char *buf)
> > +{
> > +	struct ad4052_state *st = iio_priv(dev_to_iio_dev(dev));
> > +	int len = 0;
> > +
> > +	for (u8 i = AD4052_FS_OFFSET(st->grade);
> > +	     i < AD4052_FS_LEN(st->grade); i++)
> > +		len += sprintf(buf + len, "%s ", ad4052_conversion_freqs[i]);
> > +
> > +	return sprintf(buf + len, "\n") + len;
> 
> sysfs_emit_at(). Use of sprintf() is quite wrong here even if don't care about
> sysfs_emit*() APIs.
> 

Ack.

> > +}
> 
> > +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> > +		       ad4052_events_frequency_show,
> > +		       ad4052_events_frequency_store, 0);
> > +
> > +static IIO_DEVICE_ATTR(sampling_frequency_available, 0444,
> > +		       ad4052_events_frequency_available_show,
> > +		       NULL, 0);
> 
> Please, move each of them closer to the callback. Also, why not
> IIO_DEVICE_ATTR_RO() to begin with?
> 

Moved closer to the callback.
Yes, IIO_DEVICE_ATTR_RO should have been used here.

> ...
> 
> > +static struct attribute *ad4052_event_attributes[] = {
> > +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> > +	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > +	NULL,
> 
> No comma in terminator entry.
> 
Ack.
> > +};
> 
> ...
> 
> > +static void ad4052_update_xfer_raw(struct iio_dev *indio_dev,
> > +				   struct iio_chan_spec const *chan)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	const struct iio_scan_type *scan_type;
> > +	struct spi_transfer *xfer = &st->xfer;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> 
> > +
> 
> Unneeded blank line.
> 

Ack.
> > +	if (IS_ERR(scan_type))
> > +		return;
> > +
> > +	xfer->bits_per_word = scan_type->realbits;
> > +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
> > +}
> > +
> > +static int ad4052_update_xfer_offload(struct iio_dev *indio_dev,
> > +				      struct iio_chan_spec const *chan)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	const struct iio_scan_type *scan_type;
> > +	struct spi_transfer *xfer = &st->xfer;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> > +
> 
> Ditto.
> 
Ack.
> > +	if (IS_ERR(scan_type))
> > +		return PTR_ERR(scan_type);
> > +
> > +	xfer = &st->offload_xfer;
> > +	xfer->bits_per_word = scan_type->realbits;
> > +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
> > +
> > +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> > +	st->offload_msg.offload = st->offload;
> > +
> > +	return spi_optimize_message(st->spi, &st->offload_msg);
> > +}
> 
> ...
> 
> > +static int ad4052_set_oversampling_ratio(struct iio_dev *indio_dev,
> > +					 const struct iio_chan_spec *chan,
> > +					 unsigned int val)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if ((val) < 1 || (val) > BIT(st->chip->max_avg + 1))
> 
> Too many parentheses.
> 
Ack.

> > +		return -EINVAL;
> > +
> > +	/* 1 disables oversampling */
> > +	if (val == 1) {
> > +		st->mode = AD4052_SAMPLE_MODE;
> > +	} else {
> > +		val = ilog2(val);
> > +		st->mode = AD4052_BURST_AVERAGING_MODE;
> > +		ret = regmap_write(st->regmap, AD4052_REG_AVG_CONFIG, val - 1);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ad4052_update_xfer_raw(indio_dev, chan);
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4052_get_oversampling_ratio(struct ad4052_state *st,
> > +					 unsigned int *val)
> > +{
> > +	int ret;
> > +
> > +	if (st->mode == AD4052_SAMPLE_MODE) {
> > +		*val = 1;
> > +		return 0;
> > +	}
> > +
> > +	ret = regmap_read(st->regmap, AD4052_REG_AVG_CONFIG, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*val = BIT(*val + 1);
> 
> Please, introduce a local variable and use it. This one looks bad because it
> will write into output knowing when it's an error case.
> 
Makes sense.
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
> > +{
> > +	struct pwm_state pwm_st;
> > +
> > +	if (freq <= 0 || freq > AD4052_MAX_RATE(st->grade))
> 
> in_range() from minmax.h?
> 

Done.

> > +		return -EINVAL;
> > +
> > +	pwm_get_state(st->cnv_pwm, &pwm_st);
> > +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> > +	return pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
> > +}
> 
> ...
> 
> > +static int ad4052_soft_reset(struct ad4052_state *st)
> > +{
> > +	int ret;
> > +
> > +	memset(st->buf_reset_pattern, 0xFF, sizeof(st->buf_reset_pattern));
> > +	for (int i = 0; i < 3; i++)
> > +		st->buf_reset_pattern[6 * (i + 1) - 1] = 0xFE;
> 
> Only three times and simple oneliner, can we unroll the loop and show
> the indices explicitly? It will help a lot in understanding what the actual
> pattern is.
> 
Of course, done.
> > +	ret = spi_write(st->spi, st->buf_reset_pattern,
> > +			sizeof(st->buf_reset_pattern));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Wait AD4052 reset delay */
> > +	fsleep(5000);
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int ad4052_setup(struct iio_dev *indio_dev,
> > +			struct iio_chan_spec const *chan)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	const struct iio_scan_type *scan_type;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> 
> > +
> 
> Unneeded blank line.
> 
Ok.
> > +	if (IS_ERR(scan_type))
> > +		return PTR_ERR(scan_type);
> > +
> > +	u8 val = FIELD_PREP(AD4052_GP_CONF_MODE_MSK_0, AD4052_GP_INTR) |
> > +		 FIELD_PREP(AD4052_GP_CONF_MODE_MSK_1, AD4052_GP_DRDY);
> > +	int ret;
> > +
> > +	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONF,
> > +				 AD4052_GP_CONF_MODE_MSK_1 | AD4052_GP_CONF_MODE_MSK_0,
> > +				 val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val = FIELD_PREP(AD4052_INTR_CONF_EN_MSK_0, (AD4052_INTR_EN_EITHER)) |
> > +	      FIELD_PREP(AD4052_INTR_CONF_EN_MSK_1, (AD4052_INTR_EN_NEITHER));
> > +
> > +	ret = regmap_update_bits(st->regmap, AD4052_REG_INTR_CONF,
> > +				 AD4052_INTR_CONF_EN_MSK_0 | AD4052_INTR_CONF_EN_MSK_1,
> > +				 val);
> > +	if (ret)
> > +		return ret;
> 
> > +	val = 0;
> > +	if (scan_type->sign == 's')
> > +		val |= AD4052_ADC_MODES_DATA_FORMAT;
> > +
> > +	st->data_format = val;
> 
> Why not simply:
> 
> 	if (scan_type->sign == 's')
> 		st->data_format = val | AD4052_ADC_MODES_DATA_FORMAT;
> 	else
> 		st->data_format = 0;
> 
> ?
> 

Changed to

	if (scan_type->sign == 's')
	        st->data_format = AD4052_ADC_MODES_DATA_FORMAT;
	else
	        st->data_format = 0;

> > +	if (st->grade == AD4052_500KSPS) {
> > +		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
> > +				   FIELD_PREP(AD4052_TIMER_CONFIG_FS_MASK,
> > +					      AD4052_TIMER_CONFIG_300KSPS));
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return regmap_write(st->regmap, AD4052_REG_ADC_MODES, val);
> > +}
> 
> ...
> 
> > +static int ad4052_request_irq(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct device *dev = &st->spi->dev;
> 
> > +	int ret = 0;
> 
> Why an assignment?
> 
Ack.

> > +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> > +	if (ret <= 0)
> > +		return ret ? ret : -EINVAL;
> > +
> > +	ret = devm_request_threaded_irq(dev, ret, NULL,
> > +					ad4052_irq_handler_thresh,
> > +					IRQF_ONESHOT, indio_dev->name,
> > +					indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
> > +	if (ret <= 0)
> 
> Ahy comparison to 0?
> 
> > +		return ret ? ret : -EINVAL;
> 
> This is not needed in such a form. Please, read the above API's doc and act
> accordingly.
> 
You are right, I missed the fix [1], sorry.
https://lore.kernel.org/all/Y1gNDtE4dRC4WuP%2F@smile.fi.intel.com/
                             
> > +	st->gp1_irq = ret;   
> > +	return devm_request_threaded_irq(dev, ret, NULL,
> > +					 ad4052_irq_handler_drdy,
> > +					 IRQF_ONESHOT, indio_dev->name,
> > +					 st);
> > +}
> 
> ...
> 
> > +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
> > +{
> > +	struct spi_device *spi = st->spi;
> > +	int ret;
> 
> > +	struct spi_transfer t_cnv = {
> > +		.len = 0
> 
> Missing comma, but...
> 
> > +	};
> 
> This all is not needed, just make it
> 
> 	struct spi_transfer t_cnv = {};
Fair, applied.
> 
> > +	reinit_completion(&st->completion);
> > +
> > +	if (st->cnv_gp) {
> > +		gpiod_set_value_cansleep(st->cnv_gp, 1);
> 
> No delay?
> 
In practice it is not needed since tcnv high minimum is 10ns,
no system should switch gpio that fast through these methods.
One approach is to have gpiod_set_value around spi_sync_transfer
"making a sandwich", but since there is the fallback with spi
transfer to also trigger a conversion, this seemed better.
[#f2]
> > +		gpiod_set_value_cansleep(st->cnv_gp, 0);
> > +	} else {
> > +		ret = spi_sync_transfer(spi, &t_cnv, 1);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	/*
> > +	 * Single sample read should be used only for oversampling and
> > +	 * sampling frequency pairs that take less than 1 sec.
> > +	 */
> > +	ret = wait_for_completion_timeout(&st->completion,
> > +					  msecs_to_jiffies(1000));
> 
> Where msec_to_jiffies() is defined?
> 
Ack.
> > +	if (!ret)
> > +		return -ETIMEDOUT;
> > +
> > +	ret = spi_sync_transfer(spi, &st->xfer, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->xfer.len == 2) {
> > +		*val = be16_to_cpu(st->d16);
> > +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> > +			*val = sign_extend32(*val, 15);
> 
> Where sign_extend32() is defined?
> 
Ack.
> > +	} else {
> > +		*val = be32_to_cpu(st->d32);
> > +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> > +			*val = sign_extend32(*val, 23);
> > +	}
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +static int ad4052_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct pwm_state pwm_st;
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> 
> > +		iio_device_release_direct(indio_dev);
> > +		return -EBUSY;
> 
> Inconsistent approach, use the same goto.
> 
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = ad4052_read_chan_raw(indio_dev, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		ret = ad4052_get_oversampling_ratio(st, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> > +		if (ret)
> > +			goto out_release;
> > +
> > +		if (!pwm_st.enabled)
> > +			pwm_get_state(st->cnv_pwm, &pwm_st);
> > +
> > +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> > +
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> 
> You may have a great deal of reducing or at least making this readable if split
> to two, one is claim_direct wrapped.
> 
Done.

> > +}
> > +
> > +static int ad4052_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int val,
> > +			    int val2, long info)
> 
> Ditto for the above function. At least try and see the result. I believe that
> it might even shrink number of LoCs.
> 
> ...
Done.
> 
> > +static int ad4052_monitor_mode_disable(struct ad4052_state *st)
> > +{
> > +	int ret;
> > +
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +
> > +	ret = ad4052_exit_command(st);
> 
> > +
> 
> Unneeded blank line.
Ack.
> 
> > +	if (ret)
> > +		pm_runtime_resume_and_get(&st->spi->dev);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	u8 reg, size = 1;
> 
> Make size assignment explicit in each case, it will help a lot for both:
> maintaining in long term and reading the code.
Agreed.
> 
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		iio_device_release_direct(indio_dev);
> > +		return -EBUSY;
> > +	}
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			reg = AD4052_REG_MAX_LIMIT;
> > +		else
> > +			reg = AD4052_REG_MIN_LIMIT;
> > +		size = 2;
> > +		break;
> > +	case IIO_EV_INFO_HYSTERESIS:
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			reg = AD4052_REG_MAX_HYST;
> > +		else
> > +			reg = AD4052_REG_MIN_HYST;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		goto out_release;
> > +	}
> > +
> > +	ret = regmap_bulk_read(st->regmap, reg, &st->d32, size);
> > +	if (ret)
> > +		goto out_release;
> > +
> > +	if (reg == AD4052_REG_MAX_LIMIT || reg == AD4052_REG_MIN_LIMIT) {
> > +		*val = be16_to_cpu(st->d16);
> > +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> > +			*val = sign_extend32(*val, 11);
> 
> Better pattern is to use local variable and assign if and only if the function
> returns success.
Agree.
> 
> > +	} else {
> > +		*val = be32_to_cpu(st->d32);
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret ? ret : IIO_VAL_INT;
> 
> Again, try with a wrapper.
> 
Here all paths require claiming the iio device, so in this case I don't
see a problem.
> > +}
> 
> ...
> 
> > +static int ad4052_write_event_value(struct iio_dev *indio_dev,
> > +				    const struct iio_chan_spec *chan,
> > +				    enum iio_event_type type,
> > +				    enum iio_event_direction dir,
> > +				    enum iio_event_info info, int val,
> > +				    int val2)
> 
> Same comments as per previous one.
> 
Same as above.
Here would be useful to use the macro iio_device_claim_direct_scoped.
But it was removed because static analyzers didn't play nice with it.
4c571885898c5 (iio: Drop iio_device_claim_direct_scoped() and related infrastructure)
https://patch.msgid.link/20250209180624.701140-28-jic23@kernel.org
[#f1]

> > +static int ad4052_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	pwm_disable(st->cnv_pwm);
> > +
> > +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> > +	spi_unoptimize_message(&st->offload_msg);
> > +	enable_irq(st->gp1_irq);
> > +
> > +	ret = ad4052_exit_command(st);
> 
> You leave IRQ enabled even in error case, is it on purpose?
> 
Yes, there is not much to be done here.
The important part it to have it enabled after the trigger is disabled,
to not get an IRQ from the offload execution after the buffer is
disabled.
I will just move after the ad4052_exit_command with the pm calls.
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +static int ad4052_request_offload(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct device *dev = &st->spi->dev;
> > +	struct dma_chan *rx_dma;
> > +	struct spi_offload_trigger_info trigger_info = {
> > +		.fwnode = dev_fwnode(dev),
> > +		.ops = &ad4052_offload_trigger_ops,
> > +		.priv = st,
> > +	};
> > +	struct pwm_state pwm_st;
> > +	int ret;
> > +
> > +	indio_dev->setup_ops = &ad4052_buffer_setup_ops;
> > +
> > +	ret = devm_spi_offload_trigger_register(dev, &trigger_info);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to register offload trigger\n");
> 
> One line?
> 
Ok
> > +	st->offload_trigger = devm_spi_offload_trigger_get(dev, st->offload,
> > +							   SPI_OFFLOAD_TRIGGER_DATA_READY);
> > +	if (IS_ERR(st->offload_trigger))
> > +		return PTR_ERR(st->offload_trigger);
> > +
> > +	st->cnv_pwm = devm_pwm_get(dev, NULL);
> > +	if (IS_ERR(st->cnv_pwm))
> > +		return dev_err_probe(dev, PTR_ERR(st->cnv_pwm),
> > +				     "failed to get CNV PWM\n");
> 
> Can be one line (you already have above 91 character long line).
> 
Ok
> > +	pwm_init_state(st->cnv_pwm, &pwm_st);
> > +
> > +	pwm_st.enabled = false;
> > +	pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2;
> 
> > +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> > +					 AD4052_MAX_RATE(st->grade));
> 
> It can be one line (83 characters).
> 
Ok
> 
> > +	ret = pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to apply CNV PWM\n");
> > +
> > +	ret = devm_add_action_or_reset(dev, ad4052_pwm_disable, st->cnv_pwm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	rx_dma = devm_spi_offload_rx_stream_request_dma_chan(dev, st->offload);
> > +	if (IS_ERR(rx_dma))
> > +		return PTR_ERR(rx_dma);
> > +
> > +	return devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev, rx_dma,
> > +							   IIO_BUFFER_DIRECTION_IN);
> > +}
> 
> ...
> 
> > +static int ad4052_probe(struct spi_device *spi)
> > +{
> > +	const struct ad4052_chip_info *chip;
> > +	struct device *dev = &spi->dev;
> > +	struct iio_dev *indio_dev;
> > +	struct ad4052_state *st;
> 
> > +	int ret = 0;
> 
> What is this for?
> 
For Nothing
> > +	chip = spi_get_device_match_data(spi);
> > +	if (!chip)
> > +		return dev_err_probe(dev, -ENODEV,
> > +				     "Could not find chip info data\n");
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +	st->spi = spi;
> > +	spi_set_drvdata(spi, st);
> > +	init_completion(&st->completion);
> > +
> > +	st->regmap = devm_regmap_init_spi(spi, &ad4052_regmap_config);
> > +	if (IS_ERR(st->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> > +				     "Failed to initialize regmap\n");
> > +
> > +	st->mode = AD4052_SAMPLE_MODE;
> > +	st->wait_event = false;
> > +	st->chip = chip;
> > +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
> > +	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
> > +	st->events_frequency = AD4052_FS_OFFSET(st->grade);
> > +
> > +	st->cnv_gp = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
> > +	if (IS_ERR(st->cnv_gp))
> > +		return dev_err_probe(dev, PTR_ERR(st->cnv_gp),
> > +				     "Failed to get cnv gpio\n");
> > +
> > +	indio_dev->modes = INDIO_BUFFER_HARDWARE | INDIO_DIRECT_MODE;
> > +	indio_dev->num_channels = 1;
> > +	indio_dev->info = &ad4052_info;
> > +	indio_dev->name = chip->name;
> > +
> > +	st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config);
> > +	if (IS_ERR(st->offload))
> > +		return PTR_ERR(st->offload);
> 
> > +	if (ret && ret != -ENODEV)
> > +		return dev_err_probe(dev, ret, "Failed to get offload\n");
> 
> Huh?! Leftover?
> 
Yes, leftover.
> > +	if (ret == -ENODEV) {
> > +		st->offload_trigger = NULL;
> > +		indio_dev->channels = chip->channels;
> 
> How this is not a dead code, please?
> 
It is dead code.
The important part here is that -ENODEV should be catch to set
offload_triffer NULL, since this feature is optional, both on the driver
as well as for the SPI controllers themself.
> > +	} else {
> > +		indio_dev->channels = chip->offload_channels;
> > +		ret = ad4052_request_offload(indio_dev);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to configure offload\n");
> > +	}
> > +
> > +	st->xfer.rx_buf = &st->d32;
> > +
> > +	ret = ad4052_soft_reset(st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "AD4052 failed to soft reset\n");
> > +
> > +	ret = ad4052_check_ids(st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "AD4052 fields assertions failed\n");
> > +
> > +	ret = ad4052_setup(indio_dev, indio_dev->channels);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> > +			   AD4052_DEVICE_STATUS_DEVICE_RESET);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4052_request_irq(indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ad4052_update_xfer_raw(indio_dev, indio_dev->channels);
> > +
> > +	pm_runtime_set_active(dev);
> > +	ret = devm_pm_runtime_enable(dev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "Failed to enable pm_runtime\n");
> > +
> > +	pm_runtime_set_autosuspend_delay(dev, 1000);
> > +	pm_runtime_use_autosuspend(dev);
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> > +}
> 
> ...
> 
> > +static int ad4052_runtime_resume(struct device *dev)
> > +{
> > +	struct ad4052_state *st = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> > +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
> > +	return ret;
> 
> Redundant local variable.
Ack.
> 
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Regards,
Jorge

Hello,

On Tue, Apr 22, 2025 at 01:34:50PM +0200, Jorge Marques wrote:
> +static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
> +{
> +	struct pwm_state pwm_st;
> +
> +	if (freq <= 0 || freq > AD4052_MAX_RATE(st->grade))
> +		return -EINVAL;
> +
> +	pwm_get_state(st->cnv_pwm, &pwm_st);
> +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> +	return pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);

Is it clear that pwm_st.duty_cycle isn't greater than
DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);

I'm not a big fan of pwm_get_state() because the semantic is a bit
strange. My preferred alternative would be to either use pwm_init_state
and initialize all fields, or maintain a struct pwm_state in struct
ad4052_state.

> +}
> +
> +static int ad4052_soft_reset(struct ad4052_state *st)
> +{
> +	int ret;
> +
> +	memset(st->buf_reset_pattern, 0xFF, sizeof(st->buf_reset_pattern));
> +	for (int i = 0; i < 3; i++)
> +		st->buf_reset_pattern[6 * (i + 1) - 1] = 0xFE;
> +
> +	ret = spi_write(st->spi, st->buf_reset_pattern,
> +			sizeof(st->buf_reset_pattern));
> +	if (ret)
> +		return ret;
> +
> +	/* Wait AD4052 reset delay */
> +	fsleep(5000);
> +
> +	return 0;
> +}
> +
> +static int ad4052_setup(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +
> +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> +
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);
> +
> +	u8 val = FIELD_PREP(AD4052_GP_CONF_MODE_MSK_0, AD4052_GP_INTR) |
> +		 FIELD_PREP(AD4052_GP_CONF_MODE_MSK_1, AD4052_GP_DRDY);
> +	int ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_GP_CONF,
> +				 AD4052_GP_CONF_MODE_MSK_1 | AD4052_GP_CONF_MODE_MSK_0,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(AD4052_INTR_CONF_EN_MSK_0, (AD4052_INTR_EN_EITHER)) |
> +	      FIELD_PREP(AD4052_INTR_CONF_EN_MSK_1, (AD4052_INTR_EN_NEITHER));
> +
> +	ret = regmap_update_bits(st->regmap, AD4052_REG_INTR_CONF,
> +				 AD4052_INTR_CONF_EN_MSK_0 | AD4052_INTR_CONF_EN_MSK_1,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	val = 0;
> +	if (scan_type->sign == 's')
> +		val |= AD4052_ADC_MODES_DATA_FORMAT;
> +
> +	st->data_format = val;
> +
> +	if (st->grade == AD4052_500KSPS) {
> +		ret = regmap_write(st->regmap, AD4052_REG_TIMER_CONFIG,
> +				   FIELD_PREP(AD4052_TIMER_CONFIG_FS_MASK,
> +					      AD4052_TIMER_CONFIG_300KSPS));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regmap_write(st->regmap, AD4052_REG_ADC_MODES, val);
> +}
> +
> +static irqreturn_t ad4052_irq_handler_thresh(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +
> +	iio_push_event(indio_dev,
> +		       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, 0,
> +					    IIO_EV_TYPE_THRESH,
> +					    IIO_EV_DIR_EITHER),
> +		       iio_get_time_ns(indio_dev));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t ad4052_irq_handler_drdy(int irq, void *private)
> +{
> +	struct ad4052_state *st = private;
> +
> +	complete(&st->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ad4052_request_irq(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;
> +	int ret = 0;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp0");
> +	if (ret <= 0)
> +		return ret ? ret : -EINVAL;
> +
> +	ret = devm_request_threaded_irq(dev, ret, NULL,
> +					ad4052_irq_handler_thresh,
> +					IRQF_ONESHOT, indio_dev->name,
> +					indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(&st->spi->dev), "gp1");
> +	if (ret <= 0)
> +		return ret ? ret : -EINVAL;
> +
> +	st->gp1_irq = ret;
> +	return devm_request_threaded_irq(dev, ret, NULL,
> +					 ad4052_irq_handler_drdy,
> +					 IRQF_ONESHOT, indio_dev->name,
> +					 st);
> +}
> +
> +static const int ad4052_oversampling_avail[] = {
> +	1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096,
> +};
> +
> +static int ad4052_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, const int **vals,
> +			     int *type, int *len, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = ad4052_oversampling_avail;
> +		*len = ARRAY_SIZE(ad4052_oversampling_avail);
> +		*type = IIO_VAL_INT;
> +
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int __ad4052_read_chan_raw(struct ad4052_state *st, int *val)
> +{
> +	struct spi_device *spi = st->spi;
> +	int ret;
> +	struct spi_transfer t_cnv = {
> +		.len = 0
> +	};
> +
> +	reinit_completion(&st->completion);
> +
> +	if (st->cnv_gp) {
> +		gpiod_set_value_cansleep(st->cnv_gp, 1);
> +		gpiod_set_value_cansleep(st->cnv_gp, 0);
> +	} else {
> +		ret = spi_sync_transfer(spi, &t_cnv, 1);
> +		if (ret)
> +			return ret;
> +	}
> +	/*
> +	 * Single sample read should be used only for oversampling and
> +	 * sampling frequency pairs that take less than 1 sec.
> +	 */
> +	ret = wait_for_completion_timeout(&st->completion,
> +					  msecs_to_jiffies(1000));
> +	if (!ret)
> +		return -ETIMEDOUT;
> +
> +	ret = spi_sync_transfer(spi, &st->xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (st->xfer.len == 2) {
> +		*val = be16_to_cpu(st->d16);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 15);
> +	} else {
> +		*val = be32_to_cpu(st->d32);
> +		if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT)
> +			*val = sign_extend32(*val, 23);
> +	}
> +
> +	return ret;
> +}
> +
> +static int ad4052_read_chan_raw(struct iio_dev *indio_dev, int *val)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_set_operation_mode(st, st->mode);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = __ad4052_read_chan_raw(st, val);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = ad4052_exit_command(st);
> +
> +out_error:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +	return ret;
> +}
> +
> +static int ad4052_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct pwm_state pwm_st;
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	if (st->wait_event) {
> +		iio_device_release_direct(indio_dev);
> +		return -EBUSY;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad4052_read_chan_raw(indio_dev, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ad4052_get_oversampling_ratio(st, val);
> +		if (ret)
> +			goto out_release;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> +		if (ret)
> +			goto out_release;
> +
> +		if (!pwm_st.enabled)
> +			pwm_get_state(st->cnv_pwm, &pwm_st);
> +
> +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);

Is this the expected semantic? I.e. if the PWM isn't running report
sample freq assuming the last set period (or if the pwm wasn't set
before the configured period length set by the bootloader, or the value
specified in the device tree)?

> +
> +		ret = IIO_VAL_INT;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> [...]
> +static int ad4052_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4052_state *st = iio_priv(indio_dev);
> +	struct spi_offload_trigger_config config = {
> +		.type = SPI_OFFLOAD_TRIGGER_DATA_READY,
> +	};
> +	int ret;
> +
> +	if (st->wait_event)
> +		return -EBUSY;
> +
> +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4052_set_operation_mode(st, st->mode);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = ad4052_update_xfer_offload(indio_dev, indio_dev->channels);
> +	if (ret)
> +		goto out_error;
> +
> +	/* SPI Offload handles the data ready irq */
> +	disable_irq(st->gp1_irq);
> +
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &config);
> +	if (ret)
> +		goto out_offload_error;
> +
> +	ret = pwm_enable(st->cnv_pwm);
> +	if (ret)
> +		goto out_pwm_error;

pwm_enable() is another disguised pwm_get_state().

> +
> +	return 0;
> +
> +out_pwm_error:
> +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> +out_offload_error:
> +	enable_irq(st->gp1_irq);
> +	spi_unoptimize_message(&st->offload_msg);
> +	ad4052_exit_command(st);
> +out_error:
> +	pm_runtime_mark_last_busy(&st->spi->dev);
> +	pm_runtime_put_autosuspend(&st->spi->dev);
> +
> +	return ret;
> +}

Best regards
Uwe

Hi Uwe,

On Fri, May 16, 2025 at 12:11:56PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Apr 22, 2025 at 01:34:50PM +0200, Jorge Marques wrote:
> > +static int ad4052_set_sampling_freq(struct ad4052_state *st, unsigned int freq)
> > +{
> > +	struct pwm_state pwm_st;
> > +
> > +	if (freq <= 0 || freq > AD4052_MAX_RATE(st->grade))
> > +		return -EINVAL;
> > +
> > +	pwm_get_state(st->cnv_pwm, &pwm_st);
> > +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> > +	return pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
> 
> Is it clear that pwm_st.duty_cycle isn't greater than
> DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> 
> I'm not a big fan of pwm_get_state() because the semantic is a bit
> strange. My preferred alternative would be to either use pwm_init_state
> and initialize all fields, or maintain a struct pwm_state in struct
> ad4052_state.

Ack. I will mantain pwm_state in ad4052_state.

> 
> > +static int ad4052_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct pwm_state pwm_st;
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		iio_device_release_direct(indio_dev);
> > +		return -EBUSY;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = ad4052_read_chan_raw(indio_dev, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		ret = ad4052_get_oversampling_ratio(st, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> > +		if (ret)
> > +			goto out_release;
> > +
> > +		if (!pwm_st.enabled)
> > +			pwm_get_state(st->cnv_pwm, &pwm_st);
> > +
> > +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> 
> Is this the expected semantic? I.e. if the PWM isn't running report
> sample freq assuming the last set period (or if the pwm wasn't set
> before the configured period length set by the bootloader, or the value
> specified in the device tree)?
> 

Yes, but I will just use the (new) managed pwm_state instead:

  *val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, st->pwm_st.period);
  return IIO_VAL_INT;

> > +
> > [...]
> > +
> > +	ret = pwm_enable(st->cnv_pwm);
> > +	if (ret)
> > +		goto out_pwm_error;
> 
> pwm_enable() is another disguised pwm_get_state().
> 

Ack.

> > +
> > +	return 0;
> > +
> > +out_pwm_error:
> > +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> > +out_offload_error:
> > +	enable_irq(st->gp1_irq);
> > +	spi_unoptimize_message(&st->offload_msg);
> > +	ad4052_exit_command(st);
> > +out_error:
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +
> > +	return ret;
> > +}
> 
> Best regards
> Uwe

Best regards,
Jorge

Hi Jonathan,

On Sat, Apr 26, 2025 at 05:03:02PM +0100, Jonathan Cameron wrote:
> On Tue, 22 Apr 2025 13:34:50 +0200
> Jorge Marques <jorge.marques@analog.com> wrote:
>
> > The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit,
> > successive approximation register (SAR) analog-to-digital converter (ADC)
> > that enables low-power, high-density data acquisition solutions without
> > sacrificing precision.
> > This ADC offers a unique balance of performance and power efficiency,
> > plus innovative features for seamlessly switching between high-resolution
> > and low-power modes tailored to the immediate needs of the system.
> > The AD4052/AD4058/AD4050/AD4056 are ideal for battery-powered,
> > compact data acquisition and edge sensing applications.
> >
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> Hi Jorge,
>
> A few additional comments from me.
>
> Thanks,
>
> Jonathan
>
> > diff --git a/drivers/iio/adc/ad4052.c b/drivers/iio/adc/ad4052.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..f412f0884bab4f500091f6c7ca761967c8f6e3b6
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4052.c
> > @@ -0,0 +1,1425 @@
>
>
> > +static int ad4052_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct pwm_state pwm_st;
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		iio_device_release_direct(indio_dev);
> > +		return -EBUSY;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = ad4052_read_chan_raw(indio_dev, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > +		ret = ad4052_get_oversampling_ratio(st, val);
> > +		if (ret)
> > +			goto out_release;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		ret = pwm_get_state_hw(st->cnv_pwm, &pwm_st);
> > +		if (ret)
> > +			goto out_release;
> > +
> > +		if (!pwm_st.enabled)
> > +			pwm_get_state(st->cnv_pwm, &pwm_st);
> > +
> > +		*val = DIV_ROUND_UP_ULL(NSEC_PER_SEC, pwm_st.period);
> > +
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> > +}
>
>
> > +static int ad4052_write_event_value(struct iio_dev *indio_dev,
> > +				    const struct iio_chan_spec *chan,
> > +				    enum iio_event_type type,
> > +				    enum iio_event_direction dir,
> > +				    enum iio_event_info info, int val,
> > +				    int val2)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret = -EINVAL;
> > +	u8 reg, size = 1;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
>
> Sometimes it is worth a n internal function factored out in cases
> like this to allow direct returns in error cases with the release
> always happening before we check if the inner function failed.
>
> That suggestion applies in other places in this code.

Ack. I will refactor out the methods using auxiliary methods suffixed
with _dispatch.

> > +
> > +	if (st->wait_event) {
> > +		iio_device_release_direct(indio_dev);
> > +		return -EBUSY;
> > +	}
> > +
> > +	switch (type) {
> > +	case IIO_EV_TYPE_THRESH:
> > +		switch (info) {
> > +		case IIO_EV_INFO_VALUE:
> > +			if (st->data_format & AD4052_ADC_MODES_DATA_FORMAT) {
> > +				if (val > 2047 || val < -2048)
> > +					goto out_release;
> > +			} else if (val > 4095 || val < 0) {
> > +				goto out_release;
> > +			}
> > +			if (dir == IIO_EV_DIR_RISING)
> > +				reg = AD4052_REG_MAX_LIMIT;
> > +			else
> > +				reg = AD4052_REG_MIN_LIMIT;
> > +			size = 2;
> > +			st->d16 = cpu_to_be16(val);
> > +			break;
> > +		case IIO_EV_INFO_HYSTERESIS:
> > +			if (val & BIT(7))
> > +				goto out_release;
> > +			if (dir == IIO_EV_DIR_RISING)
> > +				reg = AD4052_REG_MAX_HYST;
> > +			else
> > +				reg = AD4052_REG_MIN_HYST;
> > +			st->d16 = cpu_to_be16(val >> 8);
> > +			break;
> > +		default:
> > +			goto out_release;
> > +		}
> > +		break;
> > +	default:
> > +		goto out_release;
> > +	}
> > +
> > +	ret = regmap_bulk_write(st->regmap, reg, &st->d16, size);
> > +
> > +out_release:
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> > +}
> > +
> > +static int ad4052_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct spi_offload_trigger_config config = {
> > +		.type = SPI_OFFLOAD_TRIGGER_DATA_READY,
> > +	};
> > +	int ret;
> > +
> > +	if (st->wait_event)
> > +		return -EBUSY;
> > +
> > +	ret = pm_runtime_resume_and_get(&st->spi->dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4052_set_operation_mode(st, st->mode);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	ret = ad4052_update_xfer_offload(indio_dev, indio_dev->channels);
> > +	if (ret)
> > +		goto out_error;
> > +
> > +	/* SPI Offload handles the data ready irq */
> > +	disable_irq(st->gp1_irq);
> > +
> > +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> > +					 &config);
> > +	if (ret)
> > +		goto out_offload_error;
> > +
> > +	ret = pwm_enable(st->cnv_pwm);
> > +	if (ret)
> > +		goto out_pwm_error;
> > +
> > +	return 0;
> > +
> > +out_pwm_error:
> > +	spi_offload_trigger_disable(st->offload, st->offload_trigger);
> > +out_offload_error:
> > +	enable_irq(st->gp1_irq);
> > +	spi_unoptimize_message(&st->offload_msg);
> > +	ad4052_exit_command(st);
>
> What is this matching to?  Feels like it would be set_operation_mode()
> but I may be wrong on that.  If it is then you need another label
> to call only this update_xfer_offload fails.
>

Yep, here an additional label is needed, thanks!
Changing to:

	out_pwm_error:
		spi_offload_trigger_disable(st->offload, st->offload_trigger);
	out_offload_error:
		enable_irq(st->gp1_irq);
		spi_unoptimize_message(&st->offload_msg);
	out_xfer_error:
		ad4052_exit_command(st);
	out_mode_error:
		pm_runtime_mark_last_busy(&st->spi->dev);
		pm_runtime_put_autosuspend(&st->spi->dev);

		return ret;

> > +out_error:
> > +	pm_runtime_mark_last_busy(&st->spi->dev);
> > +	pm_runtime_put_autosuspend(&st->spi->dev);
> > +
> > +	return ret;
> > +}
>
> > +static int ad4052_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> > +				     unsigned int writeval, unsigned int *readval)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (!iio_device_claim_direct(indio_dev))
> > +		return -EBUSY;
> > +
> > +	if (st->wait_event) {
> > +		iio_device_release_direct(indio_dev);
> Probably use a goto to release this in only one place.
>
Ack.

> > +		return -EBUSY;
> > +	}
> > +
> > +	if (readval)
> > +		ret = regmap_read(st->regmap, reg, readval);
> > +	else
> > +		ret = regmap_write(st->regmap, reg, writeval);
> > +
> > +	iio_device_release_direct(indio_dev);
> > +	return ret;
> > +}
> > +
> > +static int ad4052_get_current_scan_type(const struct iio_dev *indio_dev,
> > +					const struct iio_chan_spec *chan)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +
> > +	if (iio_buffer_enabled(indio_dev))
>
> This is the bit I'm not really following. Why is the enabling or not of
> the buffer related to whether offload is going on?
>
Will be dropped and the scan_type only depends on the oversampling value.
My misunderstanding is explained on thread
[PATCH v2 2/5] iio: code: mark iio_dev as const in iio_buffer_enabled
> > +		return st->mode == AD4052_BURST_AVERAGING_MODE ?
> > +				   AD4052_SCAN_TYPE_OFFLOAD_BURST_AVG :
> > +				   AD4052_SCAN_TYPE_OFFLOAD_SAMPLE;
> > +
> > +	return st->mode == AD4052_BURST_AVERAGING_MODE ?
> > +			   AD4052_SCAN_TYPE_BURST_AVG :
> > +			   AD4052_SCAN_TYPE_SAMPLE;
> > +}
>
>
> > +static int ad4052_probe(struct spi_device *spi)
> > +{
> ...
>
> > +
> > +	st->mode = AD4052_SAMPLE_MODE;
> > +	st->wait_event = false;
> > +	st->chip = chip;
> > +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
>
> That feels like it should be encoded directly in chip.  Basing it
> on prod_id seems liable to bite us at somepoint in the future.
>
Ack. Moved to a const chip_info.grade.
> > +
> > +static int ad4052_runtime_resume(struct device *dev)
> > +{
> > +	struct ad4052_state *st = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> > +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
> > +	return ret;
>
> 	return regmap_write();
> and no need for the local variable ret.
>

A sleep of 4ms will be added to ensure not triggering NOT_RDY_ERROR
flag, and therefore ret will be kept:

	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
	fsleep(4000);
	return ret;

> > +}
> > +
> > +static DEFINE_RUNTIME_DEV_PM_OPS(ad4052_pm_ops, ad4052_runtime_suspend,
> > +				 ad4052_runtime_resume, NULL);

Best regards,
Jorge

Hi David,

On Fri, Apr 25, 2025 at 06:13:48PM -0500, David Lechner wrote:
> On 4/22/25 6:34 AM, Jorge Marques wrote:
> > The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit,
> > successive approximation register (SAR) analog-to-digital converter (ADC)
> > that enables low-power, high-density data acquisition solutions without
> > sacrificing precision.
> > This ADC offers a unique balance of performance and power efficiency,
> > plus innovative features for seamlessly switching between high-resolution
> > and low-power modes tailored to the immediate needs of the system.
> > The AD4052/AD4058/AD4050/AD4056 are ideal for battery-powered,
> > compact data acquisition and edge sensing applications.
> > 
> > Signed-off-by: Jorge Marques <jorge.marques@analog.com>
> > ---
> >  MAINTAINERS              |    1 +
> >  drivers/iio/adc/Kconfig  |   14 +
> >  drivers/iio/adc/Makefile |    1 +
> >  drivers/iio/adc/ad4052.c | 1425 ++++++++++++++++++++++++++++++++++++++++++++++
> 
> This patch is way too big, so I didn't review most of it yet. But time to call
> it quits for today. In the future, it would be a lot easier for reviewers if
> you can split things into multiple patches instead of implementing all of the
> features at once. E.g. start with just a basic driver, then a patch to add
> oversampling support, then another patch to add SPI offload support. 500 lines
> is a more manageable size for review.
> 

Ack. I split v3 into three commits: base, offload and iio events.
My playground is here: https://github.com/gastmaier/adi-linux/pull/9

> ...
> 
> > +static int ad4052_update_xfer_offload(struct iio_dev *indio_dev,
> > +				      struct iio_chan_spec const *chan)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	const struct iio_scan_type *scan_type;
> > +	struct spi_transfer *xfer = &st->xfer;
> > +
> > +	scan_type = iio_get_current_scan_type(indio_dev, chan);
> > +
> > +	if (IS_ERR(scan_type))
> > +		return PTR_ERR(scan_type);
> > +
> > +	xfer = &st->offload_xfer;
> > +	xfer->bits_per_word = scan_type->realbits;
> > +	xfer->len = BITS_TO_BYTES(scan_type->storagebits);
> 
> This doesn't work for oversampling. realbits may be 16 while storagebits is 32.
> But the SPI controller needs to know how many realbits-sized words to read.
> 
> So this should be 
> 
> 	xfer->len = BITS_TO_BYTES(scan_type->realbits);
> 

Agreed, but due to spi message optimization needs to be:

  	xfer->len = scan_type->realbits == 24 ? 4 : 2;

Because 3 bytes cannot be optimized.
> > +
> > +	spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> > +	st->offload_msg.offload = st->offload;
> > +
> > +	return spi_optimize_message(st->spi, &st->offload_msg);
> 
> I know it is like this in a few other drivers already, but I don't like having
> spi_optimize_message() in this funtion because it makes it really easy to
> forget to do have balanced calls to spi_unoptimize_message().
> 

Ack.

> > +}
> > +
> 
> ...
> 
> > +static const struct iio_buffer_setup_ops ad4052_buffer_setup_ops = {
> > +	.postenable = &ad4052_buffer_postenable,
> > +	.predisable = &ad4052_buffer_predisable,
> > +};
> 
> Would be nice to add "offload" to the name of this struct and the callbacks
> to make it clear that these are only for the SPI offload use case.
> 
Ack.
> ...
> 
> > +
> > +static bool ad4052_offload_trigger_match(struct spi_offload_trigger *trigger,
> > +					 enum spi_offload_trigger_type type,
> > +					 u64 *args, u32 nargs)
> > +{
> 
> We should be checking the args here according to what I suggested in my reply
> to the devicetree bindings patch. Right now it is assuming that we are only
> using this for SPI offload and that the pin used is GP1 and the event is data
> read. We should at least verify that the args match those assumptions.
> 
> For bonus points, we could implement allowing GPO as well.
> 

Yes, it is assuming as you mentioned.
I'm okay with "at least verifying".
but then I need to look-up at the parent node first, since it resides at
the spi-controller node, if that's ok.

> > +	return type == SPI_OFFLOAD_TRIGGER_DATA_READY;
> > +}
> > +
> > +static const struct spi_offload_trigger_ops ad4052_offload_trigger_ops = {
> > +	.match = ad4052_offload_trigger_match,
> > +};
> > +
> > +static int ad4052_request_offload(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4052_state *st = iio_priv(indio_dev);
> > +	struct device *dev = &st->spi->dev;
> > +	struct dma_chan *rx_dma;
> > +	struct spi_offload_trigger_info trigger_info = {
> > +		.fwnode = dev_fwnode(dev),
> > +		.ops = &ad4052_offload_trigger_ops,
> > +		.priv = st,
> > +	};
> > +	struct pwm_state pwm_st;
> > +	int ret;
> > +
> > +	indio_dev->setup_ops = &ad4052_buffer_setup_ops;
> > +
> > +	ret = devm_spi_offload_trigger_register(dev, &trigger_info);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to register offload trigger\n");
> 
> Strictly speaking, the trigger-source provider is indendant of using it for
> SPI offload. I guess this is fine here for now though.
> 
Ok.
> > +
> > +	st->offload_trigger = devm_spi_offload_trigger_get(dev, st->offload,
> > +							   SPI_OFFLOAD_TRIGGER_DATA_READY);
> > +	if (IS_ERR(st->offload_trigger))
> > +		return PTR_ERR(st->offload_trigger);
> > +
> > +	st->cnv_pwm = devm_pwm_get(dev, NULL);
> > +	if (IS_ERR(st->cnv_pwm))
> > +		return dev_err_probe(dev, PTR_ERR(st->cnv_pwm),
> > +				     "failed to get CNV PWM\n");
> > +
> > +	pwm_init_state(st->cnv_pwm, &pwm_st);
> > +
> > +	pwm_st.enabled = false;
> > +	pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2;
> > +	pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC,
> > +					 AD4052_MAX_RATE(st->grade));
> > +
> > +	ret = pwm_apply_might_sleep(st->cnv_pwm, &pwm_st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to apply CNV PWM\n");
> > +
> > +	ret = devm_add_action_or_reset(dev, ad4052_pwm_disable, st->cnv_pwm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	rx_dma = devm_spi_offload_rx_stream_request_dma_chan(dev, st->offload);
> > +	if (IS_ERR(rx_dma))
> > +		return PTR_ERR(rx_dma);
> > +
> > +	return devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev, rx_dma,
> > +							   IIO_BUFFER_DIRECTION_IN);
> > +}
> > +
> > +static int ad4052_probe(struct spi_device *spi)
> > +{
> > +	const struct ad4052_chip_info *chip;
> > +	struct device *dev = &spi->dev;
> > +	struct iio_dev *indio_dev;
> > +	struct ad4052_state *st;
> > +	int ret = 0;
> > +
> > +	chip = spi_get_device_match_data(spi);
> > +	if (!chip)
> > +		return dev_err_probe(dev, -ENODEV,
> > +				     "Could not find chip info data\n");
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +	st->spi = spi;
> > +	spi_set_drvdata(spi, st);
> > +	init_completion(&st->completion);
> > +
> > +	st->regmap = devm_regmap_init_spi(spi, &ad4052_regmap_config);
> > +	if (IS_ERR(st->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> > +				     "Failed to initialize regmap\n");
> > +
> > +	st->mode = AD4052_SAMPLE_MODE;
> > +	st->wait_event = false;
> > +	st->chip = chip;
> > +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
> > +	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
> > +	st->events_frequency = AD4052_FS_OFFSET(st->grade);
> 
> Somewhere around here, we should be turning on the power supplies. Also, it
> looks like we need some special handling to get the reference volage. If there
> is a supply connected to REF, use that, if not, use VDD which requires writing
> to a register to let the chip know.
> 
Yes, v3 will add regulators.
Vref can be sourced from either REF (default) or VDD.
So the idea is, if REF node not provided, set VDD as REF?

> > +
> > +	st->cnv_gp = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
> > +	if (IS_ERR(st->cnv_gp))
> > +		return dev_err_probe(dev, PTR_ERR(st->cnv_gp),
> > +				     "Failed to get cnv gpio\n");
> > +
> > +	indio_dev->modes = INDIO_BUFFER_HARDWARE | INDIO_DIRECT_MODE;
> 
> INDIO_BUFFER_HARDWARE should not be set here. If using SPI offload,
> devm_iio_dmaengine_buffer_setup_with_handle() will add it automatically.
> For non-SPI-offload operation, it should not be set.
> 
Ack.
> > +	indio_dev->num_channels = 1;
> > +	indio_dev->info = &ad4052_info;
> > +	indio_dev->name = chip->name;
> > +
> > +	st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config);
> 
> This
> 
> > +	if (IS_ERR(st->offload))
> > +		return PTR_ERR(st->offload);
> 
> should be
> 
> 	ret = PTR_ERR_OR_ZERO(st->offload);
> 
Ack.

> > +
> > +	if (ret && ret != -ENODEV)
> > +		return dev_err_probe(dev, ret, "Failed to get offload\n");
> > +
> > +	if (ret == -ENODEV) {
> > +		st->offload_trigger = NULL;
> > +		indio_dev->channels = chip->channels;
> > +	} else {
> > +		indio_dev->channels = chip->offload_channels;
> > +		ret = ad4052_request_offload(indio_dev);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to configure offload\n");
> > +	}
> > +
> > +	st->xfer.rx_buf = &st->d32;
> 
> I don't think we want this set globally. I.e. it doesn't make sense for SPI
> offload xfers.
> 
Ack.

> > +
> > +	ret = ad4052_soft_reset(st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "AD4052 failed to soft reset\n");
> > +
> > +	ret = ad4052_check_ids(st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "AD4052 fields assertions failed\n");
> > +
> > +	ret = ad4052_setup(indio_dev, indio_dev->channels);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS,
> > +			   AD4052_DEVICE_STATUS_DEVICE_RESET);
> 
> Why not include this in ad4052_setup() or even ad4052_soft_reset()?
> 
Ack.
But on setup to not write registers before doing the sanity test.

> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ad4052_request_irq(indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ad4052_update_xfer_raw(indio_dev, indio_dev->channels);
> > +
> > +	pm_runtime_set_active(dev);
> > +	ret = devm_pm_runtime_enable(dev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "Failed to enable pm_runtime\n");
> > +
> > +	pm_runtime_set_autosuspend_delay(dev, 1000);
> > +	pm_runtime_use_autosuspend(dev);
> > +
> > +	return devm_iio_device_register(dev, indio_dev);
> > +}
> > +
> > +static int ad4052_runtime_suspend(struct device *dev)
> > +{
> > +	struct ad4052_state *st = dev_get_drvdata(dev);
> > +
> > +	return regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> > +			    FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK,
> > +				       AD4052_DEVICE_CONFIG_LOW_POWER_MODE));
> > +}
> > +
> > +static int ad4052_runtime_resume(struct device *dev)
> > +{
> > +	struct ad4052_state *st = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG,
> > +			   FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0));
> 
> regmap_clear_bits() would be shorter if there isn't going to be a macro to
> explain the meaning of 0.
> 
Ack.
> > +	return ret;
> > +}
> > +

Regards,
Jorge

On 6/2/25 6:16 AM, Jorge Marques wrote:
> Hi David,
> 
> On Fri, Apr 25, 2025 at 06:13:48PM -0500, David Lechner wrote:
>> On 4/22/25 6:34 AM, Jorge Marques wrote:

...

>>> +static int ad4052_probe(struct spi_device *spi)
>>> +{
>>> +	const struct ad4052_chip_info *chip;
>>> +	struct device *dev = &spi->dev;
>>> +	struct iio_dev *indio_dev;
>>> +	struct ad4052_state *st;
>>> +	int ret = 0;
>>> +
>>> +	chip = spi_get_device_match_data(spi);
>>> +	if (!chip)
>>> +		return dev_err_probe(dev, -ENODEV,
>>> +				     "Could not find chip info data\n");
>>> +
>>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>>> +	if (!indio_dev)
>>> +		return -ENOMEM;
>>> +
>>> +	st = iio_priv(indio_dev);
>>> +	st->spi = spi;
>>> +	spi_set_drvdata(spi, st);
>>> +	init_completion(&st->completion);
>>> +
>>> +	st->regmap = devm_regmap_init_spi(spi, &ad4052_regmap_config);
>>> +	if (IS_ERR(st->regmap))
>>> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
>>> +				     "Failed to initialize regmap\n");
>>> +
>>> +	st->mode = AD4052_SAMPLE_MODE;
>>> +	st->wait_event = false;
>>> +	st->chip = chip;
>>> +	st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS;
>>> +	st->oversampling_frequency = AD4052_FS_OFFSET(st->grade);
>>> +	st->events_frequency = AD4052_FS_OFFSET(st->grade);
>>
>> Somewhere around here, we should be turning on the power supplies. Also, it
>> looks like we need some special handling to get the reference voltage. If there
>> is a supply connected to REF, use that, if not, use VDD which requires writing
>> to a register to let the chip know.
>>
> Yes, v3 will add regulators.
> Vref can be sourced from either REF (default) or VDD.
> So the idea is, if REF node not provided, set VDD as REF?
> 
Yes, you can do this with devm_regulator_get_enable_read_voltage() if it
returns -ENODEV, for the REF supply, then call it again on the VDD supply.