Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Daniel Vetter]

I've chatted a bit with Hans Verkuil about this topic at fosdem and
apparently both v4l and alsa have something like this already in their
helper libraries. Adding more people as fyi in case they want to
switch to the new driver core stuff from Russell.
-Daniel

On Thu, Jan 2, 2014 at 10:27 PM, Russell King
<rmk+kernel@arm.linux.org.uk> wrote:
> Subsystems such as ALSA, DRM and others require a single card-level
> device structure to represent a subsystem.  However, firmware tends to
> describe the individual devices and the connections between them.
>
> Therefore, we need a way to gather up the individual component devices
> together, and indicate when we have all the component devices.
>
> We do this in DT by providing a "superdevice" node which specifies
> the components, eg:
>
>         imx-drm {
>                 compatible = "fsl,drm";
>                 crtcs = <&ipu1>;
>                 connectors = <&hdmi>;
>         };
>
> The superdevice is declared into the component support, along with the
> subcomponents.  The superdevice receives callbacks to locate the
> subcomponents, and identify when all components are present.  At this
> point, we bind the superdevice, which causes the appropriate subsystem
> to be initialised in the conventional way.
>
> When any of the components or superdevice are removed from the system,
> we unbind the superdevice, thereby taking the subsystem down.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/base/Makefile     |    2 +-
>  drivers/base/component.c  |  379 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/component.h |   31 ++++
>  3 files changed, 411 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/base/component.c
>  create mode 100644 include/linux/component.h
>
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 94e8a80e87f8..870ecfd503af 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -1,6 +1,6 @@
>  # Makefile for the Linux device tree
>
> -obj-y                  := core.o bus.o dd.o syscore.o \
> +obj-y                  := component.o core.o bus.o dd.o syscore.o \
>                            driver.o class.o platform.o \
>                            cpu.o firmware.o init.o map.o devres.o \
>                            attribute_container.o transport_class.o \
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> new file mode 100644
> index 000000000000..5492cd8d2247
> --- /dev/null
> +++ b/drivers/base/component.c
> @@ -0,0 +1,379 @@
> +/*
> + * Componentized device handling.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This is work in progress.  We gather up the component devices into a list,
> + * and bind them when instructed.  At the moment, we're specific to the DRM
> + * subsystem, and only handles one master device, but this doesn't have to be
> + * the case.
> + */
> +#include <linux/component.h>
> +#include <linux/device.h>
> +#include <linux/kref.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +struct master {
> +       struct list_head node;
> +       struct list_head components;
> +       bool bound;
> +
> +       const struct component_master_ops *ops;
> +       struct device *dev;
> +};
> +
> +struct component {
> +       struct list_head node;
> +       struct list_head master_node;
> +       struct master *master;
> +       bool bound;
> +
> +       const struct component_ops *ops;
> +       struct device *dev;
> +};
> +
> +static DEFINE_MUTEX(component_mutex);
> +static LIST_HEAD(component_list);
> +static LIST_HEAD(masters);
> +
> +static struct master *__master_find(struct device *dev, const struct component_master_ops *ops)
> +{
> +       struct master *m;
> +
> +       list_for_each_entry(m, &masters, node)
> +               if (m->dev == dev && (!ops || m->ops == ops))
> +                       return m;
> +
> +       return NULL;
> +}
> +
> +/* Attach an unattached component to a master. */
> +static void component_attach_master(struct master *master, struct component *c)
> +{
> +       c->master = master;
> +
> +       list_add_tail(&c->master_node, &master->components);
> +}
> +
> +/* Detach a component from a master. */
> +static void component_detach_master(struct master *master, struct component *c)
> +{
> +       list_del(&c->master_node);
> +
> +       c->master = NULL;
> +}
> +
> +int component_master_add_child(struct master *master,
> +       int (*compare)(struct device *, void *), void *compare_data)
> +{
> +       struct component *c;
> +       int ret = -ENXIO;
> +
> +       list_for_each_entry(c, &component_list, node) {
> +               if (c->master)
> +                       continue;
> +
> +               if (compare(c->dev, compare_data)) {
> +                       component_attach_master(master, c);
> +                       ret = 0;
> +                       break;
> +               }
> +       }
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(component_master_add_child);
> +
> +/* Detach all attached components from this master */
> +static void master_remove_components(struct master *master)
> +{
> +       while (!list_empty(&master->components)) {
> +               struct component *c = list_first_entry(&master->components,
> +                                       struct component, master_node);
> +
> +               WARN_ON(c->master != master);
> +
> +               component_detach_master(master, c);
> +       }
> +}
> +
> +/*
> + * Try to bring up a master.  If component is NULL, we're interested in
> + * this master, otherwise it's a component which must be present to try
> + * and bring up the master.
> + *
> + * Returns 1 for successful bringup, 0 if not ready, or -ve errno.
> + */
> +static int try_to_bring_up_master(struct master *master,
> +       struct component *component)
> +{
> +       int ret = 0;
> +
> +       if (!master->bound) {
> +               /*
> +                * Search the list of components, looking for components that
> +                * belong to this master, and attach them to the master.
> +                */
> +               if (master->ops->add_components(master->dev, master)) {
> +                       /* Failed to find all components */
> +                       master_remove_components(master);
> +                       ret = 0;
> +                       goto out;
> +               }
> +
> +               if (component && component->master != master) {
> +                       master_remove_components(master);
> +                       ret = 0;
> +                       goto out;
> +               }
> +
> +               /* Found all components */
> +               ret = master->ops->bind(master->dev);
> +               if (ret < 0) {
> +                       master_remove_components(master);
> +                       goto out;
> +               }
> +
> +               master->bound = true;
> +               ret = 1;
> +       }
> +out:
> +
> +       return ret;
> +}
> +
> +static int try_to_bring_up_masters(struct component *component)
> +{
> +       struct master *m;
> +       int ret = 0;
> +
> +       list_for_each_entry(m, &masters, node) {
> +               ret = try_to_bring_up_master(m, component);
> +               if (ret != 0)
> +                       break;
> +       }
> +
> +       return ret;
> +}
> +
> +static void take_down_master(struct master *master)
> +{
> +       if (master->bound) {
> +               master->ops->unbind(master->dev);
> +               master->bound = false;
> +       }
> +
> +       master_remove_components(master);
> +}
> +
> +int component_master_add(struct device *dev, const struct component_master_ops *ops)
> +{
> +       struct master *master;
> +       int ret;
> +
> +       master = kzalloc(sizeof(*master), GFP_KERNEL);
> +       if (!master)
> +               return -ENOMEM;
> +
> +       master->dev = dev;
> +       master->ops = ops;
> +       INIT_LIST_HEAD(&master->components);
> +
> +       /* Add to the list of available masters. */
> +       mutex_lock(&component_mutex);
> +       list_add(&master->node, &masters);
> +
> +       ret = try_to_bring_up_master(master, NULL);
> +
> +       if (ret < 0) {
> +               /* Delete off the list if we weren't successful */
> +               list_del(&master->node);
> +               kfree(master);
> +       }
> +       mutex_unlock(&component_mutex);
> +
> +       return ret < 0 ? ret : 0;
> +}
> +EXPORT_SYMBOL_GPL(component_master_add);
> +
> +void component_master_del(struct device *dev, const struct component_master_ops *ops)
> +{
> +       struct master *master;
> +
> +       mutex_lock(&component_mutex);
> +       master = __master_find(dev, ops);
> +       if (master) {
> +               take_down_master(master);
> +
> +               list_del(&master->node);
> +               kfree(master);
> +       }
> +       mutex_unlock(&component_mutex);
> +}
> +EXPORT_SYMBOL_GPL(component_master_del);
> +
> +static void component_unbind(struct component *component,
> +       struct master *master, void *data)
> +{
> +       WARN_ON(!component->bound);
> +
> +       component->ops->unbind(component->dev, master->dev, data);
> +       component->bound = false;
> +
> +       /* Release all resources claimed in the binding of this component */
> +       devres_release_group(component->dev, component);
> +}
> +
> +void component_unbind_all(struct device *master_dev, void *data)
> +{
> +       struct master *master;
> +       struct component *c;
> +
> +       WARN_ON(!mutex_is_locked(&component_mutex));
> +
> +       master = __master_find(master_dev, NULL);
> +       if (!master)
> +               return;
> +
> +       list_for_each_entry_reverse(c, &master->components, master_node)
> +               component_unbind(c, master, data);
> +}
> +EXPORT_SYMBOL_GPL(component_unbind_all);
> +
> +static int component_bind(struct component *component, struct master *master,
> +       void *data)
> +{
> +       int ret;
> +
> +       /*
> +        * Each component initialises inside its own devres group.
> +        * This allows us to roll-back a failed component without
> +        * affecting anything else.
> +        */
> +       if (!devres_open_group(master->dev, NULL, GFP_KERNEL))
> +               return -ENOMEM;
> +
> +       /*
> +        * Also open a group for the device itself: this allows us
> +        * to release the resources claimed against the sub-device
> +        * at the appropriate moment.
> +        */
> +       if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
> +               devres_release_group(master->dev, NULL);
> +               return -ENOMEM;
> +       }
> +
> +       dev_dbg(master->dev, "binding %s (ops %ps)\n",
> +               dev_name(component->dev), component->ops);
> +
> +       ret = component->ops->bind(component->dev, master->dev, data);
> +       if (!ret) {
> +               component->bound = true;
> +
> +               /*
> +                * Close the component device's group so that resources
> +                * allocated in the binding are encapsulated for removal
> +                * at unbind.  Remove the group on the DRM device as we
> +                * can clean those resources up independently.
> +                */
> +               devres_close_group(component->dev, NULL);
> +               devres_remove_group(master->dev, NULL);
> +
> +               dev_info(master->dev, "bound %s (ops %ps)\n",
> +                        dev_name(component->dev), component->ops);
> +       } else {
> +               devres_release_group(component->dev, NULL);
> +               devres_release_group(master->dev, NULL);
> +
> +               dev_err(master->dev, "failed to bind %s (ops %ps): %d\n",
> +                       dev_name(component->dev), component->ops, ret);
> +       }
> +
> +       return ret;
> +}
> +
> +int component_bind_all(struct device *master_dev, void *data)
> +{
> +       struct master *master;
> +       struct component *c;
> +       int ret = 0;
> +
> +       WARN_ON(!mutex_is_locked(&component_mutex));
> +
> +       master = __master_find(master_dev, NULL);
> +       if (!master)
> +               return -EINVAL;
> +
> +       list_for_each_entry(c, &master->components, master_node) {
> +               ret = component_bind(c, master, data);
> +               if (ret)
> +                       break;
> +       }
> +
> +       if (ret != 0) {
> +               list_for_each_entry_continue_reverse(c, &master->components,
> +                                                    master_node)
> +                       component_unbind(c, master, data);
> +       }
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(component_bind_all);
> +
> +int component_add(struct device *dev, const struct component_ops *ops)
> +{
> +       struct component *component;
> +       int ret;
> +
> +       component = kzalloc(sizeof(*component), GFP_KERNEL);
> +       if (!component)
> +               return -ENOMEM;
> +
> +       component->ops = ops;
> +       component->dev = dev;
> +
> +       dev_dbg(dev, "adding component (ops %ps)\n", ops);
> +
> +       mutex_lock(&component_mutex);
> +       list_add_tail(&component->node, &component_list);
> +
> +       ret = try_to_bring_up_masters(component);
> +       if (ret < 0) {
> +               list_del(&component->node);
> +
> +               kfree(component);
> +       }
> +       mutex_unlock(&component_mutex);
> +
> +       return ret < 0 ? ret : 0;
> +}
> +EXPORT_SYMBOL_GPL(component_add);
> +
> +void component_del(struct device *dev, const struct component_ops *ops)
> +{
> +       struct component *c, *component = NULL;
> +
> +       mutex_lock(&component_mutex);
> +       list_for_each_entry(c, &component_list, node)
> +               if (c->dev == dev && c->ops == ops) {
> +                       list_del(&c->node);
> +                       component = c;
> +                       break;
> +               }
> +
> +       if (component && component->master)
> +               take_down_master(component->master);
> +
> +       mutex_unlock(&component_mutex);
> +
> +       WARN_ON(!component);
> +       kfree(component);
> +}
> +EXPORT_SYMBOL_GPL(component_del);
> +
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/component.h b/include/linux/component.h
> new file mode 100644
> index 000000000000..73657636db0b
> --- /dev/null
> +++ b/include/linux/component.h
> @@ -0,0 +1,31 @@
> +#ifndef COMPONENT_H
> +#define COMPONENT_H
> +
> +struct device;
> +
> +struct component_ops {
> +       int (*bind)(struct device *, struct device *, void *);
> +       void (*unbind)(struct device *, struct device *, void *);
> +};
> +
> +int component_add(struct device *, const struct component_ops *);
> +void component_del(struct device *, const struct component_ops *);
> +
> +int component_bind_all(struct device *, void *);
> +void component_unbind_all(struct device *, void *);
> +
> +struct master;
> +
> +struct component_master_ops {
> +       int (*add_components)(struct device *, struct master *);
> +       int (*bind)(struct device *);
> +       void (*unbind)(struct device *);
> +};
> +
> +int component_master_add(struct device *, const struct component_master_ops *);
> +void component_master_del(struct device *, const struct component_master_ops *);
> +
> +int component_master_add_child(struct master *master,
> +       int (*compare)(struct device *, void *), void *compare_data);
> +
> +#endif
> --
> 1.7.4.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 10:04:30AM +0100, Daniel Vetter wrote:
> I've chatted a bit with Hans Verkuil about this topic at fosdem and
> apparently both v4l and alsa have something like this already in their
> helper libraries. Adding more people as fyi in case they want to
> switch to the new driver core stuff from Russell.

It's not ALSA, but ASoC which has this.  Mark is already aware of this
and will be looking at it from an ASoC perspective.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Jean-Francois Moine]

