[PATCH 1/2] Staging: fbtft: Added blank line after declaration

[PATCH 1/2] Staging: fbtft: Added blank line after declaration

[aybuke ozdemir]

WARNING: Missing a blank line after declaration
chackpatch.pl warning in fb_ssd1351.c

Signed-off-by: aybuke ozdemir <aybuke.147@gmail.com>
---
 drivers/staging/fbtft/fb_ssd1351.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/fbtft/fb_ssd1351.c b/drivers/staging/fbtft/fb_ssd1351.c
index 062d986..b59120c 100644
--- a/drivers/staging/fbtft/fb_ssd1351.c
+++ b/drivers/staging/fbtft/fb_ssd1351.c
@@ -72,6 +72,7 @@ static void set_addr_win(struct fbtft_par *par, int xs, int ys, int xe, int ye)
 static int set_var(struct fbtft_par *par)
 {
 	unsigned remap;
+
 	fbtft_par_dbg(DEBUG_INIT_DISPLAY, par, "%s()\n", __func__);
 
 	if (par->fbtftops.init_display != init_display) {
-- 
1.9.1

[PATCH 2/2] Staging: fbtft: Fix struct backlight_ops should normally be const

[aybuke ozdemir]

This patch fixes following checkpatch.pl warningi in fb_ssd1351.c:
WARNING: struct backlight_ops should normally be const

Signed-off-by: aybuke ozdemir <aybuke.147@gmail.com>
---
 drivers/staging/fbtft/fb_ssd1351.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fbtft/fb_ssd1351.c b/drivers/staging/fbtft/fb_ssd1351.c
index b59120c..bc4d188 100644
--- a/drivers/staging/fbtft/fb_ssd1351.c
+++ b/drivers/staging/fbtft/fb_ssd1351.c
@@ -212,11 +212,11 @@ static void register_onboard_backlight(struct fbtft_par *par)
 {
 	struct backlight_device *bd;
 	struct backlight_properties bl_props = { 0, };
-	struct backlight_ops *bl_ops;
+	const struct backlight_ops *bl_ops;
 
 	fbtft_par_dbg(DEBUG_BACKLIGHT, par, "%s()\n", __func__);
 
-	bl_ops = devm_kzalloc(par->info->device, sizeof(struct backlight_ops),
+	bl_ops = devm_kzalloc(par->info->device, sizeof(const struct backlight_ops),
 				GFP_KERNEL);
 	if (!bl_ops) {
 		dev_err(par->info->device,
-- 
1.9.1

Re: [Outreachy kernel] [PATCH 2/2] Staging: fbtft: Fix struct backlight_ops should normally be const

[Laurent Pinchart]

Hi Aybuke,

Thank you for the patch.

On Thursday 19 February 2015 20:42:44 aybuke ozdemir wrote:
> This patch fixes following checkpatch.pl warningi in fb_ssd1351.c:
> WARNING: struct backlight_ops should normally be const

A good commit message is split in two parts:

- The subject line should summarize what the commit does. In this case, you 
could say "staging: fbtft: Make struct backlight_ops const".

- The commit message body should explain why the change is needed. There's a 
reason behind checkpatch warnings, they indicate issues with the code. You 
should try to understand why a non-const struct backlight_ops is an issue, and 
explain it. In this case the reason why struct backlight_ops is usually const 
would lead to a totally different fix than the one below.

> Signed-off-by: aybuke ozdemir <aybuke.147@gmail.com>
> ---
>  drivers/staging/fbtft/fb_ssd1351.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/fbtft/fb_ssd1351.c
> b/drivers/staging/fbtft/fb_ssd1351.c index b59120c..bc4d188 100644
> --- a/drivers/staging/fbtft/fb_ssd1351.c
> +++ b/drivers/staging/fbtft/fb_ssd1351.c
> @@ -212,11 +212,11 @@ static void register_onboard_backlight(struct
> fbtft_par *par) {
>  	struct backlight_device *bd;
>  	struct backlight_properties bl_props = { 0, };
> -	struct backlight_ops *bl_ops;
> +	const struct backlight_ops *bl_ops;

This introduces a compilation error as the code later assigns bl_ops-
>update_status. Please make sure you always at least compile the code before 
submitting a patch.

>  	fbtft_par_dbg(DEBUG_BACKLIGHT, par, "%s()\n", __func__);
> 
> -	bl_ops = devm_kzalloc(par->info->device, sizeof(struct backlight_ops),
> +	bl_ops = devm_kzalloc(par->info->device, sizeof(const struct
> backlight_ops),

The kernel coding style tends to favour sizeof(var) instead of sizeof(type). 
In this case this would become sizeof(*bl_ops). This ensures that the size 
stays correct if the type of the variable is changed.

> GFP_KERNEL);
>  	if (!bl_ops) {
>  		dev_err(par->info->device,

-- 
Regards,

Laurent Pinchart

Re: [Outreachy kernel] [PATCH 2/2] Staging: fbtft: Fix struct backlight_ops should normally be const

[aybüke özdemir]

[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]

Hi Laurent,

I will pay much more attention to the commit messages and subject line. I use
this command while compiling this file: make M=drivers/staging/fbtft
I didn't encounter a problem. I guess, I'm using the wrong the "make"
command.
Thanks :)


2015-02-19 20:52 GMT+02:00 Laurent Pinchart <
laurent.pinchart@ideasonboard.com>:

> Hi Aybuke,
>
> Thank you for the patch.
>
> On Thursday 19 February 2015 20:42:44 aybuke ozdemir wrote:
> > This patch fixes following checkpatch.pl warningi in fb_ssd1351.c:
> > WARNING: struct backlight_ops should normally be const
>
> A good commit message is split in two parts:
>
> - The subject line should summarize what the commit does. In this case, you
> could say "staging: fbtft: Make struct backlight_ops const".
>
> - The commit message body should explain why the change is needed. There's
> a
> reason behind checkpatch warnings, they indicate issues with the code. You
> should try to understand why a non-const struct backlight_ops is an issue,
> and
> explain it. In this case the reason why struct backlight_ops is usually
> const
> would lead to a totally different fix than the one below.
>
> > Signed-off-by: aybuke ozdemir <aybuke.147@gmail.com>
> > ---
> >  drivers/staging/fbtft/fb_ssd1351.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/fbtft/fb_ssd1351.c
> > b/drivers/staging/fbtft/fb_ssd1351.c index b59120c..bc4d188 100644
> > --- a/drivers/staging/fbtft/fb_ssd1351.c
> > +++ b/drivers/staging/fbtft/fb_ssd1351.c
> > @@ -212,11 +212,11 @@ static void register_onboard_backlight(struct
> > fbtft_par *par) {
> >       struct backlight_device *bd;
> >       struct backlight_properties bl_props = { 0, };
> > -     struct backlight_ops *bl_ops;
> > +     const struct backlight_ops *bl_ops;
>
> This introduces a compilation error as the code later assigns bl_ops-
> >update_status. Please make sure you always at least compile the code
> before
> submitting a patch.
>
> >       fbtft_par_dbg(DEBUG_BACKLIGHT, par, "%s()\n", __func__);
> >
> > -     bl_ops = devm_kzalloc(par->info->device, sizeof(struct
> backlight_ops),
> > +     bl_ops = devm_kzalloc(par->info->device, sizeof(const struct
> > backlight_ops),
>
> The kernel coding style tends to favour sizeof(var) instead of
> sizeof(type).
> In this case this would become sizeof(*bl_ops). This ensures that the size
> stays correct if the type of the variable is changed.
>
> > GFP_KERNEL);
> >       if (!bl_ops) {
> >               dev_err(par->info->device,
>
> --
> Regards,
>
> Laurent Pinchart
>
>


-- 
Aybüke Özdemir
http://www.aybukeozdemir.com

[-- Attachment #2: Type: text/html, Size: 4229 bytes --]

Re: [Outreachy kernel] [PATCH 2/2] Staging: fbtft: Fix struct backlight_ops should normally be const

[Laurent Pinchart]

Hi Aybüke,

On Thursday 19 February 2015 21:43:22 aybüke özdemir wrote:
> Hi Laurent,
> 
> I will pay much more attention to the commit messages and subject line. I
> use this command while compiling this file: make M=drivers/staging/fbtft I
> didn't encounter a problem. I guess, I'm using the wrong the "make"
> command.

The command is correct, but you also need to make sure that the driver is 
selected in the kernel configuration, otherwise it won't get compiled.

A good way to find out whether a .c source file has been compiled is to check 
that a .o file with the same base name has been created in the same directory. 
The kernel build system should also print

  CC      drivers/staging/fbtft/fb_ssd1351.o

during compilation.

-- 
Regards,

Laurent Pinchart

Re: [Outreachy kernel] [PATCH 2/2] Staging: fbtft: Fix struct backlight_ops should normally be const

[aybüke özdemir]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

Many thanks for clarifying, When I compile , I will consider your
description :)

2015-02-19 22:13 GMT+02:00 Laurent Pinchart <
laurent.pinchart@ideasonboard.com>:

> Hi Aybüke,
>
> On Thursday 19 February 2015 21:43:22 aybüke özdemir wrote:
> > Hi Laurent,
> >
> > I will pay much more attention to the commit messages and subject line. I
> > use this command while compiling this file: make M=drivers/staging/fbtft
> I
> > didn't encounter a problem. I guess, I'm using the wrong the "make"
> > command.
>
> The command is correct, but you also need to make sure that the driver is
> selected in the kernel configuration, otherwise it won't get compiled.
>
> A good way to find out whether a .c source file has been compiled is to
> check
> that a .o file with the same base name has been created in the same
> directory.
> The kernel build system should also print
>
>   CC      drivers/staging/fbtft/fb_ssd1351.o
>
> during compilation.
>
> --
> Regards,
>
> Laurent Pinchart
>
>


-- 
Aybüke Özdemir
http://www.aybukeozdemir.com

[-- Attachment #2: Type: text/html, Size: 2076 bytes --]