Re: [PATCH v3] staging: pi433: replace simple switch statements

[marcus . wolf]

Hi Dan,

I'd like to mention once more, that the idea of the abstraction was to 
support multiple modules of Hope-RF.
If the decision of the "team" of developer of this driver is, that it 
should be reduced to a Pi433 or RFM69CW driver only, I fully agree, 
that the abstraction layer isn't necessary (tough improving readability).

But if the "team" wants to extend the driver - here I explicitly want to 
Name Marcin Ciupak and Hogo Lefeuvre, both were discussing this with me - 
I highly recommend to keep the abstraction layer.

And once again, I have to announce, that - if noone appears, who wants to 
help me with selling Pi433 - I can't effort to let Pi433 on the market 
longer then end of this year. From this Point of view on long term it 
might be senseless to prepare a Pi433-only driver.

Cheers,
Marcus

Dan Carpenter schrieb am 25.06.2018 14:35:
> I'd still prefer if we just removed this abstraction entirely and used
> OPMODE_MODE_TRANSMIT everywhere instead of bringing "transmit" into it.
> 
> I know that every author thinks their abstraction will definitely be
> useful in the future, but generally kernel style is to remove
> abstractions.
> 
> But I guess this code is an improvement over the original so the patch
> is fine.
> 
> regards,
> dan carpenter
> 
>

Re: [PATCH v3] staging: pi433: replace simple switch statements

[marcus . wolf]

Hi Dan,

I'd like to mention once more, that the idea of the abstraction was to 
support multiple modules of Hope-RF.
If the decision of the "team" of developer of this driver is, that it 
should be reduced to a Pi433 or RFM69CW driver only, I fully agree, 
that the abstraction layer isn't necessary (tough improving readability).

But if the "team" wants to extend the driver - here I explicitly want to 
Name Marcin Ciupak and Hogo Lefeuvre, both were discussing this with me - 
I highly recommend to keep the abstraction layer.

And once again, I have to announce, that - if noone appears, who wants to 
help me with selling Pi433 - I can't effort to let Pi433 on the market 
longer then end of this year. From this Point of view on long term it 
might be senseless to prepare a Pi433-only driver.

Cheers,
Marcus

Dan Carpenter schrieb am 25.06.2018 14:35:
> I'd still prefer if we just removed this abstraction entirely and used
> OPMODE_MODE_TRANSMIT everywhere instead of bringing "transmit" into it.
> 
> I know that every author thinks their abstraction will definitely be
> useful in the future, but generally kernel style is to remove
> abstractions.
> 
> But I guess this code is an improvement over the original so the patch
> is fine.
> 
> regards,
> dan carpenter
> 
>

Re: [PATCH v3] staging: pi433: replace simple switch statements

[marcus . wolf]

Reviewed-by: Marcus Wolf 

Thank you Valentin, very nice patch :-)

Valentin Vidic schrieb am 24.06.2018 18:31:

Use const array to map switch cases to resulting values.

Signed-off-by: Valentin Vidic 
---
v2: use correct type for const arrays
v3: add missing static keyword for af_map

 drivers/staging/pi433/rf69.c | 233 ++-
 1 file changed, 93 insertions(+), 140 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 90280e9b006d..341d6901a801 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -111,27 +111,22 @@ static inline int rf69_read_mod_write(struct spi_device
*spi, u8 reg,
 
 int rf69_set_mode(struct spi_device *spi, enum mode mode)
 {
-   switch (mode) {
-   case transmit:
-   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
-  OPMODE_MODE_TRANSMIT);
-   case receive:
-   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
-  OPMODE_MODE_RECEIVE);
-   case synthesizer:
-   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
-  OPMODE_MODE_SYNTHESIZER);
-   case standby:
-   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
-  OPMODE_MODE_STANDBY);
-   case mode_sleep:
-   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
-  OPMODE_MODE_SLEEP);
-   default:
+   static const u8 mode_map[] = {
+   [transmit] = OPMODE_MODE_TRANSMIT,
+   [receive] = OPMODE_MODE_RECEIVE,
+   [synthesizer] = OPMODE_MODE_SYNTHESIZER,
+   [standby] = OPMODE_MODE_STANDBY,
+   [mode_sleep] = OPMODE_MODE_SLEEP,
+   };
+
+   if (unlikely(mode >= ARRAY_SIZE(mode_map))) {
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
}
 
+   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
+  mode_map[mode]);
+
// we are using packet mode, so this check is not really needed
// but waiting for mode ready is necessary when going from sleep 
because the
FIFO may not be immediately available from previous mode
//while (_mode == RF69_MODE_SLEEP && (READ_REG(REG_IRQFLAGS1) &
RF_IRQFLAGS1_MODEREADY) == 0x00); // Wait for ModeReady
@@ -145,19 +140,19 @@ int rf69_set_data_mode(struct spi_device *spi, u8
data_mode)
 
 int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)
 {
-   switch (modulation) {
-   case OOK:
-   return rf69_read_mod_write(spi, REG_DATAMODUL,
-  MASK_DATAMODUL_MODULATION_TYPE,
-  DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK:
-   return rf69_read_mod_write(spi, REG_DATAMODUL,
-  MASK_DATAMODUL_MODULATION_TYPE,
-  DATAMODUL_MODULATION_TYPE_FSK);
-   default:
+   static const u8 modulation_map[] = {
+   [OOK] = DATAMODUL_MODULATION_TYPE_OOK,
+   [FSK] = DATAMODUL_MODULATION_TYPE_FSK,
+   };
+
+   if (unlikely(modulation >= ARRAY_SIZE(modulation_map))) {
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
}
+
+   return rf69_read_mod_write(spi, REG_DATAMODUL,
+  MASK_DATAMODUL_MODULATION_TYPE,
+  modulation_map[modulation]);
 }
 
 static enum modulation rf69_get_modulation(struct spi_device *spi)
@@ -373,43 +368,30 @@ int rf69_set_output_power_level(struct spi_device *spi, u8
power_level)
 
 int rf69_set_pa_ramp(struct spi_device *spi, enum pa_ramp pa_ramp)
 {
-   switch (pa_ramp) {
-   case ramp3400:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_3400);
-   case ramp2000:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_2000);
-   case ramp1000:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_1000);
-   case ramp500:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_500);
-   case ramp250:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_250);
-   case ramp125:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_125);
-   case ramp100:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_100);
-   case ramp62:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_62);
-   case ramp50:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_50);
-   case ramp40:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_40);
-   case r

Re: [PATCH v3] staging: pi433: replace simple switch statements

[marcus . wolf]

Reviewed-by: Marcus Wolf 

Thank you Valentin, very nice patch :-)

Valentin Vidic schrieb am 24.06.2018 18:31:

Use const array to map switch cases to resulting values.

Signed-off-by: Valentin Vidic 
---
v2: use correct type for const arrays
v3: add missing static keyword for af_map

 drivers/staging/pi433/rf69.c | 233 ++-
 1 file changed, 93 insertions(+), 140 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 90280e9b006d..341d6901a801 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -111,27 +111,22 @@ static inline int rf69_read_mod_write(struct spi_device
*spi, u8 reg,
 
 int rf69_set_mode(struct spi_device *spi, enum mode mode)
 {
-   switch (mode) {
-   case transmit:
-   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
-  OPMODE_MODE_TRANSMIT);
-   case receive:
-   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
-  OPMODE_MODE_RECEIVE);
-   case synthesizer:
-   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
-  OPMODE_MODE_SYNTHESIZER);
-   case standby:
-   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
-  OPMODE_MODE_STANDBY);
-   case mode_sleep:
-   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
-  OPMODE_MODE_SLEEP);
-   default:
+   static const u8 mode_map[] = {
+   [transmit] = OPMODE_MODE_TRANSMIT,
+   [receive] = OPMODE_MODE_RECEIVE,
+   [synthesizer] = OPMODE_MODE_SYNTHESIZER,
+   [standby] = OPMODE_MODE_STANDBY,
+   [mode_sleep] = OPMODE_MODE_SLEEP,
+   };
+
+   if (unlikely(mode >= ARRAY_SIZE(mode_map))) {
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
}
 
+   return rf69_read_mod_write(spi, REG_OPMODE, MASK_OPMODE_MODE,
+  mode_map[mode]);
+
// we are using packet mode, so this check is not really needed
// but waiting for mode ready is necessary when going from sleep 
because the
FIFO may not be immediately available from previous mode
//while (_mode == RF69_MODE_SLEEP && (READ_REG(REG_IRQFLAGS1) &
RF_IRQFLAGS1_MODEREADY) == 0x00); // Wait for ModeReady
@@ -145,19 +140,19 @@ int rf69_set_data_mode(struct spi_device *spi, u8
data_mode)
 
 int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)
 {
-   switch (modulation) {
-   case OOK:
-   return rf69_read_mod_write(spi, REG_DATAMODUL,
-  MASK_DATAMODUL_MODULATION_TYPE,
-  DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK:
-   return rf69_read_mod_write(spi, REG_DATAMODUL,
-  MASK_DATAMODUL_MODULATION_TYPE,
-  DATAMODUL_MODULATION_TYPE_FSK);
-   default:
+   static const u8 modulation_map[] = {
+   [OOK] = DATAMODUL_MODULATION_TYPE_OOK,
+   [FSK] = DATAMODUL_MODULATION_TYPE_FSK,
+   };
+
+   if (unlikely(modulation >= ARRAY_SIZE(modulation_map))) {
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
}
+
+   return rf69_read_mod_write(spi, REG_DATAMODUL,
+  MASK_DATAMODUL_MODULATION_TYPE,
+  modulation_map[modulation]);
 }
 
 static enum modulation rf69_get_modulation(struct spi_device *spi)
@@ -373,43 +368,30 @@ int rf69_set_output_power_level(struct spi_device *spi, u8
power_level)
 
 int rf69_set_pa_ramp(struct spi_device *spi, enum pa_ramp pa_ramp)
 {
-   switch (pa_ramp) {
-   case ramp3400:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_3400);
-   case ramp2000:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_2000);
-   case ramp1000:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_1000);
-   case ramp500:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_500);
-   case ramp250:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_250);
-   case ramp125:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_125);
-   case ramp100:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_100);
-   case ramp62:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_62);
-   case ramp50:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_50);
-   case ramp40:
-   return rf69_write_reg(spi, REG_PARAMP, PARAMP_40);
-   case r

Re: pi433: initialization of tx config in pi433_open()

[Marcus Wolf]

Hi Hugo,

thank you for all your work on Pi433 driver.

For a better understanding some info about Pi433 and the ideas behind it.
Pi433 was developed by me in order to have a simple to mount
CE-compliant 433MHz shield for the Raspberry Pi. I wanted to put it on
sale on the one side and develop a further product for home automation,
using the Pi433.

After three years of development and hard trying to find sales partner,
I almost gave up, since after three years still earn on those topics by
far do not cover the spendings. If nothing changes, I'll have to close
my company at the end of this year.

At a distinct point, the work on trying to sell exceeded the technical
work, so no time was left for the driver development. And now I started
over in freelancing to earn money. That's why all of you nearly hear
nothing from me - very sad about that!

Back to technics:
There was already the idea of equipping the driver with default values.
I see no benefit with setting the default values from the data sheet,
since they do not lead to a good, maybe even not working setup of the
rf69. Idea was to choose settings, that allow to use pipeoperators on
the command sehll for transmitting and receiving telegrams. There was a
longer discussion about one year ago with Marcin Ciupak about that topic.
Most important point from my side was, that the defaults should be in a
way, that CE recommandations are meat. You can find a lot of
configurations, making Pi433 work in a way, that it isn't CE-compliant
any more.
The 4711 sound like just beeing a place holder.
Since - like I told before - I inteded to use Pi433 mainly for OOK/ASK,
my idea was to use an good OOK/ASK setup as default. I could provide you
with a source code, I used the Pi433 with - but I think attachments are
unwanted here.

As far as I can remember, there were some more details to modify on the
driver, to use it with pipes on command line. But I had a rough
implementation. At least send was working... To long ago to remeber :-(


Since it might happen, that Pi433 will go off the market in a few
monthes (tears in my eyes), I think it's a good idea to modify the
driver to become a generic hope-RF driver.
I already did investigations on different hope-RF chips and asked
hope-RF for a little support (e.g. partnership), but they were of
opinion, that they don't need such a driver. It would be easy to cover
up to five/six chips of them - even their brand new LoRaWan-chip. RFM-12
will be a little bit harder, since it works a little bit different.
There were already preparations to support several chips - e. g. by
capsulating the HW layer. But even hard discussions one year ago didn't
help - according to kernel guide lines, it wasn't allowed to keep this
capsulations. So the strict capsulation was broken and some of the
basics for supporting more chips are lost now.
Also on this topic I had several discussions with Marcin Ciupak, how to
realise the support of the next chips.
Maybe you can search the mailing list? If not, I can try to find the
discussions in my inbox.


I would love to help you with these extending topics, but since I am
almost out of money, at the moment there is no margin for complimentary
developments any more :-/

But if you like, I can discuss some stuff with you from time to time.

Thank you so much for your interest in Pi433 and my driver!!

If you need hradware for testing, let me know.

Marcus



Am 22.06.2018 um 04:47 schrieb Hugo Lefeuvre:
> Hi Marcus,
> 
> I'm currently working on the following TODO:
> 
>  966 /* setup instance data*/
>  967 instance->device = device;
>  968 instance->tx_cfg.bit_rate = 4711;
>  969 // TODO: fill instance->tx_cfg;
> 
> If a user calls write() right after open()-ing an instance, the driver
> might try to setup the device with uninitialized garbage. In fact
> nothing really bad will happen because the rf69 interface abstraction
> will filter out wrong values, but this might be a confusing behavior
> for the user.
> 
> What do you think about initializing instance->tx_cfg with the default
> values of the rf69 datasheet[0] ?
> 
> Also, is there a specific reason why you chose 4711 as a default value
> for the bit rate ? I couldn't find it anywhere in the datasheet nor on
> the internet.
> 
> Thanks !
> 
> Regards,
>  Hugo
> 
> [0] http://www.hoperf.com/upload/rf/RFM69CW-V1.1.pdf
> 

Re: pi433: initialization of tx config in pi433_open()

[Marcus Wolf]

Hi Hugo,

thank you for all your work on Pi433 driver.

For a better understanding some info about Pi433 and the ideas behind it.
Pi433 was developed by me in order to have a simple to mount
CE-compliant 433MHz shield for the Raspberry Pi. I wanted to put it on
sale on the one side and develop a further product for home automation,
using the Pi433.

After three years of development and hard trying to find sales partner,
I almost gave up, since after three years still earn on those topics by
far do not cover the spendings. If nothing changes, I'll have to close
my company at the end of this year.

At a distinct point, the work on trying to sell exceeded the technical
work, so no time was left for the driver development. And now I started
over in freelancing to earn money. That's why all of you nearly hear
nothing from me - very sad about that!

Back to technics:
There was already the idea of equipping the driver with default values.
I see no benefit with setting the default values from the data sheet,
since they do not lead to a good, maybe even not working setup of the
rf69. Idea was to choose settings, that allow to use pipeoperators on
the command sehll for transmitting and receiving telegrams. There was a
longer discussion about one year ago with Marcin Ciupak about that topic.
Most important point from my side was, that the defaults should be in a
way, that CE recommandations are meat. You can find a lot of
configurations, making Pi433 work in a way, that it isn't CE-compliant
any more.
The 4711 sound like just beeing a place holder.
Since - like I told before - I inteded to use Pi433 mainly for OOK/ASK,
my idea was to use an good OOK/ASK setup as default. I could provide you
with a source code, I used the Pi433 with - but I think attachments are
unwanted here.

As far as I can remember, there were some more details to modify on the
driver, to use it with pipes on command line. But I had a rough
implementation. At least send was working... To long ago to remeber :-(


Since it might happen, that Pi433 will go off the market in a few
monthes (tears in my eyes), I think it's a good idea to modify the
driver to become a generic hope-RF driver.
I already did investigations on different hope-RF chips and asked
hope-RF for a little support (e.g. partnership), but they were of
opinion, that they don't need such a driver. It would be easy to cover
up to five/six chips of them - even their brand new LoRaWan-chip. RFM-12
will be a little bit harder, since it works a little bit different.
There were already preparations to support several chips - e. g. by
capsulating the HW layer. But even hard discussions one year ago didn't
help - according to kernel guide lines, it wasn't allowed to keep this
capsulations. So the strict capsulation was broken and some of the
basics for supporting more chips are lost now.
Also on this topic I had several discussions with Marcin Ciupak, how to
realise the support of the next chips.
Maybe you can search the mailing list? If not, I can try to find the
discussions in my inbox.


I would love to help you with these extending topics, but since I am
almost out of money, at the moment there is no margin for complimentary
developments any more :-/

But if you like, I can discuss some stuff with you from time to time.

Thank you so much for your interest in Pi433 and my driver!!

If you need hradware for testing, let me know.

Marcus



Am 22.06.2018 um 04:47 schrieb Hugo Lefeuvre:
> Hi Marcus,
> 
> I'm currently working on the following TODO:
> 
>  966 /* setup instance data*/
>  967 instance->device = device;
>  968 instance->tx_cfg.bit_rate = 4711;
>  969 // TODO: fill instance->tx_cfg;
> 
> If a user calls write() right after open()-ing an instance, the driver
> might try to setup the device with uninitialized garbage. In fact
> nothing really bad will happen because the rf69 interface abstraction
> will filter out wrong values, but this might be a confusing behavior
> for the user.
> 
> What do you think about initializing instance->tx_cfg with the default
> values of the rf69 datasheet[0] ?
> 
> Also, is there a specific reason why you chose 4711 as a default value
> for the bit rate ? I couldn't find it anywhere in the datasheet nor on
> the internet.
> 
> Thanks !
> 
> Regards,
>  Hugo
> 
> [0] http://www.hoperf.com/upload/rf/RFM69CW-V1.1.pdf
> 

Re: rf69_set_deviation in rf69.c (pi433 driver)

[Marcus Wolf]

Hi Hugo,

sorry for the late response and thank you for all that deep
investigation in the pi433 driver!

> According to the datasheet[0], the deviation should always be smaller
> than 300kHz, and the following equation should be respected:
> 
>   (1) FDA + BRF/2 =< 500 kHz
> 
> Why did you choose 500kHz as max for FDA, instead of 300kHz ? It looks like
> a bug to me.

My focus was always on OOK and ASK. PSK was only used for a few
measurements in the laboratory, I engaged to check CE compliance.
Most probably 500kHz was a value, that's common for PSK and I didn't pay
any attention to the datasheet. So I think, you are right: This is a bug
and could be revised.
Never the less: While using it in the lab, the transmission was fine and
the signals over air have been clean and fitted to the recommondations
of the CE.

> 
> Concerning the TODO, I can see two solutions currently:
> 
> 1. Introduce a new rf69_get_bit_rate function which reads REG_BITRATE_MSB
>and REG_BITRATE_LSB and returns reconstructed BRF.
>We could use this function in rf69_set_deviation to retrieve the BRF.
> 
> + clean
> + intuitive
> - heavy / slow

Why not: I like clean and intuitive implementations. Since it's used
during configuration, we shouldn't be that squeezed in time, that we
need to hurry.
> 
> 2. Store BRF somewhere in rf69.c, initialize it with the default value
>(4.8 kb/s) and update it when rf69_set_bit_rate is called.
> 
> + easy
> - dirty, doesn't fit well with the design of rf69.c (do not store
>   anything, simply expose API)

Up to my experience, storing reg values is always a bit problematic,
since the value may be outdated. And if you update the value every time
you want to use it to be sure, it's correct, there is no win in having
the copy of the reg value.
So this wouldn't be my favourite.
> 
> I'd really prefer going for the first one, but I wanted to have your opinion
> on this.

Agree.

> Thanks for your work !

Your welcome :-)

Marcus

Re: rf69_set_deviation in rf69.c (pi433 driver)

[Marcus Wolf]

Hi Hugo,

sorry for the late response and thank you for all that deep
investigation in the pi433 driver!

> According to the datasheet[0], the deviation should always be smaller
> than 300kHz, and the following equation should be respected:
> 
>   (1) FDA + BRF/2 =< 500 kHz
> 
> Why did you choose 500kHz as max for FDA, instead of 300kHz ? It looks like
> a bug to me.

My focus was always on OOK and ASK. PSK was only used for a few
measurements in the laboratory, I engaged to check CE compliance.
Most probably 500kHz was a value, that's common for PSK and I didn't pay
any attention to the datasheet. So I think, you are right: This is a bug
and could be revised.
Never the less: While using it in the lab, the transmission was fine and
the signals over air have been clean and fitted to the recommondations
of the CE.

> 
> Concerning the TODO, I can see two solutions currently:
> 
> 1. Introduce a new rf69_get_bit_rate function which reads REG_BITRATE_MSB
>and REG_BITRATE_LSB and returns reconstructed BRF.
>We could use this function in rf69_set_deviation to retrieve the BRF.
> 
> + clean
> + intuitive
> - heavy / slow

Why not: I like clean and intuitive implementations. Since it's used
during configuration, we shouldn't be that squeezed in time, that we
need to hurry.
> 
> 2. Store BRF somewhere in rf69.c, initialize it with the default value
>(4.8 kb/s) and update it when rf69_set_bit_rate is called.
> 
> + easy
> - dirty, doesn't fit well with the design of rf69.c (do not store
>   anything, simply expose API)

Up to my experience, storing reg values is always a bit problematic,
since the value may be outdated. And if you update the value every time
you want to use it to be sure, it's correct, there is no win in having
the copy of the reg value.
So this wouldn't be my favourite.
> 
> I'd really prefer going for the first one, but I wanted to have your opinion
> on this.

Agree.

> Thanks for your work !

Your welcome :-)

Marcus

Re: [PATCH v2] staging: pi433: cleanup tx_fifo locking

[Marcus Wolf]

Reviewed-by: Marcus Wolf <marcus.w...@wolf-entwicklungen.de>

