2017-12-13 15:06:12

by Valentin Vidic

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

Fixes checkpatch warning:

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

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-13 14:48:25

by Dan Carpenter

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

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 <[email protected]>
> ---
> 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

2017-12-13 14:49:59

by Dan Carpenter

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

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 <[email protected]>

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

regards,
dan carpenter

2017-12-13 14:53:03

by Dan Carpenter

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

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

2017-12-13 15:06:07

by Valentin Vidic

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

Fixes checkpatch warning:

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

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-13 15:06:16

by Valentin Vidic

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

Fixes checkpatch warning:

WARNING: Possible unnecessary 'out of memory' message

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-13 15:06:20

by Valentin Vidic

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

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 <[email protected]>
---
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

2017-12-13 15:06:28

by Valentin Vidic

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

Fixes checkpatch warning:

WARNING: Missing a blank line after declarations

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-13 15:06:45

by Valentin Vidic

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

Fixes checkpatch warning:

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

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-13 15:06:57

by Dan Carpenter

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

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 <[email protected]>
> ---
> 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

2017-12-13 15:07:50

by Valentin Vidic

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

Avoid shifting by magic numbers and use defines instead:

SHIFT_DATAMODUL_MODULATION_TYPE
SHIFT_LNA_CURRENT_GAIN

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-13 15:08:05

by Valentin Vidic

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

Fixes checkpatch warning:

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

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-13 15:23:22

by Valentin Vidic

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

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

2017-12-13 15:24:25

by Marcus Wolf

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

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 <[email protected]>
> ---
> 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

2017-12-13 15:32:44

by Marcus Wolf

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

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 <[email protected]>
> ---
> 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

2017-12-13 15:57:59

by Valentin Vidic

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

Fixes checkpatch warning:

WARNING: Missing a blank line after declarations

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-13 16:55:39

by Valentin Vidic

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

Avoid shifting by magic numbers and use defines instead.

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-13 16:58:48

by Valentin Vidic

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 <[email protected]>
> > ---
> > 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

2017-12-13 17:15:21

by Marcus Wolf

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

Am 13.12.2017 um 18:55 schrieb Valentin Vidic:
> Avoid shifting by magic numbers and use defines instead.
>
> Signed-off-by: Valentin Vidic <[email protected]>
> ---
> 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

2017-12-13 17:45:10

by Valentin Vidic

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

Avoid shifting by magic numbers and use defines instead.

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-13 17:52:39

by Marcus Wolf

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

Am 13.12.2017 um 19:44 schrieb Valentin Vidic:
> Avoid shifting by magic numbers and use defines instead.
>
> Signed-off-by: Valentin Vidic <[email protected]>
> ---
> 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

2017-12-13 19:01:42

by Joe Perches

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

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.

2017-12-13 19:46:17

by Valentin Vidic

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

Fix variable naming and checkpatch warning:

WARNING: Missing a blank line after declarations

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-14 14:52:56

by Dan Carpenter

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

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

2017-12-14 15:20:33

by Valentin Vidic

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

As it turns out rf69_get_lna_gain is not used at all.

Signed-off-by: Valentin Vidic <[email protected]>
---
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

2017-12-14 16:08:26

by Marcus Wolf

Subject: rf69_get_lna_gain

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

2017-12-14 18:14:04

by Simon Sandström

Subject: Re: rf69_get_lna_gain

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

2017-12-19 14:12:24

by Greg Kroah-Hartman

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

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 <[email protected]>
> ---
> 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