* staging: pi433: Possible bug in rf69.c
@ 2017-11-10 17:23 Marcus Wolf
2017-11-10 19:32 ` Dan Carpenter
2017-11-11 16:02 ` Joe Perches
0 siblings, 2 replies; 14+ messages in thread
From: Marcus Wolf @ 2017-11-10 17:23 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, linux-kernel, dan.carpenter
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-10 17:23 staging: pi433: Possible bug in rf69.c Marcus Wolf
@ 2017-11-10 19:32 ` Dan Carpenter
2017-11-11 7:55 ` Marcus Wolf
2017-11-11 16:02 ` Joe Perches
1 sibling, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2017-11-10 19:32 UTC (permalink / raw)
To: Marcus Wolf; +Cc: Greg Kroah-Hartman, devel, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-10 19:32 ` Dan Carpenter
@ 2017-11-11 7:55 ` Marcus Wolf
2017-11-11 8:40 ` Dan Carpenter
2017-11-11 8:45 ` Dan Carpenter
0 siblings, 2 replies; 14+ messages in thread
From: Marcus Wolf @ 2017-11-11 7:55 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Greg Kroah-Hartman, devel, linux-kernel
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-11 7:55 ` Marcus Wolf
@ 2017-11-11 8:40 ` Dan Carpenter
2017-11-11 8:45 ` Dan Carpenter
1 sibling, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2017-11-11 8:40 UTC (permalink / raw)
To: Marcus Wolf; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Sat, Nov 11, 2017 at 08:55:30AM +0100, Marcus Wolf wrote:
> 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!
>
linux-next is a collection of staging-next and slightly over 100 other
devel trees. You can either using staging-next or linux-next, it's the
same thing basically.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-11 7:55 ` Marcus Wolf
2017-11-11 8:40 ` Dan Carpenter
@ 2017-11-11 8:45 ` Dan Carpenter
2017-11-11 9:42 ` Marcus Wolf
1 sibling, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2017-11-11 8:45 UTC (permalink / raw)
To: Marcus Wolf; +Cc: devel, Greg Kroah-Hartman, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-11 8:45 ` Dan Carpenter
@ 2017-11-11 9:42 ` Marcus Wolf
2017-11-11 11:18 ` Greg Kroah-Hartman
2017-11-11 12:10 ` Dan Carpenter
0 siblings, 2 replies; 14+ messages in thread
From: Marcus Wolf @ 2017-11-11 9:42 UTC (permalink / raw)
To: Dan Carpenter; +Cc: devel, Greg Kroah-Hartman, linux-kernel
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-11 9:42 ` Marcus Wolf
@ 2017-11-11 11:18 ` Greg Kroah-Hartman
2017-11-11 11:42 ` Marcus Wolf
2017-11-11 12:10 ` Dan Carpenter
1 sibling, 1 reply; 14+ messages in thread
From: Greg Kroah-Hartman @ 2017-11-11 11:18 UTC (permalink / raw)
To: Marcus Wolf; +Cc: Dan Carpenter, devel, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-11 11:18 ` Greg Kroah-Hartman
@ 2017-11-11 11:42 ` Marcus Wolf
2017-11-11 11:49 ` Greg Kroah-Hartman
0 siblings, 1 reply; 14+ messages in thread
From: Marcus Wolf @ 2017-11-11 11:42 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Dan Carpenter, devel, linux-kernel
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-11 11:42 ` Marcus Wolf
@ 2017-11-11 11:49 ` Greg Kroah-Hartman
2017-11-11 11:51 ` Marcus Wolf
0 siblings, 1 reply; 14+ messages in thread
From: Greg Kroah-Hartman @ 2017-11-11 11:49 UTC (permalink / raw)
To: Marcus Wolf; +Cc: Dan Carpenter, devel, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-11 11:49 ` Greg Kroah-Hartman
@ 2017-11-11 11:51 ` Marcus Wolf
2017-11-11 12:33 ` Greg Kroah-Hartman
0 siblings, 1 reply; 14+ messages in thread
From: Marcus Wolf @ 2017-11-11 11:51 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Dan Carpenter, devel, linux-kernel
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-11 9:42 ` Marcus Wolf
2017-11-11 11:18 ` Greg Kroah-Hartman
@ 2017-11-11 12:10 ` Dan Carpenter
1 sibling, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2017-11-11 12:10 UTC (permalink / raw)
To: Marcus Wolf; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Sat, Nov 11, 2017 at 11:42:09AM +0200, Marcus Wolf wrote:
> 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.
Ah, I see now that the confusion is Al's patch. Al is a law unto
himself so I don't know the answer. Normally the advice would be to
work off staging-next.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-11 11:51 ` Marcus Wolf
@ 2017-11-11 12:33 ` Greg Kroah-Hartman
0 siblings, 0 replies; 14+ messages in thread
From: Greg Kroah-Hartman @ 2017-11-11 12:33 UTC (permalink / raw)
To: Marcus Wolf; +Cc: devel, linux-kernel, Dan Carpenter
On Sat, Nov 11, 2017 at 01:51:10PM +0200, Marcus Wolf wrote:
> Hi Greg,
>
> ok.
>
> I'll postpone all my work until then. Give me a hook, when I can start :-)
I am not going to remember, sorry, I deal with over 1000 patches a week.
Just watch kernel.org for when the new kernel is released.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-10 17:23 staging: pi433: Possible bug in rf69.c Marcus Wolf
2017-11-10 19:32 ` Dan Carpenter
@ 2017-11-11 16:02 ` Joe Perches
2017-11-11 16:40 ` Marcus Wolf
1 sibling, 1 reply; 14+ messages in thread
From: Joe Perches @ 2017-11-11 16:02 UTC (permalink / raw)
To: Marcus Wolf, Greg Kroah-Hartman, devel, linux-kernel, dan.carpenter
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_48 0x06
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);
...
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: staging: pi433: Possible bug in rf69.c
2017-11-11 16:02 ` Joe Perches
@ 2017-11-11 16:40 ` Marcus Wolf
0 siblings, 0 replies; 14+ messages in thread
From: Marcus Wolf @ 2017-11-11 16:40 UTC (permalink / raw)
To: Joe Perches, Greg Kroah-Hartman, devel, linux-kernel, dan.carpenter
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_48 0x06
>
> 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);
> ...
> }
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-11-11 16:40 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-10 17:23 staging: pi433: Possible bug in rf69.c Marcus Wolf
2017-11-10 19:32 ` Dan Carpenter
2017-11-11 7:55 ` Marcus Wolf
2017-11-11 8:40 ` Dan Carpenter
2017-11-11 8:45 ` Dan Carpenter
2017-11-11 9:42 ` Marcus Wolf
2017-11-11 11:18 ` Greg Kroah-Hartman
2017-11-11 11:42 ` Marcus Wolf
2017-11-11 11:49 ` Greg Kroah-Hartman
2017-11-11 11:51 ` Marcus Wolf
2017-11-11 12:33 ` Greg Kroah-Hartman
2017-11-11 12:10 ` Dan Carpenter
2017-11-11 16:02 ` Joe Perches
2017-11-11 16:40 ` Marcus Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).