[PATCH 27/57] fbdev: sh_mobile_lcdc: Pass a video mode to the notify callback

[PATCH 27/57] fbdev: sh_mobile_lcdc: Pass a video mode to the notify callback

[Laurent Pinchart]

Pass pointers to struct fb_videomode and struct fb_monspecs instead of
struct fb_var_screeninfo to the notify callback.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/video/sh_mobile_hdmi.c   |   59 +++++++++++++++++--------------------
 drivers/video/sh_mobile_lcdcfb.c |   40 ++++++++++++++-----------
 drivers/video/sh_mobile_lcdcfb.h |    3 +-
 3 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
index 055cd92..2617609 100644
--- a/drivers/video/sh_mobile_hdmi.c
+++ b/drivers/video/sh_mobile_hdmi.c
@@ -220,7 +220,7 @@ struct sh_hdmi {
 	struct clk *hdmi_clk;
 	struct device *dev;
 	struct delayed_work edid_work;
-	struct fb_var_screeninfo var;
+	struct fb_videomode mode;
 	struct fb_monspecs monspec;
 };
 
@@ -291,24 +291,24 @@ static struct snd_soc_codec_driver soc_codec_dev_sh_hdmi = {
 /* External video parameter settings */
 static void sh_hdmi_external_video_param(struct sh_hdmi *hdmi)
 {
-	struct fb_var_screeninfo *var = &hdmi->var;
+	struct fb_videomode *mode = &hdmi->mode;
 	u16 htotal, hblank, hdelay, vtotal, vblank, vdelay, voffset;
 	u8 sync = 0;
 
-	htotal = var->xres + var->right_margin + var->left_margin + var->hsync_len;
-
-	hdelay = var->hsync_len + var->left_margin;
-	hblank = var->right_margin + hdelay;
+	htotal = mode->xres + mode->right_margin + mode->left_margin
+	       + mode->hsync_len;
+	hdelay = mode->hsync_len + mode->left_margin;
+	hblank = mode->right_margin + hdelay;
 
 	/*
 	 * Vertical timing looks a bit different in Figure 18,
 	 * but let's try the same first by setting offset = 0
 	 */
-	vtotal = var->yres + var->upper_margin + var->lower_margin + var->vsync_len;
-
-	vdelay = var->vsync_len + var->upper_margin;
-	vblank = var->lower_margin + vdelay;
-	voffset = min(var->upper_margin / 2, 6U);
+	vtotal = mode->yres + mode->upper_margin + mode->lower_margin
+	       + mode->vsync_len;
+	vdelay = mode->vsync_len + mode->upper_margin;
+	vblank = mode->lower_margin + vdelay;
+	voffset = min(mode->upper_margin / 2, 6U);
 
 	/*
 	 * [3]: VSYNC polarity: Positive
@@ -316,14 +316,14 @@ static void sh_hdmi_external_video_param(struct sh_hdmi *hdmi)
 	 * [1]: Interlace/Progressive: Progressive
 	 * [0]: External video settings enable: used.
 	 */
-	if (var->sync & FB_SYNC_HOR_HIGH_ACT)
+	if (mode->sync & FB_SYNC_HOR_HIGH_ACT)
 		sync |= 4;
-	if (var->sync & FB_SYNC_VERT_HIGH_ACT)
+	if (mode->sync & FB_SYNC_VERT_HIGH_ACT)
 		sync |= 8;
 
 	dev_dbg(hdmi->dev, "H: %u, %u, %u, %u; V: %u, %u, %u, %u; sync 0x%x\n",
-		htotal, hblank, hdelay, var->hsync_len,
-		vtotal, vblank, vdelay, var->vsync_len, sync);
+		htotal, hblank, hdelay, mode->hsync_len,
+		vtotal, vblank, vdelay, mode->vsync_len, sync);
 
 	hdmi_write(hdmi, sync | (voffset << 4), HDMI_EXTERNAL_VIDEO_PARAM_SETTINGS);
 
@@ -336,8 +336,8 @@ static void sh_hdmi_external_video_param(struct sh_hdmi *hdmi)
 	hdmi_write(hdmi, hdelay, HDMI_EXTERNAL_H_DELAY_7_0);
 	hdmi_write(hdmi, hdelay >> 8, HDMI_EXTERNAL_H_DELAY_9_8);
 
-	hdmi_write(hdmi, var->hsync_len, HDMI_EXTERNAL_H_DURATION_7_0);
-	hdmi_write(hdmi, var->hsync_len >> 8, HDMI_EXTERNAL_H_DURATION_9_8);
+	hdmi_write(hdmi, mode->hsync_len, HDMI_EXTERNAL_H_DURATION_7_0);
+	hdmi_write(hdmi, mode->hsync_len >> 8, HDMI_EXTERNAL_H_DURATION_9_8);
 
 	hdmi_write(hdmi, vtotal, HDMI_EXTERNAL_V_TOTAL_7_0);
 	hdmi_write(hdmi, vtotal >> 8, HDMI_EXTERNAL_V_TOTAL_9_8);
@@ -346,7 +346,7 @@ static void sh_hdmi_external_video_param(struct sh_hdmi *hdmi)
 
 	hdmi_write(hdmi, vdelay, HDMI_EXTERNAL_V_DELAY);
 
-	hdmi_write(hdmi, var->vsync_len, HDMI_EXTERNAL_V_DURATION);
+	hdmi_write(hdmi, mode->vsync_len, HDMI_EXTERNAL_V_DURATION);
 
 	/* Set bit 0 of HDMI_EXTERNAL_VIDEO_PARAM_SETTINGS here for external mode */
 	if (!hdmi->preprogrammed_vic)
@@ -473,7 +473,7 @@ static void sh_hdmi_audio_config(struct sh_hdmi *hdmi)
  */
 static void sh_hdmi_phy_config(struct sh_hdmi *hdmi)
 {
-	if (hdmi->var.pixclock < 10000) {
+	if (hdmi->mode.pixclock < 10000) {
 		/* for 1080p8bit 148MHz */
 		hdmi_write(hdmi, 0x1d, HDMI_SLIPHDMIT_PARAM_SETTINGS_1);
 		hdmi_write(hdmi, 0x00, HDMI_SLIPHDMIT_PARAM_SETTINGS_2);
@@ -484,7 +484,7 @@ static void sh_hdmi_phy_config(struct sh_hdmi *hdmi)
 		hdmi_write(hdmi, 0x0e, HDMI_SLIPHDMIT_PARAM_SETTINGS_8);
 		hdmi_write(hdmi, 0x25, HDMI_SLIPHDMIT_PARAM_SETTINGS_9);
 		hdmi_write(hdmi, 0x04, HDMI_SLIPHDMIT_PARAM_SETTINGS_10);
-	} else if (hdmi->var.pixclock < 30000) {
+	} else if (hdmi->mode.pixclock < 30000) {
 		/* 720p, 8bit, 74.25MHz. Might need to be adjusted for other formats */
 		/*
 		 * [1:0]	Speed_A
@@ -735,7 +735,6 @@ static int sh_hdmi_read_edid(struct sh_hdmi *hdmi, unsigned long *hdmi_rate,
 {
 	struct sh_mobile_lcdc_chan *ch = hdmi->entity.lcdc;
 	struct fb_info *info = ch ? ch->info : NULL;
-	struct fb_var_screeninfo var;
 	const struct fb_videomode *mode, *found = NULL;
 	const struct fb_modelist *modelist = NULL;
 	unsigned int f_width = 0, f_height = 0, f_refresh = 0;
@@ -855,10 +854,9 @@ static int sh_hdmi_read_edid(struct sh_hdmi *hdmi, unsigned long *hdmi_rate,
 		}
 
 		/* Check if supported: sufficient fb memory, supported clock-rate */
-		fb_videomode_to_var(&var, mode);
-
 		if (ch && ch->notify &&
-		    ch->notify(ch, SH_MOBILE_LCDC_EVENT_DISPLAY_MODE, &var)) {
+		    ch->notify(ch, SH_MOBILE_LCDC_EVENT_DISPLAY_MODE, mode,
+			       NULL)) {
 			scanning = true;
 			preferred_bad = true;
 			continue;
@@ -868,9 +866,6 @@ static int sh_hdmi_read_edid(struct sh_hdmi *hdmi, unsigned long *hdmi_rate,
 		found_rate_error = rate_error;
 	}
 
-	hdmi->var.width = hdmi->monspec.max_x * 10;
-	hdmi->var.height = hdmi->monspec.max_y * 10;
-
 	/*
 	 * TODO 1: if no ->info is present, postpone running the config until
 	 * after ->info first gets registered.
@@ -916,7 +911,7 @@ static int sh_hdmi_read_edid(struct sh_hdmi *hdmi, unsigned long *hdmi_rate,
 		found->xres, found->yres, found->refresh,
 		PICOS2KHZ(found->pixclock) * 1000, found_rate_error);
 
-	fb_videomode_to_var(&hdmi->var, found);
+	hdmi->mode = *found;
 	sh_hdmi_external_video_param(hdmi);
 
 	return 0;
@@ -1017,9 +1012,9 @@ static int sh_hdmi_display_on(struct sh_mobile_lcdc_entity *entity)
 		hdmi_write(hdmi, 0x80, HDMI_SYSTEM_CTRL);
 		dev_dbg(hdmi->dev, "HDMI running\n");
 		break;
-	case HDMI_HOTPLUG_DISCONNECTED:
 	default:
-		hdmi->var = ch->display_var;
+		fb_var_to_videomode(&hdmi->mode, &ch->display_var);
+		break;
 	}
 
 	return hdmi->hp_state = HDMI_HOTPLUG_DISCONNECTED
@@ -1107,7 +1102,7 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
 
 		if (ch && ch->notify)
 			ch->notify(ch, SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT,
-				   &hdmi->var);
+				   &hdmi->mode, &hdmi->monspec);
 	} else {
 		hdmi->monspec.modedb_len = 0;
 		fb_destroy_modedb(hdmi->monspec.modedb);
@@ -1115,7 +1110,7 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
 
 		if (ch && ch->notify)
 			ch->notify(ch, SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT,
-				   NULL);
+				   NULL, NULL);
 
 		ret = 0;
 	}
diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
index 21e5f10..4ec216e 100644
--- a/drivers/video/sh_mobile_lcdcfb.c
+++ b/drivers/video/sh_mobile_lcdcfb.c
@@ -365,35 +365,31 @@ static void sh_mobile_lcdc_display_off(struct sh_mobile_lcdc_chan *ch)
 
 static bool
 sh_mobile_lcdc_must_reconfigure(struct sh_mobile_lcdc_chan *ch,
-				const struct fb_var_screeninfo *new_var)
+				const struct fb_videomode *new_mode)
 {
 	struct fb_var_screeninfo *old_var = &ch->display_var;
 	struct fb_videomode old_mode;
-	struct fb_videomode new_mode;
 
 	fb_var_to_videomode(&old_mode, old_var);
-	fb_var_to_videomode(&new_mode, new_var);
 
 	dev_dbg(ch->info->dev, "Old %ux%u, new %ux%u\n",
-		old_mode.xres, old_mode.yres, new_mode.xres, new_mode.yres);
+		old_mode.xres, old_mode.yres, new_mode->xres, new_mode->yres);
 
-	if (fb_mode_is_equal(&old_mode, &new_mode)) {
-		/* It can be a different monitor with an equal video-mode */
-		old_var->width = new_var->width;
-		old_var->height = new_var->height;
+	/* It can be a different monitor with an equal video-mode */
+	if (fb_mode_is_equal(&old_mode, new_mode))
 		return false;
-	}
 
 	dev_dbg(ch->info->dev, "Switching %u -> %u lines\n",
-		old_mode.yres, new_mode.yres);
-	*old_var = *new_var;
+		old_mode.yres, new_mode->yres);
+	fb_videomode_to_var(old_var, new_mode);
 
 	return true;
 }
 
 static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
 					 enum sh_mobile_lcdc_entity_event event,
-					 struct fb_var_screeninfo *var)
+					 const struct fb_videomode *mode,
+					 const struct fb_monspecs *monspec)
 {
 	struct fb_info *info = ch->info;
 	int ret = 0;
@@ -404,14 +400,17 @@ static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
 		if (lock_fb_info(info)) {
 			console_lock();
 
-			if (!sh_mobile_lcdc_must_reconfigure(ch, var) &&
+			ch->display_var.width = monspec->max_x * 10;
+			ch->display_var.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 = var->width;
-				info->var.height = var->height;
+				info->var.width = monspec->max_x * 10;
+				info->var.height = monspec->max_y * 10;
 				sh_mobile_lcdc_display_on(ch);
 			} else {
 				/* New monitor or have to wake up */
@@ -433,12 +432,17 @@ static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
 		}
 		break;
 
-	case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
+	case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE: {
+		struct fb_var_screeninfo var;
+
 		/* Validate a proposed new mode */
-		var->bits_per_pixel = info->var.bits_per_pixel;
-		ret = info->fbops->fb_check_var(var, info);
+		fb_videomode_to_var(&var, mode);
+		var.bits_per_pixel = info->var.bits_per_pixel;
+		var.grayscale = info->var.grayscale;
+		ret = info->fbops->fb_check_var(&var, info);
 		break;
 	}
+	}
 
 	return ret;
 }
