Re: [PATCH 08/10] iio: backend: add new functionality

["Nuno Sá" <noname.nuno@gmail.com>]

On Thu, 2024-03-28 at 15:16 +0000, Jonathan Cameron wrote:
> On Thu, 28 Mar 2024 14:22:32 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > This adds the needed backend ops for supporting a backend inerfacing
> > with an high speed dac. The new ops are:
> > 
> > * data_source_set();
> > * set_sampling_freq();
> > * extend_chan_spec();
> > * ext_info_set();
> > * ext_info_get().
> > 
> > Also to note the new helpers that are meant to be used by the backends
> > when extending an IIO channel (adding extended info):
> > 
> > * iio_backend_ext_info_set();
> > * iio_backend_ext_info_get().
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> I'm pretty flexible on this so far as I think we are still learning how front
> ends and backends should interact. Maybe we'll figure that out in the medium
> term and rework this stuff. For now it looks fine. A few minor things inline.
> >  
> > +/**
> > + * iio_backend_ext_info_get - IIO ext_info read callback
> > + * @indio_dev:	IIO device
> > + * @private:	Data private to the driver
> > + * @chan:	IIO channel
> > + * @buf:	Buffer where to place the attribute data
> > + *
> > + * This helper is intended to be used by backends that extend an IIO channel
> > + * (trough iio_backend_extend_chan_spec()) with extended info. In that case,
> > + * backends are not supposed to give their own callbacks (as they would not
> > + * a way to get te backend from indio_dev). This is the getter.
> 
> te->the?

Yes and some more typos :).

> 
> 
> > +/**
> > + * iio_backend_extend_chan_spec - Extend an IIO channel
> > + * @indio_dev:	IIO device
> > + * @back:	Backend device
> > + * @chan:	IIO channel
> > + *
> > + * Some backends may have their own functionalities and hence capable of
> > + * extending a frontend's channel.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
> > +				 struct iio_backend *back,
> > +				 struct iio_chan_spec *chan)
> > +{
> > +	const struct iio_chan_spec_ext_info *ext_info = chan->ext_info;
> This is getting confusing.  So this one is the front end value of ext_info?
> Name it as such frontend_ext_info

Yes, it's the frontend pointer. Just to enforce the below constrain. Will rename as
suggested.

> 
> > +	int ret;
> > +
> > +	ret = iio_backend_op_call(back, extend_chan_spec, chan);
> > +	if (ret)
> > +		return ret;
> > +	/*
> > +	 * Let's keep things simple for now. Don't allow to overwrite the
> > +	 * frontend's extended info. If ever needed, we can support appending
> > +	 * it.
> > +	 */
> > +	if (ext_info && chan->ext_info != ext_info)
> > +		return -EOPNOTSUPP;
> > +	if (!chan->ext_info)
> 
> This is checking if the backend added anything? Perhaps a comment on that
> as we don't need a backend_ext_info local variable...

Yes, but just regarding ext_info as that is the only thing we're handling below
(doing some sanity checks). Note that (as you said above) I'm keeping things a bit
"open" in that the backend is open to extend whatever it wants. With time we may
learn better what do we want constrain or not.

> 
> > +		return 0;
> > +	/*
> > +	 * !\NOTE: this will break as soon as we have multiple backends on one
> > +	 * frontend and all of them extend channels. In that case, the core
> > +	 * backend code has no way to get the correct backend given the
> > +	 * iio device.
> > +	 *
> > +	 * One solution for this could be introducing a new backend
> > +	 * dedicated callback in struct iio_info so we can callback into the
> > +	 * frontend so it can give us the right backend given a chan_spec.
> > +	 */
> 
> Hmm. This is indeed messy.  Could we associate it with the buffer as presuably
> a front end with multiple backends is using multiple IIO buffers?
> 

Hmm, the assumption of having multiple buffers seems plausible to me but considering
the example we have in hands it would be cumbersome to get the backend. Considering
iio_backend_ext_info_get(), how could we get the backend if it was associated to one
of the IIO buffers? I think we would need more "intrusive" changes to make that work
or do you have something in mind=
 
> As you say a dance via the front end would work fine.

