Re: [PATCH] omapfb: Wrong test on unsigned?

[Tony Lindgren <tony@atomide.com>]

* Aguirre Rodriguez, Sergio Alberto <saaguirre@ti.com> [091021 08:41]:
>  
> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org 
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Roel Kluin
> > Sent: Wednesday, October 21, 2009 10:44 AM
> > To: Aguirre Rodriguez, Sergio Alberto
> > Cc: Imre Deak; linux-fbdev-devel@lists.sourceforge.net; 
> > linux-omap@vger.kernel.org; Andrew Morton
> > Subject: Re: [PATCH] omapfb: Wrong test on unsigned?
> > 
> > regno is unsigned so the test didn't work. If regno
> > can't be used return an error.
> > 
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> > >> Is this correct? please review.
> > >>
> > >> diff --git a/drivers/video/omap/omapfb_main.c 
> > >> b/drivers/video/omap/omapfb_main.c
> > >> index 0d0c8c8..cc7dd93 100644
> > >> --- a/drivers/video/omap/omapfb_main.c
> > >> +++ b/drivers/video/omap/omapfb_main.c
> > >> @@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info 
> > >> *info, u_int regno, u_int red, u_int green,
> > >>  		if (r != 0)
> > >>  			break;
> > >>  
> > >> -		if (regno < 0) {
> > >> +		if ((int)regno < 0) {
> > > 
> > > Hmm...
> > > 
> > > Isn't regno unsigned integer from the start?
> > 
> > yes
> > 
> > > 2 things here:
> > > 
> > > - regno will never be negative.
> > > - Casting won't make a difference in the meaning., it'll 
> > make a negative only when:
> > > 
> > > 	regno > ((2^32) / 2)
> > > 
> > > Which doesn't make any sense IMHO.
> > 
> > I think it is strange that _setcolreg() accepts a regno that 
> > is invalid,
> > ignores it and just returns as if all was OK. If you agree 
> > then you may
> > like the patch below.
> 
> Yep. Looks nicer to me ;)
> 
> Acked-by: Sergio Aguirre <saaguirre@ti.com>

Looks good to me too.

Acked-by: Tony Lindgren <tony@atomide.com>

> 
> Regards,
> Sergio
> > 
> > > Regards,
> > > Sergio
> > 
> > Thanks,
> > 
> > Roel
> > 
> > diff --git a/drivers/video/omap/omapfb_main.c 
> > b/drivers/video/omap/omapfb_main.c
> > index 0d0c8c8..4da94d0 100644
> > --- a/drivers/video/omap/omapfb_main.c
> > +++ b/drivers/video/omap/omapfb_main.c
> > @@ -285,12 +285,6 @@ static int _setcolreg(struct fb_info 
> > *info, u_int regno, u_int red, u_int green,
> >  	case OMAPFB_COLOR_RGB444:
> >  		if (r != 0)
> >  			break;
> > -
> > -		if (regno < 0) {
> > -			r = -EINVAL;
> > -			break;
> > -		}
> > -
> >  		if (regno < 16) {
> >  			u16 pal;
> >  			pal = ((red >> (16 - var->red.length)) <<
> > @@ -299,6 +293,8 @@ static int _setcolreg(struct fb_info 
> > *info, u_int regno, u_int red, u_int green,
> >  					var->green.offset) |
> >  			      (blue >> (16 - var->blue.length));
> >  			((u32 *)(info->pseudo_palette))[regno] = pal;
> > +		} else {
> > +			r = -EINVAL;
> >  		}
> >  		break;
> >  	default:
> > --
> > To unsubscribe from this list: send the line "unsubscribe 
> > linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html