Re: [PATCH] drm: bridge: adv7511: Add check for mipi_dsi_driver_register
On Wed, Jun 01, 2022 at 07:12:55PM +0800, Jiasheng Jiang wrote: > On Wed, Jun 01, 2022 at 05:15:37PM +0800, Laurent Pinchart wrote: > >> Well, as far as I am concerned, the adv7511_exit() in the same file has > >> already dealt with the issue. > >> Therefore, it might not be necessary to add another > >> mipi_dsi_driver_unregister(). > > > > The issue is that adv7511_exit() is not called if adv7511_init() fails. > > Sorry, I can not find the caller of adv7511_init(). > Please give me more detail. It's called by the kernel when the module is unloaded (module_exit). It's not what Laurent is saying though. His point is that in adv7511, if i2c_add_driver fails, you'll still need to call mipi_dsi_driver_unregister to clean things up. Maxime
Re: Re: [PATCH] drm: bridge: adv7511: Add check for mipi_dsi_driver_register
On Wed, Jun 01, 2022 at 05:15:37PM +0800, Laurent Pinchart wrote: >> Well, as far as I am concerned, the adv7511_exit() in the same file has >> already dealt with the issue. >> Therefore, it might not be necessary to add another >> mipi_dsi_driver_unregister(). > > The issue is that adv7511_exit() is not called if adv7511_init() fails. Sorry, I can not find the caller of adv7511_init(). Please give me more detail. Thanks, Jiang
Re: Re: [PATCH] drm: bridge: adv7511: Add check for mipi_dsi_driver_register
On Wed, Jun 01, 2022 at 04:45:01PM +0800, Jiasheng Jiang wrote: > On Wed, Jun 01, 2022 at 02:52:00PM +0800, Laurent Pinchart wrote: > >> static int __init adv7511_init(void) > >> { > >> - if (IS_ENABLED(CONFIG_DRM_MIPI_DSI)) > >> - mipi_dsi_driver_register(_dsi_driver); > >> + int ret; > >> + > >> + if (IS_ENABLED(CONFIG_DRM_MIPI_DSI)) { > >> + ret = mipi_dsi_driver_register(_dsi_driver); > >> + if (ret) > >> + return ret; > >> + } > >> > >>return i2c_add_driver(_driver); > > > > While at it, should this then call mipi_dsi_driver_unregister() on > > failure ? > > Well, as far as I am concerned, the adv7511_exit() in the same file has > already dealt with the issue. > Therefore, it might not be necessary to add another > mipi_dsi_driver_unregister(). The issue is that adv7511_exit() is not called if adv7511_init() fails. -- Regards, Laurent Pinchart
Re: Re: [PATCH] drm: bridge: adv7511: Add check for mipi_dsi_driver_register
On Wed, Jun 01, 2022 at 02:52:00PM +0800, Laurent Pinchart wrote: >> static int __init adv7511_init(void) >> { >> -if (IS_ENABLED(CONFIG_DRM_MIPI_DSI)) >> -mipi_dsi_driver_register(_dsi_driver); >> +int ret; >> + >> +if (IS_ENABLED(CONFIG_DRM_MIPI_DSI)) { >> +ret = mipi_dsi_driver_register(_dsi_driver); >> +if (ret) >> +return ret; >> +} >> >> return i2c_add_driver(_driver); > > While at it, should this then call mipi_dsi_driver_unregister() on > failure ? Well, as far as I am concerned, the adv7511_exit() in the same file has already dealt with the issue. Therefore, it might not be necessary to add another mipi_dsi_driver_unregister(). Thanks, Jiang
Re: [PATCH] drm: bridge: adv7511: Add check for mipi_dsi_driver_register
Hi Jiasheng, Thank you for the patch. On Wed, Jun 01, 2022 at 10:48:22AM +0800, Jiasheng Jiang wrote: > As mipi_dsi_driver_register could return error if fails, > it should be better to check the return value and return error > if fails. > > Fixes: 1e4d58cd7f88 ("drm/bridge: adv7533: Create a MIPI DSI device") > Signed-off-by: Jiasheng Jiang > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 5bb9300040dd..795855b41eb2 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -1392,8 +1392,13 @@ static struct i2c_driver adv7511_driver = { > > static int __init adv7511_init(void) > { > - if (IS_ENABLED(CONFIG_DRM_MIPI_DSI)) > - mipi_dsi_driver_register(_dsi_driver); > + int ret; > + > + if (IS_ENABLED(CONFIG_DRM_MIPI_DSI)) { > + ret = mipi_dsi_driver_register(_dsi_driver); > + if (ret) > + return ret; > + } > > return i2c_add_driver(_driver); While at it, should this then call mipi_dsi_driver_unregister() on failure ? > } > -- > 2.25.1 > -- Regards, Laurent Pinchart
[PATCH] drm: bridge: adv7511: Add check for mipi_dsi_driver_register
As mipi_dsi_driver_register could return error if fails, it should be better to check the return value and return error if fails. Fixes: 1e4d58cd7f88 ("drm/bridge: adv7533: Create a MIPI DSI device") Signed-off-by: Jiasheng Jiang --- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 5bb9300040dd..795855b41eb2 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1392,8 +1392,13 @@ static struct i2c_driver adv7511_driver = { static int __init adv7511_init(void) { - if (IS_ENABLED(CONFIG_DRM_MIPI_DSI)) - mipi_dsi_driver_register(_dsi_driver); + int ret; + + if (IS_ENABLED(CONFIG_DRM_MIPI_DSI)) { + ret = mipi_dsi_driver_register(_dsi_driver); + if (ret) + return ret; + } return i2c_add_driver(_driver); } -- 2.25.1