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(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 29bd37669059..a19afda5b188 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_chip_version(spi);
switch (retval) {
case 0x24:
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index d64df072d8e8..1516012f9bb7 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -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[] = {
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index b648ba5fff89..ca9b75267840 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_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);
--
2.25.4
On Thu, Jan 06, 2022 at 10:31:10PM +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. 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]>
Hi, Paulo.
Thanks for a patch.
I think it's good overall. But I have some opinion below.
> ---
> 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(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 29bd37669059..a19afda5b188 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_chip_version(spi);
>
> switch (retval) {
> case 0x24:
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index d64df072d8e8..1516012f9bb7 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -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;
> +}
> +
If we don't modify retval, why don't we just return directly without
retval?
> 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..ca9b75267840 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_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.
> 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
>
On Thu, Jan 06, 2022 at 10:31:10PM +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. 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(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 29bd37669059..a19afda5b188 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_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
On Thu, Jan 06, 2022 at 10:19:46AM +0000, Sidong Yang wrote:
> > +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.
> > @@ -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.
On Thu, Jan 06, 2022 at 03:04:09PM +0100, Greg KH wrote:
> > --- 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_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.
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 <[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(-)
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
On Thu, Jan 06, 2022 at 10:31:10PM +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. 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
>
> 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
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
On Fri, Jan 07, 2022 at 11:32:58AM +0300, Dan Carpenter wrote:
> > 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
> >
> > 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
>
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/[email protected]/
If it is, I'm happy to add it back but I just wanted to confirm it
first.
thanks,
Paulo A.
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/[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
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/[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.
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
>
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/[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.
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.
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(-)
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