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

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

On Tue, 2024-02-06 at 16:27 +0200, Andy Shevchenko wrote:
> 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.
> 

No special meaning...

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

I do have one in the cover and spoke on the possibility of having it here but no
one really expressed any interest on it. Personally, for now, I'm also not
seeing it as *that* important.

> 
> >  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?

Would pass the 80 limit column. I would not mind at all to have the one liner
but I'm playing by Jonathan's rules. Stick with that limit unless it hurts
readability.

> 
> > +               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() ?

Yeah, I can switch to that as on top of being devm APIs, these APIs are really
intended to be called during probe().

- Nuno Sá