subject:"\[PATCH 6\/8\] staging\: pi433\: use defines for shifting register values"

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

2017-12-13 Thread 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 
> > ---
> >   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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

2017-12-13 Thread Marcus Wolf




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



case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
default:return UNDEF;
@@ -340,7 +340,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
  
  	currentValue = rf69_read_reg(spi, REG_LNA);
  
-	switch ((currentValue & MASK_LNA_CURRENT_GAIN) >> 3) { // improvement: change 3 to define

+   switch ((currentValue & MASK_LNA_CURRENT_GAIN) >> 
SHIFT_LNA_CURRENT_GAIN) {


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.


Thx,

Marcus


case LNA_GAIN_AUTO: return automatic;
case LNA_GAIN_MAX:  return max;
case LNA_GAIN_MAX_MINUS_6:  return maxMinus6;
diff --git a/drivers/staging/pi433/rf69_registers.h 
b/drivers/staging/pi433/rf69_registers.h
index 981b57d7cc0b..da12642cf249 100644
--- a/drivers/staging/pi433/rf69_registers.h
+++ b/drivers/staging/pi433/rf69_registers.h
@@ -124,6 +124,7 @@
  #define  MASK_DATAMODUL_MODE  0x06
  #define  MASK_DATAMODUL_MODULATION_TYPE   0x18
  #define  MASK_DATAMODUL_MODULATION_SHAPE  0x03
+#define  SHIFT_DATAMODUL_MODULATION_TYPE   3
  
  #define  DATAMODUL_MODE_PACKET			0x00 /* default */

  #define  DATAMODUL_MODE_CONTINUOUS0x40
@@ -235,6 +236,7 @@
  #define  MASK_LNA_ZIN 0x80
  #define  MASK_LNA_CURRENT_GAIN0x38
  #define  MASK_LNA_GAIN0x07
+#define  SHIFT_LNA_CURRENT_GAIN3
  
  #define  LNA_GAIN_AUTO0x00 /* default */

  #define  LNA_GAIN_MAX 0x01



--
Smarthome-Wolf UG (haftungsbeschränkt)
Helene-Lange-Weg 23
80637 München
Amtsgericht München, HRB 223529
Umastzsteuer-ID: DE304719911
Geschäftsführer: Marcus Wolf
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

2017-12-13 Thread Valentin Vidic

Avoid shifting by magic numbers and use defines instead:

  SHIFT_DATAMODUL_MODULATION_TYPE
  SHIFT_LNA_CURRENT_GAIN

Signed-off-by: Valentin Vidic 
---
 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) {
case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
default:return UNDEF;
@@ -340,7 +340,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
 
currentValue = rf69_read_reg(spi, REG_LNA);
 
-   switch ((currentValue & MASK_LNA_CURRENT_GAIN) >> 3) { // improvement: 
change 3 to define
+   switch ((currentValue & MASK_LNA_CURRENT_GAIN) >> 
SHIFT_LNA_CURRENT_GAIN) {
case LNA_GAIN_AUTO: return automatic;
case LNA_GAIN_MAX:  return max;
case LNA_GAIN_MAX_MINUS_6:  return maxMinus6;
diff --git a/drivers/staging/pi433/rf69_registers.h 
b/drivers/staging/pi433/rf69_registers.h
index 981b57d7cc0b..da12642cf249 100644
--- a/drivers/staging/pi433/rf69_registers.h
+++ b/drivers/staging/pi433/rf69_registers.h
@@ -124,6 +124,7 @@
 #define  MASK_DATAMODUL_MODE   0x06
 #define  MASK_DATAMODUL_MODULATION_TYPE0x18
 #define  MASK_DATAMODUL_MODULATION_SHAPE   0x03
+#define  SHIFT_DATAMODUL_MODULATION_TYPE   3
 
 #define  DATAMODUL_MODE_PACKET 0x00 /* default */
 #define  DATAMODUL_MODE_CONTINUOUS 0x40
@@ -235,6 +236,7 @@
 #define  MASK_LNA_ZIN  0x80
 #define  MASK_LNA_CURRENT_GAIN 0x38
 #define  MASK_LNA_GAIN 0x07
+#define  SHIFT_LNA_CURRENT_GAIN3
 
 #define  LNA_GAIN_AUTO 0x00 /* default */
 #define  LNA_GAIN_MAX  0x01
-- 
2.15.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information