linux-kernel - Re: [PATCH] staging: pi433: rf69.c: Replace macros READ_REG and WRITE_REG with inline functions rf69_set_bit, rf69_reset_bit and rf69_read_modify

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20171204192938.3zxfnb4vscflsjds@mwanda>
Date:   Mon, 4 Dec 2017 22:29:38 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Marcus Wolf <linux@...f-entwicklungen.de>
Cc:     gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: pi433: rf69.c: Replace macros READ_REG and
 WRITE_REG with inline functions rf69_set_bit, rf69_reset_bit and
 rf69_read_modify_write

The subject is way too long.

On Mon, Dec 04, 2017 at 08:18:51PM +0200, Marcus Wolf wrote:
> To increase the readability of the register accesses, the abstraction
> of the helpers was increased from simple read and write to set bit,
> reset bit and read modify write bit. In addition - according to the
> proposal from Walter Harms from 20.07.2017 - instead of marcros inline
> functions were used.
> 
> Signed-off-by: Marcus Wolf <linux@...f-entwicklungen.de>
> ---
>  drivers/staging/pi433/rf69.c |  340 ++++++++++++++++++++++--------------------
>  1 file changed, 182 insertions(+), 158 deletions(-)
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e69a215..8a31ed7 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -33,8 +33,32 @@
>  
>  /*-------------------------------------------------------------------------*/
>  
> -#define READ_REG(x)	rf69_read_reg (spi, x)
> -#define WRITE_REG(x, y)	rf69_write_reg(spi, x, y)
> +static inline int rf69_set_bit(struct spi_device *spi, u8 reg, u8 mask)
          ^^^^^^
Remove the inline.  Leave that for the compiler to decide.


> +{
> +	u8 tmpVal;

Use checkpatch.pl.  No camelCase.

> +
> +	tmpVal = rf69_read_reg (spi, reg);
                              ^
Remove this space.

> +	tmpVal = tmpVal | mask;
> +	return rf69_write_reg(spi, reg, tmpVal);
> +}
> +
> +static inline int rf69_reset_bit(struct spi_device *spi, u8 reg, u8 mask)
                          ^^^^^
Change the name to rf69_clear_bit().  That matches the rest of kernel
naming.  "reset" is ambigous to me.

> +{
> +	u8 tmpVal;
> +
> +	tmpVal = rf69_read_reg (spi, reg);
> +	tmpVal = tmpVal & ~mask;
> +	return rf69_write_reg(spi, reg, tmpVal);
> +}
> +
> +static inline int rf69_read_modify_write(struct spi_device *spi, u8 reg, u8 mask, u8 value)
> +{
> +	u8 tmpVal;
> +
> +	tmpVal = rf69_read_reg (spi, reg);
> +	tmpVal = (tmpVal & ~mask) | value;
> +	return rf69_write_reg(spi, reg, tmpVal);
> +}
>  
>  /*-------------------------------------------------------------------------*/
>  
> @@ -45,11 +69,11 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
>  	#endif
>  
>  	switch (mode) {
> -	case transmit:	  return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | OPMODE_MODE_TRANSMIT);
> -	case receive:	  return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | OPMODE_MODE_RECEIVE);
> -	case synthesizer: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | OPMODE_MODE_SYNTHESIZER);
> -	case standby:	  return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | OPMODE_MODE_STANDBY);
> -	case mode_sleep:  return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | OPMODE_MODE_SLEEP);
> +	case transmit:	  return rf69_read_modify_write(spi, REG_OPMODE, MASK_OPMODE_MODE, OPMODE_MODE_TRANSMIT);
> +	case receive:	  return rf69_read_modify_write(spi, REG_OPMODE, MASK_OPMODE_MODE, OPMODE_MODE_RECEIVE);
> +	case synthesizer: return rf69_read_modify_write(spi, REG_OPMODE, MASK_OPMODE_MODE, OPMODE_MODE_SYNTHESIZER);
> +	case standby:	  return rf69_read_modify_write(spi, REG_OPMODE, MASK_OPMODE_MODE, OPMODE_MODE_STANDBY);
> +	case mode_sleep:  return rf69_read_modify_write(spi, REG_OPMODE, MASK_OPMODE_MODE, OPMODE_MODE_SLEEP);

All these lines are over 80 characters long.  The new
rf69_read_modify_write() function name is too many characters.  We could
probably change the names to rf69_read(), rf69_write() and
rf69_read_mod_write().

> @@ -137,7 +161,7 @@ int rf69_set_modulation_shaping(struct spi_device *spi, enum modShaping modShapi
>  	}
>  }
>  
> -int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate)
> +int deviceName_rate(struct spi_device *spi, u16 bitRate)

CamelCase

>  {
>  	int retval;
>  	u32 bitRate_min;
> @@ -152,7 +176,7 @@ int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate)
>  	// check input value
>  	bitRate_min = F_OSC / 8388608; // 8388608 = 2^23;
>  	if (bitRate < bitRate_min) {
> -		dev_dbg(&spi->dev, "setBitRate: illegal input param");
> +		dev_dbg(&spi->dev, "rf69_set_bitRate: illegal input param");

Just use __func__