Hi Jonathan thank you for the review, comments excluded in the reply are
agreed and applied.
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > To compile this driver as a module, choose M here: the module will be
> > called ad4130.
> >
> > +config AD4052
> Aim for alphanumeric order so this should at least be before AD4130
Ups, I got tricked during cherry-pick.
> > +#define AD4052_MIN_FLAG BIT(2)
> > +#define AD4052_EVENT_CLEAR (AD4052_THRESH_OVERRUN | AD4052_MAX_FLAG | AD4052_MIN_FLAG)
> Wrap the define with \ to break the line.
Deadcode... removed.
> > +};
> > +
> > +static const char *const ad4052_sample_rates[] = {
> > + "2000000", "1000000", "300000", "100000", "33300",
> > + "10000", "3000", "500", "333", "250", "200",
> > + "166", "140", "124", "111",
> Not sure why this can't be done with read_avail and the values above.
Since it is the internal device sample rate, it is an extended
device attribute.
The channel IIO_SAMPLING_FREQUENCY is used for the sampling frequency during
buffer readings.
The macro IIO_ENUM_AVAILABLE is used to reduce redundancy.
The previous integer declaration was unused since the char array index is
used as the register r/w value.
> > +};
> > +
> > +static int ad4052_iio_device_claim_direct(struct iio_dev *indio_dev,
> > + struct ad4052_state *st)
> > +{
> > + if (!iio_device_claim_direct(indio_dev))
> > + return false;
>
> This might stretch sparses ability to keep track or __acquire / __release.
> Make sure to check that with a C=1 build. If the cond_acquires
> stuff is merged into sparse, this may need a revisit for markings.
You are right!
I also did further fixes caught by sparse.
> > +{
> > + int ret;
> > +
> > + if (st->mode == AD4052_SAMPLE_MODE) {
> > + *val = 0;
>
> Probably = 1 to reflect no oversampling.
> IIRC the attribute allows either but to me at least a default of 1
> is more logical.
Agreed, 1 is now the only no oversampling value.
> > +}
> > +
> > +static int ad4052_assert(struct ad4052_state *st)
> Slighly odd name. check_ids or something like that.
>
Went with 'check_ids'.
> > +
> > + val = be16_to_cpu(st->d16);
> > + if (val != st->chip->prod_id)
> > + return -ENODEV;
>
> Should not be treated as a failure as that breaks the future use of
> fallback compatible values in DT (support new hardware on old kernel)
> Instead just print a message saying it didn't match and carry on as if it did.
Noted, added:
"Production ID x%x does not match known values", val);
> > +{
> > + struct ad4052_state *st = iio_priv(indio_dev);
> > + struct device *dev = &st->spi->dev;
> > + int ret = 0;
> > +
> > + ret = fwnode_irq_get(dev_fwnode(&st->spi->dev), 0);
>
> As per the binding review, use named variant as we should
> not be controlling the order, but rather specifying which
> is which in the dt-binding.
Makes more sense indeed.
> > + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>
> Direction should come from firmware, not be specified here.
> There might be an inverter in the path. That used to be a common cheap
> way of doing level conversion for interrupt lines and it is handled by
> flipping the sense of the interrupt in the dts.
>
Agreed, defined the level flags as 0, and kept IRQF_ONESHOT the irq
flag, to dismiss the threaded IRQ with NULL handler needs to be oneshot.
> > +static int ad4052_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + bool state)
> > +{
> > + struct ad4052_state *st = iio_priv(indio_dev);
> > + int ret = -EBUSY;
> > +
> > + if (!iio_device_claim_direct(indio_dev))
> > + return -EBUSY;
> > +
> > + if (st->wait_event == state)
> > + goto out_release;
> > +
> > + if (state) {
> > + ret = pm_runtime_resume_and_get(&st->spi->dev);
> > + if (ret)
> > + goto out_release;
> > +
> > + ret = ad4052_set_operation_mode(st, AD4052_MONITOR_MODE);
> > + if (ret)
> > + goto out_err_suspend;
> Given the error handling is different in the two paths, I'd
> split this into two helpers - one each for enable and disable.
> Probably take the direct claim around where they are called.
Yes, that will make it easier to follow.
> > +
> > + ret = ad4052_update_xfer_offload(indio_dev, indio_dev->channels);
> > + if (ret)
> > + goto out_error;
> > +
> > + disable_irq(st->gp1_irq);
>
> Add a comment on why.
Added
/* SPI Offload handles the data ready irq */
> > + struct ad4052_state *st = iio_priv(indio_dev);
> > +
> > + /*
> > + * REVISIT: the supported offload has a fixed length of 32-bits
> > + * to fit the 24-bits oversampled case, requiring the additional
> > + * offload scan types.
> > + */
>
> That's an additional feature I think. We don't need to have a comment
> about things we haven't done in the driver.
Removed comment.
> > + 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);
> > + ret = PTR_ERR_OR_ZERO(st->offload);
>
> Use IS_ERR() to detect error and PTR_ERR() to get it if needed (will
> end up calling PTR_ERR() twice but similar anyway.
Ok.
> > + ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS, buf);
> > + 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_autosuspend_delay(dev, 1000);
> > + pm_runtime_use_autosuspend(dev);
>
> These autosuspend things are normally done after enabling runtime pm.
> If nothing else that keeps the devm cleanup as being in opposite
> order of what is set up here.
> https://elixir.bootlin.com/linux/v6.13.5/source/drivers/base/power/runtime.c#L1548
Makes sense.
> > + if (ret)
> > + return ret;
> > +
> > + fsleep(2000);
>
> Sleeps like this should ideally have a spec reference as a comment to
> justify why that value is chosen.
>
This sleep is not needed since there is no timing specification in the
datasheet, removed.
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops ad4052_pm_ops = {
> > + RUNTIME_PM_OPS(ad4052_runtime_suspend, ad4052_runtime_resume, NULL)
> Can you allow this to be used for suspend and resume as well? e.g.
> DEFINE_RUNTIME_DEV_PM_OPS()
>
> It is a rare case where that isn't safe to do even if there might be
> deeper sleep states available that would be even better.
Yes.
> > + {}
> Trivial but I'm slowly trying to standardize formatting of these tables
> in IIO and I randomly decided to go with
> { }
> Please use that for terminating entries in this new driver.
Done on all instances.
I will wait further reviews before resubmitting the patch
If useful for other reviewers, my current head is at
https://github.com/analogdevicesinc/linux/tree/staging/ad4052
Jorge
