Hi Walter,
since the construction
WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) |
LNA_GAIN_MAX_MINUS_6) )
aka
WRITE_REG(regname, ( (READ_REG(regname) & ~regmask ) | vale
) )
is used nearly everywhere, I think, about using a more gneric macro like
#define READ_MODIFY_WRITE(reg, mask, value) /
WRITE_REG(reg, ((READ_REG(reg) & ~mask) | vale ))
What do you think about it?
Cheers,
Marcus
> walter harms <wharms@xxxxxx> hat am 20. Juli 2017 um 14:22 geschrieben:
>
>
>
>
> Am 19.07.2017 20:18, schrieb Wolf Entwicklungen:
> > Bugfixes for rf69_set_modulation, rf69_set_deviation, rf69_set_lna_gain and
> > rf69_get_lna_gain
> > The fixes are cross-checked with the datasheet of the rfm69cw
> >
> > Fixes: 874bcba65f9a ("staging: pi433: New driver")
> > Signed-off-by: Marcus Wolf <linux@xxxxxxxxxxxxxxxxxxxxx>
> >
> > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > --- a/drivers/staging/pi433/rf69.c
> > +++ b/drivers/staging/pi433/rf69.c
> > @@ -101,7 +101,7 @@ int rf69_set_modulation(struct spi_device *spi, enum
> > modulation modulation)
> >
> > currentValue = READ_REG(REG_DATAMODUL);
> >
> > - switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> 3) // TODO
> > improvement: change 3 to define
> > + switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE)
> > {
> > case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
> > case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
> > @@ -203,7 +203,7 @@ int rf69_set_deviation(struct spi_device *spi, u32
> > deviation)
> > lsb = (f_reg&0xff);
> >
> > // check msb
> > - if (msb & !FDEVMASB_MASK)
> > + if (msb & ~FDEVMASB_MASK)
> > {
> > dev_dbg(&spi->dev, "set_deviation: err in calc of msb");
> > INVALID_PARAM;
> > @@ -366,13 +366,13 @@ int rf69_set_lna_gain(struct spi_device *spi, enum
> > lnaGain lnaGain)
> > #endif
> >
> > switch(lnaGain) {
> > - case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_AUTO) );
> > - case max: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN)
> > & LNA_GAIN_MAX) );
> > - case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_6) );
> > - case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_12) );
> > - case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_24) );
> > - case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_36) );
> > - case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_48) );
> > + case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_AUTO) );
> > + case max: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN)
> > | LNA_GAIN_MAX) );
> > + case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_6) );
> > + case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_12) );
> > + case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_24) );
> > + case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_36) );
> > + case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_48) );
> > default: INVALID_PARAM;
> > }
> > }
>
>
> looks resonable,
> some nitpicking: perhaps you can can improve readability by using a helper
> like:
>
> static int setstbit(int arg)
> {
> int get=READ_REG(REG_LNA) & ~MASK_LNA_GAIN;
> return WRITE_REG( get| arg);
>
> }
>
> so this switch would be reduced to ...
>
> case maxMinus6: return setstbit(LNA_GAIN_MAX_MINUS_6);
>
> re,
> wh
>
>
> > @@ -387,7 +387,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
> >
> > currentValue = READ_REG(REG_LNA);
> >
> > - switch (currentValue & MASK_LNA_CURRENT_GAIN >> 3) // improvement: change
> > 3 to define
> > + switch (currentValue & MASK_LNA_GAIN)
> > {
> > case LNA_GAIN_AUTO: return automatic;
> > case LNA_GAIN_MAX: return max;
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors"
> > in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