On Fri, 7 Feb 2014 09:46:56 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Fri, Feb 07, 2014 at 10:04:30AM +0100, Daniel Vetter wrote:
> > I've chatted a bit with Hans Verkuil about this topic at fosdem and
> > apparently both v4l and alsa have something like this already in their
> > helper libraries. Adding more people as fyi in case they want to
> > switch to the new driver core stuff from Russell.  
> 
> It's not ALSA, but ASoC which has this.  Mark is already aware of this
> and will be looking at it from an ASoC perspective.

Russell,

I started to use your code (which works fine, thanks), and it avoids a
lot of problems, especially, about probe_defer in a DT context.

I was wondering if your componentised mechanism could be extended to the
devices defined by DT.

In the DT, when a device_node is a phandle, this means it is referenced
by some other device(s), and these device(s) will not start until the
phandle device is registered.

Then, the idea is to do a component_add() for such phandle devices in
device_add() (device_register).

Pratically,

- the component_add() call in device_register would not include any
  bind/unbind callback function, so, this should be tested in
  component_bind/unbind(),

- component_add would not be called if the device being added already
  called component_add in its probe function. A simple flag in the
  struct device_node should solve this problem.

What do you think about this?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 12:57:21PM +0100, Jean-Francois Moine wrote:
> I started to use your code (which works fine, thanks), and it avoids a
> lot of problems, especially, about probe_defer in a DT context.
> 
> I was wondering if your componentised mechanism could be extended to the
> devices defined by DT.

