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@xxxxxxxxxx> wrote:
>
> > From: Nuno Sa <nuno.sa@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
>
> 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á
>
