Re: [PATCH v2 1/1] staging: fbtft: line over 80 characters.

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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