diff --git a/drivers/video/sh_mobile_lcdcfb.h b/drivers/video/sh_mobile_lcdcfb.h
index e2eb7af..10086ae 100644
--- a/drivers/video/sh_mobile_lcdcfb.h
+++ b/drivers/video/sh_mobile_lcdcfb.h
@@ -79,7 +79,8 @@ struct sh_mobile_lcdc_chan {
 
 	int (*notify)(struct sh_mobile_lcdc_chan *ch,
 		      enum sh_mobile_lcdc_entity_event event,
-		      struct fb_var_screeninfo *var);
+		      const struct fb_videomode *mode,
+		      const struct fb_monspecs *monspec);
 };
 
 #endif
-- 
1.7.3.4

Re: [PATCH 27/57] fbdev: sh_mobile_lcdc: Pass a video mode to the

[Guennadi Liakhovetski]

On Tue, 13 Dec 2011, Laurent Pinchart wrote:

> Pass pointers to struct fb_videomode and struct fb_monspecs instead of
> struct fb_var_screeninfo to the notify callback.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/video/sh_mobile_hdmi.c   |   59 +++++++++++++++++--------------------
>  drivers/video/sh_mobile_lcdcfb.c |   40 ++++++++++++++-----------
>  drivers/video/sh_mobile_lcdcfb.h |    3 +-
>  3 files changed, 51 insertions(+), 51 deletions(-)

