Re: [PATCH 6/8] iio: adc: adi-axi-adc: support digital interface calibration

["Nuno Sá" <noname.nuno@gmail.com>]

On Sat, 2024-04-20 at 16:13 +0100, Jonathan Cameron wrote:
> On Fri, 19 Apr 2024 17:36:49 +0200
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > Implement the new IIO backend APIs for calibrating the data
> > digital interfaces.
> > 
> > While at it, removed the tabs in 'struct adi_axi_adc_state' and used
> > spaces for the members.
> 
> Ideally a precursor patch, but meh, it's 2 lines so I'll just moan about
> it and move on.
> 
> A few minor things inline.
> 
> 
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/adc/adi-axi-adc.c | 113 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 111 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> > index b312369b7366..d58fa05499c4 100644
> > --- a/drivers/iio/adc/adi-axi-adc.c
> > +++ b/drivers/iio/adc/adi-axi-adc.c
> > @@ -7,11 +7,13 @@
> >   */
> >  
> >  #include <linux/bitfield.h>
> > +#include <linux/cleanup.h>
> >  #include <linux/clk.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> >  #include <linux/delay.h>
> >  #include <linux/module.h>
> > +#include <linux/mutex.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/property.h>
> > @@ -37,6 +39,9 @@
> >  #define   ADI_AXI_REG_RSTN_MMCM_RSTN		BIT(1)
> >  #define   ADI_AXI_REG_RSTN_RSTN			BIT(0)
> >  
> > +#define ADI_AXI_ADC_REG_CTRL			0x0044
> > +#define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
> > +
> >  /* ADC Channel controls */
> >  
> >  #define ADI_AXI_REG_CHAN_CTRL(c)		(0x0400 + (c) * 0x40)
> > @@ -51,14 +56,28 @@
> >  #define   ADI_AXI_REG_CHAN_CTRL_PN_TYPE_OWR	BIT(1)
> >  #define   ADI_AXI_REG_CHAN_CTRL_ENABLE		BIT(0)
> >  
> > +#define ADI_AXI_ADC_REG_CHAN_STATUS(c)		(0x0404 + (c) * 0x40)
> > +#define   ADI_AXI_ADC_CHAN_STAT_PN_MASK		GENMASK(2, 1)
> > +
> > +#define ADI_AXI_ADC_REG_CHAN_CTRL_3(c)		(0x0418 + (c) * 0x40)
> > +#define   ADI_AXI_ADC_CHAN_PN_SEL_MASK		GENMASK(19, 16)
> > +
> > +/* IO Delays */
> > +#define ADI_AXI_ADC_REG_DELAY(l)		(0x0800 + (l) * 0x4)
> > +#define   AXI_ADC_DELAY_CTRL_MASK		GENMASK(4, 0)
> > +
> > +#define ADI_AXI_ADC_MAX_IO_NUM_LANES		15
> > +
> >  #define ADI_AXI_REG_CHAN_CTRL_DEFAULTS		\
> >  	(ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT |	\
> >  	 ADI_AXI_REG_CHAN_CTRL_FMT_EN |		\
> >  	 ADI_AXI_REG_CHAN_CTRL_ENABLE)
> >  
> >  struct adi_axi_adc_state {
> > -	struct regmap				*regmap;
> > -	struct device				*dev;
> > +	struct regmap *regmap;
> > +	struct device *dev;
> > +	/* lock to protect multiple accesses to the device registers */
> 
> Why?  The locking in regmap protects register accesses in general I believe.
> I guess this is to prevent accesses during that error detection routine?
> Needs more detail in the comment.

Yes, but if you have, for example, two regmap_*() calls in a row you won't have the
complete access protected as regmap only internally protects each call. And often,
trying to argue which of these multiple accesses are safe or not is very subtle and
prone to issues. That's why I used the "multiple accesses" wording.

As you pointed out, the error detection routine should indeed be atomic. On the
iodelay_set() we also have a read after write and this is one of those cases I'm not
sure we could actually have an issue if we have concurrent calls (maybe not), But for
correctness, it definitely makes sense for it to be atomic (as we should check we
could set the value we just wrote and not something else).

Also, on a second look, the enable/disable() routines should also be protected (for
correctness). If we think on the theoretical (in practice it should not happen :))
case of concurrent enable/disable() calls, we should not be able to disable the core
in the middle of enabling (either before or after).

> 
> > +	struct mutex lock;
> >  };
> >  
> >  static int axi_adc_enable(struct iio_backend *back)
> > @@ -104,6 +123,92 @@ static int axi_adc_data_format_set(struct iio_backend *back,
> > unsigned int chan,
> >  				  ADI_AXI_REG_CHAN_CTRL_FMT_MASK, val);
> >  }
> >  
> > +static int axi_adc_data_sample_trigger(struct iio_backend *back,
> > +				       enum iio_backend_sample_trigger trigger)
> > +{
> > +	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> > +
> > +	if (trigger == IIO_BACKEND_SAMPLE_TRIGGER_EDGE_RISING)
> 
> It's an enum, so can't have more than one. Hence switch statement is probably
> more extensible and natural to read.

alright..

> 
> > +		return regmap_clear_bits(st->regmap, ADI_AXI_ADC_REG_CTRL,
> > +					 ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK);
> > +	if (trigger == IIO_BACKEND_SAMPLE_TRIGGER_EDGE_FALLING)
> > +		return regmap_set_bits(st->regmap, ADI_AXI_ADC_REG_CTRL,
> > +				       ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK);
> > +
> > +	return -EINVAL;
> > +}
> > +
> 
> 
> > +static int axi_adc_chan_status(struct iio_backend *back, unsigned int chan,
> > +			       struct iio_backend_chan_status *status)
> > +{
> > +	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> > +	int ret;
> > +	u32 val;
> > +
> > +	guard(mutex)(&st->lock);
> > +	/* reset test bits by setting them */
> > +	ret = regmap_write(st->regmap, ADI_AXI_ADC_REG_CHAN_STATUS(chan),
> > +			   ADI_AXI_ADC_CHAN_STAT_PN_MASK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	fsleep(1000);
> Why this particular length sleep?  Is this a case of let it sit a while an dsee
> if an error shows up?  If so a comment on that would be good.

Exactly, will add a comment.

- Nuno Sá
>