Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions

2017-12-06 Thread Simon Sandström 
On Wed, Dec 06, 2017 at 04:16:04PM +0100, Greg KH wrote:
> 
> I had to stop here in applying this series, as the merge conflicts just
> got too much for me to resolve by hand.
> 
> Can you rebase this series on my staging-testing branch of staging.git
> and send the remaining patches please?
> 
> thanks,
> 
> greg k-h

Hi,

Yes, I'll send a new patchset with the remaining patches.


Regards,
Simon

Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions

2017-12-06 Thread Greg KH 
On Tue, Dec 05, 2017 at 11:08:44PM +0100, Simon Sandström wrote:
> Splits rf69_set_crc_enabled(dev, enabled) into
> rf69_enable_crc(dev) and rf69_disable_crc(dev).
> 
> Signed-off-by: Simon Sandström 
> ---
>  drivers/staging/pi433/pi433_if.c | 22 --
>  drivers/staging/pi433/rf69.c | 18 ++
>  drivers/staging/pi433/rf69.h |  4 ++--
>  3 files changed, 28 insertions(+), 16 deletions(-)

I had to stop here in applying this series, as the merge conflicts just
got too much for me to resolve by hand.

Can you rebase this series on my staging-testing branch of staging.git
and send the remaining patches please?

thanks,

greg k-h

Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions

2017-12-06 Thread Greg KH 
On Wed, Dec 06, 2017 at 12:07:20PM +0200, Marcus Wolf wrote:
> 
> 
> Am 06.12.2017 um 11:37 schrieb Dan Carpenter:
> > On Wed, Dec 06, 2017 at 11:05:22AM +0200, Marcus Wolf wrote:
> > > 
> > > 
> > > Am 06.12.2017 um 00:08 schrieb Simon Sandström:
> > > > Splits rf69_set_crc_enabled(dev, enabled) into
> > > > rf69_enable_crc(dev) and rf69_disable_crc(dev).
> > > > 
> > > > Signed-off-by: Simon Sandström 
> > > > ---
> > > >drivers/staging/pi433/pi433_if.c | 22 --
> > > >drivers/staging/pi433/rf69.c | 18 ++
> > > >drivers/staging/pi433/rf69.h |  4 ++--
> > > >3 files changed, 28 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/pi433/pi433_if.c 
> > > > b/drivers/staging/pi433/pi433_if.c
> > > > index 2ae19ac565d1..614eec7dd904 100644
> > > > --- a/drivers/staging/pi433/pi433_if.c
> > > > +++ b/drivers/staging/pi433/pi433_if.c
> > > > @@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
> > > > pi433_rx_cfg *rx_cfg)
> > > > return ret;
> > > > }
> > > > SET_CHECKED(rf69_set_adressFiltering(dev->spi, 
> > > > rx_cfg->enable_address_filtering));
> > > > -   SET_CHECKED(rf69_set_crc_enable (dev->spi, 
> > > > rx_cfg->enable_crc));
> > > > +
> > > > +   if (rx_cfg->enable_crc == OPTION_ON) {
> > > > +   ret = rf69_enable_crc(dev->spi);
> > > > +   if (ret < 0)
> > > > +   return ret;
> > > > +   } else {
> > > > +   ret = rf69_disable_crc(dev->spi);
> > > > +   if (ret < 0)
> > > > +   return ret;
> > > > +   }
> > > 
> > > Why don't you use SET_CHECKED(...)?
> > > 
> > 
> > Marcus, please don't introduce new uses of SET_CHECKED().  It has a
> > hidden return in it which is against kernel style and introduces very
> > predictable and avoidable bugs.  For example, in probe().
> 
> Ah ok.
> 
> Thanks for clarifiytion!
> 
> What a pitty - another bunch of extra lines of code...
> 
> Or is there an other construction, allowing for one line per register
> change? Something like
> 
>   ret = rf69_set_xyz(...); if (ret) return ret;
>   ret = rf69_set_abc(...); if (ret) return ret;
> 
> is pretty ugly and voids the style guide...

