Re: [PATCH v5 6/8] iio: add the IIO backend framework

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

On Sat, 2024-01-13 at 17:22 +0000, Jonathan Cameron wrote:
> On Fri, 12 Jan 2024 17:40:20 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > This is a Framework to handle complex IIO aggregate devices.
> > 
> > The typical architecture is to have one device as the frontend device which
> > can be "linked" against one or multiple backend devices. All the IIO and
> > userspace interface is expected to be registers/managed by the frontend
> > device which will callback into the backends when needed (to get/set
> > some configuration that it does not directly control).
> > 
> > The basic framework interface is pretty simple:
> >  - Backends should register themselves with @devm_iio_backend_register()
> >  - Frontend devices should get backends with @devm_iio_backend_get()
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> 
> A few minor comments inline.
> 
> ...
> 
> > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> > backend.c
> > new file mode 100644
> > index 000000000000..994bc68c2679
> > --- /dev/null
> > +++ b/drivers/iio/industrialio-backend.c
> > @@ -0,0 +1,411 @@
> 
> ...
> 
> > +
> > +/*
> > + * Helper struct for requesting buffers. Allows for multiple buffers per
> > + * backend.
> Only seems to be used to ensure we have all the data needed to free it...
> So comment seems less than obviously connected to that.

I'll update the comment...

> > + */
> > +struct iio_backend_buffer_pair {
> > +	struct iio_backend *back;
> > +	struct iio_buffer *buffer;
> > +};
> > +
> 
> > +/**
> > + * iio_backend_chan_enable - Enable a backend channel.
> > + * @back:	Backend device.
> > + * @chan:	Channel number.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan)
> > +{
> > +	return iio_backend_op_call(back, chan_enable, chan);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_backend_chan_enable, IIO_BACKEND);
> > +
> > +/**
> > + * iio_backend_chan_disable - Disable a backend channel.
> > + * @back:	Backend device.
> > + * @chan:	Channel number.
> Would be good to be consistent on . or not.

Agreed.

> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int iio_backend_chan_disable(struct iio_backend *back, unsigned int chan)
> > +{
> > +	return iio_backend_op_call(back, chan_disable, chan);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iio_backend_chan_disable, IIO_BACKEND);
> > +
> > +/**
> > + * iio_backend_chan_enable - Enable the backend.
> > + * @back:	Backend device
> 
> 
> 
> ...
> 
> 
> > +/**
> > + * devm_iio_backend_get_from_fwnode_lookup
> 
> Not valid kernel doc + name is wrong.  Make sure you run
> the kernel-doc script over this and fix any errors or warnings
> reported.

Noted.

> 
> > + * @dev:	Device where to bind the backend lifetime.
> > + * @fwnode:	Firmware node of the backend device.
> > + *
> > + * Search the backend list for a device matching @fwnode.
> > + * This API should not be used and it's only present for preventing the
> > first
> > + * user of this framework to break it's DT ABI.
> > + *
> > + * RETURNS:
> > + * A backend pointer, negative error pointer otherwise.
> > + */
> > +struct iio_backend *
> > +__devm_iio_backend_get_from_fwnode_lookup(struct device *dev,
> > +					  struct fwnode_handle *fwnode)
> > +{
> > +	struct iio_backend *back;
> > +	int ret;
> > +
> > +	guard(mutex)(&iio_back_lock);
> > +	list_for_each_entry(back, &iio_back_list, entry) {
> > +		if (!device_match_fwnode(back->dev, fwnode))
> > +			continue;
> > +
> > +		ret = __devm_iio_backend_get(dev, back);
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +
> > +		return back;
> > +	}
> > +
> > +	return ERR_PTR(-EPROBE_DEFER);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(__devm_iio_backend_get_from_fwnode_lookup,
> > IIO_BACKEND);
> > 
> 
> 
> > +/**
> > + * devm_iio_backend_register - Register a new backend device
> > + * @dev:	Backend device being registered.
> > + * @ops:	Backend ops
> > + * @priv:	Device private data.
> > + *
> > + * @ops and @priv are both mandatory. Not providing them results in -
> > EINVAL.
> 
> It's unusual to 'insist' on the private data.
> Whilst it's highly likely it will always be there from a core point of view
> we don't mind it being NULL.  This is different from the ops as we want
> to be able to call those without checking they are there.
> 

Hmm, you're right. The private is for the callers to care as we don't really
touch it.

- Nuno Sá
>