diff mbox series

drm/ssd130x: Silence a `dubious: x & !y` warning

Message ID 20230121190930.2804224-1-javierm@redhat.com
State New
Headers
Series drm/ssd130x: Silence a `dubious: x & !y` warning |

Commit Message

Javier Martinez Canillas Jan. 21, 2023, 7:09 p.m. UTC 
  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(-)

Comments

David Laight Jan. 23, 2023, 9:26 a.m. UTC | #1 
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)
Javier Martinez Canillas Jan. 23, 2023, 9:33 a.m. UTC | #2 
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
Javier Martinez Canillas Jan. 24, 2023, 10:10 a.m. UTC | #3 
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 mbox series

Patch

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)