staging: pi433: move get version func to where all other functions are

PATCH Superseded Paulo Miguel Almeida 4 participants 15 messages Jan 6, 2022 – Jan 8, 2022 Staging

Share & tools

Paulo Miguel Almeida

author

2022-01-06 09:31 UTC #0 lore raw

PATCH 1/1 staging: pi433: move get version func to where all other functions are

As a convention for the pi433 driver, all routines that deals with the rf69 chip are defined in the rf69.c file. There was an exception in which the uC version verification was being done directly elsewhere. While at it, the Version Register hardcoded value was replaced with a pre-existing constant in the driver.

This patch adds rf69_get_chip_version function to rf69.c

Additionally, the patch below must be applied first as it was sent before and touches the same file.
https://lore.kernel.org/lkml/[email protected]/

Signed-off-by Paulo Miguel Almeida <[email protected]>

---
 drivers/staging/pi433/pi433_if.c | 4 +---
 drivers/staging/pi433/rf69.c     | 8 ++++++++
 drivers/staging/pi433/rf69.h     | 1 +
 3 files changed, 10 insertions(+), 3 deletions(-)

drivers/staging/pi433/pi433_if.c +1 -3

index 29bd37669059..a19afda5b188 100644
@@ -1116,9 +1116,7 @@ static int pi433_probe(struct spi_device *spi)
 		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)
