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);
...
}
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
