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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> 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__
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
