Re: [PATCH RFC 0/2] drivers/base: simplify simple DT-based components
On Fri, 7 Feb 2014 20:23:51 + Russell King - ARM Linux li...@arm.linux.org.uk wrote: 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 I rewrote the tda998x as a simple encoder+connector (i.e. not a slave_encoder) with your component helper, and the code is much clearer and simpler: the DRM driver has nothing to do except to know that the tda998x is a component and to set the possible_crtcs. AFAIK, only the tilcdc drm driver is using the tda998x as a slave_encoder. It does a (encoder+connector) conversion to (slave_encoder). Then, in your changes in the TDA998x, you do a (slave_encoder) translation to (encoder+connector). This seems rather complicated! I think it would be easier to use your component helper and rewrite (remove?) tilcdc_slave.c. 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. This function exists already in drivers/of/platform.c as of_dev_node_match(). It just needs to be exported. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 0/2] drivers/base: simplify simple DT-based components
On Sun, Feb 09, 2014 at 10:22:19AM +0100, Jean-Francois Moine wrote: On Fri, 7 Feb 2014 20:23:51 + Russell King - ARM Linux li...@arm.linux.org.uk wrote: 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 I rewrote the tda998x as a simple encoder+connector (i.e. not a slave_encoder) with your component helper, and the code is much clearer and simpler: the DRM driver has nothing to do except to know that the tda998x is a component and to set the possible_crtcs. That's exactly what I've done - the slave encoder veneer can be simply deleted when tilcdc is converted. AFAIK, only the tilcdc drm driver is using the tda998x as a slave_encoder. It does a (encoder+connector) conversion to (slave_encoder). Then, in your changes in the TDA998x, you do a (slave_encoder) translation to (encoder+connector). This seems rather complicated! No. I first split out the slave encoder functions to be a veneer onto the tda998x backend, and then add the encoder connector component support. Later, the slave encoder veneer can be deleted. The reason for this is that virtually all the tda998x backend is what's required by the encoder connector support - which is completely logical when you realise that the generic slave encoder support is just a veneer itself adapting the encoder connector support to a slave encoder. So, we're basically getting rid of two veneers, but we end up with exactly the same functionality. 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. This function exists already in drivers/of/platform.c as of_dev_node_match(). It just needs to be exported. Good, thanks for pointing that out. -- 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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-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(-) -- 1.9.rc1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 0/2] drivers/base: simplify simple DT-based components
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. -- 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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 0/2] drivers/base: simplify simple DT-based components
On Fri, 7 Feb 2014 17:33:26 + Russell King - ARM Linux li...@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...). -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 0/2] drivers/base: simplify simple DT-based components
On Fri, Feb 07, 2014 at 07:42:04PM +0100, Jean-Francois Moine wrote: On Fri, 7 Feb 2014 17:33:26 + Russell King - ARM Linux li...@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. -- 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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 0/2] drivers/base: simplify simple DT-based components
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-devel
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
Re: [PATCH RFC 0/2] drivers/base: simplify simple DT-based components
On Fri, Feb 07, 2014 at 06:59:11PM +, 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 -- 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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
