Re: [PATCH 4/8] iio: backend: add API for interface configuration

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 19 Dec 2024 16:42:33 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 17 Dec 2024 11:13:59 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Mon, 2024-12-16 at 21:36 +0100, Angelo Dureghello wrote:  
> > > From: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > 
> > > Add backend support for setting and getting the interface type
> > > in use.
> > > 
> > > Link:
> > > https://lore.kernel.org/linux-iio/20241129153546.63584-1-antoniu.miclaus@analog.com/T/#m6d86939078d780512824f1540145aade38b0990b
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > Co-developed-by: Angelo Dureghello <adureghello@baylibre.com>
> > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > > ---
> > > This patch has been picked up from the Antoniu patchset
> > > still not accepted, and extended with the interface setter,
> > > fixing also namespace names to be between quotation marks.
> > > ---
> > >  drivers/iio/industrialio-backend.c | 42
> > > ++++++++++++++++++++++++++++++++++++++
> > >  include/linux/iio/backend.h        | 19 +++++++++++++++++
> > >  2 files changed, 61 insertions(+)
> > > 
> > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> > > backend.c
> > > index 363281272035..6edc3e685f6a 100644
> > > --- a/drivers/iio/industrialio-backend.c
> > > +++ b/drivers/iio/industrialio-backend.c
> > > @@ -636,6 +636,48 @@ 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 interface 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_MAX)
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, "IIO_BACKEND");
> > > +
> > > +/**
> > > + * iio_backend_interface_type_set - set the interface type used.
> > > + * @back: Backend device
> > > + * @type: Interface type
> > > + *
> > > + * RETURNS:
> > > + * 0 on success, negative error number on failure.
> > > + */
> > > +int iio_backend_interface_type_set(struct iio_backend *back,
> > > +				   enum iio_backend_interface_type type)
> > > +{
> > > +	if (type >= IIO_BACKEND_INTERFACE_MAX)
> > > +		return -EINVAL;
> > > +
> > > +	return  iio_backend_op_call(back, interface_type_set, type);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_set, "IIO_BACKEND");
> > > +
> > >  /**
> > >   * iio_backend_extend_chan_spec - Extend an IIO channel
> > >   * @back: Backend device
> > > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> > > index 10be00f3b120..2b7221099d8c 100644
> > > --- a/include/linux/iio/backend.h
> > > +++ b/include/linux/iio/backend.h
> > > @@ -70,6 +70,15 @@ enum iio_backend_sample_trigger {
> > >  	IIO_BACKEND_SAMPLE_TRIGGER_MAX
> > >  };
> > >  
> > > +enum iio_backend_interface_type {
> > > +	IIO_BACKEND_INTERFACE_SERIAL_LVDS,
> > > +	IIO_BACKEND_INTERFACE_SERIAL_CMOS,    
> > 
> > The above are apparently not used in the next patch so I would not add them now.  
> > > +	IIO_BACKEND_INTERFACE_SERIAL_SPI,
> > > +	IIO_BACKEND_INTERFACE_SERIAL_DSPI,
> > > +	IIO_BACKEND_INTERFACE_SERIAL_QSPI,    
> > 
> > I'll throw my 2 cents but it would be nice to have more feedback on this. I'm
> > not completely sure about the xSPI stuff in here. We treated the QSPI as a bus
> > both for control and data in which we also add child devices. And we've been
> > adding specific bus operations/configurations through the 'struct
> > ad3552r_hs_platform_data' interface. So, I'm wondering if this should also not
> > be set through that interface?  
> 
> Maybe - kind of hard to tell until we actually have code.
> I'd go for kicking them into the long grass for now if they aren't used and
> just dropping them from this patch.  If we ever need them,easy to bring back
> and then we should have a justification for why!

oops. Misread. Obviously Nuno was saying the ones above aren't used, not the
SPI ones...  I don't feel strongly either way on setting these via
this generic interface, or via the other path.

Jonathan

> 
> J
> 
> 
> > 
> > LVDS/CMOS still looks slightly different to me...
> > 
> > - Nuno Sá
> > 
> > 
> >   
> 
>