[PATCH 1/8] staging: pi433: collapse else block after return statement

[PATCH 1/8] staging: pi433: collapse else block after return statement

[Valentin Vidic]

Fixes checkpatch warning:

  WARNING: else is not generally useful after a break or return

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
 drivers/staging/pi433/pi433_if.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b4e6094ad553..02887988d2ea 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -773,11 +773,11 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
 	if (device->rx_active) {
 		mutex_unlock(&device->rx_lock);
 		return -EAGAIN;
-	} else {
-		device->rx_active = true;
-		mutex_unlock(&device->rx_lock);
 	}
 
+	device->rx_active = true;
+	mutex_unlock(&device->rx_lock);
+
 	/* start receiving */
 	/* will block until something was received*/
 	device->rx_buffer_size = size;
@@ -1117,12 +1117,12 @@ static int pi433_probe(struct spi_device *spi)
 	if (retval) {
 		dev_dbg(&spi->dev, "configuration of SPI interface failed!\n");
 		return retval;
-	} else {
-		dev_dbg(&spi->dev,
-			"spi interface setup: mode 0x%2x, %d bits per word, %dhz max speed",
-			spi->mode, spi->bits_per_word, spi->max_speed_hz);
 	}
 
+	dev_dbg(&spi->dev,
+		"spi interface setup: mode 0x%2x, %d bits per word, %dhz max speed",
+		spi->mode, spi->bits_per_word, spi->max_speed_hz);
+
 	/* Ping the chip by reading the version register */
 	retval = spi_w8r8(spi, 0x10);
 	if (retval < 0)
-- 
2.15.0

[PATCH 2/8] staging: pi433: move var declaration to function level

[Valentin Vidic]

Fixes checkpatch warning:

  WARNING: Missing a blank line after declarations

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
 drivers/staging/pi433/pi433_if.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 02887988d2ea..07e5352ae5ad 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -565,6 +565,7 @@ pi433_tx_thread(void *data)
 	bool   rx_interrupted = false;
 	int    position, repetitions;
 	int    retval;
