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

[PATCH v2] staging: pi433: move get version func to where all other functions are

Posted by Paulo Miguel Almeida 4 years, 5 months ago

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_version function to rf69.c

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
v2: function names where altered to match suggestions given during code
review. Requester: Sidong Yang
v1: https://lore.kernel.org/lkml/20220106093110.GA20011@mail.google.com/
---
 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(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 68c09fa016ed..1372361d56e1 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -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:
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index d64df072d8e8..ee8c81d164e1 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -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[] = {
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index b648ba5fff89..c25942f142a6 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -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);
-- 
2.25.4

Re: [PATCH v2] staging: pi433: move get version func to where all other functions are

Posted by Dan Carpenter 4 years, 5 months ago

On Fri, Jan 07, 2022 at 10:33:25AM +1300, Paulo Miguel Almeida wrote:
> 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.

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

This is good, though.

> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 68c09fa016ed..1372361d56e1 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -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.

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

Re: [PATCH v2] staging: pi433: move get version func to where all other functions are

Posted by Paulo Miguel Almeida 4 years, 5 months ago

On Fri, Jan 07, 2022 at 11:53:44AM +0300, Dan Carpenter wrote:
> On Fri, Jan 07, 2022 at 10:33:25AM +1300, Paulo Miguel Almeida wrote:
> > 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 :)

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

you are right, I will change this.

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

> 	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/20220106210134.GB3416@mail.google.com/

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

thanks,

Paulo A.

Re: [PATCH v2] staging: pi433: move get version func to where all other functions are

Posted by Dan Carpenter 4 years, 5 months ago

On Sat, Jan 08, 2022 at 08:24:38AM +1300, Paulo Miguel Almeida wrote:
> On Fri, Jan 07, 2022 at 11:53:44AM +0300, Dan Carpenter wrote:
> > 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.

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

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

> 
> > 	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/20220106210134.GB3416@mail.google.com/ 
> 
> 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

Re: [PATCH v2] staging: pi433: move get version func to where all other functions are

Posted by Sidong Yang 4 years, 5 months ago

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.

> On Sat, Jan 08, 2022 at 08:24:38AM +1300, Paulo Miguel Almeida wrote:
> > On Fri, Jan 07, 2022 at 11:53:44AM +0300, Dan Carpenter wrote:
> > > 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.
> 
> > 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.
> 
> > 
> > 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.
> 
> > 
> > > 	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/20220106210134.GB3416@mail.google.com/ 
> > 
> > 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.

> 
> regards,
> dan carpenter
>

Re: [PATCH v2] staging: pi433: move get version func to where all other functions are

Posted by Paulo Miguel Almeida 4 years, 5 months ago

On Sat, Jan 08, 2022 at 04:36:21PM +0000, Sidong Yang wrote:
> On Sat, Jan 08, 2022 at 02:19:10PM +0300, Dan Carpenter wrote:
> > 
> > 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.

Re: [PATCH v2] staging: pi433: move get version func to where all other functions are

Posted by Paulo Miguel Almeida 4 years, 5 months ago

On Sat, Jan 08, 2022 at 02:19:10PM +0300, Dan Carpenter wrote:
> > 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.

> > I just want to double-check if this suggestion is taking into
> > consideration what was mentioned here:
> > https://lore.kernel.org/lkml/20220106210134.GB3416@mail.google.com/ 
> > 
> > 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.

[PATCH v3] staging: pi433: move get version func to where all other functions are

Posted by Paulo Miguel Almeida 4 years, 5 months ago

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 <paulo.miguel.almeida.rodenas@gmail.com>
---
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/20220106093110.GA20011@mail.google.com/
---
 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(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 68c09fa016ed..051c9052aeeb 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -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;
 
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index d64df072d8e8..ee8c81d164e1 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -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[] = {
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index b648ba5fff89..c25942f142a6 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -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);
-- 
2.25.4