It was developed against imx-drm, which is purely DT based.  I already
have a solution for the cubox armada DRM.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Guennadi Liakhovetski]

Hi Russell

(I suspect this my email will be rejected by ALKML too like other my 
recent emails, but at least other MLs will pick it up and individual CCs 
too, so, if replying, maybe it would be good to keep my entire reply, all 
the more that it's going to be very short)

On Thu, 2 Jan 2014, Russell King wrote:

> Subsystems such as ALSA, DRM and others require a single card-level
> device structure to represent a subsystem.  However, firmware tends to
> describe the individual devices and the connections between them.
> 
> Therefore, we need a way to gather up the individual component devices
> together, and indicate when we have all the component devices.
> 
> We do this in DT by providing a "superdevice" node which specifies
> the components, eg:
> 
> 	imx-drm {
> 		compatible = "fsl,drm";
> 		crtcs = <&ipu1>;
> 		connectors = <&hdmi>;
> 	};

It is a pity linux-media wasn't CC'ed and apparently V4L developers didn't 
notice this and other related patches in a "clean up" series, and now this 
patch is already in the mainline. But at least I'd like to ask whether the 
bindings, defined in 
Documentation/devicetree/bindings/media/video-interfaces.txt and 
implemented in drivers/media/v4l2-core/v4l2-of.c have been considered for 
this job, and - if so - why have they been found unsuitable? Wouldn't it 
have been better to use and - if needed - extend them to cover any 
deficiencies? Even though the implementation is currently located under 
drivers/media/v4l2-code/ it's pretty generic and should be easily 
transferable to a more generic location.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Russell King - ARM Linux]

On Wed, Feb 26, 2014 at 10:00:25PM +0100, Guennadi Liakhovetski wrote:
> Hi Russell
> 
> (I suspect this my email will be rejected by ALKML too like other my 
> recent emails, but at least other MLs will pick it up and individual CCs 
> too, so, if replying, maybe it would be good to keep my entire reply, all 
> the more that it's going to be very short)
> 
> On Thu, 2 Jan 2014, Russell King wrote:
> 
> > Subsystems such as ALSA, DRM and others require a single card-level
> > device structure to represent a subsystem.  However, firmware tends to
> > describe the individual devices and the connections between them.
> > 
> > Therefore, we need a way to gather up the individual component devices
> > together, and indicate when we have all the component devices.
> > 
> > We do this in DT by providing a "superdevice" node which specifies
> > the components, eg:
> > 
> > 	imx-drm {
> > 		compatible = "fsl,drm";
> > 		crtcs = <&ipu1>;
> > 		connectors = <&hdmi>;
> > 	};
> 
> It is a pity linux-media wasn't CC'ed and apparently V4L developers didn't 
> notice this and other related patches in a "clean up" series, and now this 
> patch is already in the mainline. But at least I'd like to ask whether the 
> bindings, defined in 
> Documentation/devicetree/bindings/media/video-interfaces.txt and 
> implemented in drivers/media/v4l2-core/v4l2-of.c have been considered for 
> this job, and - if so - why have they been found unsuitable? Wouldn't it 
> have been better to use and - if needed - extend them to cover any 
> deficiencies? Even though the implementation is currently located under 
> drivers/media/v4l2-code/ it's pretty generic and should be easily 
> transferable to a more generic location.

The component helpers have nothing to do with DT apart from solving
the problem of how to deal with subsystems which expect a single device,
but we have a group of devices and their individual drivers to cope with.
Subsystems like DRM and ALSA.

It is completely agnostic to whether you're using platform data, DT or
even ACPI - this code could *not* care less.  None of that comes anywhere
near what this patch does.  It merely provides a way to collect up
individual devices from co-operating drivers, and control their binding
such that a subsystem like DRM or ALSA can be presented with a "card"
level view of the hardware rather than a multi-device medusa with all
the buggy, racy, crap fsckage that people come up to make that kind of
thing work.

Now, as for the binding above, first, what does "eg" mean... and
secondly, how would a binding which refers to crtcs and connectors
have anything to do with ALSA?  Clearly this isn't an example of a
binding for an ALSA use, which was talked about in the very first
line of the above commit commentry.  So it's quite clear that what is
given there is an example of how it /could/ be used.

I suppose I could have instead turned imx-drm into a completely unusable
mess by not coming up with some kind of binding, and instead submitted
a whole pile of completely untested code.  Alternatively, I could've
used the OF binding as you're suggesting, but that would mean radically
changing the /existing/ bindings for the IPU as a whole - something
which others are better suited at as they have a /much/ better
understanding of the complexities of this hardware than I.

So, what I have done is implemented - for a driver in staging which is
still subject to ongoing development and non-stable DT bindings -
something which allows forward progress with a *minimum* of disruption
to the existing DT bindings for everyone, while still allowing forward
progress.

Better bindings for imx-drm are currently being worked on.  Philipp
Zabel of Pengutronix is currently looking at it, and has posted many
RFC patches on this very subject, including moving the V4L2 OF helpers
to a more suitable location.  OF people have been involved in that
discussion over the preceding weeks, and there's a working implementation
of imx-drm using these helpers from v4l2.

I'm finding people who are working in the same area and trying to get
everyone talking to each other so that we /do/ end up with a set of
bindings for the display stuff which are suitable for everyone.  Tomi
from TI has already expressed his input to this ongoing discussion.

You're welcome to get involved in those discussions too.

I hope this makes it clear, and clears up the confusion.

Thanks.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Guennadi Liakhovetski]