Just spell it out:
ret = rf69_set_xyz();
if (ret)
goto unwind_xyz;

Almost never do you want to instantly return.  You should clean up from
the error first.

But if you do just want to exit, that's fine too, just return.  That's
the normal way here, don't do funny things in macros (like return from a
function), that way lies madness...

thanks,

greg k-h

Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions

2017-12-06 Thread Marcus Wolf 


>>
>> rf69 -set/get - action
>> -> rf69_set_crc_enable
>
> No...  Simon's name is better.  His is shorter and makes more sense.


I disagree. If I am going to implement a new functionality and need to 
think about the naming of the function name, every time I need to change 
a register setting that's awfull.


I usually have code on one monitor and datasheet on the other. So if I 
want to set a bit/reg/whatever, I have the datasheet in front of my 
nose. I can easy write the code, if function names refer to the names in 
the datasheet and follow a strict naming convention. If the naming 
convetion is broken, I need to switch to the header and search manually 
for each register, I want to set.



There is so much potential in this young driver, that could be 
developed. Would be pitty, if all that wouldn't take place some day.



Marcus

Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions

2017-12-06 Thread Marcus Wolf 



Am 06.12.2017 um 11:37 schrieb Dan Carpenter:

On Wed, Dec 06, 2017 at 11:05:22AM +0200, Marcus Wolf wrote:



Am 06.12.2017 um 00:08 schrieb Simon Sandström:

Splits rf69_set_crc_enabled(dev, enabled) into
rf69_enable_crc(dev) and rf69_disable_crc(dev).

Signed-off-by: Simon Sandström 
---
   drivers/staging/pi433/pi433_if.c | 22 --
   drivers/staging/pi433/rf69.c | 18 ++
   drivers/staging/pi433/rf69.h |  4 ++--
   3 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 2ae19ac565d1..614eec7dd904 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
return ret;
}
SET_CHECKED(rf69_set_adressFiltering(dev->spi, 
rx_cfg->enable_address_filtering));
-   SET_CHECKED(rf69_set_crc_enable (dev->spi, rx_cfg->enable_crc));
+
+   if (rx_cfg->enable_crc == OPTION_ON) {
+   ret = rf69_enable_crc(dev->spi);
+   if (ret < 0)
+   return ret;
+   } else {
+   ret = rf69_disable_crc(dev->spi);
+   if (ret < 0)
+   return ret;
+   }


Why don't you use SET_CHECKED(...)?



Marcus, please don't introduce new uses of SET_CHECKED().  It has a
hidden return in it which is against kernel style and introduces very
predictable and avoidable bugs.  For example, in probe().


Ah ok.

Thanks for clarifiytion!

What a pitty - another bunch of extra lines of code...

Or is there an other construction, allowing for one line per register 
change? Something like


ret = rf69_set_xyz(...); if (ret) return ret;
ret = rf69_set_abc(...); if (ret) return ret;

is pretty ugly and voids the style guide...

Thx,

Marcus

Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions

2017-12-06 Thread Dan Carpenter 
On Wed, Dec 06, 2017 at 11:05:22AM +0200, Marcus Wolf wrote:
> 
> 
> Am 06.12.2017 um 00:08 schrieb Simon Sandström:
> > Splits rf69_set_crc_enabled(dev, enabled) into
> > rf69_enable_crc(dev) and rf69_disable_crc(dev).
> > 
> > Signed-off-by: Simon Sandström 
> > ---
> >   drivers/staging/pi433/pi433_if.c | 22 --
> >   drivers/staging/pi433/rf69.c | 18 ++
> >   drivers/staging/pi433/rf69.h |  4 ++--
> >   3 files changed, 28 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/staging/pi433/pi433_if.c 
> > b/drivers/staging/pi433/pi433_if.c
> > index 2ae19ac565d1..614eec7dd904 100644
> > --- a/drivers/staging/pi433/pi433_if.c
> > +++ b/drivers/staging/pi433/pi433_if.c
> > @@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
> > pi433_rx_cfg *rx_cfg)
> > return ret;
> > }
> > SET_CHECKED(rf69_set_adressFiltering(dev->spi, 
> > rx_cfg->enable_address_filtering));
> > -   SET_CHECKED(rf69_set_crc_enable (dev->spi, rx_cfg->enable_crc));
> > +
> > +   if (rx_cfg->enable_crc == OPTION_ON) {
> > +   ret = rf69_enable_crc(dev->spi);
> > +   if (ret < 0)
> > +   return ret;
> > +   } else {
> > +   ret = rf69_disable_crc(dev->spi);
> > +   if (ret < 0)
> > +   return ret;
> > +   }
> 
> Why don't you use SET_CHECKED(...)?
> 