+	int    temp;
 
 	while (1) {
 		/* wait for fifo to be populated or for request to terminate*/
@@ -700,7 +701,7 @@ pi433_tx_thread(void *data)
 		while ((repetitions > 0) && (size > position)) {
 			if ((size - position) > device->free_in_fifo) {
 				/* msg to big for fifo - take a part */
-				int temp = device->free_in_fifo;
+				temp = device->free_in_fifo;
 				device->free_in_fifo = 0;
 				rf69_write_fifo(spi,
 						&buffer[position],
-- 
2.15.0

Re: [PATCH 2/8] staging: pi433: move var declaration to function level

[Dan Carpenter]

On Wed, Dec 13, 2017 at 03:21:50PM +0100, Valentin Vidic wrote:
> Fixes checkpatch warning:
> 
>   WARNING: Missing a blank line after declarations
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
> ---
>  drivers/staging/pi433/pi433_if.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 02887988d2ea..07e5352ae5ad 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -565,6 +565,7 @@ pi433_tx_thread(void *data)
>  	bool   rx_interrupted = false;
>  	int    position, repetitions;
>  	int    retval;
> +	int    temp;

Generally, "temp" means "temperature" and "tmp" means "temporary".  The
kernel deals with both.  Btw "buff" means an 80s dude in a sleeveless
shirt and "buf" means a buffer, but the kernel doesn't deal with 80's
dudes so that one is less confusing.

But the name "tmp" still isn't really ideal here.

regards,
dan carpenter

[PATCH 2/8 v2] staging: pi433: move var declaration to function level

[Valentin Vidic]

Fixes checkpatch warning:

  WARNING: Missing a blank line after declarations

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
v2: use a better variable name

 drivers/staging/pi433/pi433_if.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 02887988d2ea..dfa771304d97 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -565,6 +565,7 @@ pi433_tx_thread(void *data)
 	bool   rx_interrupted = false;
 	int    position, repetitions;
 	int    retval;
+	int    write_size;
 
 	while (1) {
 		/* wait for fifo to be populated or for request to terminate*/
@@ -700,12 +701,12 @@ pi433_tx_thread(void *data)
 		while ((repetitions > 0) && (size > position)) {
 			if ((size - position) > device->free_in_fifo) {
 				/* msg to big for fifo - take a part */
-				int temp = device->free_in_fifo;
+				write_size = device->free_in_fifo;
 				device->free_in_fifo = 0;
 				rf69_write_fifo(spi,
 						&buffer[position],
-						temp);
-				position += temp;
+						write_size);
+				position += write_size;
 			} else {
 				/* msg fits into fifo - take all */
 				device->free_in_fifo -= size;
-- 
2.15.0

Re: [PATCH 2/8] staging: pi433: move var declaration to function level

[Joe Perches]

On Wed, 2017-12-13 at 15:21 +0100, Valentin Vidic wrote:
>   WARNING: Missing a blank line after declarations
[]
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
[]
> @@ -565,6 +565,7 @@ pi433_tx_thread(void *data)
>  	bool   rx_interrupted = false;
>  	int    position, repetitions;
>  	int    retval;
> +	int    temp;
>  
>  	while (1) {
>  		/* wait for fifo to be populated or for request to terminate*/
> @@ -700,7 +701,7 @@ pi433_tx_thread(void *data)
>  		while ((repetitions > 0) && (size > position)) {
>  			if ((size - position) > device->free_in_fifo) {
>  				/* msg to big for fifo - take a part */
> -				int temp = device->free_in_fifo;
> +				temp = device->free_in_fifo;

It's generally better to keep variable declarations
to the innermost scope possible and not move them
outwards unnecessarily.

[PATCH 2/8 v3] staging: pi433: cleanup local variable declaration

[Valentin Vidic]

Fix variable naming and checkpatch warning:

  WARNING: Missing a blank line after declarations

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
v2: use a better variable name
v3: keep the variable scope

 drivers/staging/pi433/pi433_if.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 02887988d2ea..86709a7100ad 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -699,13 +699,15 @@ pi433_tx_thread(void *data)
 		repetitions = tx_cfg.repetitions;
 		while ((repetitions > 0) && (size > position)) {
 			if ((size - position) > device->free_in_fifo) {
+				int write_size;
+
 				/* msg to big for fifo - take a part */
-				int temp = device->free_in_fifo;
+				write_size = device->free_in_fifo;
 				device->free_in_fifo = 0;
 				rf69_write_fifo(spi,
 						&buffer[position],
-						temp);
-				position += temp;
+						write_size);
+				position += write_size;
 			} else {
 				/* msg fits into fifo - take all */
 				device->free_in_fifo -= size;
-- 
2.15.0

[PATCH 3/8] staging: pi433: replace unsigned with unsigned int

[Valentin Vidic]

Fixes checkpatch warning:

  WARNING: Prefer 'unsigned int' to bare use of 'unsigned'

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
 drivers/staging/pi433/pi433_if.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 07e5352ae5ad..8234473e1add 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -78,7 +78,7 @@ struct pi433_device {
 	struct device		*dev;
 	struct cdev		*cdev;
 	struct spi_device	*spi;
-	unsigned		users;
+	unsigned int		users;
 
 	/* irq related values */
 	struct gpio_desc	*gpiod[NUM_DIO];
-- 
2.15.0

[PATCH 4/8] staging: pi433: add parentheses to mask and shift

[Valentin Vidic]

Fixes checkpatch warning:

  WARNING: Possible precedence defect with mask then right shift - may need parentheses

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
 drivers/staging/pi433/rf69.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index f77ecd60f43a..b1e243e5bcac 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) >> 3) { // TODO improvement: change 3 to define
 	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) >> 3) { // improvement: change 3 to define
 	case LNA_GAIN_AUTO:	    return automatic;
 	case LNA_GAIN_MAX:	    return max;
 	case LNA_GAIN_MAX_MINUS_6:  return maxMinus6;
-- 
2.15.0

Re: [PATCH 4/8] staging: pi433: add parentheses to mask and shift

[Dan Carpenter]

On Wed, Dec 13, 2017 at 03:21:52PM +0100, Valentin Vidic wrote:
> Fixes checkpatch warning:
> 
>   WARNING: Possible precedence defect with mask then right shift - may need parentheses
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>

Please do your work against linux-next or staging-next.  There have been
a ton of changes.

regards,
dan carpenter

Re: [PATCH 4/8] staging: pi433: add parentheses to mask and shift

[Marcus Wolf]

Am 13.12.2017 um 16:21 schrieb Valentin Vidic:
> Fixes checkpatch warning:
> 
>    WARNING: Possible precedence defect with mask then right shift - may need parentheses
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
> ---
>   drivers/staging/pi433/rf69.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index f77ecd60f43a..b1e243e5bcac 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) >> 3) { // TODO improvement: change 3 to define

As mentioned by Dan, this change isn't needed any more, since ther was a 
bug, that's already fixed.

>   	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) >> 3) { // improvement: change 3 to define

To my knowledge,'>>' is stronger than '&'.
So this change will modify the behaviour.
If I am wrong, the code was wrong from the beginning.

What should happen here, is:
read the value from reg and do a bitwise and with the shifted value from 
MASK_...

>   	case LNA_GAIN_AUTO:	    return automatic;
>   	case LNA_GAIN_MAX:	    return max;
>   	case LNA_GAIN_MAX_MINUS_6:  return maxMinus6;
> 

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

[PATCH 5/8] staging: pi433: fix DATAMODUL_MODULATION_TYPE_OOK value

[Valentin Vidic]

Reading from the spec, allowed values for modulation scheme
after the shift are:

  00 FSK
  01 OOK
  10 - 11 reserved

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
 drivers/staging/pi433/rf69_registers.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h
index 33fd91518bb0..981b57d7cc0b 100644
--- a/drivers/staging/pi433/rf69_registers.h
+++ b/drivers/staging/pi433/rf69_registers.h
@@ -130,7 +130,7 @@
 #define  DATAMODUL_MODE_CONTINUOUS_NOSYNC	0x60
 
 #define  DATAMODUL_MODULATION_TYPE_FSK		0x00 /* default */
-#define  DATAMODUL_MODULATION_TYPE_OOK		0x08
+#define  DATAMODUL_MODULATION_TYPE_OOK		0x01
 
 #define  DATAMODUL_MODULATION_SHAPE_NONE	0x00 /* default */
 #define  DATAMODUL_MODULATION_SHAPE_1_0		0x01
-- 
2.15.0

Re: [PATCH 5/8] staging: pi433: fix DATAMODUL_MODULATION_TYPE_OOK value

[Dan Carpenter]

On Wed, Dec 13, 2017 at 03:21:53PM +0100, Valentin Vidic wrote:
> Reading from the spec, allowed values for modulation scheme
> after the shift are:
> 
>   00 FSK
>   01 OOK
>   10 - 11 reserved
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
> ---
>  drivers/staging/pi433/rf69_registers.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h
> index 33fd91518bb0..981b57d7cc0b 100644
> --- a/drivers/staging/pi433/rf69_registers.h
> +++ b/drivers/staging/pi433/rf69_registers.h
> @@ -130,7 +130,7 @@
>  #define  DATAMODUL_MODE_CONTINUOUS_NOSYNC	0x60
>  
>  #define  DATAMODUL_MODULATION_TYPE_FSK		0x00 /* default */
> -#define  DATAMODUL_MODULATION_TYPE_OOK		0x08
> +#define  DATAMODUL_MODULATION_TYPE_OOK		0x01

Look how DATAMODUL_MODULATION_TYPE_OOK is used (in linux-next).  We
removed the shift.

regards,
dan carpenter

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

[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@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) {
 	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_TYPE		0x18
 #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_GAIN			3
 
 #define  LNA_GAIN_AUTO				0x00 /* default */
 #define  LNA_GAIN_MAX				0x01
-- 
2.15.0

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

[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 <Valentin.Vidic@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.

>   	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_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_GAIN			3
>   
>   #define  LNA_GAIN_AUTO				0x00 /* 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

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

[Valentin Vidic]

Avoid shifting by magic numbers and use defines instead.

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
v2: - drop change for SHIFT_DATAMODUL_MODULATION_TYPE
    - move shifting to the header file

 drivers/staging/pi433/rf69.c           | 16 ++++++++--------
 drivers/staging/pi433/rf69_registers.h |  8 ++++++++
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index f77ecd60f43a..ce259b4f0b0e 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -340,14 +340,14 @@ 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
-	case LNA_GAIN_AUTO:	    return automatic;
-	case LNA_GAIN_MAX:	    return max;
-	case LNA_GAIN_MAX_MINUS_6:  return maxMinus6;
-	case LNA_GAIN_MAX_MINUS_12: return maxMinus12;
-	case LNA_GAIN_MAX_MINUS_24: return maxMinus24;
-	case LNA_GAIN_MAX_MINUS_36: return maxMinus36;
-	case LNA_GAIN_MAX_MINUS_48: return maxMinus48;
+	switch (currentValue & MASK_LNA_CURRENT_GAIN) {
+	case LNA_GAIN_AUTO_SHIFTED:	    return automatic;
+	case LNA_GAIN_MAX_SHIFTED:	    return max;
+	case LNA_GAIN_MAX_MINUS_6_SHIFTED:  return maxMinus6;
+	case LNA_GAIN_MAX_MINUS_12_SHIFTED: return maxMinus12;
+	case LNA_GAIN_MAX_MINUS_24_SHIFTED: return maxMinus24;
+	case LNA_GAIN_MAX_MINUS_36_SHIFTED: return maxMinus36;
+	case LNA_GAIN_MAX_MINUS_48_SHIFTED: return maxMinus48;
 	default:		    return undefined;
 	}
 }
diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h
index d23b8b668ef5..6329bbf9e499 100644
--- a/drivers/staging/pi433/rf69_registers.h
+++ b/drivers/staging/pi433/rf69_registers.h
@@ -237,6 +237,7 @@
 #define  MASK_LNA_ZIN				0x80
 #define  MASK_LNA_CURRENT_GAIN			0x38
 #define  MASK_LNA_GAIN				0x07
+#define  SHIFT_LNA_CURRENT_GAIN			3
 
 #define  LNA_GAIN_AUTO				0x00 /* default */
 #define  LNA_GAIN_MAX				0x01
@@ -246,6 +247,13 @@
 #define  LNA_GAIN_MAX_MINUS_36			0x05
 #define  LNA_GAIN_MAX_MINUS_48			0x06
 
+#define  LNA_GAIN_AUTO_SHIFTED			(LNA_GAIN_AUTO		<< SHIFT_LNA_CURRENT_GAIN)
+#define  LNA_GAIN_MAX_SHIFTED			(LNA_GAIN_MAX		<< SHIFT_LNA_CURRENT_GAIN)
+#define  LNA_GAIN_MAX_MINUS_6_SHIFTED		(LNA_GAIN_MAX_MINUS_6	<< SHIFT_LNA_CURRENT_GAIN)
+#define  LNA_GAIN_MAX_MINUS_12_SHIFTED		(LNA_GAIN_MAX_MINUS_12	<< SHIFT_LNA_CURRENT_GAIN)
+#define  LNA_GAIN_MAX_MINUS_24_SHIFTED		(LNA_GAIN_MAX_MINUS_24	<< SHIFT_LNA_CURRENT_GAIN)
+#define  LNA_GAIN_MAX_MINUS_36_SHIFTED		(LNA_GAIN_MAX_MINUS_36	<< SHIFT_LNA_CURRENT_GAIN)
+#define  LNA_GAIN_MAX_MINUS_48_SHIFTED		(LNA_GAIN_MAX_MINUS_48	<< SHIFT_LNA_CURRENT_GAIN)
 
 /* RegRxBw (0x19) and RegAfcBw (0x1A) */
 #define  MASK_BW_DCC_FREQ			0xE0
-- 
2.15.0

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

[Marcus Wolf]

Am 13.12.2017 um 18:55 schrieb Valentin Vidic:
> Avoid shifting by magic numbers and use defines instead.
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
> ---
> v2: - drop change for SHIFT_DATAMODUL_MODULATION_TYPE
>      - move shifting to the header file
> 
>   drivers/staging/pi433/rf69.c           | 16 ++++++++--------
>   drivers/staging/pi433/rf69_registers.h |  8 ++++++++
>   2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index f77ecd60f43a..ce259b4f0b0e 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -340,14 +340,14 @@ 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
> -	case LNA_GAIN_AUTO:	    return automatic;
> -	case LNA_GAIN_MAX:	    return max;
> -	case LNA_GAIN_MAX_MINUS_6:  return maxMinus6;
> -	case LNA_GAIN_MAX_MINUS_12: return maxMinus12;
> -	case LNA_GAIN_MAX_MINUS_24: return maxMinus24;
> -	case LNA_GAIN_MAX_MINUS_36: return maxMinus36;
> -	case LNA_GAIN_MAX_MINUS_48: return maxMinus48;
> +	switch (currentValue & MASK_LNA_CURRENT_GAIN) {
> +	case LNA_GAIN_AUTO_SHIFTED:	    return automatic;
> +	case LNA_GAIN_MAX_SHIFTED:	    return max;
> +	case LNA_GAIN_MAX_MINUS_6_SHIFTED:  return maxMinus6;
> +	case LNA_GAIN_MAX_MINUS_12_SHIFTED: return maxMinus12;
> +	case LNA_GAIN_MAX_MINUS_24_SHIFTED: return maxMinus24;
> +	case LNA_GAIN_MAX_MINUS_36_SHIFTED: return maxMinus36;
> +	case LNA_GAIN_MAX_MINUS_48_SHIFTED: return maxMinus48;
>   	default:		    return undefined;
>   	}
>   }
> diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h
> index d23b8b668ef5..6329bbf9e499 100644
> --- a/drivers/staging/pi433/rf69_registers.h
> +++ b/drivers/staging/pi433/rf69_registers.h
> @@ -237,6 +237,7 @@
>   #define  MASK_LNA_ZIN				0x80
>   #define  MASK_LNA_CURRENT_GAIN			0x38
>   #define  MASK_LNA_GAIN				0x07
> +#define  SHIFT_LNA_CURRENT_GAIN			3
>   
>   #define  LNA_GAIN_AUTO				0x00 /* default */
>   #define  LNA_GAIN_MAX				0x01
> @@ -246,6 +247,13 @@
>   #define  LNA_GAIN_MAX_MINUS_36			0x05
>   #define  LNA_GAIN_MAX_MINUS_48			0x06
>   
> +#define  LNA_GAIN_AUTO_SHIFTED			(LNA_GAIN_AUTO		<< SHIFT_LNA_CURRENT_GAIN)
> +#define  LNA_GAIN_MAX_SHIFTED			(LNA_GAIN_MAX		<< SHIFT_LNA_CURRENT_GAIN)
> +#define  LNA_GAIN_MAX_MINUS_6_SHIFTED		(LNA_GAIN_MAX_MINUS_6	<< SHIFT_LNA_CURRENT_GAIN)
> +#define  LNA_GAIN_MAX_MINUS_12_SHIFTED		(LNA_GAIN_MAX_MINUS_12	<< SHIFT_LNA_CURRENT_GAIN)
> +#define  LNA_GAIN_MAX_MINUS_24_SHIFTED		(LNA_GAIN_MAX_MINUS_24	<< SHIFT_LNA_CURRENT_GAIN)
> +#define  LNA_GAIN_MAX_MINUS_36_SHIFTED		(LNA_GAIN_MAX_MINUS_36	<< SHIFT_LNA_CURRENT_GAIN)
> +#define  LNA_GAIN_MAX_MINUS_48_SHIFTED		(LNA_GAIN_MAX_MINUS_48	<< SHIFT_LNA_CURRENT_GAIN)
>   
>   /* RegRxBw (0x19) and RegAfcBw (0x1A) */
>   #define  MASK_BW_DCC_FREQ			0xE0
> 

Hi Valentin,

nice :-)

Name is a bit strange, since it's not really shifted. If you have a look 
at the datasheet (RegLNA, page 68), there are two sections "by chance" 
both using the max, -6, -12 ...
But auto is not needed for current gain (was already bad in my old 
implementation!), since the current gain will always report a real 
value, never auto.
So maybe it is a little(!) better not to derive the "current" defines 
from the "select" defines and use names like LNA_GAIN_MAX_SELECT and 
LNA_GAIN_MAX_CURRENT.

Never the less - I like patch 6/8 v2 a lot more, than the original one :-)

Thank you very much for your help,

Marcus

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

[Valentin Vidic]

Avoid shifting by magic numbers and use defines instead.

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
v2: - drop change for SHIFT_DATAMODUL_MODULATION_TYPE
    - move shifting to the header file
v3: - drop auto case
    - use CURRENT suffix
    - precompute define values

 drivers/staging/pi433/rf69.c           | 15 +++++++--------
 drivers/staging/pi433/rf69_registers.h |  6 ++++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index f77ecd60f43a..0889c65d5a31 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -340,14 +340,13 @@ 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
-	case LNA_GAIN_AUTO:	    return automatic;
-	case LNA_GAIN_MAX:	    return max;
-	case LNA_GAIN_MAX_MINUS_6:  return maxMinus6;
-	case LNA_GAIN_MAX_MINUS_12: return maxMinus12;
-	case LNA_GAIN_MAX_MINUS_24: return maxMinus24;
-	case LNA_GAIN_MAX_MINUS_36: return maxMinus36;
-	case LNA_GAIN_MAX_MINUS_48: return maxMinus48;
+	switch (currentValue & MASK_LNA_CURRENT_GAIN) {
+	case LNA_GAIN_MAX_CURRENT:	    return max;
+	case LNA_GAIN_MAX_MINUS_6_CURRENT:  return maxMinus6;
+	case LNA_GAIN_MAX_MINUS_12_CURRENT: return maxMinus12;
+	case LNA_GAIN_MAX_MINUS_24_CURRENT: return maxMinus24;
+	case LNA_GAIN_MAX_MINUS_36_CURRENT: return maxMinus36;
+	case LNA_GAIN_MAX_MINUS_48_CURRENT: return maxMinus48;
 	default:		    return undefined;
 	}
 }
diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h
index d23b8b668ef5..4a189c6a4ab3 100644
--- a/drivers/staging/pi433/rf69_registers.h
+++ b/drivers/staging/pi433/rf69_registers.h
@@ -246,6 +246,12 @@
 #define  LNA_GAIN_MAX_MINUS_36			0x05
 #define  LNA_GAIN_MAX_MINUS_48			0x06
 
+#define  LNA_GAIN_MAX_CURRENT			0x08
+#define  LNA_GAIN_MAX_MINUS_6_CURRENT		0x10
+#define  LNA_GAIN_MAX_MINUS_12_CURRENT		0x18
+#define  LNA_GAIN_MAX_MINUS_24_CURRENT		0x20
+#define  LNA_GAIN_MAX_MINUS_36_CURRENT		0x28
+#define  LNA_GAIN_MAX_MINUS_48_CURRENT		0x30
 
 /* RegRxBw (0x19) and RegAfcBw (0x1A) */
 #define  MASK_BW_DCC_FREQ			0xE0
-- 
2.15.0

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

[Marcus Wolf]

Am 13.12.2017 um 19:44 schrieb Valentin Vidic:
> Avoid shifting by magic numbers and use defines instead.
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
> ---
> v2: - drop change for SHIFT_DATAMODUL_MODULATION_TYPE
>      - move shifting to the header file
> v3: - drop auto case
>      - use CURRENT suffix
>      - precompute define values
> 
>   drivers/staging/pi433/rf69.c           | 15 +++++++--------
>   drivers/staging/pi433/rf69_registers.h |  6 ++++++
>   2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index f77ecd60f43a..0889c65d5a31 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -340,14 +340,13 @@ 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
> -	case LNA_GAIN_AUTO:	    return automatic;
> -	case LNA_GAIN_MAX:	    return max;
> -	case LNA_GAIN_MAX_MINUS_6:  return maxMinus6;
> -	case LNA_GAIN_MAX_MINUS_12: return maxMinus12;
> -	case LNA_GAIN_MAX_MINUS_24: return maxMinus24;
> -	case LNA_GAIN_MAX_MINUS_36: return maxMinus36;
> -	case LNA_GAIN_MAX_MINUS_48: return maxMinus48;
> +	switch (currentValue & MASK_LNA_CURRENT_GAIN) {
> +	case LNA_GAIN_MAX_CURRENT:	    return max;
> +	case LNA_GAIN_MAX_MINUS_6_CURRENT:  return maxMinus6;
> +	case LNA_GAIN_MAX_MINUS_12_CURRENT: return maxMinus12;
> +	case LNA_GAIN_MAX_MINUS_24_CURRENT: return maxMinus24;
> +	case LNA_GAIN_MAX_MINUS_36_CURRENT: return maxMinus36;
> +	case LNA_GAIN_MAX_MINUS_48_CURRENT: return maxMinus48;
>   	default:		    return undefined;
>   	}
>   }
> diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h
> index d23b8b668ef5..4a189c6a4ab3 100644
> --- a/drivers/staging/pi433/rf69_registers.h
> +++ b/drivers/staging/pi433/rf69_registers.h
> @@ -246,6 +246,12 @@
>   #define  LNA_GAIN_MAX_MINUS_36			0x05
>   #define  LNA_GAIN_MAX_MINUS_48			0x06
>   
> +#define  LNA_GAIN_MAX_CURRENT			0x08
> +#define  LNA_GAIN_MAX_MINUS_6_CURRENT		0x10
> +#define  LNA_GAIN_MAX_MINUS_12_CURRENT		0x18
> +#define  LNA_GAIN_MAX_MINUS_24_CURRENT		0x20
> +#define  LNA_GAIN_MAX_MINUS_36_CURRENT		0x28
> +#define  LNA_GAIN_MAX_MINUS_48_CURRENT		0x30
>   
>   /* RegRxBw (0x19) and RegAfcBw (0x1A) */
>   #define  MASK_BW_DCC_FREQ			0xE0
> 

Hi Valetin,

perfect :-)

Thank you so much!

Marcus

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

[Dan Carpenter]

You'll need to resend everything.  The thread is too messy.

On Wed, Dec 13, 2017 at 06:44:44PM +0100, Valentin Vidic wrote:
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index f77ecd60f43a..0889c65d5a31 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -340,14 +340,13 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)

The rf69_get_lna_gain() function is never called.  Just delete it.

Also the enum lnaGain is just a pointless abstraction which complicates
the code needlessly.  It will be deleted eventually too.

regards,
dan carpenter

[PATCH 6/8 v4] staging: pi433: remove unused function

[Valentin Vidic]

As it turns out rf69_get_lna_gain is not used at all.

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
v2: - drop change for SHIFT_DATAMODUL_MODULATION_TYPE
    - move shifting to the header file
v3: - drop auto case
    - use CURRENT suffix
    - precompute define values
v4: - drop the whole function since it is not called
      from anywhere

 drivers/staging/pi433/rf69.c | 18 ------------------
 drivers/staging/pi433/rf69.h |  1 -
 2 files changed, 19 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index f77ecd60f43a..2aae23a813a0 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -334,24 +334,6 @@ int rf69_set_lna_gain(struct spi_device *spi, enum lnaGain lnaGain)
 	}
 }
 
-enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
-{
-	u8 currentValue;
-
-	currentValue = rf69_read_reg(spi, REG_LNA);
-
-	switch (currentValue & MASK_LNA_CURRENT_GAIN >> 3) { // improvement: change 3 to define
-	case LNA_GAIN_AUTO:	    return automatic;
-	case LNA_GAIN_MAX:	    return max;
-	case LNA_GAIN_MAX_MINUS_6:  return maxMinus6;
-	case LNA_GAIN_MAX_MINUS_12: return maxMinus12;
-	case LNA_GAIN_MAX_MINUS_24: return maxMinus24;
-	case LNA_GAIN_MAX_MINUS_36: return maxMinus36;
-	case LNA_GAIN_MAX_MINUS_48: return maxMinus48;
-	default:		    return undefined;
-	}
-}
-
 int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg, enum dccPercent dccPercent)
 {
 	switch (dccPercent) {
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index e8803241b13b..e8640b0ee33b 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -39,7 +39,6 @@ int rf69_set_output_power_level(struct spi_device *spi, u8 powerLevel);
 int rf69_set_pa_ramp(struct spi_device *spi, enum paRamp paRamp);
 int rf69_set_antenna_impedance(struct spi_device *spi, enum antennaImpedance antennaImpedance);
 int rf69_set_lna_gain(struct spi_device *spi, enum lnaGain lnaGain);
-enum lnaGain rf69_get_lna_gain(struct spi_device *spi);
 int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg, enum dccPercent dccPercent);
 int rf69_set_dc_cut_off_frequency(struct spi_device *spi, enum dccPercent dccPercent);
 int rf69_set_dc_cut_off_frequency_during_afc(struct spi_device *spi, enum dccPercent dccPercent);
-- 
2.15.0

rf69_get_lna_gain

[Marcus Wolf]

Hi!

This is an information for all of you, doing experiments with real hardware!

I wanted to explain, what this lna_gain stuff is used for:

If you are receiving messages from different sender (let's say several 
thermometers), it may happen (e. g. due to different distance and 
different battery level) that the automatic gain control of the receiver 
amp isn't able to work correctly. E.g. if it gets a very strong singal 
first and a very weak afterwards, it is possible, that the agc isn't 
capable to recognize the second telegram predictable.

The procedure, need to be done in such a case is:
Switch on agc. Wait for a correct telegram to be received. Read the 
lna_gain_current value and store it. This is the gain setting for 
optimal reception for one of your sender. You now can set gain_select to 
this value, in order to receive stuff from this sender (instead of using 
agc).
If you want to receive stuff from an other sender, you may want to try 
with different other settings. As soon, as you have success, you can 
store this value and use it for receiving stuff from that sender.

Since I have just a 35m² flat, it is quite hard to have a setup, to test 
such stuff. In summer I did some experiments on the pavement, but the 
experimental code never was integrated in the productional source repo, 
because I focused on tx first. Deeper use of rx is planned for late 
spring next year.

If you want to do experiments with rx of signals with different 
strength, maybe you want to save a copy of the abstracions 
(rf69_set_lna_gain and rf69_get_lna_gain) befor they get deleted in 
staging-next.

Regards,

Marcus

Re: rf69_get_lna_gain

[Simon Sandström]

On Thu, Dec 14, 2017 at 06:08:11PM +0200, Marcus Wolf wrote:
> Hi!
> 
> This is an information for all of you, doing experiments with real hardware!
> 
> I wanted to explain, what this lna_gain stuff is used for:
> 
> If you are receiving messages from different sender (let's say several
> thermometers), it may happen (e. g. due to different distance and different
> battery level) that the automatic gain control of the receiver amp isn't
> able to work correctly. E.g. if it gets a very strong singal first and a
> very weak afterwards, it is possible, that the agc isn't capable to
> recognize the second telegram predictable.
> 
> The procedure, need to be done in such a case is:
> Switch on agc. Wait for a correct telegram to be received. Read the
> lna_gain_current value and store it. This is the gain setting for optimal
> reception for one of your sender. You now can set gain_select to this value,
> in order to receive stuff from this sender (instead of using agc).
> If you want to receive stuff from an other sender, you may want to try with
> different other settings. As soon, as you have success, you can store this
> value and use it for receiving stuff from that sender.
> 
> Since I have just a 35m² flat, it is quite hard to have a setup, to test
> such stuff. In summer I did some experiments on the pavement, but the
> experimental code never was integrated in the productional source repo,
> because I focused on tx first. Deeper use of rx is planned for late spring
> next year.
> 
> If you want to do experiments with rx of signals with different strength,
> maybe you want to save a copy of the abstracions (rf69_set_lna_gain and
> rf69_get_lna_gain) befor they get deleted in staging-next.
> 
> Regards,
> 
> Marcus
> 

Hi Marcus,

There is no need to make backups of code as it's still there in the git
history. If we ever need to re-introduce rf69_get_lna_gain, or any other
function that is removed due to not being used right now, it's just a
simple matter of reverting the commit that removed them. Either revert
them directly or use the revert as a base for re-introducing the
functionality.

So you don't have to feel like we're throwing away work that you've
probably spent lots of time on. It will still be available for us when
we actually need it. But right now, from what I've read in this thread,
the functionality isn't used at all right now (dead code) and should
therefore be removed.


Regards,
Simon

Re: [PATCH 6/8 v4] staging: pi433: remove unused function

[Greg Kroah-Hartman]

On Thu, Dec 14, 2017 at 04:20:20PM +0100, Valentin Vidic wrote:
> As it turns out rf69_get_lna_gain is not used at all.
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
> ---
> v2: - drop change for SHIFT_DATAMODUL_MODULATION_TYPE
>     - move shifting to the header file
> v3: - drop auto case
>     - use CURRENT suffix
>     - precompute define values
> v4: - drop the whole function since it is not called
>       from anywhere

This patch series is crazy, I have no idea how to pick out the "correct"
patches here at all.

Please just resend the whole thing, as 'v5', in order, so that I know
what to do.

thanks,

greg k-h

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

[PATCH 7/8] staging: pi433: avoid logging ENOMEM messages

[Valentin Vidic]

Fixes checkpatch warning:

  WARNING: Possible unnecessary 'out of memory' message

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
 drivers/staging/pi433/pi433_if.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 8234473e1add..9e558154a143 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -936,10 +936,8 @@ static int pi433_open(struct inode *inode, struct file *filp)
 
 	if (!device->rx_buffer) {
 		device->rx_buffer = kmalloc(MAX_MSG_SIZE, GFP_KERNEL);
-		if (!device->rx_buffer) {
-			dev_dbg(device->dev, "open/ENOMEM\n");
+		if (!device->rx_buffer)
 			return -ENOMEM;
-		}
 	}
 
 	device->users++;
-- 
2.15.0

[PATCH 8/8] staging: pi433: replace printk calls with dev_dbg

[Valentin Vidic]

Fixes checkpatch warning:

  WARNING: printk() should include KERN_<LEVEL> facility level

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
 drivers/staging/pi433/pi433_if.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 9e558154a143..02a5ba019801 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -720,7 +720,7 @@ pi433_tx_thread(void *data)
 			retval = wait_event_interruptible(device->fifo_wait_queue,
 							  device->free_in_fifo > 0);
 			if (retval) {
-				printk("ABORT\n");
+				dev_dbg(device->dev, "ABORT\n");
 				goto abort;
 			}
 		}
