> -----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> 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