Hi Russell,

Sorry for a long delay.

On Wed, 26 Feb 2014, Russell King - ARM Linux wrote:

[snip]

> Better bindings for imx-drm are currently being worked on.  Philipp
> Zabel of Pengutronix is currently looking at it, and has posted many
> RFC patches on this very subject, including moving the V4L2 OF helpers
> to a more suitable location.  OF people have been involved in that
> discussion over the preceding weeks, and there's a working implementation
> of imx-drm using these helpers from v4l2.

Yes, I'm aware of that patch series, and I do look at the discussion from 
time to time, unfortunately I don't have too much time for it now. But in 
any case if this work is going to be used with imx-drm too, that should be 
a good direction to take, I think.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Laurent Pinchart]

Hi Russell,

Time for me to jump in. The more, the merrier I suppose.

On Wednesday 26 February 2014 22:19:39 Russell King - ARM Linux wrote:
> On Wed, Feb 26, 2014 at 10:00:25PM +0100, Guennadi Liakhovetski wrote:
> > Hi Russell
> > 
> > (I suspect this my email will be rejected by ALKML too like other my
> > recent emails, but at least other MLs will pick it up and individual CCs
> > too, so, if replying, maybe it would be good to keep my entire reply, all
> > the more that it's going to be very short)
> > 
> > On Thu, 2 Jan 2014, Russell King wrote:
> > > Subsystems such as ALSA, DRM and others require a single card-level
> > > device structure to represent a subsystem.  However, firmware tends to
> > > describe the individual devices and the connections between them.
> > > 
> > > Therefore, we need a way to gather up the individual component devices
> > > together, and indicate when we have all the component devices.
> > > 
> > > We do this in DT by providing a "superdevice" node which specifies
> > > the components, eg:
> > > 	imx-drm {
> > > 		compatible = "fsl,drm";
> > > 		crtcs = <&ipu1>;
> > > 		connectors = <&hdmi>;
> > > 	};
> > 
> > It is a pity linux-media wasn't CC'ed and apparently V4L developers didn't
> > notice this and other related patches in a "clean up" series, and now this
> > patch is already in the mainline. But at least I'd like to ask whether the
> > bindings, defined in
> > Documentation/devicetree/bindings/media/video-interfaces.txt and
> > implemented in drivers/media/v4l2-core/v4l2-of.c have been considered for
> > this job, and - if so - why have they been found unsuitable? Wouldn't it
> > have been better to use and - if needed - extend them to cover any
> > deficiencies? Even though the implementation is currently located under
> > drivers/media/v4l2-code/ it's pretty generic and should be easily
> > transferable to a more generic location.
> 
> The component helpers have nothing to do with DT apart from solving
> the problem of how to deal with subsystems which expect a single device,
> but we have a group of devices and their individual drivers to cope with.
> Subsystems like DRM and ALSA.

(and V4L2)

Point duly taken. First of all I want to mention that your proposal is 
greatly appreciated. This is a problem that crosses subsystem boundaries, and 
should thus be addressed centrally.

However, we (as in the V4L2 community, and me personally) would have 
appreciated to be CC'ed on the proposal. As you might know we already had a 
solution for this problem, albeit V4L2-specific, in drivers/media/v4l2-
core/v4l2-async.c. Whether or not this solution should have been made generic 
instead of coming up with a new separate implementation would of course have 
been debatable, but the most important point would have been to make sure that 
v4l2-async could easily be implemented on top of the common component 
architecture.

The topic is particularly hot given that a similar solution was also proposed 
as part of the now defunct (or at least hibernating) common display framework. 
If I had replied to this mail thread without sleeping on it first I might not 
have known better and have got half-paranoid, wondereding whether there had 
been a deliberate attempt to fast-track this API before the V4L2 developers 
noticed. To be perfectly clear, there is *NO* implicit or explicit such 
accusation here, as I know better.

Let's all take this as a positive opportunity to cooperate more closely, media 
devices still need a huge effort to be cleanly supported on modern hardware, 
and we'll need all the development power we can get.

Accordingly, I would like to comment on the component API, despite the fact 
that it has been merged in mainline already. The first thing that I believe is 
missing is documentation. Do you have any pending patch for that, either as 
kerneldoc or as a text file for Documentation/ ? As I've read the code to 
understand it I might have missed so design goals, so please bear with the 
stupid questions that may follow.

I'll first provide a brief comparison of the two models to make the rest of 
the comments easier to understand.

v4l2-async calls the component master object v4l2_async_notifier. The base 
component child object is a v4l2_subdev instance instead of being a plain 
device. v4l2_subdev instances are stored in v4l2-async lists similarly to how 
the component framework stores objects, except that the list head is directly 
embedded inside the v4l2_subdev structure instead of being part of a separate 
structure allocated by the framework.

The notifier has three callback functions, bound, complete and unbind. The 
bound function is called when one component has been bound to the master. 
Similarly the unbind function is called when one component is about to be 
unbound from the master. The complete function is called when all components 
have been bound, and is thus equivalent to the bind function of the component 
framework.

Notifiers are registered along with a list of match entries. A match entry is 
roughly equivalent to the compare function passed to 
component_master_add_child, except that it includes built-in support for 
matching on an OF node, dev_name or I2C bus number and child address.

Whenever a subdev (component child) is registered with 
v4l2_async_register_subdev (equivalent to component_add), the list of 
notifiers (masters) is walked and their match entries are processed. If a 
matching entry is found the subdev is bound to the notifier immediately, 
otherwise it is added to a list of unbound subdevices (component_list). 
Whenever a notifier (component master) is registered with 
v4l2_async_notifier_register (component_master_add) the list of unbound 
subdevs is walked and every match entry of the notifier is tested. If a 
matching entry is found the subdev is bound to the notifier.

I've seen a couple of core differences in concept between your component model 
and the v4l2-async model:

- The component framework uses private master and component structures. 
Wouldn't it simplify the code from a memory management point of view to expose 
the master structure (which would then be embedded in driver-specific 
structures) and the component structure (which would be embedded in struct 
device) ? The latter would be slightly more intrusive from a struct device 
point of view, so I don't have a strong opinion there yet, exposing the master 
structure only might be better.

- The component framework requires the master to provide an add_components 
operation that will call the component_master_add_child function for every 
component it needs, with a compare function. The add child function is called 
when the master is registered, and then for every component added to the 
system. I'm not sure to understand the design decisions behind this, but these 
two levels of indirection appear pretty complex and confusing. Wouldn't it be 
simpler to pass an array of match entries to the master registration function 
instead and remove the add_components operation ? A match entry would 
basically be a structure with a compare function and a compare_data pointer.

We could also extend the match entry with explicit support for OF node and I2C 
bus number + address matching as those are the most common cases, or at least 
provide a couple of standard compare functions for those cases.

- The component framework doesn't provide partial bind support. Children are 
bound to the master only when all children are available. This makes it 
impossible in practice to implement v4l2-async on top of the component 
framework. What would you think about adding optional partial bind support ? 
The master operations would then have partial bind, complete bind, partial 
unbind and complete unbind functions. Drivers that only need full bind support 
could set the partial bind and unbind functions to NULL.

> It is completely agnostic to whether you're using platform data, DT or
> even ACPI - this code could *not* care less.  None of that comes anywhere
> near what this patch does.  It merely provides a way to collect up
> individual devices from co-operating drivers, and control their binding
> such that a subsystem like DRM or ALSA can be presented with a "card"
> level view of the hardware rather than a multi-device medusa with all
> the buggy, racy, crap fsckage that people come up to make that kind of
> thing work.
> 
> Now, as for the binding above, first, what does "eg" mean... and
> secondly, how would a binding which refers to crtcs and connectors
> have anything to do with ALSA?  Clearly this isn't an example of a
> binding for an ALSA use, which was talked about in the very first
> line of the above commit commentry.  So it's quite clear that what is
> given there is an example of how it /could/ be used.
> 
> I suppose I could have instead turned imx-drm into a completely unusable
> mess by not coming up with some kind of binding, and instead submitted
> a whole pile of completely untested code.  Alternatively, I could've
> used the OF binding as you're suggesting, but that would mean radically
> changing the /existing/ bindings for the IPU as a whole - something
> which others are better suited at as they have a /much/ better
> understanding of the complexities of this hardware than I.
> 
> So, what I have done is implemented - for a driver in staging which is
> still subject to ongoing development and non-stable DT bindings -
> something which allows forward progress with a *minimum* of disruption
> to the existing DT bindings for everyone, while still allowing forward
> progress.
> 
> Better bindings for imx-drm are currently being worked on.  Philipp
> Zabel of Pengutronix is currently looking at it, and has posted many
> RFC patches on this very subject, including moving the V4L2 OF helpers
> to a more suitable location.  OF people have been involved in that
> discussion over the preceding weeks, and there's a working implementation
> of imx-drm using these helpers from v4l2.
> 
> I'm finding people who are working in the same area and trying to get
> everyone talking to each other so that we /do/ end up with a set of
> bindings for the display stuff which are suitable for everyone.  Tomi
> from TI has already expressed his input to this ongoing discussion.
> 
> You're welcome to get involved in those discussions too.
> 
> I hope this makes it clear, and clears up the confusion.
> 
> Thanks.

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Laurent Pinchart]

