Re: [PATCH 4/6] drm/omapdrm: Remove fbdev from struct omap_drm_private

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 30/03/2023 11:32, Thomas Zimmermann wrote:
> The DRM device stores a pointer to the fbdev helper. Remove struct
> omap_drm_private.fbdev, which contains the same value. No functional
> changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
>   drivers/gpu/drm/omapdrm/omap_debugfs.c | 6 +++---
>   drivers/gpu/drm/omapdrm/omap_drv.c     | 1 +
>   drivers/gpu/drm/omapdrm/omap_drv.h     | 3 ---
>   drivers/gpu/drm/omapdrm/omap_fbdev.c   | 7 ++-----
>   4 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_debugfs.c b/drivers/gpu/drm/omapdrm/omap_debugfs.c
> index bfb2ccb40bd1..a3d470468e5b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_debugfs.c
> +++ b/drivers/gpu/drm/omapdrm/omap_debugfs.c
> @@ -47,15 +47,15 @@ static int fb_show(struct seq_file *m, void *arg)
>   {
>   	struct drm_info_node *node = (struct drm_info_node *) m->private;
>   	struct drm_device *dev = node->minor->dev;
> -	struct omap_drm_private *priv = dev->dev_private;
> +	struct drm_fb_helper *helper = dev->fb_helper;
>   	struct drm_framebuffer *fb;
>   
>   	seq_printf(m, "fbcon ");
> -	omap_framebuffer_describe(priv->fbdev->fb, m);
> +	omap_framebuffer_describe(helper->fb, m);
>   
>   	mutex_lock(&dev->mode_config.fb_lock);
>   	list_for_each_entry(fb, &dev->mode_config.fb_list, head) {
> -		if (fb == priv->fbdev->fb)
> +		if (fb == helper->fb)
>   			continue;
>   
>   		seq_printf(m, "user ");
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index fb403b44769c..6a2f446c960f 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -25,6 +25,7 @@
>   
>   #include "omap_dmm_tiler.h"
>   #include "omap_drv.h"
> +#include "omap_fbdev.h"
>   
>   #define DRIVER_NAME		MODULE_NAME
>   #define DRIVER_DESC		"OMAP DRM"
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index 825960fd3ea9..4c7217b35f6b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -21,7 +21,6 @@
>   #include "omap_crtc.h"
>   #include "omap_encoder.h"
>   #include "omap_fb.h"
> -#include "omap_fbdev.h"
>   #include "omap_gem.h"
>   #include "omap_irq.h"
>   #include "omap_plane.h"
> @@ -77,8 +76,6 @@ struct omap_drm_private {
>   
>   	struct drm_private_obj glob_obj;
>   
> -	struct drm_fb_helper *fbdev;
> -
>   	struct workqueue_struct *wq;
>   
>   	/* lock for obj_list below */
> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> index d04a20f95e3d..79e417b391bf 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -250,8 +250,6 @@ void omap_fbdev_init(struct drm_device *dev)
>   	if (ret)
>   		goto fini;
>   
> -	priv->fbdev = helper;
> -
>   	return;
>   
>   fini:
> @@ -265,8 +263,7 @@ void omap_fbdev_init(struct drm_device *dev)
>   
>   void omap_fbdev_fini(struct drm_device *dev)
>   {
> -	struct omap_drm_private *priv = dev->dev_private;
> -	struct drm_fb_helper *helper = priv->fbdev;
> +	struct drm_fb_helper *helper = dev->fb_helper;
>   	struct drm_framebuffer *fb;
>   	struct drm_gem_object *bo;
>   	struct omap_fbdev *fbdev;
> @@ -297,5 +294,5 @@ void omap_fbdev_fini(struct drm_device *dev)
>   	drm_fb_helper_unprepare(helper);
>   	kfree(fbdev);
>   
> -	priv->fbdev = NULL;
> +	dev->fb_helper = NULL;

Is this line needed anymore? As you dropped the priv->fbdev assignment 
in omap_fbdev_init(), it would look symmetrical to also drop this one. 
I'm sure it doesn't hurt, just wondering if this is something drivers 
are supposed to do, or is this just an extra line in the driver.

In either case:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>

 Tomi