staging: pi433: remove rf69_get_flag function resolving enum conflict
PATCH 1/1 staging: pi433: remove rf69_get_flag function resolving enum conflict
The reason why rf69_get_flag() existed was to provide a high-level way to obtain values out of 1 (of 2) flags registers using bit masking. The idea was to map the possible flag values found in the data sheet like shown in page 70 of the RFM69HCW datasheet.
However, due to the fact that enums values in C must be unique, there was a naming conflict on 'fifo_not_empty' which is used by the tx_start_condition enum. So the author decided to create a 'fifo_empty' one which would negate the value that comes from the flag register as the solution to that conflict (which is very confusing).
this patch removes rf69_get_flag function which subsequently solves the enum redeclaration problem so kernel developers can follow the data sheet more easily.
--- drivers/staging/pi433/pi433_if.c | 8 +++--- drivers/staging/pi433/rf69.c | 44 ------------------------------- drivers/staging/pi433/rf69.h | 1 - drivers/staging/pi433/rf69_enum.h | 20 -------------- 4 files changed, 4 insertions(+), 69 deletions(-)
drivers/staging/pi433/pi433_if.c +4 -4
@@ -434,7 +434,7 @@ static int pi433_receive(void *data)return retval;/* now check RSSI, if low wait for getting high (RSSI interrupt) */ -while (!rf69_get_flag (dev->spi, rssi_exceeded_threshold)) { +while (!(rf69_read_reg (spi,REG_IRQFLAGS1 ) &MASK_IRQFLAGS1_RSSI )) {/* allow tx to interrupt us while waiting for high RSSI */ dev->interrupt_rx_allowed = true;wake_up_interruptible (&dev->tx_wait_queue);
@@ -442,8 +442,8 @@ static int pi433_receive(void *data)/* wait for RSSI level to become high */ dev_dbg (dev->dev,"rx: going to wait for high RSSI level\n" ); retval =wait_event_interruptible (dev->rx_wait_queue, -rf69_get_flag (dev->spi, - rssi_exceeded_threshold)); +rf69_read_reg (spi,REG_IRQFLAGS1 ) + &MASK_IRQFLAGS1_RSSI );if (retval)/* wait was interrupted */ goto abort; dev->interrupt_rx_allowed = false;
@@ -510,7 +510,7 @@ static int pi433_receive(void *data)/* get payload */ while (dev->rx_position < bytes_total) { -if (!rf69_get_flag (dev->spi, payload_ready)) { +if (!(rf69_read_reg (spi,REG_IRQFLAGS2 ) &MASK_IRQFLAGS2_PAYLOAD_READY )) { retval =wait_event_interruptible (dev->fifo_wait_queue, dev->free_in_fifo <FIFO_SIZE );if (retval)/* wait was interrupted */
drivers/staging/pi433/rf69.c +0 -44
50 lines not on this page · raw message
drivers/staging/pi433/rf69.h +0 -1
@@ -42,7 +42,6 @@ int rf69_set_bandwidth_during_afc(struct spi_device *spi,int rf69_set_ook_threshold_dec (struct spi_device *spi,enum threshold_decrement threshold_decrement);int rf69_set_dio_mapping (struct spi_device *spi,u8 dio_number,u8 value); -bool rf69_get_flag (struct spi_device *spi,enum flag flag);int rf69_set_rssi_threshold (struct spi_device *spi,u8 threshold);int rf69_set_preamble_length (struct spi_device *spi,u16 preamble_length);int rf69_enable_sync (struct spi_device *spi);
drivers/staging/pi433/rf69_enum.h +0 -20
@@ -84,26 +84,6 @@ enum threshold_decrement {dec_16times }; -enum flag { - mode_switch_completed, - ready_to_receive, - ready_to_send, - pll_locked, - rssi_exceeded_threshold, - timeout, - automode, - sync_address_match, - fifo_full, -// fifo_not_empty, collision with next enum; replaced by following enum... - fifo_empty, - fifo_level_below_threshold, - fifo_overrun, - packet_sent, - payload_ready, - crc_ok, - battery_low -}; -enum fifo_fill_condition { after_sync_interrupt, always
Signature · 1 line
2.34.1
Looks good.
Quoted 15 lines · Paulo Miguel Almeida
The reason why
rf69_get_flag()existed was to provide a high-level way to obtain values out of 1 (of 2) flags registers using bit masking. The idea was to map the possible flag values found in the data sheet like shown in page 70 of the RFM69HCW datasheet.However, due to the fact that enums values in C must be unique, there was a naming conflict on '
fifo_not_empty' which is used by thetx_start_conditionenum. So the author decided to create a 'fifo_empty' one which would negate the value that comes from the flag register as the solution to that conflict (which is very confusing).this patch removes
rf69_get_flagfunction which subsequently solves the enum redeclaration problem so kernel developers can follow the data sheet more easily.
Quoted 7 lines
Signed-off-by Paulo Miguel Almeida <[email protected]>--- drivers/staging/pi433/pi433_if.c | 8 +++--- drivers/staging/pi433/rf69.c | 44 ------------------------------- drivers/staging/pi433/rf69.h | 1 - drivers/staging/pi433/rf69_enum.h | 20 -------------- 4 files changed, 4 insertions(+), 69 deletions(-)
You don't really need to write a long commit message for a commit which deletes 69 - 4 = 65 lines. Just say "Remove pointless rf69_get_flag() function and call rf69_read_reg() directly. This cleanup removes 65 lines of code and it more obvious to read."
Quoted 15 lines
drivers/staging/pi433/pi433_if.c +1
-1@@ -434,7 +434,7 @@ static int pi433_receive(void *data)return retval;/* now check RSSI, if low wait for getting high (RSSI interrupt) */ -while (!rf69_get_flag (dev->spi, rssi_exceeded_threshold)) { +while (!(rf69_read_reg (spi,REG_IRQFLAGS1 ) &MASK_IRQFLAGS1_RSSI )) {/* allow tx to interrupt us while waiting for high RSSI */ dev->interrupt_rx_allowed = true;wake_up_interruptible (&dev->tx_wait_queue);
drivers/staging/pi433/pi433_if.c static int pi433_receive(void *data) · line 446
Quoted 7 lines
drivers/staging/pi433/pi433_if.c +2
-2@@ -442,8 +442,8 @@ static int pi433_receive(void *data)/* wait for RSSI level to become high */ dev_dbg (dev->dev,"rx: going to wait for high RSSI level\n" ); retval =wait_event_interruptible (dev->rx_wait_queue, -rf69_get_flag (dev->spi, - rssi_exceeded_threshold)); +rf69_read_reg (spi,REG_IRQFLAGS1 ) + &MASK_IRQFLAGS1_RSSI );
The & character should go on the first line.
rf69_read_reg(spi, REG_IRQFLAGS1) & MASK_IRQFLAGS1_RSSI);
But that can be done in a follow on patch if you want. Or you can leave it as-is.
regards,
dan carpenter
Quoted 4 lines · Dan Carpenter
Looks good.
Reviewed-by Dan Carpenter <[email protected]>
Thanks for reviewing my patch Dan.
Quoted 7 lines
Quoted 1 line
4 files changed, 4 insertions(+), 69 deletions(-)You don't really need to write a long commit message for a commit which deletes 69 - 4 = 65 lines. Just say "Remove pointless
rf69_get_flag()function and callrf69_read_reg()directly. This cleanup removes 65 lines of code and it more obvious to read."
Thanks for the feedback. I swear I don't do that on purpose ... I have always struggled to be succint tbh. It's just something I'm actively working on...
Quoted 7 lines
drivers/staging/pi433/pi433_if.c static int pi433_receive(void *data) · line 446
Quoted 4 lines
drivers/staging/pi433/pi433_if.c +2
-2-rf69_get_flag (dev->spi, - rssi_exceeded_threshold)); +rf69_read_reg (spi,REG_IRQFLAGS1 ) + & MASK_IRQFLAGS1_RSSI);The & character should go on the first line.
Quoted 6 lines
rf69_read_reg(spi, REG_IRQFLAGS1) & MASK_IRQFLAGS1_RSSI);But that can be done in a follow on patch if you want. Or you can leave it as-is.
Noted. this patch was already merged into the staging-testing. I will send another one fixing it.
Best regards,
Paulo Almeida