Hi Russell,

(CC'ing Philipp Zabel who might be able to provide feedback as a user of the 
component framework)

Could you please have a look at the questions below and provide an answer when 
you'll have time ? I'd like to bridge the gap between the component and the 
V4L2 asynchronous registration implementations.

On Friday 07 March 2014 00:24:33 Laurent Pinchart wrote:
> On Wednesday 26 February 2014 22:19:39 Russell King - ARM Linux wrote:
> > On Wed, Feb 26, 2014 at 10:00:25PM +0100, Guennadi Liakhovetski wrote:
> > > Hi Russell
> > > 
> > > (I suspect this my email will be rejected by ALKML too like other my
> > > recent emails, but at least other MLs will pick it up and individual CCs
> > > too, so, if replying, maybe it would be good to keep my entire reply,
> > > all the more that it's going to be very short)
> > > 
> > > On Thu, 2 Jan 2014, Russell King wrote:
> > > > Subsystems such as ALSA, DRM and others require a single card-level
> > > > device structure to represent a subsystem.  However, firmware tends to
> > > > describe the individual devices and the connections between them.
> > > > 
> > > > Therefore, we need a way to gather up the individual component devices
> > > > together, and indicate when we have all the component devices.
> > > > 
> > > > We do this in DT by providing a "superdevice" node which specifies
> > > > 
> > > > the components, eg:
> > > > 	imx-drm {
> > > > 		compatible = "fsl,drm";
> > > > 		crtcs = <&ipu1>;
> > > > 		connectors = <&hdmi>;
> > > > 	};
> > > 
> > > It is a pity linux-media wasn't CC'ed and apparently V4L developers
> > > didn't notice this and other related patches in a "clean up" series, and
> > > now this patch is already in the mainline. But at least I'd like to ask
> > > whether the bindings, defined in
> > > Documentation/devicetree/bindings/media/video-interfaces.txt and
> > > implemented in drivers/media/v4l2-core/v4l2-of.c have been considered
> > > for this job, and - if so - why have they been found unsuitable?
> > > Wouldn't it have been better to use and - if needed - extend them to
> > > cover any deficiencies? Even though the implementation is currently
> > > located under drivers/media/v4l2-code/ it's pretty generic and should be
> > > easily transferable to a more generic location.
> > 
> > The component helpers have nothing to do with DT apart from solving
> > the problem of how to deal with subsystems which expect a single device,
> > but we have a group of devices and their individual drivers to cope with.
> > Subsystems like DRM and ALSA.
> 
> (and V4L2)
> 
> Point duly taken. First of all I want to mention that your proposal is
> greatly appreciated. This is a problem that crosses subsystem boundaries,
> and should thus be addressed centrally.
> 
> However, we (as in the V4L2 community, and me personally) would have
> appreciated to be CC'ed on the proposal. As you might know we already had a
> solution for this problem, albeit V4L2-specific, in drivers/media/v4l2-
> core/v4l2-async.c. Whether or not this solution should have been made
> generic instead of coming up with a new separate implementation would of
> course have been debatable, but the most important point would have been to
> make sure that v4l2-async could easily be implemented on top of the common
> component architecture.
> 
> The topic is particularly hot given that a similar solution was also
> proposed as part of the now defunct (or at least hibernating) common
> display framework. If I had replied to this mail thread without sleeping on
> it first I might not have known better and have got half-paranoid,
> wondereding whether there had been a deliberate attempt to fast-track this
> API before the V4L2 developers noticed. To be perfectly clear, there is
> *NO* implicit or explicit such accusation here, as I know better.
> 
> Let's all take this as a positive opportunity to cooperate more closely,
> media devices still need a huge effort to be cleanly supported on modern
> hardware, and we'll need all the development power we can get.
> 
> Accordingly, I would like to comment on the component API, despite the fact
> that it has been merged in mainline already. The first thing that I believe
> is missing is documentation. Do you have any pending patch for that, either
> as kerneldoc or as a text file for Documentation/ ? As I've read the code
> to understand it I might have missed so design goals, so please bear with
> the stupid questions that may follow.
> 
> I'll first provide a brief comparison of the two models to make the rest of
> the comments easier to understand.
> 
> v4l2-async calls the component master object v4l2_async_notifier. The base
> component child object is a v4l2_subdev instance instead of being a plain
> device. v4l2_subdev instances are stored in v4l2-async lists similarly to
> how the component framework stores objects, except that the list head is
> directly embedded inside the v4l2_subdev structure instead of being part of
> a separate structure allocated by the framework.
> 
> The notifier has three callback functions, bound, complete and unbind. The
> bound function is called when one component has been bound to the master.
> Similarly the unbind function is called when one component is about to be
> unbound from the master. The complete function is called when all components
> have been bound, and is thus equivalent to the bind function of the
> component framework.
> 
> Notifiers are registered along with a list of match entries. A match entry
> is roughly equivalent to the compare function passed to
> component_master_add_child, except that it includes built-in support for
> matching on an OF node, dev_name or I2C bus number and child address.
> 
> Whenever a subdev (component child) is registered with
> v4l2_async_register_subdev (equivalent to component_add), the list of
> notifiers (masters) is walked and their match entries are processed. If a
> matching entry is found the subdev is bound to the notifier immediately,
> otherwise it is added to a list of unbound subdevices (component_list).
> Whenever a notifier (component master) is registered with
> v4l2_async_notifier_register (component_master_add) the list of unbound
> subdevs is walked and every match entry of the notifier is tested. If a
> matching entry is found the subdev is bound to the notifier.
> 
> I've seen a couple of core differences in concept between your component
> model and the v4l2-async model:
> 
> - The component framework uses private master and component structures.
> Wouldn't it simplify the code from a memory management point of view to
> expose the master structure (which would then be embedded in
> driver-specific structures) and the component structure (which would be
> embedded in struct device) ? The latter would be slightly more intrusive
> from a struct device point of view, so I don't have a strong opinion there
> yet, exposing the master structure only might be better.
> 
> - The component framework requires the master to provide an add_components
> operation that will call the component_master_add_child function for every
> component it needs, with a compare function. The add child function is
> called when the master is registered, and then for every component added to
> the system. I'm not sure to understand the design decisions behind this,
> but these two levels of indirection appear pretty complex and confusing.
> Wouldn't it be simpler to pass an array of match entries to the master
> registration function instead and remove the add_components operation ? A
> match entry would basically be a structure with a compare function and a
> compare_data pointer.
> 
> We could also extend the match entry with explicit support for OF node and
> I2C bus number + address matching as those are the most common cases, or at
> least provide a couple of standard compare functions for those cases.
> 
> - The component framework doesn't provide partial bind support. Children are
> bound to the master only when all children are available. This makes it
> impossible in practice to implement v4l2-async on top of the component
> framework. What would you think about adding optional partial bind support
> ? The master operations would then have partial bind, complete bind,
> partial unbind and complete unbind functions. Drivers that only need full
> bind support could set the partial bind and unbind functions to NULL.
> 
> > It is completely agnostic to whether you're using platform data, DT or
> > even ACPI - this code could *not* care less.  None of that comes anywhere
> > near what this patch does.  It merely provides a way to collect up
> > individual devices from co-operating drivers, and control their binding
> > such that a subsystem like DRM or ALSA can be presented with a "card"
> > level view of the hardware rather than a multi-device medusa with all
> > the buggy, racy, crap fsckage that people come up to make that kind of
> > thing work.
> > 
> > Now, as for the binding above, first, what does "eg" mean... and
> > secondly, how would a binding which refers to crtcs and connectors
> > have anything to do with ALSA?  Clearly this isn't an example of a
> > binding for an ALSA use, which was talked about in the very first
> > line of the above commit commentry.  So it's quite clear that what is
> > given there is an example of how it /could/ be used.
> > 
> > I suppose I could have instead turned imx-drm into a completely unusable
> > mess by not coming up with some kind of binding, and instead submitted
> > a whole pile of completely untested code.  Alternatively, I could've
> > used the OF binding as you're suggesting, but that would mean radically
> > changing the /existing/ bindings for the IPU as a whole - something
> > which others are better suited at as they have a /much/ better
> > understanding of the complexities of this hardware than I.
> > 
> > So, what I have done is implemented - for a driver in staging which is
> > still subject to ongoing development and non-stable DT bindings -
> > something which allows forward progress with a *minimum* of disruption
> > to the existing DT bindings for everyone, while still allowing forward
> > progress.
> > 
> > Better bindings for imx-drm are currently being worked on.  Philipp
> > Zabel of Pengutronix is currently looking at it, and has posted many
> > RFC patches on this very subject, including moving the V4L2 OF helpers
> > to a more suitable location.  OF people have been involved in that
> > discussion over the preceding weeks, and there's a working implementation
> > of imx-drm using these helpers from v4l2.
> > 
> > I'm finding people who are working in the same area and trying to get
> > everyone talking to each other so that we /do/ end up with a set of
> > bindings for the display stuff which are suitable for everyone.  Tomi
> > from TI has already expressed his input to this ongoing discussion.
> > 
> > You're welcome to get involved in those discussions too.
> > 
> > I hope this makes it clear, and clears up the confusion.
> > 
> > Thanks.

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Russell King - ARM Linux]