[snip]

> diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
> index 21e5f10..4ec216e 100644
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c

[ditto]

> @@ -433,12 +432,17 @@ static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
>  		}
>  		break;
>  
> -	case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> +	case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE: {
> +		struct fb_var_screeninfo var;
> +
>  		/* Validate a proposed new mode */
> -		var->bits_per_pixel = info->var.bits_per_pixel;
> -		ret = info->fbops->fb_check_var(var, info);
> +		fb_videomode_to_var(&var, mode);
> +		var.bits_per_pixel = info->var.bits_per_pixel;
> +		var.grayscale = info->var.grayscale;
> +		ret = info->fbops->fb_check_var(&var, info);
>  		break;
>  	}
> +	}

nitpick - please, realign:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 27/57] fbdev: sh_mobile_lcdc: Pass a video mode to the notify callback

[Laurent Pinchart]

Hi Guennadi,

On Thursday 15 December 2011 20:01:54 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > Pass pointers to struct fb_videomode and struct fb_monspecs instead of
> > struct fb_var_screeninfo to the notify callback.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/video/sh_mobile_hdmi.c   |   59
> >  +++++++++++++++++-------------------- drivers/video/sh_mobile_lcdcfb.c
> >  |   40 ++++++++++++++----------- drivers/video/sh_mobile_lcdcfb.h |   
> >  3 +-
> >  3 files changed, 51 insertions(+), 51 deletions(-)
> 
> [snip]
> 
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index 21e5f10..4ec216e 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
> 
> [ditto]
> 
> > @@ -433,12 +432,17 @@ static int sh_mobile_lcdc_display_notify(struct
> > sh_mobile_lcdc_chan *ch,
> > 
> >  		}
> >  		break;
> > 
> > -	case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> > +	case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE: {
> > +		struct fb_var_screeninfo var;
> > +
> > 
> >  		/* Validate a proposed new mode */
> > 
> > -		var->bits_per_pixel = info->var.bits_per_pixel;
> > -		ret = info->fbops->fb_check_var(var, info);
> > +		fb_videomode_to_var(&var, mode);
> > +		var.bits_per_pixel = info->var.bits_per_pixel;
> > +		var.grayscale = info->var.grayscale;
> > +		ret = info->fbops->fb_check_var(&var, info);
> > 
> >  		break;
> >  	
> >  	}
> > 
> > +	}
> 
> nitpick - please, realign:-)

