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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171213165843.GD3255@gavran.carpriv.carnet.hr>
Date:   Wed, 13 Dec 2017 17:58:43 +0100
From:   Valentin Vidic <Valentin.Vidic@...Net.hr>
To:     Marcus Wolf <marcus.wolf@...rthome-wolf.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Simon Sandström <simon@...anor.nu>,
        Marcin Ciupak <marcin.s.ciupak@...il.com>,
        Marcus Wolf <linux@...f-entwicklungen.de>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/8] staging: pi433: use defines for shifting register
 values

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@...Net.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