On Wed, Mar 19, 2014 at 06:22:14PM +0100, Laurent Pinchart wrote:
> Hi Russell,
> 
> (CC'ing Philipp Zabel who might be able to provide feedback as a user of the 
> component framework)
> 
> Could you please have a look at the questions below and provide an answer when 
> you'll have time ? I'd like to bridge the gap between the component and the 
> V4L2 asynchronous registration implementations.

I have a reply partly prepared, but I'm snowed under by the L2 cache stuff
at the moment, sorry.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

[Russell King - ARM Linux]

On Fri, Mar 07, 2014 at 12:24:33AM +0100, Laurent Pinchart wrote:
> However, we (as in the V4L2 community, and me personally) would have 
> appreciated to be CC'ed on the proposal. As you might know we already had a 
> solution for this problem, albeit V4L2-specific, in drivers/media/v4l2-
> core/v4l2-async.c.

There's a lot of people who would have liked to be on the Cc, but there's
two problems: 1. the Cc list would be too big for mailing lists to accept
the message, and 2. finding everyone who should be on the Cc list is quite
an impossible task.

> The topic is particularly hot given that a similar solution was also
> proposed as part of the now defunct (or at least hibernating) common
> display framework. 

Yes, I am aware of CDF.  However, the annoying thing is that it's another
case of the bigger picture not being looked at - which is that we don't
need yet another subsystem specific solution to a problem which is not
subsystem specific.