It's common in driver code not to increase the identation level twice in a 
case statement if braces are needed. However, those "braced" statements are 
usually not the last ones in the switch. In this particular case this creates 
an alignment issue at the end, as your correctly pointed out. However, I'd 
like to avoid increasing the indentation of the whole block. Increasing the 
indentation of the closing brace only also causes an alignment issue. I can 
add a default case that returns an error to fix this, but that's adding code 
to fix a cosmetic problem :-) What's your preference.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 27/57] fbdev: sh_mobile_lcdc: Pass a video mode to the

[Guennadi Liakhovetski]

On Fri, 16 Dec 2011, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Thursday 15 December 2011 20:01:54 Guennadi Liakhovetski wrote:
> > On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > > Pass pointers to struct fb_videomode and struct fb_monspecs instead of
> > > struct fb_var_screeninfo to the notify callback.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/video/sh_mobile_hdmi.c   |   59
> > >  +++++++++++++++++-------------------- drivers/video/sh_mobile_lcdcfb.c
> > >  |   40 ++++++++++++++----------- drivers/video/sh_mobile_lcdcfb.h |   
> > >  3 +-
> > >  3 files changed, 51 insertions(+), 51 deletions(-)
> > 
> > [snip]
> > 
> > > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > > b/drivers/video/sh_mobile_lcdcfb.c index 21e5f10..4ec216e 100644
> > > --- a/drivers/video/sh_mobile_lcdcfb.c
> > > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > 
> > [ditto]
> > 
> > > @@ -433,12 +432,17 @@ static int sh_mobile_lcdc_display_notify(struct
> > > sh_mobile_lcdc_chan *ch,
> > > 
> > >  		}
> > >  		break;
> > > 
> > > -	case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> > > +	case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE: {
> > > +		struct fb_var_screeninfo var;
> > > +
> > > 
> > >  		/* Validate a proposed new mode */
> > > 
> > > -		var->bits_per_pixel = info->var.bits_per_pixel;
> > > -		ret = info->fbops->fb_check_var(var, info);
> > > +		fb_videomode_to_var(&var, mode);
> > > +		var.bits_per_pixel = info->var.bits_per_pixel;
> > > +		var.grayscale = info->var.grayscale;
> > > +		ret = info->fbops->fb_check_var(&var, info);
> > > 
> > >  		break;
> > >  	
> > >  	}
> > > 
> > > +	}
> > 
> > nitpick - please, realign:-)
> 
> It's common in driver code not to increase the identation level twice in a 
> case statement if braces are needed. However, those "braced" statements are 
> usually not the last ones in the switch. In this particular case this creates 
> an alignment issue at the end, as your correctly pointed out. However, I'd 
> like to avoid increasing the indentation of the whole block. Increasing the 
> indentation of the closing brace only also causes an alignment issue. I can 
> add a default case that returns an error to fix this, but that's adding code 
> to fix a cosmetic problem :-) What's your preference.

