[alsa-devel] [PATCH RFC 0/2] drivers/base: simplify simple DT-based components
This patch series tries to simplify the code of simple devices in case they are part of componentised subsystems, are declared in a DT, and are not using the component bin/unbind functions.
Jean-Francois Moine (2): drivers/base: permit base components to omit the bind/unbind ops drivers/base: declare phandle DT nodes as components
drivers/base/component.c | 21 +++++++++++++++++++-- drivers/base/core.c | 18 ++++++++++++++++++ include/linux/of.h | 2 ++ 3 files changed, 39 insertions(+), 2 deletions(-)
Some simple components don't need to do any specific action on bind to / unbind from a master component.
This patch permits such components to omit the bind/unbind operations.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/base/component.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/base/component.c b/drivers/base/component.c index c53efe6..0a39d7a 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -225,7 +225,8 @@ static void component_unbind(struct component *component, { WARN_ON(!component->bound);
- component->ops->unbind(component->dev, master->dev, data); + if (component->ops) + component->ops->unbind(component->dev, master->dev, data); component->bound = false;
/* Release all resources claimed in the binding of this component */ @@ -274,7 +275,11 @@ static int component_bind(struct component *component, struct master *master, 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 (component->ops) + ret = component->ops->bind(component->dev, master->dev, data); + else + ret = 0; + if (!ret) { component->bound = true;
On Fri, Feb 07, 2014 at 04:55:00PM +0100, Jean-Francois Moine wrote:
Some simple components don't need to do any specific action on bind to / unbind from a master component.
This patch permits such components to omit the bind/unbind operations.
Signed-off-by: Jean-Francois Moine moinejf@free.fr
drivers/base/component.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/base/component.c b/drivers/base/component.c index c53efe6..0a39d7a 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -225,7 +225,8 @@ static void component_unbind(struct component *component, { WARN_ON(!component->bound);
- component->ops->unbind(component->dev, master->dev, data);
if (component->ops)
component->ops->unbind(component->dev, master->dev, data);component->bound = false;
/* Release all resources claimed in the binding of this component */
@@ -274,7 +275,11 @@ static int component_bind(struct component *component, struct master *master, 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 (component->ops)
ret = component->ops->bind(component->dev, master->dev, data);- else
ret = 0;
NAK. If this is done, there's absolutely no point to this code.
At system startup time, some devices depends on the availability of some other devices before starting. The infrastructure for componentised subsystems permits to handle this dependence, each driver defining its own role.
This patch does an automatic creation of the lowest components in case of DT. This permits simple devices to be part of complex componentised subsystems without any specific code.
When created from DT, the device dependence is generally indicated by phandle: a device which is pointed to by a phandle must be started before the pointing device(s).
So, at device register time, the devices which are pointed to by a phandle are declared as components, except when they declared themselves as such in their probe function.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/base/component.c | 12 ++++++++++++ drivers/base/core.c | 18 ++++++++++++++++++ include/linux/of.h | 2 ++ 3 files changed, 32 insertions(+)
diff --git a/drivers/base/component.c b/drivers/base/component.c index 0a39d7a..3cab26b 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -17,6 +17,7 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/slab.h> +#include <linux/of.h>
struct master { struct list_head node; @@ -336,6 +337,7 @@ EXPORT_SYMBOL_GPL(component_bind_all); int component_add(struct device *dev, const struct component_ops *ops) { struct component *component; + struct device_node *np; int ret;
component = kzalloc(sizeof(*component), GFP_KERNEL); @@ -356,6 +358,11 @@ int component_add(struct device *dev, const struct component_ops *ops)
kfree(component); } + + np = dev->of_node; + if (np) + np->_flags |= OF_DEV_COMPONENT; + mutex_unlock(&component_mutex);
return ret < 0 ? ret : 0; @@ -365,6 +372,7 @@ EXPORT_SYMBOL_GPL(component_add); void component_del(struct device *dev, const struct component_ops *ops) { struct component *c, *component = NULL; + struct device_node *np;
mutex_lock(&component_mutex); list_for_each_entry(c, &component_list, node) @@ -377,6 +385,10 @@ void component_del(struct device *dev, const struct component_ops *ops) if (component && component->master) take_down_master(component->master);
+ np = dev->of_node; + if (np) + np->_flags &= ~OF_DEV_COMPONENT; + mutex_unlock(&component_mutex);
WARN_ON(!component); diff --git a/drivers/base/core.c b/drivers/base/core.c index 2b56717..0517b91 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -27,6 +27,7 @@ #include <linux/pm_runtime.h> #include <linux/netdevice.h> #include <linux/sysfs.h> +#include <linux/component.h>
#include "base.h" #include "power/power.h" @@ -980,6 +981,7 @@ int device_add(struct device *dev) struct device *parent = NULL; struct kobject *kobj; struct class_interface *class_intf; + struct device_node *np; int error = -EINVAL;
dev = get_device(dev); @@ -1088,6 +1090,15 @@ int device_add(struct device *dev) class_intf->add_dev(dev, class_intf); mutex_unlock(&dev->class->p->mutex); } + + /* if DT-created device referenced by phandle, create a component */ + np = dev->of_node; + if (np && np->phandle && + !(np->_flags & OF_DEV_COMPONENT)) { + component_add(dev, NULL); + np->_flags |= OF_DEV_IMPLICIT_COMPONENT; + } + done: put_device(dev); return error; @@ -1189,10 +1200,17 @@ void device_del(struct device *dev) { struct device *parent = dev->parent; struct class_interface *class_intf; + struct device_node *np;
/* Notify clients of device removal. This call must come * before dpm_sysfs_remove(). */ + np = dev->of_node; + if (np && (np->_flags & OF_DEV_COMPONENT)) { + component_del(dev, NULL); + np->_flags &= ~OF_DEV_IMPLICIT_COMPONENT; + } + if (dev->bus) blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_DEL_DEVICE, dev); diff --git a/include/linux/of.h b/include/linux/of.h index 70c64ba..40f1c34 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -59,6 +59,8 @@ struct device_node { struct proc_dir_entry *pde; /* this node's proc directory */ struct kref kref; unsigned long _flags; +#define OF_DEV_COMPONENT 1 +#define OF_DEV_IMPLICIT_COMPONENT 2 void *data; #if defined(CONFIG_SPARC) const char *path_component_name;
On Fri, Feb 07, 2014 at 05:53:27PM +0100, Jean-Francois Moine wrote:
At system startup time, some devices depends on the availability of some other devices before starting. The infrastructure for componentised subsystems permits to handle this dependence, each driver defining its own role.
This patch does an automatic creation of the lowest components in case of DT. This permits simple devices to be part of complex componentised subsystems without any specific code.
A component with no operations makes precisely no sense - with that, there's no way for the component to be a stand-alone driver. Your approach forces your ideas onto every DT device that is referenced as a phandle. That's extremely restrictive.
I don't want the component stuff knowing anything about OF. I don't want it knowing about driver matching. I don't want it knowing about ACPI either. That's the whole point behind it - it is 100% agnostic about how that stuff works.
The model is quite simply this:
- a master device is the covering component for the "card" - the master device knows what components to expect by some means. In the case of DT, that's by phandle references to the components. - the master device handles the component independent setup of the "card", creating the common resources that are required. When it's ready, it asks for the components to be bound. - upon removal of any component, the master component is unbound, which triggers the removal of the "card" from the subsystem. - as part of the removal, sub-components are unbound. - the master device should have as /little/ knowledge about the components as possible to permit component re-use.
- a component driver should be independent of it's master. - A component which is probed from the device model should simply register itself using component_add() with an appropriate operations structure, and a removal function which deletes itself. - When the driver is ready to be initialised (when the "card" level resources have been set in place) the "bind" method will be called. At this point, the component does everything that a classical driver model driver would do in it's probe callback. - unbind is the same as remove in the classical driver model.
So, please, no DT stuff in the component support - it's simply not required.
On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
This patch series tries to simplify the code of simple devices in case they are part of componentised subsystems, are declared in a DT, and are not using the component bin/unbind functions.
I wonder - I said earlier today that this works absolutely fine without modification with DT, so why are you messing about with it adding DT support?
This is totally the wrong approach. The idea is that this deals with /devices/ and /devices/ only. It groups up /devices/.
It's up to the add_component callback to the master device to decide how to deal with that.
Jean-Francois Moine (2): drivers/base: permit base components to omit the bind/unbind ops
And this patch has me wondering if you even understand how to use this... The master bind/unbind callbacks are the ones which establish the "card" based context with the subsystem.
Please, before buggering up this nicely designed implementation, please /first/ look at the imx-drm rework which was posted back in early January which illustrates how this is used in a DT context - which is something I've already pointed you at once today already.
On Fri, 7 Feb 2014 17:33:26 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
This patch series tries to simplify the code of simple devices in case they are part of componentised subsystems, are declared in a DT, and are not using the component bin/unbind functions.
I wonder - I said earlier today that this works absolutely fine without modification with DT, so why are you messing about with it adding DT support?
This is totally the wrong approach. The idea is that this deals with /devices/ and /devices/ only. It groups up /devices/.
It's up to the add_component callback to the master device to decide how to deal with that.
Jean-Francois Moine (2): drivers/base: permit base components to omit the bind/unbind ops
And this patch has me wondering if you even understand how to use this... The master bind/unbind callbacks are the ones which establish the "card" based context with the subsystem.
Please, before buggering up this nicely designed implementation, please /first/ look at the imx-drm rework which was posted back in early January which illustrates how this is used in a DT context - which is something I've already pointed you at once today already.
As I told in a previous mail, your code works fine in my DT-based Cubox. I am rewriting the TDA988x as a normal encoder/connector, and, yes, the bind/unbind functions are useful in this case.
But you opened a door. In a DT context, you know that the probe_defer mechanism does not work correctly. Your work permits to solve delicate cases: your component_add tells exactly when a device is available, and the master bind callback is the green signal for the device waiting for its resources. Indeed, your system was not created for such a usage, but it works as it is (anyway, the component bind/unbind functions may be empty...).
On Fri, Feb 07, 2014 at 07:42:04PM +0100, Jean-Francois Moine wrote:
On Fri, 7 Feb 2014 17:33:26 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
This patch series tries to simplify the code of simple devices in case they are part of componentised subsystems, are declared in a DT, and are not using the component bin/unbind functions.
I wonder - I said earlier today that this works absolutely fine without modification with DT, so why are you messing about with it adding DT support?
This is totally the wrong approach. The idea is that this deals with /devices/ and /devices/ only. It groups up /devices/.
It's up to the add_component callback to the master device to decide how to deal with that.
Jean-Francois Moine (2): drivers/base: permit base components to omit the bind/unbind ops
And this patch has me wondering if you even understand how to use this... The master bind/unbind callbacks are the ones which establish the "card" based context with the subsystem.
Please, before buggering up this nicely designed implementation, please /first/ look at the imx-drm rework which was posted back in early January which illustrates how this is used in a DT context - which is something I've already pointed you at once today already.
As I told in a previous mail, your code works fine in my DT-based Cubox. I am rewriting the TDA988x as a normal encoder/connector, and, yes, the bind/unbind functions are useful in this case.
So, which bit of "I've already got that" was missed?
But you opened a door. In a DT context, you know that the probe_defer mechanism does not work correctly. Your work permits to solve delicate cases: your component_add tells exactly when a device is available, and the master bind callback is the green signal for the device waiting for its resources. Indeed, your system was not created for such a usage, but it works as it is (anyway, the component bind/unbind functions may be empty...).
Sorry. Deferred probe does work, it's been tested with imx-drm, not only from the master component but also the sub-components. There's no problem here.
And no component bind/unbind function should ever be empty.
Again, I put it to you that you don't understand this layer.
On Fri, Feb 07, 2014 at 06:59:11PM +0000, Russell King - ARM Linux wrote:
Sorry. Deferred probe does work, it's been tested with imx-drm, not only from the master component but also the sub-components. There's no problem here.
Here's the proof that it also works with the Cubox, and armada DRM:
[drm] Initialized drm 1.1.0 20060810 ... armada-drm armada-510-drm: master bind failed: -517 i2c 0-0070: Driver tda998x requests probe deferral ... tda998x 0-0070: found TDA19988 armada-drm armada-510-drm: bound 0-0070 (ops tda998x_ops)
So, in the above sequence, the armada DRM driver was bound to its driver initially, but the TDA998x driver wasn't.
Then, the TDA998x driver is bound, which completes the requirements for the DRM master. So the system attempts to bind.
In doing so, the master probe function discovers a missing clock (because the SIL5531 driver hasn't probed) and it returns -EPROBE_DEFER. This causes the probe of the TDA998x to be deferred.
Later, deferred probes are run - at this time the SIL5531 driver has probed its device, and the clocks are now available. So when the TDA998x driver is re-probed, it retriggers the binding attempt, and as the clock can now be found, the system is bound and the DRM system for the device is initialised.
I've just committed a patch locally which makes Armada DRM fully use the component helper, which removes in totality the four armada_output.* and armada_slave.* files since they're no longer required:
[cubox-3.13 e2713ff5ac2f] DRM: armada: remove non-component support 7 files changed, 8 insertions(+), 437 deletions(-) delete mode 100644 drivers/gpu/drm/armada/armada_output.c delete mode 100644 drivers/gpu/drm/armada/armada_output.h delete mode 100644 drivers/gpu/drm/armada/armada_slave.c delete mode 100644 drivers/gpu/drm/armada/armada_slave.h
On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
This patch series tries to simplify the code of simple devices in case they are part of componentised subsystems, are declared in a DT, and are not using the component bin/unbind functions.
Here's my changes to the TDA998x driver to add support for the component helper. The TDA998x driver retains support for the old way so that drivers can be transitioned. For any one DRM "card" the transition to using the component layer must be all-in or all-out - partial transitions are not permitted with the simple locking implementation currently in the component helper due to the possibility of deadlock. (Master binds, holding the component lock, master declares i2c device, i2c device is bound to tda998x, which tries to register with the component layer, trying to take the held lock.)
http://ftp.arm.linux.org.uk/cgit/linux-cubox.git/log/?h=unstable/tda998x-dev...
It's marked unstable because the two "drivers/base" commits in there are _not_ the upstream commits. You'll notice that I just sent one of those to Gregkh in patch form.
Now, the bits which aren't in that branch but which update the Armada driver is the below, which needs some clean up and removal of some local differences I have in my cubox tree, but nicely illustrates why /your/ patches to the component stuff is the wrong approach.
Not only does the patch below add support for using the componentised TDA998x driver in the above link, but it allows that componentised support to be specified not only via platform data but also via DT. The DT stuff is untested, but it works exactly the same way as imx-drm does which has been tested.
And yes, I'm thinking that maybe moving compare_of() into the component support so that drivers can share this generic function may be a good idea.
So... as I've been saying, no changes to component.c are required here.
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c index 6f6554bac93f..1f2b7c60bff9 100644 --- a/drivers/gpu/drm/armada/armada_drv.c +++ b/drivers/gpu/drm/armada/armada_drv.c @@ -6,7 +6,9 @@ * published by the Free Software Foundation. */ #include <linux/clk.h> +#include <linux/component.h> #include <linux/module.h> +#include <linux/platform_data/armada_drm.h> #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include "armada_crtc.h" @@ -53,6 +55,11 @@ static const struct armada_drm_slave_config tda19988_config = { }; #endif
+static bool is_componentized(struct device *dev) +{ + return dev->of_node || dev->platform_data; +} + static void armada_drm_unref_work(struct work_struct *work) { struct armada_private *priv = @@ -171,16 +178,22 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags) goto err_kms; }
+ if (is_componentized(dev->dev)) { + ret = component_bind_all(dev->dev, dev); + if (ret) + goto err_kms; + } else { #ifdef CONFIG_DRM_ARMADA_TDA1998X - ret = armada_drm_connector_slave_create(dev, &tda19988_config); - if (ret) - goto err_kms; + ret = armada_drm_connector_slave_create(dev, &tda19988_config); + if (ret) + goto err_kms; #endif #ifdef CONFIG_DRM_ARMADA_TDA1998X_NXP - ret = armada_drm_tda19988_nxp_create(dev); - if (ret) - goto err_kms; + ret = armada_drm_tda19988_nxp_create(dev); + if (ret) + goto err_kms; #endif + }
ret = drm_vblank_init(dev, n); if (ret) @@ -204,6 +217,10 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags) drm_irq_uninstall(dev); err_kms: drm_mode_config_cleanup(dev); + + if (is_componentized(dev->dev)) + component_unbind_all(dev->dev, dev); + drm_mm_takedown(&priv->linear); flush_work(&priv->fb_unref_work);
@@ -218,6 +235,10 @@ static int armada_drm_unload(struct drm_device *dev) armada_fbdev_fini(dev); drm_irq_uninstall(dev); drm_mode_config_cleanup(dev); + + if (is_componentized(dev->dev)) + component_unbind_all(dev->dev, dev); + drm_mm_takedown(&priv->linear); flush_work(&priv->fb_unref_work); dev->dev_private = NULL; @@ -380,14 +401,82 @@ static struct drm_driver armada_drm_driver = { .fops = &armada_drm_fops, };
+static int armada_drm_bind(struct device *dev) +{ + return drm_platform_init(&armada_drm_driver, to_platform_device(dev)); +} + +static void armada_drm_unbind(struct device *dev) +{ + drm_platform_exit(&armada_drm_driver, to_platform_device(dev)); +} + +static int compare_of(struct device *dev, void *data) +{ + return dev->of_node == data; +} + +static int armada_drm_compare_name(struct device *dev, void *data) +{ + const char *name = data; + return !strcmp(dev_name(dev), name); +} + +static int armada_drm_add_components(struct device *dev, struct master *master) +{ + int i, ret = -ENXIO; + + if (dev->of_node) { + struct device_node *np = dev->of_node; + + for (i = 0; ; i++) { + struct device_node *node; + + node = of_parse_phandle(np, "connectors", i); + if (!node) + break; + + ret = component_master_add_child(master, compare_of, + node); + of_node_put(node); + if (ret) + break; + } + } else if (dev->platform_data) { + struct armada_drm_platform_data *data = dev->platform_data; + + for (i = 0; i < data->num_devices; i++) { + ret = component_master_add_child(master, + armada_drm_compare_name, + (void *)data->devices[i]); + if (ret) + break; + } + } + + return ret; +} + +static const struct component_master_ops armada_master_ops = { + .add_components = armada_drm_add_components, + .bind = armada_drm_bind, + .unbind = armada_drm_unbind, +}; + static int armada_drm_probe(struct platform_device *pdev) { - return drm_platform_init(&armada_drm_driver, pdev); + if (is_componentized(&pdev->dev)) + return component_master_add(&pdev->dev, &armada_master_ops); + else + return drm_platform_init(&armada_drm_driver, pdev); }
static int armada_drm_remove(struct platform_device *pdev) { - drm_platform_exit(&armada_drm_driver, pdev); + if (is_componentized(&pdev->dev)) + component_master_del(&pdev->dev, &armada_master_ops); + else + drm_platform_exit(&armada_drm_driver, pdev); return 0; }
participants (2)
-
Jean-Francois Moine -
Russell King - ARM Linux