* Aguirre Rodriguez, Sergio Alberto <saaguirre@xxxxxx> [091021 08:41]: > > > > -----Original Message----- > > From: linux-omap-owner@xxxxxxxxxxxxxxx > > [mailto:linux-omap-owner@xxxxxxxxxxxxxxx] On Behalf Of Roel Kluin > > Sent: Wednesday, October 21, 2009 10:44 AM > > To: Aguirre Rodriguez, Sergio Alberto > > Cc: Imre Deak; linux-fbdev-devel@xxxxxxxxxxxxxxxxxxxxx; > > linux-omap@xxxxxxxxxxxxxxx; 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@xxxxxxxxx> > > --- > > >> 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@xxxxxx> Looks good to me too. Acked-by: Tony Lindgren <tony@xxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx > > 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@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
