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
