[PATCH] Staging: fbtft: Do not use same argument more than once in bit operation.

[PATCH] Staging: fbtft: Do not use same argument more than once in bit operation.

[Sandhya Bankar]

Do not use same argument more than once in bit operation.

Signed-off-by: Sandhya Bankar <bankarsandhya512@gmail.com>
---
 drivers/staging/fbtft/fb_uc1611.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/fbtft/fb_uc1611.c b/drivers/staging/fbtft/fb_uc1611.c
index 4e82814..bffe694 100644
--- a/drivers/staging/fbtft/fb_uc1611.c
+++ b/drivers/staging/fbtft/fb_uc1611.c
@@ -100,7 +100,7 @@ static int init_display(struct fbtft_par *par)
 	write_reg(par, 0x2C | (pump & 0x03));
 
 	/* Set inverse display */
-	write_reg(par, 0xA6 | (0x01 & 0x01));
+	write_reg(par, 0xA6 | 0x01);
 
 	/* Set 4-bit grayscale mode */
 	write_reg(par, 0xD0 | (0x02 & 0x03));
@@ -166,8 +166,8 @@ static int set_var(struct fbtft_par *par)
 		/* Set RAM address control */
 		write_reg(par, 0x88
 			| (0x0 & 0x1) << 2 /* Increment positively */
-			| (0x1 & 0x1) << 1 /* Increment page first */
-			| (0x1 & 0x1));    /* Wrap around (default) */
+			| (0x1 << 1) /* Increment page first */
+			| 0x1);    /* Wrap around (default) */
 
 		/* Set LCD mapping */
 		write_reg(par, 0xC0
@@ -180,11 +180,11 @@ static int set_var(struct fbtft_par *par)
 		write_reg(par, 0x88
 			| (0x0 & 0x1) << 2 /* Increment positively */
 			| (0x0 & 0x1) << 1 /* Increment column first */
-			| (0x1 & 0x1));    /* Wrap around (default) */
+			| 0x1);    /* Wrap around (default) */
 
 		/* Set LCD mapping */
 		write_reg(par, 0xC0
-			| (0x1 & 0x1) << 2 /* Mirror Y ON */
+			| (0x1 << 2) /* Mirror Y ON */
 			| (0x0 & 0x1) << 1 /* Mirror X OFF */
 			| (0x0 & 0x1));    /* MS nibble last (default) */
 		break;
@@ -192,13 +192,13 @@ static int set_var(struct fbtft_par *par)
 		/* Set RAM address control */
 		write_reg(par, 0x88
 			| (0x0 & 0x1) << 2 /* Increment positively */
-			| (0x1 & 0x1) << 1 /* Increment page first */
-			| (0x1 & 0x1));    /* Wrap around (default) */
+			| (0x1 << 1) /* Increment page first */
+			| 0x1);    /* Wrap around (default) */
 
 		/* Set LCD mapping */
 		write_reg(par, 0xC0
-			| (0x1 & 0x1) << 2 /* Mirror Y ON */
-			| (0x1 & 0x1) << 1 /* Mirror X ON */
+			| (0x1 << 2) /* Mirror Y ON */
+			| (0x1 << 1) /* Mirror X ON */
 			| (0x0 & 0x1));    /* MS nibble last (default) */
 		break;
 	default:
@@ -206,12 +206,12 @@ static int set_var(struct fbtft_par *par)
 		write_reg(par, 0x88
 			| (0x0 & 0x1) << 2 /* Increment positively */
 			| (0x0 & 0x1) << 1 /* Increment column first */
-			| (0x1 & 0x1));    /* Wrap around (default) */
+			| 0x1);    /* Wrap around (default) */
 
 		/* Set LCD mapping */
 		write_reg(par, 0xC0
 			| (0x0 & 0x1) << 2 /* Mirror Y OFF */
-			| (0x1 & 0x1) << 1 /* Mirror X ON */
+			| (0x1 << 1) /* Mirror X ON */
 			| (0x0 & 0x1));    /* MS nibble last (default) */
 		break;
 	}
-- 
1.8.3.4

Re: [Outreachy kernel] [PATCH] Staging: fbtft: Do not use same argument more than once in bit operation.

[Julia Lawall]

On Thu, 3 Mar 2016, Sandhya Bankar wrote:

> Do not use same argument more than once in bit operation.

These look so useless that there could be a reason for them.

julia

