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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 23 Apr 2024 09:27:26 +0200
Nuno Sá <noname.nuno@xxxxxxxxx> wrote:

> 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@xxxxxxxxxx> wrote:
> >   
> > > From: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>
> > > ---
> > >  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.

Ah.  You meant protecting a sequence of multiple accesses. I was thinking 'against'
multiple simultaneous accesses.  I'm fine with your intended meaning. Perhaps
we just need to refer to sequences of access or something like that.

> 
> 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).

Protection makes sense, I was just moaning about the meaning of the comment :)

Jonathan






[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux