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@xxxxxxxxxx> 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>
> 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
