Fri, Feb 02, 2024 at 04:08:36PM +0100, Nuno Sa kirjoitti:
> 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()
...
> + * Copyright (C) 2023 Analog Devices Inc.
2024 as well?
...
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
Missing types.h and maybe more. (E.g., IIRC linux/err.h doesn't cover
linux/errno.h for Linux internal error codes, >= 512.)
...
> +int devm_iio_backend_request_buffer(struct device *dev,
> + struct iio_backend *back,
> + struct iio_dev *indio_dev)
> +{
> + struct iio_backend_buffer_pair *pair;
> + struct iio_buffer *buffer;
> +
> + buffer = iio_backend_ptr_op_call(back, request_buffer, indio_dev);
> + if (IS_ERR(buffer))
> + return PTR_ERR(buffer);
> +
> + pair = devm_kzalloc(dev, sizeof(*pair), GFP_KERNEL);
> + if (!pair)
> + return -ENOMEM;
Shouldn't we try memory allocation first? Otherwise seems to me like freeing
buffer is missed here.
> + /* weak reference should be all what we need */
> + pair->back = back;
> + pair->buffer = buffer;
> +
> + return devm_add_action_or_reset(dev, iio_backend_free_buffer, pair);
> +}
...
> +static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back)
> +{
> + struct device_link *link;
> + int ret;
> +
> + /*
> + * Make sure the provider cannot be unloaded before the consumer module.
> + * Note that device_links would still guarantee that nothing is
> + * accessible (and breaks) but this makes it explicit that the consumer
> + * module must be also unloaded.
> + */
> + if (!try_module_get(back->owner)) {
> + pr_err("%s: Cannot get module reference\n", dev_name(dev));
NIH dev_err(). If you want the prefix, define dev_fmt() (or how is it called?)
as well.
> + return -ENODEV;
> + }
> +
> + ret = devm_add_action_or_reset(dev, iio_backend_release, back);
> + if (ret)
> + return ret;
> +
> + link = device_link_add(dev, back->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> + if (!link) {
> + pr_err("%s: Could not link to supplier(%s)\n", dev_name(dev),
> + dev_name(back->dev));
Ditto.
> + return -EINVAL;
> + }
> +
> + pr_debug("%s: Found backend(%s) device\n", dev_name(dev),
> + dev_name(back->dev));
Ditto (dev_dbg() here).
> + return 0;
> +}
...
> +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
Same comments regarding pr_*() vs. dev_*().
> + struct fwnode_handle *fwnode;
> + struct iio_backend *back;
> + int index = 0, ret;
Wouldn't be better to have it done differently and actually using int is not
okay strictly speaking? It's unsigned in your case.
unsigned int index;
int ret;
> + if (name) {
> + index = device_property_match_string(dev, "io-backends-names",
> + name);
> + if (index < 0)
> + return ERR_PTR(index);
> + }
if (name) {
ret = device_property_match_string(dev, "io-backends-names", name);
if (ret < 0)
return ERR_PTR(ret);
index = ret;
} else {
index = 0;
}
> + fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
> + if (IS_ERR(fwnode)) {
> + /* not an error if optional */
> + pr_debug("%s: Cannot get Firmware reference\n", dev_name(dev));
> + return ERR_CAST(fwnode);
> + }
> +
> + guard(mutex)(&iio_back_lock);
> + list_for_each_entry(back, &iio_back_list, entry) {
> + if (!device_match_fwnode(back->dev, fwnode))
> + continue;
> +
> + fwnode_handle_put(fwnode);
> + ret = __devm_iio_backend_get(dev, back);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return back;
> + }
> +
> + fwnode_handle_put(fwnode);
> + return ERR_PTR(-EPROBE_DEFER);
> +}
...
> +static void iio_backend_unregister(void *arg)
> +{
> + struct iio_backend *back = arg;
No guard() here, why?
> + mutex_lock(&iio_back_lock);
> + list_del(&back->entry);
> + mutex_unlock(&iio_back_lock);
> +}
> +int devm_iio_backend_register(struct device *dev,
> + const struct iio_backend_ops *ops, void *priv)
Use dev_err() et al.
...
> + mutex_lock(&iio_back_lock);
> + list_add(&back->entry, &iio_back_list);
> + mutex_unlock(&iio_back_lock);
scoped_guard()?
--
With Best Regards,
Andy Shevchenko
