Message-ID: <zofjbh4yvtz4sfj2t6cpdohqqlrgwqdqtiahpvalbbfv2tdqqi@g5zpdp3zn4gb>
Date: Mon, 2 Jun 2025 13:16:22 +0200
From: Jorge Marques <gastmaier@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, 
	Michael Hennerich <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jonathan Corbet <corbet@....net>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Uwe Kleine-König <ukleinek@...nel.org>, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v2 5/5] iio: adc: add support for ad4052

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@...log.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

Powered by blists - more mailing lists