linux-kernel - Re: [PATCH] staging: pi433: rf69.c: Replace macros READ_REG and WRITE_REG with inline functions setBit rstBit and rmwBit

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: <20171202150320.GE1242@kroah.com>
Date:   Sat, 2 Dec 2017 15:03:20 +0000
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Marcus Wolf <linux@...f-entwicklungen.de>
Cc:     dan.carpenter@...cle.com, 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 setBit rstBit and rmwBit

On Sat, Dec 02, 2017 at 02:21:32PM +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.
> 
> As a bonus, with this refactoring a lot of lines were shortened a lot. So
> some of them now undershoot 80 chars, thus reducing the total number of
> complaints of checkPatch.pl in rf69.c.
> ---
>  drivers/staging/pi433/rf69.c |  347 ++++++++++++++++++++++--------------------
>  1 file changed, 185 insertions(+), 162 deletions(-)

No signed-off-by line.  Always use scripts/checkpatch.pl so you don't
get grumpy emails from maintainers telling you to use
scripts/checkpatch.pl.

> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index 0df084e..f6d0b82 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -30,13 +30,37 @@
>  #include "rf69.h"
>  #include "rf69_registers.h"
>  
>  #define F_OSC 32000000 /* in Hz */
>  #define FIFO_SIZE 66   /* in byte */
>  
>  /*-------------------------------------------------------------------------*/
>  
> -#define READ_REG(x)	rf69_read_reg (spi, x)
> -#define WRITE_REG(x, y)	rf69_write_reg(spi, x, y)
> +inline static int setBit(struct spi_device *spi, u8 reg, u8 mask)

We have setbit functions, perhaps name this a bit differently as it is
doing something else?  And no interCaps please.

> +{

What kind of crazy git options do you have that it creates so much
context for the diff?

> +	u8 tmpVal;
> +
> +	tmpVal = rf69_read_reg (spi, reg);
> +	tmpVal = tmpVal | mask;
> +	return rf69_write_reg(spi, reg, tmpVal);
> +}
> +
> +inline static int rstBit(struct spi_device *spi, u8 reg, u8 mask)

rstBit()?  What does that mean?

> +{
> +	u8 tmpVal;
> +
> +	tmpVal = rf69_read_reg (spi, reg);
> +	tmpVal = tmpVal & ~mask;
> +	return rf69_write_reg(spi, reg, tmpVal);
> +}
> +
> +inline static int rmw(struct spi_device *spi, u8 reg, u8 mask, u8 value)

what does rmw() mean?  Spell it out so no one has to try to guess :)

thanks,

greg k-h