2019-05-24 08:56:22

by Daniel Vetter

[permalink] [raw]
Subject: [PATCH 11/33] fbdev/sh_mobile: remove sh_mobile_lcdc_display_notify

It's dead code, and removing it avoids me having to understand
what it's doing with lock_fb_info.

Signed-off-by: Daniel Vetter <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Daniel Vetter <[email protected]>
---
drivers/video/fbdev/sh_mobile_lcdcfb.c | 63 --------------------------
drivers/video/fbdev/sh_mobile_lcdcfb.h | 5 --
2 files changed, 68 deletions(-)

diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
index dc46be38c970..c5924f5e98c6 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
@@ -556,67 +556,6 @@ sh_mobile_lcdc_must_reconfigure(struct sh_mobile_lcdc_chan *ch,
static int sh_mobile_lcdc_check_var(struct fb_var_screeninfo *var,
struct fb_info *info);

-static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
- enum sh_mobile_lcdc_entity_event event,
- const struct fb_videomode *mode,
- const struct fb_monspecs *monspec)
-{
- struct fb_info *info = ch->info;
- struct fb_var_screeninfo var;
- int ret = 0;
-
- switch (event) {
- case SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT:
- /* HDMI plug in */
- console_lock();
- if (lock_fb_info(info)) {
-
-
- ch->display.width = monspec->max_x * 10;
- ch->display.height = monspec->max_y * 10;
-
- if (!sh_mobile_lcdc_must_reconfigure(ch, mode) &&
- info->state == FBINFO_STATE_RUNNING) {
- /* First activation with the default monitor.
- * Just turn on, if we run a resume here, the
- * logo disappears.
- */
- info->var.width = ch->display.width;
- info->var.height = ch->display.height;
- sh_mobile_lcdc_display_on(ch);
- } else {
- /* New monitor or have to wake up */
- fb_set_suspend(info, 0);
- }
-
-
- unlock_fb_info(info);
- }
- console_unlock();
- break;
-
- case SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT:
- /* HDMI disconnect */
- console_lock();
- if (lock_fb_info(info)) {
- fb_set_suspend(info, 1);
- unlock_fb_info(info);
- }
- console_unlock();
- break;
-
- case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
- /* Validate a proposed new mode */
- fb_videomode_to_var(&var, mode);
- var.bits_per_pixel = info->var.bits_per_pixel;
- var.grayscale = info->var.grayscale;
- ret = sh_mobile_lcdc_check_var(&var, info);
- break;
- }
-
- return ret;
-}
-
/* -----------------------------------------------------------------------------
* Format helpers
*/
@@ -2540,8 +2479,6 @@ sh_mobile_lcdc_channel_init(struct sh_mobile_lcdc_chan *ch)
unsigned int max_size;
unsigned int i;

- ch->notify = sh_mobile_lcdc_display_notify;
-
/* Validate the format. */
format = sh_mobile_format_info(cfg->fourcc);
if (format == NULL) {
diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.h b/drivers/video/fbdev/sh_mobile_lcdcfb.h
index b8e47a8bd8ab..589400372098 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.h
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.h
@@ -87,11 +87,6 @@ struct sh_mobile_lcdc_chan {
unsigned long base_addr_c;
unsigned int line_size;

- int (*notify)(struct sh_mobile_lcdc_chan *ch,
- enum sh_mobile_lcdc_entity_event event,
- const struct fb_videomode *mode,
- const struct fb_monspecs *monspec);
-
/* Backlight */
struct backlight_device *bl;
unsigned int bl_brightness;
--
2.20.1


2019-05-25 15:03:38

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH 11/33] fbdev/sh_mobile: remove sh_mobile_lcdc_display_notify

Hi Daniel

> It's dead code, and removing it avoids me having to understand
> what it's doing with lock_fb_info.

I pushed the series through my build tests which include the sh
architecture.

One error and one warning was triggered from sh_mobile_lcdcfb.c.
The rest was fine.

The patch below removed the sole user of
sh_mobile_lcdc_must_reconfigure() so this triggers a warning.

And I also get the following error:
drivers/video/fbdev/sh_mobile_lcdcfb.c: In function ‘sh_mobile_fb_reconfig’:
drivers/video/fbdev/sh_mobile_lcdcfb.c:1800:2: error: implicit declaration of function ‘fbcon_update_vcs’; did you mean ‘file_update_time’? [-Werror=implicit-function-declaration]
fbcon_update_vcs(info, true);
^~~~~~~~~~~~~~~~
file_update_time

I did not check but assume the error was triggered in patch 28 where
fbcon_update_vcs() in introduced.


Both are trivially fixed by appended patch.

Sam

diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
index bb1a610d0363..b8454424910d 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
@@ -15,6 +15,7 @@
#include <linux/ctype.h>
#include <linux/dma-mapping.h>
#include <linux/delay.h>
+#include <linux/fbcon.h>
#include <linux/gpio.h>
#include <linux/init.h>
#include <linux/interrupt.h>
@@ -533,25 +534,6 @@ static void sh_mobile_lcdc_display_off(struct sh_mobile_lcdc_chan *ch)
ch->tx_dev->ops->display_off(ch->tx_dev);
}

