Re: [PATCH 6/8] staging: pi433: use defines for shifting register values

[Valentin Vidic]

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.Vidic@xxxxxxxxx>
> > ---
> >   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