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

[Jonathan Cameron <jic23@kernel.org>]

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.
> + */
> +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.
> + *
> + * 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.

> + * @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.

> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int devm_iio_backend_register(struct device *dev,
> +			      const struct iio_backend_ops *ops, void *priv)
> +{
> +	struct iio_backend *back;
> +
> +	if (!ops || !priv) {

> +		pr_err("%s: No backend ops or private data given\n",
> +		       dev_name(dev));
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Through device_links, we guarantee that a frontend device cannot be
> +	 * bound/exist if the backend driver is not around. Hence, we can bind
> +	 * the backend object lifetime with the device being passed since
> +	 * removing it will torn the frontend down.

"will have torn" or "will tear the"

> +	 */
> +	back = devm_kzalloc(dev, sizeof(*back), GFP_KERNEL);
> +	if (!back)
> +		return -ENOMEM;
> +
> +	back->ops = ops;
> +	back->owner = dev->driver->owner;
> +	back->dev = dev;
> +	back->priv = priv;
> +	mutex_lock(&iio_back_lock);
> +	list_add(&back->entry, &iio_back_list);
> +	mutex_unlock(&iio_back_lock);
> +
> +	return devm_add_action_or_reset(dev, iio_backend_unregister, back);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_iio_backend_register, IIO_BACKEND);
> +
> +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
> +MODULE_DESCRIPTION("Framework to handle complex IIO aggregate devices");
> +MODULE_LICENSE("GPL");