-		return retval;
+	retval = rf69_get_chip_version(spi);
 
 	switch (retval) {
 	case 0x24:

drivers/staging/pi433/rf69.c +8 -0

index d64df072d8e8..1516012f9bb7 100644
@@ -102,6 +102,14 @@ static inline int rf69_read_mod_write(struct spi_device *spi, u8 reg,
 
 /*-------------------------------------------------------------------------*/
 
+int rf69_get_chip_version(struct spi_device *spi)
+{
+	int retval;
+
+	retval = rf69_read_reg(spi, REG_VERSION);
+	return retval;
+}
+
 int rf69_set_mode(struct spi_device *spi, enum mode mode)
 {
 	static const u8 mode_map[] = {

drivers/staging/pi433/rf69.h +1 -0

index b648ba5fff89..ca9b75267840 100644
@@ -17,6 +17,7 @@
 #define FIFO_SIZE	66		/* bytes */
 #define FIFO_THRESHOLD	15		/* bytes */
 
+int rf69_get_chip_version(struct spi_device *spi);
 int rf69_set_mode(struct spi_device *spi, enum mode mode);
 int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
 int rf69_set_modulation(struct spi_device *spi, enum modulation modulation);

Signature · 1 line

2.25.4

Sidong Yang

2022-01-06 10:19 UTC #1 lore raw

Quoted 6 lines · Paulo Miguel Almeida

As a convention for the pi433 driver, all routines that deals with the rf69 chip are defined in the rf69.c file. There was an exception in which the uC version verification was being done directly elsewhere. While at it, the Version Register hardcoded value was replaced with a pre-existing constant in the driver.

Quoted 7 lines

This patch adds rf69_get_chip_version function to rf69.c
Additionally, the patch below must be applied first as it was sent before and touches the same file.
https://lore.kernel.org/lkml/[email protected]/
Signed-off-by Paulo Miguel Almeida <[email protected]>

Hi, Paulo.
Thanks for a patch.

I think it's good overall. But I have some opinion below.

Quoted 27 lines

---
 drivers/staging/pi433/pi433_if.c | 4 +---
 drivers/staging/pi433/rf69.c     | 8 ++++++++
 drivers/staging/pi433/rf69.h     | 1 +
 3 files changed, 10 insertions(+), 3 deletions(-)

drivers/staging/pi433/pi433_if.c +1 -3

index 29bd37669059..a19afda5b188 100644
@@ -1116,9 +1116,7 @@ static int pi433_probe(struct spi_device *spi)
 		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)
-		return retval;
+	retval = rf69_get_chip_version(spi);
 
 	switch (retval) {
 	case 0x24:

drivers/staging/pi433/rf69.c +0 -0

index d64df072d8e8..1516012f9bb7 100644
@@ -102,6 +102,14 @@ static inline int rf69_read_mod_write(struct spi_device *spi, u8 reg,

drivers/staging/pi433/rf69.c static inline int rf69_read_mod_write(struct spi_device *spi, u8 reg, · line 112

Quoted 10 lines

drivers/staging/pi433/rf69.c +8 -0

 /*-------------------------------------------------------------------------*/
 
+int rf69_get_chip_version(struct spi_device *spi)
+{
+	int retval;
+
+	retval = rf69_read_reg(spi, REG_VERSION);
+	return retval;
+}
+

If we don't modify retval, why don't we just return directly without retval?

drivers/staging/pi433/rf69.h line 20

Quoted 12 lines

drivers/staging/pi433/rf69.c +0 -0

 int rf69_set_mode(struct spi_device *spi, enum mode mode)
 {
 	static const u8 mode_map[] = {

drivers/staging/pi433/rf69.h +1 -0

index b648ba5fff89..ca9b75267840 100644
@@ -17,6 +17,7 @@
 #define FIFO_SIZE	66		/* bytes */
 #define FIFO_THRESHOLD	15		/* bytes */
 
+int rf69_get_chip_version(struct spi_device *spi);

IMHO, I think that we don't need to include 'chip'. Because all other functions in this code don't have 'chip' in function name. and version code seems to be more accurate representation.

Quoted 6 lines

drivers/staging/pi433/rf69.h +0 -0

 int rf69_set_mode(struct spi_device *spi, enum mode mode);
 int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
 int rf69_set_modulation(struct spi_device *spi, enum modulation modulation);

Signature · 1 line

2.25.4

Paulo Miguel Almeida

author

2022-01-06 20:14 UTC #2 lore raw

Quoted 10 lines · Sidong Yang

drivers/staging/pi433/rf69.c static inline int rf69_read_mod_write(struct spi_device *spi, u8 reg, · line 112
Quoted 8 lines
drivers/staging/pi433/rf69.c +8 -0
+int rf69_get_chip_version(struct spi_device *spi)
+{
+	int retval;
+
+	retval = rf69_read_reg(spi, REG_VERSION);
+	return retval;
+}
+
If we don't modify retval, why don't we just return directly without retval?

fair point, I will change that.

Quoted 9 lines

drivers/staging/pi433/rf69.h line 20
Quoted 5 lines
drivers/staging/pi433/rf69.h +1 -0
@@ -17,6 +17,7 @@
 #define FIFO_SIZE	66		/* bytes */
 #define FIFO_THRESHOLD	15		/* bytes */
 
+int rf69_get_chip_version(struct spi_device *spi);
IMHO, I think that we don't need to include 'chip'. Because all other functions in this code don't have 'chip' in function name. and version code seems to be more accurate representation.

will change that too. Thanks for taking the time to review this patch.

thanks,

Paulo A.

Paulo Miguel Almeida

author

2022-01-06 21:33 UTC #3 lore raw

PATCH v2 1/1 staging: pi433: move get version func to where all other functions are

This patch adds rf69_get_version function to rf69.c

Signed-off-by Paulo Miguel Almeida <[email protected]>

---
v2: function names where altered to match suggestions given during code review. Requester: Sidong Yang
v1: https://lore.kernel.org/lkml/[email protected]/

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

drivers/staging/pi433/pi433_if.c +1 -3

index 68c09fa016ed..1372361d56e1 100644
@@ -1116,9 +1116,7 @@ static int pi433_probe(struct spi_device *spi)
 		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)
-		return retval;
+	retval = rf69_get_version(spi);
 
 	switch (retval) {
 	case 0x24:

drivers/staging/pi433/rf69.c +5 -0

index d64df072d8e8..ee8c81d164e1 100644
@@ -102,6 +102,11 @@ static inline int rf69_read_mod_write(struct spi_device *spi, u8 reg,
 
 /*-------------------------------------------------------------------------*/
 
+int rf69_get_version(struct spi_device *spi)
+{
+	return rf69_read_reg(spi, REG_VERSION);
+}
+
 int rf69_set_mode(struct spi_device *spi, enum mode mode)
 {
 	static const u8 mode_map[] = {

drivers/staging/pi433/rf69.h +1 -0

index b648ba5fff89..c25942f142a6 100644
@@ -17,6 +17,7 @@
 #define FIFO_SIZE	66		/* bytes */
 #define FIFO_THRESHOLD	15		/* bytes */
 
+int rf69_get_version(struct spi_device *spi);
 int rf69_set_mode(struct spi_device *spi, enum mode mode);
 int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
 int rf69_set_modulation(struct spi_device *spi, enum modulation modulation);

Signature · 1 line

2.25.4

Dan Carpenter

2022-01-07 08:54 UTC #4 lore raw

Quoted 2 lines · Paulo Miguel Almeida

As a convention for the pi433 driver, all routines that deals with the rf69 chip are defined in the rf69.c file.

That's some EnterpriseQuality[tm] style guidelines. It's an over fussy rule that just makes the code harder to read for no reason.

Quoted 2 lines

While at it, the Version Register hardcoded value was replaced with a pre-existing constant in the driver.

This is good, though.

drivers/staging/pi433/pi433_if.c static int pi433_probe(struct spi_device *spi) · line 1118

Quoted 8 lines

drivers/staging/pi433/pi433_if.c +0 -0

index 68c09fa016ed..1372361d56e1 100644
@@ -1116,9 +1116,7 @@ static int pi433_probe(struct spi_device *spi)
 		spi->mode, spi->bits_per_word, spi->max_speed_hz);
 
 	/* Ping the chip by reading the version register */

This comment doesn't make sense now.

drivers/staging/pi433/pi433_if.c static int pi433_probe(struct spi_device *spi) · line 1119

Quoted 4 lines

drivers/staging/pi433/pi433_if.c +1 -3

-	retval = spi_w8r8(spi, 0x10);
-	if (retval < 0)
-		return retval;
+	retval = rf69_get_version(spi);

Just say:

	retval = rf69_read_reg(spi, REG_VERSION);
	if (retval < 0)
		return retval;

Deleting the error handling was a bad style choice. Also preserve the error code.

regards,
dan carpenter

Paulo Miguel Almeida

author

2022-01-07 19:24 UTC #5 lore raw

Quoted 6 lines · Dan Carpenter

Quoted 2 lines · Paulo Miguel Almeida
As a convention for the pi433 driver, all routines that deals with the rf69 chip are defined in the rf69.c file.
That's some EnterpriseQuality[tm] style guidelines. It's an over fussy rule that just makes the code harder to read for no reason.

EnterpriseQuality[tm] was witty :)

Quoted 4 lines

Quoted 2 lines
/* Ping the chip by reading the version register */
This comment doesn't make sense now.

you are right, I will change this.

Quoted 8 lines

drivers/staging/pi433/pi433_if.c static int pi433_probe(struct spi_device *spi) · line 1119
Quoted 4 lines
drivers/staging/pi433/pi433_if.c +1 -3
-	retval = spi_w8r8(spi, 0x10);
-	if (retval < 0)
-		return retval;
+	retval = rf69_get_version(spi);
Just say:
	retval = rf69_read_reg(spi, REG_VERSION);

atm rf69_read_reg is a static function in rf69.c.

I do agree that this is technically possible to write the code exactly as you suggested but on the other hand, that would be the only exception to the rule when considering all other higher-level functions provided in the rf69.c

are you strongly set on the rf69_read_reg approach or would you be open to keep the original approach? (rf69_get_version)

Quoted 6 lines

	if (retval < 0)
		return retval;
Deleting the error handling was a bad style choice. Also preserve the error code.

I just want to double-check if this suggestion is taking into consideration what was mentioned here:
https://lore.kernel.org/lkml/[email protected]/

If it is, I'm happy to add it back but I just wanted to confirm it first.

thanks,

Paulo A.

Dan Carpenter

2022-01-08 11:19 UTC #6 lore raw

Quoted 7 lines · Paulo Miguel Almeida

Quoted 3 lines · Dan Carpenter
Just say:
	retval = rf69_read_reg(spi, REG_VERSION);
atm rf69_read_reg is a static function in rf69.c.

I would remove be the static.

Quoted 4 lines

I do agree that this is technically possible to write the code exactly as you suggested but on the other hand, that would be the only exception to the rule when considering all other higher-level functions provided in the rf69.c

There may be other functions which will benefit from this later on. So instead of "only" a better word is "first". Some of those high level functions make sense because they are slightly complicated and have multiple callers. But in this case open coding it seems easier to read.

Quoted 3 lines

are you strongly set on the rf69_read_reg approach or would you be open to keep the original approach? (rf69_get_version)

I think my approach is best but I don't care.

Quoted 8 lines

Quoted 7 lines
	if (retval < 0)
		return retval;
Deleting the error handling was a bad style choice. Also preserve the error code.

Quoted 6 lines

I just want to double-check if this suggestion is taking into consideration what was mentioned here:
https://lore.kernel.org/lkml/[email protected]/
If it is, I'm happy to add it back but I just wanted to confirm it first.

Yes. Keep the error handling. Your way makes the code more complicated to read.

regards,
dan carpenter

Sidong Yang

2022-01-08 16:36 UTC #7 lore raw

On Sat, Jan 08, 2022 at 02:19:10PM +0300, Dan Carpenter wrote:

Hi, Paul.
Thanks for applying my opinion. And there is one thing to metion.

Quoted 39 lines · Paulo Miguel Almeida

Quoted 7 lines · Dan Carpenter
Quoted 3 lines
Just say:
	retval = rf69_read_reg(spi, REG_VERSION);
atm rf69_read_reg is a static function in rf69.c.
I would remove be the static.
Quoted 4 lines
I do agree that this is technically possible to write the code exactly as you suggested but on the other hand, that would be the only exception to the rule when considering all other higher-level functions provided in the rf69.c
There may be other functions which will benefit from this later on. So instead of "only" a better word is "first". Some of those high level functions make sense because they are slightly complicated and have multiple callers. But in this case open coding it seems easier to read.
Quoted 3 lines
are you strongly set on the rf69_read_reg approach or would you be open to keep the original approach? (rf69_get_version)
I think my approach is best but I don't care.
Quoted 12 lines
Quoted 7 lines
	if (retval < 0)
		return retval;
Deleting the error handling was a bad style choice. Also preserve the error code.
I just want to double-check if this suggestion is taking into consideration what was mentioned here:
https://lore.kernel.org/lkml/[email protected]/

Quoted 5 lines

Quoted 2 lines
If it is, I'm happy to add it back but I just wanted to confirm it first.
Yes. Keep the error handling. Your way makes the code more complicated to read.

I totally really agree with it.
Because the switch clause under this patch catches error with 'default:' but it returns '-ENODEV'. I worried that it lost retval from reading register if it has error.

Quoted 4 lines

regards,
dan carpenter

Paulo Miguel Almeida

author

2022-01-08 21:02 UTC #8 lore raw

Quoted 10 lines · Sidong Yang

Quoted 3 lines · Dan Carpenter
Yes. Keep the error handling. Your way makes the code more complicated to read.
I totally really agree with it.
Because the switch clause under this patch catches error with 'default:' but it returns '-ENODEV'. I worried that it lost retval from reading register if it has error.

I will add it back to the patch.

thanks,

Paulo A.

Paulo Miguel Almeida

author

2022-01-08 21:00 UTC #9 lore raw

In reply to Dan Carpenter

Quoted 4 lines · Dan Carpenter

Quoted 2 lines
are you strongly set on the rf69_read_reg approach or would you be open to keep the original approach? (rf69_get_version)
I think my approach is best but I don't care.

Thanks for being so flexible. I'll keep your suggestion at the back of my head and if I come across more scenarios in which rf69_read_reg would be the easiest way, I will gladly send a different patch to make it reality.

Quoted 10 lines

Quoted 6 lines
I just want to double-check if this suggestion is taking into consideration what was mentioned here:
https://lore.kernel.org/lkml/[email protected]/
If it is, I'm happy to add it back but I just wanted to confirm it first.
Yes. Keep the error handling. Your way makes the code more complicated to read.

Agreed. I will add it back.

thanks,

Paulo A.

Paulo Miguel Almeida

author

2022-01-08 21:27 UTC #10 lore raw

PATCH v3 1/1 staging: pi433: move get version func to where all other functions are

As a convention for the pi433 driver, all routines that deals with the rf69 chip are defined in the rf69.c file. There was an exception to the rule in which the uC version verification was being done directly elsewhere. While at it, the Version Register hardcoded value was replaced with a pre-existing constant in the driver.

This patch adds rf69_get_version function to rf69.c

Signed-off-by Paulo Miguel Almeida <[email protected]>

---
v3: add original validation right after reading version value via spi.
Requesters: Dan Carpenter, Sidong Yang, Greg k-h
v2: function names where altered to match suggestions given during code review. Requester: Sidong Yang
v1: https://lore.kernel.org/lkml/[email protected]/

---
 drivers/staging/pi433/pi433_if.c | 4 ++--
 drivers/staging/pi433/rf69.c     | 5 +++++
 drivers/staging/pi433/rf69.h     | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)

drivers/staging/pi433/pi433_if.c +2 -2

index 68c09fa016ed..051c9052aeeb 100644
@@ -1115,8 +1115,8 @@ static int pi433_probe(struct spi_device *spi)
 		"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);
+	/* read chip version */
+	retval = rf69_get_version(spi);
 	if (retval < 0)
 		return retval;

drivers/staging/pi433/rf69.c +5 -0

index d64df072d8e8..ee8c81d164e1 100644
@@ -102,6 +102,11 @@ static inline int rf69_read_mod_write(struct spi_device *spi, u8 reg,
 
 /*-------------------------------------------------------------------------*/
 
+int rf69_get_version(struct spi_device *spi)
+{
+	return rf69_read_reg(spi, REG_VERSION);
+}
+
 int rf69_set_mode(struct spi_device *spi, enum mode mode)
 {
 	static const u8 mode_map[] = {

drivers/staging/pi433/rf69.h +1 -0

index b648ba5fff89..c25942f142a6 100644
@@ -17,6 +17,7 @@
 #define FIFO_SIZE	66		/* bytes */
 #define FIFO_THRESHOLD	15		/* bytes */
 
+int rf69_get_version(struct spi_device *spi);
 int rf69_set_mode(struct spi_device *spi, enum mode mode);
 int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
 int rf69_set_modulation(struct spi_device *spi, enum modulation modulation);

Signature · 1 line

2.25.4

Greg KH

maintainer

2022-01-06 14:04 UTC #11 lore raw

In reply to Paulo Miguel Almeida

Quoted 24 lines · Paulo Miguel Almeida

As a convention for the pi433 driver, all routines that deals with the rf69 chip are defined in the rf69.c file. There was an exception in which the uC version verification was being done directly elsewhere. While at it, the Version Register hardcoded value was replaced with a pre-existing constant in the driver.
This patch adds rf69_get_chip_version function to rf69.c
Additionally, the patch below must be applied first as it was sent before and touches the same file.
https://lore.kernel.org/lkml/[email protected]/
Signed-off-by Paulo Miguel Almeida <[email protected]>
---
 drivers/staging/pi433/pi433_if.c | 4 +---
 drivers/staging/pi433/rf69.c     | 8 ++++++++
 drivers/staging/pi433/rf69.h     | 1 +
 3 files changed, 10 insertions(+), 3 deletions(-)
drivers/staging/pi433/pi433_if.c +0 -0
index 29bd37669059..a19afda5b188 100644

drivers/staging/pi433/pi433_if.c static int pi433_probe(struct spi_device *spi) · line 1119

Quoted 7 lines

drivers/staging/pi433/pi433_if.c +1 -3

@@ -1116,9 +1116,7 @@ static int pi433_probe(struct spi_device *spi)
 		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)
-		return retval;
+	retval = rf69_get_chip_version(spi);

This can not fail anymore, like it used to be able to. So I think you just broke the functionality for why this call was being made in the first place (i.e. ping the chip to see if it was alive, and fail if it is not.)

thanks,

greg k-h

Paulo Miguel Almeida

author

2022-01-06 21:01 UTC #12 lore raw

Quoted 5 lines · Greg KH

Quoted 5 lines

drivers/staging/pi433/pi433_if.c +0 -0

@@ -1116,9 +1116,7 @@ static int pi433_probe(struct spi_device *spi)
 		spi->mode, spi->bits_per_word, spi->max_speed_hz);

Quoted 11 lines

drivers/staging/pi433/pi433_if.c static int pi433_probe(struct spi_device *spi) · line 1119
Quoted 5 lines
drivers/staging/pi433/pi433_if.c +1 -3
 	/* Ping the chip by reading the version register */
-	retval = spi_w8r8(spi, 0x10);
-	if (retval < 0)
-		return retval;
+	retval = rf69_get_chip_version(spi);
This can not fail anymore, like it used to be able to. So I think you just broke the functionality for why this call was being made in the first place (i.e. ping the chip to see if it was alive, and fail if it is not.)

I thought that this if statement was somewhat redudant because right after obtaining the chip version, there is a switch statement that checks if the value is what we expect or return an error otherwise.

Unfortunately, in the patch file generated the whole switch statement isn't visible so I admit that it looks funny at first. I will paste the routine here:

/* Ping the chip by reading the version register */ retval = rf69_get_chip_version(spi);

switch (retval) {
case 0x24:

	dev_dbg(&spi->dev, "found pi433 (ver. 0x%x)", retval);
	break;
default:
	dev_dbg(&spi->dev, "unknown chip version: 0x%x", retval);
	return -ENODEV;
}

Let me know if you agree with the approach I've taken, otherwise I am more than happy to add the original if statement if you think I'm missing any edge case here.

Once again, thanks for taking the time to review my patch :)

thanks,

Paulo A.

Dan Carpenter

2022-01-07 08:33 UTC #13 lore raw

In reply to Paulo Miguel Almeida

Quoted 11 lines · Paulo Miguel Almeida

As a convention for the pi433 driver, all routines that deals with the rf69 chip are defined in the rf69.c file. There was an exception in which the uC version verification was being done directly elsewhere. While at it, the Version Register hardcoded value was replaced with a pre-existing constant in the driver.
This patch adds rf69_get_chip_version function to rf69.c
Additionally, the patch below must be applied first as it was sent before and touches the same file.
https://lore.kernel.org/lkml/[email protected]/

Put these sorts of meta commentary underneath the --- cut off

Quoted 3 lines

Signed-off-by Paulo Miguel Almeida <[email protected]>
---

^^^

here. We don't need to preserve that information in the git log for all of history.

Other people have basically said all the other stuff that I was going to say...

regards,
dan carpenter

Paulo Miguel Almeida

author

2022-01-07 18:45 UTC #14 lore raw

Quoted 5 lines · Dan Carpenter

Quoted 3 lines
Additionally, the patch below must be applied first as it was sent before and touches the same file.
https://lore.kernel.org/lkml/[email protected]/
Put these sorts of meta commentary underneath the --- cut off

Thanks, I didn't know. Will start doing this from now onwards

Quoted 14 lines

Quoted 3 lines
Signed-off-by Paulo Miguel Almeida <[email protected]>
---
^^^
here. We don't need to preserve that information in the git log for all of history.
Other people have basically said all the other stuff that I was going to say...
regards,
dan carpenter