Re: [PATCH v9 5/7] iio: add the IIO backend framework

Andy Shevchenko
2024-02-06T14:28:07Z

On Tue, Feb 6, 2024 at 12:08 PM 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()

Not sure what the meaning of @ in the above lines.

...

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

Can we have an ASCII art with an example?

> 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 framework interface is pretty simple:
> + * - Backends should register themselves with @devm_iio_backend_register()
> + * - Frontend devices should get backends with @devm_iio_backend_get()
> + *
> + * Also to note that the primary target for this framework are converters like
> + * ADC/DACs so @iio_backend_ops will have some operations typical of converter
> + * devices. On top of that, this is "generic" for all IIO which means any kind
> + * of device can make use of the framework. That said, If the @iio_backend_ops
> + * struct begins to grow out of control, we can always refactor things so that
> + * the industrialio-backend.c is only left with the really generic stuff. Then,
> + * we can build on top of it depending on the needs.
> + *
> + * Copyright (C) 2023-2024 Analog Devices Inc.
> + */

...

> +/*
> + * Helper macros to call backend ops. Makes sure the option is supported

Missing period.

> + */

...

> +/**
> + * iio_backend_chan_enable - Enable a backend channel
> + * @back: Backend device
> + * @chan: Channel number
> + *
> + * RETURNS:

Not sure if this is the style used in other IIO core files...

> + * 0 on success, negative error number on failure.
> + */

...

> + if (!try_module_get(back->owner)) {
> + dev_err(dev, "Cannot get module reference\n");
> + return -ENODEV;

devm_*() are supposed to be used only at ->probe() stage, hence
dev_err_probe() is fine (and eventually would be nice to have for the
sake of making messaging uniform).

> + }

...

> + link = device_link_add(dev, back->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> + if (!link) {
> + dev_err(dev, "Could not link to supplier(%s)\n",
> + dev_name(back->dev));
> + return -EINVAL;

Ditto.

> + }

...

> + if (name) {
> + ret = device_property_match_string(dev, "io-backend-names",
> + name);

One line?

> + 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)) {
> + dev_err(dev, "Cannot get Firmware reference\n");
> + return ERR_CAST(fwnode);

dev_err_probe() ?

> + }

...

> + if (!ops) {
> + dev_err(dev, "No backend ops given\n");
> + return -EINVAL;

dev_err_probe() ?

> + }

--
With Best Regards,
Andy Shevchenko