Re: staging: pi433: Possible bug in rf69.c

[Joe Perches <joe@perches.com>]

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);
	...
	}