Personally, I don't like just using braces without a related statement 
like "if," "do," etc. In such "case" cases I usually avoid adding own 
braces and just put any declarations, that are needed there in the 
enclosing block, e.g., the function. If you really don't want either, I 
would probably prefer a style like

	switch (x) {
	case X:
		{
			int y;
			my_statement(y);
			break;
		}
	}

You could also do

	switch (x) {
	case X:
		do {
			int y;
			my_statement(y);
			break;
		} while (0);
	}

So, choose your poison:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 27/57] fbdev: sh_mobile_lcdc: Pass a video mode to the notify callback

[Laurent Pinchart]

Hi Guennadi,

On Friday 16 December 2011 11:33:55 Guennadi Liakhovetski wrote:
> On Fri, 16 Dec 2011, Laurent Pinchart wrote:
> > On Thursday 15 December 2011 20:01:54 Guennadi Liakhovetski wrote:
> > > On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > > > Pass pointers to struct fb_videomode and struct fb_monspecs instead
> > > > of struct fb_var_screeninfo to the notify callback.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > 
> > > >  drivers/video/sh_mobile_hdmi.c   |   59
> > > >  +++++++++++++++++--------------------
> > > >  drivers/video/sh_mobile_lcdcfb.c
> > > >  
> > > >  |   40 ++++++++++++++----------- drivers/video/sh_mobile_lcdcfb.h |
> > > >  
> > > >  3 +-
> > > >  3 files changed, 51 insertions(+), 51 deletions(-)
> > > 
> > > [snip]
> > > 
> > > > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > > > b/drivers/video/sh_mobile_lcdcfb.c index 21e5f10..4ec216e 100644
> > > > --- a/drivers/video/sh_mobile_lcdcfb.c
> > > > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > > 
> > > [ditto]
> > > 
> > > > @@ -433,12 +432,17 @@ static int sh_mobile_lcdc_display_notify(struct
> > > > sh_mobile_lcdc_chan *ch,
> > > > 
> > > >  		}
> > > >  		break;
> > > > 
> > > > -	case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> > > > +	case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE: {
> > > > +		struct fb_var_screeninfo var;
> > > > +
> > > > 
> > > >  		/* Validate a proposed new mode */
> > > > 
> > > > -		var->bits_per_pixel = info->var.bits_per_pixel;
> > > > -		ret = info->fbops->fb_check_var(var, info);
> > > > +		fb_videomode_to_var(&var, mode);
> > > > +		var.bits_per_pixel = info->var.bits_per_pixel;
> > > > +		var.grayscale = info->var.grayscale;
> > > > +		ret = info->fbops->fb_check_var(&var, info);
> > > > 
> > > >  		break;
> > > >  	
> > > >  	}
> > > > 
> > > > +	}
> > > 
> > > nitpick - please, realign:-)
> > 
> > It's common in driver code not to increase the identation level twice in
> > a case statement if braces are needed. However, those "braced"
> > statements are usually not the last ones in the switch. In this
> > particular case this creates an alignment issue at the end, as your
> > correctly pointed out. However, I'd like to avoid increasing the
> > indentation of the whole block. Increasing the indentation of the
> > closing brace only also causes an alignment issue. I can add a default
> > case that returns an error to fix this, but that's adding code to fix a
> > cosmetic problem :-) What's your preference.
> 
> Personally, I don't like just using braces without a related statement
> like "if," "do," etc. In such "case" cases I usually avoid adding own
> braces and just put any declarations, that are needed there in the
> enclosing block, e.g., the function.

Using braces with case statements is a common practice in the kernel, without 
adding an extra indentation level.

In this particular case I can move the variable declaration at the top of the 
function, as the generated code on ARM (at least with gcc ('cs2009q3-67-hard-
sb4') 4.4.1) doesn't differ.

> If you really don't want either, I would probably prefer a style like
> 
> 	switch (x) {
> 	case X:
> 		{
> 			int y;
> 			my_statement(y);
> 			break;
> 		}
> 	}
> 
> You could also do
> 
> 	switch (x) {
> 	case X:
> 		do {
> 			int y;
> 			my_statement(y);
> 			break;
> 		} while (0);
> 	}
> 
> So, choose your poison:-)


-- 
Regards,

Laurent Pinchart