[PATCH] staging: pi433: #define shift constants in rf69.c

[PATCH] staging: pi433: #define shift constants in rf69.c

[Joshua Abraham]

This patch completes TODO improvements in rf69.c to change shift
constants to a define.

Signed-off-by: Joshua Abraham <j.abraham1776@gmail.com>
---
 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) {
 	case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
 	case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
 	default:			    return undefined;
@@ -409,7 +409,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_CURRENT_GAIN >> SHIFT_LNA_CURRENT_GAIN) {
 	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 6335d42142fe..119df543e80e 100644
--- a/drivers/staging/pi433/rf69_registers.h
+++ b/drivers/staging/pi433/rf69_registers.h
@@ -121,6 +121,8 @@
 #define  OPMODE_MODE_RECEIVE			0x10
 
 /* RegDataModul */
+#define  SHIFT_DATAMODUL_MODE				0x03
+
 #define  MASK_DATAMODUL_MODE			0x06
 #define  MASK_DATAMODUL_MODULATION_TYPE		0x18
 #define  MASK_DATAMODUL_MODULATION_SHAPE	0x03
@@ -234,6 +236,8 @@
  */
 
 /* RegLna (0x18) */
+#define  SHIFT_LNA_CURRENT_GAIN			0x03
+
 #define  MASK_LNA_ZIN				0x80
 #define  MASK_LNA_CURRENT_GAIN			0x38
 #define  MASK_LNA_GAIN				0x07
-- 
2.15.0

Re: [PATCH] staging: pi433: #define shift constants in rf69.c

[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 <j.abraham1776@gmail.com>
> ---
>  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

Re: [PATCH] staging: pi433: #define shift constants in rf69.c

[Marcus Wolf]

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 <j.abraham1776@gmail.com>
>> ---
>>   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
> 

Re: [PATCH] staging: pi433: #define shift constants in rf69.c

[Josh Abraham]

On Wed, Nov 08, 2017 at 02:52:30PM +0300, Dan Carpenter wrote:
> 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 <j.abraham1776@gmail.com>
> > ---
> >  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...
> 

Understood. I am just excited about submitting patches.

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

I named the define to be consistent with the extant code, but I agree
that the names could be better.

> >  /* 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.
> 

I added the blank line to separate shifts from masks, but since the shift
will only be performed on the mask I supposed it isn't needed.

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

Thanks for the criticism.  I will be better.