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

[David Lechner]

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@xxxxxxxxxx>
> ---
>  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;
> +}
> +