On Sat, 2024-04-20 at 16:00 +0100, Jonathan Cameron wrote:
> On Fri, 19 Apr 2024 17:36:44 +0200
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:
>
> > From: Nuno Sa <nuno.sa@xxxxxxxxxx>
> >
> > This is in preparation for supporting interface tuning in one for the
> > devices using the axi-adc backend. The new added interfaces are all
> > needed for that calibration:
>
> Would be good to have a little more info in this commit message on what
> interface tuning involves? I hope a tuning fork and a very good sense
will do
> of hearing...
>
> >
> > * iio_backend_test_pattern_set();
> > * iio_backend_chan_status();
> > * iio_backend_iodelay_set();
> > * iio_backend_data_sample_trigger().
> >
> > Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> Otherwise, trivial stuff inline. Mostly looks fine.
>
> I appreciate you pointed out the taps thing was unit free and hence
> possibly controversial. Not much we can do about it and reality is
> its a tweak factor - things like calibbias are unit free as well
> for exactly the reason that they tend to be incredibly hardware dependent
> and sometimes even instance of hardware dependent.
>
Agreed. We could do the iodelays_get() dance but I don't think that would be that
beneficial...
> Jonathan
>
> > ---
> > drivers/iio/industrialio-backend.c | 86 ++++++++++++++++++++++++++++++++++++++
> > include/linux/iio/backend.h | 57 +++++++++++++++++++++----
> > 2 files changed, 136 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> > backend.c
> > index 2fea2bbbe47fd..45eea3b725a35 100644
> > --- a/drivers/iio/industrialio-backend.c
> > +++ b/drivers/iio/industrialio-backend.c
> > @@ -186,6 +186,92 @@ int iio_backend_data_format_set(struct iio_backend *back,
> > unsigned int chan,
> > }
> > EXPORT_SYMBOL_NS_GPL(iio_backend_data_format_set, IIO_BACKEND);
> >
> > +/**
> > + * iio_backend_test_pattern_set - Configure a test pattern
> > + * @back: Backend device
> > + * @chan: Channel number
> > + * @pattern:
> > + *
> > + * Configure a test pattern on the backend. This is typically used for
> > + * calibrating the timings on the data digital interface.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int iio_backend_test_pattern_set(struct iio_backend *back,
> > + unsigned int chan,
> > + enum iio_backend_test_pattern pattern)
> > +{
> > + if (pattern >= IIO_BACKEND_TEST_PATTERN_MAX)
> > + return -EINVAL;
> > +
> > + return iio_backend_op_call(back, test_pattern_set, chan, pattern);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_backend_test_pattern_set, IIO_BACKEND);
> > +
> > +/**
> > + * iio_backend_chan_status - Get the channel status
> > + * @back: Backend device
> > + * @chan: Channel number
> > + * @status: Channel status
>
> Feels premature to define a structure for status when it simply returns if
> there is an error so far. Maybe simplify for now, and revisit once that
> structure needs to be more complex?
Can do that. No strong feelings :). I'll strip out the boolean and drop the struct.
>
> > + *
> > + * Get the current state of the backend channel. Typically used to check if
> > + * there were any errors sending/receiving data.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int iio_backend_chan_status(struct iio_backend *back, unsigned int chan,
> > + struct iio_backend_chan_status *status)
> > +{
> > + return iio_backend_op_call(back, chan_status, chan, status);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_backend_chan_status, IIO_BACKEND);
> > +
> > +/**
> > + * iio_backend_iodelay_set - Set digital I/O delay
> > + * @back: Backend device
> > + * @lane: Lane number
> > + * @tap: Number of taps
> > + *
> > + * Controls delays on sending/receiving data. One usecase for this is to
> > + * calibrate the data digital interface so we get the best results when
> > + * transferring data. Note that @tap has no unit since the actual delay per tap
> > + * is very backend specific. Hence, frontend devices typically should go through
> > + * an array of @taps (the size of that array should typically match the size of
> > + * calibration points on the frontend device) and call this API.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int iio_backend_iodelay_set(struct iio_backend *back, unsigned int lane,
> > + unsigned int tap)
>
> taps maybe given it's a number of them?
yeps...
> Is this an industry standard term - sounds like it probably is but my
> google fu is failing.
>
Not really (I think). It's very AMD/Xilinx specific. If you google for Xilinx IDELAY
control you may found something. I could not find a good name (originally I just had
'delay' but without a proper unit it felt weird), so I admit I used the one it made
more sense for my specific usecase. Open to suggestions though :).
> > +{
> > + return iio_backend_op_call(back, iodelay_set, lane, tap);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_backend_iodelay_set, IIO_BACKEND);
> > +
> > +/**
> > + * iio_backend_data_sample_trigger - Control when to sample data
> > + * @back: Backend device
> > + * @trigger: Data trigger
> > + *
> > + * Mostly useful for input backends. Configures the backend for when to sample
> > + * data (eg: rising vs falling edge).
>
> Feels like it might become a flags field at some point, but enum is fine for
> trigger for now I guess.
>
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int iio_backend_data_sample_trigger(struct iio_backend *back,
> > + enum iio_backend_sample_trigger trigger)
> > +{
> > + if (trigger >= IIO_BACKEND_SAMPLE_TRIGGER_MAX)
> > + return -EINVAL;
> > +
> > + return iio_backend_op_call(back, data_sample_trigger, trigger);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_backend_data_sample_trigger, IIO_BACKEND);
> > +
> > static void iio_backend_free_buffer(void *arg)
> > {
> > struct iio_backend_buffer_pair *pair = arg;
> > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> > index a6d79381866ec..ad793fe0d78c2 100644
> > --- a/include/linux/iio/backend.h
> > +++ b/include/linux/iio/backend.h
> > @@ -15,6 +15,19 @@ enum iio_backend_data_type {
> > IIO_BACKEND_DATA_TYPE_MAX
> > };
> >
> > +/* vendor specific from 32 */
> > +enum iio_backend_test_pattern {
> > + /* modified prbs9 */
> > + IIO_BACKEND_ADI_PRBS_9A = 32,
>
> Not knowing anything much about this, does it make sense to use an enum,
> or should we face facts that we can't have a true generic interface
> and just use a suitably sized int?
>
I'm also not a fan of the above but we do have generic/standard patterns in this core
(and that could be used by others):
- 0x0: pn9a (device specific, modified pn9)
- 0x1: pn23a (device specific, modified pn23)
- 0x4: pn7 (standard O.150)
- 0x5: pn15 (standard O.150)
- 0x6: pn23 (standard O.150)
- 0x7: pn31 (standard O.150)
- 0x9: pnX (device specific, e.g. ad9361)
- 0x0A: Nibble ramp (Device specific e.g. adrv9001)
- 0x0B: 16 bit ramp
Lucky enough the user we have for this is only using a custom/modified pattern. my
issue with the int is that how do frontends know what value do they need to pass into
the API? It would really be very backend specific. I know we do expect frontends to
have some assumed knowledge on the backend they're connected too but I would like to
avoid making those assumptions bigger than they need to be.
My expectation with the enum is that we can have some "contract" between backends and
frontends on the pattern to use. I guess we could give it a try (unless you have some
other idea) and if it starts going out of control, I can assume defeat and change it
to an int.
Or, is the idea to just have the int parameter and some plain defines in the backend
header?
> How do you unset the test pattern? I expected a IIO_BACKEND_NO_TESTPATERN = 0
> or something like that.
>
Since this is on the input direction (and for our particular core), we don't have to
unset it. When you choose a test pattern, it just tells the core to match for a
specific signal/pattern. So when you do start getting "real" data, we may still have
those status bits saying there are "errors" but in reality we don't care. We just
care during the tuning/calibration procedure as we configure matching patters between
frontend and backend...
OTOH for the axi-dac, for example, we do need to unset the test pattern. And we do
that by (re)configuring the internal CW tone or the external data source (typically
some DMA core).
> > + IIO_BACKEND_TEST_PATTERN_MAX
> > +};
> > +
> > +enum iio_backend_sample_trigger {
> > + IIO_BACKEND_SAMPLE_TRIGGER_EDGE_FALLING,
> > + IIO_BACKEND_SAMPLE_TRIGGER_EDGE_RISING,
> > + IIO_BACKEND_SAMPLE_TRIGGER_MAX
> > +};
> > +
> > /**
> > * struct iio_backend_data_fmt - Backend data format
> > * @type: Data type.
> > @@ -28,15 +41,27 @@ struct iio_backend_data_fmt {
> > bool enable;
> > };
> >
> > +/**
> > + * struct iio_backend_chan_status - Backend channel status
> > + * @errors: Errors occurred when sending/receiving data.
>
> error, it's only a bool so we know there was at least one.
ack
>
> > + */
> > +struct iio_backend_chan_status {
> > + bool errors;
> > +};
> > +
> > /**
> > * struct iio_backend_ops - operations structure for an iio_backend
> > - * @enable: Enable backend.
> > - * @disable: Disable backend.
> > - * @chan_enable: Enable one channel.
> > - * @chan_disable: Disable one channel.
> > - * @data_format_set: Configure the data format for a specific channel.
> > - * @request_buffer: Request an IIO buffer.
> > - * @free_buffer: Free an IIO buffer.
> > + * @enable: Enable backend.
>
> Hmm. I dislike aligning comments because of this sort of noise.
> I guess I can cope without the ideal precursor patch making the padding
> change, but I am moaning about it...
Yeah, I also dislike it in struct members but for some reason I went like this in
here... I'll send a precursor patch in v2.
- Nuno Sá
