Date: Sat, 2 Dec 2017 15:03:20 +0000
From: Greg KH <gregkh@linuxfoundation.org>
To: Marcus Wolf <linux@wolf-entwicklungen.de>
Cc: dan.carpenter@oracle.com, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: pi433: rf69.c: Replace macros READ_REG and
 WRITE_REG with inline functions setBit rstBit and rmwBit
Message-ID: <20171202150320.GE1242@kroah.com>
References: <1512217292-8533-1-git-send-email-linux@wolf-entwicklungen.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1512217292-8533-1-git-send-email-linux@wolf-entwicklungen.de>
User-Agent: Mutt/1.9.1 (2017-09-22)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2221
Lines: 69

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