Am 19.04.2018 um 12:25 schrieb Valentin Vidic:
> pi433_write requires locking due to multiple writers.  After acquiring
> the lock check if enough free space is available in the kfifo to write
> the whole message. This check should prevent partial writes to tx_fifo
> so kfifo_reset is not needed anymore.
> 
> pi433_tx_thread is the only reader so it does not require locking
> after kfifo_reset is removed.
> 
> Signed-off-by: Valentin Vidic <valentin.vi...@carnet.hr>
> ---
> v2: print a warning if partial fifo write happens
> 
>  drivers/staging/pi433/pi433_if.c | 23 ++-
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c 
> b/drivers/staging/pi433/pi433_if.c
> index d1e0ddbc79ce..2a05eff88469 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -87,7 +87,7 @@ struct pi433_device {
>  
>   /* tx related values */
>   STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
> - struct mutextx_fifo_lock; // TODO: check, whether necessary 
> or obsolete
> + struct mutextx_fifo_lock; /* serialize userspace writers */
>   struct task_struct  *tx_task_struct;
>   wait_queue_head_t   tx_wait_queue;
>   u8  free_in_fifo;
> @@ -589,19 +589,15 @@ pi433_tx_thread(void *data)
>* - size of message
>* - message
>*/
> - mutex_lock(>tx_fifo_lock);
> -
>   retval = kfifo_out(>tx_fifo, _cfg, sizeof(tx_cfg));
>   if (retval != sizeof(tx_cfg)) {
>   dev_dbg(device->dev, "reading tx_cfg from fifo failed: 
> got %d byte(s), expected %d", retval, (unsigned int)sizeof(tx_cfg));
> - mutex_unlock(>tx_fifo_lock);
>   continue;
>   }
>  
>   retval = kfifo_out(>tx_fifo, , sizeof(size_t));
>   if (retval != sizeof(size_t)) {
>   dev_dbg(device->dev, "reading msg size from fifo 
> failed: got %d, expected %d", retval, (unsigned int)sizeof(size_t));
> - mutex_unlock(>tx_fifo_lock);
>   continue;
>   }
>  
> @@ -634,7 +630,6 @@ pi433_tx_thread(void *data)
>  sizeof(device->buffer) - position);
>   dev_dbg(device->dev,
>   "read %d message byte(s) from fifo queue.", retval);
> - mutex_unlock(>tx_fifo_lock);
>  
>   /* if rx is active, we need to interrupt the waiting for
>* incoming telegrams, to be able to send something.
> @@ -818,7 +813,7 @@ pi433_write(struct file *filp, const char __user *buf,
>   struct pi433_instance   *instance;
>   struct pi433_device *device;
>   int retval;
> - unsigned intcopied;
> + unsigned intrequired, available, copied;
>  
>   instance = filp->private_data;
>   device = instance->device;
> @@ -833,6 +828,16 @@ pi433_write(struct file *filp, const char __user *buf,
>* - message
>*/
>   mutex_lock(>tx_fifo_lock);
> +
> + required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
> + available = kfifo_avail(>tx_fifo);
> + if (required > available) {
> + dev_dbg(device->dev, "write to fifo failed: %d bytes required 
> but %d available",
> + required, available);
> + mutex_unlock(>tx_fifo_lock);
> + return -EAGAIN;
> + }
> +
>   retval = kfifo_in(>tx_fifo, >tx_cfg,
> sizeof(instance->tx_cfg));
>   if (retval != sizeof(instance->tx_cfg))
> @@ -855,8 +860,8 @@ pi433_write(struct file *filp, const char __user *buf,
>   return copied;
>  
>  abort:
> - dev_dbg(device->dev, "write to fifo failed: 0x%x", retval);
> - kfifo_reset(>tx_fifo); // TODO: maybe find a solution, not to 
> discard already stored, valid entries
> + dev_warn(device->dev,
> +  "write to fifo failed, non recoverable: 0x%x", retval);
>   mutex_unlock(>tx_fifo_lock);
>   return -EAGAIN;
>  }
> 

Re: [PATCH v2] staging: pi433: cleanup tx_fifo locking

[Marcus Wolf]

Reviewed-by: Marcus Wolf 

Am 19.04.2018 um 12:25 schrieb Valentin Vidic:
> pi433_write requires locking due to multiple writers.  After acquiring
> the lock check if enough free space is available in the kfifo to write
> the whole message. This check should prevent partial writes to tx_fifo
> so kfifo_reset is not needed anymore.
> 
> pi433_tx_thread is the only reader so it does not require locking
> after kfifo_reset is removed.
> 
> Signed-off-by: Valentin Vidic 
> ---
> v2: print a warning if partial fifo write happens
> 
>  drivers/staging/pi433/pi433_if.c | 23 ++-
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c 
> b/drivers/staging/pi433/pi433_if.c
> index d1e0ddbc79ce..2a05eff88469 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -87,7 +87,7 @@ struct pi433_device {
>  
>   /* tx related values */
>   STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
> - struct mutextx_fifo_lock; // TODO: check, whether necessary 
> or obsolete
> + struct mutextx_fifo_lock; /* serialize userspace writers */
>   struct task_struct  *tx_task_struct;
>   wait_queue_head_t   tx_wait_queue;
>   u8  free_in_fifo;
> @@ -589,19 +589,15 @@ pi433_tx_thread(void *data)
>* - size of message
>* - message
>*/
> - mutex_lock(>tx_fifo_lock);
> -
>   retval = kfifo_out(>tx_fifo, _cfg, sizeof(tx_cfg));
>   if (retval != sizeof(tx_cfg)) {
>   dev_dbg(device->dev, "reading tx_cfg from fifo failed: 
> got %d byte(s), expected %d", retval, (unsigned int)sizeof(tx_cfg));
> - mutex_unlock(>tx_fifo_lock);
>   continue;
>   }
>  
>   retval = kfifo_out(>tx_fifo, , sizeof(size_t));
>   if (retval != sizeof(size_t)) {
>   dev_dbg(device->dev, "reading msg size from fifo 
> failed: got %d, expected %d", retval, (unsigned int)sizeof(size_t));
> - mutex_unlock(>tx_fifo_lock);
>   continue;
>   }
>  
> @@ -634,7 +630,6 @@ pi433_tx_thread(void *data)
>  sizeof(device->buffer) - position);
>   dev_dbg(device->dev,
>   "read %d message byte(s) from fifo queue.", retval);
> - mutex_unlock(>tx_fifo_lock);
>  
>   /* if rx is active, we need to interrupt the waiting for
>* incoming telegrams, to be able to send something.
> @@ -818,7 +813,7 @@ pi433_write(struct file *filp, const char __user *buf,
>   struct pi433_instance   *instance;
>   struct pi433_device *device;
>   int retval;
> - unsigned intcopied;
> + unsigned intrequired, available, copied;
>  
>   instance = filp->private_data;
>   device = instance->device;
> @@ -833,6 +828,16 @@ pi433_write(struct file *filp, const char __user *buf,
>* - message
>*/
>   mutex_lock(>tx_fifo_lock);
> +
> + required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
> + available = kfifo_avail(>tx_fifo);
> + if (required > available) {
> + dev_dbg(device->dev, "write to fifo failed: %d bytes required 
> but %d available",
> + required, available);
> + mutex_unlock(>tx_fifo_lock);
> + return -EAGAIN;
> + }
> +
>   retval = kfifo_in(>tx_fifo, >tx_cfg,
> sizeof(instance->tx_cfg));
>   if (retval != sizeof(instance->tx_cfg))
> @@ -855,8 +860,8 @@ pi433_write(struct file *filp, const char __user *buf,
>   return copied;
>  
>  abort:
> - dev_dbg(device->dev, "write to fifo failed: 0x%x", retval);
> - kfifo_reset(>tx_fifo); // TODO: maybe find a solution, not to 
> discard already stored, valid entries
> + dev_warn(device->dev,
> +  "write to fifo failed, non recoverable: 0x%x", retval);
>   mutex_unlock(>tx_fifo_lock);
>   return -EAGAIN;
>  }
> 

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi!

So if you like, give me your address, and I'll send you two development
samples of Pi433.

Cheers,

Marcus

Am 19.04.2018 um 11:47 schrieb Valentin Vidic:
> On Thu, Apr 19, 2018 at 11:25:19AM +0200, Marcus Wolf wrote:
>> let me know, what you like to have. For sure with just one station and
>> no other 433MHz equipment, options for testing are quite limited.
> 
> I can get Rpi3 and with two shields test 433MHz communication between
> Rpi2 and Rpi3.
> 

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi!

So if you like, give me your address, and I'll send you two development
samples of Pi433.

Cheers,

Marcus

Am 19.04.2018 um 11:47 schrieb Valentin Vidic:
> On Thu, Apr 19, 2018 at 11:25:19AM +0200, Marcus Wolf wrote:
>> let me know, what you like to have. For sure with just one station and
>> no other 433MHz equipment, options for testing are quite limited.
> 
> I can get Rpi3 and with two shields test 433MHz communication between
> Rpi2 and Rpi3.
> 

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Am 12.04.2018 um 20:46 schrieb Valentin Vidic:
> On Sun, Apr 08, 2018 at 05:22:46PM +0200, Marcus Wolf wrote:
>> Regarding your patch, I did not understand, why you did not remove
>> the mutex_lock in pi433_write. Wasn't it the goal to remove it?
> 
> Is it possible for more than one userspace program to open the pi433
> device and send messages?  In that case more than one pi433_write
> could be running and it needs to hold a mutex_lock before calling
> kfifo_in.

You are right. The write function is open for multiple userspace programs.
So mutex_lock schould remain.

>> Below find a proposal of pi433_write function, I wrote on base
>> of my outdated (!), private repo. It is not compiled and not tested.
>> Since there is no more handling in case of an error (as well in the
>> propsal as in your patch), I removed the error handling completely.
>> I only do a test to detect proplems while writing to the tx_fifo,
>> but (like in your patch) do nothing for solving, just printing a line.
>> If this unexpected situation will occur (most probably never),
>> the tx_fifo will be (and stay) out of sync until driver gets unloaded.
>> We have to decide, whether we can stay with that. Like written above,
>> I thinkt the benefits are great, the chance of such kind of error
>> very low.
>> What do you think?
> 
> Yes, if there is only one writer and it checks the available size,
> kfifo_in should not fail.  The only problem might be copy_from_user
> but perhaps that is also quite unlikely.  A workaround for that could
> be to copy the data into a temporary kernel buffer first and than
> start kfifo writes using only kernel memory.

For my feeling, that's a safe solution but most probably oversized.

>> It could be discussed, whether it is better to return EMSGSIZE or
>> EAGAIN on the first check. On the one hand, there is a problem with
>> the message size, on the other hand (if message isn't generally too
>> big) after a while, there should be some more space available in
>> fifo, so EAGAIN may be better choice.
> 
> EAGAIN does seem better unless the message is too big to ever fit
> in the kfifo.
> 
>>  if (retval != required ) {
>>  dev_dbg(device->dev, "write to fifo failed, reason unknown, non 
>> recoverable.");
>>  return -EAGAIN;
>>  }
> 
> Maybe this should be dev_warn or even dev_crit if the driver is not
> usable anymore when this happens?  The error message should than also
> be adjusted to EBADF or something similar.

>From my point of veiew, the warning is (in combination of the
size-check) the by far better solution then the fifo reset.

So all in all, I think, we should go with your proposal.

Unfortunaly still no time to setup my test environment with a current
version of the driver in order to give it a try :-(
Sorry,

Marcus

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Am 12.04.2018 um 20:46 schrieb Valentin Vidic:
> On Sun, Apr 08, 2018 at 05:22:46PM +0200, Marcus Wolf wrote:
>> Regarding your patch, I did not understand, why you did not remove
>> the mutex_lock in pi433_write. Wasn't it the goal to remove it?
> 
> Is it possible for more than one userspace program to open the pi433
> device and send messages?  In that case more than one pi433_write
> could be running and it needs to hold a mutex_lock before calling
> kfifo_in.

You are right. The write function is open for multiple userspace programs.
So mutex_lock schould remain.

>> Below find a proposal of pi433_write function, I wrote on base
>> of my outdated (!), private repo. It is not compiled and not tested.
>> Since there is no more handling in case of an error (as well in the
>> propsal as in your patch), I removed the error handling completely.
>> I only do a test to detect proplems while writing to the tx_fifo,
>> but (like in your patch) do nothing for solving, just printing a line.
>> If this unexpected situation will occur (most probably never),
>> the tx_fifo will be (and stay) out of sync until driver gets unloaded.
>> We have to decide, whether we can stay with that. Like written above,
>> I thinkt the benefits are great, the chance of such kind of error
>> very low.
>> What do you think?
> 
> Yes, if there is only one writer and it checks the available size,
> kfifo_in should not fail.  The only problem might be copy_from_user
> but perhaps that is also quite unlikely.  A workaround for that could
> be to copy the data into a temporary kernel buffer first and than
> start kfifo writes using only kernel memory.

For my feeling, that's a safe solution but most probably oversized.

>> It could be discussed, whether it is better to return EMSGSIZE or
>> EAGAIN on the first check. On the one hand, there is a problem with
>> the message size, on the other hand (if message isn't generally too
>> big) after a while, there should be some more space available in
>> fifo, so EAGAIN may be better choice.
> 
> EAGAIN does seem better unless the message is too big to ever fit
> in the kfifo.
> 
>>  if (retval != required ) {
>>  dev_dbg(device->dev, "write to fifo failed, reason unknown, non 
>> recoverable.");
>>  return -EAGAIN;
>>  }
> 
> Maybe this should be dev_warn or even dev_crit if the driver is not
> usable anymore when this happens?  The error message should than also
> be adjusted to EBADF or something similar.

>From my point of veiew, the warning is (in combination of the
size-check) the by far better solution then the fifo reset.

So all in all, I think, we should go with your proposal.

Unfortunaly still no time to setup my test environment with a current
version of the driver in order to give it a try :-(
Sorry,

Marcus

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Am 12.04.2018 um 19:15 schrieb Valentin Vidic:
> On Sun, Apr 08, 2018 at 04:15:30PM +0200, Marcus Wolf wrote:
>> The hardware of Pi433 is working with every Raspberry Pi (on zero, you
>> need to solder the GPIO-pins) and with several other fruits like banana
>> pi. The only thing is, that you need different versions of the driver,
>> according to the processor, mounted on your fruit.
>>
>> If you'd like to test more then ther is no hang up or crash, you will
>> need a second party. You could use a 433MHz socket for testing TX or a
>> 433 thermometer for testing RX. An example code for communication with a
>> socket is available on the Pi433 productpage (www.pi433.de). The sample
>> for the thermometer isn't published yet (as always lack of time).
>>
>> If you want to test more deeply (using different features of the Rf69
>> chip or even do some long time testing, you have more options, if you
>> use to Pis with Pi433.
>>
>> Just let me know, what you'd like to do and what equipment, you need.
> 
> At the moment I have Rpi2 but not any other 433MHz equipment, so I
> could only do some basic testing unfortunately.  In case I get the
> new Rpi3 some time soon I would be able to do something better.
> 
Hi Valentin,



let me know, what you like to have. For sure with just one station and
no other 433MHz equipment, options for testing are quite limited.



Marcus

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Am 12.04.2018 um 19:15 schrieb Valentin Vidic:
> On Sun, Apr 08, 2018 at 04:15:30PM +0200, Marcus Wolf wrote:
>> The hardware of Pi433 is working with every Raspberry Pi (on zero, you
>> need to solder the GPIO-pins) and with several other fruits like banana
>> pi. The only thing is, that you need different versions of the driver,
>> according to the processor, mounted on your fruit.
>>
>> If you'd like to test more then ther is no hang up or crash, you will
>> need a second party. You could use a 433MHz socket for testing TX or a
>> 433 thermometer for testing RX. An example code for communication with a
>> socket is available on the Pi433 productpage (www.pi433.de). The sample
>> for the thermometer isn't published yet (as always lack of time).
>>
>> If you want to test more deeply (using different features of the Rf69
>> chip or even do some long time testing, you have more options, if you
>> use to Pis with Pi433.
>>
>> Just let me know, what you'd like to do and what equipment, you need.
> 
> At the moment I have Rpi2 but not any other 433MHz equipment, so I
> could only do some basic testing unfortunately.  In case I get the
> new Rpi3 some time soon I would be able to do something better.
> 
Hi Valentin,



let me know, what you like to have. For sure with just one station and
no other 433MHz equipment, options for testing are quite limited.



Marcus

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

like promissed, I finally had a deeper look to your proposal with
kfifo_avail and your patch from 24th of March.
In principle, I think it is a very nice idea, and we should try
to implement it.
But there is a snag: There is no guarantee, that kfifo_in will
only fail, if the fifo is full. If there will be any another
reason for kfifo_in to fail, with the new implementation there
will be no handling for that.
But I think the chance of such an situation is low to impossible
and the win in simplicity of the code is really great.

Regarding your patch, I did not understand, why you did not remove
the mutex_lock in pi433_write. Wasn't it the goal to remove it?

Below find a proposal of pi433_write function, I wrote on base
of my outdated (!), private repo. It is not compiled and not tested.
Since there is no more handling in case of an error (as well in the
propsal as in your patch), I removed the error handling completely.
I only do a test to detect proplems while writing to the tx_fifo,
but (like in your patch) do nothing for solving, just printing a line.
If this unexpected situation will occur (most probably never),
the tx_fifo will be (and stay) out of sync until driver gets unloaded.
We have to decide, whether we can stay with that. Like written above,
I thinkt the benefits are great, the chance of such kind of error
very low.
What do you think?

It could be discussed, whether it is better to return EMSGSIZE or
EAGAIN on the first check. On the one hand, there is a problem with
the message size, on the other hand (if message isn't generally too
big) after a while, there should be some more space available in
fifo, so EAGAIN may be better choice.


static ssize_t
pi433_write(struct file *filp, const char __user *buf,
size_t count, loff_t *f_pos)
{
struct pi433_instance   *instance;
struct pi433_device *device;
intrequired, copied, retval;

instance = filp->private_data;
device = instance->device;

/* check whether there is enough space available in tx_fifo */
required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
if ( num_of_bytes_to_store_in_fifo > kfifo_avail(>tx_fifo) ) {
dev_dbg(device->dev, "Not enough space in fifo. Need %d, but 
only have"
   , required
   , 
kfifo_avail(>tx_fifo) );
return -EMSGSIZE;
}

/* write the following sequence into fifo:
 * - tx_cfg
 * - size of message
 * - message
 */
retval  = kfifo_in(>tx_fifo, >tx_cfg, 
sizeof(instance->tx_cfg));
retval += kfifo_in (>tx_fifo, , sizeof(size_t));
retval += kfifo_from_user(>tx_fifo, buf, count, );

if (retval != required ) {
dev_dbg(device->dev, "write to fifo failed, reason unknown, non 
recoverable.");
return -EAGAIN;
}

/* start transfer */
wake_up_interruptible(>tx_wait_queue);
dev_dbg(device->dev, "write: generated new msg with %d bytes.", copied);

return 0;
}

Hope this helps :-)

Marcus


Am 25.03.2018 um 15:09 schrieb Valentin Vidic:
> On Sun, Mar 25, 2018 at 03:00:09PM +0200, Marcus Wolf wrote:
>> Unfortunaly I can't find the time to have a closer look on the code this
>> weekend - still busy with tax stuff :-(
>>
>> Idea sounds great. I'll try to look at the code and think about it
>> during Easter hollidays.
> 
> No problem, there is no hurry. But please do test the patch I sent yesterday:
> 
>   [PATCH] staging: pi433: cleanup tx_fifo locking
> 
> As I don't have the hardware this is just compile tested now :)
> 

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

like promissed, I finally had a deeper look to your proposal with
kfifo_avail and your patch from 24th of March.
In principle, I think it is a very nice idea, and we should try
to implement it.
But there is a snag: There is no guarantee, that kfifo_in will
only fail, if the fifo is full. If there will be any another
reason for kfifo_in to fail, with the new implementation there
will be no handling for that.
But I think the chance of such an situation is low to impossible
and the win in simplicity of the code is really great.

Regarding your patch, I did not understand, why you did not remove
the mutex_lock in pi433_write. Wasn't it the goal to remove it?

Below find a proposal of pi433_write function, I wrote on base
of my outdated (!), private repo. It is not compiled and not tested.
Since there is no more handling in case of an error (as well in the
propsal as in your patch), I removed the error handling completely.
I only do a test to detect proplems while writing to the tx_fifo,
but (like in your patch) do nothing for solving, just printing a line.
If this unexpected situation will occur (most probably never),
the tx_fifo will be (and stay) out of sync until driver gets unloaded.
We have to decide, whether we can stay with that. Like written above,
I thinkt the benefits are great, the chance of such kind of error
very low.
What do you think?

It could be discussed, whether it is better to return EMSGSIZE or
EAGAIN on the first check. On the one hand, there is a problem with
the message size, on the other hand (if message isn't generally too
big) after a while, there should be some more space available in
fifo, so EAGAIN may be better choice.


static ssize_t
pi433_write(struct file *filp, const char __user *buf,
size_t count, loff_t *f_pos)
{
struct pi433_instance   *instance;
struct pi433_device *device;
intrequired, copied, retval;

instance = filp->private_data;
device = instance->device;

/* check whether there is enough space available in tx_fifo */
required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
if ( num_of_bytes_to_store_in_fifo > kfifo_avail(>tx_fifo) ) {
dev_dbg(device->dev, "Not enough space in fifo. Need %d, but 
only have"
   , required
   , 
kfifo_avail(>tx_fifo) );
return -EMSGSIZE;
}

/* write the following sequence into fifo:
 * - tx_cfg
 * - size of message
 * - message
 */
retval  = kfifo_in(>tx_fifo, >tx_cfg, 
sizeof(instance->tx_cfg));
retval += kfifo_in (>tx_fifo, , sizeof(size_t));
retval += kfifo_from_user(>tx_fifo, buf, count, );

if (retval != required ) {
dev_dbg(device->dev, "write to fifo failed, reason unknown, non 
recoverable.");
return -EAGAIN;
}

/* start transfer */
wake_up_interruptible(>tx_wait_queue);
dev_dbg(device->dev, "write: generated new msg with %d bytes.", copied);

return 0;
}

Hope this helps :-)

Marcus


Am 25.03.2018 um 15:09 schrieb Valentin Vidic:
> On Sun, Mar 25, 2018 at 03:00:09PM +0200, Marcus Wolf wrote:
>> Unfortunaly I can't find the time to have a closer look on the code this
>> weekend - still busy with tax stuff :-(
>>
>> Idea sounds great. I'll try to look at the code and think about it
>> during Easter hollidays.
> 
> No problem, there is no hurry. But please do test the patch I sent yesterday:
> 
>   [PATCH] staging: pi433: cleanup tx_fifo locking
> 
> As I don't have the hardware this is just compile tested now :)
> 

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

The hardware of Pi433 is working with every Raspberry Pi (on zero, you
need to solder the GPIO-pins) and with several other fruits like banana
pi. The only thing is, that you need different versions of the driver,
according to the processor, mounted on your fruit.

If you'd like to test more then ther is no hang up or crash, you will
need a second party. You could use a 433MHz socket for testing TX or a
433 thermometer for testing RX. An example code for communication with a
socket is available on the Pi433 productpage (www.pi433.de). The sample
for the thermometer isn't published yet (as always lack of time).

If you want to test more deeply (using different features of the Rf69
chip or even do some long time testing, you have more options, if you
use to Pis with Pi433.

Just let me know, what you'd like to do and what equipment, you need.

Cheers,

Marcus

Am 25.03.2018 um 16:24 schrieb Valentin Vidic:
> On Sun, Mar 25, 2018 at 03:12:52PM +0200, Marcus Wolf wrote:
>> I am not at home the next two weeks. So I can do a codereading on
>> Easter, but testing will not take place earlier then mid/end of April :-(
>>
>> If you are interested, I can provide you an engineering sample of Pi433.
> 
> Sure, let me know which version of Rpi it supports and if I need two
> to test communication.
> 

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

The hardware of Pi433 is working with every Raspberry Pi (on zero, you
need to solder the GPIO-pins) and with several other fruits like banana
pi. The only thing is, that you need different versions of the driver,
according to the processor, mounted on your fruit.

If you'd like to test more then ther is no hang up or crash, you will
need a second party. You could use a 433MHz socket for testing TX or a
433 thermometer for testing RX. An example code for communication with a
socket is available on the Pi433 productpage (www.pi433.de). The sample
for the thermometer isn't published yet (as always lack of time).

If you want to test more deeply (using different features of the Rf69
chip or even do some long time testing, you have more options, if you
use to Pis with Pi433.

Just let me know, what you'd like to do and what equipment, you need.

Cheers,

Marcus

Am 25.03.2018 um 16:24 schrieb Valentin Vidic:
> On Sun, Mar 25, 2018 at 03:12:52PM +0200, Marcus Wolf wrote:
>> I am not at home the next two weeks. So I can do a codereading on
>> Easter, but testing will not take place earlier then mid/end of April :-(
>>
>> If you are interested, I can provide you an engineering sample of Pi433.
> 
> Sure, let me know which version of Rpi it supports and if I need two
> to test communication.
> 

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

I am not at home the next two weeks. So I can do a codereading on
Easter, but testing will not take place earlier then mid/end of April :-(

If you are interested, I can provide you an engineering sample of Pi433.

Cheers,

Marcus

Am 25.03.2018 um 15:09 schrieb Valentin Vidic:
> On Sun, Mar 25, 2018 at 03:00:09PM +0200, Marcus Wolf wrote:
>> Unfortunaly I can't find the time to have a closer look on the code this
>> weekend - still busy with tax stuff :-(
>>
>> Idea sounds great. I'll try to look at the code and think about it
>> during Easter hollidays.
> 
> No problem, there is no hurry. But please do test the patch I sent yesterday:
> 
>   [PATCH] staging: pi433: cleanup tx_fifo locking
> 
> As I don't have the hardware this is just compile tested now :)
> 

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

I am not at home the next two weeks. So I can do a codereading on
Easter, but testing will not take place earlier then mid/end of April :-(

If you are interested, I can provide you an engineering sample of Pi433.

Cheers,

Marcus

Am 25.03.2018 um 15:09 schrieb Valentin Vidic:
> On Sun, Mar 25, 2018 at 03:00:09PM +0200, Marcus Wolf wrote:
>> Unfortunaly I can't find the time to have a closer look on the code this
>> weekend - still busy with tax stuff :-(
>>
>> Idea sounds great. I'll try to look at the code and think about it
>> during Easter hollidays.
> 
> No problem, there is no hurry. But please do test the patch I sent yesterday:
> 
>   [PATCH] staging: pi433: cleanup tx_fifo locking
> 
> As I don't have the hardware this is just compile tested now :)
> 

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

> But I have an idea to remove this kfifo_reset call in pi433_write
> handling partial message writes:
> 
>   kfifo_reset(>tx_fifo); // TODO: maybe find a solution, not to 
> discard already stored, valid entries
> 
> The writer could acquire the lock and than use kfifo_avail to check if
> there is enough space to write the whole message.  What do you think?

Unfortunaly I can't find the time to have a closer look on the code this
weekend - still busy with tax stuff :-(

Idea sounds great. I'll try to look at the code and think about it
during Easter hollidays.

Cheers,

Marcus

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

> But I have an idea to remove this kfifo_reset call in pi433_write
> handling partial message writes:
> 
>   kfifo_reset(>tx_fifo); // TODO: maybe find a solution, not to 
> discard already stored, valid entries
> 
> The writer could acquire the lock and than use kfifo_avail to check if
> there is enough space to write the whole message.  What do you think?

Unfortunaly I can't find the time to have a closer look on the code this
weekend - still busy with tax stuff :-(

Idea sounds great. I'll try to look at the code and think about it
during Easter hollidays.

Cheers,

Marcus

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

I had no time to work on the code for monthes now and the memorisation
of my thoughts when I was programming that (approx. one year ago) is
quite pale.

As far as I remember, I read something, that the fifo has an integrated
protection, so no external protection is needed. But absolutely unsure.

If I will find some time within the next days, I'll have a look at the
code and try to recall.

But the most important thing already took place: We started thinking
about it :-)

Thanks,

Marcus

Am 23.03.2018 um 12:42 schrieb Valentin Vidic:
> On Fri, Mar 23, 2018 at 11:22:39AM +0100, Marcus Wolf wrote:
>> could you please decribe in short words, why you think, that hte lock
>> isn't obsolete?
>>
>> I wasn't sure, but close to remove the lock. That's why I putted the
>> comment.
> 
> Sure, if pi433_tx_thread runs on one CPU it might be possible
> to call pi433_write concurrently on another CPU and they would
> both modify tx_fifo.  But maybe there is some other protection
> in place that would prevent this?
> 

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

I had no time to work on the code for monthes now and the memorisation
of my thoughts when I was programming that (approx. one year ago) is
quite pale.

As far as I remember, I read something, that the fifo has an integrated
protection, so no external protection is needed. But absolutely unsure.

If I will find some time within the next days, I'll have a look at the
code and try to recall.

But the most important thing already took place: We started thinking
about it :-)

Thanks,

Marcus

Am 23.03.2018 um 12:42 schrieb Valentin Vidic:
> On Fri, Mar 23, 2018 at 11:22:39AM +0100, Marcus Wolf wrote:
>> could you please decribe in short words, why you think, that hte lock
>> isn't obsolete?
>>
>> I wasn't sure, but close to remove the lock. That's why I putted the
>> comment.
> 
> Sure, if pi433_tx_thread runs on one CPU it might be possible
> to call pi433_write concurrently on another CPU and they would
> both modify tx_fifo.  But maybe there is some other protection
> in place that would prevent this?
> 

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

could you please decribe in short words, why you think, that hte lock
isn't obsolete?

I wasn't sure, but close to remove the lock. That's why I putted the
comment.

Thanks,

Marcus

Am 23.03.2018 um 10:47 schrieb Valentin Vidic:
> Removes TODO for tx_fifo_lock as tx_fifo is modified from
> both pi433_tx_thread and pi433_write.
> 
> Fixes checkpatch warning:
> 
>   CHECK: struct mutex definition without comment
> 
> Signed-off-by: Valentin Vidic <valentin.vi...@carnet.hr>
> ---
>  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 d1e0ddbc79ce..f6f106a3ff8e 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -87,7 +87,7 @@ struct pi433_device {
>  
>   /* tx related values */
>   STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
> - struct mutextx_fifo_lock; // TODO: check, whether necessary 
> or obsolete
> + struct mutextx_fifo_lock; /* serialize access to tx_fifo */
>   struct task_struct  *tx_task_struct;
>   wait_queue_head_t   tx_wait_queue;
>   u8  free_in_fifo;
> @@ -100,7 +100,7 @@ struct pi433_device {
>   u32 rx_bytes_to_drop;
>   u32 rx_bytes_dropped;
>   unsigned intrx_position;
> - struct mutexrx_lock;
> + struct mutexrx_lock; /* serialize read requests */
>   wait_queue_head_t   rx_wait_queue;
>  
>   /* fifo wait queue */
> 

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

Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Marcus Wolf]

Hi Valentin,

could you please decribe in short words, why you think, that hte lock
isn't obsolete?

I wasn't sure, but close to remove the lock. That's why I putted the
comment.

Thanks,

Marcus

Am 23.03.2018 um 10:47 schrieb Valentin Vidic:
> Removes TODO for tx_fifo_lock as tx_fifo is modified from
> both pi433_tx_thread and pi433_write.
> 
> Fixes checkpatch warning:
> 
>   CHECK: struct mutex definition without comment
> 
> Signed-off-by: Valentin Vidic 
> ---
>  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 d1e0ddbc79ce..f6f106a3ff8e 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -87,7 +87,7 @@ struct pi433_device {
>  
>   /* tx related values */
>   STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
> - struct mutextx_fifo_lock; // TODO: check, whether necessary 
> or obsolete
> + struct mutextx_fifo_lock; /* serialize access to tx_fifo */
>   struct task_struct  *tx_task_struct;
>   wait_queue_head_t   tx_wait_queue;
>   u8  free_in_fifo;
> @@ -100,7 +100,7 @@ struct pi433_device {
>   u32 rx_bytes_to_drop;
>   u32 rx_bytes_dropped;
>   unsigned intrx_position;
> - struct mutexrx_lock;
> + struct mutexrx_lock; /* serialize read requests */
>   wait_queue_head_t   rx_wait_queue;
>  
>   /* fifo wait queue */
> 

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

Re: [PATCH] staging: pi433: remove unnecessary parentheses

[marcus . wolf]

Joe Perches schrieb am 10.01.2018 10:05:
> On Wed, 2018-01-10 at 09:44 +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 09, 2018 at 11:42:16AM -0800, Joe Perches wrote:
> > > if (a == b && c == d)
> > > is pretty trivial.
> >
> > But again, don't do that.
> 
>  We disagree. Life goes on.
> 
> cheers, Joe
>
>

For me the line above isn't obvious and easy to read. If I would be in doubt, 
whether it really performs correctly, I would have to ask the c-guide, to be 
absolutely shure.
But to be honest: If I need to find a bug arround taht lines, I wouldn't ask 
the c-guide, but simply add some (). Then it would be 100% clear and no one 
would be in doubt any more.

What's the disadvantage of () to emphasise waht is going on. An other Option 
for me would be, to spend a command line and write that info in form of a 
comment.


Just my opinion and the way, I would go on if I am in doubt and need to find a 
bug.

Cheers,

Marcus

Re: [PATCH] staging: pi433: remove unnecessary parentheses

[marcus . wolf]

Joe Perches schrieb am 10.01.2018 10:05:
> On Wed, 2018-01-10 at 09:44 +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 09, 2018 at 11:42:16AM -0800, Joe Perches wrote:
> > > if (a == b && c == d)
> > > is pretty trivial.
> >
> > But again, don't do that.
> 
>  We disagree. Life goes on.
> 
> cheers, Joe
>
>

For me the line above isn't obvious and easy to read. If I would be in doubt, 
whether it really performs correctly, I would have to ask the c-guide, to be 
absolutely shure.
But to be honest: If I need to find a bug arround taht lines, I wouldn't ask 
the c-guide, but simply add some (). Then it would be 100% clear and no one 
would be in doubt any more.

What's the disadvantage of () to emphasise waht is going on. An other Option 
for me would be, to spend a command line and write that info in form of a 
comment.


Just my opinion and the way, I would go on if I am in doubt and need to find a 
bug.

Cheers,

Marcus

[PATCH] Staging: Pi433: Bugfix for wrong argument for sizeof() in TX thread

[Marcus Wolf]

sizeof(array) != sizeof(pointer to array)
Fixes: "staging: pi433: reduce stack size in tx thread"

Signed-off-by: Marcus Wolf <li...@wolf-entwicklungen.de>
---
 drivers/staging/pi433/pi433_if.c |   13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 1f3ba55..6e6f595 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -565,7 +565,6 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
struct pi433_device *device = data;
struct spi_device *spi = device->spi;
struct pi433_tx_cfg tx_cfg;
-   u8 *buffer = device->buffer;
size_t size;
bool   rx_interrupted = false;
intposition, repetitions;
@@ -614,19 +613,19 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
size++;
 
/* prime buffer */
-   memset(buffer, 0, size);
+   memset(device->buffer, 0, size);
position = 0;
 
/* add length byte, if requested */
if (tx_cfg.enable_length_byte  == OPTION_ON)
-   buffer[position++] = size - 1; /* according to spec 
length byte itself must be excluded from the length calculation */
+   device->buffer[position++] = size - 1; /* according to 
spec length byte itself must be excluded from the length calculation */
 
/* add adr byte, if requested */
if (tx_cfg.enable_address_byte == OPTION_ON)
-   buffer[position++] = tx_cfg.address_byte;
+   device->buffer[position++] = tx_cfg.address_byte;
 
/* finally get message data from fifo */
-   retval = kfifo_out(>tx_fifo, [position], 
sizeof(buffer) - position);
+   retval = kfifo_out(>tx_fifo, >buffer[position], 
sizeof(device->buffer) - position);
dev_dbg(device->dev, "read %d message byte(s) from fifo 
queue.", retval);
mutex_unlock(>tx_fifo_lock);
 
@@ -708,7 +707,7 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
int temp = device->free_in_fifo;
device->free_in_fifo = 0;
rf69_write_fifo(spi,
-   [position],
+   >buffer[position],
temp);
position += temp;
} else {
@@ -716,7 +715,7 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
device->free_in_fifo -= size;
repetitions--;
rf69_write_fifo(spi,
-   [position],
+   >buffer[position],
(size - position));
position = 0; /* reset for next repetition */
}
-- 
1.7.10.4

[PATCH] Staging: Pi433: Bugfix for wrong argument for sizeof() in TX thread

[Marcus Wolf]

sizeof(array) != sizeof(pointer to array)
Fixes: "staging: pi433: reduce stack size in tx thread"

Signed-off-by: Marcus Wolf 
---
 drivers/staging/pi433/pi433_if.c |   13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 1f3ba55..6e6f595 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -565,7 +565,6 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
struct pi433_device *device = data;
struct spi_device *spi = device->spi;
struct pi433_tx_cfg tx_cfg;
-   u8 *buffer = device->buffer;
size_t size;
bool   rx_interrupted = false;
intposition, repetitions;
@@ -614,19 +613,19 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
size++;
 
/* prime buffer */
-   memset(buffer, 0, size);
+   memset(device->buffer, 0, size);
position = 0;
 
/* add length byte, if requested */
if (tx_cfg.enable_length_byte  == OPTION_ON)
-   buffer[position++] = size - 1; /* according to spec 
length byte itself must be excluded from the length calculation */
+   device->buffer[position++] = size - 1; /* according to 
spec length byte itself must be excluded from the length calculation */
 
/* add adr byte, if requested */
if (tx_cfg.enable_address_byte == OPTION_ON)
-   buffer[position++] = tx_cfg.address_byte;
+   device->buffer[position++] = tx_cfg.address_byte;
 
/* finally get message data from fifo */
-   retval = kfifo_out(>tx_fifo, [position], 
sizeof(buffer) - position);
+   retval = kfifo_out(>tx_fifo, >buffer[position], 
sizeof(device->buffer) - position);
dev_dbg(device->dev, "read %d message byte(s) from fifo 
queue.", retval);
mutex_unlock(>tx_fifo_lock);
 
@@ -708,7 +707,7 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
int temp = device->free_in_fifo;
device->free_in_fifo = 0;
rf69_write_fifo(spi,
-   [position],
+   >buffer[position],
temp);
position += temp;
} else {
@@ -716,7 +715,7 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
device->free_in_fifo -= size;
repetitions--;
rf69_write_fifo(spi,
-   [position],
+   >buffer[position],
(size - position));
position = 0; /* reset for next repetition */
}
-- 
1.7.10.4

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:18 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 08:59:35PM +0200, Marcus Wolf wrote:


Am 04.12.2017 um 12:33 schrieb Dan Carpenter:

On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:

diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 34ff0d4807bd..bcfe29840889 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -63,7 +63,7 @@ struct pi433_tx_cfg {
__u16   bit_rate;
__u32   dev_frequency;
enum modulation modulation;
-   enum modShaping modShaping;
+   enum mod_shapingmod_shaping;


I looked at how mod_shaping is set and the only place is in the ioctl:

 789  case PI433_IOC_WR_TX_CFG:
 790  if (copy_from_user(>tx_cfg, argp,
 791  sizeof(struct pi433_tx_cfg)))
 792  return -EFAULT;
 793  break;

We just write over the whole config.  Including important things like
rx_cfg.fixed_message_length.  There is no locking so when we do things
like:

 385  /* fixed or unlimited length? */
 386  if (dev->rx_cfg.fixed_message_length != 0)
 387  {
 388  if (dev->rx_cfg.fixed_message_length > 
dev->rx_buffer_size)
  
^^
check

 389  {
 390  retval = -1;
 391  goto abort;
 392  }
 393  bytes_total = dev->rx_cfg.fixed_message_length;
^
set this in the ioctl after the check but before this line and it looks
like a security problem.

 394  dev_dbg(dev->dev,"rx: msg len set to %d by fixed 
length", bytes_total);
 395  }

Anyway, I guess this patch is fine.

regards,
dan carpenter



Hi Dan,

you are mixing rx and tx. The part from IOCTL is copied from the tx-part,
the lower part is dealing with rx.

With rx there should be no problem, since IOCTL is blocked, as long as an rx
operation is going on.

With tx, I also expect no problems, since instance->tx_cfg is never used to
configure the rf69. Everytime, you pass in new data via write() a copy of
tx_cfg is made. Transmission is done, using the copy of the tx_cfg, never by
using instance->tx_cfg.

But maybe I didn't got your point and misunderstand your intention.



No.  You're right.  I mixed up rx and tx.  But the ioctl interface still
seems really horrible.  We generally frown on adding new ioctls at all,
but in this case to just write over the whole struct with no locking
seems really bad.

regards,
dan carpenter



Hi Dan,

unexpectetly I was into the driver code today, because a customer asked 
for an enhancment. In doing so, I also had a look at the points we 
discussed above.


Since both - the tx_cfg and the rx_cfg buffer belong to the instance, in 
order to get into trouble, you need to use the same file descriptor. If 
an other app is changing its config, it doesn't touch the current app.


So for RX: If a programm has called read(), it won't be able to 
succesfully call ioctl any more, because it is blocked:


case PI433_IOC_WR_RX_CFG:
mutex_lock(>rx_lock);

/* during pendig read request, change of config not 
allowed */

if (device->rx_active) {
mutex_unlock(>rx_lock);
return -EAGAIN;
}

For TX in fact there is a little risk:
If a programm is using two tasks and passes the descriptor to both 
tasks, one is using the ioctl() and one is using write() and they are 
not synchronised, it might happen, that the ioctl is in the middle of 
the update the tx_cfg, while the write() is in the middle of copying the 
tx_cfg to the kernel fifo.
On one hand, that might be an "open point" at the driver, on the other 
hand no one will do such a programm architecture. Even if the driver 
will prevent a broken tx_cfg by mutex, the programm will never know, 
what it gets, if it issues ioctl() and write() unsynchronised from 
different tasks.
For fixing the driver, it might help to lock the write to the tx_cfg in 
ioctl() with the tx_fifo_lock, since write() is only copying the tx_cfg 
if it has the tx_fifo_lock.


I am not 100% sure. Maybe you (or someone else) want to crosscheck?

Regards,

Marcus

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:18 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 08:59:35PM +0200, Marcus Wolf wrote:


Am 04.12.2017 um 12:33 schrieb Dan Carpenter:

On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:

diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 34ff0d4807bd..bcfe29840889 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -63,7 +63,7 @@ struct pi433_tx_cfg {
__u16   bit_rate;
__u32   dev_frequency;
enum modulation modulation;
-   enum modShaping modShaping;
+   enum mod_shapingmod_shaping;


I looked at how mod_shaping is set and the only place is in the ioctl:

 789  case PI433_IOC_WR_TX_CFG:
 790  if (copy_from_user(>tx_cfg, argp,
 791  sizeof(struct pi433_tx_cfg)))
 792  return -EFAULT;
 793  break;

We just write over the whole config.  Including important things like
rx_cfg.fixed_message_length.  There is no locking so when we do things
like:

 385  /* fixed or unlimited length? */
 386  if (dev->rx_cfg.fixed_message_length != 0)
 387  {
 388  if (dev->rx_cfg.fixed_message_length > 
dev->rx_buffer_size)
  
^^
check

 389  {
 390  retval = -1;
 391  goto abort;
 392  }
 393  bytes_total = dev->rx_cfg.fixed_message_length;
^
set this in the ioctl after the check but before this line and it looks
like a security problem.

 394  dev_dbg(dev->dev,"rx: msg len set to %d by fixed 
length", bytes_total);
 395  }

Anyway, I guess this patch is fine.

regards,
dan carpenter



Hi Dan,

you are mixing rx and tx. The part from IOCTL is copied from the tx-part,
the lower part is dealing with rx.

With rx there should be no problem, since IOCTL is blocked, as long as an rx
operation is going on.

With tx, I also expect no problems, since instance->tx_cfg is never used to
configure the rf69. Everytime, you pass in new data via write() a copy of
tx_cfg is made. Transmission is done, using the copy of the tx_cfg, never by
using instance->tx_cfg.

But maybe I didn't got your point and misunderstand your intention.



No.  You're right.  I mixed up rx and tx.  But the ioctl interface still
seems really horrible.  We generally frown on adding new ioctls at all,
but in this case to just write over the whole struct with no locking
seems really bad.

regards,
dan carpenter



Hi Dan,

unexpectetly I was into the driver code today, because a customer asked 
for an enhancment. In doing so, I also had a look at the points we 
discussed above.


Since both - the tx_cfg and the rx_cfg buffer belong to the instance, in 
order to get into trouble, you need to use the same file descriptor. If 
an other app is changing its config, it doesn't touch the current app.


So for RX: If a programm has called read(), it won't be able to 
succesfully call ioctl any more, because it is blocked:


case PI433_IOC_WR_RX_CFG:
mutex_lock(>rx_lock);

/* during pendig read request, change of config not 
allowed */

if (device->rx_active) {
mutex_unlock(>rx_lock);
return -EAGAIN;
}

For TX in fact there is a little risk:
If a programm is using two tasks and passes the descriptor to both 
tasks, one is using the ioctl() and one is using write() and they are 
not synchronised, it might happen, that the ioctl is in the middle of 
the update the tx_cfg, while the write() is in the middle of copying the 
tx_cfg to the kernel fifo.
On one hand, that might be an "open point" at the driver, on the other 
hand no one will do such a programm architecture. Even if the driver 
will prevent a broken tx_cfg by mutex, the programm will never know, 
what it gets, if it issues ioctl() and write() unsynchronised from 
different tasks.
For fixing the driver, it might help to lock the write to the tx_cfg in 
ioctl() with the tx_fifo_lock, since write() is only copying the tx_cfg 
if it has the tx_fifo_lock.


I am not 100% sure. Maybe you (or someone else) want to crosscheck?

Regards,

Marcus

rf69_get_lna_gain

[Marcus Wolf]

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

rf69_get_lna_gain

[Marcus Wolf]

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

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

[Marcus Wolf]

Am 13.12.2017 um 19:44 schrieb Valentin Vidic:

Avoid shifting by magic numbers and use defines instead.

Signed-off-by: Valentin Vidic 
---
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_360x05
  #define  LNA_GAIN_MAX_MINUS_480x06
  
+#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

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

[Marcus Wolf]

Am 13.12.2017 um 19:44 schrieb Valentin Vidic:

Avoid shifting by magic numbers and use defines instead.

Signed-off-by: Valentin Vidic 
---
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_360x05
  #define  LNA_GAIN_MAX_MINUS_480x06
  
+#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

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

[Marcus Wolf]

Am 13.12.2017 um 18:55 schrieb Valentin Vidic:

Avoid shifting by magic numbers and use defines instead.

Signed-off-by: Valentin Vidic 
---
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_GAIN0x38
  #define  MASK_LNA_GAIN0x07
+#define  SHIFT_LNA_CURRENT_GAIN3
  
  #define  LNA_GAIN_AUTO0x00 /* default */

  #define  LNA_GAIN_MAX 0x01
@@ -246,6 +247,13 @@
  #define  LNA_GAIN_MAX_MINUS_360x05
  #define  LNA_GAIN_MAX_MINUS_480x06
  
+#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

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

[Marcus Wolf]

Am 13.12.2017 um 18:55 schrieb Valentin Vidic:

Avoid shifting by magic numbers and use defines instead.

Signed-off-by: Valentin Vidic 
---
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_GAIN0x38
  #define  MASK_LNA_GAIN0x07
+#define  SHIFT_LNA_CURRENT_GAIN3
  
  #define  LNA_GAIN_AUTO0x00 /* default */

  #define  LNA_GAIN_MAX 0x01
@@ -246,6 +247,13 @@
  #define  LNA_GAIN_MAX_MINUS_360x05
  #define  LNA_GAIN_MAX_MINUS_480x06
  
+#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

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

[Marcus Wolf]

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 <valentin.vi...@carnet.hr>
---
  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_CONTINUOUS0x40
@@ -235,6 +236,7 @@
  #define  MASK_LNA_ZIN 0x80
  #define  MASK_LNA_CURRENT_GAIN0x38
  #define  MASK_LNA_GAIN0x07
+#define  SHIFT_LNA_CURRENT_GAIN3
  
  #define  LNA_GAIN_AUTO0x00 /* 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

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

[Marcus Wolf]

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 
---
  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_CONTINUOUS0x40
@@ -235,6 +236,7 @@
  #define  MASK_LNA_ZIN 0x80
  #define  MASK_LNA_CURRENT_GAIN0x38
  #define  MASK_LNA_GAIN0x07
+#define  SHIFT_LNA_CURRENT_GAIN3
  
  #define  LNA_GAIN_AUTO0x00 /* 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

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

[Marcus Wolf]

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 <valentin.vi...@carnet.hr>
---
  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

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

[Marcus Wolf]

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

staging: pi433: Plans from Smarthome-Wolf

[Marcus Wolf]

Am 06.12.2017 um 14:47 schrieb Dan Carpenter:
> On Wed, Dec 06, 2017 at 11:11:27AM +0200, Marcus Wolf wrote:
>>
>> Since the rule for kernel development seems to be, not to care about future,
>> most probably you patch is fine, anyway.
>>
> 
> Yeah.  Deleting code if there is no user is required to move this code
> out of staging...
> 
> I've worked in staging for a long time and I've seen patches from
> thousands of people.  Simon really seems to understand kernel style and
> that's less common than one might think.  It's a valuable skill.  If you
> would just leave him to work then this driver would get fixed...
> 
> You're making it very difficult because you're complaining about the
> work which *needs* to be done.  It's discouraging for people sending
> patches.  This is very frustrating...  :(
> 
> On the naming, I think having a function which is named "enable" but
> actually disables a feature is a non-starter.  What about we do either
> one of these:
>  pi433_enable_(spi);
>  pi433_disable_(spi);
> Or:
>  pi433_set_(spi, SETTING);
> 
> It's still a standard and we'll just decide on a case by case basis what
> to use.  This is a tiny driver and it's really easy to grep for the
> feature name to find the functions you want.  Plus all the config is
> done in one location so it's really not that hard.
> 
> regards,
> dan carpenter
> 

Hi Greg, Dan, Simon and all others,

yesterday we had a project meeting. Though I am the boss, I was
"punished" that I spend such a bunch of time to discuss here, instead of
finishing stuff from my todo list :-P

I get the point, that I am not really deep in the kernel style guide and
it is better, if I don't disturb the nerds.

Both facts in combination tell me, that I should lean back, wait a while
and see, what the driver will become.


On the other hand, my team was - like me - a little bit in worry about
this "closing a door", that happend a few times during the last weeks
and the possible redesign of the driver architecture. It would be pitty,
if the effort for integration of new features will be complicated a lot.


We finally decided, that I actually should reduce focussing on the
driver for the moment and let things go. We'd like to use this mail, to
tell our ideas and the plan for next year:

When submitting the driver, we wanted to add support to the kernel for
the technical really good modules from HopeRf. The idea was to support
serveral chips and several modules (carrying those chips). Due to
financial and time restrictions, we finally had to reduce to rfm69cw and
focused on Pi433.
Plan for the next year:
* Tweak the driver (if needed) to enable the reception of telegrams
without preamble and sync pattern. We never tested this before. This
feature will be needed aprox. in March'18.
* Support for the rfm69cw-868 - Almost the same module as the current,
but for the 868MHz ISM band. Will be needed approx. end of summer next year.
* We would like to add support for some more features of the chip (e. g.
AES) - you can see, there are lots of so far not covered registers
(commented lines in rf69_registers.h). No shedule for this.
* If we will have a (financially) successfull 2018, we think about
integration of the rf95 chip.

>From third parties we were asked about the follwoing features:
* Implement support for continuous mode (e. g. from projects fhem,
domotics).
* Do a little bit more generic inteface implementation, to also support
other hardware setups, using the rf69 chip.
Second sounds very interesting - especially in terms of a real Linux
driver. But both topics so far aren't on our agenda.


So I will withdraw from the development of pi433 driver for the next
monthes and will be back at the beginning of spring. I hope, there are
not too many closed doors, so I can easily start over with head rev. and
don't have to fall back to my old base. To ease a start over, I would
love the following things (as much, as possible without breaking the rules):
* do not modify the register abstraction in a way, that it doesn't
represent the real hardware any more (e. g. the stuff with amp - the
chip does not have two registers, taking chipnumbers, but 3 bits, taking
on/off information). In doubt have a look at the data sheet.
* stay with the naming convention for the register abstraction
(rf69_set_... and rf69_get_...). If for some reason a register (or bit)
needs to be read back for some reason in future, it is unlovely to have
rf69_do_something and you need to find a adequate name for the getter.
rf69_get_do_something is ugly most times.
* if possible, do not remove my tiny "debug system" in the register
abstraction. It's very powerfull, if you work on config changes.
* try to keep the current or find a new way, that a setting from user
space could be mapped to (not identical) register sets of diff

staging: pi433: Plans from Smarthome-Wolf

[Marcus Wolf]

Am 06.12.2017 um 14:47 schrieb Dan Carpenter:
> On Wed, Dec 06, 2017 at 11:11:27AM +0200, Marcus Wolf wrote:
>>
>> Since the rule for kernel development seems to be, not to care about future,
>> most probably you patch is fine, anyway.
>>
> 
> Yeah.  Deleting code if there is no user is required to move this code
> out of staging...
> 
> I've worked in staging for a long time and I've seen patches from
> thousands of people.  Simon really seems to understand kernel style and
> that's less common than one might think.  It's a valuable skill.  If you
> would just leave him to work then this driver would get fixed...
> 
> You're making it very difficult because you're complaining about the
> work which *needs* to be done.  It's discouraging for people sending
> patches.  This is very frustrating...  :(
> 
> On the naming, I think having a function which is named "enable" but
> actually disables a feature is a non-starter.  What about we do either
> one of these:
>  pi433_enable_(spi);
>  pi433_disable_(spi);
> Or:
>  pi433_set_(spi, SETTING);
> 
> It's still a standard and we'll just decide on a case by case basis what
> to use.  This is a tiny driver and it's really easy to grep for the
> feature name to find the functions you want.  Plus all the config is
> done in one location so it's really not that hard.
> 
> regards,
> dan carpenter
> 

Hi Greg, Dan, Simon and all others,

yesterday we had a project meeting. Though I am the boss, I was
"punished" that I spend such a bunch of time to discuss here, instead of
finishing stuff from my todo list :-P

I get the point, that I am not really deep in the kernel style guide and
it is better, if I don't disturb the nerds.

Both facts in combination tell me, that I should lean back, wait a while
and see, what the driver will become.


On the other hand, my team was - like me - a little bit in worry about
this "closing a door", that happend a few times during the last weeks
and the possible redesign of the driver architecture. It would be pitty,
if the effort for integration of new features will be complicated a lot.


We finally decided, that I actually should reduce focussing on the
driver for the moment and let things go. We'd like to use this mail, to
tell our ideas and the plan for next year:

When submitting the driver, we wanted to add support to the kernel for
the technical really good modules from HopeRf. The idea was to support
serveral chips and several modules (carrying those chips). Due to
financial and time restrictions, we finally had to reduce to rfm69cw and
focused on Pi433.
Plan for the next year:
* Tweak the driver (if needed) to enable the reception of telegrams
without preamble and sync pattern. We never tested this before. This
feature will be needed aprox. in March'18.
* Support for the rfm69cw-868 - Almost the same module as the current,
but for the 868MHz ISM band. Will be needed approx. end of summer next year.
* We would like to add support for some more features of the chip (e. g.
AES) - you can see, there are lots of so far not covered registers
(commented lines in rf69_registers.h). No shedule for this.
* If we will have a (financially) successfull 2018, we think about
integration of the rf95 chip.

>From third parties we were asked about the follwoing features:
* Implement support for continuous mode (e. g. from projects fhem,
domotics).
* Do a little bit more generic inteface implementation, to also support
other hardware setups, using the rf69 chip.
Second sounds very interesting - especially in terms of a real Linux
driver. But both topics so far aren't on our agenda.


So I will withdraw from the development of pi433 driver for the next
monthes and will be back at the beginning of spring. I hope, there are
not too many closed doors, so I can easily start over with head rev. and
don't have to fall back to my old base. To ease a start over, I would
love the following things (as much, as possible without breaking the rules):
* do not modify the register abstraction in a way, that it doesn't
represent the real hardware any more (e. g. the stuff with amp - the
chip does not have two registers, taking chipnumbers, but 3 bits, taking
on/off information). In doubt have a look at the data sheet.
* stay with the naming convention for the register abstraction
(rf69_set_... and rf69_get_...). If for some reason a register (or bit)
needs to be read back for some reason in future, it is unlovely to have
rf69_do_something and you need to find a adequate name for the getter.
rf69_get_do_something is ugly most times.
* if possible, do not remove my tiny "debug system" in the register
abstraction. It's very powerfull, if you work on config changes.
* try to keep the current or find a new way, that a setting from user
space could be mapped to (not identical) register sets of diff

Re: [PATCH 5/6] staging: pi433: Move enum option_on_off to pi433_if.h

[Marcus Wolf]

Am 06.12.2017 um 22:42 schrieb Simon Sandström:

The enum is now only used for ioctl, so move it pi433_if.h.

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.h  | 5 +
  drivers/staging/pi433/rf69_enum.h | 5 -
  2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index bcfe29840889..c8697d144e2b 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -37,6 +37,11 @@
  
  /*---*/
  
+enum option_on_off {

+   OPTION_OFF,
+   OPTION_ON
+};
+
  /* IOCTL structs and commands */
  
  /**

diff --git a/drivers/staging/pi433/rf69_enum.h 
b/drivers/staging/pi433/rf69_enum.h
index b0715b4eb6ac..4e64e38ae4ff 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -18,11 +18,6 @@
  #ifndef RF69_ENUM_H
  #define RF69_ENUM_H
  
-enum option_on_off {

-   OPTION_OFF,
-   OPTION_ON
-};
-
  enum mode {
mode_sleep,
standby,



Hi Simon,

sorry - I have one more question.

Why do you want to get rid of the enum option_on_off in rf69_enum.h. I 
generated that file just to store the enums.


Now we have one enum at an other place...

Regards,
Marcus

Re: [PATCH 5/6] staging: pi433: Move enum option_on_off to pi433_if.h

[Marcus Wolf]

Am 06.12.2017 um 22:42 schrieb Simon Sandström:

The enum is now only used for ioctl, so move it pi433_if.h.

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.h  | 5 +
  drivers/staging/pi433/rf69_enum.h | 5 -
  2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index bcfe29840889..c8697d144e2b 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -37,6 +37,11 @@
  
  /*---*/
  
+enum option_on_off {

+   OPTION_OFF,
+   OPTION_ON
+};
+
  /* IOCTL structs and commands */
  
  /**

diff --git a/drivers/staging/pi433/rf69_enum.h 
b/drivers/staging/pi433/rf69_enum.h
index b0715b4eb6ac..4e64e38ae4ff 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -18,11 +18,6 @@
  #ifndef RF69_ENUM_H
  #define RF69_ENUM_H
  
-enum option_on_off {

-   OPTION_OFF,
-   OPTION_ON
-};
-
  enum mode {
mode_sleep,
standby,



Hi Simon,

sorry - I have one more question.

Why do you want to get rid of the enum option_on_off in rf69_enum.h. I 
generated that file just to store the enums.


Now we have one enum at an other place...

Regards,
Marcus

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 06.12.2017 um 21:57 schrieb Simon Sandström:

On Wed, Dec 06, 2017 at 01:44:14PM +0300, Dan Carpenter wrote:

On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:



Am 06.12.2017 um 12:23 schrieb Dan Carpenter:



Wow...  This was the one patch I thought was going to sink this
patchset...


I don't get that. What do you mean?


Isn't enum optionOnOff part of the userspace headers?

I thought we weren't allowed to change that.


All enums are for user space (or inteded to be used in userspace in future).
Didn't introduce enums for internal use.


So what I'm asking is if we do this change, does it break any userspace
programs which are used *right now*.  In other words will programs stop
compiling because we've renamed an enum?

regards,
dan carpenter



Hi,

I'm not sure about this so I have to ask: wouldn't the header need to be
in include/uapi/ for userspace to use them? Or is it "allowed" for
userspace programs to use this internal header? Or are we afraid to
break userspace programs that keeps a copy of pi433_if.h?


Regards,
Simon



Hi Simon,

in principle, I think you are right. With my first patch, offering the 
driver I did it that way, but Greg asked me to migrate everything 
(including docu) into staging/pi433. I think, that's related to being in 
staging.
At the moment, I copy pi433_if.h and rf69_enum.h to my userspace 
program, in order to be able to compile it.


To be honest, I am a little bit astonished about that question. Don't 
you do a regression test after such a redesign? I would be a little bit 
afraid to offer such redesign without testing.



Regards,
Marcus

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 06.12.2017 um 21:57 schrieb Simon Sandström:

On Wed, Dec 06, 2017 at 01:44:14PM +0300, Dan Carpenter wrote:

On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:



Am 06.12.2017 um 12:23 schrieb Dan Carpenter:



Wow...  This was the one patch I thought was going to sink this
patchset...


I don't get that. What do you mean?


Isn't enum optionOnOff part of the userspace headers?

I thought we weren't allowed to change that.


All enums are for user space (or inteded to be used in userspace in future).
Didn't introduce enums for internal use.


So what I'm asking is if we do this change, does it break any userspace
programs which are used *right now*.  In other words will programs stop
compiling because we've renamed an enum?

regards,
dan carpenter



Hi,

I'm not sure about this so I have to ask: wouldn't the header need to be
in include/uapi/ for userspace to use them? Or is it "allowed" for
userspace programs to use this internal header? Or are we afraid to
break userspace programs that keeps a copy of pi433_if.h?


Regards,
Simon



Hi Simon,

in principle, I think you are right. With my first patch, offering the 
driver I did it that way, but Greg asked me to migrate everything 
(including docu) into staging/pi433. I think, that's related to being in 
staging.
At the moment, I copy pi433_if.h and rf69_enum.h to my userspace 
program, in order to be able to compile it.


To be honest, I am a little bit astonished about that question. Don't 
you do a regression test after such a redesign? I would be a little bit 
afraid to offer such redesign without testing.



Regards,
Marcus

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Sorry, for sending HTML as well - I am writing from my phone...

Yes, you will break something, when renaming.

Since order isn't changed, most probably old programs will still compile with 
old enum.h.

If we want to move from optionOnOff to bool, we will also break old progs.

Since driver is new (mainline for just something like 2 monthes) and stil under 
devel, I think we should "risk" it.

Gruß,

Marcus 

> Am 06.12.2017 um 11:44 schrieb Dan Carpenter <dan.carpen...@oracle.com>:
> 
>> On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
>> 
>> 
>>> Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
>>> On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
>>>>> diff --git a/drivers/staging/pi433/rf69_enum.h 
>>>>> b/drivers/staging/pi433/rf69_enum.h
>>>>> index babe597e2ec6..5247e9269de9 100644
>>>>> --- a/drivers/staging/pi433/rf69_enum.h
>>>>> +++ b/drivers/staging/pi433/rf69_enum.h
>>>>> @@ -18,9 +18,9 @@
>>>>>   #ifndef RF69_ENUM_H
>>>>>   #define RF69_ENUM_H
>>>>> -enum optionOnOff {
>>>>> -optionOff,
>>>>> -optionOn
>>>>> +enum option_on_off {
>>>>> +OPTION_OFF,
>>>>> +OPTION_ON
>>>>>   };
>>>>>   enum mode {
>>>>> 
>>>> 
>>>> Hi Simon,
>>>> 
>>>> nice work.
>>>> 
>>>> Thank you very much for all the style fixes :-)
>>>> 
>>> 
>>> 
>>> Wow...  This was the one patch I thought was going to sink this
>>> patchset...
>> 
>> I don't get that. What do you mean?
>> 
>>> Isn't enum optionOnOff part of the userspace headers?
>>> 
>>> I thought we weren't allowed to change that.
>> 
>> All enums are for user space (or inteded to be used in userspace in future).
>> Didn't introduce enums for internal use.
> 
> So what I'm asking is if we do this change, does it break any userspace
> programs which are used *right now*.  In other words will programs stop
> compiling because we've renamed an enum?
> 
> regards,
> dan carpenter

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Sorry, for sending HTML as well - I am writing from my phone...

Yes, you will break something, when renaming.

Since order isn't changed, most probably old programs will still compile with 
old enum.h.

If we want to move from optionOnOff to bool, we will also break old progs.

Since driver is new (mainline for just something like 2 monthes) and stil under 
devel, I think we should "risk" it.

Gruß,

Marcus 

> Am 06.12.2017 um 11:44 schrieb Dan Carpenter :
> 
>> On Wed, Dec 06, 2017 at 12:31:31PM +0200, Marcus Wolf wrote:
>> 
>> 
>>> Am 06.12.2017 um 12:23 schrieb Dan Carpenter:
>>> On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:
>>>>> diff --git a/drivers/staging/pi433/rf69_enum.h 
>>>>> b/drivers/staging/pi433/rf69_enum.h
>>>>> index babe597e2ec6..5247e9269de9 100644
>>>>> --- a/drivers/staging/pi433/rf69_enum.h
>>>>> +++ b/drivers/staging/pi433/rf69_enum.h
>>>>> @@ -18,9 +18,9 @@
>>>>>   #ifndef RF69_ENUM_H
>>>>>   #define RF69_ENUM_H
>>>>> -enum optionOnOff {
>>>>> -optionOff,
>>>>> -optionOn
>>>>> +enum option_on_off {
>>>>> +OPTION_OFF,
>>>>> +OPTION_ON
>>>>>   };
>>>>>   enum mode {
>>>>> 
>>>> 
>>>> Hi Simon,
>>>> 
>>>> nice work.
>>>> 
>>>> Thank you very much for all the style fixes :-)
>>>> 
>>> 
>>> 
>>> Wow...  This was the one patch I thought was going to sink this
>>> patchset...
>> 
>> I don't get that. What do you mean?
>> 
>>> Isn't enum optionOnOff part of the userspace headers?
>>> 
>>> I thought we weren't allowed to change that.
>> 
>> All enums are for user space (or inteded to be used in userspace in future).
>> Didn't introduce enums for internal use.
> 
> So what I'm asking is if we do this change, does it break any userspace
> programs which are used *right now*.  In other words will programs stop
> compiling because we've renamed an enum?
> 
> regards,
> dan carpenter

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

[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

[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 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 06.12.2017 um 12:23 schrieb Dan Carpenter:

On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:

diff --git a/drivers/staging/pi433/rf69_enum.h 
b/drivers/staging/pi433/rf69_enum.h
index babe597e2ec6..5247e9269de9 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -18,9 +18,9 @@
   #ifndef RF69_ENUM_H
   #define RF69_ENUM_H
-enum optionOnOff {
-   optionOff,
-   optionOn
+enum option_on_off {
+   OPTION_OFF,
+   OPTION_ON
   };
   enum mode {



Hi Simon,

nice work.

Thank you very much for all the style fixes :-)




Wow...  This was the one patch I thought was going to sink this
patchset... 


I don't get that. What do you mean?


Isn't enum optionOnOff part of the userspace headers?

I thought we weren't allowed to change that.


All enums are for user space (or inteded to be used in userspace in 
future). Didn't introduce enums for internal use.


Reagrds,
Marcus

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 06.12.2017 um 12:23 schrieb Dan Carpenter:

On Wed, Dec 06, 2017 at 11:46:41AM +0200, Marcus Wolf wrote:

diff --git a/drivers/staging/pi433/rf69_enum.h 
b/drivers/staging/pi433/rf69_enum.h
index babe597e2ec6..5247e9269de9 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -18,9 +18,9 @@
   #ifndef RF69_ENUM_H
   #define RF69_ENUM_H
-enum optionOnOff {
-   optionOff,
-   optionOn
+enum option_on_off {
+   OPTION_OFF,
+   OPTION_ON
   };
   enum mode {



Hi Simon,

nice work.

Thank you very much for all the style fixes :-)




Wow...  This was the one patch I thought was going to sink this
patchset... 


I don't get that. What do you mean?


Isn't enum optionOnOff part of the userspace headers?

I thought we weren't allowed to change that.


All enums are for user space (or inteded to be used in userspace in 
future). Didn't introduce enums for internal use.


Reagrds,
Marcus

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

[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 <si...@nikanor.nu>
---
   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

[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] staging: pi433: Fixes issue with bit shift in rf69_get_modulation

[Marcus Wolf]

Am 06.12.2017 um 11:02 schrieb Greg KH:

On Wed, Nov 08, 2017 at 07:13:56PM +0200, Marcus Wolf wrote:

Fixes issue with bit shift in rf69_get_modulation


What "issue"?



Signed-off-by: Marcus Wolf <li...@wolf-entwicklungen.de>
---
  drivers/staging/pi433/rf69.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 290b419..c945b4b 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 = READ_REG(REG_DATAMODUL);
  
-	switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> 3) { // TODO improvement: change 3 to define

+   switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE) {


Doesn't this change the logic here?

thanks,

greg k-h



Hi Greg,

yes, it does.

This is one of the very few changes to pi433 driver, that does not 
modify the architecture or optics of the code, but really fixes a bug. 
This function wasn't working from the very beginning, and we had already 
several reports and patches (from me and otheres), announcing or trying 
to fix the bug. But so far all patches were skipped for some reason.



Please take the patch.

Marcus

Re: [PATCH] staging: pi433: Fixes issue with bit shift in rf69_get_modulation

[Marcus Wolf]

Am 06.12.2017 um 11:02 schrieb Greg KH:

On Wed, Nov 08, 2017 at 07:13:56PM +0200, Marcus Wolf wrote:

Fixes issue with bit shift in rf69_get_modulation


What "issue"?



Signed-off-by: Marcus Wolf 
---
  drivers/staging/pi433/rf69.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 290b419..c945b4b 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 = READ_REG(REG_DATAMODUL);
  
-	switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> 3) { // TODO improvement: change 3 to define

+   switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE) {


Doesn't this change the logic here?

thanks,

greg k-h



Hi Greg,

yes, it does.

This is one of the very few changes to pi433 driver, that does not 
modify the architecture or optics of the code, but really fixes a bug. 
This function wasn't working from the very beginning, and we had already 
several reports and patches (from me and otheres), announcing or trying 
to fix the bug. But so far all patches were skipped for some reason.



Please take the patch.

Marcus

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

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

Renames the enum optionOnOff and its values optionOn, optionOff to enum
option_on_off and OPTION_ON, OPTION_OFF. Fixes checkpatch.pl warnings:
"Avoid CamelCase: , , ".

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c  | 34 ++---
  drivers/staging/pi433/pi433_if.h  | 16 +++---
  drivers/staging/pi433/rf69.c  | 45 ++-
  drivers/staging/pi433/rf69.h  | 15 -
  drivers/staging/pi433/rf69_enum.h |  6 +++---
  5 files changed, 63 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b8efe6a74a2a..4f6830f369e9 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -198,7 +198,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
/* packet config */
/* enable */
SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, 
afterSyncInterrupt));
}
@@ -206,7 +206,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
}
-   if (rx_cfg->enable_length_byte == optionOn) {
+   if (rx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -220,14 +220,14 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
  
  	/* lengths */

SET_CHECKED(rf69_set_sync_size(dev->spi, rx_cfg->sync_length));
-   if (rx_cfg->enable_length_byte == optionOn)
+   if (rx_cfg->enable_length_byte == OPTION_ON)
{
SET_CHECKED(rf69_set_payload_length(dev->spi, 0xff));
}
else if (rx_cfg->fixed_message_length != 0)
{
payload_length = rx_cfg->fixed_message_length;
-   if (rx_cfg->enable_length_byte  == optionOn) payload_length++;
+   if (rx_cfg->enable_length_byte  == OPTION_ON) payload_length++;
if (rx_cfg->enable_address_filtering != filteringOff) 
payload_length++;
SET_CHECKED(rf69_set_payload_length(dev->spi, payload_length));
}
@@ -237,7 +237,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
}
  
  	/* values */

-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_sync_values(dev->spi, 
rx_cfg->sync_pattern));
}
@@ -264,7 +264,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_tx_start_condition(dev->spi, 
tx_cfg->tx_start_condition));
  
  	/* packet format enable */

-   if (tx_cfg->enable_preamble == optionOn)
+   if (tx_cfg->enable_preamble == OPTION_ON)
{
SET_CHECKED(rf69_set_preamble_length(dev->spi, 
tx_cfg->preamble_length));
}
@@ -273,7 +273,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_preamble_length(dev->spi, 0));
}
SET_CHECKED(rf69_set_sync_enable  (dev->spi, tx_cfg->enable_sync));
-   if (tx_cfg->enable_length_byte == optionOn) {
+   if (tx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -285,7 +285,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_crc_enable   (dev->spi, tx_cfg->enable_crc));
  
  	/* configure sync, if enabled */

-   if (tx_cfg->enable_sync == optionOn) {
+   if (tx_cfg->enable_sync == OPTION_ON) {
SET_CHECKED(rf69_set_sync_size(dev->spi, tx_cfg->sync_length));
SET_CHECKED(rf69_set_sync_values(dev->spi, 
tx_cfg->sync_pattern));
}
@@ -400,7 +400,7 @@ pi433_receive(void *data)
}
  
  	/* length byte enabled? */

-   if (dev->rx_cfg.enable_length_byte == optionOn)
+   if (dev->rx_cfg.enable_length_byte == OPTION_ON)
{
retval = wait_event_interruptible(dev->fifo_wait_queue,
  dev->free_in_fifo < 
FIFO_SIZE);
@@ -525,11 +525,11 @@ pi433_tx_thread(void *data)
size = tx_cfg.fixed_message_length;
  
  		/* increase size, if len byte is requested */

-   if (tx_cfg.enable_length_byte == optionOn)
+   if (tx_cfg.enable_length_byte == OPTION_ON)
size++;
  
  		/* increase size, if adr byte is requested */

-  

Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

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

Renames the enum optionOnOff and its values optionOn, optionOff to enum
option_on_off and OPTION_ON, OPTION_OFF. Fixes checkpatch.pl warnings:
"Avoid CamelCase: , , ".

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c  | 34 ++---
  drivers/staging/pi433/pi433_if.h  | 16 +++---
  drivers/staging/pi433/rf69.c  | 45 ++-
  drivers/staging/pi433/rf69.h  | 15 -
  drivers/staging/pi433/rf69_enum.h |  6 +++---
  5 files changed, 63 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b8efe6a74a2a..4f6830f369e9 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -198,7 +198,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
/* packet config */
/* enable */
SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, 
afterSyncInterrupt));
}
@@ -206,7 +206,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
}
-   if (rx_cfg->enable_length_byte == optionOn) {
+   if (rx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -220,14 +220,14 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
  
  	/* lengths */

SET_CHECKED(rf69_set_sync_size(dev->spi, rx_cfg->sync_length));
-   if (rx_cfg->enable_length_byte == optionOn)
+   if (rx_cfg->enable_length_byte == OPTION_ON)
{
SET_CHECKED(rf69_set_payload_length(dev->spi, 0xff));
}
else if (rx_cfg->fixed_message_length != 0)
{
payload_length = rx_cfg->fixed_message_length;
-   if (rx_cfg->enable_length_byte  == optionOn) payload_length++;
+   if (rx_cfg->enable_length_byte  == OPTION_ON) payload_length++;
if (rx_cfg->enable_address_filtering != filteringOff) 
payload_length++;
SET_CHECKED(rf69_set_payload_length(dev->spi, payload_length));
}
@@ -237,7 +237,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
}
  
  	/* values */

-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_sync_values(dev->spi, 
rx_cfg->sync_pattern));
}
@@ -264,7 +264,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_tx_start_condition(dev->spi, 
tx_cfg->tx_start_condition));
  
  	/* packet format enable */

-   if (tx_cfg->enable_preamble == optionOn)
+   if (tx_cfg->enable_preamble == OPTION_ON)
{
SET_CHECKED(rf69_set_preamble_length(dev->spi, 
tx_cfg->preamble_length));
}
@@ -273,7 +273,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_preamble_length(dev->spi, 0));
}
SET_CHECKED(rf69_set_sync_enable  (dev->spi, tx_cfg->enable_sync));
-   if (tx_cfg->enable_length_byte == optionOn) {
+   if (tx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -285,7 +285,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_crc_enable   (dev->spi, tx_cfg->enable_crc));
  
  	/* configure sync, if enabled */

-   if (tx_cfg->enable_sync == optionOn) {
+   if (tx_cfg->enable_sync == OPTION_ON) {
SET_CHECKED(rf69_set_sync_size(dev->spi, tx_cfg->sync_length));
SET_CHECKED(rf69_set_sync_values(dev->spi, 
tx_cfg->sync_pattern));
}
@@ -400,7 +400,7 @@ pi433_receive(void *data)
}
  
  	/* length byte enabled? */

-   if (dev->rx_cfg.enable_length_byte == optionOn)
+   if (dev->rx_cfg.enable_length_byte == OPTION_ON)
{
retval = wait_event_interruptible(dev->fifo_wait_queue,
  dev->free_in_fifo < 
FIFO_SIZE);
@@ -525,11 +525,11 @@ pi433_tx_thread(void *data)
size = tx_cfg.fixed_message_length;
  
  		/* increase size, if len byte is requested */

-   if (tx_cfg.enable_length_byte == optionOn)
+   if (tx_cfg.enable_length_byte == OPTION_ON)
size++;
  
  		/* increase size, if adr byte is requested */

-   if 

Re: [PATCH v2 09/11] staging: pi433: Combine all rf69_set_amplifier_x()

[Marcus Wolf]

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

Replaces the functions rf69_set_amplifier_1, _2, _3 with two
functions: rf69_enable_amplifier(dev, amp_mask) and
rf69_disable_amplifier(dev, amp_mask).

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c |  6 +++---
  drivers/staging/pi433/rf69.c | 46 
  drivers/staging/pi433/rf69.h |  8 ++-
  3 files changed, 9 insertions(+), 51 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 3b4170b9ba94..688d0cf00782 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1126,9 +1126,9 @@ static int pi433_probe(struct spi_device *spi)
/* setup the radio module */
SET_CHECKED(rf69_set_mode   (spi, standby));
SET_CHECKED(rf69_set_data_mode  (spi, DATAMODUL_MODE_PACKET));
-   SET_CHECKED(rf69_set_amplifier_0(spi, OPTION_ON));
-   SET_CHECKED(rf69_set_amplifier_1(spi, OPTION_OFF));
-   SET_CHECKED(rf69_set_amplifier_2(spi, OPTION_OFF));
+   SET_CHECKED(rf69_enable_amplifier(spi, MASK_PALEVEL_PA0));
+   SET_CHECKED(rf69_disable_amplifier(spi, MASK_PALEVEL_PA1));
+   SET_CHECKED(rf69_disable_amplifier(spi, MASK_PALEVEL_PA2));
SET_CHECKED(rf69_set_output_power_level (spi, 13));
SET_CHECKED(rf69_set_antenna_impedance  (spi, fiftyOhm));
  
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c

index 4c9eb97978ef..c97c65ea8acd 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -262,52 +262,14 @@ int rf69_set_frequency(struct spi_device *spi, u32 
frequency)
return 0;
  }
  
-int rf69_set_amplifier_0(struct spi_device *spi,

-enum option_on_off option_on_off)
+int rf69_enable_amplifier(struct spi_device *spi, u8 amplifier_mask)
  {
-   #ifdef DEBUG
-   dev_dbg(>dev, "set: amp #0");
-   #endif
-
-   switch (option_on_off) {
-   case OPTION_ON:  return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) | 
 MASK_PALEVEL_PA0));
-   case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & 
~MASK_PALEVEL_PA0));
-   default:
-   dev_dbg(>dev, "set: illegal input param");
-   return -EINVAL;
-   }
-}
-
-int rf69_set_amplifier_1(struct spi_device *spi,
-enum option_on_off option_on_off)
-{
-   #ifdef DEBUG
-   dev_dbg(>dev, "set: amp #1");
-   #endif
-
-   switch (option_on_off) {
-   case OPTION_ON:  return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) | 
 MASK_PALEVEL_PA1));
-   case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & 
~MASK_PALEVEL_PA1));
-   default:
-   dev_dbg(>dev, "set: illegal input param");
-   return -EINVAL;
-   }
+   return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) | amplifier_mask));
  }
  
-int rf69_set_amplifier_2(struct spi_device *spi,

-enum option_on_off option_on_off)
+int rf69_disable_amplifier(struct spi_device *spi, u8 amplifier_mask)
  {
-   #ifdef DEBUG
-   dev_dbg(>dev, "set: amp #2");
-   #endif
-
-   switch (option_on_off) {
-   case OPTION_ON:  return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) | 
 MASK_PALEVEL_PA2));
-   case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & 
~MASK_PALEVEL_PA2));
-   default:
-   dev_dbg(>dev, "set: illegal input param");
-   return -EINVAL;
-   }
+   return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & 
~amplifier_mask));
  }
  
  int rf69_set_output_power_level(struct spi_device *spi, u8 powerLevel)

diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 18296b4502f3..ba25ab6aeae7 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -33,12 +33,8 @@ int rf69_set_modulation_shaping(struct spi_device *spi, enum 
mod_shaping mod_sha
  int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate);
  int rf69_set_deviation(struct spi_device *spi, u32 deviation);
  int rf69_set_frequency(struct spi_device *spi, u32 frequency);
-int rf69_set_amplifier_0(struct spi_device *spi,
-enum option_on_off option_on_off);
-int rf69_set_amplifier_1(struct spi_device *spi,
-enum option_on_off option_on_off);
-int rf69_set_amplifier_2(struct spi_device *spi,
-enum option_on_off option_on_off);
+int rf69_enable_amplifier(struct spi_device *spi, u8 amplifier_mask);
+int rf69_disable_amplifier(struct spi_device *spi, u8 amplifier_mask);
  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 

Re: [PATCH v2 09/11] staging: pi433: Combine all rf69_set_amplifier_x()

[Marcus Wolf]

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

Replaces the functions rf69_set_amplifier_1, _2, _3 with two
functions: rf69_enable_amplifier(dev, amp_mask) and
rf69_disable_amplifier(dev, amp_mask).

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c |  6 +++---
  drivers/staging/pi433/rf69.c | 46 
  drivers/staging/pi433/rf69.h |  8 ++-
  3 files changed, 9 insertions(+), 51 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 3b4170b9ba94..688d0cf00782 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1126,9 +1126,9 @@ static int pi433_probe(struct spi_device *spi)