The fact of the matter is that /anyone/ has the opportunity to come up
with a generic solution to this problem, and no one did... instead,
more solutions were generated - the proof is "we solved this in CDF
with a CDF specific solution". :p

> If I had replied to this mail thread without sleeping on it first I
> might not have known better and have got half-paranoid, wondereding
> whether there had been a deliberate attempt to fast-track this API
> before the V4L2 developers noticed. To be perfectly clear, there is
> *NO* implicit or explicit such accusation here, as I know better.

What would have happened is that CDF would have been raised, and there
would be a big long discussion with no real resolution.  The problem
would not have been solved (even partially).  We'd be sitting here right
now still without any kind of solution that anyone can use.

Instead, what we have right now is the opportunity for people to start
making use of this and solving the real problems they have with driver
initialisation.

For example, the IPU on iMX locks up after a number of mode changes, and
it's useful to be able to unload the driver, poke about in the hardware,
and reload it.  Without this problem fixed, that's impossible without
rebooting the kernel, because removing the driver oopses the kernel due
to the broken work-arounds that it has to do - and it has to do those
because this problem has not been solved, despite it being known about
for /years/.

> Accordingly, I would like to comment on the component API, despite the fact 
> that it has been merged in mainline already. The first thing that I believe is 
> missing is documentation. Do you have any pending patch for that, either as 
> kerneldoc or as a text file for Documentation/ ? As I've read the code to 
> understand it I might have missed so design goals, so please bear with the 
> stupid questions that may follow.

There's lots of things in the kernel which you just have to read the code
for - and this is one of them at the moment. :)  (Another is PM domains...)

What I know is that this will not satisfy all your requirements - I don't
want it to initially satisfy everyone's requirements, because that's just
far too big a job, but it satisfies the common problem that most people
are suffering from and have already implemented many badly written driver
specific solutions.

In other words - this is designed to _improve_ the current situation where
we have lots of buggy implementions trying to work around this problem,
factor that code out, and fix up those problems.

Briefly, the idea is:

- there is a master device - lots of these subsystems doing this already
  have that, whether that be ALSA or DRM based stuff.
- then there are the individual component devices and their drivers.

Subsystems like ALSA and DRM are not component based subsystems.  These
subsystems have two states - either they're initialised and the entire
"card system" is known about, or they're not initialised.  There is no
possibility of a piecemeal approach, where they partially come up and
additional stuff gets added later.  With DRM, that's especially true
because of how the userspace API works - to change that probably means
changing stuff all the way through things like the X server and its
xrandr application interface.  This is probably the reason why David said
at KS that DRM isn't going to do the hotplugging of components.

The master device has a privileged position - it gets to make the decision
about which component devices are relevant to it, and when the "card system"
is fully known.  As far as DT goes, we've had a long discussion about this
approach in the past, and we've accepted this approach - we have the
"sound" node which doesn't actually refer to any hardware block, it's a
node which describes how the hardware blocks are connected together, which
gets translated into a platform device.

When a master device gets added, it gets added to the list of master
devices, and then it's asked whether all the components that it needs
are present.  Since we don't want to fixate on any one particular
method to make that decision, it calls back via a method to the master
driver for that.  The master then determines which components should
be present, and asks the component helpers to check whether they are.
The master does this in the order which the components are to be later
bound (there's subsystems which have requirements on the ordering of
individual components appearing - for example, with DRM the CRTCs
need to be available before the encoders so that you know the CRTC
indexes to bind the encoders to.)

Each component that is found is added to a master-specific list, making
it unavailable to other masters (any particular component can't be owned
by two masters.)

If a component isn't found, then this process is unwound, and we wait for
more components to show up.

When a component is added, it is added to a list of all components.  All
unbound masters are polled to see whether they require this component in
the same way as above.

When a master indicates that it is complete, the master is bound, which
kicks off what would be the standard driver probe on non-componentised
systems.  Since we now know that we have all the components associated
with this master, this is now safe to do, and the master driver gets to
decide at what point during subsystem initialisation the components
should be bound.  This will normally be when the DRM device private
structures or ALSA card has been allocated, and are available for the
components to bind themselves to.

Should any component or master return a deferred probe, that error code
(or in fact any error code) is propagated all the way back through the
chain, unwinding the effects as we go - including devres allocated
resources.  Eventually, we hit the caller to component_add() or
component_master_add(), which would be a driver probe function, and this
propagates it back to the driver model.

So, the last driver (master or component) gets to eat any errors from
the bring up - that's reasonable, because we can't return an error from
an already completed probe function.  The side effect of this is very
important though - it gets us deferred probing.

That failed driver probe with -EPROBE_DEFER will be retried later,
hopefully when the resources that any one of those components needed is
now available.

