diff mbox series

[v2,07/12] drm: bridge: samsung-dsim: Add module init, exit

Message ID 20220504114021.33265-8-jagan@amarulasolutions.com
State New
Headers
Series drm: bridge: Add Samsung MIPI DSIM bridge |

Commit Message

Jagan Teki May 4, 2022, 11:40 a.m. UTC 
Add module init and exit functions for the bridge to register
and unregister dsi_driver.

Exynos drm driver stack will register the platform_driver separately
in the common of it's exynos_drm_drv.c including dsi_driver.

Register again would return -EBUSY, so return 0 for such cases as
dsi_driver is already registered.

v2, v1:
* none

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Marek Szyprowski May 9, 2022, 12:05 p.m. UTC | #1 
On 04.05.2022 13:40, Jagan Teki wrote:
> Add module init and exit functions for the bridge to register
> and unregister dsi_driver.
>
> Exynos drm driver stack will register the platform_driver separately
> in the common of it's exynos_drm_drv.c including dsi_driver.
>
> Register again would return -EBUSY, so return 0 for such cases as
> dsi_driver is already registered.
>
> v2, v1:
> * none
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 8f9ae16d45bc..b618e52d0ee3 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1740,6 +1740,28 @@ struct platform_driver dsi_driver = {
>   	},
>   };
>   
> +static int __init samsung_mipi_dsim_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&dsi_driver);
> +
> +	/**
> +	 * Exynos drm driver stack will register the platform_driver
> +	 * separately in the common of it's exynos_drm_drv.c including
> +	 * dsi_driver. Register again would return -EBUSY, so return 0
> +	 * for such cases as dsi_driver is already registered.
> +	 */
> +	return ret == -EBUSY ? 0 : ret;
> +}
> +module_init(samsung_mipi_dsim_init);

I've just noticed this. The above approach is really a bad pattern: 
registering the same driver 2 times and relying on the error.

This gives the following error on Exynos boards:

Error: Driver 'samsung-dsim' is already registered, aborting...

which a bit misleading, because it is assumed that this will be ok.

This will also break if one compile it as modules, because the driver 
operation will depend on the order of module loading (and Exynos DSI 
won't be able to load as a second 'driver').

However the most important issue with such pattern is lack of 
multi-platform support (used usually by generic distros). One would not 
be able to compile a kernel with both Exynos and IMX support built-in. 
New drivers should really follow the multi-platform friendly patterns.


> +
> +static void __exit samsung_mipi_dsim_exit(void)
> +{
> +	platform_driver_unregister(&dsi_driver);
> +}
> +module_exit(samsung_mipi_dsim_exit);
> +
>   MODULE_AUTHOR("Jagan Teki <jagan@amarulasolutions.com>");
>   MODULE_DESCRIPTION("Samsung MIPI DSIM controller bridge");
>   MODULE_LICENSE("GPL");

Best regards
Jagan Teki July 19, 2022, 12:11 p.m. UTC | #2 
On Mon, May 9, 2022 at 5:35 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 04.05.2022 13:40, Jagan Teki wrote:
> > Add module init and exit functions for the bridge to register
> > and unregister dsi_driver.
> >
> > Exynos drm driver stack will register the platform_driver separately
> > in the common of it's exynos_drm_drv.c including dsi_driver.
> >
> > Register again would return -EBUSY, so return 0 for such cases as
> > dsi_driver is already registered.
> >
> > v2, v1:
> > * none
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >   drivers/gpu/drm/bridge/samsung-dsim.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 8f9ae16d45bc..b618e52d0ee3 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1740,6 +1740,28 @@ struct platform_driver dsi_driver = {
> >       },
> >   };
> >
> > +static int __init samsung_mipi_dsim_init(void)
> > +{
> > +     int ret;
> > +
> > +     ret = platform_driver_register(&dsi_driver);
> > +
> > +     /**
> > +      * Exynos drm driver stack will register the platform_driver
> > +      * separately in the common of it's exynos_drm_drv.c including
> > +      * dsi_driver. Register again would return -EBUSY, so return 0
> > +      * for such cases as dsi_driver is already registered.
> > +      */
> > +     return ret == -EBUSY ? 0 : ret;
> > +}
> > +module_init(samsung_mipi_dsim_init);
>
> I've just noticed this. The above approach is really a bad pattern:
> registering the same driver 2 times and relying on the error.

If it tries to register 2nd time, then it returns EBUSY so we are
returning 0 for that case. not sure why it registers 2nd time again.

Jagan.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 8f9ae16d45bc..b618e52d0ee3 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1740,6 +1740,28 @@  struct platform_driver dsi_driver = {
 	},
 };
 
+static int __init samsung_mipi_dsim_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&dsi_driver);
+
+	/**
+	 * Exynos drm driver stack will register the platform_driver
+	 * separately in the common of it's exynos_drm_drv.c including
+	 * dsi_driver. Register again would return -EBUSY, so return 0
+	 * for such cases as dsi_driver is already registered.
+	 */
+	return ret == -EBUSY ? 0 : ret;
+}
+module_init(samsung_mipi_dsim_init);
+
+static void __exit samsung_mipi_dsim_exit(void)
+{
+	platform_driver_unregister(&dsi_driver);
+}
+module_exit(samsung_mipi_dsim_exit);
+
 MODULE_AUTHOR("Jagan Teki <jagan@amarulasolutions.com>");
 MODULE_DESCRIPTION("Samsung MIPI DSIM controller bridge");
 MODULE_LICENSE("GPL");