/* setup the radio module */
SET_CHECKED(rf69_set_mode   (spi, standby));
SET_CHECKED(rf69_set_data_mode  (spi, DATAMODUL_MODE_PACKET));
-   SET_CHECKED(rf69_set_amplifier_0(spi, OPTION_ON));
-   SET_CHECKED(rf69_set_amplifier_1(spi, OPTION_OFF));
-   SET_CHECKED(rf69_set_amplifier_2(spi, OPTION_OFF));
+   SET_CHECKED(rf69_enable_amplifier(spi, MASK_PALEVEL_PA0));
+   SET_CHECKED(rf69_disable_amplifier(spi, MASK_PALEVEL_PA1));
+   SET_CHECKED(rf69_disable_amplifier(spi, MASK_PALEVEL_PA2));
SET_CHECKED(rf69_set_output_power_level (spi, 13));
SET_CHECKED(rf69_set_antenna_impedance  (spi, fiftyOhm));
  
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c

index 4c9eb97978ef..c97c65ea8acd 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -262,52 +262,14 @@ int rf69_set_frequency(struct spi_device *spi, u32 
frequency)
return 0;
  }
  
-int rf69_set_amplifier_0(struct spi_device *spi,

-enum option_on_off option_on_off)
+int rf69_enable_amplifier(struct spi_device *spi, u8 amplifier_mask)
  {
-   #ifdef DEBUG
-   dev_dbg(>dev, "set: amp #0");
-   #endif
-
-   switch (option_on_off) {
-   case OPTION_ON:  return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) | 
 MASK_PALEVEL_PA0));
