Re: [PATCH v2] staging: sm750fb: minor coding style cleanup

[Dan Carpenter]

On Mon, May 11, 2026 at 09:54:05PM +0545, Chhabilal Dangal wrote:
> Clean up various coding style issues including spacing in struct initializers and indentation of wrapped lines.
> 

This patch does too many things and quite a few are not really desired.
The changes have to be an obvious improvement, not just a matter of
opinion.  If everyone with an opinion changed the code then we'd just
be changing it back and forth forever.

> Signed-off-by: Alone <yogeshdangal66@xxxxxxxxx>

This need to be your real name.

> ---
>  drivers/staging/sm750fb/sm750.c | 203 ++++++++++++++++----------------
>  1 file changed, 103 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
> index 9f3e3d37e82a..7fca2c9f6966 100644
> --- a/drivers/staging/sm750fb/sm750.c
> +++ b/drivers/staging/sm750fb/sm750.c
> @@ -33,7 +33,8 @@
>  static int g_hwcursor = 1;
>  static int g_noaccel;
>  static int g_nomtrr;
> -static const char *g_fbmode[] = {NULL, NULL};
> +/* intentionally non-const since array is modified at runtime */

This kind of comment is not required since it's obvious.

> +static const char *g_fbmode[] = { NULL, NULL };
>  static const char *g_def_fbmode = "1024x768-32@60";
>  static char *g_settings;
>  static int g_dualview;

[ snip ]

> @@ -120,8 +119,7 @@ static int lynxfb_ops_cursor(struct fb_info *info, struct fb_cursor *fbcursor)
>  
>  	sm750_hw_cursor_disable(cursor);
>  	if (fbcursor->set & FB_CUR_SETSIZE)
> -		sm750_hw_cursor_set_size(cursor,
> -					 fbcursor->image.width,
> +		sm750_hw_cursor_set_size(cursor, fbcursor->image.width,
>  					 fbcursor->image.height);
>  
>  	if (fbcursor->set & FB_CUR_SETPOS)

I'm fine with the original.  I like how "width" and "height" line up.
The truth is that I often split thing up on multiple lines more than
other people.

> @@ -134,19 +132,23 @@ static int lynxfb_ops_cursor(struct fb_info *info, struct fb_cursor *fbcursor)
>  		u16 fg, bg;
>  
>  		fg = ((info->cmap.red[fbcursor->image.fg_color] & 0xf800)) |
> -		     ((info->cmap.green[fbcursor->image.fg_color] & 0xfc00) >> 5) |
> -		     ((info->cmap.blue[fbcursor->image.fg_color] & 0xf800) >> 11);
> +		     ((info->cmap.green[fbcursor->image.fg_color] & 0xfc00) >>
> +		      5) |
> +		     ((info->cmap.blue[fbcursor->image.fg_color] & 0xf800) >>
> +		      11);

This is obviously worse than before.

[ snip ]

> @@ -556,8 +551,7 @@ static int lynxfb_ops_setcolreg(unsigned int regno,
>  	if (info->fix.visual == FB_VISUAL_TRUECOLOR && regno < 256) {
>  		u32 val;
>  
> -		if (var->bits_per_pixel == 16 ||
> -		    var->bits_per_pixel == 32 ||
> +		if (var->bits_per_pixel == 16 || var->bits_per_pixel == 32 ||
>  		    var->bits_per_pixel == 24) {
>  			val = chan_to_field(red, &var->red);
>  			val |= chan_to_field(green, &var->green);
> @@ -616,7 +610,8 @@ static int sm750fb_set_drv(struct lynxfb_par *par)
>  
>  	/* chip specific phase */
>  	sm750_dev->accel.de_wait = (sm750_dev->revid == SM750LE_REVISION_ID) ?
> -				    hw_sm750le_de_wait : hw_sm750_de_wait;
> +					   hw_sm750le_de_wait :
> +					   hw_sm750_de_wait;

These two changes are so weird because on the one you're combining
statements and on the other you're splitting them up.  When both
seems almost the same to me.  It's not consistent.

In the end, the original code was fine in both cases.  Just leave it.
> @@ -1112,8 +1111,12 @@ static int __init lynxfb_setup(char *options)
>  }
>  
>  static const struct pci_device_id smi_pci_table[] = {
> -	{ PCI_DEVICE(0x126f, 0x0750), },
> -	{0,}
> +	{
> +		PCI_DEVICE(0x126f, 0x0750),
> +	},
> +	{
> +		0,
> +	}
>  };

The original was better.

(I haven't reviewed it all.  This is just to give you an idea).

regards,
dan carpenter