Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752716AbdLDT37 (ORCPT ); Mon, 4 Dec 2017 14:29:59 -0500 Received: from userp2130.oracle.com ([156.151.31.86]:48345 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752603AbdLDT36 (ORCPT ); Mon, 4 Dec 2017 14:29:58 -0500 Date: Mon, 4 Dec 2017 22:29:38 +0300 From: Dan Carpenter To: Marcus Wolf Cc: gregkh@linuxfoundation.org, 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 rf69_set_bit, rf69_reset_bit and rf69_read_modify_write Message-ID: <20171204192938.3zxfnb4vscflsjds@mwanda> References: <1512411531-13701-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: <1512411531-13701-1-git-send-email-linux@wolf-entwicklungen.de> User-Agent: NeoMutt/20170609 (1.8.3) X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=8735 signatures=668637 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1712040274 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4171 Lines: 108 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 > --- > 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__