[PATCH v3 0/4] staging: fbtft: code cleanup patchset - checkpatch

[PATCH v3 0/4] staging: fbtft: code cleanup patchset - checkpatch

[Deepak R Varma]

Implement code formatting corrections as flagged by checkpatch
script. Common issues around lines exceeding 80 column limit and similar
formatting issues for the fbtft driver files are considered in the scope
of this patchset.

Note that top two patches in the list below were sent to the mailing
list for review as individual patches earlier. As I found additional
similar issues in the same area, I am requesting to combine these
related patches in this patchset.


Changes since v2:
    - Implement Corrections suggested by Stefano for [patch 1/4, 2/4, 3/4]:
        1. Typos in patch description.
	2. Improve variable name as per coding style guidelines.
    - Advised following for Patch 4/4
	1. Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

Changes since v1:
    - Corrections suggested by Julia for [patch 4 /4]:
        1. Update commit message - use word "potential" in place of "likely".
        2. Update log message - use word "parentheses" in place of "braces".
        3. Rephrase log message - make it concise and easy to understand.


Deepak R Varma (4):
  staging: fbtft: Reformat line over 80 characters
  staging: fbtft: Reformat long macro definitions
  staging: fbtft: simplify array index computation
  staging: fbtft: Avoid potential precedence issues

 drivers/staging/fbtft/fbtft-core.c  |  4 +++-
 drivers/staging/fbtft/fbtft-sysfs.c |  6 +++++-
 drivers/staging/fbtft/fbtft.h       | 18 +++++++++++++-----
 3 files changed, 21 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH 1/4] staging: fbtft: Reformat line over 80 characters

[Deepak R Varma]

A long variable name beyond 80 characters extends  into the next
line. Reformatting the line makes it more readable. Also adding an extra
line for the next instruction following current if block helps
understand it better.

Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
---
 drivers/staging/fbtft/fbtft-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index d3e098b41b1a..4f362dad4436 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -1186,7 +1186,9 @@ static struct fbtft_platform_data *fbtft_properties_read(struct device *dev)
 	if (device_property_present(dev, "led-gpios"))
 		pdata->display.backlight = 1;
 	if (device_property_present(dev, "init"))
-		pdata->display.fbtftops.init_display = fbtft_init_display_from_property;
+		pdata->display.fbtftops.init_display =
+			fbtft_init_display_from_property;
+
 	pdata->display.fbtftops.request_gpios = fbtft_request_gpios;
 
 	return pdata;
-- 
2.17.1

[PATCH 2/4] staging: fbtft: Reformat long macro definitions

[Deepak R Varma]

Multiple macro definitions crossing 80 character line length makes them
hard to understand. Reformatting the these lines makes the code more
readable and easier to understand. Issue flagged by checkpatch script.

Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
---
 drivers/staging/fbtft/fbtft.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 5f782da51959..81da30f4062e 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -348,9 +348,17 @@ module_exit(fbtft_driver_module_exit);
 
 /* shorthand debug levels */
 #define DEBUG_LEVEL_1	DEBUG_REQUEST_GPIOS
-#define DEBUG_LEVEL_2	(DEBUG_LEVEL_1 | DEBUG_DRIVER_INIT_FUNCTIONS | DEBUG_TIME_FIRST_UPDATE)
-#define DEBUG_LEVEL_3	(DEBUG_LEVEL_2 | DEBUG_RESET | DEBUG_INIT_DISPLAY | DEBUG_BLANK | DEBUG_REQUEST_GPIOS | DEBUG_FREE_GPIOS | DEBUG_VERIFY_GPIOS | DEBUG_BACKLIGHT | DEBUG_SYSFS)
-#define DEBUG_LEVEL_4	(DEBUG_LEVEL_2 | DEBUG_FB_READ | DEBUG_FB_WRITE | DEBUG_FB_FILLRECT | DEBUG_FB_COPYAREA | DEBUG_FB_IMAGEBLIT | DEBUG_FB_BLANK)
+#define DEBUG_LEVEL_2	(DEBUG_LEVEL_1 | DEBUG_DRIVER_INIT_FUNCTIONS        \
+				       | DEBUG_TIME_FIRST_UPDATE)
+#define DEBUG_LEVEL_3	(DEBUG_LEVEL_2 | DEBUG_RESET | DEBUG_INIT_DISPLAY   \
+				       | DEBUG_BLANK | DEBUG_REQUEST_GPIOS  \
+				       | DEBUG_FREE_GPIOS                   \
+				       | DEBUG_VERIFY_GPIOS                 \
+				       | DEBUG_BACKLIGHT | DEBUG_SYSFS)
+#define DEBUG_LEVEL_4	(DEBUG_LEVEL_2 | DEBUG_FB_READ | DEBUG_FB_WRITE     \
+				       | DEBUG_FB_FILLRECT                  \
+				       | DEBUG_FB_COPYAREA                  \
+				       | DEBUG_FB_IMAGEBLIT | DEBUG_FB_BLANK)
 #define DEBUG_LEVEL_5	(DEBUG_LEVEL_3 | DEBUG_UPDATE_DISPLAY)
 #define DEBUG_LEVEL_6	(DEBUG_LEVEL_4 | DEBUG_LEVEL_5)
 #define DEBUG_LEVEL_7	0xFFFFFFFF
