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

[Dan Carpenter]

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(>dev, "setBitRate: illegal input param");
> + dev_dbg(>dev, "rf69_set_bitRate: illegal input param");

Just use __func__

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

[Dan Carpenter]

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(>dev, "setBitRate: illegal input param");
> + dev_dbg(>dev, "rf69_set_bitRate: illegal input param");

Just use __func__

[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

[Marcus Wolf]

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)
+{
+   u8 tmpVal;
+
+   tmpVal = rf69_read_reg (spi, reg);
+   tmpVal = tmpVal | mask;
+   return rf69_write_reg(spi, reg, tmpVal);
+}
+
+static inline int rf69_reset_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   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);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -68,9 +92,9 @@ int rf69_set_data_mode(struct spi_device *spi, enum dataMode 
dataMode)
#endif
 
switch (dataMode) {
-   case packet:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-   case continuous:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-   case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | 
DATAMODUL_MODE_CONTINUOUS_NOSYNC);
+   case packet:return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_PACKET);
+   case continuous:return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS);
+   case continuousNoSync:  return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS_NOSYNC);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -84,8 +108,8 @@ int rf69_set_modulation(struct spi_device *spi, enum 
modulation modulation)
#endif
 
switch (modulation) {
-   case OOK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_FSK);
+   case OOK: return rf69_read_modify_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
+   case FSK: return rf69_read_modify_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -100,7 +124,7 @@ 

[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

[Marcus Wolf]

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)
+{
+   u8 tmpVal;
+
+   tmpVal = rf69_read_reg (spi, reg);
+   tmpVal = tmpVal | mask;
+   return rf69_write_reg(spi, reg, tmpVal);
+}
+
+static inline int rf69_reset_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   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);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -68,9 +92,9 @@ int rf69_set_data_mode(struct spi_device *spi, enum dataMode 
dataMode)
#endif
 
switch (dataMode) {
-   case packet:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-   case continuous:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-   case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | 
DATAMODUL_MODE_CONTINUOUS_NOSYNC);
+   case packet:return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_PACKET);
+   case continuous:return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS);
+   case continuousNoSync:  return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS_NOSYNC);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -84,8 +108,8 @@ int rf69_set_modulation(struct spi_device *spi, enum 
modulation modulation)
#endif
 
switch (modulation) {
-   case OOK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_FSK);
+   case OOK: return rf69_read_modify_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
+   case FSK: return rf69_read_modify_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -100,7 +124,7 @@ enum modulation