Re: [PATCH] staging: pi433: #define shift constants in rf69.c
Hello everybody!
Concerning the naming:
==
When writing the rf69.c it wasn't intended to write a driver for Linux.
This file was written at a time, where the complete controlling of Pi433
was implemented in the application. Therefore it is written in a
completely different coding style.
Never the less - except from MASK and SHIFT, these names 100% - or let's
better say 97% comply with the naming of the registers and the bits you
will find in the RFM69 data sheet. I imported the table from datasheet
to start the reg.h file
If you are maintaining the function/features of the code (working with
the data sheet) that helps a lot.
So in my opinion - if desired - we should change from
MASK_registername_bit(s)name
to
registername_bit(s)name_MASK
but we should keep register and bit(s)names untouched.
Regarding the long line:
If someone is fixing a bug on a certain line, I would strongly
prefer not to touch the long line, just to please checkpatch.
In general for sure we should fix the long lines everywhere, it can be
done without reducing readability. But it should be done as a whole. In
rf69.c there are constructions, that appear over and over again, because
everything over there deals with register access, thus always doing the
same stuff in a slightly different way.
In my opinion there should be one kind of coding, that should be used
for all similar lines.
If the functionality needs service, I would hate to have the same
functionality implemented in several different styles.
At the moment I am recovering from a surgery of my back that was
necessary due to a disease at my discs that started several months
ago. So at the moment it stil is hard for me to sit at the desk for a
longer time.
Yesterday I started to review all driver mails I got in the last two
months. There were several attempts to fix style problems as a whole. Up
to now, I haven't checked, why those patches haven't been accepted.
I'll proceed checking all that stuff within the next week(s).
Regarding the bit shift:
Indeed there is a bug. I already discussed that topic long time ago.
Most probably I even sent a fix with a completly different
implementation that time, but it was rejected due to missformated patch.
I'll try to pass in a new patch today or tomorrow.
By the way one question:
If I for example want to send one patch per week and the patch of the
third week impacts a line, that was already impacted in the patch of the
first week, should the patch in week three be a diff to master or a diff
to patch one?
Cheers,
Marcus
Am 08.11.2017 um 13:52 schrieb Dan Carpenter:
On Wed, Nov 08, 2017 at 06:25:06AM -0500, Joshua Abraham wrote:
This patch completes TODO improvements in rf69.c to change shift
constants to a define.
Signed-off-by: Joshua Abraham
---
drivers/staging/pi433/rf69.c | 4 ++--
drivers/staging/pi433/rf69_registers.h | 4
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a2153c999..cfcace195be9 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 >>
SHIFT_DATAMODUL_MODE) {
You've send a few of mechanical patches without waiting for feedback and
you should probably slow down...
The first thing to notice is that the original code is probably buggy
and needs parenthesis.
switch ((currentValue & MASK_DATAMODUL_MODULATION_TYPE) >> 3) {
But that still doesn't fix the problem that x18 >> 3 is never going to
equal to DATAMODUL_MODULATION_TYPE_OOK which is 0x8... So there are a
couple bugs here.
The line is over 80 characters, so checkpatch will complain about your
patch. Please run checkpatch.pl on all your patches. Really, I hate
all the naming here... Surely we can think of a better name than
MASK_DATAMODUL_MODULATION_TYPE? Normally the "MASK" and "SHIFT" part of
the name go at the end instead of the start.
/* RegDataModul */
+#define SHIFT_DATAMODUL_MODE 0x03
+
#define MASK_DATAMODUL_MODE 0x06
Why did you add a blank line? Don't use hex values for shifting, use
normal numbers. The 0x3 is indented too far.
Anyway, take your time and really think about patches before you send
them. Normally, I write a patch, then wait overnight, then review it
and again in the morning before I send it.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Fixes issue with bit shift in rf69_get_modulation
Fixes issue with bit shift in rf69_get_modulation
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) {
case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
default:return undefined;
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: fix missing break in switch statement.
Hi all!
Tryed to cross check...
Don't get it, sorry.
On my private version control (my SVN), where I initially developed the
driver the break isn't missing.
Same with my git copy of Gregs staging tree. Break is there...
Who removed it, why is it missing in Colins copy?
Am I working on a wrong version?
marcus@Laptop-Wolf:~/staging/drivers/staging/pi433$ git remote show origin
* remote origin
Fetch URL:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
Push URL:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
Can anybody help me?
Thanks,
Marcus
Am 09.11.2017 um 19:19 schrieb Colin King:
From: Colin Ian King
The PI433_IOC_WR_RX_CFG case is missing a break and will fall through
to the default case and errorenously return -EINVAL. Fix this by
adding in missing break.
Detected by CoverityScan, CID#1461286 ("Missing break in switch")
Fixes: f81f0b5c9a30 ("pi433: sanitize ioctl")
Signed-off-by: Colin Ian King
---
drivers/staging/pi433/pi433_if.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 3bcb59811cdf..a960fe2e7875 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -811,6 +811,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned
long arg)
}
mutex_unlock(&device->rx_lock);
+ break;
default:
retval = -EINVAL;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: fix missing break in switch statement.
Hi Colin,
thanks for clarification.
Should I switch to that repo for further work, too?
If so, can anybody provide me the link?
Do I need Gregs staging any longer, or is it kind of dead for devel on
pi433 driver?
Thank you so much!
Marcus
Am 10.11.2017 um 18:04 schrieb Colin Ian King:
> On 10/11/17 16:49, Marcus Wolf wrote:
>> Hi all!
>>
>> Tryed to cross check...
>>
>> Don't get it, sorry.
>>
>> On my private version control (my SVN), where I initially developed the
>> driver the break isn't missing.
>> Same with my git copy of Gregs staging tree. Break is there...
>>
>> Who removed it, why is it missing in Colins copy?
>>
>> Am I working on a wrong version?
>
> I was working on the latest, that got landed into linux-next. This had
> picked up some modifications from Al-Viro.
>
> Hope that clarifies things
>
> Colin
>
>>
>> marcus@Laptop-Wolf:~/staging/drivers/staging/pi433$ git remote show origin
>> * remote origin
>> Fetch URL:
>> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
>> Push URL:
>> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
>>
>>
>> Can anybody help me?
>>
>> Thanks,
>>
>> Marcus
>>
>>
>> Am 09.11.2017 um 19:19 schrieb Colin King:
>>> From: Colin Ian King
>>>
>>> The PI433_IOC_WR_RX_CFG case is missing a break and will fall through
>>> to the default case and errorenously return -EINVAL. Fix this by
>>> adding in missing break.
>>>
>>> Detected by CoverityScan, CID#1461286 ("Missing break in switch")
>>>
>>> Fixes: f81f0b5c9a30 ("pi433: sanitize ioctl")
>>> Signed-off-by: Colin Ian King
>>> ---
>>> drivers/staging/pi433/pi433_if.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/staging/pi433/pi433_if.c
>>> b/drivers/staging/pi433/pi433_if.c
>>> index 3bcb59811cdf..a960fe2e7875 100644
>>> --- a/drivers/staging/pi433/pi433_if.c
>>> +++ b/drivers/staging/pi433/pi433_if.c
>>> @@ -811,6 +811,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd,
>>> unsigned long arg)
>>> }
>>> mutex_unlock(&device->rx_lock);
>>> + break;
>>> default:
>>> retval = -EINVAL;
>>> }
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> kernel-janitors" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
staging: pi433: Possible bug in rf69.c
Hi everybody! Just comparing the master of Gregs statging of pi433 with my local SVN to review all changes, that were done the last monthes. I am not sure, but maybe we imported a bug in rf69.c lines 378 and following: Gregs repo: case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_AUTO) ); case max:return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX) ); case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_6) ); case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_12) ); case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_24) ); case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_36) ); case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_48) ); my repo: case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_AUTO) ); case max:return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX) ); case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_6) ); case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_12) ); case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_24) ); case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_36) ); case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_48) ); Up to my opinion, my (old) version is better then Gregs (new) version. If you agree, I'll prepare a patch, to revert the modification. Thanks, Marcus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: pi433: Possible bug in rf69.c
Hi Dan,
I checked it on my local SVN. You are right. I submitted the code with '&'.
Accodring to a check-in message on my SVN, there was a bugreport end of
July and most probably a patch - either from me, you, Joseph Wright,
Colin King or Julia Lawall, changing '&' to '|'. I guess the patch for
some reason wasn't accepted, but fortunatley I introduced the change to
my SVN.
So from my point of view, we need a change from '&' to '|'.
I could prepare such a patch, but I am still unsure, which repo to use.
Shortly befor I fell ill, you proposed me to use Gregs staging for my
further development. But Colin yesterday was working on a repo, called
linux-next.
Can you (or anyone else) please tell me, when (or for which kind of
patches) to use the Gregs staging and wehen (or for which kind of
patches) to use the linux-next? Sorry for not being familiar with that
stuff!
Thanks a lot,
Marcus
Am 10.11.2017 um 20:32 schrieb Dan Carpenter:
> On Fri, Nov 10, 2017 at 06:23:32PM +0100, Marcus Wolf wrote:
>> Hi everybody!
>>
>> Just comparing the master of Gregs statging of pi433 with my local SVN
>> to review all changes, that were done the last monthes.
>>
>> I am not sure, but maybe we imported a bug in rf69.c lines 378 and
>> following:
>>
>> Gregs repo:
>> case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>> ~MASK_LNA_GAIN) & LNA_GAIN_AUTO) );
>> my repo:
>> case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>> ~MASK_LNA_GAIN) | LNA_GAIN_AUTO) );
>
> I edited the lines for clarity. The difference is that your repo does
> a bitwise OR "| LNA_GAIN_AUTO" and the kernel.org code does a bitwise
> "& LNA_GAIN_AUTO".
>
> The kernel repo hasn't changed since you sent us the driver in commit
> 874bcba65f9a ('staging: pi433: New driver'). I agree that & doesn't
> seem to make sense and I'm disapointed that it doesn't cause a Smatch
> warning.
>
> But LNA_GAIN_AUTO is zero so maybe | BIT(LNA_GAIN_AUTO) was intended
> instead of | LNA_GAIN_AUTO. I don't know... No one on this list knows
> the answer probably. :/
>
> regards,
> dan caprenter
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: pi433: Possible bug in rf69.c
Hi Dan, thanks fot the link. I can't remeber, why and what I wanted to redo. Maybe there was a complaint about the format of the patch... In that patch, we also have the topic with the '>> 3', we were discussing a few days ago! I'd suggest, not to invest the history any more. I'm ok with preparing a new patch/new patches, so we can import the fixes. I also have several improvements for the rf69.c, I'd like to offer. But I still need to know when to use staging and when to use linux-next. I don't want to prepare patches for the wrong tree. Cheers, Marcus Am 11.11.2017 um 10:45 schrieb Dan Carpenter: On Sat, Nov 11, 2017 at 08:55:30AM +0100, Marcus Wolf wrote: Hi Dan, I checked it on my local SVN. You are right. I submitted the code with '&'. Accodring to a check-in message on my SVN, there was a bugreport end of July and most probably a patch - either from me, you, Joseph Wright, Colin King or Julia Lawall, changing '&' to '|'. I guess the patch for some reason wasn't accepted, but fortunatley I introduced the change to my SVN. You sent the patch, but then talked about sending a new version so that's why it wasn't merged. Greg probably would have merged it as-is if it hadn't sounded like you were going to redo it. http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2017-July/108821.html regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: pi433: Possible bug in rf69.c
Hi Greg, that's fine. Is this the right URL: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git Is there already an aprox. date, when 4.15rc1 will be out and backintegration will be done? Thx, Marcus Am 11.11.2017 um 13:18 schrieb Greg Kroah-Hartman: On Sat, Nov 11, 2017 at 11:42:09AM +0200, Marcus Wolf wrote: Hi Dan, thanks fot the link. I can't remeber, why and what I wanted to redo. Maybe there was a complaint about the format of the patch... In that patch, we also have the topic with the '>> 3', we were discussing a few days ago! I'd suggest, not to invest the history any more. I'm ok with preparing a new patch/new patches, so we can import the fixes. I also have several improvements for the rf69.c, I'd like to offer. But I still need to know when to use staging and when to use linux-next. I don't want to prepare patches for the wrong tree. I recommend waiting for 4.15-rc1 to come out, all of the different trees will be merged, and then you can just work off of the staging-next tree and I can take the patches. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: pi433: Possible bug in rf69.c
Hi Greg, ok. I'll postpone all my work until then. Give me a hook, when I can start :-) Thanks, Marcus Am 11.11.2017 um 13:49 schrieb Greg Kroah-Hartman: On Sat, Nov 11, 2017 at 01:42:27PM +0200, Marcus Wolf wrote: Hi Greg, that's fine. Is this the right URL: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git Yes. Is there already an aprox. date, when 4.15rc1 will be out and backintegration will be done? Should be 2 weeks from now. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: pi433: Possible bug in rf69.c
Hi Joe,
thank you for your suggestion.
The enums are necessary for the (old fashioned) ioctl interface, too.
So the user space uses these enums in order to configure the driver.
If we want to completely remove rf69_enum.h, we need to find a solution
for that, too.
From the optics/readability, I like your idea with the Macro for the cases.
On the other hand, I have already prepared a patch, that uses setbit,
resetbit
and readmodifywrite inline fuctions instead of the macros WRITE_REG, ...
That was an idea of Walter Harms in order to increase readability and
reduce macros, because Walter prefers inline functions to macros.
As discussed with Greg, I will provide the patch, as soon as 4.15rc1 is out.
Maybe we should move the discussion to then, so you can have a look to that?
Cheers,
Marcus
Am 11.11.2017 um 18:02 schrieb Joe Perches:
On Fri, 2017-11-10 at 18:23 +0100, Marcus Wolf wrote:
Hi everybody!
Just comparing the master of Gregs statging of pi433 with my local SVN
to review all changes, that were done the last monthes.
I am not sure, but maybe we imported a bug in rf69.c lines 378 and
following:
Gregs repo:
case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) & LNA_GAIN_AUTO) );
case max:return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) & LNA_GAIN_MAX) );
case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_6) );
case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_12) );
case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_24) );
case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_36) );
case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_48) );
my repo:
case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) | LNA_GAIN_AUTO) );
case max:return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) | LNA_GAIN_MAX) );
case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_6) );
case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_12) );
case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_24) );
case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_36) );
case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_48) );
Up to my opinion, my (old) version is better then Gregs (new) version.
If you agree, I'll prepare a patch, to revert the modification.
There seems to be a lot of enum/#define duplication in this driver.
For instance:
drivers/staging/pi433/rf69_registers.h
#define LNA_GAIN_AUTO 0x00 /* default */
#define LNA_GAIN_MAX 0x01
#define LNA_GA
IN_MAX_MINUS_6 0x02
#define LNA_GAIN_MAX_MINUS_12
0x03
#define LNA_GAIN_MAX_MINUS_24
0x04
#define LNA_GAIN_MAX_MINUS_36 0x05
#d
efine LNA_GAIN_MAX_MINUS_480x06
vs
drivers/staging/pi433/rf69_enum.h
enum lnaGain
{
automatic,
max,
maxMinus6,
maxMinus12,
maxM
inus24,
maxMinus36,
maxMinus48,
undefined
};
My suggestion would be to remove drivers/staging/pi433/rf69_enum.h
where possible and convert all these switch/case entries into
macros like
#define GAIN_CASE(type) \
case type: return WRITE_REG(REG_LNA,\
(READ_REG(REG_LNA) & ~MASK_LNA_GAIN) |
(type));
so for example this switch becomes
switch (lnaGain) {
GAIN_CASE(LNA_GAIN_AUTO);
...
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: staging: pi433: Possible bug in rf69.c
Hi Greg, don't know, wether that's best option. With that procedure, it will be very hard, to integrate large patches, if the owner of the patch isn't dealing with kernel source in his daily business and thus isn't able to react on new releases within no time. I've seen the release of 4.15rc1 on Tuesday and already pulled the new head. But I am busy with my customer (proprietary software for an really ancient µC), so I won't be able to prepare my patches before weekend. The patches will touch almost every function in rf69.c, since they change some basic concepts over there. Cheers, Marcus Am 30.11.2017 um 19:12 schrieb Greg KH: On Thu, Nov 30, 2017 at 06:01:46PM +0100, Marcin Ciupak wrote: On Sat, Nov 11, 2017 at 01:51:10PM +0200, Marcus Wolf wrote: Hi Marcus, since 4.15-rc1 is out I would like to ask if you are going to provide your changes anytime soon? I would like to send a few patches as well and do not want to block your work. Just send patches, first one to my inbox always wins, don't wait for someone else :) greg k-h -- 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: pi433: Bugfix for shift operation in rf69_get_modulation()
From: root
Defines used in cases contain already shifted bits, so currentValue must not be
shifted.
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 e69a215..12c9df9 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) {
case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
default:return undefined;
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: Bugfix for shift operation in rf69_get_modulation()
Hi Greg, yes, of course. Just got the wrong console to enter the commit command - but when I recognized, it was already too late :-/ The patch already was generated in user console. I am wondering myself, where this From: comes from. I used 'git send-email -1 --annotate' How to prevent the From: line? Should I resend the patch? Thanks, Marcus Am 02.12.2017 um 12:06 schrieb Greg KH: On Sat, Dec 02, 2017 at 11:58:12AM +0200, Marcus Wolf wrote: From: root I think something went wrong here :) Also, you should never need to do kernel development as root... greg k-h -- 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: pi433: Bugfix for shift operation in rf69_get_modulation()
Defines used in cases contain already shifted bits, so currentValue must not be
shifted.
signed_of_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 e69a215..12c9df9 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) {
case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
default:return undefined;
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: pi433: Bugfix for shift operation in rf69_get_modulation()
Defines used in cases contain already shifted bits, so currentValue must not be
shifted.
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 e69a215..12c9df9 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) {
case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
default:return undefined;
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: pi433: rf69.c: Introduced define DEBUG_FUNC_ENTRY
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.
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(&spi->dev, "set: mode");
#endif
@@ -63,7 +65,7 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
int rf69_set_data_mode(struct spi_device *spi, enum dataMode dataMode)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: data mode");
#endif
@@ -79,7 +81,7 @@ int rf69_set_data_mode(struct spi_device *spi, enum dataMode
dataMode)
int rf69_set_modulation(struct spi_device *spi, enum modulation modulation)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: modulation");
#endif
@@ -96,7 +98,7 @@ enum modulation rf69_get_modulation(struct spi_device *spi)
{
u8 currentValue;
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "get: mode");
#endif
@@ -111,7 +113,7 @@ enum modulation rf69_get_modulation(struct spi_device *spi)
int rf69_set_modulation_shaping(struct spi_device *spi, enum modShaping
modShaping)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: mod shaping");
#endif
@@ -145,7 +147,7 @@ int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate)
u8 msb;
u8 lsb;
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: bit rate");
#endif
@@ -177,18 +179,18 @@ int rf69_set_bit_rate(struct spi_device *spi, u16 bitRate)
int rf69_set_deviation(struct spi_device *spi, u32 deviation)
{
int retval;
-// u32 f_max; TODO: Abh�ngigkeit von Bitrate beachten!!
+// u32 f_max; TODO: Abhaengigkeit von Bitrate beachten!!
u64 f_reg;
u64 f_step;
u8 msb;
u8 lsb;
u64 factor = 100; // to improve precision of calculation
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: deviation");
#endif
- if (deviation < 600 || deviation > 50) { //TODO: Abh�ngigkeit von
Bitrate beachten!!
+ if (deviation < 600 || deviation > 50) { //TODO: Abhaengigkeit von
Bitrate beachten!!
dev_dbg(&spi->dev, "set_deviation: illegal input param");
return -EINVAL;
}
@@ -233,7 +235,7 @@ int rf69_set_frequency(struct spi_device *spi, u32
frequency)
u8 lsb;
u64 factor = 100; // to improve precision of calculation
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: frequency");
#endif
@@ -274,7 +276,7 @@ int rf69_set_frequency(struct spi_device *spi, u32
frequency)
int rf69_set_amplifier_0(struct spi_device *spi, enum optionOnOff optionOnOff)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: amp #0");
#endif
@@ -289,7 +291,7 @@ int rf69_set_amplifier_0(struct spi_device *spi, enum
optionOnOff optionOnOff)
int rf69_set_amplifier_1(struct spi_device *spi, enum optionOnOff optionOnOff)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: amp #1");
#endif
@@ -304,7 +306,7 @@ int rf69_set_amplifier_1(struct spi_device *spi, enum
optionOnOff optionOnOff)
int rf69_set_amplifier_2(struct spi_device *spi, enum optionOnOff optionOnOff)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FUNC_ENTRY
dev_dbg(&spi->dev, "set: amp #2");
#endif
@@ -319,11 +321,11 @@ int rf69_set_amplifier_2(struct spi_device *spi, enum
optionOnOff optionOnOff)
int rf69_set_output_power_level(struct spi_device *spi, u8 powerLevel)
{
- #ifdef DEBUG
+ #ifdef DEBUG_FU
[PATCH] staging: pi433: rf69.c: Replace macros READ_REG and WRITE_REG with inline functions setBit rstBit and rmwBit
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.
As a bonus, with this refactoring a lot of lines were shortened a lot. So
some of them now undershoot 80 chars, thus reducing the total number of
complaints of checkPatch.pl in rf69.c.
---
drivers/staging/pi433/rf69.c | 347 ++
1 file changed, 185 insertions(+), 162 deletions(-)
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 0df084e..f6d0b82 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -30,13 +30,37 @@
#include "rf69.h"
#include "rf69_registers.h"
#define F_OSC 3200 /* in Hz */
#define FIFO_SIZE 66 /* in byte */
/*-*/
-#define READ_REG(x)rf69_read_reg (spi, x)
-#define WRITE_REG(x, y)rf69_write_reg(spi, x, y)
+inline static int setBit(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);
+}
+
+inline static int rstBit(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);
+}
+
+inline static int rmw(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);
+}
/*-*/
@@ -47,11 +71,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 rmw(spi, REG_OPMODE, MASK_OPMODE_MODE,
OPMODE_MODE_TRANSMIT);
+ case receive: return rmw(spi, REG_OPMODE, MASK_OPMODE_MODE,
OPMODE_MODE_RECEIVE);
+ case synthesizer: return rmw(spi, REG_OPMODE, MASK_OPMODE_MODE,
OPMODE_MODE_SYNTHESIZER);
+ case standby: return rmw(spi, REG_OPMODE, MASK_OPMODE_MODE,
OPMODE_MODE_STANDBY);
+ case mode_sleep: return rmw(spi, REG_OPMODE, MASK_OPMODE_MODE,
OPMODE_MODE_SLEEP);
default:
dev_dbg(&spi->dev, "set: illegal input param");
return -EINVAL;
@@ -70,9 +94,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 rmw(spi, REG_DATAMODUL,
MASK_DATAMODUL_MODE, DATAMODUL_MODE_PACKET);
+ case continuous:return rmw(spi, REG_DATAMODUL,
MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS);
+ case continuousNoSync: return rmw(spi, REG_DATAMODUL,
MASK_DATAMODUL_MODE, DATAMODUL_MODE_CONTINUOUS_NOSYNC);
default:
dev_dbg(&spi->dev, "set: illegal input param");
return -EINVAL;
@@ -86,8 +110,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 rmw(spi, REG_DATAMODUL,
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_OOK);
+ case FSK: return rmw(spi, REG_DATAMODUL,
MASK_DATAMODUL_MODULATION_TYPE, DATAMODUL_MODULATION_TYPE_FSK);
default:
dev_dbg(&spi->dev, "set: illegal input param");
[PATCH] staging: pi433: Removed some obsolete or duplicated defines; moved two defines to better locations
The define FIFO_SIZE was moved to rf69_registers.h. Although it is not a register, it is a value, that is given by hardware (like the registers). The define FIFO_THRESHOLD was moved to pi433_if.c, since it is a value, that is freely choosen by the interface implementation. The better the response time of the driver, the lower threshold can be set. Signed-off-by: Marcus Wolf --- drivers/staging/pi433/pi433_if.c |1 + drivers/staging/pi433/rf69.c |1 - drivers/staging/pi433/rf69.h |5 - drivers/staging/pi433/rf69_registers.h |5 + 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c index 2a205c6..292f121 100644 --- a/drivers/staging/pi433/pi433_if.c +++ b/drivers/staging/pi433/pi433_if.c @@ -59,6 +59,7 @@ #define MAX_MSG_SIZE 900 /* min: FIFO_SIZE! */ #define MSG_FIFO_SIZE 65536 /* 65536 = 2^16 */ #define NUM_DIO2 +#define FIFO_THRESHOLD 15 /* in byte */ static dev_t pi433_dev; static DEFINE_IDR(pi433_idr); diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c index 767b565..ec4b540 100644 --- a/drivers/staging/pi433/rf69.c +++ b/drivers/staging/pi433/rf69.c @@ -31,7 +31,6 @@ #include "rf69_registers.h" #define F_OSC 3200 /* in Hz */ -#define FIFO_SIZE 66 /* in byte */ /*-*/ diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h index 5c0c956..645c8df 100644 --- a/drivers/staging/pi433/rf69.h +++ b/drivers/staging/pi433/rf69.h @@ -20,11 +20,6 @@ #include "rf69_enum.h" #include "rf69_registers.h" -#define F_OSC 3200 /* in Hz */ -#define FREQUENCY 43392 /* in Hz, modifying this value impacts CE certification */ -#define FIFO_SIZE 66 /* in byte */ -#define FIFO_THRESHOLD 15 /* 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_modulation(struct spi_device *spi, enum modulation modulation); diff --git a/drivers/staging/pi433/rf69_registers.h b/drivers/staging/pi433/rf69_registers.h index 6335d42..cffafd1 100644 --- a/drivers/staging/pi433/rf69_registers.h +++ b/drivers/staging/pi433/rf69_registers.h @@ -16,6 +16,11 @@ */ /***/ +/* size of the hardware fifo */ +/***/ +#define FIFO_SIZE 66 + +/***/ /* RF69 register addresses*/ /***/ #define REG_FIFO 0x00 -- 1.7.10.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static
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);
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/5] staging: pi433: rf69.c style fix - code indent should use tabs
Revieved-by: Marcus Wolf Am 11.10.2017 um 21:24 schrieb Marcin Ciupak: This patch fixes the following checkpatch.pl error: ERROR: code indent should use tabs where possible in rf69.c file as requested by TODO file. Additionally some style warnings remain valid here and could be fixed by another patch. Signed-off-by: Marcin Ciupak --- 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 23d609474836..6420d1b67ccc 100644 --- a/drivers/staging/pi433/rf69.c +++ b/drivers/staging/pi433/rf69.c @@ -959,8 +959,8 @@ int rf69_read_fifo (struct spi_device *spi, u8 *buffer, unsigned int size) /* prepare a bidirectional transfer */ local_buffer[0] = REG_FIFO; memset(&transfer, 0, sizeof(transfer)); - transfer.tx_buf = local_buffer; - transfer.rx_buf = local_buffer; + transfer.tx_buf = local_buffer; + transfer.rx_buf = local_buffer; transfer.len= size+1; retval = spi_sync_transfer(spi, &transfer, 1); -- Marcus Wolf Wolf-Entwicklungen Helene-Lange-Weg 23 80637 München ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: pi433: Style fix - spaces required
Leftover changes from patch [PATCH 2/5] staging: pi433: rf69.c style fix - spaces required around from Marcin Ciupak (11.10.2017) Signed-of-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 7a1f8d1..35bb3f9 100644 --- a/drivers/staging/pi433/rf69.c +++ b/drivers/staging/pi433/rf69.c @@ -633,7 +633,7 @@ int rf69_set_dio_mapping(struct spi_device *spi, u8 DIONumber, u8 value) } // read reg - regValue=rf69_read_reg(spi, regaddr); + regValue = rf69_read_reg(spi, regaddr); // delete old value regValue = regValue & ~mask; // add new value -- 1.7.10.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: Removed some obsolete or duplicated defines; moved two defines to better locations
Hi Greg, for me the action was "clean up the defines in rf69.c". So for me it was fine, that two defines were moved and several other deleted at the same. Is it ok for you, or should I split the patch belated? Cheers, Marcus Am 02.12.2017 um 17:55 schrieb Greg KH: On Sat, Dec 02, 2017 at 05:14:08PM +0200, Marcus Wolf wrote: The define FIFO_SIZE was moved to rf69_registers.h. Although it is not a register, it is a value, that is given by hardware (like the registers). The define FIFO_THRESHOLD was moved to pi433_if.c, since it is a value, that is freely choosen by the interface implementation. The better the response time of the driver, the lower threshold can be set. Shouldn't this be two separate patches? Remember, each patch just does one thing. thanks, greg k-h -- 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static
Hi! I am sorry :-/ Can you recover, or do I need to resend? Cheers, Marcus Am 02.12.2017 um 17:56 schrieb Greg KH: On Sat, Dec 02, 2017 at 05:20:22PM +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 No blank line before signed-off-by :( -- Marcus Wolf Wolf-Entwicklungen Helene-Lange-Weg 23 80637 München ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: Removed some obsolete or duplicated defines; moved two defines to better locations
Am 02.12.2017 um 18:09 schrieb Greg KH: A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Sat, Dec 02, 2017 at 06:00:28PM +0200, Marcus Wolf wrote: Hi Greg, for me the action was "clean up the defines in rf69.c". So for me it was fine, that two defines were moved and several other deleted at the same. Is it ok for you, or should I split the patch belated? Please split it up, and make a patch series. Don't dribble patches out over many hours as that just ensures that I will apply them in the wrong order :( Take the time, make a patch series, test that they pass the scripts that we have, and then send them all out at once. thanks, greg k-h Hi Greg, thanks for the info on patch series. I thought series are to be used if the patches are logicaly linked together. Didn't know, that patch series even is ok, if the patches deal with different topics. I am working with kernel quite rarely. So it's quite hard to do everything 100% correct. I already took a lot of time today, to try to make everything perfect. That's why I was working on these five patches for all the day (aka 7h). And there was no need to implement a single line - everything was already implemented and tested on HW in July. But 7h weren't enough or wasted: Again not a single patch was ok :-( I can understand, your time is limited, too. But with constantly having no success and constant redo from start, the motivation is fading. Don't know, whether there could be a better way, how to build up a volunteer. This way - I am sorry - is frustrating. Since I have another peace of software, that needs to be implemented (not just moved into patches) for a customer (paying for it) until tomorrow, I will leave / have to leave the hobby topic pi433 for now. Please skip all my patches and I'll give them a third try - maybe arround Christmas... Thanks, Marcus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: rf69.c: Introduced define DEBUG_FUNC_ENTRY
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(&spi->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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static
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(&spi->dev, "set: illegal input param");
re
Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static
Am 03.12.2017 um 11:56 schrieb Marcin Ciupak:
On Sat, Dec 02, 2017 at 08:46:15AM -0800, Joe Perches wrote:
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(&spi->dev,
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] 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
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(&spi->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(&spi->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(&spi->dev, "s
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h
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(&instance->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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h
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(&instance->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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/6] staging: pi433: Rename enum dataMode in rf69_enum.h
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[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
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(&spi->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(&spi->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(&spi->dev, "set: illegal input para
[PATCH V3] staging: pi433: rf69.c: Replace macros READ_REG and WRITE_REG with smarter functions
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(&spi->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(&spi->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(&spi->dev, "set: illegal input para
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 08/11] staging: pi433: Remove enum data_mode
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(&spi->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(&spi->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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 09/11] staging: pi433: Combine all rf69_set_amplifier_x()
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(&spi->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(&spi->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(&spi->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(&spi->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(&spi->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(&spi->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 antennaImpeda
Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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 (tx_cfg.enable
Re: [PATCH] staging: pi433: Fixes issue with bit shift in rf69_get_modulation
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions
>> >> 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 04/11] staging: pi433: Rename enum optionOnOff in rf69_enum.h
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 5/6] staging: pi433: Move enum option_on_off to pi433_if.h
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
staging: pi433: Plans from Smarthome-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 wa
Re: first try to send data with pi433 on Raspberry Pi
Am 11.12.2017 um 20:40 schrieb Oliver Graute:
Hello list,
I just got my pi433 working somehow on Raspberry Pi Model B Rev 2.
Here are my findings:
first I need to enabling spi in config.txt on boot partition.
dtparam=spi=on
then adding this node to bcm2835-rpi-b-rev2.dts and compile.
&spi0_gpio7 {
pi433: pi433@0 {
compatible = "Smarthome-Wolf,pi433";
reg = <0>; /* CE 0 */
#address-cells = <1>;
#size-cells = <0>;
spi-max-frequency = <1000>;
pinctrl-0 = <&pi433_pins>;
DIO0-gpio = <&gpio 24 0>;
DIO1-gpio = <&gpio 25 0>;
DIO2-gpio = <&gpio 7 0>;
status = "okay";
};
};
Then loading spi_bcm2835 and pi433 modules:
modprobe spi_bcm2835
modprobe pi433
Is this the right usage to send some data over the air?
echo 1 > /dev/pi433
[ 31.223963] spi-bcm2835 20204000.spi: registered master spi0
[ 31.224300] spi spi0.0: setup mode 0, 8 bits/w, 1000 Hz max --> 0
[ 31.224625] spi-bcm2835 20204000.spi: registered child spi0.0
[ 31.224687] spi spi0.1: setup mode 0, 8 bits/w, 12500 Hz max --> 0
[ 31.224977] spi-bcm2835 20204000.spi: registered child spi0.1
[ 34.062430] random: crng init done
[ 43.122779] pi433: module is from the staging directory, the quality is
unknown, you have been warned.
[ 43.143436] pi433 spi0.0: setup mode 0, 8 bits/w, 1000 Hz max --> 0
[ 43.143465] pi433 spi0.0: spi interface setup: mode 0x 0, 8 bits per word,
1000hz max speed
[ 43.143587] pi433 spi0.0: found pi433 (ver. 0x24)
[ 43.144027] pi433 spi0.0: DIO0 succesfully configured
[ 43.144259] pi433 spi0.0: DIO1 succesfully configured
[ 43.144274] pi433 spi0.0: set: mode
[ 43.144387] pi433 spi0.0: read 0x4 from reg 0x1
[ 43.144463] pi433 spi0.0: wrote 0x4 to reg 0x1
[ 43.144476] pi433 spi0.0: set: data mode
[ 43.144548] pi433 spi0.0: read 0x8 from reg 0x2
[ 43.144617] pi433 spi0.0: wrote 0x8 to reg 0x2
[ 43.144632] pi433 spi0.0: set: amp #0
[ 43.144703] pi433 spi0.0: read 0x9f from reg 0x11
[ 43.144772] pi433 spi0.0: wrote 0x9f to reg 0x11
[ 43.144785] pi433 spi0.0: set: amp #1
[ 43.144856] pi433 spi0.0: read 0x9f from reg 0x11
[ 43.144923] pi433 spi0.0: wrote 0x9f to reg 0x11
[ 43.144936] pi433 spi0.0: set: amp #2
[ 43.145007] pi433 spi0.0: read 0x9f from reg 0x11
[ 43.145074] pi433 spi0.0: wrote 0x9f to reg 0x11
[ 43.145088] pi433 spi0.0: set: power level
[ 43.145158] pi433 spi0.0: read 0x9f from reg 0x11
[ 43.145225] pi433 spi0.0: wrote 0x9f to reg 0x11
[ 43.145237] pi433 spi0.0: set: antenna impedance
[ 43.145308] pi433 spi0.0: read 0x8 from reg 0x18
[ 43.145376] pi433 spi0.0: wrote 0x8 to reg 0x18
[ 43.145935] pi433 pi433: created device for major 244, minor 0
[ 43.148742] pi433 pi433: thread: going to wait for new messages
Now I got hundreds messages of kind:
[ 64.221295] pi433 pi433: write: generated new msg with 2 bytes.
snip
[ 64.222671] pi433 pi433: write: generated new msg with 2 bytes.
[ 64.223882] pi433 pi433: read 2 message byte(s) from fifo queue.
[ 64.223919] pi433 spi0.0: set: mode
[ 64.224010] pi433 spi0.0: read 0x4 from reg 0x1
[ 64.224053] pi433 spi0.0: wrote 0x4 to reg 0x1
[ 64.224064] pi433 spi0.0: set: fifo threshold
[ 64.224100] pi433 spi0.0: read 0xf from reg 0x3c
[ 64.224129] pi433 spi0.0: wrote 0xf to reg 0x3c
[ 64.224159] pi433 spi0.0: 0 - 0x0
[ 64.224170] pi433 spi0.0: set: payload length
[ 64.224197] pi433 spi0.0: wrote 0x0 to reg 0x38
[ 64.224209] pi433 spi0.0: set: frequency
[ 64.224237] pi433 spi0.0: wrote 0x0 to reg 0x7
[ 64.224263] pi433 spi0.0: wrote 0x0 to reg 0x8
[ 64.224292] pi433 spi0.0: wrote 0x0 to reg 0x9
[ 64.224303] pi433 spi0.0: set: bit rate
[ 64.224331] pi433 spi0.0: wrote 0x1a to reg 0x3
[ 64.224359] pi433 spi0.0: wrote 0x88 to reg 0x4
[ 64.224370] pi433 spi0.0: set: modulation
[ 64.224404] pi433 spi0.0: read 0x8 from reg 0x2
[ 64.224431] pi433 spi0.0: wrote 0x8 to reg 0x2
[ 64.224442] pi433 spi0.0: set: deviation
[ 64.224450] pi433 spi0.0: set_deviation: illegal input param
[ 64.224458] pi433 spi0.0: set: deviation
[ 64.224466] pi433 spi0.0: set_deviation: illegal input param
[ 64.224475] pi433 spi0.0: set: DIO mapping
[ 64.224506] pi433 spi0.0: read 0x0 from reg 0x25
[ 64.224537] pi433 spi0.0: wrote 0x0 to reg 0x25
[ 64.224555] pi433 spi0.0: set: DIO mapping
[ 64.224591] pi433 spi0.0: read 0x0 from reg 0x25
[ 64.224618] pi433 spi0.0: wrote 0x0 to reg 0x25
[ 64.224637] pi433 spi0.0: set: mode
[ 64.224673] pi433 spi0.0: read 0x4 from reg 0x1
[ 64.224702] pi433 spi0.0: wrote 0xc to reg 0x1
[ 64.224718] pi433 pi433: thread: wait for packet to get sent/fifo to be empty
[ 64.224728] pi433 pi433: thread: Packet sent. Set mode to stby.
[ 64.224736] pi433 spi0.0: set: mode
[ 64.224769] pi433 spi0.0: read 0x8 from reg 0x1
[ 64.224799] pi433 spi0.0: wro
Re: [PATCH 4/8] staging: pi433: add parentheses to mask and shift
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/8] staging: pi433: use defines for shifting register values
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/8 v2] staging: pi433: use defines for shifting register values
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/8 v3] staging: pi433: use defines for shifting register values
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
rf69_get_lna_gain
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: pi433: Rename enum modShaping in rf69_enum.h
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(&instance->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(&device->rx_lock);
/* during pendig read request, change of config not
allowed */
if (device->rx_active) {
mutex_unlock(&device->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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: Pi433: Bugfix for wrong argument for sizeof() in TX thread
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(&device->tx_fifo, &buffer[position],
sizeof(buffer) - position);
+ retval = kfifo_out(&device->tx_fifo, &device->buffer[position],
sizeof(device->buffer) - position);
dev_dbg(device->dev, "read %d message byte(s) from fifo
queue.", retval);
mutex_unlock(&device->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,
- &buffer[position],
+ &device->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,
- &buffer[position],
+ &device->buffer[position],
(size - position));
position = 0; /* reset for next repetition */
}
--
1.7.10.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: pi433: replace simple switch statements
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(&spi->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(&spi->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, PAR
Re: [PATCH v3] staging: pi433: replace simple switch statements
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 > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: add descriptions for mutex locks
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. > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: add descriptions for mutex locks
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(&device->tx_fifo) ) {
dev_dbg(device->dev, "Not enough space in fifo. Need %d, but
only have"
, required
,
kfifo_avail(&device->tx_fifo) );
return -EMSGSIZE;
}
/* write the following sequence into fifo:
* - tx_cfg
* - size of message
* - message
*/
retval = kfifo_in(&device->tx_fifo, &instance->tx_cfg,
sizeof(instance->tx_cfg));
retval += kfifo_in (&device->tx_fifo, &count, sizeof(size_t));
retval += kfifo_from_user(&device->tx_fifo, buf, count, &copied);
if (retval != required ) {
dev_dbg(device->dev, "write to fifo failed, reason unknown, non
recoverable.");
return -EAGAIN;
}
/* start transfer */
wake_up_interruptible(&device->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 :)
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: add descriptions for mutex locks
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: add descriptions for mutex locks
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: add descriptions for mutex locks
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: pi433: cleanup tx_fifo locking
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(&device->tx_fifo_lock);
> -
> retval = kfifo_out(&device->tx_fifo, &tx_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(&device->tx_fifo_lock);
> continue;
> }
>
> retval = kfifo_out(&device->tx_fifo, &size, 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(&device->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(&device->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(&device->tx_fifo_lock);
> +
> + required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
> + available = kfifo_avail(&device->tx_fifo);
> + if (required > available) {
> + dev_dbg(device->dev, "write to fifo failed: %d bytes required
> but %d available",
> + required, available);
> + mutex_unlock(&device->tx_fifo_lock);
> + return -EAGAIN;
> + }
> +
> retval = kfifo_in(&device->tx_fifo, &instance->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(&device->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(&device->tx_fifo_lock);
> return -EAGAIN;
> }
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rf69_set_deviation in rf69.c (pi433 driver)
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: pi433: initialization of tx config in pi433_open()
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 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Submit of a driver for Pi433 - a radio module for Raspberry Pi
Hi! It's me again. Seems like I also need help on sending the email :-/ I checked the whitespace/line wrap problem, but couldn't find any suspicious lines. What I did: * Looked into my outbox - the copy of my mail to you seems to be okay... * I sent the patch once again (just to me) - result: Seems to be fine, too. Maybe you can bounce back my mail? Maybe I'll get an idea what went wrong, if I see the smashed code... Cheers, Marcus > Greg KH hat am 15. Juli 2017 um 13:24 > geschrieben: > > > On Sat, Jul 15, 2017 at 01:15:43PM +0200, Marcus Wolf wrote: > > Hi Greg, > > > > thanks for your reply :-) > > > > Today I moved the documentation and header files to drivers/staging/pi433 > > and > > fromated it as a single patch. > > > > Cheers, > > > > Marcus > > I still need it in a format I can apply it in (i.e. proper subject, > changelog text, signed-off-by, etc.) Can you do that? > > Also, for staging drivers, I need a TODO file, much like the existing > drivers/staging/*/TODO files saying what needs to be done with the code > in order to get it out of staging. > > And finally, your patch seemed to have the whitespace corrupted and was > line-wrapped, making it impossible to apply even if I could have :( > > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] drivers/staging/pi433: New driver (fwd)
Hi Julia,
thank you very much for your investigation!
I totally agree - on line 894 device->dev must not be used. Any suggestions? Is
there a kind of generic value, I can pass as first argument of dev_dbg() in such
cases? Or switch to printk()?
Regarding second bug:
line 688 modified as follows:
if (bytes_received > 0)
and line 654 modified as follows:
int bytes_received;
The fix will already be included in my next try, to send a well formatted patch
to Greg.
the position of the brackets is wrong throughout the complete source. The source
was developed against an other style guide. It's one of the todos to adjust the
formatting that it fully complies with the kernel coding style guide.
Cheers,
Marcus
> Julia Lawall hat am 15. Juli 2017 um 22:50 geschrieben:
>
>
> Please check on lines 894 and 688 (not shown) for the issues mentioned
> below.
>
> Note that the ifs and {s in the following code snippet don't always follow
> the kernel coding style, eg on line 901.
>
> julia
>
> -- Forwarded message --
> Date: Sun, 16 Jul 2017 04:35:49 +0800
> From: kbuild test robot
> To: kbu...@01.org
> Cc: Julia Lawall
> Subject: Re: [PATCH 1/1] drivers/staging/pi433: New driver
>
> Hi Wolf,
>
> [auto build test WARNING on staging/staging-testing]
> [also build test WARNING on v4.12 next-20170714]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Wolf-Entwicklungen/drivers-staging-pi433-New-driver/20170716-021625
> :: branch date: 2 hours ago
> :: commit date: 2 hours ago
>
> >> drivers/staging/pi433/pi433_if.c:894:18-21: ERROR: device is NULL but
> >> dereferenced.
> --
> >> drivers/staging/pi433/pi433_if.c:688:5-19: WARNING: Unsigned expression
> >> compared with zero: bytes_received >= 0
>
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 6b5d85fc273ec7c19addf7770155414da647de7e
> vim +894 drivers/staging/pi433/pi433_if.c
>
> 6b5d85fc Wolf Entwicklungen 2017-07-15 883
> 6b5d85fc Wolf Entwicklungen 2017-07-15 884 static int pi433_open(struct inode
> *inode, struct file *filp)
> 6b5d85fc Wolf Entwicklungen 2017-07-15 885 {
> 6b5d85fc Wolf Entwicklungen 2017-07-15 886 struct pi433_device *device;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 887 struct pi433_instance *instance;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 888
> 6b5d85fc Wolf Entwicklungen 2017-07-15 889 mutex_lock(&minor_lock);
> 6b5d85fc Wolf Entwicklungen 2017-07-15 890 device = idr_find(&pi433_idr,
> iminor(inode));
> 6b5d85fc Wolf Entwicklungen 2017-07-15 891
> 6b5d85fc Wolf Entwicklungen 2017-07-15 892 mutex_unlock(&minor_lock);
> 6b5d85fc Wolf Entwicklungen 2017-07-15 893 if (!device) {
> 6b5d85fc Wolf Entwicklungen 2017-07-15 @894 dev_dbg(device->dev, "device:
> minor %d unknown.\n", iminor(inode));
> 6b5d85fc Wolf Entwicklungen 2017-07-15 895 return -ENODEV;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 896 }
> 6b5d85fc Wolf Entwicklungen 2017-07-15 897
> 6b5d85fc Wolf Entwicklungen 2017-07-15 898 if (!device->rx_buffer) {
> 6b5d85fc Wolf Entwicklungen 2017-07-15 899 device->rx_buffer =
> kmalloc(MAX_MSG_SIZE, GFP_KERNEL);
> 6b5d85fc Wolf Entwicklungen 2017-07-15 900 if (!device->rx_buffer)
> 6b5d85fc Wolf Entwicklungen 2017-07-15 901 {
> 6b5d85fc Wolf Entwicklungen 2017-07-15 902 dev_dbg(device->dev,
> "open/ENOMEM\n");
> 6b5d85fc Wolf Entwicklungen 2017-07-15 903 return -ENOMEM;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 904 }
> 6b5d85fc Wolf Entwicklungen 2017-07-15 905 }
> 6b5d85fc Wolf Entwicklungen 2017-07-15 906
> 6b5d85fc Wolf Entwicklungen 2017-07-15 907 device->users++;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 908 instance =
> kzalloc(sizeof(*instance), GFP_KERNEL);
> 6b5d85fc Wolf Entwicklungen 2017-07-15 909 if (!instance)
> 6b5d85fc Wolf Entwicklungen 2017-07-15 910 {
> 6b5d85fc Wolf Entwicklungen 2017-07-15 911 kfree(device->rx_buffer);
> 6b5d85fc Wolf Entwicklungen 2017-07-15 912 device->rx_buffer = NULL;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 913 return -ENOMEM;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 914 }
> 6b5d85fc Wolf Entwicklungen 2017-07-15 915
> 6b5d85fc Wolf Entwicklungen 2017-07-15 916 /* setup instance data*/
> 6b5d85fc Wolf Entwicklungen 2017-07-15 917 instance->device = device;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 918 instance->tx_cfg.bit_rate = 4711;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 919 // TODO: fill instance->tx_cfg;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 920
> 6b5d85fc Wolf Entwicklungen 2017-07-15 921 /* instance data as context */
> 6b5d85fc Wolf Entwicklungen 2017-07-15 922 filp->private_data = instance;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 923 nonseekable_open(inode, filp);
> 6b5d85fc Wolf Entwicklungen 2017-07-15 924
> 6b5d85fc Wolf Entwicklungen 2017-07-15 925 return 0;
> 6b5d85fc Wolf Entwicklungen 2017-07-15 926 }
> 6b5d85fc Wolf Entwicklungen 2017-07-15 927
>
> ---
> 0-DA
Re: [PATCH 1/1] drivers/staging/pi433: New driver
Hi Greg, like I wrote before - unfortunally I couldn't find a git command, squashing all my commits into one single patch. Therfore I copy and pasted the patch manually. Never the less, the first three rows were copied from a patch, originally generated by git. I used git format-patch master --stdout -p > pi433_patch for the first rows and git diff master > pi433_patch fot the dif/patch itself. If someone could help me with better git commands, I would be happy :-) Thanks a lot, Marcus > Greg KH hat am 16. Juli 2017 um 12:15 > geschrieben: > > > On Sun, Jul 16, 2017 at 11:52:32AM +0200, Wolf Entwicklungen wrote: > > From: Marcus Wolf > > Date: Tue,16 Jul 2017 11:52:06 +0100 > > Subject: [PATCH 1/1] drivers/staging/pi433: New driver > > Why is this all here in the patch body? Usually git will strip this > out, but in the future, please don't put it here. > > I'll try to queue this up in a few days and let's see what breaks :) > > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: fix a precedence bug
Once again in plain text, only - sorry for frogetting to set the switch in my mailtool Hi Dan, Hi Walter, thank you for your investigation. rf69_get_modulation, rf69_get_lna_gain, rf69_set_lna_gain: Seems like I messed up a few things :-/ In the beginning, the defines were left-bound "real" values, like you can see for the LNA_GAIN values. Later on, I deceided not to use left-bound "real" values, but to leave the bits at the position, they have in the register. These two impelemtation types are mixed up here. Sorry. Result: The shift is obsolete and the LNA_GAIN defines are old-style and need to be reviesed. I will send a patch with a proposal in approx. half an hour. rf69_set_deviation: Yes Dan, bitwise negate was intended. Once again, thanks a lot for your investigation, Marcus Thanks a lot, Marcus > Dan Carpenter hat am 19. Juli 2017 um 12:26 > geschrieben: > > > Actually, please drop this patch. I looked at this one the same as I > did the other one, but I didn't look closely enough. It's the same > situation where we have two bugs that probably cancel each other out > somewhat. It maybe needs to be fixed by someone who can test the code. > > In rf69_set_lna_gain() we should be left shifting << 3 but we don't. > > regards, > dan carpenter > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] Staging: pi433: check error after kthread_run()
Hi Joseph,
tested your patch and didn't observe a problem.
Thanks for your help :-)
Marcus
> Joseph Wright hat am 16. Juli 2017 um 16:48
> geschrieben:
>
>
> Error should be checked with IS_ERR after calling kthread_run()
> instead of comparing the returned pointer to an int.
>
> Found by sparse warning:
>
> incompatible types for operation (<)
> left side has type struct task_struct *tx_task_struct
> right side has type int
>
> Signed-off-by: Joseph Wright
> ---
> drivers/staging/pi433/pi433_if.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c
> b/drivers/staging/pi433/pi433_if.c
> index 46461b4..4f724a5 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -1152,7 +1152,7 @@ static int pi433_probe(struct spi_device *spi)
> device->tx_task_struct = kthread_run(pi433_tx_thread,
> device,
> "pi433_tx_task");
> - if (device->tx_task_struct < 0)
> + if (IS_ERR(device->tx_task_struct))
> {
> dev_dbg(device->dev, "start of send thread failed");
> goto send_thread_failed;
> --
> 2.9.3
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3][staging-next] staging: pi433: Make functions rf69_set_bandwidth_intern static
Hi Colin,
thanks for your patches.
#1 is fine. Same fix was provided by Joseph Wright. I tested it and it works
fine.
#2 looks fine, too.
Conerning #3, I would suggest to declare rf69_set_dc_cut_off_frequency static,
as well. Would you prefer to remove rf69_set_dc_cut_off_frequency from the
header (rf69.h) or would you prefer to add a static there?
If you prefer to keep the line in the header, we should spend a line for
rf69_set_bandwidth_intern in the header, too.
Again thank you :-)
Marcus
> Colin King hat am 18. Juli 2017 um 15:03
> geschrieben:
>
>
> From: Colin Ian King
>
> The function rf69_set_bandwidth_intern is local to the source
> and do not need to be in global scope, so make it static. Also
> break overly wide line.
>
> Cleans up sparse warning:
> symbol 'update_share_count' was not declared. Should it be static?
>
> Signed-off-by: Colin Ian King
> ---
> drivers/staging/pi433/rf69.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e391ce777bc7..04af906476e3 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -433,7 +433,8 @@ int rf69_set_dc_cut_off_frequency_during_afc(struct
> spi_device *spi, enum dccPer
> return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
> }
>
> -int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse
> mantisse, u8 exponent)
> +static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
> + enum mantisse mantisse, u8 exponent)
> {
> u8 newValue;
>
> --
> 2.11.0
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: Fix a couple of spelling mistakes
Reviewed-by: Marcus Wolf
> Colin King hat am 18. Juli 2017 um 07:40
> geschrieben:
>
>
> From: Colin Ian King
>
> Trivial fix to spelling mistakes in dev_dbg debug messages
>
> "wiat" -> "wait"
> "fonud" -> "found"
>
> Signed-off-by: Colin Ian King
> ---
> 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 1bc478a7f49e..d9328ce5ec1d 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -618,7 +618,7 @@ pi433_tx_thread(void *data)
> }
>
> /* we are done. Wait for packet to get sent */
> - dev_dbg(device->dev, "thread: wiat for packet to get sent/fifo to be
> empty");
> + dev_dbg(device->dev, "thread: wait for packet to get sent/fifo to be
> empty");
> wait_event_interruptible(device->fifo_wait_queue,
> device->free_in_fifo == FIFO_SIZE ||
> kthread_should_stop() );
> @@ -1101,7 +1101,7 @@ static int pi433_probe(struct spi_device *spi)
> switch(retval)
> {
> case 0x24:
> - dev_dbg(&spi->dev, "fonud pi433 (ver. 0x%x)", retval);
> + dev_dbg(&spi->dev, "found pi433 (ver. 0x%x)", retval);
> break;
> default:
> dev_dbg(&spi->dev, "unknown chip version: 0x%x", retval);
> --
> 2.11.0
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] drivers/staging/pi433: New driver
Hi Greg, below you can see the report with the error on m68k. It was sent to me from a test robot of intel. According to my research, this problem occured, because there is no hardware support for floating point on the m68k (or it was configured not to use it). Therefore gcc uses an internal function, provided by libgcc. Obviously libgcc wasn't linked... I don't know how to come arround this problem by modifying my code (except reducing the accuracy of the calculation by not using floating point). Can we ignore the error or can I do something else? Maybe a special include just in case of m68k can help??!? I don't have an environment for building m68k. Cheers, Marcus > kbuild test robot hat am 17. Juli 2017 um 15:26 geschrieben: > > > Hi Marcus, > > [auto build test ERROR on staging/staging-testing] > [also build test ERROR on v4.13-rc1 next-20170717] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system] > > url: > https://github.com/0day-ci/linux/commits/Wolf-Entwicklungen/drivers-staging-pi433-New-driver/20170716-181617 > config: m68k-allyesconfig (attached as .config) > compiler: m68k-linux-gcc (GCC) 4.9.0 > reproduce: > wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross > -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=m68k > > All errors (new ones prefixed by >>): > > drivers/staging/pi433/rf69.o: In function `rf69_set_frequency': > >> rf69.c:(.text+0x9e2): undefined reference to `__udivdi3' > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1][staging-next] staging: pi433: Make functions rf69_set_dc_cut_off_frequency_intern static
Hi Colin,
mine is an aditional patch for an other function, that's pretty similar to the
one, you improoved. So we need both patches, yours and mine!
Cheers,
Marcus
> Colin Ian King hat am 20. Juli 2017 um 14:58
> geschrieben:
>
>
> On 20/07/17 12:01, Wolf Entwicklungen wrote:
> > Declare rf69_set_dc_cut_off_frequency_intern as static since it
> > is used internaly only
> >
> > Fixes: 874bcba65f9a ("staging: pi433: New driver")
> > Signed-off-by: Marcus Wolf
> >
> > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > --- a/drivers/staging/pi433/rf69.c
> > +++ b/drivers/staging/pi433/rf69.c
> > @@ -433,7 +433,7 @@
> > return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
> > }
> >
> > -int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse
> > mantisse, u8 exponent)
> > +static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum
> > mantisse mantisse, u8 exponent)
> > {
> > u8 newValue;
> > diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> > --- a/drivers/staging/pi433/rf69.h
> > +++ b/drivers/staging/pi433/rf69.h
> > @@ -41,7 +41,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);
> >
>
> This is better than my original patch, so ignore my patch "staging:
> pi433: Make functions rf69_set_bandwidth_intern static"
>
> Colin
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/1] staging: pi433: fix problem with division in rf69_set_deviation
Fixes problem with division in rf69_set_deviation
Fixes: 874bcba65f9a ("staging: pi433: New driver")
Signed-off-by: Marcus Wolf
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -221,7 +221,7 @@ int rf69_set_frequency(struct spi_device *spi, u32
frequency)
int rf69_set_frequency(struct spi_device *spi, u32 frequency)
{
int retval;
- u32 f_max;
+ u64 f_max;
u64 f_reg;
u64 f_step;
u8 msb;
@@ -238,7 +238,8 @@ int rf69_set_frequency(struct spi_device *spi, u32
frequency)
do_div(f_step, 524288); // 524288 = 2^19
// check input value
- f_max = f_step * 8388608 / factor;
+ f_max = f_step * 8388608;
+ do_div(f_max, factor);
if (frequency > f_max)
{
dev_dbg(&spi->dev, "setFrequency: illegal input param");
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Test of [PATCH 1/1] staging: pi433: fix problem with division in rf69_set_deviation
Hi!
Since I don't have an environment for m68k and I would appreciate not
having to set it up, I would be very happy, if someone could give
the patch, named in the subjext, a try on ARCH=m68k...
Thanks,
Marcus
> Geert Uytterhoeven hat am 20. Juli 2017 um 14:27
> geschrieben:
>
>
> Hi Dan,
>
> On Thu, Jul 20, 2017 at 1:37 PM, Dan Carpenter
> wrote:
> > On Thu, Jul 20, 2017 at 01:23:05PM +0200, Marcus Wolf wrote:
> >> below you can see the report with the error on m68k. It was sent to me from
> >> a
> >> test robot of intel.
> >>
> >> According to my research, this problem occured, because there is no
> >> hardware
> >> support for floating point on the m68k (or it was configured not to use
> >> it).
> >> Therefore gcc uses an internal function, provided by libgcc. Obviously
> >> libgcc
> >> wasn't linked...
> >>
> >> I don't know how to come arround this problem by modifying my code (except
> >> reducing the accuracy of the calculation by not using floating point).
> >
> > I don't see any floating point? You're not allowed to use floating
> > point in the kernel.
>
> Indeed. __udivdi3 is used for 64-bit by 32-bit division.
>
> >> Can we ignore the error or can I do something else?
> >> Maybe a special include just in case of m68k can help??!?
> >> I don't have an environment for building m68k.
>
> https://www.kernel.org/pub/tools/crosstool/
>
> > I think the answer is to use div_u64() and div64_u64 instead of
> > do_div()?
>
> do_div() is fine
> The link error is not caused by do_div(), but by not using do_div() where
> needed.
>
> > Or you could just add a depend in the Kconfig.
>
> Depend on what? !M68K? It's gonna fail on several 32-bit platforms.
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: fix bugs in register abstraction of rf69 chip
Hi Walter,
since the construction
WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) |
LNA_GAIN_MAX_MINUS_6) )
aka
WRITE_REG(regname, ( (READ_REG(regname) & ~regmask ) | vale
) )
is used nearly everywhere, I think, about using a more gneric macro like
#define READ_MODIFY_WRITE(reg, mask, value) /
WRITE_REG(reg, ((READ_REG(reg) & ~mask) | vale ))
What do you think about it?
Cheers,
Marcus
> walter harms hat am 20. Juli 2017 um 14:22 geschrieben:
>
>
>
>
> Am 19.07.2017 20:18, schrieb Wolf Entwicklungen:
> > Bugfixes for rf69_set_modulation, rf69_set_deviation, rf69_set_lna_gain and
> > rf69_get_lna_gain
> > The fixes are cross-checked with the datasheet of the rfm69cw
> >
> > Fixes: 874bcba65f9a ("staging: pi433: New driver")
> > Signed-off-by: Marcus Wolf
> >
> > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > --- a/drivers/staging/pi433/rf69.c
> > +++ b/drivers/staging/pi433/rf69.c
> > @@ -101,7 +101,7 @@ int rf69_set_modulation(struct spi_device *spi, enum
> > modulation modulation)
> >
> > currentValue = READ_REG(REG_DATAMODUL);
> >
> > - switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> 3) // TODO
> > improvement: change 3 to define
> > + switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE)
> > {
> > case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
> > case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
> > @@ -203,7 +203,7 @@ int rf69_set_deviation(struct spi_device *spi, u32
> > deviation)
> > lsb = (f_reg&0xff);
> >
> > // check msb
> > - if (msb & !FDEVMASB_MASK)
> > + if (msb & ~FDEVMASB_MASK)
> > {
> > dev_dbg(&spi->dev, "set_deviation: err in calc of msb");
> > INVALID_PARAM;
> > @@ -366,13 +366,13 @@ int rf69_set_lna_gain(struct spi_device *spi, enum
> > lnaGain lnaGain)
> > #endif
> >
> > switch(lnaGain) {
> > - case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_AUTO) );
> > - case max: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN)
> > & LNA_GAIN_MAX) );
> > - case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_6) );
> > - case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_12) );
> > - case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_24) );
> > - case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_36) );
> > - case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_48) );
> > + case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_AUTO) );
> > + case max: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN)
> > | LNA_GAIN_MAX) );
> > + case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_6) );
> > + case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_12) );
> > + case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_24) );
> > + case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_36) );
> > + case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
> > ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_48) );
> > default: INVALID_PARAM;
> > }
> > }
>
>
> looks resonable,
> some nitpicking: perhaps you can can improve readability by using a helper
> like:
>
> static int setstbit(int arg)
> {
> int get=READ_REG(REG_LNA) & ~MASK_LNA_GAIN;
> return WRITE_REG( get| arg);
>
> }
>
> so this switch would be reduced to ...
>
> case maxMinus6: return setstbit(LNA_GAIN_MAX_MINUS_6);
>
> re,
> wh
>
>
> > @@ -387,7 +387,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
> >
> > currentValue = READ_REG(REG_LNA);
> >
> > - switch (currentValue & MASK_LNA_CURRENT_GAIN >> 3) // improvement: change
> > 3 to define
> > + switch (currentValue & MASK_LNA_GAIN)
> > {
> > case LNA_GAIN_AUTO: return automatic;
> > case LNA_GAIN_MAX: return max;
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors"
> > in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Make functions rf69_set_bandwidth_intern and rf69_set_dc_cut_off_frequency_intern static
Reviewed-by: Marcus Wolf
Attention: This patch is nothing new, just a combination of Patch
[PATCH 2/3][staging-next] staging: pi433: Make functions
rf69_set_bandwidth_intern static
and
[PATCH 1/1][staging-next] staging: pi433: Make functions
rf69_set_dc_cut_off_frequency_intern static
> Colin King hat am 21. Juli 2017 um 00:33
> geschrieben:
>
>
> From: Colin Ian King
>
> The functions rf69_set_bandwidth_intern and also
> rf69_set_dc_cut_off_frequency_intern is local to the source and
> do not need to be in global scope, so make it static. Also break
> break overly wide line. Finally, remove the function declaration
> rf69_set_dc_cut_off_frequency_intern from the rf69.h header.
>
> Cleans up sparse warning:
> symbol 'update_share_count' was not declared. Should it be static?
>
> Signed-off-by: Colin Ian King
> ---
> drivers/staging/pi433/rf69.c | 7 +--
> drivers/staging/pi433/rf69.h | 1 -
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e391ce777bc7..170e9cc59cde 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -400,7 +400,9 @@ 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 WRITE_REG(reg, ( (READ_REG(reg) & ~MASK_BW_DCC_FREQ)
> | BW_DCC_16_PERCENT) );
> @@ -433,7 +435,8 @@ int rf69_set_dc_cut_off_frequency_during_afc(struct
> spi_device *spi, enum dccPer
> return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
> }
>
> -int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse
> mantisse, u8 exponent)
> +static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg,
> + enum mantisse mantisse, u8 exponent)
> {
> u8 newValue;
>
> diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> index b81e0762032e..3fdf5d3d225b 100644
> --- a/drivers/staging/pi433/rf69.h
> +++ b/drivers/staging/pi433/rf69.h
> @@ -41,7 +41,6 @@ int rf69_set_pa_ramp(struct spi_device *spi, enum paRamp
> paRamp);
> 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);
> --
> 2.11.0
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] staging: pi433: - style fix, space at start of line
Reviewed-by: Marcus Wolf
Regarding the patch for rf69.c, I'd prefer to have all mantisses allinged (all
'm' as a column below each other, all 'a' below each other, ...). For me that
improves the readability a lot. Maybe that can be acchieved somehow without
breaking the style rules - but for sure, that's just luxury :-)
Thanks to Derk for his routine piece of work :-)
Marcus
> Derek Robson hat am 22. Juli 2017 um 05:51 geschrieben:
>
>
> Fixed checkpatch errors of "please, no spaces at the start of a line"
>
> Signed-off-by: Derek Robson
> ---
> drivers/staging/pi433/rf69.c | 4 +-
> drivers/staging/pi433/rf69_enum.h | 206 +++---
> 2 files changed, 105 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index d931437f0b6a..f450bbf3fbbc 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -440,8 +440,8 @@ int rf69_set_bandwidth_intern(struct spi_device *spi, u8
> reg, enum mantisse mant
> // check value for mantisse and exponent
> if (exponent > 7) INVALID_PARAM;
> if ( (mantisse!=mantisse16) &&
> - (mantisse!=mantisse20) &&
> - (mantisse!=mantisse24) ) INVALID_PARAM;
> + (mantisse!=mantisse20) &&
> + (mantisse!=mantisse24) ) INVALID_PARAM;
>
> // read old value
> newValue = READ_REG(reg);
> diff --git a/drivers/staging/pi433/rf69_enum.h
> b/drivers/staging/pi433/rf69_enum.h
> index fbfb59bd3f3d..635629415e63 100644
> --- a/drivers/staging/pi433/rf69_enum.h
> +++ b/drivers/staging/pi433/rf69_enum.h
> @@ -20,181 +20,181 @@
>
> enum optionOnOff
> {
> - optionOff,
> - optionOn
> + optionOff,
> + optionOn
> };
>
> enum mode
> {
> - mode_sleep,
> - standby,
> - synthesizer,
> - transmit,
> - receive
> + mode_sleep,
> + standby,
> + synthesizer,
> + transmit,
> + receive
> };
>
> enum dataMode
> {
> - packet,
> - continuous,
> - continuousNoSync
> + packet,
> + continuous,
> + continuousNoSync
> };
>
> enum modulation
> {
> - OOK,
> - FSK
> + OOK,
> + FSK
> };
>
> enum modShaping
> {
> - shapingOff,
> - shaping1_0,
> - shaping0_5,
> - shaping0_3,
> - shapingBR,
> - shaping2BR
> + shapingOff,
> + shaping1_0,
> + shaping0_5,
> + shaping0_3,
> + shapingBR,
> + shaping2BR
> };
>
> enum paRamp
> {
> - ramp3400,
> - ramp2000,
> - ramp1000,
> - ramp500,
> - ramp250,
> - ramp125,
> - ramp100,
> - ramp62,
> - ramp50,
> - ramp40,
> - ramp31,
> - ramp25,
> - ramp20,
> - ramp15,
> - ramp12,
> - ramp10
> + ramp3400,
> + ramp2000,
> + ramp1000,
> + ramp500,
> + ramp250,
> + ramp125,
> + ramp100,
> + ramp62,
> + ramp50,
> + ramp40,
> + ramp31,
> + ramp25,
> + ramp20,
> + ramp15,
> + ramp12,
> + ramp10
> };
>
> enum antennaImpedance
> {
> - fiftyOhm,
> - twohundretOhm
> + fiftyOhm,
> + twohundretOhm
> };
>
> enum lnaGain
> {
> - automatic,
> - max,
> - maxMinus6,
> - maxMinus12,
> - maxMinus24,
> - maxMinus36,
> - maxMinus48,
> - undefined
> + automatic,
> + max,
> + maxMinus6,
> + maxMinus12,
> + maxMinus24,
> + maxMinus36,
> + maxMinus48,
> + undefined
> };
>
> enum dccPercent
> {
> - dcc16Percent,
> - dcc8Percent,
> - dcc4Percent,
> - dcc2Percent,
> - dcc1Percent,
> - dcc0_5Percent,
> - dcc0_25Percent,
> - dcc0_125Percent
> + dcc16Percent,
> + dcc8Percent,
> + dcc4Percent,
> + dcc2Percent,
> + dcc1Percent,
> + dcc0_5Percent,
> + dcc0_25Percent,
> + dcc0_125Percent
> };
>
> enum mantisse
> {
> - mantisse16,
> - mantisse20,
> - mantisse24
> + mantisse16,
> + mantisse20,
> + mantisse24
> };
>
> enum thresholdType
> {
> - fixed,
> - peak,
> - average
> + fixed,
> + peak,
> + average
> };
>
> enum thresholdStep
> {
> - step_0_5db,
> - step_1_0db,
> - step_1_5db,
> - step_2_0db,
> - step_3_0db,
> - step_4_0db,
> - step_5_0db,
> - step_6_0db
> + step_0_5db,
> + step_1_0db,
> + step_1_5db,
> + step_2_0db,
> + step_3_0db,
> + step_4_0db,
> + step_5_0db,
> + step_6_0db
> };
>
> enum thresholdDecrement
> {
> - dec_every8th,
> - dec_every4th,
> - dec_every2nd,
> - dec_once,
> - dec_twice,
> - dec_4times,
> - dec_8times,
> - dec_16times
> + dec_every8th,
> + dec_every4th,
> + dec_every2nd,
> + dec_once,
> + dec_twice,
> + dec_4times,
> + dec_8times,
> + dec_16times
> };
>
> enum flag
> {
> - modeSwitchCompleted,
> - readyToReceive,
> - r
Re: [PATCH 2/3] staging: pi433: - style fix, space before tabs
Reviewed-by: Marcus Wolf
>From my point of view, the rearrangement of the block of SET_CHECKED reduces
>the
readability a lot. I like same stuff to be aligned (all brakets below each other
as a column, all spi->dev below each other and so on) But if it is necessary to
fullfill the rules, we have to do it the new way.
Thanks to Derek for his routine piece of work!
Marcus
> Derek Robson hat am 22. Juli 2017 um 05:51 geschrieben:
>
>
> Fixed checkpatch errors of "no space before tabs"
>
> Signed-off-by: Derek Robson
> ---
> drivers/staging/pi433/pi433_if.c | 12 ++--
> drivers/staging/pi433/pi433_if.h | 4 ++--
> drivers/staging/pi433/rf69.c | 8
> drivers/staging/pi433/rf69.h | 6 +++---
> 4 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c
> b/drivers/staging/pi433/pi433_if.c
> index 9cdebe93657c..b9e9292c01d9 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -190,12 +190,12 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct
> pi433_rx_cfg *rx_cfg)
> SET_CHECKED(rf69_set_frequency (dev->spi, rx_cfg->frequency));
> SET_CHECKED(rf69_set_bit_rate (dev->spi, rx_cfg->bit_rate));
> SET_CHECKED(rf69_set_modulation (dev->spi, rx_cfg->modulation));
> - SET_CHECKED(rf69_set_antenna_impedance (dev->spi,
> rx_cfg->antenna_impedance));
> - SET_CHECKED(rf69_set_rssi_threshold (dev->spi,
> rx_cfg->rssi_threshold));
> - SET_CHECKED(rf69_set_ook_threshold_dec (dev->spi,
> rx_cfg->thresholdDecrement));
> - SET_CHECKED(rf69_set_bandwidth (dev->spi, rx_cfg->bw_mantisse,
> rx_cfg->bw_exponent));
> + SET_CHECKED(rf69_set_antenna_impedance(dev->spi,
> rx_cfg->antenna_impedance));
> + SET_CHECKED(rf69_set_rssi_threshold(dev->spi, rx_cfg->rssi_threshold));
> + SET_CHECKED(rf69_set_ook_threshold_dec(dev->spi,
> rx_cfg->thresholdDecrement));
> + SET_CHECKED(rf69_set_bandwidth(dev->spi, rx_cfg->bw_mantisse,
> rx_cfg->bw_exponent));
> SET_CHECKED(rf69_set_bandwidth_during_afc(dev->spi, rx_cfg->bw_mantisse,
> rx_cfg->bw_exponent));
> - SET_CHECKED(rf69_set_dagc(dev->spi, rx_cfg->dagc));
> + SET_CHECKED(rf69_set_dagc(dev->spi, rx_cfg->dagc));
>
> dev->rx_bytes_to_drop = rx_cfg->bytes_to_drop;
>
> @@ -959,7 +959,7 @@ static int pi433_release(struct inode *inode, struct file
> *filp)
>
> static int setup_GPIOs(struct pi433_device *device)
> {
> - charname[5];
> + charname[5];
> int retval;
> int i;
>
> diff --git a/drivers/staging/pi433/pi433_if.h
> b/drivers/staging/pi433/pi433_if.h
> index e6ed3cd9b2e2..aae71f029c60 100644
> --- a/drivers/staging/pi433/pi433_if.h
> +++ b/drivers/staging/pi433/pi433_if.h
> @@ -57,7 +57,7 @@
> *
> * NOTE: struct layout is the same in 64bit and 32bit userspace.
> */
> -#define PI433_TX_CFG_IOCTL_NR0
> +#define PI433_TX_CFG_IOCTL_NR0
> struct pi433_tx_cfg
> {
> __u32 frequency;
> @@ -107,7 +107,7 @@ struct pi433_tx_cfg
> *
> * NOTE: struct layout is the same in 64bit and 32bit userspace.
> */
> -#define PI433_RX_CFG_IOCTL_NR1
> +#define PI433_RX_CFG_IOCTL_NR1
> struct pi433_rx_cfg {
> __u32 frequency;
> __u16 bit_rate;
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index 7f4db9a1f39a..d931437f0b6a 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -28,8 +28,8 @@
> #include "rf69.h"
> #include "rf69_registers.h"
>
> -#define F_OSC 3200 /* in Hz */
> -#define FIFO_SIZE 66/* in byte */
> +#define F_OSC 3200 /* in Hz */
> +#define FIFO_SIZE 66 /* in byte */
>
> /*-*/
>
> @@ -885,8 +885,8 @@ int rf69_read_fifo (struct spi_device *spi, u8 *buffer,
> unsigned int size)
> /* prepare a bidirectional transfer */
> local_buffer[0] = REG_FIFO;
> memset(&transfer, 0, sizeof(transfer));
> - transfer.tx_buf = local_buffer;
> - transfer.rx_buf = local_buffer;
> + transfer.tx_buf = local_buffer;
> + transfer.rx_buf = local_buffer;
> transfer.len= size+1;
>
> retval = spi_sync_transfer(spi, &transfer, 1);
> diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> index b81e0762032e..373df64b6891 100644
> --
Re: [PATCH 1/3] staging: pi433: Style fix - align block comments
Reviewed-by: Marcus Wolf
Thanks for your work, Derek!
> Derek Robson hat am 22. Juli 2017 um 05:50 geschrieben:
>
>
> Fixed the alignment of block comments
> Found using checkpatch
>
> Signed-off-by: Derek Robson
> ---
> drivers/staging/pi433/pi433_if.c | 38 +++--
> drivers/staging/pi433/rf69.c | 10 +-
> drivers/staging/pi433/rf69_registers.h | 280
> -
> 3 files changed, 169 insertions(+), 159 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c
> b/drivers/staging/pi433/pi433_if.c
> index 1bc478a7f49e..9cdebe93657c 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -67,9 +67,11 @@ static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
> static struct class *pi433_class; /* mainly for udev to create /dev/pi433 */
>
> /* tx config is instance specific
> - so with each open a new tx config struct is needed */
> + * so with each open a new tx config struct is needed
> + */
> /* rx config is device specific
> - so we have just one rx config, ebedded in device struct */
> + * so we have just one rx config, ebedded in device struct
> + */
> struct pi433_device {
> /* device handling related values */
> dev_t devt;
> @@ -486,9 +488,10 @@ pi433_tx_thread(void *data)
> return 0;
>
> /* get data from fifo in the following order:
> -- tx_cfg
> -- size of message
> -- message */
> + * - tx_cfg
> + * - size of message
> + * - message
> + */
> mutex_lock(&device->tx_fifo_lock);
>
> retval = kfifo_out(&device->tx_fifo, &tx_cfg, sizeof(tx_cfg));
> @@ -537,23 +540,26 @@ pi433_tx_thread(void *data)
> mutex_unlock(&device->tx_fifo_lock);
>
> /* if rx is active, we need to interrupt the waiting for
> -incoming telegrams, to be able to send something.
> -We are only allowed, if currently no reception takes
> -place otherwise we need to wait for the incoming telegram
> -to finish */
> + * incoming telegrams, to be able to send something.
> + * We are only allowed, if currently no reception takes
> + * place otherwise we need to wait for the incoming telegram
> + * to finish
> + */
> wait_event_interruptible(device->tx_wait_queue,
>!device->rx_active ||
> device->interrupt_rx_allowed == true);
>
> /* prevent race conditions
> -irq will be reenabled after tx config is set */
> + * irq will be reenabled after tx config is set
> + */
> disable_irq(device->irq_num[DIO0]);
> device->tx_active = true;
>
> if (device->rx_active && rx_interrupted == false)
> {
> /* rx is currently waiting for a telegram;
> -we need to set the radio module to standby */
> + * we need to set the radio module to standby
> + */
> SET_CHECKED(rf69_set_mode(device->spi, standby));
> rx_interrupted = true;
> }
> @@ -712,9 +718,10 @@ pi433_write(struct file *filp, const char __user *buf,
> return -EMSGSIZE;
>
> /* write the following sequence into fifo:
> -- tx_cfg
> -- size of message
> -- message */
> + * - tx_cfg
> + * - size of message
> + * - message
> + */
> mutex_lock(&device->tx_fifo_lock);
> retval = kfifo_in(&device->tx_fifo, &instance->tx_cfg,
> sizeof(instance->tx_cfg));
> if ( retval != sizeof(instance->tx_cfg) )
> @@ -1269,7 +1276,8 @@ static int __init pi433_init(void)
> int status;
>
> /* If MAX_MSG_SIZE is smaller then FIFO_SIZE, the driver won't
> - work stable - risk of buffer overflow */
> + * work stable - risk of buffer overflow
> + */
> if (MAX_MSG_SIZE < FIFO_SIZE)
> return -EINVAL;
>
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e391ce777bc7..7f4db9a1f39a 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -939,8 +939,9 @@ u8 rf69_read_reg(struct spi_device *spi, u8 addr)
Re: [PATCH] staging: pi433: fix sparse warning: missing static
reviewed-by: Marcus Wolf
The fixes of this patch are fine, but there are already patches out there,
containing these fixes.
Thanks,
Marcus
> David Wittman hat am 24. Juli 2017 um 00:46 geschrieben:
>
>
> A few local functions in the pi433 module were getting flagged by Sparse
> for missing declarations, so I added static qualifiers to clean up the
> warnings.
>
> Signed-off-by: David Wittman
> ---
> drivers/staging/pi433/pi433_if.c | 4 ++--
> drivers/staging/pi433/rf69.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c
> b/drivers/staging/pi433/pi433_if.c
> index 1bc478a..46461b4 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -313,7 +313,7 @@ struct pi433_instance {
>
> /*-*/
>
> -int
> +static int
> pi433_receive(void *data)
> {
> struct pi433_device *dev = data;
> @@ -463,7 +463,7 @@ struct pi433_instance {
> return bytes_total;
> }
>
> -int
> +static int
> pi433_tx_thread(void *data)
> {
> struct pi433_device *device = data;
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e391ce7..e5eee84 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -433,7 +433,7 @@ int rf69_set_dc_cut_off_frequency_during_afc(struct
> spi_device *spi, enum dccPer
> return rf69_set_dc_cut_off_frequency_intern(spi, REG_AFCBW, dccPercent);
> }
>
> -int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum mantisse
> mantisse, u8 exponent)
> +static int rf69_set_bandwidth_intern(struct spi_device *spi, u8 reg, enum
> mantisse mantisse, u8 exponent)
> {
> u8 newValue;
>
> --
> 1.9.1
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] staging: pi433: Style fix - align block comments
Hi Dan, when I started with the RFM69CW about two years ago (at the beginning not as a driver, but everything within the applicaton) I kind of automatically produced a list of all registers of the chip (most probably by importing and reorganizing the datasheet in Excel). Everytime I need to touch a register, I started to crosscheck the names and masks and took them out of comment. All registers, I did not touch the last two years are from this autogeneration and commented. There is stil a lot of functionality of the chip, that's not supported (or not needed) by the driver. Cheers, Marcus > Dan Carpenter hat am 24. Juli 2017 um 09:41 > geschrieben: > > > On Sat, Jul 22, 2017 at 03:50:50PM +1200, Derek Robson wrote: > > + * // RegOsc1 > > + * #define OSC1_RCCAL_START 0x80 > > + * #define OSC1_RCCAL_DONE 0x40 > > + * > > Why do we have all these commented out defines? > > regards, > dan carpenter > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: fix Kconfig entry
Reviewed-by: Marcus Wolf
This is similar to patch "[PATCH -next] staging: pi433: depends on SPI" of
Randy Dunlap
> Arnd Bergmann hat am 25. Juli 2017 um 17:38 geschrieben:
>
>
> I ran into a build error with the new pi433 driver and
> CONFIG_SPI disabled:
>
> drivers/staging/pi433/pi433_if.o: In function `pi433_probe.part.6':
> pi433_if.c:(.text+0x1657): undefined reference to `spi_write_then_read'
> drivers/staging/pi433/pi433_if.o: In function `pi433_probe':
> pi433_if.c:(.text+0x1d28): undefined reference to `spi_setup'
> drivers/staging/pi433/pi433_if.o: In function `pi433_init':
>
> I'm adding a Kconfig dependency on CONFIG_SPI here to avoid that,
> and since I spot two cosmetic mistakes in the entry, I'm also
> fixing the whitespace and remove the redundant 'default n'.
>
> Fixes: 874bcba65f9a ("staging: pi433: New driver")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/pi433/Kconfig | 24
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/pi433/Kconfig b/drivers/staging/pi433/Kconfig
> index b2716b85d5af..2fd07991c871 100644
> --- a/drivers/staging/pi433/Kconfig
> +++ b/drivers/staging/pi433/Kconfig
> @@ -1,16 +1,16 @@
> config PI433
> - tristate "Pi433 - a 433MHz radio module for Raspberry Pi"
> - default n
> - ---help---
> - This option allows you to enable support for the radio module Pi433.
> + tristate "Pi433 - a 433MHz radio module for Raspberry Pi"
> + depends on SPI
> + ---help---
> + This option allows you to enable support for the radio module Pi433.
>
> - Pi433 is a shield that fits onto the GPIO header of a Raspberry Pi
> - or compatible. It extends the Raspberry Pi with the option, to
> - send and receive data in the 433MHz ISM band - for example to
> - communicate between two systems without using ethernet or bluetooth
> - or for control or read sockets, actors, sensors, widely available
> - for low price.
> + Pi433 is a shield that fits onto the GPIO header of a Raspberry Pi
> + or compatible. It extends the Raspberry Pi with the option, to
> + send and receive data in the 433MHz ISM band - for example to
> + communicate between two systems without using ethernet or bluetooth
> + or for control or read sockets, actors, sensors, widely available
> + for low price.
>
> - For details or the option to buy, please visit https://pi433.de/en.html
> + For details or the option to buy, please visit https://pi433.de/en.html
>
> - If in doubt, say N here, but saying yes most probably won't hurt
> + If in doubt, say N here, but saying yes most probably won't hurt
> --
> 2.9.0
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: use div_u64 for 64-bit division
Hi Arnd,
we already have a patch for this:
[PATCH 1/1] staging: pi433: fix problem with division in rf69_set_deviation
from 20.07.2017
Maybe I did something wrong, but my first solution was exactly like your
proposal. As far as I remeber, I wasn't able to compile it that way. Therefore I
made a little bit more complicated fix. If I did something wrong and yours is
fine, we should go for yours, because it is a shorter solution.
Thanks,
Marcus
> Arnd Bergmann hat am 28. Juli 2017 um 15:23 geschrieben:
>
>
> I ran into this link error on an ARM OABI build:
>
> drivers/staging/pi433/rf69.o: In function `rf69_set_frequency':
> rf69.c:(.text+0xc9c): undefined reference to `__udivdi3'
>
> No idea why I didn't see it with the default EABI configurations,
> but the right solution here seems to be to use div_u64()
> to get the external division implementation.
>
> Fixes: 874bcba65f9a ("staging: pi433: New driver")
> Signed-off-by: Arnd Bergmann
> ---
> 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 e391ce777bc7..e5267b5638c0 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -238,7 +238,7 @@ int rf69_set_frequency(struct spi_device *spi, u32
> frequency)
> do_div(f_step, 524288); // 524288 = 2^19
>
> // check input value
> - f_max = f_step * 8388608 / factor;
> + f_max = div_u64(f_step * 8388608, factor);
> if (frequency > f_max)
> {
> dev_dbg(&spi->dev, "setFrequency: illegal input param");
> --
> 2.9.0
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: use div_u64 for 64-bit division
Hi Dan, Thanks for the hint. I don't get, what went wrong. If I take the mail from my outbox and view it, it looks nice. Seems, I really need to look for another mailtool. But for several reasons, that's not possible at the moment (slow move of 20 domains with someting arround 80 mail adresses and several mailboxes from one provider to another within the next two monthes). Sorry for any inconvenience, Marcus > Dan Carpenter hat am 28. Juli 2017 um 16:26 > geschrieben: > > > On Fri, Jul 28, 2017 at 04:21:05PM +0200, Marcus Wolf wrote: > > Hi Arnd, > > > > we already have a patch for this: > > [PATCH 1/1] staging: pi433: fix problem with division in rf69_set_deviation > > from 20.07.2017 > > > https://patchwork.kernel.org/patch/9855261/ > > > > > Maybe I did something wrong, but my first solution was exactly like your > > proposal. As far as I remeber, I wasn't able to compile it that way. > > Therefore I > > made a little bit more complicated fix. If I did something wrong and yours > > is > > fine, we should go for yours, because it is a shorter solution. > > > > Your patch doesn't apply for one thing :( Read > Documentation/process/email-clients.rst It probably would have been Ok > otherwise. > > I'm pretty sure that Arnd's patch is going to be fine. > > regards, > dan carpenter > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Send a large patch right now or is it better to do it later?
Hi Greg, according to the proposals of Walter Harms, I revised the rf69.c: I replaced some macros with inline functions and removed some obsolete ifdefs. According to walter this will improve the resource situation. In addition the readybility is enhanced, since lines got shorter. It's a quite big change, that touched nearly every function in that file. I was testing the new code for a while now and did not observer a problem so far. But I don't have a kind of unit test, so my tests for sure didn't cover everything. Is it a good time, to submit such a change in these days, or is it prefrable to submit it later? In adition, I am a bit afraid of my current mailtool doing something unexpected... If you like, I can give it a try! Cheers, Marcus P.S. Can you process diffs fom SVN, too, or is it mandatory to create the diff with git? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: pi433: fix problem with division in rf69_set_deviation
Hi Greg,
already had a discussion concerning that patch with Dan yesterday.
I really don't know what's going on there. I detached the patch once more from
my outbox and had a very close look in an editor and looked at it with a
difftool. In my outbox the patch is fine. I really don't knwo why it reaches you
crapped.
Since there are contsant problems with my patches, I will stop sending patches
for a while. As soon as I can find time to deeply confess with the tooling, I
will start over with trying.
Concerning this patch: You can use Arnds Patch from yesterday instead:
[PATCH] staging: pi433: use div_u64 for 64-bit division
It's a bit different to my patch, but according to yesterdays discussion, it
should also fix the problem.
Sorry for any inconvenience,
Marcus
> Greg KH hat am 29. Juli 2017 um 02:01
> geschrieben:
>
>
> On Thu, Jul 20, 2017 at 05:56:36PM +0200, Marcus Wolf wrote:
> > Fixes problem with division in rf69_set_deviation
> >
> > Fixes: 874bcba65f9a ("staging: pi433: New driver")
> > Signed-off-by: Marcus Wolf
> >
> > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > --- a/drivers/staging/pi433/rf69.c
> > +++ b/drivers/staging/pi433/rf69.c
> > @@ -221,7 +221,7 @@ int rf69_set_frequency(struct spi_device *spi, u32
> > frequency)
>
> Patch is line-wrapped and does not apply :(
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1][staging-next] staging: pi433: Make functions rf69_set_dc_cut_off_frequency_intern static
Hi Greg, also had a very close look to this patch. Even in your reply I can't find any problems with line wraps or other corruptions :-/ But we have alternative patches, solving these problems as well. You e.g. could use the patch [PATCH] Make functions rf69_set_bandwidth_intern and rf69_set_dc_cut_off_frequency_intern static from Colin King 21/07/2017. It's doing exactly the same, my patch should have done. But be careful - tonight you added patch staging: pi433: Make functions rf69_set_bandwidth_intern static" Clins patch includes that changes as well! Once again sorry for my crappy patches, Marcus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Concerning [PATCH] staging: pi433: fix bugs in register abstraction of rf69 chip
Hi Greg, there is another patch from me (see subject). I sent it 19/07/2017. Most probably, there is also something wrong with the patch, but maybe you can get it to work. The patch would be important. It doesn't fix any warnings or errors of static code analysis, but it fixes true implementation errors (wrong access of registers). From my point of view, that is one of the most important changes to the code, we had during the last weeks. Thanks, Marcus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Concerning [PATCH] staging: pi433: fix bugs in register abstraction of rf69 chip
Hi Dan, the "big" patch, I was telling about yesterday will touch exactly that file. So if you are planning deep changes, we will most probably run into conflicts. If I'll find some time (maybe tomorrow), I'll try to send that long patch, so you can see. Already nervous about next failing patch :-( Cheers, Marcus > Dan Carpenter hat am 29. Juli 2017 um 12:52 > geschrieben: > > > On Sat, Jul 29, 2017 at 11:16:11AM +0200, Marcus Wolf wrote: > > Hi Greg, > > > > there is another patch from me (see subject). I sent it 19/07/2017. > > > > Most probably, there is also something wrong with the patch, but maybe you > > can > > get it to work. > > > > The patch would be important. It doesn't fix any warnings or errors of > > static > > code analysis, but it fixes true implementation errors (wrong access of > > registers). > > It does fix some static checker warnings for me... ;) And I've been > working on making add even more checker complaints inspired by that > patch but haven't finished. > > > From my point of view, that is one of the most important changes to > > the code, we had during the last weeks. > > The patch applies fine so no worries. > > Anyway, just send some patches to yourself and try applying them with > `git am`. That's what everyone is doing on the recieving end. > > regards, > dan carpenter > > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: pi433: fix problem with division in rf69_set_deviation
Hi Greg, hi Dan the toolingproblem is a little bit more complicated. Since a mainline kernel isn't directly able to boot on the RasPi, for testing I work with a Raspbian kernel. Since at the beginning, I never thought of submitting the driver, I started with a side-build project with SVN - that's stil my master... In addition, disk is full :-( So in principle nothing is really adequate to meet the need of easy participating in kernel development. I will revise my tooling, as soon as I find the time and quietness - most probably I need to setup from scratch. For the announced change in rf69.c I will try to equip an SVN-diff with apropriate headers next week, so Dan can crosscheck with his work - I hope, I will make it without errors... Have a nice weekend, Marcus > Greg KH hat am 30. Juli 2017 um 00:21 > geschrieben: > > > On Sat, Jul 29, 2017 at 10:51:15AM +0200, Marcus Wolf wrote: > > Hi Greg, > > > > already had a discussion concerning that patch with Dan yesterday. > > I really don't know what's going on there. I detached the patch once more > > from > > my outbox and had a very close look in an editor and looked at it with a > > difftool. In my outbox the patch is fine. I really don't knwo why it reaches > > you > > crapped. > > > > Since there are contsant problems with my patches, I will stop sending > > patches > > for a while. As soon as I can find time to deeply confess with the tooling, > > I > > will start over with trying. > > Just use 'git send-email' for patches if you are having problems with > your email client. And get a better email client the kernel > Documentation has a whole file just about that topic and how to do it > correctly. > > > Concerning this patch: You can use Arnds Patch from yesterday instead: > > [PATCH] staging: pi433: use div_u64 for 64-bit division > > It's a bit different to my patch, but according to yesterdays discussion, it > > should also fix the problem. > > I already took it into my tree. > > thanks, > > greg k-h > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/5] staging/pi433:Removed Coding style issues from pi433_if.h and other dependencies arising from it.
Hi! Ok. Didn't know that I have to check for such stuff. So far i was just checking the code changes, not the style of the patch itself. Will try to be more strict... Is it mandatory, that I compile the code, or is it ok, if I do the review "just" by reading? Reason for the question: If reading is ok, too, I can do a review of a simple change, even when I am abroad at a customer (my customers do not deal with linux, just comercial stuff). With compiling, I can only do them at home... Cheers, Marcus > Dan Carpenter hat am 2. August 2017 um 10:34 > geschrieben: > > > On Wed, Aug 02, 2017 at 10:08:04AM +0200, Wolf Entwicklungen wrote: > > Reviewed-by: Marcus Wolf > > > > Just reviewed, not tested. > > As far as I can see, there is no technical issue with this patch. > > You need to be a bit more strict in your reviews... There were a few > obvious problems in this patchset. These are show stoppers: > 1) Breaks git bisect > 2) Doing multiple things in the same patch > 3) No changelog > > > > > I prefer the names of the enumerations in camel case, because then they are > > a bit shorter. > > If camel case is unwanted, for sure we need that change. > > Camel case are unwanted. > > > > > Please mind the allignment. For enhanced readability of structs, I always > > try to start the type > > in the same column, the variable name in the same column and - if nneded - > > the comments in the > > same column - so you see all members of the struct optically in a kind of > > table. > > Rishabh is going to have to redo the patchset anyway so don't feel bad > about asking for changes. Put these review comments next to the change > you are complaining about. > > No top posting. > > regards, > dan carpenter > > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: pi433: remove some macros, introduce some inline functions
Hi Dan,
I get your point and I understand, that there need to be rules to
simplify the life for the maintainers...
But I honestly must confess, that at the moment, I don't have the
time for doing that. I am into two customer projects, that keep me
very busy these weeks.
The only thing, I can offer, is to remove all the lines, dealing
with the #ifdef DEBUG and leave the rest, as it is and send it again.
Then all other changes are related to the move from macro to inline...
If that helps, please let me know.
If it needs further fragmentation, it'll take something like half
a day / a day. I most probably can spent that day end of August,
begin of September the earliest.
Sorry,
Marcus
> Dan Carpenter hat am 2. August 2017 um 13:53
> geschrieben:
>
>
> I'm afraid this patch is going to need to be divided into several
> patches that do one thing per patch. And I totally get that redoing
> patches sucks... Sorry.
>
> On Wed, Aug 02, 2017 at 01:10:14PM +0200, Wolf Entwicklungen wrote:
> > According to the proposal of Walter Harms, I removed some macros
> > and added some inline functions.
> >
> > Since I used a bit more intelligent interface, this enhances
> > readability and reduces problems with checkpatch.pl at the same time.
> >
> > In addition obsolete debug ifdefs were removed.
> >
> > Signed-off-by: Marcus Wolf
> > ---
> > drivers/staging/pi433/rf69.c | 544
> > ++
> > 1 file changed, 238 insertions(+), 306 deletions(-)
> >
> > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> > --- a/drivers/staging/pi433/rf69.c
> > +++ b/drivers/staging/pi433/rf69.c
> > @@ -28,33 +28,57 @@
> > #include "rf69.h"
> > #include "rf69_registers.h"
> >
> > -#define F_OSC 3200 /* in Hz */
> > -#define FIFO_SIZE 66 /* in byte */
> > +#define F_OSC 3200 /* in Hz */
> > +#define FIFO_SIZE 66 /* in byte */
>
>
> These are unrelated white space changes so they'll need to go in a
> separate patch.
>
> >
> > /*-*/
> >
> > -#define READ_REG(x) rf69_read_reg (spi, x)
> > -#define WRITE_REG(x,y) rf69_write_reg(spi, x, y)
> > #define INVALID_PARAM \
> > { \
> > dev_dbg(&spi->dev, "set: illegal input param"); \
> > return -EINVAL; \
> > }
> >
> > +inline static int setBit(struct spi_device *spi, u8 reg, u8 mask)
>
> Don't put "inline" on functions, let the compiler decide. setBit is
> camel case and also the name is probably too generic. Add a prefix so
> it's rf69_set_bits().
>
>
> > +{
> > + u8 tempVal;
>
> Camel case.
>
> > +
> > + tempVal = rf69_read_reg (spi, reg);
>
> No space before the '(' char.
>
> > + tempVal = tempVal | mask;
> > + return rf69_write_reg(spi, reg, tempVal);
> > +}
> > +
> > +inline static int rstBit(struct spi_device *spi, u8 reg, u8 mask)
>
> Maybe clear_bits is better than reset_bit.
>
> > +{
> > + u8 tempVal;
> > +
> > + tempVal = rf69_read_reg (spi, reg);
> > + tempVal = tempVal & ~mask;
> > + return rf69_write_reg(spi, reg, tempVal);
> > +}
> > +
> > +inline static int rmw(struct spi_device *spi, u8 reg, u8 mask, u8 value)
>
> What does rmw mean? Maybe just full words here or add a comment. I
> guess read_modify_write is too long...
>
>
> > +{
> > + u8 tempVal;
> > +
> > + tempVal = rf69_read_reg (spi, reg);
> > + tempVal = (tempVal & ~mask) | value;
> > + return rf69_write_reg(spi, reg, tempVal);
> > +}
> > +
> > /*-*/
> >
> > int rf69_set_mode(struct spi_device *spi, enum mode mode)
> > {
> > - #ifdef DEBUG
> > - dev_dbg(&spi->dev, "set: mode");
> > - #endif
> >
> > + dev_dbg(&spi->dev, "set: mode");
>
> This change is unrelated. Also just delete the line, because we can
> get the same information use ftrace instead.
>
> Anyway, I glanced through the rest of the patch and I think probably
> it should be broken up like this:
>
> [patch 1] add rf69_rmw() and convert callers
> [patch 2] add rf69_set_bit and rf69_clear_bit() and convert callers
> [patch 3] convert remaining callers to use rf69_read_reg() directly
> [patch 4] get rid of #ifdef debug, deleting code as appropriate
> [patch 5] other white space changes
>
> Greg is going to apply patches as they arrive on the email list, first
> come, first served. The problem is that some patches might have
> problems so you kind of have to guess which are good patches that he
> will apply and which are bad an then write your patch on top of that.
>
> Normally, I just write my patch based on linux-next and hope for the
> best. If I have to redo it because of conflicts, that's just part of
> the process. But my patches are normally small so they don't often
> conflict.
>
> regards,
> dan carpenter
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
