* [PATCH v2 1/1] staging: fbtft: line over 80 characters.
@ 2019-03-04 11:17 Bhagyashri Dighole
2019-03-04 11:50 ` [Outreachy kernel] " Julia Lawall
2019-03-05 7:46 ` Greg Kroah-Hartman
0 siblings, 2 replies; 5+ messages in thread
From: Bhagyashri Dighole @ 2019-03-04 11:17 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Outreachy
Fix coding style issues, which solves checkpatch.pl warning:
"WARNING: line over 80 characters"
Signed-off-by: Bhagyashri Dighole <digholebhagyashri@gmail.com>
---
Changes in v2:
- Improve code order.
- Make the code more readable.
drivers/staging/fbtft/fb_ssd1306.c | 3 ++-
drivers/staging/fbtft/fb_ssd1331.c | 3 ++-
drivers/staging/fbtft/fbtft-sysfs.c | 3 ++-
drivers/staging/fbtft/fbtft.h | 20 ++++++++++++++------
drivers/staging/fbtft/flexfb.c | 12 ++++++++----
5 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/fbtft/fb_ssd1306.c b/drivers/staging/fbtft/fb_ssd1306.c
index d7c5e2e..412871b 100644
--- a/drivers/staging/fbtft/fb_ssd1306.c
+++ b/drivers/staging/fbtft/fb_ssd1306.c
@@ -184,7 +184,8 @@ static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
for (y = 0; y < yres / 8; y++) {
*buf = 0x00;
for (i = 0; i < 8; i++)
- *buf |= (vmem16[(y * 8 + i) * xres + x] ? 1 : 0) << i;
+ *buf |= (vmem16[(y * 8 + i) * xres + x] ?
+ 1 : 0) << i;
buf++;
}
}
diff --git a/drivers/staging/fbtft/fb_ssd1331.c b/drivers/staging/fbtft/fb_ssd1331.c
index 9f54fe2..4cfe9f8 100644
--- a/drivers/staging/fbtft/fb_ssd1331.c
+++ b/drivers/staging/fbtft/fb_ssd1331.c
@@ -74,7 +74,8 @@ static void write_reg8_bus8(struct fbtft_par *par, int len, ...)
for (i = 0; i < len; i++)
buf[i] = (u8)va_arg(args, unsigned int);
va_end(args);
- fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device, u8, buf, len, "%s: ", __func__);
+ fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device,
+ u8, buf, len, "%s: ", __func__);
}
va_start(args, len);
diff --git a/drivers/staging/fbtft/fbtft-sysfs.c b/drivers/staging/fbtft/fbtft-sysfs.c
index 2a5c630..26c971f 100644
--- a/drivers/staging/fbtft/fbtft-sysfs.c
+++ b/drivers/staging/fbtft/fbtft-sysfs.c
@@ -68,7 +68,8 @@ 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;
+ curves[curve_counter * par->gamma.num_values + value_counter]
+ = val;
value_counter++;
}
if (value_counter != par->gamma.num_values) {
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 7fdd3b0..7b66926 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -347,9 +347,15 @@ 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_VERI Y_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
@@ -390,9 +396,11 @@ module_exit(fbtft_driver_module_exit);
#define fbtft_init_dbg(dev, format, arg...) \
do { \
- if (unlikely((dev)->platform_data && \
- (((struct fbtft_platform_data *)(dev)->platform_data)->display.debug & DEBUG_DRIVER_INIT_FUNCTIONS))) \
- dev_info(dev, format, ##arg); \
+ if (unlikely((dev)->platform_data && \
+ (((struct fbtft_platform_data *) \
+ (dev)->platform_data)->display.debug \
+ & DEBUG_DRIVER_INIT_FUNCTIONS))) \
+ dev_info(dev, format, ##arg);
} while (0)
#define fbtft_par_dbg(level, par, format, arg...) \
diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
index c5fa591..48e9c73 100644
--- a/drivers/staging/fbtft/flexfb.c
+++ b/drivers/staging/fbtft/flexfb.c
@@ -667,11 +667,13 @@ static int flexfb_probe_common(struct spi_device *sdev,
case 8:
par->fbtftops.write_vmem = fbtft_write_vmem16_bus8;
if (!par->startbyte)
- par->fbtftops.verify_gpios = flexfb_verify_gpios_dc;
+ par->fbtftops.verify_gpios =
+ flexfb_verify_gpios_dc;
break;
case 9:
if (regwidth == 16) {
- dev_err(dev, "argument 'regwidth': %d is not supported with buswidth=%d and SPI.\n", regwidth, buswidth);
+ dev_err(dev, "argument 'regwidth': %d is not supported with buswidth=%d and SPI.\n",
+ regwidth, buswidth);
return -EINVAL;
}
par->fbtftops.write_register = fbtft_write_reg8_bus9;
@@ -712,13 +714,15 @@ static int flexfb_probe_common(struct spi_device *sdev,
case 16:
par->fbtftops.write_register = fbtft_write_reg16_bus16;
if (latched)
- par->fbtftops.write = fbtft_write_gpio16_wr_latched;
+ par->fbtftops.write =
+ fbtft_write_gpio16_wr_latched;
else
par->fbtftops.write = fbtft_write_gpio16_wr;
par->fbtftops.write_vmem = fbtft_write_vmem16_bus16;
break;
default:
- dev_err(dev, "argument 'buswidth': %d is not supported with parallel.\n", buswidth);
+ dev_err(dev, "argument 'buswidth': %d is not supported with parallel.\n",
+ buswidth);
return -EINVAL;
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Outreachy kernel] [PATCH v2 1/1] staging: fbtft: line over 80 characters.
2019-03-04 11:17 [PATCH v2 1/1] staging: fbtft: line over 80 characters Bhagyashri Dighole
@ 2019-03-04 11:50 ` Julia Lawall
2019-03-04 18:10 ` Bhagyashri Dighole
2019-03-05 7:46 ` Greg Kroah-Hartman
1 sibling, 1 reply; 5+ messages in thread
From: Julia Lawall @ 2019-03-04 11:50 UTC (permalink / raw)
To: Bhagyashri Dighole; +Cc: Greg Kroah-Hartman, Outreachy
> #define fbtft_init_dbg(dev, format, arg...) \
> do { \
> - if (unlikely((dev)->platform_data && \
> - (((struct fbtft_platform_data *)(dev)->platform_data)->display.debug & DEBUG_DRIVER_INIT_FUNCTIONS))) \
> - dev_info(dev, format, ##arg); \
> + if (unlikely((dev)->platform_data && \
> + (((struct fbtft_platform_data *) \
> + (dev)->platform_data)->display.debug \
> + & DEBUG_DRIVER_INIT_FUNCTIONS))) \
> + dev_info(dev, format, ##arg);
> } while (0)
This one still doesn't look good, because you have separated the cast and
the casted thing. One wants to be able to see easily what is going on.
Given that there are so many parentheses, it would be better to keep the
cast and the casted thing on the same line, to see what is what.
julia
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Outreachy kernel] [PATCH v2 1/1] staging: fbtft: line over 80 characters.
2019-03-04 11:50 ` [Outreachy kernel] " Julia Lawall
@ 2019-03-04 18:10 ` Bhagyashri Dighole
0 siblings, 0 replies; 5+ messages in thread
From: Bhagyashri Dighole @ 2019-03-04 18:10 UTC (permalink / raw)
To: Julia Lawall; +Cc: Greg Kroah-Hartman, Outreachy
[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]
If i made change according to suggestion, It will give same warning `line
over 80 character`. As per my knowledge to avoid this we need to separate
cast and casted thing.
Thanks
Bhagyashri
On Mon, Mar 4, 2019 at 5:20 PM Julia Lawall <julia.lawall@lip6.fr> wrote:
> > #define fbtft_init_dbg(dev, format, arg...) \
> > do { \
> > - if (unlikely((dev)->platform_data && \
> > - (((struct fbtft_platform_data
> *)(dev)->platform_data)->display.debug & DEBUG_DRIVER_INIT_FUNCTIONS))) \
> > - dev_info(dev, format, ##arg); \
> > + if (unlikely((dev)->platform_data && \
> > + (((struct fbtft_platform_data *) \
> > + (dev)->platform_data)->display.debug \
> > + & DEBUG_DRIVER_INIT_FUNCTIONS))) \
> > + dev_info(dev, format, ##arg);
> > } while (0)
>
> This one still doesn't look good, because you have separated the cast and
> the casted thing. One wants to be able to see easily what is going on.
> Given that there are so many parentheses, it would be better to keep the
> cast and the casted thing on the same line, to see what is what.
>
> julia
>
> --
> 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/alpine.DEB.2.20.1903041248180.4370%40hadrien
> .
> For more options, visit https://groups.google.com/d/optout.
>
[-- Attachment #2: Type: text/html, Size: 2808 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] staging: fbtft: line over 80 characters.
2019-03-04 11:17 [PATCH v2 1/1] staging: fbtft: line over 80 characters Bhagyashri Dighole
2019-03-04 11:50 ` [Outreachy kernel] " Julia Lawall
@ 2019-03-05 7:46 ` Greg Kroah-Hartman
2019-03-05 9:33 ` Bhagyashri Dighole
1 sibling, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2019-03-05 7:46 UTC (permalink / raw)
To: Bhagyashri Dighole; +Cc: Outreachy
On Mon, Mar 04, 2019 at 04:47:54PM +0530, Bhagyashri Dighole wrote:
> Fix coding style issues, which solves checkpatch.pl warning:
> "WARNING: line over 80 characters"
>
> Signed-off-by: Bhagyashri Dighole <digholebhagyashri@gmail.com>
> ---
> Changes in v2:
> - Improve code order.
> - Make the code more readable.
>
> drivers/staging/fbtft/fb_ssd1306.c | 3 ++-
> drivers/staging/fbtft/fb_ssd1331.c | 3 ++-
> drivers/staging/fbtft/fbtft-sysfs.c | 3 ++-
> drivers/staging/fbtft/fbtft.h | 20 ++++++++++++++------
> drivers/staging/fbtft/flexfb.c | 12 ++++++++----
> 5 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/fbtft/fb_ssd1306.c b/drivers/staging/fbtft/fb_ssd1306.c
> index d7c5e2e..412871b 100644
> --- a/drivers/staging/fbtft/fb_ssd1306.c
> +++ b/drivers/staging/fbtft/fb_ssd1306.c
> @@ -184,7 +184,8 @@ static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
> for (y = 0; y < yres / 8; y++) {
> *buf = 0x00;
> for (i = 0; i < 8; i++)
> - *buf |= (vmem16[(y * 8 + i) * xres + x] ? 1 : 0) << i;
> + *buf |= (vmem16[(y * 8 + i) * xres + x] ?
> + 1 : 0) << i;
This is now harder to read, the original was fine.
Well, it was horrible, the whole ? : style is a mess and impossible to
read. I would just rewrite that as a proper if() statement as I have no
idea what the code is doing when I first look at it :(
> buf++;
> }
> }
> diff --git a/drivers/staging/fbtft/fb_ssd1331.c b/drivers/staging/fbtft/fb_ssd1331.c
> index 9f54fe2..4cfe9f8 100644
> --- a/drivers/staging/fbtft/fb_ssd1331.c
> +++ b/drivers/staging/fbtft/fb_ssd1331.c
> @@ -74,7 +74,8 @@ static void write_reg8_bus8(struct fbtft_par *par, int len, ...)
> for (i = 0; i < len; i++)
> buf[i] = (u8)va_arg(args, unsigned int);
> va_end(args);
> - fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device, u8, buf, len, "%s: ", __func__);
> + fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device,
> + u8, buf, len, "%s: ", __func__);
This is fine.
> }
>
> va_start(args, len);
> diff --git a/drivers/staging/fbtft/fbtft-sysfs.c b/drivers/staging/fbtft/fbtft-sysfs.c
> index 2a5c630..26c971f 100644
> --- a/drivers/staging/fbtft/fbtft-sysfs.c
> +++ b/drivers/staging/fbtft/fbtft-sysfs.c
> @@ -68,7 +68,8 @@ 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;
> + curves[curve_counter * par->gamma.num_values + value_counter]
> + = val;
Not good, the original is fine.
> value_counter++;
> }
> if (value_counter != par->gamma.num_values) {
> diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
> index 7fdd3b0..7b66926 100644
> --- a/drivers/staging/fbtft/fbtft.h
> +++ b/drivers/staging/fbtft/fbtft.h
> @@ -347,9 +347,15 @@ 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)
We normally write things with the operator at the end of the line, so
this would be:
#define DEBUG_LEVEL_2 (DEBUG_LEVEL_1 | DEBUG_DRIVER_INIT_FUNCTIONS | \
DEBUG_TIME_FIRST_UPDATE)
Also note the tab before the '\' character so you can line them all up
with the other entries below.
> +#define DEBUG_LEVEL_3 (DEBUG_LEVEL_2 | DEBUG_RESET | DEBUG_INIT_DISPLAY \
> + | DEBUG_BLANK | DEBUG_REQUEST_GPIOS \
> + | DEBUG_FREE_GPIOS | DEBUG_VERI Y_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
> @@ -390,9 +396,11 @@ module_exit(fbtft_driver_module_exit);
>
> #define fbtft_init_dbg(dev, format, arg...) \
> do { \
> - if (unlikely((dev)->platform_data && \
> - (((struct fbtft_platform_data *)(dev)->platform_data)->display.debug & DEBUG_DRIVER_INIT_FUNCTIONS))) \
> - dev_info(dev, format, ##arg); \
> + if (unlikely((dev)->platform_data && \
> + (((struct fbtft_platform_data *) \
> + (dev)->platform_data)->display.debug \
> + & DEBUG_DRIVER_INIT_FUNCTIONS))) \
> + dev_info(dev, format, ##arg);
The original code is fine, I would just leave it alone.
> } while (0)
>
> #define fbtft_par_dbg(level, par, format, arg...) \
> diff --git a/drivers/staging/fbtft/flexfb.c b/drivers/staging/fbtft/flexfb.c
> index c5fa591..48e9c73 100644
> --- a/drivers/staging/fbtft/flexfb.c
> +++ b/drivers/staging/fbtft/flexfb.c
> @@ -667,11 +667,13 @@ static int flexfb_probe_common(struct spi_device *sdev,
> case 8:
> par->fbtftops.write_vmem = fbtft_write_vmem16_bus8;
> if (!par->startbyte)
> - par->fbtftops.verify_gpios = flexfb_verify_gpios_dc;
> + par->fbtftops.verify_gpios =
> + flexfb_verify_gpios_dc;
Original is fine.
> break;
> case 9:
> if (regwidth == 16) {
> - dev_err(dev, "argument 'regwidth': %d is not supported with buswidth=%d and SPI.\n", regwidth, buswidth);
> + dev_err(dev, "argument 'regwidth': %d is not supported with buswidth=%d and SPI.\n",
> + regwidth, buswidth);
This change is good.
> return -EINVAL;
> }
> par->fbtftops.write_register = fbtft_write_reg8_bus9;
> @@ -712,13 +714,15 @@ static int flexfb_probe_common(struct spi_device *sdev,
> case 16:
> par->fbtftops.write_register = fbtft_write_reg16_bus16;
> if (latched)
> - par->fbtftops.write = fbtft_write_gpio16_wr_latched;
> + par->fbtftops.write =
> + fbtft_write_gpio16_wr_latched;
Original is fine.
> else
> par->fbtftops.write = fbtft_write_gpio16_wr;
> par->fbtftops.write_vmem = fbtft_write_vmem16_bus16;
> break;
> default:
> - dev_err(dev, "argument 'buswidth': %d is not supported with parallel.\n", buswidth);
> + dev_err(dev, "argument 'buswidth': %d is not supported with parallel.\n",
> + buswidth);
This change is good.
Can you fix this patch up based on these comments and resend?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] staging: fbtft: line over 80 characters.
2019-03-05 7:46 ` Greg Kroah-Hartman
@ 2019-03-05 9:33 ` Bhagyashri Dighole
0 siblings, 0 replies; 5+ messages in thread
From: Bhagyashri Dighole @ 2019-03-05 9:33 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Outreachy
[-- Attachment #1: Type: text/plain, Size: 8778 bytes --]
Yes, I will resend with the necessary changes.
On Tue, Mar 5, 2019 at 1:16 PM Greg Kroah-Hartman <
gregkh@linuxfoundation.org> wrote:
> On Mon, Mar 04, 2019 at 04:47:54PM +0530, Bhagyashri Dighole wrote:
> > Fix coding style issues, which solves checkpatch.pl warning:
> > "WARNING: line over 80 characters"
> >
> > Signed-off-by: Bhagyashri Dighole <digholebhagyashri@gmail.com>
> > ---
> > Changes in v2:
> > - Improve code order.
> > - Make the code more readable.
> >
> > drivers/staging/fbtft/fb_ssd1306.c | 3 ++-
> > drivers/staging/fbtft/fb_ssd1331.c | 3 ++-
> > drivers/staging/fbtft/fbtft-sysfs.c | 3 ++-
> > drivers/staging/fbtft/fbtft.h | 20 ++++++++++++++------
> > drivers/staging/fbtft/flexfb.c | 12 ++++++++----
> > 5 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/staging/fbtft/fb_ssd1306.c
> b/drivers/staging/fbtft/fb_ssd1306.c
> > index d7c5e2e..412871b 100644
> > --- a/drivers/staging/fbtft/fb_ssd1306.c
> > +++ b/drivers/staging/fbtft/fb_ssd1306.c
> > @@ -184,7 +184,8 @@ static int write_vmem(struct fbtft_par *par, size_t
> offset, size_t len)
> > for (y = 0; y < yres / 8; y++) {
> > *buf = 0x00;
> > for (i = 0; i < 8; i++)
> > - *buf |= (vmem16[(y * 8 + i) * xres + x] ?
> 1 : 0) << i;
> > + *buf |= (vmem16[(y * 8 + i) * xres + x] ?
> > + 1 : 0) << i;
>
> This is now harder to read, the original was fine.
>
> Well, it was horrible, the whole ? : style is a mess and impossible to
> read. I would just rewrite that as a proper if() statement as I have no
> idea what the code is doing when I first look at it :(
>
>
> > buf++;
> > }
> > }
> > diff --git a/drivers/staging/fbtft/fb_ssd1331.c
> b/drivers/staging/fbtft/fb_ssd1331.c
> > index 9f54fe2..4cfe9f8 100644
> > --- a/drivers/staging/fbtft/fb_ssd1331.c
> > +++ b/drivers/staging/fbtft/fb_ssd1331.c
> > @@ -74,7 +74,8 @@ static void write_reg8_bus8(struct fbtft_par *par, int
> len, ...)
> > for (i = 0; i < len; i++)
> > buf[i] = (u8)va_arg(args, unsigned int);
> > va_end(args);
> > - fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par,
> par->info->device, u8, buf, len, "%s: ", __func__);
> > + fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par,
> par->info->device,
> > + u8, buf, len, "%s: ", __func__);
>
> This is fine.
>
> > }
> >
> > va_start(args, len);
> > diff --git a/drivers/staging/fbtft/fbtft-sysfs.c
> b/drivers/staging/fbtft/fbtft-sysfs.c
> > index 2a5c630..26c971f 100644
> > --- a/drivers/staging/fbtft/fbtft-sysfs.c
> > +++ b/drivers/staging/fbtft/fbtft-sysfs.c
> > @@ -68,7 +68,8 @@ 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;
> > + curves[curve_counter * par->gamma.num_values +
> value_counter]
> > + = val;
>
> Not good, the original is fine.
>
>
> > value_counter++;
> > }
> > if (value_counter != par->gamma.num_values) {
> > diff --git a/drivers/staging/fbtft/fbtft.h
> b/drivers/staging/fbtft/fbtft.h
> > index 7fdd3b0..7b66926 100644
> > --- a/drivers/staging/fbtft/fbtft.h
> > +++ b/drivers/staging/fbtft/fbtft.h
> > @@ -347,9 +347,15 @@ 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)
>
> We normally write things with the operator at the end of the line, so
> this would be:
>
> #define DEBUG_LEVEL_2 (DEBUG_LEVEL_1 | DEBUG_DRIVER_INIT_FUNCTIONS | \
> DEBUG_TIME_FIRST_UPDATE)
>
> Also note the tab before the '\' character so you can line them all up
> with the other entries below.
>
>
>
> > +#define DEBUG_LEVEL_3 (DEBUG_LEVEL_2 | DEBUG_RESET |
> DEBUG_INIT_DISPLAY \
> > + | DEBUG_BLANK | DEBUG_REQUEST_GPIOS
> \
> > + | DEBUG_FREE_GPIOS | DEBUG_VERI Y_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
> > @@ -390,9 +396,11 @@ module_exit(fbtft_driver_module_exit);
> >
> > #define fbtft_init_dbg(dev, format, arg...) \
> > do { \
> > - if (unlikely((dev)->platform_data && \
> > - (((struct fbtft_platform_data
> *)(dev)->platform_data)->display.debug & DEBUG_DRIVER_INIT_FUNCTIONS))) \
> > - dev_info(dev, format, ##arg); \
> > + if (unlikely((dev)->platform_data && \
> > + (((struct fbtft_platform_data *) \
> > + (dev)->platform_data)->display.debug \
> > + & DEBUG_DRIVER_INIT_FUNCTIONS))) \
> > + dev_info(dev, format, ##arg);
>
> The original code is fine, I would just leave it alone.
>
>
>
> > } while (0)
> >
> > #define fbtft_par_dbg(level, par, format, arg...) \
> > diff --git a/drivers/staging/fbtft/flexfb.c
> b/drivers/staging/fbtft/flexfb.c
> > index c5fa591..48e9c73 100644
> > --- a/drivers/staging/fbtft/flexfb.c
> > +++ b/drivers/staging/fbtft/flexfb.c
> > @@ -667,11 +667,13 @@ static int flexfb_probe_common(struct spi_device
> *sdev,
> > case 8:
> > par->fbtftops.write_vmem = fbtft_write_vmem16_bus8;
> > if (!par->startbyte)
> > - par->fbtftops.verify_gpios =
> flexfb_verify_gpios_dc;
> > + par->fbtftops.verify_gpios =
> > + flexfb_verify_gpios_dc;
>
> Original is fine.
>
> > break;
> > case 9:
> > if (regwidth == 16) {
> > - dev_err(dev, "argument 'regwidth': %d is
> not supported with buswidth=%d and SPI.\n", regwidth, buswidth);
> > + dev_err(dev, "argument 'regwidth': %d is
> not supported with buswidth=%d and SPI.\n",
> > + regwidth, buswidth);
>
> This change is good.
>
> > return -EINVAL;
> > }
> > par->fbtftops.write_register =
> fbtft_write_reg8_bus9;
> > @@ -712,13 +714,15 @@ static int flexfb_probe_common(struct spi_device
> *sdev,
> > case 16:
> > par->fbtftops.write_register =
> fbtft_write_reg16_bus16;
> > if (latched)
> > - par->fbtftops.write =
> fbtft_write_gpio16_wr_latched;
> > + par->fbtftops.write =
> > + fbtft_write_gpio16_wr_latched;
>
> Original is fine.
>
> > else
> > par->fbtftops.write =
> fbtft_write_gpio16_wr;
> > par->fbtftops.write_vmem =
> fbtft_write_vmem16_bus16;
> > break;
> > default:
> > - dev_err(dev, "argument 'buswidth': %d is not
> supported with parallel.\n", buswidth);
> > + dev_err(dev, "argument 'buswidth': %d is not
> supported with parallel.\n",
> > + buswidth);
>
> This change is good.
>
> Can you fix this patch up based on these comments and resend?
>
> thanks,
>
> greg k-h
>
[-- Attachment #2: Type: text/html, Size: 11385 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-05 9:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-04 11:17 [PATCH v2 1/1] staging: fbtft: line over 80 characters Bhagyashri Dighole
2019-03-04 11:50 ` [Outreachy kernel] " Julia Lawall
2019-03-04 18:10 ` Bhagyashri Dighole
2019-03-05 7:46 ` Greg Kroah-Hartman
2019-03-05 9:33 ` Bhagyashri Dighole
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.