Marcus, please don't introduce new uses of SET_CHECKED().  It has a
hidden return in it which is against kernel style and introduces very
predictable and avoidable bugs.  For example, in probe().

> I stil don't like this kind of changes - and not using SET_CHECKED makes it
> even worse, since that further increases code length.
> 
> The idea was to have the configuration as compact, as you can see in the
> receiver config section. It's a pitty that the packet config already needs
> such a huge number of exceptions due to technical reasons. We shouldn't
> further extend the numbers of exceptions and shouldn't extend the number of
> lines for setting a reg.
> 
> Initially this function was just like
> set_rx_cfg()
> {
> SET_CHECKED(...)
> SET_CHECKED(...)
> SET_CHECKED(...)
> SET_CHECKED(...)
> }
> 
> It should be easy,
> * to survey, which chip settings are touched, if set_rx_cfg is called.
> * to survey, that all params of the rx_cfg struct are taken care of.
> 
> The longer the function gets, the harder it is, to service it.
> I really would be happy, if we don't go this way.
> 
> 
> Anyway, please keep the naming convention of rf69.c:
> 
> rf69 -set/get - action
> -> rf69_set_crc_enable

No...  Simon's name is better.  His is shorter and makes more sense.  :(

regards,
dan carpenter

Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions

2017-12-06 Thread Marcus Wolf 



Am 06.12.2017 um 00:08 schrieb Simon Sandström:

Splits rf69_set_crc_enabled(dev, enabled) into
rf69_enable_crc(dev) and rf69_disable_crc(dev).

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c | 22 --
  drivers/staging/pi433/rf69.c | 18 ++
  drivers/staging/pi433/rf69.h |  4 ++--
  3 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 2ae19ac565d1..614eec7dd904 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
return ret;
}
SET_CHECKED(rf69_set_adressFiltering(dev->spi, 
rx_cfg->enable_address_filtering));
-   SET_CHECKED(rf69_set_crc_enable (dev->spi, rx_cfg->enable_crc));
+
+   if (rx_cfg->enable_crc == OPTION_ON) {
+   ret = rf69_enable_crc(dev->spi);
+   if (ret < 0)
+   return ret;
+   } else {
+   ret = rf69_disable_crc(dev->spi);
+   if (ret < 0)
+   return ret;
+   }


Why don't you use SET_CHECKED(...)?

I stil don't like this kind of changes - and not using SET_CHECKED makes 
it even worse, since that further increases code length.


The idea was to have the configuration as compact, as you can see in the 
receiver config section. It's a pitty that the packet config already 
needs such a huge number of exceptions due to technical reasons. We 
shouldn't further extend the numbers of exceptions and shouldn't extend 
the number of lines for setting a reg.


Initially this function was just like
set_rx_cfg()
{
SET_CHECKED(...)
SET_CHECKED(...)
SET_CHECKED(...)
SET_CHECKED(...)
}

It should be easy,
* to survey, which chip settings are touched, if set_rx_cfg is called.
* to survey, that all params of the rx_cfg struct are taken care of.

The longer the function gets, the harder it is, to service it.
I really would be happy, if we don't go this way.


Anyway, please keep the naming convention of rf69.c:

rf69 -set/get - action
-> rf69_set_crc_enable

Thanks,

Marcus

[PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions

2017-12-05 Thread Simon Sandström 
Splits rf69_set_crc_enabled(dev, enabled) into
rf69_enable_crc(dev) and rf69_disable_crc(dev).

Signed-off-by: Simon Sandström 
---
 drivers/staging/pi433/pi433_if.c | 22 --
 drivers/staging/pi433/rf69.c | 18 ++
 drivers/staging/pi433/rf69.h |  4 ++--
 3 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 2ae19ac565d1..614eec7dd904 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
return ret;
}
SET_CHECKED(rf69_set_adressFiltering(dev->spi, 
rx_cfg->enable_address_filtering));
-   SET_CHECKED(rf69_set_crc_enable (dev->spi, rx_cfg->enable_crc));
+
+   if (rx_cfg->enable_crc == OPTION_ON) {
+   ret = rf69_enable_crc(dev->spi);
+   if (ret < 0)
+   return ret;
+   } else {
+   ret = rf69_disable_crc(dev->spi);
+   if (ret < 0)
+   return ret;
+   }
 
/* lengths */
SET_CHECKED(rf69_set_sync_size(dev->spi, rx_cfg->sync_length));
@@ -282,7 +291,16 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
if (ret < 0)
return ret;
}
-   SET_CHECKED(rf69_set_crc_enable   (dev->spi, tx_cfg->enable_crc));
+
+   if (tx_cfg->enable_crc == OPTION_ON) {
+   ret = rf69_enable_crc(dev->spi);
+   if (ret < 0)
+   return ret;
+   } else {
+   ret = rf69_disable_crc(dev->spi);
+   if (ret < 0)
+   return ret;
+   }
 
/* configure sync, if enabled */
if (tx_cfg->enable_sync == OPTION_ON) {
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index b19bca8a0f26..612d59f61f88 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -822,20 +822,14 @@ int rf69_set_packet_format(struct spi_device *spi, enum 
packetFormat packetForma
}
 }
 
-int rf69_set_crc_enable(struct spi_device *spi,
-   enum option_on_off option_on_off)
+int rf69_enable_crc(struct spi_device *spi)
 {
-   #ifdef DEBUG
-   dev_dbg(&spi->dev, "set: crc enable");
-   #endif
+   return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) |  
MASK_PACKETCONFIG1_CRC_ON));
+}
 
-   switch (option_on_off) {
-   case OPTION_ON:  return WRITE_REG(REG_PACKETCONFIG1, 
(READ_REG(REG_PACKETCONFIG1) |  MASK_PACKETCONFIG1_CRC_ON));
-   case OPTION_OFF: return WRITE_REG(REG_PACKETCONFIG1, 
(READ_REG(REG_PACKETCONFIG1) & ~MASK_PACKETCONFIG1_CRC_ON));
-   default:
-   dev_dbg(&spi->dev, "set: illegal input param");
-   return -EINVAL;
-   }
+int rf69_disable_crc(struct spi_device *spi)
+{
+   return WRITE_REG(REG_PACKETCONFIG1, (READ_REG(REG_PACKETCONFIG1) & 
~MASK_PACKETCONFIG1_CRC_ON));
 }
 
 int rf69_set_adressFiltering(struct spi_device *spi, enum addressFiltering 
addressFiltering)
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 1cb6db33d6fe..9428dee97de7 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -66,8 +66,8 @@ int rf69_set_sync_size(struct spi_device *spi, u8 sync_size);
 int rf69_set_sync_tolerance(struct spi_device *spi, u8 syncTolerance);
 int rf69_set_sync_values(struct spi_device *spi, u8 syncValues[8]);
 int rf69_set_packet_format(struct spi_device *spi, enum packetFormat 
packetFormat);
-int rf69_set_crc_enable(struct spi_device *spi,
-   enum option_on_off option_on_off);
+int rf69_enable_crc(struct spi_device *spi);
+int rf69_disable_crc(struct spi_device *spi);
 int rf69_set_adressFiltering(struct spi_device *spi, enum addressFiltering 
addressFiltering);
 int rf69_set_payload_length(struct spi_device *spi, u8 payloadLength);
 u8  rf69_get_payload_length(struct spi_device *spi);
-- 
2.11.0