Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h
On Tue, Dec 05, 2017 at 01:40:02PM +0100, Marcus Wolf wrote: > > 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's liked splitting it up but he also proposed this alternative: rf69_set_sync_operation(spi, true/false); but I removed the "_operation" because I think it doesn't add anything. > > 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). To me it's just weird that "_enable" disables anything. I really prefer just splitting it up. I don't think it will bloat the code. But I'm also fine with: rf69_set_sync(spi, true/false) rf69_set_amp(spi, true/false) rf69_set_crc(spi, true/false) Anyway, I feel like I'll like whatever Simon does. Some of these things, you can't tell how they'll look until the end until you try. Let's wait until we see a patch before we debate any more. regards, dan carpenter
Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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
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
Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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
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. > > 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 - Simon
Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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
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
Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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
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
Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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
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
Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h
On Sun, Dec 03, 2017 at 04:17:24PM +0100, Simon Sandström wrote:
> 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)
I haven't told anyone yet, but I wish someone would just get rid of
optionOn entirely. This should just be:
if (rx_cfg->enable_sync) {
> diff --git a/drivers/staging/pi433/pi433_if.h
> b/drivers/staging/pi433/pi433_if.h
> index 6b31c1584b3a..34ff0d4807bd 100644
> --- a/drivers/staging/pi433/pi433_if.h
> +++ b/drivers/staging/pi433/pi433_if.h
> @@ -73,11 +73,11 @@ struct pi433_tx_cfg {
>
>
> /* packet format */
> - enum optionOnOffenable_preamble;
> - enum optionOnOffenable_sync;
> - enum optionOnOffenable_length_byte;
> - enum optionOnOffenable_address_byte;
> - enum optionOnOffenable_crc;
> + enum option_on_off enable_preamble;
> + enum option_on_off enable_sync;
> + enum option_on_off enable_length_byte;
> + enum option_on_off enable_address_byte;
> + enum option_on_off enable_crc;
These should be bool.
> -int rf69_set_amplifier_0(struct spi_device *spi, enum optionOnOff
> optionOnOff)
> +int rf69_set_amplifier_0(struct spi_device *spi,
> + enum option_on_off option_on_off)
This should be two functions:
int rf69_enable_amplifier_0(struct spi_device *spi)
{
return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) |
MASK_PALEVEL_PA0));
}
int rf69_disable_amplifier_0(struct spi_device *spi)
{
return WRITE_REG(REG_PALEVEL, (READ_REG(REG_PALEVEL) &
~MASK_PALEVEL_PA0));
}
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).
regards,
dan carpenter
Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h
On Sun, Dec 03, 2017 at 06:49:40PM +0200, Marcus Wolf wrote: > > Hi Simon, Hi, > > thanks for your effort. > > I have two questions: > * According to my practical experiance, enums were always written in lower > case. Does kernel style guide ask for upper case for enums? Yes. From Documentation/process/coding-style.rst: "Names of macros defining constants and labels in enums are capitalized". > For me upper case indicates, that this value will be processed by > preprocessor, not by compiler. So I used it for define constants and macros > so far... For me a upper case identifier indicates something that has a constant value. Therefore I think it makes sense that enum labels are capitalized. > * The enums are the interface to user space. Isn't it necessary to increse > the interface version number on every change of the enums? > Or can we stay with old version number, since order of the enums is > untouched? Good question. I read the note about having to change ioctl number if the contents of the struct ever change, but is this also true if only the name of a variable change? I mean, the actual content is still the same. I will investigate this more and get back to you. > > Thanks, > > Marcus Regards, Simon
Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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 (tx_cfg.enable
[PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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 (tx_cfg.enable_address_byte == option
