| Message ID | 20220504114021.33265-8-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: bridge: Add Samsung MIPI DSIM bridge | expand |
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
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 --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");
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(+)