Re: [PATCH 16/21] OMAPDSS: handle output-driver reg/unreg more dynamically
On Wednesday 07 March 2012 06:14 PM, Tomi Valkeinen wrote:
Initialize and uninitialize the output drivers by using arrays of
pointers to the init/uninit functions. This simplifies the code
slightly.
Signed-off-by: Tomi Valkeinentomi.valkei...@ti.com
---
drivers/video/omap2/dss/core.c | 111 +---
drivers/video/omap2/dss/dss.h | 41 ---
2 files changed, 59 insertions(+), 93 deletions(-)
diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index 654962a..ac4f2cb 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -503,10 +503,54 @@ static int omap_dss_bus_register(void)
}
/* INIT */
+static int (*dss_output_drv_reg_funcs[])(void) __initdata = {
+#ifdef CONFIG_OMAP2_DSS_DPI
+ dpi_init_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_SDI
+ sdi_init_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_RFBI
+ rfbi_init_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_VENC
+ venc_init_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_DSI
+ dsi_init_platform_driver,
+#endif
+#ifdef CONFIG_OMAP4_DSS_HDMI
+ hdmi_init_platform_driver,
+#endif
+};
+
+static void (*dss_output_drv_unreg_funcs[])(void) __exitdata = {
+#ifdef CONFIG_OMAP2_DSS_DPI
+ dpi_uninit_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_SDI
+ sdi_uninit_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_RFBI
+ rfbi_uninit_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_VENC
+ venc_uninit_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_DSI
+ dsi_uninit_platform_driver,
+#endif
+#ifdef CONFIG_OMAP4_DSS_HDMI
+ hdmi_uninit_platform_driver,
+#endif
+};
+
+static bool dss_output_drv_loaded[ARRAY_SIZE(dss_output_drv_reg_funcs)];
static int __init omap_dss_register_drivers(void)
{
int r;
+ int i;
r = platform_driver_probe(omap_dss_driver, omap_dss_probe);
if (r)
@@ -524,56 +568,18 @@ static int __init omap_dss_register_drivers(void)
goto err_dispc;
}
- r = dpi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize dpi platform driver\n);
- goto err_dpi;
- }
-
- r = sdi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize sdi platform driver\n);
- goto err_sdi;
- }
-
- r = rfbi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize rfbi platform driver\n);
- goto err_rfbi;
- }
-
- r = venc_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize venc platform driver\n);
- goto err_venc;
- }
-
- r = dsi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize DSI platform driver\n);
- goto err_dsi;
- }
-
- r = hdmi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize hdmi\n);
- goto err_hdmi;
+ /*
+* It's ok if the output-driver register fails. It happens, for example,
+* when there is no output-device (e.g. SDI for OMAP4).
+*/
Suppose we do a omap2plus_defconfig, CONFIG_OMAP2_DSS_SDI would be
selected, and sdi.c would be built, if we boot on OMAP4, why would a sdi
driver register cause a failure? Wouldn't the sdi driver just get
registered, and wait till eternity for the corresponding sdi platform
device to get registered?
Archit
+ for (i = 0; i ARRAY_SIZE(dss_output_drv_reg_funcs); ++i) {
+ r = dss_output_drv_reg_funcs[i]();
+ if (r == 0)
+ dss_output_drv_loaded[i] = true;
}
return 0;
-err_hdmi:
- dsi_uninit_platform_driver();
-err_dsi:
- venc_uninit_platform_driver();
-err_venc:
- rfbi_uninit_platform_driver();
-err_rfbi:
- sdi_uninit_platform_driver();
-err_sdi:
- dpi_uninit_platform_driver();
-err_dpi:
- dispc_uninit_platform_driver();
err_dispc:
dss_uninit_platform_driver();
err_dss:
@@ -584,12 +590,13 @@ err_dss:
static void __exit omap_dss_unregister_drivers(void)
{
- hdmi_uninit_platform_driver();
- dsi_uninit_platform_driver();
- venc_uninit_platform_driver();
- rfbi_uninit_platform_driver();
- sdi_uninit_platform_driver();
- dpi_uninit_platform_driver();
+ int i;
+
+ for (i = 0; i ARRAY_SIZE(dss_output_drv_unreg_funcs); ++i) {
+ if (dss_output_drv_loaded[i])
+ dss_output_drv_unreg_funcs[i]();
+ }
+
dispc_uninit_platform_driver();
dss_uninit_platform_driver();
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 24aadde..af7bed1 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -265,14 +265,9 @@ int dss_calc_clock_div(bool is_tft, unsigned long
Re: [PATCH 16/21] OMAPDSS: handle output-driver reg/unreg more dynamically
On Thu, 2012-03-08 at 14:04 +0530, Archit Taneja wrote:
On Wednesday 07 March 2012 06:14 PM, Tomi Valkeinen wrote:
- r = hdmi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize hdmi\n);
- goto err_hdmi;
+ /*
+* It's ok if the output-driver register fails. It happens, for example,
+* when there is no output-device (e.g. SDI for OMAP4).
+*/
Suppose we do a omap2plus_defconfig, CONFIG_OMAP2_DSS_SDI would be
selected, and sdi.c would be built, if we boot on OMAP4, why would a sdi
driver register cause a failure? Wouldn't the sdi driver just get
registered, and wait till eternity for the corresponding sdi platform
device to get registered?
No. Well, yes.
Currently we use platform_driver_register() to register the drivers, and
it does just what you described. But a few patches later I change
platform_driver_register() to platform_driver_probe(), which will return
ENODEV if there are no matching devices for the driver.
I originally had the platform_driver_probe() patch before this patch,
and thus the comment above made sense. Now the patch is after this
patch, so the comment is not exactly right until the probe patch is also
applied.
The point with platform_driver_probe() is that it can be used with
non-removable devices which are created at boot time, like the DSS
components. With platform_driver_probe() the probe function is called
only at that one time, and never afterwards. So probe can be in __init
section, and thrown away after init.
One side effect of using platform_driver_probe() is that it returns
ENODEV is there are no devices. In a simple module, the error can be
then returned from module_init, thus causing the whole module to be
unloaded. Our case is a bit more complex as we have multiple drivers in
the same module.
A downside with that is that we don't really know if the ENODEV error
happened because there were no devices (which is ok), or if it came from
probe function (which is not so ok). However, I thought that it doesn't
matter if an output driver has failed. We can still continue with the
other output drivers just fine.
Actually, there is a small problem. If, for example, DSI driver fails to
load, and DPI driver tries to use DSI PLL...
Tomi
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 16/21] OMAPDSS: handle output-driver reg/unreg more dynamically
On Thursday 08 March 2012 02:16 PM, Tomi Valkeinen wrote:
On Thu, 2012-03-08 at 14:04 +0530, Archit Taneja wrote:
On Wednesday 07 March 2012 06:14 PM, Tomi Valkeinen wrote:
- r = hdmi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize hdmi\n);
- goto err_hdmi;
+ /*
+* It's ok if the output-driver register fails. It happens, for example,
+* when there is no output-device (e.g. SDI for OMAP4).
+*/
Suppose we do a omap2plus_defconfig, CONFIG_OMAP2_DSS_SDI would be
selected, and sdi.c would be built, if we boot on OMAP4, why would a sdi
driver register cause a failure? Wouldn't the sdi driver just get
registered, and wait till eternity for the corresponding sdi platform
device to get registered?
No. Well, yes.
Currently we use platform_driver_register() to register the drivers, and
it does just what you described. But a few patches later I change
platform_driver_register() to platform_driver_probe(), which will return
ENODEV if there are no matching devices for the driver.
I originally had the platform_driver_probe() patch before this patch,
and thus the comment above made sense. Now the patch is after this
patch, so the comment is not exactly right until the probe patch is also
applied.
Oh okay. But the comment after the patch set still says It's ok if the
output-driver register fails., we could change it to It's ok if the
output-driver probe fails.
The point with platform_driver_probe() is that it can be used with
non-removable devices which are created at boot time, like the DSS
components. With platform_driver_probe() the probe function is called
only at that one time, and never afterwards. So probe can be in __init
section, and thrown away after init.
So platform_driver_probe() is like a driver_register() + probe().
Okay, in our case, all the devices are created at boot time, and if
omapdss were a module, the probes would have been thrown away after
module_init(), right?
One side effect of using platform_driver_probe() is that it returns
ENODEV is there are no devices. In a simple module, the error can be
then returned from module_init, thus causing the whole module to be
unloaded. Our case is a bit more complex as we have multiple drivers in
the same module.
A downside with that is that we don't really know if the ENODEV error
happened because there were no devices (which is ok), or if it came from
probe function (which is not so ok). However, I thought that it doesn't
matter if an output driver has failed. We can still continue with the
other output drivers just fine.
If we ensure that none of our probes return ENODEV(even though it may
make sense to return it if a func within probe fails), we could
differentiate between the 2 cases, right?
Actually, there is a small problem. If, for example, DSI driver fails to
load, and DPI driver tries to use DSI PLL...
If we could differentiate between an error occuring because the device
doesn't exist and an error occuring because the probe failed, we could
bail out if any of the probes fail, right?
Archit
Tomi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/21] OMAPDSS: handle output-driver reg/unreg more dynamically
On Thu, 2012-03-08 at 14:52 +0530, Archit Taneja wrote:
On Thursday 08 March 2012 02:16 PM, Tomi Valkeinen wrote:
On Thu, 2012-03-08 at 14:04 +0530, Archit Taneja wrote:
On Wednesday 07 March 2012 06:14 PM, Tomi Valkeinen wrote:
- r = hdmi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize hdmi\n);
- goto err_hdmi;
+ /*
+ * It's ok if the output-driver register fails. It happens, for example,
+ * when there is no output-device (e.g. SDI for OMAP4).
+ */
Suppose we do a omap2plus_defconfig, CONFIG_OMAP2_DSS_SDI would be
selected, and sdi.c would be built, if we boot on OMAP4, why would a sdi
driver register cause a failure? Wouldn't the sdi driver just get
registered, and wait till eternity for the corresponding sdi platform
device to get registered?
No. Well, yes.
Currently we use platform_driver_register() to register the drivers, and
it does just what you described. But a few patches later I change
platform_driver_register() to platform_driver_probe(), which will return
ENODEV if there are no matching devices for the driver.
I originally had the platform_driver_probe() patch before this patch,
and thus the comment above made sense. Now the patch is after this
patch, so the comment is not exactly right until the probe patch is also
applied.
Oh okay. But the comment after the patch set still says It's ok if the
output-driver register fails., we could change it to It's ok if the
output-driver probe fails.
Well, I guess this goes into nitpicking area, but if there are no
devices, probe is not called at all. So I think it's the driver register
that fails in that case. If there is a device, and it is probed, and
that fails, then it's probe which fails.
The point with platform_driver_probe() is that it can be used with
non-removable devices which are created at boot time, like the DSS
components. With platform_driver_probe() the probe function is called
only at that one time, and never afterwards. So probe can be in __init
section, and thrown away after init.
So platform_driver_probe() is like a driver_register() + probe().
Yes. Well, when platform_driver_register() is called, and the devices
are already present, it will call the probe also. So in that sense they
are similar. The difference is that for platform_driver_register() the
probe pointer must be in the driver struct, and it stays there even
after init. For platform_driver_probe(), the probe pointer is given as
an argument to the function, and thus it's not stored anywhere and can
be thrown away afterwards.
Okay, in our case, all the devices are created at boot time, and if
omapdss were a module, the probes would have been thrown away after
module_init(), right?
Yes. If omapdss is a module, the functions marked with __init are
discarded after the module_init is done. If omapdss is built-in, the
__init funcs are thrown away after the kernel's init done.
One side effect of using platform_driver_probe() is that it returns
ENODEV is there are no devices. In a simple module, the error can be
then returned from module_init, thus causing the whole module to be
unloaded. Our case is a bit more complex as we have multiple drivers in
the same module.
A downside with that is that we don't really know if the ENODEV error
happened because there were no devices (which is ok), or if it came from
probe function (which is not so ok). However, I thought that it doesn't
matter if an output driver has failed. We can still continue with the
other output drivers just fine.
If we ensure that none of our probes return ENODEV(even though it may
make sense to return it if a func within probe fails), we could
differentiate between the 2 cases, right?
True, I thought about that. But we can never be sure that the functions
called by the probe (clk_get, or whatever) won't return ENODEV. Of
course, we could check what they return, and change the error to
something else, but I'm not sure if that's good either.
Actually, there is a small problem. If, for example, DSI driver fails to
load, and DPI driver tries to use DSI PLL...
If we could differentiate between an error occuring because the device
doesn't exist and an error occuring because the probe failed, we could
bail out if any of the probes fail, right?
Yes, but it feels a bit hackish to try to use the error as I pointed out
above. So I'd rather go the other way: the drivers should somehow
register the stuff they offer, so for example when the DSI1 is probed,
it should register the DSI1 PLL to dss core. And DPI would have to ask
for the DSI1 PLL from dss core.
That, of course, is not a trivial change, so for the moment this stuff
is slightly broken in error cases. Perhaps we could figure out some kind
of clean hack for that...
Alternatively, if the platform driver code was changed to tell us
clearly if it was the probe that failed or if there just weren't any
Re: [PATCH 16/21] OMAPDSS: handle output-driver reg/unreg more dynamically
On Thursday 08 March 2012 03:04 PM, Tomi Valkeinen wrote: On Thu, 2012-03-08 at 14:52 +0530, Archit Taneja wrote: On Thursday 08 March 2012 02:16 PM, Tomi Valkeinen wrote: On Thu, 2012-03-08 at 14:04 +0530, Archit Taneja wrote: snip Oh okay. But the comment after the patch set still says It's ok if the output-driver register fails., we could change it to It's ok if the output-driver probe fails. Well, I guess this goes into nitpicking area, but if there are no devices, probe is not called at all. So I think it's the driver register that fails in that case. If there is a device, and it is probed, and that fails, then it's probe which fails. Yes, that's fair enough I guess. snip If we ensure that none of our probes return ENODEV(even though it may make sense to return it if a func within probe fails), we could differentiate between the 2 cases, right? True, I thought about that. But we can never be sure that the functions called by the probe (clk_get, or whatever) won't return ENODEV. Of course, we could check what they return, and change the error to something else, but I'm not sure if that's good either. That's true. snip Alternatively, if the platform driver code was changed to tell us clearly if it was the probe that failed or if there just weren't any devices, we could also use that. Yes, I wonder how that could be done. Archit Tomi -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 16/21] OMAPDSS: handle output-driver reg/unreg more dynamically
Initialize and uninitialize the output drivers by using arrays of
pointers to the init/uninit functions. This simplifies the code
slightly.
Signed-off-by: Tomi Valkeinen tomi.valkei...@ti.com
---
drivers/video/omap2/dss/core.c | 111 +---
drivers/video/omap2/dss/dss.h | 41 ---
2 files changed, 59 insertions(+), 93 deletions(-)
diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index 654962a..ac4f2cb 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -503,10 +503,54 @@ static int omap_dss_bus_register(void)
}
/* INIT */
+static int (*dss_output_drv_reg_funcs[])(void) __initdata = {
+#ifdef CONFIG_OMAP2_DSS_DPI
+ dpi_init_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_SDI
+ sdi_init_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_RFBI
+ rfbi_init_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_VENC
+ venc_init_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_DSI
+ dsi_init_platform_driver,
+#endif
+#ifdef CONFIG_OMAP4_DSS_HDMI
+ hdmi_init_platform_driver,
+#endif
+};
+
+static void (*dss_output_drv_unreg_funcs[])(void) __exitdata = {
+#ifdef CONFIG_OMAP2_DSS_DPI
+ dpi_uninit_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_SDI
+ sdi_uninit_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_RFBI
+ rfbi_uninit_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_VENC
+ venc_uninit_platform_driver,
+#endif
+#ifdef CONFIG_OMAP2_DSS_DSI
+ dsi_uninit_platform_driver,
+#endif
+#ifdef CONFIG_OMAP4_DSS_HDMI
+ hdmi_uninit_platform_driver,
+#endif
+};
+
+static bool dss_output_drv_loaded[ARRAY_SIZE(dss_output_drv_reg_funcs)];
static int __init omap_dss_register_drivers(void)
{
int r;
+ int i;
r = platform_driver_probe(omap_dss_driver, omap_dss_probe);
if (r)
@@ -524,56 +568,18 @@ static int __init omap_dss_register_drivers(void)
goto err_dispc;
}
- r = dpi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize dpi platform driver\n);
- goto err_dpi;
- }
-
- r = sdi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize sdi platform driver\n);
- goto err_sdi;
- }
-
- r = rfbi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize rfbi platform driver\n);
- goto err_rfbi;
- }
-
- r = venc_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize venc platform driver\n);
- goto err_venc;
- }
-
- r = dsi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize DSI platform driver\n);
- goto err_dsi;
- }
-
- r = hdmi_init_platform_driver();
- if (r) {
- DSSERR(Failed to initialize hdmi\n);
- goto err_hdmi;
+ /*
+* It's ok if the output-driver register fails. It happens, for example,
+* when there is no output-device (e.g. SDI for OMAP4).
+*/
+ for (i = 0; i ARRAY_SIZE(dss_output_drv_reg_funcs); ++i) {
+ r = dss_output_drv_reg_funcs[i]();
+ if (r == 0)
+ dss_output_drv_loaded[i] = true;
}
return 0;
-err_hdmi:
- dsi_uninit_platform_driver();
-err_dsi:
- venc_uninit_platform_driver();
-err_venc:
- rfbi_uninit_platform_driver();
-err_rfbi:
- sdi_uninit_platform_driver();
-err_sdi:
- dpi_uninit_platform_driver();
-err_dpi:
- dispc_uninit_platform_driver();
err_dispc:
dss_uninit_platform_driver();
err_dss:
@@ -584,12 +590,13 @@ err_dss:
static void __exit omap_dss_unregister_drivers(void)
{
- hdmi_uninit_platform_driver();
- dsi_uninit_platform_driver();
- venc_uninit_platform_driver();
- rfbi_uninit_platform_driver();
- sdi_uninit_platform_driver();
- dpi_uninit_platform_driver();
+ int i;
+
+ for (i = 0; i ARRAY_SIZE(dss_output_drv_unreg_funcs); ++i) {
+ if (dss_output_drv_loaded[i])
+ dss_output_drv_unreg_funcs[i]();
+ }
+
dispc_uninit_platform_driver();
dss_uninit_platform_driver();
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 24aadde..af7bed1 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -265,14 +265,9 @@ int dss_calc_clock_div(bool is_tft, unsigned long req_pck,
struct dispc_clock_info *dispc_cinfo);
/* SDI */
-#ifdef CONFIG_OMAP2_DSS_SDI
int sdi_init_platform_driver(void);
void sdi_uninit_platform_driver(void);
int sdi_init_display(struct omap_dss_device *display);
-#else
-static inline int sdi_init_platform_driver(void) { return 0; }
-static inline void sdi_uninit_platform_driver(void) { }
-#endif
/*