> 
> Signed-off-by: Sandhya Bankar <bankarsandhya512@gmail.com>
> ---
>  drivers/staging/fbtft/fb_uc1611.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/fbtft/fb_uc1611.c b/drivers/staging/fbtft/fb_uc1611.c
> index 4e82814..bffe694 100644
> --- a/drivers/staging/fbtft/fb_uc1611.c
> +++ b/drivers/staging/fbtft/fb_uc1611.c
> @@ -100,7 +100,7 @@ static int init_display(struct fbtft_par *par)
>  	write_reg(par, 0x2C | (pump & 0x03));
>  
>  	/* Set inverse display */
> -	write_reg(par, 0xA6 | (0x01 & 0x01));
> +	write_reg(par, 0xA6 | 0x01);
>  
>  	/* Set 4-bit grayscale mode */
>  	write_reg(par, 0xD0 | (0x02 & 0x03));
> @@ -166,8 +166,8 @@ static int set_var(struct fbtft_par *par)
>  		/* Set RAM address control */
>  		write_reg(par, 0x88
>  			| (0x0 & 0x1) << 2 /* Increment positively */
> -			| (0x1 & 0x1) << 1 /* Increment page first */
> -			| (0x1 & 0x1));    /* Wrap around (default) */
> +			| (0x1 << 1) /* Increment page first */
> +			| 0x1);    /* Wrap around (default) */
>  
>  		/* Set LCD mapping */
>  		write_reg(par, 0xC0
> @@ -180,11 +180,11 @@ static int set_var(struct fbtft_par *par)
>  		write_reg(par, 0x88
>  			| (0x0 & 0x1) << 2 /* Increment positively */
>  			| (0x0 & 0x1) << 1 /* Increment column first */
> -			| (0x1 & 0x1));    /* Wrap around (default) */
> +			| 0x1);    /* Wrap around (default) */
>  
>  		/* Set LCD mapping */
>  		write_reg(par, 0xC0
> -			| (0x1 & 0x1) << 2 /* Mirror Y ON */
> +			| (0x1 << 2) /* Mirror Y ON */
>  			| (0x0 & 0x1) << 1 /* Mirror X OFF */
>  			| (0x0 & 0x1));    /* MS nibble last (default) */
>  		break;
> @@ -192,13 +192,13 @@ static int set_var(struct fbtft_par *par)
>  		/* Set RAM address control */
>  		write_reg(par, 0x88
>  			| (0x0 & 0x1) << 2 /* Increment positively */
> -			| (0x1 & 0x1) << 1 /* Increment page first */
> -			| (0x1 & 0x1));    /* Wrap around (default) */
> +			| (0x1 << 1) /* Increment page first */
> +			| 0x1);    /* Wrap around (default) */
>  
>  		/* Set LCD mapping */
>  		write_reg(par, 0xC0
> -			| (0x1 & 0x1) << 2 /* Mirror Y ON */
> -			| (0x1 & 0x1) << 1 /* Mirror X ON */
> +			| (0x1 << 2) /* Mirror Y ON */
> +			| (0x1 << 1) /* Mirror X ON */
>  			| (0x0 & 0x1));    /* MS nibble last (default) */
>  		break;
>  	default:
> @@ -206,12 +206,12 @@ static int set_var(struct fbtft_par *par)
>  		write_reg(par, 0x88
>  			| (0x0 & 0x1) << 2 /* Increment positively */
>  			| (0x0 & 0x1) << 1 /* Increment column first */
> -			| (0x1 & 0x1));    /* Wrap around (default) */
> +			| 0x1);    /* Wrap around (default) */
>  
>  		/* Set LCD mapping */
>  		write_reg(par, 0xC0
>  			| (0x0 & 0x1) << 2 /* Mirror Y OFF */
> -			| (0x1 & 0x1) << 1 /* Mirror X ON */
> +			| (0x1 << 1) /* Mirror X ON */
>  			| (0x0 & 0x1));    /* MS nibble last (default) */
>  		break;
>  	}
> -- 
> 1.8.3.4
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160303103355.GA5375%40sandhya.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [PATCH] Staging: fbtft: Do not use same argument more than once in bit operation.

[Alison Schofield]

The reason may be that author is trying to
to explicitly reflect the register settings.

Considering this code...

static int init_display(struct fbtft_par *par)
{
.................snip........................

	/* Set temperature compensation */
        write_reg(par, 0x24 | (temp & 0x03));

        /* Set panel loading */
        write_reg(par, 0x28 | (load & 0x03));

        /* Set pump control */
        write_reg(par, 0x2C | (pump & 0x03));

        /* Set inverse display */
        write_reg(par, 0xA6 | (0x01 & 0x01));

        /* Set 4-bit grayscale mode */
        write_reg(par, 0xD0 | (0x02 & 0x03));

In the first 3 commands temp,load,pump are vars &'d with a mask.
In the last 2 commands 0x01 and 0x02 are hardcoded values &'d with a mask.
Sometimes that hardcoded value just happens to be the same as its mask.
I might even guess that this author knew it was redundant, but
that the compiler would reduce all of that for him anyway.

you can see the register definitions here if you want to get to
very bottom of this ;)
http://lcdhype.condense.de/index.php?act=Attach&type=post&id=6112&index=0

I wonder if it is better to define all those hardcode values for even
more clarity.

Re: [Outreachy kernel] Re: [PATCH] Staging: fbtft: Do not use same argument more than once in bit operation.

[Julia Lawall]

On Thu, 3 Mar 2016, Alison Schofield wrote:

> The reason may be that author is trying to
> to explicitly reflect the register settings.
> 
> Considering this code...
> 
> static int init_display(struct fbtft_par *par)
> {
> .................snip........................
> 
> 	/* Set temperature compensation */
>         write_reg(par, 0x24 | (temp & 0x03));
> 
>         /* Set panel loading */
>         write_reg(par, 0x28 | (load & 0x03));
> 
>         /* Set pump control */
>         write_reg(par, 0x2C | (pump & 0x03));
> 
>         /* Set inverse display */
>         write_reg(par, 0xA6 | (0x01 & 0x01));
> 
>         /* Set 4-bit grayscale mode */
>         write_reg(par, 0xD0 | (0x02 & 0x03));
> 
> In the first 3 commands temp,load,pump are vars &'d with a mask.
> In the last 2 commands 0x01 and 0x02 are hardcoded values &'d with a mask.
> Sometimes that hardcoded value just happens to be the same as its mask.
> I might even guess that this author knew it was redundant, but
> that the compiler would reduce all of that for him anyway.
> 
> you can see the register definitions here if you want to get to
> very bottom of this ;)
> http://lcdhype.condense.de/index.php?act=Attach&type=post&id=6112&index=0
> 
> I wonder if it is better to define all those hardcode values for even
> more clarity.

That would certainly be nice, at least the ones on the left of the &.  
Then one might not even need the comments any more.

julia