Re: [PATCH] staging: mt7621-spi: Fix Coding style issues reported by checkpatch.pl.

[NeilBrown <neil@brown.name>]

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

On Fri, Jun 01 2018, Sankalp Negi wrote:

> This patch fixes following checkpatch.pl issues:
>
> WARNING : line over 80 characters
> ERROR   : code indent should use tabs where possible
> WARNING : no spaces at the start of a line
> ERROR   : switch and case should be at the same indent
> ERROR   : space required before the open parenthesis
> WARNING : braces {} are not necessary for single statement blocks
>
> Signed-off-by: Sankalp Negi <sankalpnegi2310@gmail.com>
> ---
>  drivers/staging/mt7621-spi/spi-mt7621.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/mt7621-spi/spi-mt7621.c b/drivers/staging/mt7621-spi/spi-mt7621.c
> index 37f299080410..cd94f5f569df 100644
> --- a/drivers/staging/mt7621-spi/spi-mt7621.c
> +++ b/drivers/staging/mt7621-spi/spi-mt7621.c
> @@ -55,7 +55,8 @@
>  #define MT7621_CPOL		BIT(4)
>  #define MT7621_LSB_FIRST	BIT(3)
>  
> -#define RT2880_SPI_MODE_BITS	(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST | SPI_CS_HIGH)
> +#define RT2880_SPI_MODE_BITS	(SPI_CPOL | SPI_CPHA |		\
> +				SPI_LSB_FIRST | SPI_CS_HIGH)
>

Thanks for this.  It all looks good except that above.  I'm a bit picky
about indentation, and more picky about making the code easy to read.
The above breaks an indentation rule and (I think) hurts readability.
It was only just over 80 columns so it wasn't all that bad as it was -
let's be sure to make it better.
Some options:

#define RT2880_SPI_MODE_BITS (SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST | SPI_CS_HIGH)

i.e. replace the tab with a space.  Now it doesn't line up with the
previous lines, but I'm not sure that matters much.

#define RT2880_SPI_MODE_BITS	\
	(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST | SPI_CS_HIGH)

This keeps all the content together on one line.

#define RT2880_SPI_MODE_BITS	(SPI_CPOL | SPI_CPHA |		\
				 SPI_LSB_FIRST | SPI_CS_HIGH)

This is close to what you had, but doesn't break the indenting rule:
everything inside brackets must be to the right of the opening bracket
unless that opening bracket is at the end of a line.

Any of these would be acceptable - my personal preference is the second
one.

Thanks,
NeilBrown


>  struct mt7621_spi;
>  
> @@ -104,7 +105,7 @@ static void mt7621_spi_set_cs(struct spi_device *spi, int enable)
>  	int cs = spi->chip_select;
>  	u32 polar = 0;
>  
> -        mt7621_spi_reset(rs, cs);
> +	mt7621_spi_reset(rs, cs);
>  	if (enable)
>  		polar = BIT(cs);
>  	mt7621_spi_write(rs, MT7621_SPI_POLAR, polar);
> @@ -137,18 +138,18 @@ static int mt7621_spi_prepare(struct spi_device *spi, unsigned int speed)
>  		reg |= MT7621_LSB_FIRST;
>  
>  	reg &= ~(MT7621_CPHA | MT7621_CPOL);
> -	switch(spi->mode & (SPI_CPOL | SPI_CPHA)) {
> -		case SPI_MODE_0:
> -			break;
> -		case SPI_MODE_1:
> -			reg |= MT7621_CPHA;
> -			break;
> -		case SPI_MODE_2:
> -			reg |= MT7621_CPOL;
> -			break;
> -		case SPI_MODE_3:
> -			reg |= MT7621_CPOL | MT7621_CPHA;
> -			break;
> +	switch (spi->mode & (SPI_CPOL | SPI_CPHA)) {
> +	case SPI_MODE_0:
> +		break;
> +	case SPI_MODE_1:
> +		reg |= MT7621_CPHA;
> +		break;
> +	case SPI_MODE_2:
> +		reg |= MT7621_CPOL;
> +		break;
> +	case SPI_MODE_3:
> +		reg |= MT7621_CPOL | MT7621_CPHA;
> +		break;
>  	}
>  	mt7621_spi_write(rs, MT7621_SPI_MASTER, reg);
>  
> @@ -164,9 +165,8 @@ static inline int mt7621_spi_wait_till_ready(struct spi_device *spi)
>  		u32 status;
>  
>  		status = mt7621_spi_read(rs, MT7621_SPI_TRANS);
> -		if ((status & SPITRANS_BUSY) == 0) {
> +		if ((status & SPITRANS_BUSY) == 0)
>  			return 0;
> -		}
>  		cpu_relax();
>  		udelay(1);
>  	}
> -- 
> 2.11.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]