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