From: Jonathan Cameron <jic23@kernel.org>
To: "Nuno Sá" <noname.nuno@gmail.com>
Cc: Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org>,
nuno.sa@analog.com, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org,
Jonathan Cameron <Jonathan.Cameron@huawei.com>,
Alexandru Ardelean <alexandru.ardelean@analog.com>,
Lars-Peter Clausen <lars@metafoo.de>,
Michael Hennerich <Michael.Hennerich@analog.com>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Olivier Moysan <olivier.moysan@foss.st.com>
Subject: Re: [PATCH 6/8] iio: adc: adi-axi-adc: support digital interface calibration
Date: Sun, 28 Apr 2024 16:49:51 +0100 [thread overview]
Message-ID: <20240428164951.0651845e@jic23-huawei> (raw)
In-Reply-To: <96d226df0670cdfc307aacb196bea13e1ad644ad.camel@gmail.com>
On Tue, 23 Apr 2024 09:27:26 +0200
Nuno Sá <noname.nuno@gmail.com> 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@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.
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
next prev parent reply other threads:[~2024-04-28 15:50 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-19 15:36 [PATCH 0/8] iio: ad9467: support interface tuning Nuno Sa via B4 Relay
2024-04-19 15:36 ` [PATCH 1/8] iio: backend: add API for " Nuno Sa via B4 Relay
2024-04-20 15:00 ` Jonathan Cameron
2024-04-22 15:40 ` Nuno Sá
2024-04-22 17:13 ` Jonathan Cameron
2024-04-23 7:52 ` Nuno Sá
2024-04-28 15:46 ` Jonathan Cameron
2024-04-19 15:36 ` [PATCH 2/8] iio: adc: adi-axi-adc: only error out in major version mismatch Nuno Sa via B4 Relay
2024-04-20 15:02 ` Jonathan Cameron
2024-04-19 15:36 ` [PATCH 3/8] dt-bindings: adc: axi-adc: add clocks property Nuno Sa via B4 Relay
2024-04-19 16:11 ` Krzysztof Kozlowski
2024-04-20 15:04 ` Jonathan Cameron
2024-04-22 15:06 ` Nuno Sá
2024-04-22 17:09 ` Jonathan Cameron
2024-04-19 15:36 ` [PATCH 4/8] iio: adc: axi-adc: make sure AXI clock is enabled Nuno Sa via B4 Relay
2024-04-20 15:04 ` Jonathan Cameron
2024-04-19 15:36 ` [PATCH 5/8] iio: adc: adi-axi-adc: remove regmap max register Nuno Sa via B4 Relay
2024-04-19 15:36 ` [PATCH 6/8] iio: adc: adi-axi-adc: support digital interface calibration Nuno Sa via B4 Relay
2024-04-20 15:13 ` Jonathan Cameron
2024-04-23 7:27 ` Nuno Sá
2024-04-28 15:49 ` Jonathan Cameron [this message]
2024-04-19 15:36 ` [PATCH 7/8] iio: adc: ad9467: cache the sample rate Nuno Sa via B4 Relay
2024-04-20 15:19 ` Jonathan Cameron
2024-04-22 15:46 ` Nuno Sá
2024-04-22 17:08 ` Jonathan Cameron
2024-04-19 15:36 ` [PATCH 8/8] iio: adc: ad9467: support digital interface calibration Nuno Sa via B4 Relay
2024-04-20 15:34 ` Jonathan Cameron
2024-04-23 7:32 ` Nuno Sá
2024-04-20 15:39 ` [PATCH 0/8] iio: ad9467: support interface tuning Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240428164951.0651845e@jic23-huawei \
--to=jic23@kernel.org \
--cc=Jonathan.Cameron@huawei.com \
--cc=Michael.Hennerich@analog.com \
--cc=alexandru.ardelean@analog.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=devnull+nuno.sa.analog.com@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=noname.nuno@gmail.com \
--cc=nuno.sa@analog.com \
--cc=olivier.moysan@foss.st.com \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).