@@ -731,7 +731,7 @@ pi433_tx_thread(void *data)
 					 device->free_in_fifo == FIFO_SIZE ||
 					 kthread_should_stop());
 		if (kthread_should_stop())
-			printk("ABORT\n");
+			dev_dbg(device->dev, "ABORT\n");
 
 		/* STOP_TRANSMISSION */
 		dev_dbg(device->dev, "thread: Packet sent. Set mode to stby.");
-- 
2.15.0

Re: [PATCH 8/8] staging: pi433: replace printk calls with dev_dbg

[Dan Carpenter]

On Wed, Dec 13, 2017 at 03:21:56PM +0100, Valentin Vidic wrote:
>  drivers/staging/pi433/pi433_if.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 9e558154a143..02a5ba019801 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -720,7 +720,7 @@ pi433_tx_thread(void *data)
>  			retval = wait_event_interruptible(device->fifo_wait_queue,
>  							  device->free_in_fifo > 0);
>  			if (retval) {
> -				printk("ABORT\n");
> +				dev_dbg(device->dev, "ABORT\n");
>  				goto abort;

Hm...  The kernel.org version of this driver has never looked like this.
I wonder what you are working against...

regards,
dan carpenter

Re: [PATCH 8/8] staging: pi433: replace printk calls with dev_dbg

[Valentin Vidic]

On Wed, Dec 13, 2017 at 05:52:36PM +0300, Dan Carpenter wrote:
> On Wed, Dec 13, 2017 at 03:21:56PM +0100, Valentin Vidic wrote:
> >  drivers/staging/pi433/pi433_if.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > index 9e558154a143..02a5ba019801 100644
> > --- a/drivers/staging/pi433/pi433_if.c
> > +++ b/drivers/staging/pi433/pi433_if.c
> > @@ -720,7 +720,7 @@ pi433_tx_thread(void *data)
> >  			retval = wait_event_interruptible(device->fifo_wait_queue,
> >  							  device->free_in_fifo > 0);
> >  			if (retval) {
> > -				printk("ABORT\n");
> > +				dev_dbg(device->dev, "ABORT\n");
> >  				goto abort;
> 
> Hm...  The kernel.org version of this driver has never looked like this.
> I wonder what you are working against...

Patch was against staging-testing, in linux-next this printk looks like
this:

   if (retval) { printk("ABORT\n"); goto abort; }

-- 
Valentin