-static bool
-sh_mobile_lcdc_must_reconfigure(struct sh_mobile_lcdc_chan *ch,
- const struct fb_videomode *new_mode)
-{
- dev_dbg(ch->info->dev, "Old %ux%u, new %ux%u\n",
- ch->display.mode.xres, ch->display.mode.yres,
- new_mode->xres, new_mode->yres);
-
- /* It can be a different monitor with an equal video-mode */
- if (fb_mode_is_equal(&ch->display.mode, new_mode))
- return false;
-
- dev_dbg(ch->info->dev, "Switching %u -> %u lines\n",
- ch->display.mode.yres, new_mode->yres);
- ch->display.mode = *new_mode;
-
- return true;
-}
-
static int sh_mobile_lcdc_check_var(struct fb_var_screeninfo *var,
struct fb_info *info);



> Signed-off-by: Daniel Vetter <[email protected]>
> Reviewed-by: Geert Uytterhoeven <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> ---
> drivers/video/fbdev/sh_mobile_lcdcfb.c | 63 --------------------------
> drivers/video/fbdev/sh_mobile_lcdcfb.h | 5 --
> 2 files changed, 68 deletions(-)
>
> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> index dc46be38c970..c5924f5e98c6 100644
> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> @@ -556,67 +556,6 @@ sh_mobile_lcdc_must_reconfigure(struct sh_mobile_lcdc_chan *ch,
> static int sh_mobile_lcdc_check_var(struct fb_var_screeninfo *var,
> struct fb_info *info);
>
> -static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
> - enum sh_mobile_lcdc_entity_event event,
> - const struct fb_videomode *mode,
> - const struct fb_monspecs *monspec)
> -{
> - struct fb_info *info = ch->info;
> - struct fb_var_screeninfo var;
> - int ret = 0;
> -
> - switch (event) {
> - case SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT:
> - /* HDMI plug in */
> - console_lock();
> - if (lock_fb_info(info)) {
> -
> -
> - ch->display.width = monspec->max_x * 10;
> - ch->display.height = monspec->max_y * 10;
> -
> - if (!sh_mobile_lcdc_must_reconfigure(ch, mode) &&
> - info->state == FBINFO_STATE_RUNNING) {
> - /* First activation with the default monitor.
> - * Just turn on, if we run a resume here, the
> - * logo disappears.
> - */
> - info->var.width = ch->display.width;
> - info->var.height = ch->display.height;
> - sh_mobile_lcdc_display_on(ch);
> - } else {
> - /* New monitor or have to wake up */
> - fb_set_suspend(info, 0);
> - }
> -
> -
> - unlock_fb_info(info);
> - }
> - console_unlock();
> - break;
> -
> - case SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT:
> - /* HDMI disconnect */
> - console_lock();
> - if (lock_fb_info(info)) {
> - fb_set_suspend(info, 1);
> - unlock_fb_info(info);
> - }
> - console_unlock();
> - break;
> -
> - case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> - /* Validate a proposed new mode */
> - fb_videomode_to_var(&var, mode);
> - var.bits_per_pixel = info->var.bits_per_pixel;
> - var.grayscale = info->var.grayscale;
> - ret = sh_mobile_lcdc_check_var(&var, info);
> - break;
> - }
> -
> - return ret;
> -}
> -
> /* -----------------------------------------------------------------------------
> * Format helpers
> */
> @@ -2540,8 +2479,6 @@ sh_mobile_lcdc_channel_init(struct sh_mobile_lcdc_chan *ch)
> unsigned int max_size;
> unsigned int i;
>
> - ch->notify = sh_mobile_lcdc_display_notify;
> -
> /* Validate the format. */
> format = sh_mobile_format_info(cfg->fourcc);
> if (format == NULL) {
> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.h b/drivers/video/fbdev/sh_mobile_lcdcfb.h
> index b8e47a8bd8ab..589400372098 100644
> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.h
> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.h
> @@ -87,11 +87,6 @@ struct sh_mobile_lcdc_chan {
> unsigned long base_addr_c;
> unsigned int line_size;
>
> - int (*notify)(struct sh_mobile_lcdc_chan *ch,
> - enum sh_mobile_lcdc_entity_event event,
> - const struct fb_videomode *mode,
> - const struct fb_monspecs *monspec);
> -
> /* Backlight */
> struct backlight_device *bl;
> unsigned int bl_brightness;
> --
> 2.20.1
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

2019-05-27 06:14:53

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH 11/33] fbdev/sh_mobile: remove sh_mobile_lcdc_display_notify

On Sat, May 25, 2019 at 05:01:59PM +0200, Sam Ravnborg wrote:
> Hi Daniel
>
> > It's dead code, and removing it avoids me having to understand
> > what it's doing with lock_fb_info.
>
> I pushed the series through my build tests which include the sh
> architecture.
>
> One error and one warning was triggered from sh_mobile_lcdcfb.c.
> The rest was fine.
>
> The patch below removed the sole user of
> sh_mobile_lcdc_must_reconfigure() so this triggers a warning.
>
> And I also get the following error:
> drivers/video/fbdev/sh_mobile_lcdcfb.c: In function ‘sh_mobile_fb_reconfig’:
> drivers/video/fbdev/sh_mobile_lcdcfb.c:1800:2: error: implicit declaration of function ‘fbcon_update_vcs’; did you mean ‘file_update_time’? [-Werror=implicit-function-declaration]
> fbcon_update_vcs(info, true);
> ^~~~~~~~~~~~~~~~
> file_update_time
>
> I did not check but assume the error was triggered in patch 28 where
> fbcon_update_vcs() in introduced.

Oops. Can I have your sob so I can squash this in?

Thanks, Daniel

>
>
> Both are trivially fixed by appended patch.
>
> Sam
>
> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> index bb1a610d0363..b8454424910d 100644
> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> @@ -15,6 +15,7 @@
> #include <linux/ctype.h>
> #include <linux/dma-mapping.h>
> #include <linux/delay.h>
> +#include <linux/fbcon.h>
> #include <linux/gpio.h>
> #include <linux/init.h>
> #include <linux/interrupt.h>
> @@ -533,25 +534,6 @@ static void sh_mobile_lcdc_display_off(struct sh_mobile_lcdc_chan *ch)
> ch->tx_dev->ops->display_off(ch->tx_dev);
> }
>
> -static bool
> -sh_mobile_lcdc_must_reconfigure(struct sh_mobile_lcdc_chan *ch,
> - const struct fb_videomode *new_mode)
> -{
> - dev_dbg(ch->info->dev, "Old %ux%u, new %ux%u\n",
> - ch->display.mode.xres, ch->display.mode.yres,
> - new_mode->xres, new_mode->yres);
> -
> - /* It can be a different monitor with an equal video-mode */
> - if (fb_mode_is_equal(&ch->display.mode, new_mode))
> - return false;
> -
> - dev_dbg(ch->info->dev, "Switching %u -> %u lines\n",
> - ch->display.mode.yres, new_mode->yres);
> - ch->display.mode = *new_mode;
> -
> - return true;
> -}
> -
> static int sh_mobile_lcdc_check_var(struct fb_var_screeninfo *var,
> struct fb_info *info);
>
>
>
> > Signed-off-by: Daniel Vetter <[email protected]>
> > Reviewed-by: Geert Uytterhoeven <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Cc: Daniel Vetter <[email protected]>
> > ---
> > drivers/video/fbdev/sh_mobile_lcdcfb.c | 63 --------------------------
> > drivers/video/fbdev/sh_mobile_lcdcfb.h | 5 --
> > 2 files changed, 68 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> > index dc46be38c970..c5924f5e98c6 100644
> > --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> > @@ -556,67 +556,6 @@ sh_mobile_lcdc_must_reconfigure(struct sh_mobile_lcdc_chan *ch,
> > static int sh_mobile_lcdc_check_var(struct fb_var_screeninfo *var,
> > struct fb_info *info);
> >
> > -static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
> > - enum sh_mobile_lcdc_entity_event event,
> > - const struct fb_videomode *mode,
> > - const struct fb_monspecs *monspec)
> > -{
> > - struct fb_info *info = ch->info;
> > - struct fb_var_screeninfo var;
> > - int ret = 0;
> > -
> > - switch (event) {
> > - case SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT:
> > - /* HDMI plug in */
> > - console_lock();
> > - if (lock_fb_info(info)) {
> > -
> > -
> > - ch->display.width = monspec->max_x * 10;
> > - ch->display.height = monspec->max_y * 10;
> > -
> > - if (!sh_mobile_lcdc_must_reconfigure(ch, mode) &&
> > - info->state == FBINFO_STATE_RUNNING) {
> > - /* First activation with the default monitor.
> > - * Just turn on, if we run a resume here, the
> > - * logo disappears.
> > - */
> > - info->var.width = ch->display.width;
> > - info->var.height = ch->display.height;
> > - sh_mobile_lcdc_display_on(ch);
> > - } else {
> > - /* New monitor or have to wake up */
> > - fb_set_suspend(info, 0);
> > - }
> > -
> > -
> > - unlock_fb_info(info);
> > - }
> > - console_unlock();
> > - break;
> > -
> > - case SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT:
> > - /* HDMI disconnect */
> > - console_lock();
> > - if (lock_fb_info(info)) {
> > - fb_set_suspend(info, 1);
> > - unlock_fb_info(info);
> > - }
> > - console_unlock();
> > - break;
> > -
> > - case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> > - /* Validate a proposed new mode */
> > - fb_videomode_to_var(&var, mode);
> > - var.bits_per_pixel = info->var.bits_per_pixel;
> > - var.grayscale = info->var.grayscale;
> > - ret = sh_mobile_lcdc_check_var(&var, info);
> > - break;
> > - }
> > -
> > - return ret;
> > -}
> > -
> > /* -----------------------------------------------------------------------------
> > * Format helpers
> > */
> > @@ -2540,8 +2479,6 @@ sh_mobile_lcdc_channel_init(struct sh_mobile_lcdc_chan *ch)
> > unsigned int max_size;
> > unsigned int i;
> >
> > - ch->notify = sh_mobile_lcdc_display_notify;
> > -
> > /* Validate the format. */
> > format = sh_mobile_format_info(cfg->fourcc);
> > if (format == NULL) {
> > diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.h b/drivers/video/fbdev/sh_mobile_lcdcfb.h
> > index b8e47a8bd8ab..589400372098 100644
> > --- a/drivers/video/fbdev/sh_mobile_lcdcfb.h
> > +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.h
> > @@ -87,11 +87,6 @@ struct sh_mobile_lcdc_chan {
> > unsigned long base_addr_c;
> > unsigned int line_size;
> >
> > - int (*notify)(struct sh_mobile_lcdc_chan *ch,
> > - enum sh_mobile_lcdc_entity_event event,
> > - const struct fb_videomode *mode,
> > - const struct fb_monspecs *monspec);
> > -
> > /* Backlight */
> > struct backlight_device *bl;
> > unsigned int bl_brightness;
> > --
> > 2.20.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2019-05-27 07:32:01

by Sam Ravnborg

[permalink] [raw]
Subject: Re: [PATCH 11/33] fbdev/sh_mobile: remove sh_mobile_lcdc_display_notify

On Mon, May 27, 2019 at 08:13:06AM +0200, Daniel Vetter wrote:
> On Sat, May 25, 2019 at 05:01:59PM +0200, Sam Ravnborg wrote:
> > Hi Daniel
> >
> > > It's dead code, and removing it avoids me having to understand
> > > what it's doing with lock_fb_info.
> >
> > I pushed the series through my build tests which include the sh
> > architecture.
> >
> > One error and one warning was triggered from sh_mobile_lcdcfb.c.
> > The rest was fine.
> >
> > The patch below removed the sole user of
> > sh_mobile_lcdc_must_reconfigure() so this triggers a warning.
> >
> > And I also get the following error:
> > drivers/video/fbdev/sh_mobile_lcdcfb.c: In function ‘sh_mobile_fb_reconfig’:
> > drivers/video/fbdev/sh_mobile_lcdcfb.c:1800:2: error: implicit declaration of function ‘fbcon_update_vcs’; did you mean ‘file_update_time’? [-Werror=implicit-function-declaration]
> > fbcon_update_vcs(info, true);
> > ^~~~~~~~~~~~~~~~
> > file_update_time
> >
> > I did not check but assume the error was triggered in patch 28 where
> > fbcon_update_vcs() in introduced.
>
> Oops. Can I have your sob so I can squash this in?
Yes, here we go. (It was trivial so I thought not needed, sorry)
Signed-off-by: Sam Ravnborg <[email protected]>