| Message ID | 1518428694-18018-3-git-send-email-tomi.valkeinen@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/omap: misc patches | expand |
Hi Tomi, Thank you for the patch. On Monday, 12 February 2018 11:44:32 EET Tomi Valkeinen wrote: > omap_fbdev_init() and omap_fbdev_free() use priv->fbdev directly. > However, omap_fbdev_init() returns the fbdev, and omap_drv.c also > assigns the return value to priv->fbdev. This is slightly confusing. > > Clean this up by removing the omap_fbdev_init() return value, as we > don't care whether fbdev init succeeded or not. Also change omap_drv.c > to call omap_fbdev_init() always, and omap_fbdev_init() does the check > if fbdev was initialized. I assume you mean omap_fbdev_free() for the last two occurrences (and on a side note I wonder whether we should rename it to omap_fbdev_cleanup() or omap_fbdev_fini() for symmetry with omap_fbdev_init()). > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++++----- > drivers/gpu/drm/omapdrm/omap_fbdev.c | 10 +++++----- > drivers/gpu/drm/omapdrm/omap_fbdev.h | 5 ++--- > 3 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c > b/drivers/gpu/drm/omapdrm/omap_drv.c index dd68b2556f5b..485684c637ff > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.c > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > @@ -584,7 +584,7 @@ static int pdev_probe(struct platform_device *pdev) > for (i = 0; i < priv->num_crtcs; i++) > drm_crtc_vblank_off(priv->crtcs[i]); > > - priv->fbdev = omap_fbdev_init(ddev); > + omap_fbdev_init(ddev); > > drm_kms_helper_poll_init(ddev); > omap_modeset_enable_external_hpd(); > @@ -602,8 +602,8 @@ static int pdev_probe(struct platform_device *pdev) > err_cleanup_helpers: > omap_modeset_disable_external_hpd(); > drm_kms_helper_poll_fini(ddev); > - if (priv->fbdev) > - omap_fbdev_free(ddev); > + > + omap_fbdev_free(ddev); > err_cleanup_modeset: > drm_mode_config_cleanup(ddev); > omap_drm_irq_uninstall(ddev); > @@ -632,8 +632,7 @@ static int pdev_remove(struct platform_device *pdev) > omap_modeset_disable_external_hpd(); > drm_kms_helper_poll_fini(ddev); > > - if (priv->fbdev) > - omap_fbdev_free(ddev); > + omap_fbdev_free(ddev); > > drm_atomic_helper_shutdown(ddev); > > diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c > b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 632ebcf2165f..30ce3d562414 > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c > +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c > @@ -242,7 +242,7 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi) > } > > /* initialize fbdev helper */ > -struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) > +void omap_fbdev_init(struct drm_device *dev) > { > struct omap_drm_private *priv = dev->dev_private; > struct omap_fbdev *fbdev = NULL; > @@ -275,7 +275,7 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device > *dev) > > priv->fbdev = helper; > > - return helper; > + return; > > fini: > drm_fb_helper_fini(helper); > @@ -283,9 +283,6 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device > *dev) kfree(fbdev); > > dev_warn(dev->dev, "omap_fbdev_init failed\n"); While at it I'd remove the error message when drm_fb_helper_init() fails, it duplicates this one. > - /* well, limp along without an fbdev.. maybe X11 will work? */ > - > - return NULL; > } > > void omap_fbdev_free(struct drm_device *dev) > @@ -296,6 +293,9 @@ void omap_fbdev_free(struct drm_device *dev) > > DBG(); > > + if (!priv->fbdev) > + return; > + I'd either write this as if (!helper) and replace the other occurrences of priv->fbdev below, or replace helper with priv->fbdev in the whole function. > drm_fb_helper_unregister_fbi(helper); > > drm_fb_helper_fini(helper); > diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.h > b/drivers/gpu/drm/omapdrm/omap_fbdev.h index 1f5ba0996a1a..3bd6ea661a18 > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_fbdev.h > +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.h > @@ -24,12 +24,11 @@ struct drm_device; > struct drm_fb_helper; > > #ifdef CONFIG_DRM_FBDEV_EMULATION > -struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev); > +void omap_fbdev_init(struct drm_device *dev); > void omap_fbdev_free(struct drm_device *dev); > #else > -static inline struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) > +static inline void omap_fbdev_init(struct drm_device *dev) > { > - return NULL; > } > static inline void omap_fbdev_free(struct drm_device *dev) > {
On 27/02/18 13:38, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Monday, 12 February 2018 11:44:32 EET Tomi Valkeinen wrote: >> omap_fbdev_init() and omap_fbdev_free() use priv->fbdev directly. >> However, omap_fbdev_init() returns the fbdev, and omap_drv.c also >> assigns the return value to priv->fbdev. This is slightly confusing. >> >> Clean this up by removing the omap_fbdev_init() return value, as we >> don't care whether fbdev init succeeded or not. Also change omap_drv.c >> to call omap_fbdev_init() always, and omap_fbdev_init() does the check >> if fbdev was initialized. > > I assume you mean omap_fbdev_free() for the last two occurrences (and on a Right, true. > side note I wonder whether we should rename it to omap_fbdev_cleanup() or > omap_fbdev_fini() for symmetry with omap_fbdev_init()). Yep, I think that makes sense. >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> --- >> drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++++----- >> drivers/gpu/drm/omapdrm/omap_fbdev.c | 10 +++++----- >> drivers/gpu/drm/omapdrm/omap_fbdev.h | 5 ++--- >> 3 files changed, 11 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c >> b/drivers/gpu/drm/omapdrm/omap_drv.c index dd68b2556f5b..485684c637ff >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_drv.c >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c >> @@ -584,7 +584,7 @@ static int pdev_probe(struct platform_device *pdev) >> for (i = 0; i < priv->num_crtcs; i++) >> drm_crtc_vblank_off(priv->crtcs[i]); >> >> - priv->fbdev = omap_fbdev_init(ddev); >> + omap_fbdev_init(ddev); >> >> drm_kms_helper_poll_init(ddev); >> omap_modeset_enable_external_hpd(); >> @@ -602,8 +602,8 @@ static int pdev_probe(struct platform_device *pdev) >> err_cleanup_helpers: >> omap_modeset_disable_external_hpd(); >> drm_kms_helper_poll_fini(ddev); >> - if (priv->fbdev) >> - omap_fbdev_free(ddev); >> + >> + omap_fbdev_free(ddev); >> err_cleanup_modeset: >> drm_mode_config_cleanup(ddev); >> omap_drm_irq_uninstall(ddev); >> @@ -632,8 +632,7 @@ static int pdev_remove(struct platform_device *pdev) >> omap_modeset_disable_external_hpd(); >> drm_kms_helper_poll_fini(ddev); >> >> - if (priv->fbdev) >> - omap_fbdev_free(ddev); >> + omap_fbdev_free(ddev); >> >> drm_atomic_helper_shutdown(ddev); >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c >> b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 632ebcf2165f..30ce3d562414 >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c >> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c >> @@ -242,7 +242,7 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi) >> } >> >> /* initialize fbdev helper */ >> -struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) >> +void omap_fbdev_init(struct drm_device *dev) >> { >> struct omap_drm_private *priv = dev->dev_private; >> struct omap_fbdev *fbdev = NULL; >> @@ -275,7 +275,7 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device >> *dev) >> >> priv->fbdev = helper; >> >> - return helper; >> + return; >> >> fini: >> drm_fb_helper_fini(helper); >> @@ -283,9 +283,6 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device >> *dev) kfree(fbdev); >> >> dev_warn(dev->dev, "omap_fbdev_init failed\n"); > > While at it I'd remove the error message when drm_fb_helper_init() fails, it > duplicates this one. Ok. >> - /* well, limp along without an fbdev.. maybe X11 will work? */ >> - >> - return NULL; >> } >> >> void omap_fbdev_free(struct drm_device *dev) >> @@ -296,6 +293,9 @@ void omap_fbdev_free(struct drm_device *dev) >> >> DBG(); >> >> + if (!priv->fbdev) >> + return; >> + > > I'd either write this as if (!helper) and replace the other occurrences of > priv->fbdev below, or replace helper with priv->fbdev in the whole function. Ok. Tomi
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index dd68b2556f5b..485684c637ff 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -584,7 +584,7 @@ static int pdev_probe(struct platform_device *pdev) for (i = 0; i < priv->num_crtcs; i++) drm_crtc_vblank_off(priv->crtcs[i]); - priv->fbdev = omap_fbdev_init(ddev); + omap_fbdev_init(ddev); drm_kms_helper_poll_init(ddev); omap_modeset_enable_external_hpd(); @@ -602,8 +602,8 @@ static int pdev_probe(struct platform_device *pdev) err_cleanup_helpers: omap_modeset_disable_external_hpd(); drm_kms_helper_poll_fini(ddev); - if (priv->fbdev) - omap_fbdev_free(ddev); + + omap_fbdev_free(ddev); err_cleanup_modeset: drm_mode_config_cleanup(ddev); omap_drm_irq_uninstall(ddev); @@ -632,8 +632,7 @@ static int pdev_remove(struct platform_device *pdev) omap_modeset_disable_external_hpd(); drm_kms_helper_poll_fini(ddev); - if (priv->fbdev) - omap_fbdev_free(ddev); + omap_fbdev_free(ddev); drm_atomic_helper_shutdown(ddev); diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 632ebcf2165f..30ce3d562414 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -242,7 +242,7 @@ static struct drm_fb_helper *get_fb(struct fb_info *fbi) } /* initialize fbdev helper */ -struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) +void omap_fbdev_init(struct drm_device *dev) { struct omap_drm_private *priv = dev->dev_private; struct omap_fbdev *fbdev = NULL; @@ -275,7 +275,7 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) priv->fbdev = helper; - return helper; + return; fini: drm_fb_helper_fini(helper); @@ -283,9 +283,6 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) kfree(fbdev); dev_warn(dev->dev, "omap_fbdev_init failed\n"); - /* well, limp along without an fbdev.. maybe X11 will work? */ - - return NULL; } void omap_fbdev_free(struct drm_device *dev) @@ -296,6 +293,9 @@ void omap_fbdev_free(struct drm_device *dev) DBG(); + if (!priv->fbdev) + return; + drm_fb_helper_unregister_fbi(helper); drm_fb_helper_fini(helper); diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.h b/drivers/gpu/drm/omapdrm/omap_fbdev.h index 1f5ba0996a1a..3bd6ea661a18 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.h +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.h @@ -24,12 +24,11 @@ struct drm_device; struct drm_fb_helper; #ifdef CONFIG_DRM_FBDEV_EMULATION -struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev); +void omap_fbdev_init(struct drm_device *dev); void omap_fbdev_free(struct drm_device *dev); #else -static inline struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) +static inline void omap_fbdev_init(struct drm_device *dev) { - return NULL; } static inline void omap_fbdev_free(struct drm_device *dev) {
omap_fbdev_init() and omap_fbdev_free() use priv->fbdev directly. However, omap_fbdev_init() returns the fbdev, and omap_drv.c also assigns the return value to priv->fbdev. This is slightly confusing. Clean this up by removing the omap_fbdev_init() return value, as we don't care whether fbdev init succeeded or not. Also change omap_drv.c to call omap_fbdev_init() always, and omap_fbdev_init() does the check if fbdev was initialized. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++++----- drivers/gpu/drm/omapdrm/omap_fbdev.c | 10 +++++----- drivers/gpu/drm/omapdrm/omap_fbdev.h | 5 ++--- 3 files changed, 11 insertions(+), 13 deletions(-)