Message-ID: <3sltihsbo7z4sqyicwkbeg4wsmebz2ilv77t2bqadv3wtndfeq@cdk5kpq6qnnr>
Date: Mon, 2 Jun 2025 12:50:27 +0200
From: Jorge Marques <gastmaier@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	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>, 
	David Lechner <dlechner@...libre.com>, 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 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@...log.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@...log.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

Powered by blists - more mailing lists