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

[PATCH v2 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 pathes in this patchset. 

Changes from 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 v2 1/4] staging: fbtft: Reformat line over 80 characters

[Deepak R Varma]

A long variable name beyond 80 characters gets exended into the next
line. Reformating 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

Re: [Outreachy kernel] [PATCH v2 1/4] staging: fbtft: Reformat line over 80 characters

[Stefano Brivio]

Looks good to me, just a few typos I wouldn't normally care about
(unless a patch is fixing typos), but as you might re-post this:

On Fri, 13 Mar 2020 19:12:13 +0530
Deepak R Varma <mh12gx2825@gmail.com> wrote:

> A long variable name beyond 80 characters gets exended into the next

extended

> line. Reformating the line makes it more readable. Also adding an extra

Reformatting

-- 
Stefano

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

[Deepak R Varma]

Multiple macro definitions crossings 80 character line length nakes it
hard to understand. Reformating the these lines makes the code more
readable and easy 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

Re: [Outreachy kernel] [PATCH v2 2/4] staging: fbtft: Reformat long macro definitions

[Stefano Brivio]

Same as 1/4, looks good to me,

On Fri, 13 Mar 2020 19:13:12 +0530
Deepak R Varma <mh12gx2825@gmail.com> wrote:

> Multiple macro definitions crossings 80 character line length nakes it

crossing, makes, them

> hard to understand. Reformating the these lines makes the code more

Reformatting

> readable and easy to understand. Issue flagged by checkpatch script.

easier

-- 
Stefano

[PATCH v2 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 staement
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..4d505a13f7ab 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 counter = 0;
 
 	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;
+
+			counter = curve_counter * par->gamma.num_values +
+					value_counter;
+			curves[counter] = val;
 			value_counter++;
 		}
 		if (value_counter != par->gamma.num_values) {
-- 
2.17.1

Re: [Outreachy kernel] [PATCH v2 3/4] staging: fbtft: simplify array index computation

[Stefano Brivio]

Hmm, this isn't exactly simplifying if one looks at the diffstat:

On Fri, 13 Mar 2020 19:14:02 +0530
Deepak R Varma <mh12gx2825@gmail.com> wrote:

> An array index is being computed by mathematical calculation on the
> Lvalue side of the expression. This also further results in the staement
> 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(-)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

More on that below:

> 
> diff --git a/drivers/staging/fbtft/fbtft-sysfs.c b/drivers/staging/fbtft/fbtft-sysfs.c
> index 2a5c630dab87..4d505a13f7ab 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 counter = 0;

...there's no need to initialise this.

>  
>  	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;
> +
> +			counter = curve_counter * par->gamma.num_values +
> +					value_counter;

value_counter is summed to the whole thing, so it should be aligned
under curve_counter.

> +			curves[counter] = val;

And here, I think, the actual problem is variable naming itself. See
section "4) Naming" of Documentation/process/coding-style.rst -- this
looks a bit like Java. The "_counter" suffix doesn't add much here, I
guess those could just be "curve" and "value". I haven't checked for
ambiguities (could you have a look?), but if there's any, even "_count"
would make this a bit more tolerable.

-- 
Stefano

[PATCH v2 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 v2 4/4] staging: fbtft: Avoid potential precedence issues

[Stefano Brivio]

On Fri, 13 Mar 2020 19:14:54 +0530
Deepak R Varma <mh12gx2825@gmail.com> wrote:

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

This one looks good to me. If you re-post, you can carry my:

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

-- 
Stefano