-   case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & 
~MASK_PALEVEL_PA0));
-   default:
-   dev_dbg(>dev, "set: illegal input param");
-   return -EINVAL;
-   }
-}
-
-int rf69_set_amplifier_1(struct spi_device *spi,
-enum option_on_off option_on_off)
-{
-   #ifdef DEBUG
-   dev_dbg(>dev, "set: amp #1");
-   #endif
-
-   switch (option_on_off) {
-   case OPTION_ON:  return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) | 
 MASK_PALEVEL_PA1));
-   case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & 
~MASK_PALEVEL_PA1));
-   default:
-   dev_dbg(>dev, "set: illegal input param");
-   return -EINVAL;
-   }
+   return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) | amplifier_mask));
  }
  
-int rf69_set_amplifier_2(struct spi_device *spi,

-enum option_on_off option_on_off)
+int rf69_disable_amplifier(struct spi_device *spi, u8 amplifier_mask)
  {
-   #ifdef DEBUG
-   dev_dbg(>dev, "set: amp #2");
-   #endif
-
-   switch (option_on_off) {
-   case OPTION_ON:  return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) | 
 MASK_PALEVEL_PA2));
-   case OPTION_OFF: return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & 
~MASK_PALEVEL_PA2));
-   default:
-   dev_dbg(>dev, "set: illegal input param");
-   return -EINVAL;
-   }
+   return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) & 
~amplifier_mask));
  }
  
  int rf69_set_output_power_level(struct spi_device *spi, u8 powerLevel)

diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 18296b4502f3..ba25ab6aeae7 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -33,12 +33,8 @@ int rf69_set_modulation_shaping(struct spi_device *spi, enum 
mod_shaping mod_sha
  int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate);
  int rf69_set_deviation(struct spi_device *spi, u32 deviation);
  int rf69_set_frequency(struct spi_device *spi, u32 frequency);
-int rf69_set_amplifier_0(struct spi_device *spi,
-enum option_on_off option_on_off);
-int rf69_set_amplifier_1(struct spi_device *spi,
-enum option_on_off option_on_off);
-int rf69_set_amplifier_2(struct spi_device *spi,
-enum option_on_off option_on_off);
+int rf69_enable_amplifier(struct spi_device *spi, u8 amplifier_mask);
+int rf69_disable_amplifier(struct spi_device *spi, u8 amplifier_mask);
  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);



Hi 

Re: [PATCH v2 08/11] staging: pi433: Remove enum data_mode

[Marcus Wolf]

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

Call rf69_set_data_mode with DATAMODUL_MODE value directly.

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c  |  2 +-
  drivers/staging/pi433/rf69.c  | 15 ++-
  drivers/staging/pi433/rf69.h  |  2 +-
  drivers/staging/pi433/rf69_enum.h |  6 --
  4 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index fb500d062df8..3b4170b9ba94 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1125,7 +1125,7 @@ static int pi433_probe(struct spi_device *spi)
  
  	/* setup the radio module */

SET_CHECKED(rf69_set_mode   (spi, standby));
-   SET_CHECKED(rf69_set_data_mode  (spi, packet));
+   SET_CHECKED(rf69_set_data_mode  (spi, DATAMODUL_MODE_PACKET));
SET_CHECKED(rf69_set_amplifier_0(spi, OPTION_ON));
SET_CHECKED(rf69_set_amplifier_1(spi, OPTION_OFF));
SET_CHECKED(rf69_set_amplifier_2(spi, OPTION_OFF));
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e5b29bed35ef..4c9eb97978ef 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -61,20 +61,9 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
  
  }
  
-int rf69_set_data_mode(struct spi_device *spi, enum dataMode dataMode)

+int rf69_set_data_mode(struct spi_device *spi, u8 data_mode)
  {
-   #ifdef DEBUG
-   dev_dbg(>dev, "set: data mode");
-   #endif
-
-   switch (dataMode) {
-   case packet:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-   case continuous:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-   case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | 
DATAMODUL_MODE_CONTINUOUS_NOSYNC);
-   default:
-   dev_dbg(>dev, "set: illegal input param");
-   return -EINVAL;
-   }
+   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODE) | data_mode);
  }
  
  int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)

diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 177223451c87..18296b4502f3 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -26,7 +26,7 @@
  #define FIFO_THRESHOLD15  /* in byte */
  
  int rf69_set_mode(struct spi_device *spi, enum mode mode);

-int rf69_set_data_mode(struct spi_device *spi, enum dataMode dataMode);
+int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
  int rf69_set_modulation(struct spi_device *spi, enum modulation modulation);
  enum modulation rf69_get_modulation(struct spi_device *spi);
  int rf69_set_modulation_shaping(struct spi_device *spi, enum mod_shaping 
mod_shaping);
diff --git a/drivers/staging/pi433/rf69_enum.h 
b/drivers/staging/pi433/rf69_enum.h
index 97f615effca3..b0715b4eb6ac 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -31,12 +31,6 @@ enum mode {
receive
  };
  
-enum dataMode {

-   packet,
-   continuous,
-   continuousNoSync
-};
-
  enum modulation {
OOK,
FSK



Hi Simon,

this change is "closing a door".

The rf69 is able to work in an advanced packet mode or in a simple 
continuous mode.


The driver so far is using the advanced packet mode, only. But if it 
will support continuous mode some day, it will be necessary to configure 
this.


The open source projects fhem and domoticz already asked for such a 
change, since for their architecture, they'll need a pi433 working in 
continuous mode. But so far I am not planning to implement such a 
functionality.


Since the rule for kernel development seems to be, not to care about 
future, most probably you patch is fine, anyway.


Marcus

Re: [PATCH v2 08/11] staging: pi433: Remove enum data_mode

[Marcus Wolf]

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

Call rf69_set_data_mode with DATAMODUL_MODE value directly.

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c  |  2 +-
  drivers/staging/pi433/rf69.c  | 15 ++-
  drivers/staging/pi433/rf69.h  |  2 +-
  drivers/staging/pi433/rf69_enum.h |  6 --
  4 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index fb500d062df8..3b4170b9ba94 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1125,7 +1125,7 @@ static int pi433_probe(struct spi_device *spi)
  
  	/* setup the radio module */

SET_CHECKED(rf69_set_mode   (spi, standby));
-   SET_CHECKED(rf69_set_data_mode  (spi, packet));
+   SET_CHECKED(rf69_set_data_mode  (spi, DATAMODUL_MODE_PACKET));
SET_CHECKED(rf69_set_amplifier_0(spi, OPTION_ON));
SET_CHECKED(rf69_set_amplifier_1(spi, OPTION_OFF));
SET_CHECKED(rf69_set_amplifier_2(spi, OPTION_OFF));
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e5b29bed35ef..4c9eb97978ef 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -61,20 +61,9 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
  
  }
  
-int rf69_set_data_mode(struct spi_device *spi, enum dataMode dataMode)

+int rf69_set_data_mode(struct spi_device *spi, u8 data_mode)
  {
-   #ifdef DEBUG
-   dev_dbg(>dev, "set: data mode");
-   #endif
-
-   switch (dataMode) {
-   case packet:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-   case continuous:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-   case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | 
DATAMODUL_MODE_CONTINUOUS_NOSYNC);
-   default:
-   dev_dbg(>dev, "set: illegal input param");
-   return -EINVAL;
-   }
+   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODE) | data_mode);
  }
  
  int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)

diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 177223451c87..18296b4502f3 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -26,7 +26,7 @@
  #define FIFO_THRESHOLD15  /* in byte */
  
  int rf69_set_mode(struct spi_device *spi, enum mode mode);

-int rf69_set_data_mode(struct spi_device *spi, enum dataMode dataMode);
+int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
  int rf69_set_modulation(struct spi_device *spi, enum modulation modulation);
  enum modulation rf69_get_modulation(struct spi_device *spi);
  int rf69_set_modulation_shaping(struct spi_device *spi, enum mod_shaping 
mod_shaping);
diff --git a/drivers/staging/pi433/rf69_enum.h 
b/drivers/staging/pi433/rf69_enum.h
index 97f615effca3..b0715b4eb6ac 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -31,12 +31,6 @@ enum mode {
receive
  };
  
-enum dataMode {

-   packet,
-   continuous,
-   continuousNoSync
-};
-
  enum modulation {
OOK,
FSK



Hi Simon,

this change is "closing a door".

The rf69 is able to work in an advanced packet mode or in a simple 
continuous mode.


The driver so far is using the advanced packet mode, only. But if it 
will support continuous mode some day, it will be necessary to configure 
this.


The open source projects fhem and domoticz already asked for such a 
change, since for their architecture, they'll need a pi433 working in 
continuous mode. But so far I am not planning to implement such a 
functionality.


Since the rule for kernel development seems to be, not to care about 
future, most probably you patch is fine, anyway.


Marcus

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

[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

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

[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

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 05.12.2017 um 13:16 schrieb Dan Carpenter:
> On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
>> Keep in mind, that if you split the functions, in the interface
>> implementation you also need more code:
>>
>> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
>>
>> will have to be converted in something like
>>
>> if (rx_cfg->enable_sync)
>>  SET_CHECKED(rf69_set_sync_enbable(dev->spi);
>> else
>>  SET_CHECKED(rf69_set_sync_disable(dev->spi);
>>
> 
> Here's what the code looks like right now:
> 
>198  /* packet config */
>199  /* enable */
>200  SET_CHECKED(rf69_set_sync_enable(dev->spi, 
> rx_cfg->enable_sync));
>201  if (rx_cfg->enable_sync == optionOn)
>202  {
>203  SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, 
> afterSyncInterrupt));
>204  }
>205  else
>206  {
>207  SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, 
> always));
>208  }
> 
> That's for the rx_cfg.  We have related but different code for the
> tx_cfg.  It's strange to me that we can enable sync for rx and not for
> tx...  How does that work when the setting ends up getting stored in the
> same register?
> 
> The new code would look like this:
> 
>   if (rx_cfg->enable_sync) {
>   ret = rf69_enable_sync(spi);
  ^
>   if (ret)
>   return ret;
>   ret = rf69_set_fifo_fill_condition(dev->spi, 
> afterSyncInterrupt);
>   if (ret)
>   return ret;
>   } else {
>   ret = rf69_disable_sync(dev->spi);
>   if (ret)
>   return ret;
>   ret = rf69_set_fifo_fill_condition(dev->spi, always);
>   if (ret)
>   return ret;
>   }
> 
> It's not the greatest, but it's not the worst...  The configuration for
> ->enable_sync is a bit spread out and it might be nice to move it all to
> one function?
> 
> I liked Simon's naming scheme and I thought it was clear what the
> rf69_set_sync(spi, false) function would do.
  ^
> 
> Simon, it seems like Marcus and I both are Ok with your style choices.
> Do whatever seems best when you implement the code.  If it's awkward to
> break up the functions then don't.
> 
> regards,
> dan carpenter
> 

Hi Dan,

now I am a bit confused.

My favourit:

rf69_set_sync_enable(spi, false)
rf69_set_amp_enable(spi, false)
rf69_set_crc_enable(spi, false)

I prefer to keep the enable (or comparable), because it shows, what the
function is doing. For sync, for example, there are several setter:
size, tolerance, values ... AND enable (or comparable).

All functions in rf69.c should start with rf69_get or rf69_set.


Simons proposal:

rf69_set_sync_enable(spi)
rf69_set_sync_disable(spi)
rf69_set_amp_enable(spi)
rf69_set_amp_disable(spi)
rf69_set_crc_enable(spi)
rf69_set_crc_disable(spi)

I don't like that, because it's blowing up the code without bringing
extra feature (see my last mail).


In your code example, you use Simons proposal, but in your explaination
below, you show the original style - although you refer to Simons sheme...


Cheers,

Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 05.12.2017 um 13:16 schrieb Dan Carpenter:
> On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
>> Keep in mind, that if you split the functions, in the interface
>> implementation you also need more code:
>>
>> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
>>
>> will have to be converted in something like
>>
>> if (rx_cfg->enable_sync)
>>  SET_CHECKED(rf69_set_sync_enbable(dev->spi);
>> else
>>  SET_CHECKED(rf69_set_sync_disable(dev->spi);
>>
> 
> Here's what the code looks like right now:
> 
>198  /* packet config */
>199  /* enable */
>200  SET_CHECKED(rf69_set_sync_enable(dev->spi, 
> rx_cfg->enable_sync));
>201  if (rx_cfg->enable_sync == optionOn)
>202  {
>203  SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, 
> afterSyncInterrupt));
>204  }
>205  else
>206  {
>207  SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, 
> always));
>208  }
> 
> That's for the rx_cfg.  We have related but different code for the
> tx_cfg.  It's strange to me that we can enable sync for rx and not for
> tx...  How does that work when the setting ends up getting stored in the
> same register?
> 
> The new code would look like this:
> 
>   if (rx_cfg->enable_sync) {
>   ret = rf69_enable_sync(spi);
  ^
>   if (ret)
>   return ret;
>   ret = rf69_set_fifo_fill_condition(dev->spi, 
> afterSyncInterrupt);
>   if (ret)
>   return ret;
>   } else {
>   ret = rf69_disable_sync(dev->spi);
>   if (ret)
>   return ret;
>   ret = rf69_set_fifo_fill_condition(dev->spi, always);
>   if (ret)
>   return ret;
>   }
> 
> It's not the greatest, but it's not the worst...  The configuration for
> ->enable_sync is a bit spread out and it might be nice to move it all to
> one function?
> 
> I liked Simon's naming scheme and I thought it was clear what the
> rf69_set_sync(spi, false) function would do.
  ^
