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

[Jorge Marques]

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