I'm happy you're also open for a proper solution already. I mention this in the
cover. My idea was something like (consider the iio_backend_ext_info_get()):

if (!indio_dev->info->get_iio_backend())
	return -EOPNOTSUPP;

back = indio_dev->info->get_iio_backend(indio_dev, chan_spec);

It would be nice to have some "default/generic" implementation for cases where we
only have one backend per frontend so that the frontend would not need to define the
callback.
  
> 
> 
> > +	iio_device_set_drvdata(indio_dev, back);
> > +
> > +	/* Don't allow backends to get creative and force their own handlers */
> > +	for (ext_info = chan->ext_info; ext_info->name; ext_info++) {
> > +		if (ext_info->read != iio_backend_ext_info_get)
> > +			return -EINVAL;
> > +		if (ext_info->write != iio_backend_ext_info_set)
> > +			return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_backend_extend_chan_spec, IIO_BACKEND);
> 
> > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> > index a6d79381866e..09ff2f8f9fd8 100644
> > --- a/include/linux/iio/backend.h
> > +++ b/include/linux/iio/backend.h
> > @@ -4,6 +4,7 @@
> >  
> >  #include <linux/types.h>
> >  
> > +struct iio_chan_spec;
> >  struct fwnode_handle;
> >  struct iio_backend;
> >  struct device;
> > @@ -15,6 +16,26 @@ enum iio_backend_data_type {
> >  	IIO_BACKEND_DATA_TYPE_MAX
> >  };
> >  
> > +enum iio_backend_data_source {
> > +	IIO_BACKEND_INTERNAL_CW,
> 
> CW?  Either expand out what ever that is in definition of add a comment
> at least.

Continuous wave :)

> 
> > +	IIO_BACKEND_EXTERNAL,
> What does external mean in this case?

In this particular case comes from a DMA source (IP). I thought external to be more
generic but if you prefer, I can do something like IIO_BACKEND_DMA?


> > +	IIO_BACKEND_DATA_SOURCE_MAX
> > +};
> > +
> > +/**
> > + * IIO_BACKEND_EX_INFO - Helper for an IIO extended channel attribute
> > + * @_name:	Attribute name
> > + * @_shared:	Whether the attribute is shared between all channels
> > + * @_what:	Data private to the driver
> > + */
> > +#define IIO_BACKEND_EX_INFO(_name, _shared, _what) {	\
> > +	.name = (_name),				\
> > +	.shared = (_shared),				\
> > +	.read =  iio_backend_ext_info_get,		\
> > +	.write = iio_backend_ext_info_set,		\
> > +	.private = (_what),				\
> > +}
> > +
> >  /**
> >   * struct iio_backend_data_fmt - Backend data format
> >   * @type:		Data type.
> > @@ -35,8 +56,13 @@ struct iio_backend_data_fmt {
> >   * @chan_enable:	Enable one channel.
> >   * @chan_disable:	Disable one channel.
> >   * @data_format_set:	Configure the data format for a specific channel.
> > + * @data_source_set:	Configure the data source for a specific channel.
> > + * @set_sample_rate:	Configure the sampling rate for a specific channel.
> >   * @request_buffer:	Request an IIO buffer.
> >   * @free_buffer:	Free an IIO buffer.
> > + * @extend_chan_spec:	Extend an IIO channel.
> > + * @ext_info_set:	Extended info setter.
> > + * @ext_info_get:	Extended info getter.
> >   **/
> >  struct iio_backend_ops {
> >  	int (*enable)(struct iio_backend *back);
> > @@ -45,10 +71,21 @@ struct iio_backend_ops {
> >  	int (*chan_disable)(struct iio_backend *back, unsigned int chan);
> >  	int (*data_format_set)(struct iio_backend *back, unsigned int chan,
> >  			       const struct iio_backend_data_fmt *data);
> > +	int (*data_source_set)(struct iio_backend *back, unsigned int chan,
> > +			       enum iio_backend_data_source data);
> > +	int (*set_sample_rate)(struct iio_backend *back, unsigned int chan,
> > +			       u64 sample_rate);
> 
> Name the parameter that so we know the units.  _hz? 

yes. And u64 to not fall in the CCF problem for 32bits :)

- Nuno Sá