> 
> Simon, it seems like Marcus and I both are Ok with your style choices.
> Do whatever seems best when you implement the code.  If it's awkward to
> break up the functions then don't.
> 
> regards,
> dan carpenter
> 

Hi Dan,

now I am a bit confused.

My favourit:

rf69_set_sync_enable(spi, false)
rf69_set_amp_enable(spi, false)
rf69_set_crc_enable(spi, false)

I prefer to keep the enable (or comparable), because it shows, what the
function is doing. For sync, for example, there are several setter:
size, tolerance, values ... AND enable (or comparable).

All functions in rf69.c should start with rf69_get or rf69_set.


Simons proposal:

rf69_set_sync_enable(spi)
rf69_set_sync_disable(spi)
rf69_set_amp_enable(spi)
rf69_set_amp_disable(spi)
rf69_set_crc_enable(spi)
rf69_set_crc_disable(spi)

I don't like that, because it's blowing up the code without bringing
extra feature (see my last mail).


In your code example, you use Simons proposal, but in your explaination
below, you show the original style - although you refer to Simons sheme...


Cheers,

Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:05 schrieb Simon Sandström:
> On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
>>
>> Hi Simon, hi Dan,
>>
>> if you both are of the same opinion, for me, it's fine, if we go with two
>> functions.
>>
>> But I don't get the advantage, if we split approx. 10 functions, to get rid
>> of enum optionOnOff.
>>
>> Keep in mind, that if you split the functions, in the interface
>> implementation you also need more code:
>>
>> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
>>
>> will have to be converted in something like
>>
>> if (rx_cfg->enable_sync)
>>  SET_CHECKED(rf69_set_sync_enbable(dev->spi);
>> else
>>  SET_CHECKED(rf69_set_sync_disable(dev->spi);
> 
> I think that this makes the code very clear. If the config tells us to
> enable the sync then we'll enabled it, otherwise we'll disable it.
> 

Hi Simon,

I thought about it a while.

Yes, you are right, it makes it very clear - but doesn't tell a lot
extra. The only thing, you can see extra, is, that there is the option,
to turn on and to turn off. You even can't see, whether it is turend on
or off.
The info, that there is an option of en- or disabling - at least from my
side - is visible, too, if there is just one function that takes a bool
or optionOnOff as argument.

When you 'll redesign every setter, that now deals with optionOnOff in
that way, you will introduce something like 50 extra lines of code
without bringing any extra functionality to the driver.
>From my point of view, that's bad: The longer the text, the harder to
understand everything entierly, the more chances for bugs and a lot
harder to service.

So from my point of view such a redesign is nice for optics but bad for
further development. I would really prefer a solution, like Marcin
Ciupak offered. He is not blowing up the code, but even tries to reduce
the calls to READ_REG and WRITE_REG. That increases readbility, too, but
also reduces code size and chances for bugs.

But regardless of the arguments above, I don't mind. If it's a benefit
for you and lot of others in the community, I won't blame you for such a
change.
If you would add 50 lines of code with new funcitonality (e. g. support
the AES feature of the module), I would be a lot happier - and most
useres for sure, too.

Cheers,

Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:05 schrieb Simon Sandström:
> On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
>>
>> Hi Simon, hi Dan,
>>
>> if you both are of the same opinion, for me, it's fine, if we go with two
>> functions.
>>
>> But I don't get the advantage, if we split approx. 10 functions, to get rid
>> of enum optionOnOff.
>>
>> Keep in mind, that if you split the functions, in the interface
>> implementation you also need more code:
>>
>> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
>>
>> will have to be converted in something like
>>
>> if (rx_cfg->enable_sync)
>>  SET_CHECKED(rf69_set_sync_enbable(dev->spi);
>> else
>>  SET_CHECKED(rf69_set_sync_disable(dev->spi);
> 
> I think that this makes the code very clear. If the config tells us to
> enable the sync then we'll enabled it, otherwise we'll disable it.
> 

Hi Simon,

I thought about it a while.

Yes, you are right, it makes it very clear - but doesn't tell a lot
extra. The only thing, you can see extra, is, that there is the option,
to turn on and to turn off. You even can't see, whether it is turend on
or off.
The info, that there is an option of en- or disabling - at least from my
side - is visible, too, if there is just one function that takes a bool
or optionOnOff as argument.

When you 'll redesign every setter, that now deals with optionOnOff in
that way, you will introduce something like 50 extra lines of code
without bringing any extra functionality to the driver.
>From my point of view, that's bad: The longer the text, the harder to
understand everything entierly, the more chances for bugs and a lot
harder to service.

So from my point of view such a redesign is nice for optics but bad for
further development. I would really prefer a solution, like Marcin
Ciupak offered. He is not blowing up the code, but even tries to reduce
the calls to READ_REG and WRITE_REG. That increases readbility, too, but
also reduces code size and chances for bugs.

But regardless of the arguments above, I don't mind. If it's a benefit
for you and lot of others in the community, I won't blame you for such a
change.
If you would add 50 lines of code with new funcitonality (e. g. support
the AES feature of the module), I would be a lot happier - and most
useres for sure, too.

Cheers,

Marcus

[PATCH V3] staging: pi433: rf69.c: Replace macros READ_REG and WRITE_REG with smarter functions

[Marcus Wolf]

To increase the readability of the register accesses, the abstraction
of the helpers was increased from simple read and write to set bit,
clear bit and read modify write bit.

Annotation: This patch contains a lot of long lines and camel case
var names. These long lines and camel case vars weren't introduced
by this patch, but were long and camel cased before.

Signed-off-by: Marcus Wolf <li...@wolf-entwicklungen.de>
---
 drivers/staging/pi433/rf69.c |  336 ++
 1 file changed, 180 insertions(+), 156 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a215..4f9878f 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -33,8 +33,32 @@
 
 /*-*/
 
-#define READ_REG(x)rf69_read_reg (spi, x)
-#define WRITE_REG(x, y)rf69_write_reg(spi, x, y)
+static int rf69_set_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = tmp | mask;
+   return rf69_write_reg(spi, reg, tmp);
+}
+
+static int rf69_clear_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = tmp & ~mask;
+   return rf69_write_reg(spi, reg, tmp);
+}
+
+static inline int rf69_read_mod_write(struct spi_device *spi, u8 reg, u8 mask, 
u8 value)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = (tmp & ~mask) | value;
+   return rf69_write_reg(spi, reg, tmp);
+}
 
 /*-*/
 
@@ -45,11 +69,11 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
#endif
 
switch (mode) {
-   case transmit:return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_TRANSMIT);
-   case receive: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_RECEIVE);
-   case synthesizer: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SYNTHESIZER);
-   case standby: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_STANDBY);
-   case mode_sleep:  return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SLEEP);
+   case transmit:return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_TRANSMIT);
+   case receive: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_RECEIVE);
+   case synthesizer: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SYNTHESIZER);
+   case standby: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_STANDBY);
+   case mode_sleep:  return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SLEEP);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -68,9 +92,9 @@ int rf69_set_data_mode(struct spi_device *spi, enum dataMode 
dataMode)
#endif
 
switch (dataMode) {
-   case packet:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-   case continuous:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-   case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | 
DATAMODUL_MODE_CONTINUOUS_NOSYNC);
+   case packet:return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_PACKET);
+   case continuous:return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS);
+   case continuousNoSync:  return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS_NOSYNC);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -84,8 +108,8 @@ int rf69_set_modulation(struct spi_device *spi, enum 
modulation modulation)
#endif
 
switch (modulation) {
-   case OOK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_FSK);
+   case OOK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
+   case FSK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
default:
dev_dbg(>dev, "set: il

[PATCH V3] staging: pi433: rf69.c: Replace macros READ_REG and WRITE_REG with smarter functions

[Marcus Wolf]

To increase the readability of the register accesses, the abstraction
of the helpers was increased from simple read and write to set bit,
clear bit and read modify write bit.

Annotation: This patch contains a lot of long lines and camel case
var names. These long lines and camel case vars weren't introduced
by this patch, but were long and camel cased before.

Signed-off-by: Marcus Wolf 
---
 drivers/staging/pi433/rf69.c |  336 ++
 1 file changed, 180 insertions(+), 156 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a215..4f9878f 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -33,8 +33,32 @@
 
 /*-*/
 
-#define READ_REG(x)rf69_read_reg (spi, x)
-#define WRITE_REG(x, y)rf69_write_reg(spi, x, y)
+static int rf69_set_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = tmp | mask;
+   return rf69_write_reg(spi, reg, tmp);
+}
+
+static int rf69_clear_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = tmp & ~mask;
+   return rf69_write_reg(spi, reg, tmp);
+}
+
+static inline int rf69_read_mod_write(struct spi_device *spi, u8 reg, u8 mask, 
u8 value)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = (tmp & ~mask) | value;
+   return rf69_write_reg(spi, reg, tmp);
+}
 
 /*-*/
 
@@ -45,11 +69,11 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
#endif
 
switch (mode) {
-   case transmit:return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_TRANSMIT);
-   case receive: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_RECEIVE);
-   case synthesizer: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SYNTHESIZER);
-   case standby: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_STANDBY);
-   case mode_sleep:  return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SLEEP);
+   case transmit:return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_TRANSMIT);
+   case receive: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_RECEIVE);
+   case synthesizer: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SYNTHESIZER);
+   case standby: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_STANDBY);
+   case mode_sleep:  return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SLEEP);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -68,9 +92,9 @@ int rf69_set_data_mode(struct spi_device *spi, enum dataMode 
dataMode)
#endif
 
switch (dataMode) {
-   case packet:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-   case continuous:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-   case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | 
DATAMODUL_MODE_CONTINUOUS_NOSYNC);
+   case packet:return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_PACKET);
+   case continuous:return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS);
+   case continuousNoSync:  return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS_NOSYNC);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -84,8 +108,8 @@ int rf69_set_modulation(struct spi_device *spi, enum 
modulation modulation)
#endif
 
switch (modulation) {
-   case OOK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_FSK);
+   case OOK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
+   case FSK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
default:
dev_dbg(>dev, "set: illegal input param");
return 

[PATCH V2] staging: pi433: rf69.c: Replace macros READ_REG and WRITE_REG with inline functions rf69_set_bit, rf69_reset_bit and rf69_read_modify_write

[Marcus Wolf]

To increase the readability of the register accesses, the abstraction
of the helpers was increased from simple read and write to set bit,
clear bit and read modify write bit.

Annotation: This patch contains a lot of long lines and camel case
var names. These long lines and camel case vars weren't introduced
by this patch, but were long and camel cased before.

Signed-off-by: Marcus Wolf <li...@wolf-entwicklungen.de>
---
 drivers/staging/pi433/rf69.c |  336 ++
 1 file changed, 180 insertions(+), 156 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a215..4f9878f 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -33,8 +33,32 @@
 
 /*-*/
 
-#define READ_REG(x)rf69_read_reg (spi, x)
-#define WRITE_REG(x, y)rf69_write_reg(spi, x, y)
+static int rf69_set_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = tmp | mask;
+   return rf69_write_reg(spi, reg, tmp);
+}
+
+static int rf69_clear_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = tmp & ~mask;
+   return rf69_write_reg(spi, reg, tmp);
+}
+
+static inline int rf69_read_mod_write(struct spi_device *spi, u8 reg, u8 mask, 
u8 value)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = (tmp & ~mask) | value;
+   return rf69_write_reg(spi, reg, tmp);
+}
 
 /*-*/
 
@@ -45,11 +69,11 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
#endif
 
switch (mode) {
-   case transmit:return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_TRANSMIT);
-   case receive: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_RECEIVE);
-   case synthesizer: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SYNTHESIZER);
-   case standby: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_STANDBY);
-   case mode_sleep:  return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SLEEP);
+   case transmit:return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_TRANSMIT);
+   case receive: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_RECEIVE);
+   case synthesizer: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SYNTHESIZER);
+   case standby: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_STANDBY);
+   case mode_sleep:  return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SLEEP);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -68,9 +92,9 @@ int rf69_set_data_mode(struct spi_device *spi, enum dataMode 
dataMode)
#endif
 
switch (dataMode) {
-   case packet:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-   case continuous:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-   case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | 
DATAMODUL_MODE_CONTINUOUS_NOSYNC);
+   case packet:return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_PACKET);
+   case continuous:return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS);
+   case continuousNoSync:  return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS_NOSYNC);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -84,8 +108,8 @@ int rf69_set_modulation(struct spi_device *spi, enum 
modulation modulation)
#endif
 
switch (modulation) {
-   case OOK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_FSK);
+   case OOK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
+   case FSK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
default:
dev_dbg(>dev, "set: il

[PATCH V2] staging: pi433: rf69.c: Replace macros READ_REG and WRITE_REG with inline functions rf69_set_bit, rf69_reset_bit and rf69_read_modify_write

[Marcus Wolf]

To increase the readability of the register accesses, the abstraction
of the helpers was increased from simple read and write to set bit,
clear bit and read modify write bit.

Annotation: This patch contains a lot of long lines and camel case
var names. These long lines and camel case vars weren't introduced
by this patch, but were long and camel cased before.

Signed-off-by: Marcus Wolf 
---
 drivers/staging/pi433/rf69.c |  336 ++
 1 file changed, 180 insertions(+), 156 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a215..4f9878f 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -33,8 +33,32 @@
 
 /*-*/
 
-#define READ_REG(x)rf69_read_reg (spi, x)
-#define WRITE_REG(x, y)rf69_write_reg(spi, x, y)
+static int rf69_set_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = tmp | mask;
+   return rf69_write_reg(spi, reg, tmp);
+}
+
+static int rf69_clear_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = tmp & ~mask;
+   return rf69_write_reg(spi, reg, tmp);
+}
+
+static inline int rf69_read_mod_write(struct spi_device *spi, u8 reg, u8 mask, 
u8 value)
+{
+   u8 tmp;
+
+   tmp = rf69_read_reg(spi, reg);
+   tmp = (tmp & ~mask) | value;
+   return rf69_write_reg(spi, reg, tmp);
+}
 
 /*-*/
 
@@ -45,11 +69,11 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
#endif
 
switch (mode) {
-   case transmit:return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_TRANSMIT);
-   case receive: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_RECEIVE);
-   case synthesizer: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SYNTHESIZER);
-   case standby: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_STANDBY);
-   case mode_sleep:  return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SLEEP);
+   case transmit:return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_TRANSMIT);
+   case receive: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_RECEIVE);
+   case synthesizer: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SYNTHESIZER);
+   case standby: return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_STANDBY);
+   case mode_sleep:  return rf69_read_mod_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SLEEP);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -68,9 +92,9 @@ int rf69_set_data_mode(struct spi_device *spi, enum dataMode 
dataMode)
#endif
 
switch (dataMode) {
-   case packet:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-   case continuous:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-   case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | 
DATAMODUL_MODE_CONTINUOUS_NOSYNC);
+   case packet:return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_PACKET);
+   case continuous:return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS);
+   case continuousNoSync:  return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS_NOSYNC);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -84,8 +108,8 @@ int rf69_set_modulation(struct spi_device *spi, enum 
modulation modulation)
#endif
 
switch (modulation) {
-   case OOK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_FSK);
+   case OOK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
+   case FSK: return rf69_read_mod_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
default:
dev_dbg(>dev, "set: illegal input param");
return 

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:56 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 09:31:06PM +0200, Marcus Wolf wrote:

Then it might be, that DATAMODUL_MODE_PACKET might need an other value.


That's future code so we can delete that sentence for now.


With the rule above, you are absolutely right. But we now spend time, to
remove an currently non necessary feature ("double layer"), which will take
time to re-introduce as soon, as someone wants to support a second chip.
Isn't that double-work and a thus a pitty?



It is what it is...  In the kernel we insist all code have a user right
now when it's merged.  Unused code or future code is deleted.  We hate
abstraction layers.  Everyone argues that their abstraction layer is
different and good but kernel devs instinctively hate abstraction.

To be honest, in the kernel we do do a lot of work twice.  I made people
redo 9 quite large patches for this pi4333 driver today.  And they're
probably going end up conflicting and have to be redone again...  :/
That does suck.  I don't know what to do about it.

In my view it helps that people sending patches don't ever have to worry
about future code and we can focus on what exists now.  Greg will never
reject code for "future reasons" unless the future is almost right away
like tomorrow or maybe the next day.

regards,
dan carpenter



Hi Dan,

I am self employed and controling two small companies. For me it is very 
important to do efficient work - otherwise the 24 hours of a day are too 
short to get my work done, even if I include the night.


The goal of most projects (my own, as well as my customers) is very 
clear, but normaly you are not able to reach it in one pass. Therefore 
projects are split up in parts and try to release parts, to be on market 
earlier.
No one would accept, if I would optimise a software for a current 
release in a way, that I close doors for the final goal.


So I agree: We can't change the rules and have to take them as they are.

But if I read your lines, it's shaking me. I observed this sending the 
patch over and over again and it realy bugs me. Not beacause it might be 
boring, mainly because for me it feels like a huge waste of time - time 
I simply don't have.
Same applies to removing stuff, when I already now, (at least for my 
products) I will need it in future.


Maybe controlling a straup, developing fancy new products, the market 
likes to have (and in a time, the market is accepting you to present 
them) and contributing to the kernel need that different kind of mind 
set, that it's dam hard to do both at the same time.


Cheers,
Marcus

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:56 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 09:31:06PM +0200, Marcus Wolf wrote:

Then it might be, that DATAMODUL_MODE_PACKET might need an other value.


That's future code so we can delete that sentence for now.


With the rule above, you are absolutely right. But we now spend time, to
remove an currently non necessary feature ("double layer"), which will take
time to re-introduce as soon, as someone wants to support a second chip.
Isn't that double-work and a thus a pitty?



It is what it is...  In the kernel we insist all code have a user right
now when it's merged.  Unused code or future code is deleted.  We hate
abstraction layers.  Everyone argues that their abstraction layer is
different and good but kernel devs instinctively hate abstraction.

To be honest, in the kernel we do do a lot of work twice.  I made people
redo 9 quite large patches for this pi4333 driver today.  And they're
probably going end up conflicting and have to be redone again...  :/
That does suck.  I don't know what to do about it.

In my view it helps that people sending patches don't ever have to worry
about future code and we can focus on what exists now.  Greg will never
reject code for "future reasons" unless the future is almost right away
like tomorrow or maybe the next day.

regards,
dan carpenter



Hi Dan,

I am self employed and controling two small companies. For me it is very 
important to do efficient work - otherwise the 24 hours of a day are too 
short to get my work done, even if I include the night.


The goal of most projects (my own, as well as my customers) is very 
clear, but normaly you are not able to reach it in one pass. Therefore 
projects are split up in parts and try to release parts, to be on market 
earlier.
No one would accept, if I would optimise a software for a current 
release in a way, that I close doors for the final goal.


So I agree: We can't change the rules and have to take them as they are.

But if I read your lines, it's shaking me. I observed this sending the 
patch over and over again and it realy bugs me. Not beacause it might be 
boring, mainly because for me it feels like a huge waste of time - time 
I simply don't have.
Same applies to removing stuff, when I already now, (at least for my 
products) I will need it in future.


Maybe controlling a straup, developing fancy new products, the market 
likes to have (and in a time, the market is accepting you to present 
them) and contributing to the kernel need that different kind of mind 
set, that it's dam hard to do both at the same time.


Cheers,
Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:42 schrieb Simon Sandström:

On Mon, Dec 04, 2017 at 09:22:06PM +0200, Marcus Wolf wrote:



Am 04.12.2017 um 21:15 schrieb Dan Carpenter:


That's a bad name, because it doesn't just enable it also disables.
Please split them.

regards,
dan carpenter




Same applies to all other stuff, that's using optionOnOff:
rf69_set_sync_enable(optionOn/Off) enables and disbales sync,
rf69_set_crc_enable(optionOn/Off) enables and disables crc,
...

In my opinion, if we want perfect clarity, we should stay with optionOnOff.
If we are ok, if rf69_set_sync_enable(false) disables sync,
in my opinion, we also have to be ok, if rf69_set_amp_X_enable(false)
disables the amp.

Cheers,
Marcus


Hi,

I agree with Dan. rf69_enable_sync() / rf69_disable_sync() is clear. If
there are more functions like this (e.g. for crc) then we'll just split
those functions as well.

If you really want one single function for enabling/disabling then I
think that you need to find a better name. Something like
rf69_set_sync_operation(bool), rf69_set_crc_operation(bool), etc.


Regards,
Simon



Hi Simon, hi Dan,

if you both are of the same opinion, for me, it's fine, if we go with 
two functions.


But I don't get the advantage, if we split approx. 10 functions, to get 
rid of enum optionOnOff.


Keep in mind, that if you split the functions, in the interface 
implementation you also need more code:


SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));

will have to be converted in something like

if (rx_cfg->enable_sync)
SET_CHECKED(rf69_set_sync_enbable(dev->spi);
else
SET_CHECKED(rf69_set_sync_disable(dev->spi);


For me, it is important, that the configuration, you'll have to write in 
the user space program (aka fill out the config struct) will be 100% 
non-ambigious and easy to read.


Cheers,
Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:42 schrieb Simon Sandström:

On Mon, Dec 04, 2017 at 09:22:06PM +0200, Marcus Wolf wrote:



Am 04.12.2017 um 21:15 schrieb Dan Carpenter:


That's a bad name, because it doesn't just enable it also disables.
Please split them.

regards,
dan carpenter




Same applies to all other stuff, that's using optionOnOff:
rf69_set_sync_enable(optionOn/Off) enables and disbales sync,
rf69_set_crc_enable(optionOn/Off) enables and disables crc,
...

In my opinion, if we want perfect clarity, we should stay with optionOnOff.
If we are ok, if rf69_set_sync_enable(false) disables sync,
in my opinion, we also have to be ok, if rf69_set_amp_X_enable(false)
disables the amp.

Cheers,
Marcus


Hi,

I agree with Dan. rf69_enable_sync() / rf69_disable_sync() is clear. If
there are more functions like this (e.g. for crc) then we'll just split
those functions as well.

If you really want one single function for enabling/disabling then I
think that you need to find a better name. Something like
rf69_set_sync_operation(bool), rf69_set_crc_operation(bool), etc.


Regards,
Simon



Hi Simon, hi Dan,

if you both are of the same opinion, for me, it's fine, if we go with 
two functions.


But I don't get the advantage, if we split approx. 10 functions, to get 
rid of enum optionOnOff.


Keep in mind, that if you split the functions, in the interface 
implementation you also need more code:


SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));