-- 
2.17.1

[PATCH 3/4] staging: fbtft: simplify array index computation

[Deepak R Varma]

An array index is being computed by mathematical calculation on the
Lvalue side of the expression. This also further results in the statement
exceeding 80 character statement length.

A local variable can store the value of the array index computation. The
variable can then be used as array index. This improves readability of
the code and also address 80 character warning raised by checkpatch.

Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
---
 drivers/staging/fbtft/fbtft-sysfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft-sysfs.c b/drivers/staging/fbtft/fbtft-sysfs.c
index 2a5c630dab87..26e52cc2de64 100644
--- a/drivers/staging/fbtft/fbtft-sysfs.c
+++ b/drivers/staging/fbtft/fbtft-sysfs.c
@@ -25,6 +25,7 @@ int fbtft_gamma_parse_str(struct fbtft_par *par, u32 *curves,
 	unsigned long val = 0;
 	int ret = 0;
 	int curve_counter, value_counter;
+	int _count;
 
 	fbtft_par_dbg(DEBUG_SYSFS, par, "%s() str=\n", __func__);
 
@@ -68,7 +69,10 @@ int fbtft_gamma_parse_str(struct fbtft_par *par, u32 *curves,
 			ret = get_next_ulong(&curve_p, &val, " ", 16);
 			if (ret)
 				goto out;
-			curves[curve_counter * par->gamma.num_values + value_counter] = val;
+
+			_count = curve_counter * par->gamma.num_values +
+				 value_counter;
+			curves[_count] = val;
 			value_counter++;
 		}
 		if (value_counter != par->gamma.num_values) {
-- 
2.17.1

[PATCH 4/4] staging: fbtft: Avoid potential precedence issues

[Deepak R Varma]

Put parentheses around uses of macro parameters to avoid possible
precedence issues. Problem detected by checkpatch.

Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
---
 drivers/staging/fbtft/fbtft.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 81da30f4062e..76f8c090a837 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -406,8 +406,8 @@ do {                                                         \
 
 #define fbtft_par_dbg(level, par, format, arg...)            \
 do {                                                         \
-	if (unlikely(par->debug & level))                    \
-		dev_info(par->info->device, format, ##arg);  \
+	if (unlikely((par)->debug & (level)))                    \
+		dev_info((par)->info->device, format, ##arg);  \
 } while (0)
 
 #define fbtft_par_dbg_hex(level, par, dev, type, buf, num, format, arg...) \
-- 
2.17.1

Re: [Outreachy kernel] [PATCH 4/4] staging: fbtft: Avoid potential precedence issues

[Stefano Brivio]

By "carrying" the tag I really meant that you would, here (not in the
cover letter), add:

On Mon, 16 Mar 2020 05:12:32 +0530
Deepak R Varma <mh12gx2825@gmail.com> wrote:

> Put parentheses around uses of macro parameters to avoid possible
> precedence issues. Problem detected by checkpatch.
> 
> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>

	"Reviewed-by: Stefano Brivio <sbrivio@redhat.com>"

after your Signed-off-by. Mind, however, that you should only do this
in case the reviewer tells you explicitly (I've seen it being done
implicitly, with myself and others, it's really not nice).

In any case,

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

(will check the rest later).

-- 
Stefano

Re: [Outreachy kernel] [PATCH 4/4] staging: fbtft: Avoid potential precedence issues

[Deepak Varma]

[-- Attachment #1.1: Type: text/plain, Size: 995 bytes --]



On Monday, 16 March 2020 19:14:18 UTC+5:30, Stefano Brivio wrote:
>
> By "carrying" the tag I really meant that you would, here (not in the 
> cover letter), add: 
>
> On Mon, 16 Mar 2020 05:12:32 +0530 
> Deepak R Varma <mh12g...@gmail.com <javascript:>> wrote: 
>
> > Put parentheses around uses of macro parameters to avoid possible 
> > precedence issues. Problem detected by checkpatch. 
> > 
> > Signed-off-by: Deepak R Varma <mh12g...@gmail.com <javascript:>> 
>
>         "Reviewed-by: Stefano Brivio <sbr...@redhat.com <javascript:>>" 
>
> after your Signed-off-by. Mind, however, that you should only do this 
> in case the reviewer tells you explicitly (I've seen it being done 
> implicitly, with myself and others, it's really not nice). 
>
> In any case, 
>
> Reviewed-by: Stefano Brivio <sbr...@redhat.com <javascript:>> 
>
> (will check the rest later). 
>
> -- 
> Stefano 
>
My apologies for any confusion. I learned it now. will take care going 
forward.

Thank you,
Deepak.

[-- Attachment #1.2: Type: text/html, Size: 2110 bytes --]

Re: [PATCH 4/4] staging: fbtft: Avoid potential precedence issues

[Greg KH]

On Mon, Mar 16, 2020 at 05:12:32AM +0530, Deepak R Varma wrote:
> Put parentheses around uses of macro parameters to avoid possible
> precedence issues. Problem detected by checkpatch.
> 
> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  drivers/staging/fbtft/fbtft.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> index 81da30f4062e..76f8c090a837 100644
> --- a/drivers/staging/fbtft/fbtft.h
> +++ b/drivers/staging/fbtft/fbtft.h
> @@ -406,8 +406,8 @@ do {                                                         \
>  
>  #define fbtft_par_dbg(level, par, format, arg...)            \
>  do {                                                         \
> -	if (unlikely(par->debug & level))                    \
> -		dev_info(par->info->device, format, ##arg);  \
> +	if (unlikely((par)->debug & (level)))                    \
> +		dev_info((par)->info->device, format, ##arg);  \

"precedence issues" are impossible to happen here, it's a pointer that
we are dealing with, not some other type of expression.

So I'll take this, just to shut checkpatch up, but note that this is
_NOT_ a real issue at all.

And what about all of the other macros like this in this file?  Does
only this one need the changes?

thanks,

greg k-h

Re: [PATCH 4/4] staging: fbtft: Avoid potential precedence issues

[DEEPAK VARMA]

On Tue, Mar 17, 2020 at 12:46:14PM +0100, Greg KH wrote:
> On Mon, Mar 16, 2020 at 05:12:32AM +0530, Deepak R Varma wrote:
> > Put parentheses around uses of macro parameters to avoid possible
> > precedence issues. Problem detected by checkpatch.
> > 
> > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> > Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  drivers/staging/fbtft/fbtft.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> > index 81da30f4062e..76f8c090a837 100644
> > --- a/drivers/staging/fbtft/fbtft.h
> > +++ b/drivers/staging/fbtft/fbtft.h
> > @@ -406,8 +406,8 @@ do {                                                         \
> >  
> >  #define fbtft_par_dbg(level, par, format, arg...)            \
> >  do {                                                         \
> > -	if (unlikely(par->debug & level))                    \
> > -		dev_info(par->info->device, format, ##arg);  \
> > +	if (unlikely((par)->debug & (level)))                    \
> > +		dev_info((par)->info->device, format, ##arg);  \
> 
> "precedence issues" are impossible to happen here, it's a pointer that
> we are dealing with, not some other type of expression.
> 
> So I'll take this, just to shut checkpatch up, but note that this is
> _NOT_ a real issue at all.

Thank you Greg.

> 
> And what about all of the other macros like this in this file?  Does
> only this one need the changes?

There is one more macro that accepts arguments, but its already fixed in
the past. I did not find any other macro with similar checkpatch
warning.

> 
> thanks,
> 
> greg k-h

Re: [PATCH 4/4] staging: fbtft: Avoid potential precedence issues

[DEEPAK VARMA]

On Tue, Mar 17, 2020 at 12:46:14PM +0100, Greg KH wrote:
> On Mon, Mar 16, 2020 at 05:12:32AM +0530, Deepak R Varma wrote:
> > Put parentheses around uses of macro parameters to avoid possible
> > precedence issues. Problem detected by checkpatch.
> > 
> > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> > Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  drivers/staging/fbtft/fbtft.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> > index 81da30f4062e..76f8c090a837 100644
> > --- a/drivers/staging/fbtft/fbtft.h
> > +++ b/drivers/staging/fbtft/fbtft.h
> > @@ -406,8 +406,8 @@ do {                                                         \
> >  
> >  #define fbtft_par_dbg(level, par, format, arg...)            \
> >  do {                                                         \
> > -	if (unlikely(par->debug & level))                    \
> > -		dev_info(par->info->device, format, ##arg);  \
> > +	if (unlikely((par)->debug & (level)))                    \
> > +		dev_info((par)->info->device, format, ##arg);  \
> 
> "precedence issues" are impossible to happen here, it's a pointer that
> we are dealing with, not some other type of expression.
> 
> So I'll take this, just to shut checkpatch up, but note that this is
> _NOT_ a real issue at all.
> 
> And what about all of the other macros like this in this file?  Does
> only this one need the changes?
> 
No Greg, other macros are either taken care of earlier or do not need
similar consideration.

Thank you.

> thanks,
> 
> greg k-h