On Wed, Dec 13, 2017 at 05:32:28PM +0200, Marcus Wolf wrote:
> Am 13.12.2017 um 16:21 schrieb Valentin Vidic:
> > Avoid shifting by magic numbers and use defines instead:
> >
> > SHIFT_DATAMODUL_MODULATION_TYPE
> > SHIFT_LNA_CURRENT_GAIN
> >
> > Signed-off-by: Valentin Vidic <valentin.vi...@carnet.hr>
> > ---
> > drivers/staging/pi433/rf69.c | 4 ++--
> > drivers/staging/pi433/rf69_registers.h | 2 ++
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > index b1e243e5bcac..8c4841c9d796 100644
> > --- a/drivers/staging/pi433/rf69.c
> > +++ b/drivers/staging/pi433/rf69.c
> > @@ -102,7 +102,7 @@ enum modulation rf69_get_modulation(struct spi_device
> > *spi)
> > currentValue = rf69_read_reg(spi, REG_DATAMODUL);
> > - switch ((currentValue & MASK_DATAMODUL_MODULATION_TYPE) >> 3) { // TODO
> > improvement: change 3 to define
> > + switch ((currentValue & MASK_DATAMODUL_MODULATION_TYPE) >>
> > SHIFT_DATAMODUL_MODULATION_TYPE) {
>
> As mentioned by Dan, this change isn't needed any more, since we don't need
> the shift right here, since the DATAMODUL_MODULATION_TYPE_OOK and
> DATAMODUL_MODULATION_TYPE_FSK already contains the bits at the correct
> position.
Not sure why this TODO is still visible in staging-next?
https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/staging/+/staging-next/drivers/staging/pi433/rf69.c#99
> Regarding my previous mail: I was wrong! This way is right!!
>
> BUT: I would prefer to have a solution, like it was done for the modulation
> type: Do not shift anything here, but introduce new defines
> (LNA_GAIN_AUTO_xyz...), that are used for the casees, having the bits set at
> the right position, so theycan be used without shifting.
> Be aware: The old defines must remain untouched, since they are needed for
> an other function.
Just sent a v2 of this patch, please review if this is what you had in
mind for LNA_GAIN.
--
Valentin