will have to be converted in something like

if (rx_cfg->enable_sync)
SET_CHECKED(rf69_set_sync_enbable(dev->spi);
else
SET_CHECKED(rf69_set_sync_disable(dev->spi);


For me, it is important, that the configuration, you'll have to write in 
the user space program (aka fill out the config struct) will be 100% 
non-ambigious and easy to read.


Cheers,
Marcus

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:18 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 08:59:35PM +0200, Marcus Wolf wrote:


Am 04.12.2017 um 12:33 schrieb Dan Carpenter:

On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:

diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 34ff0d4807bd..bcfe29840889 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -63,7 +63,7 @@ struct pi433_tx_cfg {
__u16   bit_rate;
__u32   dev_frequency;
enum modulation modulation;
-   enum modShaping modShaping;
+   enum mod_shapingmod_shaping;


I looked at how mod_shaping is set and the only place is in the ioctl:

 789  case PI433_IOC_WR_TX_CFG:
 790  if (copy_from_user(>tx_cfg, argp,
 791  sizeof(struct pi433_tx_cfg)))
 792  return -EFAULT;
 793  break;

We just write over the whole config.  Including important things like
rx_cfg.fixed_message_length.  There is no locking so when we do things
like:

 385  /* fixed or unlimited length? */
 386  if (dev->rx_cfg.fixed_message_length != 0)
 387  {
 388  if (dev->rx_cfg.fixed_message_length > 
dev->rx_buffer_size)
  
^^
check

 389  {
 390  retval = -1;
 391  goto abort;
 392  }
 393  bytes_total = dev->rx_cfg.fixed_message_length;
^
set this in the ioctl after the check but before this line and it looks
like a security problem.

 394  dev_dbg(dev->dev,"rx: msg len set to %d by fixed 
length", bytes_total);
 395  }

Anyway, I guess this patch is fine.

regards,
dan carpenter



Hi Dan,

you are mixing rx and tx. The part from IOCTL is copied from the tx-part,
the lower part is dealing with rx.

With rx there should be no problem, since IOCTL is blocked, as long as an rx
operation is going on.

With tx, I also expect no problems, since instance->tx_cfg is never used to
configure the rf69. Everytime, you pass in new data via write() a copy of
tx_cfg is made. Transmission is done, using the copy of the tx_cfg, never by
using instance->tx_cfg.

But maybe I didn't got your point and misunderstand your intention.



No.  You're right.  I mixed up rx and tx.  But the ioctl interface still
seems really horrible.  We generally frown on adding new ioctls at all,
but in this case to just write over the whole struct with no locking
seems really bad.

regards,
dan carpenter



In principle, you are right. But that's even a more macroscopic problem.

If someone sets a config, he needs for a telegram, and someone else 
comes in with another config, before the first one could fire his write,

shit will happen.
Same on RX-side: First one sets his config for receiving and will not 
get, what he wants, if someone else sets an other config, before he can 
fire his read.


If noone changes the config, until read() or write() was called, we are 
out of danger - even concerning the risk you mentioned.


An option to avoid that, could be, that every write and read transaction 
needs to pass in a complete config struct.
There were reasons, not to do so, but we could think of implementing it 
that was.


Are there other options for configuration, despite IOCTL?

Cheers,

Marcus
Cheers,

Marcus

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:18 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 08:59:35PM +0200, Marcus Wolf wrote:


Am 04.12.2017 um 12:33 schrieb Dan Carpenter:

On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:

diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 34ff0d4807bd..bcfe29840889 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -63,7 +63,7 @@ struct pi433_tx_cfg {
__u16   bit_rate;
__u32   dev_frequency;
enum modulation modulation;
-   enum modShaping modShaping;
+   enum mod_shapingmod_shaping;


I looked at how mod_shaping is set and the only place is in the ioctl:

 789  case PI433_IOC_WR_TX_CFG:
 790  if (copy_from_user(>tx_cfg, argp,
 791  sizeof(struct pi433_tx_cfg)))
 792  return -EFAULT;
 793  break;

We just write over the whole config.  Including important things like
rx_cfg.fixed_message_length.  There is no locking so when we do things
like:

 385  /* fixed or unlimited length? */
 386  if (dev->rx_cfg.fixed_message_length != 0)
 387  {
 388  if (dev->rx_cfg.fixed_message_length > 
dev->rx_buffer_size)
  
^^
check

 389  {
 390  retval = -1;
 391  goto abort;
 392  }
 393  bytes_total = dev->rx_cfg.fixed_message_length;
^
set this in the ioctl after the check but before this line and it looks
like a security problem.

 394  dev_dbg(dev->dev,"rx: msg len set to %d by fixed 
length", bytes_total);
 395  }

Anyway, I guess this patch is fine.

regards,
dan carpenter



Hi Dan,

you are mixing rx and tx. The part from IOCTL is copied from the tx-part,
the lower part is dealing with rx.

With rx there should be no problem, since IOCTL is blocked, as long as an rx
operation is going on.

With tx, I also expect no problems, since instance->tx_cfg is never used to
configure the rf69. Everytime, you pass in new data via write() a copy of
tx_cfg is made. Transmission is done, using the copy of the tx_cfg, never by
using instance->tx_cfg.

But maybe I didn't got your point and misunderstand your intention.



No.  You're right.  I mixed up rx and tx.  But the ioctl interface still
seems really horrible.  We generally frown on adding new ioctls at all,
but in this case to just write over the whole struct with no locking
seems really bad.

regards,
dan carpenter



In principle, you are right. But that's even a more macroscopic problem.

If someone sets a config, he needs for a telegram, and someone else 
comes in with another config, before the first one could fire his write,

shit will happen.
Same on RX-side: First one sets his config for receiving and will not 
get, what he wants, if someone else sets an other config, before he can 
fire his read.


If noone changes the config, until read() or write() was called, we are 
out of danger - even concerning the risk you mentioned.


An option to avoid that, could be, that every write and read transaction 
needs to pass in a complete config struct.
There were reasons, not to do so, but we could think of implementing it 
that was.


Are there other options for configuration, despite IOCTL?

Cheers,

Marcus
Cheers,

Marcus

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Marcus Wolf]

Second there might be the idea of supporting different chips in the future
(I already thought about).


Linux style is never to write code for the future.


Ok. I didn't know.
To be honest, I already started writing code, also supporting the rf12 
some time ago, thus programming a rfxx.c, but never finished, due to 
lack of time.

For getting stuff started, I need to focus on rf69 and pi433.

A few monthes ago, Hope RF (the producer of those chips) proposed me a 
new chip (can't remember the number - maybe 95), that also supports 
loraWan. Seems like there will be even more interesting chips coming up, 
that could be controlled with a similar interface implementation.



Then it might be, that DATAMODUL_MODE_PACKET might need an other value.


That's future code so we can delete that sentence for now.


With the rule above, you are absolutely right. But we now spend time, to 
remove an currently non necessary feature ("double layer"), which will 
take time to re-introduce as soon, as someone wants to support a second 
chip.

Isn't that double-work and a thus a pitty?

Cheers,
Marcus

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Marcus Wolf]

Second there might be the idea of supporting different chips in the future
(I already thought about).


Linux style is never to write code for the future.


Ok. I didn't know.
To be honest, I already started writing code, also supporting the rf12 
some time ago, thus programming a rfxx.c, but never finished, due to 
lack of time.

For getting stuff started, I need to focus on rf69 and pi433.

A few monthes ago, Hope RF (the producer of those chips) proposed me a 
new chip (can't remember the number - maybe 95), that also supports 
loraWan. Seems like there will be even more interesting chips coming up, 
that could be controlled with a similar interface implementation.



Then it might be, that DATAMODUL_MODE_PACKET might need an other value.


That's future code so we can delete that sentence for now.


With the rule above, you are absolutely right. But we now spend time, to 
remove an currently non necessary feature ("double layer"), which will 
take time to re-introduce as soon, as someone wants to support a second 
chip.

Isn't that double-work and a thus a pitty?

Cheers,
Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:15 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 08:37:51PM +0200, Marcus Wolf wrote:



Am 04.12.2017 um 12:37 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 01:17:37PM +0300, Dan Carpenter wrote:

Perhaps choose different function names if you want?  You could do it
as several patches:

patch 1: change types to bool
patch 2: sed -e '/ == optionOn//'
patch 3: split the functions into two functions
patch 4: delete optionOnOff enum

patches 1 and 2 could be merged together (your choice).



Markus says that optionOn is used by user space so my you won't be able
to remove these entirely.  But as much as possible we should internally.

regards,
dan carpenter



Hi Dan, hi Simon,

I think, it's a pretty nice idea to remove th optionOnOff and replace it by
bool.


In former times, the variables in the config struct had very different names
- not containing "enable". Therefore optionOnOff was used to make absolutely
clear (in user space), wheter something was switched on, or off.
Now the variable have nice names, so bool is fine, even better now :-)


I would suggest not to split the amp-functions but to rename them, to also
contain an enable:
rf69_set_amp_X_enable()


That's a bad name, because it doesn't just enable it also disables.
Please split them.

regards,
dan carpenter




Same applies to all other stuff, that's using optionOnOff:
rf69_set_sync_enable(optionOn/Off) enables and disbales sync,
rf69_set_crc_enable(optionOn/Off) enables and disables crc,
...

In my opinion, if we want perfect clarity, we should stay with optionOnOff.
If we are ok, if rf69_set_sync_enable(false) disables sync,
in my opinion, we also have to be ok, if rf69_set_amp_X_enable(false) 
disables the amp.


Cheers,
Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 21:15 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 08:37:51PM +0200, Marcus Wolf wrote:



Am 04.12.2017 um 12:37 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 01:17:37PM +0300, Dan Carpenter wrote:

Perhaps choose different function names if you want?  You could do it
as several patches:

patch 1: change types to bool
patch 2: sed -e '/ == optionOn//'
patch 3: split the functions into two functions
patch 4: delete optionOnOff enum

patches 1 and 2 could be merged together (your choice).



Markus says that optionOn is used by user space so my you won't be able
to remove these entirely.  But as much as possible we should internally.

regards,
dan carpenter



Hi Dan, hi Simon,

I think, it's a pretty nice idea to remove th optionOnOff and replace it by
bool.


In former times, the variables in the config struct had very different names
- not containing "enable". Therefore optionOnOff was used to make absolutely
clear (in user space), wheter something was switched on, or off.
Now the variable have nice names, so bool is fine, even better now :-)


I would suggest not to split the amp-functions but to rename them, to also
contain an enable:
rf69_set_amp_X_enable()


That's a bad name, because it doesn't just enable it also disables.
Please split them.

regards,
dan carpenter




Same applies to all other stuff, that's using optionOnOff:
rf69_set_sync_enable(optionOn/Off) enables and disbales sync,
rf69_set_crc_enable(optionOn/Off) enables and disables crc,
...

In my opinion, if we want perfect clarity, we should stay with optionOnOff.
If we are ok, if rf69_set_sync_enable(false) disables sync,
in my opinion, we also have to be ok, if rf69_set_amp_X_enable(false) 
disables the amp.


Cheers,
Marcus

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 12:24 schrieb Dan Carpenter:

On Sun, Dec 03, 2017 at 04:17:25PM +0100, Simon Sandström wrote:

Renames enum dataMode and its values packet, continuous, continuousNoSync
to enum data_mode and PACKET, CONTINUOUS, CONTINUOUS_NO_SYNC. Fixes
checkpatch.pl warnings: "Avoid CamelCase: , ".


These names are too generic.  Delete them.  Use DATAMODUL_MODE_PACKET
and friends directly.

int rf69_set_data_mode(struct spi_device *spi, u8 val)
{
return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODE) | val);
}

Only DATAMODUL_MODE_PACKET is ever used.  There is no need to validate
the parameters.

regards,
dan carpenter



Hi Dan, hi Simon,

like I wrote a few days ago to Marcin Ciupak, I see two disadvantages in 
doing so.


If you want to go that way, you - as far as I believe - need to alter 
the values in rf69_enum.h, so they carry the corresponding values from 
rf69_reg.h. To avoid confusion, you will need to remove the values from 
rf69_reg.h.
But then you have to keep track of two files (enum.h and reg.h), if you 
want to further develop register access stuff. I would prefer to keep 
all chip/register related values at the same place.


Second there might be the idea of supporting different chips in the 
future (I already thought about).

Then it might be, that DATAMODUL_MODE_PACKET might need an other value.
Therefore, I introduced the "double layer" - enums as labels for the 
user space and defines, containing the values, for the register access.


For closer details, pls. see my long answer to Marcin.

I am not sure, whether simplification of the code like proposed is more 
important, then the disadvatages, I mentioned.


Cheers,

Marcus

Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 12:24 schrieb Dan Carpenter:

On Sun, Dec 03, 2017 at 04:17:25PM +0100, Simon Sandström wrote:

Renames enum dataMode and its values packet, continuous, continuousNoSync
to enum data_mode and PACKET, CONTINUOUS, CONTINUOUS_NO_SYNC. Fixes
checkpatch.pl warnings: "Avoid CamelCase: , ".


These names are too generic.  Delete them.  Use DATAMODUL_MODE_PACKET
and friends directly.

int rf69_set_data_mode(struct spi_device *spi, u8 val)
{
return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODE) | val);
}

Only DATAMODUL_MODE_PACKET is ever used.  There is no need to validate
the parameters.

regards,
dan carpenter



Hi Dan, hi Simon,

like I wrote a few days ago to Marcin Ciupak, I see two disadvantages in 
doing so.


If you want to go that way, you - as far as I believe - need to alter 
the values in rf69_enum.h, so they carry the corresponding values from 
rf69_reg.h. To avoid confusion, you will need to remove the values from 
rf69_reg.h.
But then you have to keep track of two files (enum.h and reg.h), if you 
want to further develop register access stuff. I would prefer to keep 
all chip/register related values at the same place.


Second there might be the idea of supporting different chips in the 
future (I already thought about).

Then it might be, that DATAMODUL_MODE_PACKET might need an other value.
Therefore, I introduced the "double layer" - enums as labels for the 
user space and defines, containing the values, for the register access.


For closer details, pls. see my long answer to Marcin.

I am not sure, whether simplification of the code like proposed is more 
important, then the disadvatages, I mentioned.


Cheers,

Marcus

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 12:33 schrieb Dan Carpenter:

On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:

diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 34ff0d4807bd..bcfe29840889 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -63,7 +63,7 @@ struct pi433_tx_cfg {
__u16   bit_rate;
__u32   dev_frequency;
enum modulation modulation;
-   enum modShaping modShaping;
+   enum mod_shapingmod_shaping;
  


I looked at how mod_shaping is set and the only place is in the ioctl:

789  case PI433_IOC_WR_TX_CFG:
790  if (copy_from_user(>tx_cfg, argp,
791  sizeof(struct pi433_tx_cfg)))
792  return -EFAULT;
793  break;

We just write over the whole config.  Including important things like
rx_cfg.fixed_message_length.  There is no locking so when we do things
like:

385  /* fixed or unlimited length? */
386  if (dev->rx_cfg.fixed_message_length != 0)
387  {
388  if (dev->rx_cfg.fixed_message_length > 
dev->rx_buffer_size)
 
^^
check

389  {
390  retval = -1;
391  goto abort;
392  }
393  bytes_total = dev->rx_cfg.fixed_message_length;
   ^
set this in the ioctl after the check but before this line and it looks
like a security problem.

394  dev_dbg(dev->dev,"rx: msg len set to %d by fixed 
length", bytes_total);
395  }

Anyway, I guess this patch is fine.

regards,
dan carpenter



Hi Dan,

you are mixing rx and tx. The part from IOCTL is copied from the 
tx-part, the lower part is dealing with rx.


With rx there should be no problem, since IOCTL is blocked, as long as 
an rx operation is going on.


With tx, I also expect no problems, since instance->tx_cfg is never used 
to configure the rf69. Everytime, you pass in new data via write() a 
copy of tx_cfg is made. Transmission is done, using the copy of the 
tx_cfg, never by using instance->tx_cfg.


But maybe I didn't got your point and misunderstand your intention.

Cheers,

Marcus

Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 12:33 schrieb Dan Carpenter:

On Sun, Dec 03, 2017 at 04:17:26PM +0100, Simon Sandström wrote:

diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 34ff0d4807bd..bcfe29840889 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -63,7 +63,7 @@ struct pi433_tx_cfg {
__u16   bit_rate;
__u32   dev_frequency;
enum modulation modulation;
-   enum modShaping modShaping;
+   enum mod_shapingmod_shaping;
  


I looked at how mod_shaping is set and the only place is in the ioctl:

789  case PI433_IOC_WR_TX_CFG:
790  if (copy_from_user(>tx_cfg, argp,
791  sizeof(struct pi433_tx_cfg)))
792  return -EFAULT;
793  break;

We just write over the whole config.  Including important things like
rx_cfg.fixed_message_length.  There is no locking so when we do things
like:

385  /* fixed or unlimited length? */
386  if (dev->rx_cfg.fixed_message_length != 0)
387  {
388  if (dev->rx_cfg.fixed_message_length > 
dev->rx_buffer_size)
 
^^
check

389  {
390  retval = -1;
391  goto abort;
392  }
393  bytes_total = dev->rx_cfg.fixed_message_length;
   ^
set this in the ioctl after the check but before this line and it looks
like a security problem.

394  dev_dbg(dev->dev,"rx: msg len set to %d by fixed 
length", bytes_total);
395  }

Anyway, I guess this patch is fine.

regards,
dan carpenter



Hi Dan,

you are mixing rx and tx. The part from IOCTL is copied from the 
tx-part, the lower part is dealing with rx.


With rx there should be no problem, since IOCTL is blocked, as long as 
an rx operation is going on.


With tx, I also expect no problems, since instance->tx_cfg is never used 
to configure the rf69. Everytime, you pass in new data via write() a 
copy of tx_cfg is made. Transmission is done, using the copy of the 
tx_cfg, never by using instance->tx_cfg.


But maybe I didn't got your point and misunderstand your intention.

Cheers,

Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 12:37 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 01:17:37PM +0300, Dan Carpenter wrote:

Perhaps choose different function names if you want?  You could do it
as several patches:

patch 1: change types to bool
patch 2: sed -e '/ == optionOn//'
patch 3: split the functions into two functions
patch 4: delete optionOnOff enum

patches 1 and 2 could be merged together (your choice).



Markus says that optionOn is used by user space so my you won't be able
to remove these entirely.  But as much as possible we should internally.

regards,
dan carpenter



Hi Dan, hi Simon,

I think, it's a pretty nice idea to remove th optionOnOff and replace it 
by bool.



In former times, the variables in the config struct had very different 
names - not containing "enable". Therefore optionOnOff was used to make 
absolutely clear (in user space), wheter something was switched on, or off.

Now the variable have nice names, so bool is fine, even better now :-)


I would suggest not to split the amp-functions but to rename them, to 
also contain an enable:

rf69_set_amp_X_enable()

To avoid misunderstandings maybe it is better to remove the enable from 
enable_address_filtering, since here we can't go with bool.


Thanks a lot for the ideas and improvements :-)

Marcus

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 04.12.2017 um 12:37 schrieb Dan Carpenter:

On Mon, Dec 04, 2017 at 01:17:37PM +0300, Dan Carpenter wrote:

Perhaps choose different function names if you want?  You could do it
as several patches:

patch 1: change types to bool
patch 2: sed -e '/ == optionOn//'
patch 3: split the functions into two functions
patch 4: delete optionOnOff enum

patches 1 and 2 could be merged together (your choice).



Markus says that optionOn is used by user space so my you won't be able
to remove these entirely.  But as much as possible we should internally.

regards,
dan carpenter



Hi Dan, hi Simon,

I think, it's a pretty nice idea to remove th optionOnOff and replace it 
by bool.



In former times, the variables in the config struct had very different 
names - not containing "enable". Therefore optionOnOff was used to make 
absolutely clear (in user space), wheter something was switched on, or off.

Now the variable have nice names, so bool is fine, even better now :-)


I would suggest not to split the amp-functions but to rename them, to 
also contain an enable:

rf69_set_amp_X_enable()

To avoid misunderstandings maybe it is better to remove the enable from 
enable_address_filtering, since here we can't go with bool.


Thanks a lot for the ideas and improvements :-)

Marcus

[PATCH] staging: pi433: rf69.c: Replace macros READ_REG and WRITE_REG with inline functions rf69_set_bit, rf69_reset_bit and rf69_read_modify_write

[Marcus Wolf]

To increase the readability of the register accesses, the abstraction
of the helpers was increased from simple read and write to set bit,
reset bit and read modify write bit. In addition - according to the
proposal from Walter Harms from 20.07.2017 - instead of marcros inline
functions were used.

Signed-off-by: Marcus Wolf <li...@wolf-entwicklungen.de>
---
 drivers/staging/pi433/rf69.c |  340 ++
 1 file changed, 182 insertions(+), 158 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a215..8a31ed7 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -33,8 +33,32 @@
 
 /*-*/
 
-#define READ_REG(x)rf69_read_reg (spi, x)
-#define WRITE_REG(x, y)rf69_write_reg(spi, x, y)
+static inline int rf69_set_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmpVal;
+
+   tmpVal = rf69_read_reg (spi, reg);
+   tmpVal = tmpVal | mask;
+   return rf69_write_reg(spi, reg, tmpVal);
+}
+
+static inline int rf69_reset_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmpVal;
+
+   tmpVal = rf69_read_reg (spi, reg);
+   tmpVal = tmpVal & ~mask;
+   return rf69_write_reg(spi, reg, tmpVal);
+}
+
+static inline int rf69_read_modify_write(struct spi_device *spi, u8 reg, u8 
mask, u8 value)
+{
+   u8 tmpVal;
+
+   tmpVal = rf69_read_reg (spi, reg);
+   tmpVal = (tmpVal & ~mask) | value;
+   return rf69_write_reg(spi, reg, tmpVal);
+}
 
 /*-*/
 
@@ -45,11 +69,11 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
#endif
 
switch (mode) {
-   case transmit:return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_TRANSMIT);
-   case receive: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_RECEIVE);
-   case synthesizer: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SYNTHESIZER);
-   case standby: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_STANDBY);
-   case mode_sleep:  return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SLEEP);
+   case transmit:return rf69_read_modify_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_TRANSMIT);
+   case receive: return rf69_read_modify_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_RECEIVE);
+   case synthesizer: return rf69_read_modify_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SYNTHESIZER);
+   case standby: return rf69_read_modify_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_STANDBY);
+   case mode_sleep:  return rf69_read_modify_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SLEEP);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -68,9 +92,9 @@ int rf69_set_data_mode(struct spi_device *spi, enum dataMode 
dataMode)
#endif
 
switch (dataMode) {
-   case packet:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-   case continuous:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-   case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | 
DATAMODUL_MODE_CONTINUOUS_NOSYNC);
+   case packet:return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_PACKET);
+   case continuous:return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS);
+   case continuousNoSync:  return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS_NOSYNC);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -84,8 +108,8 @@ int rf69_set_modulation(struct spi_device *spi, enum 
modulation modulation)
#endif
 
