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

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

On Thu, 2024-03-28 at 15:59 +0000, Jonathan Cameron wrote:
> On Thu, 28 Mar 2024 16:42:38 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > 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
> > > > +		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=
> 
> Nope. Just trying to get my head around the associations. I hadn't thought about
> how to make that visible in the code.  Probably a callabck anyway.
> 
> >  
> > > 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.
> Agreed - either a default that means if the callback isn't provided we get the
> single backend or if that proves fiddly at least a standard callback we can
> use in all such cases.
> 

I'll have to think a bit about it. We may need some association/link between iio_dev
and iio_backend in order to "if the callback isn't provided we get the single
backend". The easiest that comes to my mind without much thinking would be to use
iio_device_set_drvdata()/iio_device_get_drvdata() in case the frontend does not
provide a callback. This would already force callers to assign the indio_dev->info
pointer before this call. Not that nice but acceptable if properly documented I
guess.

Anyways, I'll see if I can think of something better...

> >   
> > > 
> > >   
> > > > +	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 :)
> 
> Spell that out.
> 
> > 
> > >   
> > > > +	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?
> 
> So from another IP block?   For that to be reasonably 'generic' we'd need a way
> to known where it was coming from.
> 

Yeps, in this case comes from the IIO DMA buffer which in HW means a DMA IP core...

> Now I remember advantage of reviewing on weekends - fewer replies during the
> reviews :)
> 

:)

- Nuno Sá