Re: [PATCH v3] staging: pi433: replace simple switch statements
If there are going to be actual users in the specific near term then that's fine. Otherwise if we're talking a year out, then it's too far away to predict what will happen a year from now so we should delete the dead weight. regards, dan carpenter
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 > >
Re: [PATCH v3] staging: pi433: replace simple switch statements
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
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, PARAMP_40);
- case ramp31:
- return rf69_
[PATCH v3] staging: pi433: replace simple switch statements
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, PARAMP_40);
- case ramp31:
- return rf69_write_reg(spi, REG_PARAMP, PARAMP_31);
- case ramp25:
- return rf69_write_reg(spi, REG_PARAMP, PARAMP_25)
