Re: [PATCH 1/7] iio: backend: add API for interface get

[Jonathan Cameron]

On Thu, 26 Sep 2024 12:52:39 +0200
Nuno Sá <noname.nuno@xxxxxxxxx> wrote:

> On Thu, 2024-09-26 at 10:40 +0200, David Lechner wrote:
> > On Mon, Sep 23, 2024 at 12:15 PM Antoniu Miclaus
> > <antoniu.miclaus@xxxxxxxxxx> wrote:  
> > > 
> > > Add backend support for obtaining the interface type used.
> > > 
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > > ---
> > >  drivers/iio/industrialio-backend.c | 24 ++++++++++++++++++++++++
> > >  include/linux/iio/backend.h        | 10 ++++++++++
> > >  2 files changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> > > backend.c
> > > index efe05be284b6..53ab6bc86a50 100644
> > > --- a/drivers/iio/industrialio-backend.c
> > > +++ b/drivers/iio/industrialio-backend.c
> > > @@ -449,6 +449,30 @@ ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev,
> > > uintptr_t private,
> > >  }
> > >  EXPORT_SYMBOL_NS_GPL(iio_backend_ext_info_set, IIO_BACKEND);
> > > 
> > > +/**
> > > + * iio_backend_interface_type_get - get the interace type used.
> > > + * @back: Backend device
> > > + * @type: Interface type
> > > + *
> > > + * RETURNS:
> > > + * 0 on success, negative error number on failure.
> > > + */
> > > +int iio_backend_interface_type_get(struct iio_backend *back,
> > > +                                  enum iio_backend_interface_type *type)
> > > +{
> > > +       int ret;
> > > +
> > > +       ret = iio_backend_op_call(back, interface_type_get, type);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       if (*type > IIO_BACKEND_INTERFACE_CMOS)
Put a COUNT entry or similar on the end of the enum so this doesn't need
updating for more types.

> > > +               return -EINVAL;
> > > +
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND);
> > > +
> > >  /**
> > >   * iio_backend_extend_chan_spec - Extend an IIO channel
> > >   * @indio_dev: IIO device
> > > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> > > index 8099759d7242..ba8ad30ac9ba 100644
> > > --- a/include/linux/iio/backend.h
> > > +++ b/include/linux/iio/backend.h
> > > @@ -63,6 +63,11 @@ enum iio_backend_sample_trigger {
> > >         IIO_BACKEND_SAMPLE_TRIGGER_MAX
> > >  };
> > > 
> > > +enum iio_backend_interface_type {
> > > +       IIO_BACKEND_INTERFACE_LVDS,
> > > +       IIO_BACKEND_INTERFACE_CMOS

trailing comma.

This is going to get bigger!

> > > +};
> > > +
> > >  /**
> > >   * struct iio_backend_ops - operations structure for an iio_backend
> > >   * @enable: Enable backend.
> > > @@ -81,6 +86,7 @@ enum iio_backend_sample_trigger {
> > >   * @extend_chan_spec: Extend an IIO channel.
> > >   * @ext_info_set: Extended info setter.
> > >   * @ext_info_get: Extended info getter.
> > > + * @interface_type_get: Interface type.
> > >   **/
> > >  struct iio_backend_ops {
> > >         int (*enable)(struct iio_backend *back);
> > > @@ -113,6 +119,8 @@ struct iio_backend_ops {
> > >                             const char *buf, size_t len);
> > >         int (*ext_info_get)(struct iio_backend *back, uintptr_t private,
> > >                             const struct iio_chan_spec *chan, char *buf);
> > > +       int (*interface_type_get)(struct iio_backend *back,
> > > +                                 enum iio_backend_interface_type *type);
> > >  };
> > > 
> > >  int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan);
> > > @@ -142,6 +150,8 @@ ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev,
> > > uintptr_t private,
> > >  ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private,
> > >                                  const struct iio_chan_spec *chan, char *buf);
> > > 
> > > +int iio_backend_interface_type_get(struct iio_backend *back,
> > > +                                  enum iio_backend_interface_type *type);
> > >  int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
> > >                                  struct iio_backend *back,
> > >                                  struct iio_chan_spec *chan);
> > > --
> > > 2.46.0
> > >   
> > 
> > This seems very specific to the AD485x chips and the AXI ADC backend.
> > Since it is describing how the chip is wired to the AXI DAC IP block,
> > I would be tempted to use the devicetree for this info instead of
> > adding a new backend function.  
> 
> Not sure If I'm following your point but I think this is the typical case where the
> chip (being it a DAC or ADC) supports both CMOS and LVDS interfaces. Naturally you
> only use one on your system and this is a synthesis parameter on the FPGA IP core.
> Therefore, it makes sense for the frontend to have way to ask for this information to
> the backend.
> 
> That said, we could also have a DT parameter but, ideally, we would then need a way
> to match the parameter with the backend otherwise we could have DT stating LVDS and
> the backend built with CMOS.

That would be a DTS bug that you should fix :)  For this to make sense you are
relying on an FPGA that also has pins flexible enough to support LVDS and CMOS
so it's only a firmware thing. Been a while since I last messed with FPGAs,
but that seems unlikely to be true in general.

So far I'm with David on this, feels like something we shouldn't be discovering
at runtime though maybe that's a convenience that we do want to enable.

> 
> Other thing that we could think about is a new devm_iio_backend_get_with_info() where
> the frontend would get some constant and static info about the backend (the interface
> type would an ideal match for something like this). But I feel it's still early days
> for something like this :)
> 
> - Nuno Sá