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

[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