switch (modulation) {
-   case OOK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_FSK);
+   case OOK: return rf69_read_modify_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
+   case FSK: return rf69_read_modify_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
default:
dev_dbg(>

[PATCH] staging: pi433: rf69.c: Replace macros READ_REG and WRITE_REG with inline functions rf69_set_bit, rf69_reset_bit and rf69_read_modify_write

[Marcus Wolf]

To increase the readability of the register accesses, the abstraction
of the helpers was increased from simple read and write to set bit,
reset bit and read modify write bit. In addition - according to the
proposal from Walter Harms from 20.07.2017 - instead of marcros inline
functions were used.

Signed-off-by: Marcus Wolf 
---
 drivers/staging/pi433/rf69.c |  340 ++
 1 file changed, 182 insertions(+), 158 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a215..8a31ed7 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -33,8 +33,32 @@
 
 /*-*/
 
-#define READ_REG(x)rf69_read_reg (spi, x)
-#define WRITE_REG(x, y)rf69_write_reg(spi, x, y)
+static inline int rf69_set_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmpVal;
+
+   tmpVal = rf69_read_reg (spi, reg);
+   tmpVal = tmpVal | mask;
+   return rf69_write_reg(spi, reg, tmpVal);
+}
+
+static inline int rf69_reset_bit(struct spi_device *spi, u8 reg, u8 mask)
+{
+   u8 tmpVal;
+
+   tmpVal = rf69_read_reg (spi, reg);
+   tmpVal = tmpVal & ~mask;
+   return rf69_write_reg(spi, reg, tmpVal);
+}
+
+static inline int rf69_read_modify_write(struct spi_device *spi, u8 reg, u8 
mask, u8 value)
+{
+   u8 tmpVal;
+
+   tmpVal = rf69_read_reg (spi, reg);
+   tmpVal = (tmpVal & ~mask) | value;
+   return rf69_write_reg(spi, reg, tmpVal);
+}
 
 /*-*/
 
@@ -45,11 +69,11 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
#endif
 
switch (mode) {
-   case transmit:return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_TRANSMIT);
-   case receive: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_RECEIVE);
-   case synthesizer: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SYNTHESIZER);
-   case standby: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_STANDBY);
-   case mode_sleep:  return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & 
~MASK_OPMODE_MODE) | OPMODE_MODE_SLEEP);
+   case transmit:return rf69_read_modify_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_TRANSMIT);
+   case receive: return rf69_read_modify_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_RECEIVE);
+   case synthesizer: return rf69_read_modify_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SYNTHESIZER);
+   case standby: return rf69_read_modify_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_STANDBY);
+   case mode_sleep:  return rf69_read_modify_write(spi, REG_OPMODE, 
MASK_OPMODE_MODE, OPMODE_MODE_SLEEP);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -68,9 +92,9 @@ int rf69_set_data_mode(struct spi_device *spi, enum dataMode 
dataMode)
#endif
 
switch (dataMode) {
-   case packet:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_PACKET);
-   case continuous:return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | DATAMODUL_MODE_CONTINUOUS);
-   case continuousNoSync:  return WRITE_REG(REG_DATAMODUL, 
(READ_REG(REG_DATAMODUL) & ~MASK_DATAMODUL_MODE) | 
DATAMODUL_MODE_CONTINUOUS_NOSYNC);
+   case packet:return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_PACKET);
+   case continuous:return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS);
+   case continuousNoSync:  return rf69_read_modify_write(spi, 
REG_DATAMODUL, MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS_NOSYNC);
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
@@ -84,8 +108,8 @@ int rf69_set_modulation(struct spi_device *spi, enum 
modulation modulation)
#endif
 
switch (modulation) {
-   case OOK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_OOK);
-   case FSK:   return WRITE_REG(REG_DATAMODUL, (READ_REG(REG_DATAMODUL) & 
~MASK_DATAMODUL_MODULATION_TYPE) | DATAMODUL_MODULATION_TYPE_FSK);
+   case OOK: return rf69_read_modify_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
+   case FSK: return rf69_read_modify_write(spi, REG_DATAMODUL, 
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
default:
dev_dbg(>dev, "set: illegal 

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 03.12.2017 um 17:17 schrieb Simon Sandström:

Renames the enum optionOnOff and its values optionOn, optionOff to enum
option_on_off and OPTION_ON, OPTION_OFF. Fixes checkpatch.pl warnings:
"Avoid CamelCase: , , ".

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c  | 34 ++---
  drivers/staging/pi433/pi433_if.h  | 16 +++---
  drivers/staging/pi433/rf69.c  | 45 ++-
  drivers/staging/pi433/rf69.h  | 15 -
  drivers/staging/pi433/rf69_enum.h |  6 +++---
  5 files changed, 63 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b8efe6a74a2a..4f6830f369e9 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -198,7 +198,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
/* packet config */
/* enable */
SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, 
afterSyncInterrupt));
}
@@ -206,7 +206,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
}
-   if (rx_cfg->enable_length_byte == optionOn) {
+   if (rx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -220,14 +220,14 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
  
  	/* lengths */

SET_CHECKED(rf69_set_sync_size(dev->spi, rx_cfg->sync_length));
-   if (rx_cfg->enable_length_byte == optionOn)
+   if (rx_cfg->enable_length_byte == OPTION_ON)
{
SET_CHECKED(rf69_set_payload_length(dev->spi, 0xff));
}
else if (rx_cfg->fixed_message_length != 0)
{
payload_length = rx_cfg->fixed_message_length;
-   if (rx_cfg->enable_length_byte  == optionOn) payload_length++;
+   if (rx_cfg->enable_length_byte  == OPTION_ON) payload_length++;
if (rx_cfg->enable_address_filtering != filteringOff) 
payload_length++;
SET_CHECKED(rf69_set_payload_length(dev->spi, payload_length));
}
@@ -237,7 +237,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
}
  
  	/* values */

-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_sync_values(dev->spi, 
rx_cfg->sync_pattern));
}
@@ -264,7 +264,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_tx_start_condition(dev->spi, 
tx_cfg->tx_start_condition));
  
  	/* packet format enable */

-   if (tx_cfg->enable_preamble == optionOn)
+   if (tx_cfg->enable_preamble == OPTION_ON)
{
SET_CHECKED(rf69_set_preamble_length(dev->spi, 
tx_cfg->preamble_length));
}
@@ -273,7 +273,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_preamble_length(dev->spi, 0));
}
SET_CHECKED(rf69_set_sync_enable  (dev->spi, tx_cfg->enable_sync));
-   if (tx_cfg->enable_length_byte == optionOn) {
+   if (tx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -285,7 +285,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_crc_enable   (dev->spi, tx_cfg->enable_crc));
  
  	/* configure sync, if enabled */

-   if (tx_cfg->enable_sync == optionOn) {
+   if (tx_cfg->enable_sync == OPTION_ON) {
SET_CHECKED(rf69_set_sync_size(dev->spi, tx_cfg->sync_length));
SET_CHECKED(rf69_set_sync_values(dev->spi, 
tx_cfg->sync_pattern));
}
@@ -400,7 +400,7 @@ pi433_receive(void *data)
}
  
  	/* length byte enabled? */

-   if (dev->rx_cfg.enable_length_byte == optionOn)
+   if (dev->rx_cfg.enable_length_byte == OPTION_ON)
{
retval = wait_event_interruptible(dev->fifo_wait_queue,
  dev->free_in_fifo < 
FIFO_SIZE);
@@ -525,11 +525,11 @@ pi433_tx_thread(void *data)
size = tx_cfg.fixed_message_length;
  
  		/* increase size, if len byte is requested */

-   if (tx_cfg.enable_length_byte == optionOn)
+   if (tx_cfg.enable_length_byte == OPTION_ON)
size++;
  
  		/* increase size, if adr byte is requested */

-  

Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Marcus Wolf]

Am 03.12.2017 um 17:17 schrieb Simon Sandström:

Renames the enum optionOnOff and its values optionOn, optionOff to enum
option_on_off and OPTION_ON, OPTION_OFF. Fixes checkpatch.pl warnings:
"Avoid CamelCase: , , ".

Signed-off-by: Simon Sandström 
---
  drivers/staging/pi433/pi433_if.c  | 34 ++---
  drivers/staging/pi433/pi433_if.h  | 16 +++---
  drivers/staging/pi433/rf69.c  | 45 ++-
  drivers/staging/pi433/rf69.h  | 15 -
  drivers/staging/pi433/rf69_enum.h |  6 +++---
  5 files changed, 63 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b8efe6a74a2a..4f6830f369e9 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -198,7 +198,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
/* packet config */
/* enable */
SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, 
afterSyncInterrupt));
}
@@ -206,7 +206,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
{
SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
}
-   if (rx_cfg->enable_length_byte == optionOn) {
+   if (rx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -220,14 +220,14 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
  
  	/* lengths */

SET_CHECKED(rf69_set_sync_size(dev->spi, rx_cfg->sync_length));
-   if (rx_cfg->enable_length_byte == optionOn)
+   if (rx_cfg->enable_length_byte == OPTION_ON)
{
SET_CHECKED(rf69_set_payload_length(dev->spi, 0xff));
}
else if (rx_cfg->fixed_message_length != 0)
{
payload_length = rx_cfg->fixed_message_length;
-   if (rx_cfg->enable_length_byte  == optionOn) payload_length++;
+   if (rx_cfg->enable_length_byte  == OPTION_ON) payload_length++;
if (rx_cfg->enable_address_filtering != filteringOff) 
payload_length++;
SET_CHECKED(rf69_set_payload_length(dev->spi, payload_length));
}
@@ -237,7 +237,7 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct 
pi433_rx_cfg *rx_cfg)
}
  
  	/* values */

-   if (rx_cfg->enable_sync == optionOn)
+   if (rx_cfg->enable_sync == OPTION_ON)
{
SET_CHECKED(rf69_set_sync_values(dev->spi, 
rx_cfg->sync_pattern));
}
@@ -264,7 +264,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_tx_start_condition(dev->spi, 
tx_cfg->tx_start_condition));
  
  	/* packet format enable */

-   if (tx_cfg->enable_preamble == optionOn)
+   if (tx_cfg->enable_preamble == OPTION_ON)
{
SET_CHECKED(rf69_set_preamble_length(dev->spi, 
tx_cfg->preamble_length));
}
@@ -273,7 +273,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_preamble_length(dev->spi, 0));
}
SET_CHECKED(rf69_set_sync_enable  (dev->spi, tx_cfg->enable_sync));
-   if (tx_cfg->enable_length_byte == optionOn) {
+   if (tx_cfg->enable_length_byte == OPTION_ON) {
ret = rf69_set_packet_format(dev->spi, packetLengthVar);
if (ret < 0)
return ret;
@@ -285,7 +285,7 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct 
pi433_tx_cfg *tx_cfg)
SET_CHECKED(rf69_set_crc_enable   (dev->spi, tx_cfg->enable_crc));
  
  	/* configure sync, if enabled */

-   if (tx_cfg->enable_sync == optionOn) {
+   if (tx_cfg->enable_sync == OPTION_ON) {
SET_CHECKED(rf69_set_sync_size(dev->spi, tx_cfg->sync_length));
SET_CHECKED(rf69_set_sync_values(dev->spi, 
tx_cfg->sync_pattern));
}
@@ -400,7 +400,7 @@ pi433_receive(void *data)
}
  
  	/* length byte enabled? */

-   if (dev->rx_cfg.enable_length_byte == optionOn)
+   if (dev->rx_cfg.enable_length_byte == OPTION_ON)
{
retval = wait_event_interruptible(dev->fifo_wait_queue,
  dev->free_in_fifo < 
FIFO_SIZE);
@@ -525,11 +525,11 @@ pi433_tx_thread(void *data)
size = tx_cfg.fixed_message_length;
  
  		/* increase size, if len byte is requested */

-   if (tx_cfg.enable_length_byte == optionOn)
+   if (tx_cfg.enable_length_byte == OPTION_ON)
size++;
  
  		/* increase size, if adr byte is requested */

-   if 

Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

[Marcus Wolf]

Am 02.12.2017 um 18:46 schrieb Joe Perches:

On Sat, 2017-12-02 at 17:20 +0200, Marcus Wolf wrote:

rf69_set_dc_cut_off_frequency_intern is used by rf69.c internally only.
Therefore removed the function from header and declared it staic in
the implemtation.
Signed-off-by: Marcus Wolf <li...@wolf-entwicklungen.de>
---
  drivers/staging/pi433/rf69.c |2 +-
  drivers/staging/pi433/rf69.h |1 -
  2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index ec4b540..90ccf4e 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -442,7 +442,7 @@ 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)

+static int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 
reg, enum dccPercent dccPercent)
  {
switch (dccPercent) {
case dcc16Percent:  return rmw(spi, reg, MASK_BW_DCC_FREQ, 
BW_DCC_16_PERCENT);
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 645c8df..7f580e9 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -36,7 +36,6 @@
  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);
  int rf69_set_bandwidth(struct spi_device *spi, enum mantisse mantisse, u8 
exponent);


Beyond the basics of learning to submit patches by
shutting up checkpatch messages, please always keep
in mind how to actually improve the logic and code
clarity for human readers.

The rf69_set_dc_cut_off_frequency_intern function
is actually pretty poorly written.

It's repeated logic that could be simplified and
code size reduced quite a bit.

For instance, the patch below makes it more obvious
what the function does and reduces boiler-plate
copy/paste to a single line.

And I don't particularly care that it is not
checkpatch 'clean'.  checkpatch is only a stupid,
mindless style checker.  Always try to be better
than a mindless script.

and you -really- want to make it checkpatch clean,
a new #define could be used like this below

#define case_dcc_percent(val, dcc, DCC) \
case dcc: val = DCC; break;

Anyway:

$ size drivers/staging/pi433/rf69.o*
text   data bss dec hex 
filename
   35820   5600   0   41420a1cc 
drivers/staging/pi433/rf69.o.new
   36968   5600   0   42568a648 
drivers/staging/pi433/rf69.o.old
---
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a2153c999..9e40f162ac77 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -423,19 +423,23 @@ 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 val;
+
switch (dccPercent) {
-   case dcc16Percent:  return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_16_PERCENT));
-   case dcc8Percent:   return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_8_PERCENT));
-   case dcc4Percent:   return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_4_PERCENT));
-   case dcc2Percent:   return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_2_PERCENT));
-   case dcc1Percent:   return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_1_PERCENT));
-   case dcc0_5Percent: return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_0_5_PERCENT));
-   case dcc0_25Percent:return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_0_25_PERCENT));
-   case dcc0_125Percent:   return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_0_125_PERCENT));
+   case dcc16Percent:  val = BW_DCC_16_PERCENT; break;
+   case dcc8Percent:   val = BW_DCC_8_PERCENT; break;
+   case dcc4Percent:   val = BW_DCC_4_PERCENT; break;
+   case dcc2Percent:   val = BW_DCC_2_PERCENT; break;
+   case dcc1Percent:   val = BW_DCC_1_PERCENT; break;
+   case dcc0_5Percent: val = BW_DCC_0_5_PERCENT; break;
+   case dcc0_25Percent:val = BW_DCC_0_25_PERCENT; break;
+   case dcc0_125Percent:   val = BW_DCC_0_125_PERCENT; break;
default:
dev_dbg(>dev, "set: illegal input param");
retur

Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

[Marcus Wolf]

Am 02.12.2017 um 18:46 schrieb Joe Perches:

On Sat, 2017-12-02 at 17:20 +0200, Marcus Wolf wrote:

rf69_set_dc_cut_off_frequency_intern is used by rf69.c internally only.
Therefore removed the function from header and declared it staic in
the implemtation.
Signed-off-by: Marcus Wolf 
---
  drivers/staging/pi433/rf69.c |2 +-
  drivers/staging/pi433/rf69.h |1 -
  2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index ec4b540..90ccf4e 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -442,7 +442,7 @@ 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)

+static int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 
reg, enum dccPercent dccPercent)
  {
switch (dccPercent) {
case dcc16Percent:  return rmw(spi, reg, MASK_BW_DCC_FREQ, 
BW_DCC_16_PERCENT);
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 645c8df..7f580e9 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -36,7 +36,6 @@
  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);
  int rf69_set_bandwidth(struct spi_device *spi, enum mantisse mantisse, u8 
exponent);


Beyond the basics of learning to submit patches by
shutting up checkpatch messages, please always keep
in mind how to actually improve the logic and code
clarity for human readers.

The rf69_set_dc_cut_off_frequency_intern function
is actually pretty poorly written.

It's repeated logic that could be simplified and
code size reduced quite a bit.

For instance, the patch below makes it more obvious
what the function does and reduces boiler-plate
copy/paste to a single line.

And I don't particularly care that it is not
checkpatch 'clean'.  checkpatch is only a stupid,
mindless style checker.  Always try to be better
than a mindless script.

and you -really- want to make it checkpatch clean,
a new #define could be used like this below

#define case_dcc_percent(val, dcc, DCC) \
case dcc: val = DCC; break;

Anyway:

$ size drivers/staging/pi433/rf69.o*
text   data bss dec hex 
filename
   35820   5600   0   41420a1cc 
drivers/staging/pi433/rf69.o.new
   36968   5600   0   42568a648 
drivers/staging/pi433/rf69.o.old
---
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a2153c999..9e40f162ac77 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -423,19 +423,23 @@ 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 val;
+
switch (dccPercent) {
-   case dcc16Percent:  return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_16_PERCENT));
-   case dcc8Percent:   return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_8_PERCENT));
-   case dcc4Percent:   return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_4_PERCENT));
-   case dcc2Percent:   return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_2_PERCENT));
-   case dcc1Percent:   return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_1_PERCENT));
-   case dcc0_5Percent: return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_0_5_PERCENT));
-   case dcc0_25Percent:return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_0_25_PERCENT));
-   case dcc0_125Percent:   return WRITE_REG(reg, ((READ_REG(reg) & 
~MASK_BW_DCC_FREQ) | BW_DCC_0_125_PERCENT));
+   case dcc16Percent:  val = BW_DCC_16_PERCENT; break;
+   case dcc8Percent:   val = BW_DCC_8_PERCENT; break;
+   case dcc4Percent:   val = BW_DCC_4_PERCENT; break;
+   case dcc2Percent:   val = BW_DCC_2_PERCENT; break;
+   case dcc1Percent:   val = BW_DCC_1_PERCENT; break;
+   case dcc0_5Percent: val = BW_DCC_0_5_PERCENT; break;
+   case dcc0_25Percent:val = BW_DCC_0_25_PERCENT; break;
+   case dcc0_125Percent:   val = BW_DCC_0_125_PERCENT; break;
default:
dev_dbg(>dev, "set: illegal input param");
return -EINVAL;
}

Re: [PATCH] staging: pi433: rf69.c: Introduced define DEBUG_FUNC_ENTRY

[Marcus Wolf]

Am 02.12.2017 um 17:00 schrieb Greg KH:

On Sat, Dec 02, 2017 at 01:45:50PM +0200, Marcus Wolf wrote:

Since dev_dbg already depends on define DEBUG, there was no sense, to enclose
dev_dbg lines with #ifdef DEBUG.
Instead of removing #ifdef DEBUG, I introduced define DEBUG_FUNC_ENTRY. So now 
it is
possible to switch of the function entry debug lines even while debug is 
switched on.


Ick ick ick.

No, these lines should just all be deleted.  Use ftrace if you want to
see what functions are being called, that's what it is there for.  Don't
create driver-specific defines and functionality for when we already
have that functionality for the whole of the kernel.  That's really
redundant and messy.


Signed-off-by: Marcus Wolf <li...@wolf-entwicklungen.de>
---
  drivers/staging/pi433/rf69.c |  118 +-
  1 file changed, 58 insertions(+), 60 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 12c9df9..0df084e 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -15,8 +15,10 @@
   * GNU General Public License for more details.
   */
  
-/* enable prosa debug info */

+/* generic enable/disable dev_dbg */
  #undef DEBUG
+/* enable print function entry */
+#undef DEBUG_FUNC_ENTRY
  /* enable print of values on reg access */
  #undef DEBUG_VALUES
  /* enable print of values on fifo access */
@@ -40,7 +42,7 @@
  
  int rf69_set_mode(struct spi_device *spi, enum mode mode)

  {
-   #ifdef DEBUG
+   #ifdef DEBUG_FUNC_ENTRY
dev_dbg(>dev, "set: mode");
#endif


So this whole #ifdef dev_dbg #endif should all be removed.

thanks,

greg k-h



Hi all,

just for clarification, why I introduced these dev_dbg during 
development of the driver and didn't use ftrace:


Since I wanted the driver to use a single module as sender and receiver 
at (almost) the same time, the module constantly needs to be 
reconfigured (constant switching between rx configuration and tx 
configuration - see my documentation for details on the idea).


The routine, accessing the registers is able to print the register 
number and the value, it reads / writes from / to the register. It's dam 
hard to keep the survey over the use 30...40 register numbers, in 10th 
of rows of register setting and reading, if you see just the numbers in 
the log. Especially this is / was hard, if one register was written 
several times, because it contains different settings. Then decoding of 
the adress wasn't enough, I even need to decode the bits in the value.


Therefore I finally introduced this dev_dbg lines at the enty of the 
setter (and getter): After that in the log I coud see something like this:

Set gain: 0x43 0x81
Set threshold: 0x51 0x30
Get moulation: 0x22 0x7c
instead of just numbers.
That eased the debugging a lot.

When using ftrace I would be able to see, in which order the setter were 
called, but the link to the vlaues would be missing.


If those dev_dbg are unwanted, I am ok, if someone removes them.

Hope this helps understanding

Marcus

Re: [PATCH] staging: pi433: rf69.c: Introduced define DEBUG_FUNC_ENTRY

[Marcus Wolf]

Am 02.12.2017 um 17:00 schrieb Greg KH:

On Sat, Dec 02, 2017 at 01:45:50PM +0200, Marcus Wolf wrote:

Since dev_dbg already depends on define DEBUG, there was no sense, to enclose
dev_dbg lines with #ifdef DEBUG.
Instead of removing #ifdef DEBUG, I introduced define DEBUG_FUNC_ENTRY. So now 
it is
possible to switch of the function entry debug lines even while debug is 
switched on.


Ick ick ick.

No, these lines should just all be deleted.  Use ftrace if you want to
see what functions are being called, that's what it is there for.  Don't
create driver-specific defines and functionality for when we already
have that functionality for the whole of the kernel.  That's really
redundant and messy.


Signed-off-by: Marcus Wolf 
---
  drivers/staging/pi433/rf69.c |  118 +-
  1 file changed, 58 insertions(+), 60 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 12c9df9..0df084e 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -15,8 +15,10 @@
   * GNU General Public License for more details.
   */
  
-/* enable prosa debug info */

+/* generic enable/disable dev_dbg */
  #undef DEBUG
+/* enable print function entry */
+#undef DEBUG_FUNC_ENTRY
  /* enable print of values on reg access */
  #undef DEBUG_VALUES
  /* enable print of values on fifo access */
@@ -40,7 +42,7 @@
  
  int rf69_set_mode(struct spi_device *spi, enum mode mode)

  {
-   #ifdef DEBUG
+   #ifdef DEBUG_FUNC_ENTRY
dev_dbg(>dev, "set: mode");
#endif


So this whole #ifdef dev_dbg #endif should all be removed.

thanks,

greg k-h



Hi all,

just for clarification, why I introduced these dev_dbg during 
development of the driver and didn't use ftrace:


Since I wanted the driver to use a single module as sender and receiver 
at (almost) the same time, the module constantly needs to be 
reconfigured (constant switching between rx configuration and tx 
configuration - see my documentation for details on the idea).


The routine, accessing the registers is able to print the register 
number and the value, it reads / writes from / to the register. It's dam 
hard to keep the survey over the use 30...40 register numbers, in 10th 
of rows of register setting and reading, if you see just the numbers in 
the log. Especially this is / was hard, if one register was written 
several times, because it contains different settings. Then decoding of 
the adress wasn't enough, I even need to decode the bits in the value.


Therefore I finally introduced this dev_dbg lines at the enty of the 
setter (and getter): After that in the log I coud see something like this:

Set gain: 0x43 0x81
Set threshold: 0x51 0x30
Get moulation: 0x22 0x7c
instead of just numbers.
That eased the debugging a lot.

When using ftrace I would be able to see, in which order the setter were 
called, but the link to the vlaues would be missing.


If those dev_dbg are unwanted, I am ok, if someone removes them.

Hope this helps understanding

Marcus