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

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

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/