| Message ID | 20230121190930.2804224-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Headers | show |
| Series | drm/ssd130x: Silence a `dubious: x & !y` warning | expand |
From: Javier Martinez Canillas > Sent: 21 January 2023 19:10 > > The sparse tool complains with the following warning: > > $ make M=drivers/gpu/drm/solomon/ C=2 > CC [M] drivers/gpu/drm/solomon/ssd130x.o > CHECK drivers/gpu/drm/solomon/ssd130x.c > drivers/gpu/drm/solomon/ssd130x.c:363:21: warning: dubious: x & !y > > This seems to be a false positive in my opinion but still we can silence > the tool while making the code easier to read. Let's also add a comment, > to explain why the "com_seq" logical not is used rather than its value. > > Reported-by: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > > drivers/gpu/drm/solomon/ssd130x.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c > index c3bf3a18302e..b16330a8b624 100644 > --- a/drivers/gpu/drm/solomon/ssd130x.c > +++ b/drivers/gpu/drm/solomon/ssd130x.c > @@ -81,7 +81,7 @@ > #define SSD130X_SET_PRECHARGE_PERIOD2_MASK GENMASK(7, 4) > #define SSD130X_SET_PRECHARGE_PERIOD2_SET(val) FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD2_MASK, (val)) > #define SSD130X_SET_COM_PINS_CONFIG1_MASK GENMASK(4, 4) > -#define SSD130X_SET_COM_PINS_CONFIG1_SET(val) FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, !(val)) > +#define SSD130X_SET_COM_PINS_CONFIG1_SET(val) FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, (val)) How about just changing !(val) to (val) ? 0 : 1 It should shut the compiler up and is probably more descriptive. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hello David, Thanks for your feedback. On 1/23/23 10:26, David Laight wrote: > From: Javier Martinez Canillas >> Sent: 21 January 2023 19:10 >> >> The sparse tool complains with the following warning: >> >> $ make M=drivers/gpu/drm/solomon/ C=2 >> CC [M] drivers/gpu/drm/solomon/ssd130x.o >> CHECK drivers/gpu/drm/solomon/ssd130x.c >> drivers/gpu/drm/solomon/ssd130x.c:363:21: warning: dubious: x & !y >> >> This seems to be a false positive in my opinion but still we can silence >> the tool while making the code easier to read. Let's also add a comment, >> to explain why the "com_seq" logical not is used rather than its value. >> >> Reported-by: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> >> drivers/gpu/drm/solomon/ssd130x.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c >> index c3bf3a18302e..b16330a8b624 100644 >> --- a/drivers/gpu/drm/solomon/ssd130x.c >> +++ b/drivers/gpu/drm/solomon/ssd130x.c >> @@ -81,7 +81,7 @@ >> #define SSD130X_SET_PRECHARGE_PERIOD2_MASK GENMASK(7, 4) >> #define SSD130X_SET_PRECHARGE_PERIOD2_SET(val) FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD2_MASK, (val)) >> #define SSD130X_SET_COM_PINS_CONFIG1_MASK GENMASK(4, 4) >> -#define SSD130X_SET_COM_PINS_CONFIG1_SET(val) FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, !(val)) >> +#define SSD130X_SET_COM_PINS_CONFIG1_SET(val) FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, (val)) > > How about just changing !(val) to (val) ? 0 : 1 > It should shut the compiler up and is probably more descriptive. > I'm not sure if is more descriptive than what I have with this patch. As mentioned, I believe this really is a sparse false positive but given that need to change, I preferred to make it quite explicit with a new variable and a comment. This is only executed once at driver probe time anyways. > David
On 1/21/23 20:09, Javier Martinez Canillas wrote: > The sparse tool complains with the following warning: > > $ make M=drivers/gpu/drm/solomon/ C=2 > CC [M] drivers/gpu/drm/solomon/ssd130x.o > CHECK drivers/gpu/drm/solomon/ssd130x.c > drivers/gpu/drm/solomon/ssd130x.c:363:21: warning: dubious: x & !y > > This seems to be a false positive in my opinion but still we can silence > the tool while making the code easier to read. Let's also add a comment, > to explain why the "com_seq" logical not is used rather than its value. > > Reported-by: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > Pushed this to drm-misc (drm-misc-next). Thanks!
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index c3bf3a18302e..b16330a8b624 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -81,7 +81,7 @@ #define SSD130X_SET_PRECHARGE_PERIOD2_MASK GENMASK(7, 4) #define SSD130X_SET_PRECHARGE_PERIOD2_SET(val) FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD2_MASK, (val)) #define SSD130X_SET_COM_PINS_CONFIG1_MASK GENMASK(4, 4) -#define SSD130X_SET_COM_PINS_CONFIG1_SET(val) FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, !(val)) +#define SSD130X_SET_COM_PINS_CONFIG1_SET(val) FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, (val)) #define SSD130X_SET_COM_PINS_CONFIG2_MASK GENMASK(5, 5) #define SSD130X_SET_COM_PINS_CONFIG2_SET(val) FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG2_MASK, (val)) @@ -298,6 +298,7 @@ static void ssd130x_power_off(struct ssd130x_device *ssd130x) static int ssd130x_init(struct ssd130x_device *ssd130x) { u32 precharge, dclk, com_invdir, compins, chargepump, seg_remap; + bool scan_mode; int ret; /* Set initial contrast */ @@ -360,7 +361,13 @@ static int ssd130x_init(struct ssd130x_device *ssd130x) /* Set COM pins configuration */ compins = BIT(1); - compins |= (SSD130X_SET_COM_PINS_CONFIG1_SET(ssd130x->com_seq) | + /* + * The COM scan mode field values are the inverse of the boolean DT + * property "solomon,com-seq". The value 0b means scan from COM0 to + * COM[N - 1] while 1b means scan from COM[N - 1] to COM0. + */ + scan_mode = !ssd130x->com_seq; + compins |= (SSD130X_SET_COM_PINS_CONFIG1_SET(scan_mode) | SSD130X_SET_COM_PINS_CONFIG2_SET(ssd130x->com_lrremap)); ret = ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_COM_PINS_CONFIG, compins); if (ret < 0)