Now, that's only half the story - the other half is far more important,
because it's the one which gives the most problems with the current
driver specific implementations, and is the one which leads to kernel
oopses.  That is, what happens when a bound component or master goes
away.

Remember that ALSA and DRM can't cope with that - the only way they can
cope is by having the entire "card system" removed.  It's an all or
nothing case.  With DRM, you wouldn't be able to remove a CRTC for
example, because that would break the CRTC numbering that userspace
sees via the kernel API, and also the CRTC numbering which xrandr Xorg
applications see.

So, when any component/master of a subsystem is removed, the whole setup
is unbound: this starts with the master being unbound, and the master
controls the point at which the components are unbound.  Again, normal
devres actions happen so drivers see proper devres behaviour.

> v4l2-async calls the component master object v4l2_async_notifier. The base 
> component child object is a v4l2_subdev instance instead of being a plain 
> device. v4l2_subdev instances are stored in v4l2-async lists similarly to how 
> the component framework stores objects, except that the list head is directly 
> embedded inside the v4l2_subdev structure instead of being part of a separate 
> structure allocated by the framework.

Right - I didn't want to modify existing structures like struct device,
because they're already bloated enough. Given that this currently affects
a small number of devices, it is unfair to impose this overhead on all
device structures in the kernel.

> The notifier has three callback functions, bound, complete and unbind. The 
> bound function is called when one component has been bound to the master. 
> Similarly the unbind function is called when one component is about to be 
> unbound from the master. The complete function is called when all components 
> have been bound, and is thus equivalent to the bind function of the component 
> framework.

So, what happens after all components have been bound (and the complete
callback has been called), and then one component is removed from the
system?  From your description, it sounds like nothing apart from the
"unbind" callback for that component.

Moreover, what happens if that component is then added back later?
Do you get another "bind" followed by "complete"?

How would a subsystem like, DRM or ALSA, be able to fit into this model
when these subsystems can't tolerate any component going away or being
introduced after the initial bring-up?

> Notifiers are registered along with a list of match entries. A match
> entry is roughly equivalent to the compare function passed to 
> component_master_add_child, except that it includes built-in support for 
> matching on an OF node, dev_name or I2C bus number and child address.

This is something we could add to the component model.  However, one of
the target design goals is that the core should be free of the matching
method, so that we don't end up with something that is specific only to
existing ARM systems.

However, one of the issues here is how to pass this information in - it
can't be a static array (see below.)

> Whenever a subdev (component child) is registered with 
> v4l2_async_register_subdev (equivalent to component_add), the list of 
> notifiers (masters) is walked and their match entries are processed. If a 
> matching entry is found the subdev is bound to the notifier immediately, 
> otherwise it is added to a list of unbound subdevices (component_list). 
> Whenever a notifier (component master) is registered with 
> v4l2_async_notifier_register (component_master_add) the list of unbound 
> subdevs is walked and every match entry of the notifier is tested. If a 
> matching entry is found the subdev is bound to the notifier.

The more interesting question is what happens when a component is removed.
This is what kills a lot of bad implementations of this right now.  It's
basically impossible to remove any component without oopsing the kernel.

> I've seen a couple of core differences in concept between your component
> model and the v4l2-async model:
> 
> - The component framework uses private master and component structures. 
> Wouldn't it simplify the code from a memory management point of view to
> expose the master structure (which would then be embedded in driver-specific 
> structures) and the component structure (which would be embedded in struct 
> device) ?

This doesn't work everywhere.  Take ALSA as an example.  There is struct
snd_card.  This is created by snd_card_create() and allocates your driver
private data structure for you in addition to the snd_card.

When you want to tear down the snd_card, this includes freeing all memory
associated with it, including the driver private data structure.  If a
component helper data structure were to be embedded in the driver private
data structure, it would also be freed - while still being registered
into component stuff.

Conversely, if you created the snd_card outside of the component stuff,
then you have no real way to remove the components when they go away.

Of course, the driver could allocate its own struct just for this, but
then where does it store a pointer to that?  In the driver data?  What
if the subsystem (and some do) insist on putting their own data in the
struct device driver data pointer?

DRM used to do exactly that (it's changed very recently to allow drivers
to do their own thing wrt driver_data - but this wasn't the case when the
component stuff was written, which was shortly after kernel summit.)

So, that's why I made the decision to have it as a separate structure.
Using the solution you're suggesting would result in unnecessary
restrictions on how drivers and subsystems should behave.

I don't think any of us want to go through changing the way lots of
subsystems and possibly hundreds of drivers work in this regard.  Hence,
while it may not appear the simplest solution, in the bigger picture,
it's better not to impose any kind of avoidable requirements on drivers.

> - The component framework requires the master to provide an add_components 
> operation that will call the component_master_add_child function for every 
> component it needs, with a compare function. The add child function is
> called when the master is registered, and then for every component added
> to the system. I'm not sure to understand the design decisions behind
> this, but these two levels of indirection appear pretty complex and
> confusing. Wouldn't it be simpler to pass an array of match entries to
> the master registration function instead and remove the add_components
> operation ? A match entry would basically be a structure with a compare
> function and a compare_data pointer.

We could supply an array of match function, match data, but the issue
here is that the match data generally isn't constant - the driver would
have to allocate an array of {function, data} and register that array
into the component helper.  Not much problem with that, except it
results in more complexity.

> We could also extend the match entry with explicit support for OF node
> and I2C bus number + address matching as those are the most common cases,
> or at least provide a couple of standard compare functions for those cases.

This I don't like, because it's making assumptions about the struct
device, encoding subsystem and firmware specific knowledge.  It's
fine to do that as a match helper, but not as something core.

> - The component framework doesn't provide partial bind support. Children
> are bound to the master only when all children are available. This makes
> it impossible in practice to implement v4l2-async on top of the component 
> framework. What would you think about adding optional partial bind support ? 

As you can see from the above explanation, this is intentional in this
iteration, as this is the common requirement for these subsystems that
it's trying to solve the problem for.

That's not to say that it can't be done - at the moment I have some
concerns from the point of view of these other subsystems which don't
support partial bind, and ensuring that partial bind doesn't get
mis-used there.

In any case, there is scope for the way the master discovers its
components to be changed, and I feel that would be a pre-requisit to
partial bind support.

Philipp's work on imx-drm for example, has ended up allocating an
ordered list of devices, though there's patches around to remove that
again.  That was necessary due to a work-around for the brokenness in
existing imx-drm for the CRTC nodes (which prevents a successful match
against a previously found component) which will now be removed.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.