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.
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(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 069255f023c8..3f3e863e6cc8 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -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 */
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index 1a0081ebf63c..e5b23ab39c69 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -578,50 +578,6 @@ int rf69_set_dio_mapping(struct spi_device *spi, u8 dio_number, u8 value)
return rf69_write_reg(spi, dio_addr, dio_value);
}
-bool rf69_get_flag(struct spi_device *spi, enum flag flag)
-{
- switch (flag) {
- case mode_switch_completed:
- return (rf69_read_reg(spi, REG_IRQFLAGS1) & MASK_IRQFLAGS1_MODE_READY);
- case ready_to_receive:
- return (rf69_read_reg(spi, REG_IRQFLAGS1) & MASK_IRQFLAGS1_RX_READY);
- case ready_to_send:
- return (rf69_read_reg(spi, REG_IRQFLAGS1) & MASK_IRQFLAGS1_TX_READY);
- case pll_locked:
- return (rf69_read_reg(spi, REG_IRQFLAGS1) & MASK_IRQFLAGS1_PLL_LOCK);
- case rssi_exceeded_threshold:
- return (rf69_read_reg(spi, REG_IRQFLAGS1) & MASK_IRQFLAGS1_RSSI);
- case timeout:
- return (rf69_read_reg(spi, REG_IRQFLAGS1) & MASK_IRQFLAGS1_TIMEOUT);
- case automode:
- return (rf69_read_reg(spi, REG_IRQFLAGS1) & MASK_IRQFLAGS1_AUTOMODE);
- case sync_address_match:
- return (rf69_read_reg(spi, REG_IRQFLAGS1) & MASK_IRQFLAGS1_SYNC_ADDRESS_MATCH);
- case fifo_full:
- return (rf69_read_reg(spi, REG_IRQFLAGS2) & MASK_IRQFLAGS2_FIFO_FULL);
-/*
- * case fifo_not_empty:
- * return (rf69_read_reg(spi, REG_IRQFLAGS2) & MASK_IRQFLAGS2_FIFO_NOT_EMPTY);
- */
- case fifo_empty:
- return !(rf69_read_reg(spi, REG_IRQFLAGS2) & MASK_IRQFLAGS2_FIFO_NOT_EMPTY);
- case fifo_level_below_threshold:
- return (rf69_read_reg(spi, REG_IRQFLAGS2) & MASK_IRQFLAGS2_FIFO_LEVEL);
- case fifo_overrun:
- return (rf69_read_reg(spi, REG_IRQFLAGS2) & MASK_IRQFLAGS2_FIFO_OVERRUN);
- case packet_sent:
- return (rf69_read_reg(spi, REG_IRQFLAGS2) & MASK_IRQFLAGS2_PACKET_SENT);
- case payload_ready:
- return (rf69_read_reg(spi, REG_IRQFLAGS2) & MASK_IRQFLAGS2_PAYLOAD_READY);
- case crc_ok:
- return (rf69_read_reg(spi, REG_IRQFLAGS2) & MASK_IRQFLAGS2_CRC_OK);
- case battery_low:
- return (rf69_read_reg(spi, REG_IRQFLAGS2) & MASK_IRQFLAGS2_LOW_BAT);
- default:
- return false;
- }
-}
-
int rf69_set_rssi_threshold(struct spi_device *spi, u8 threshold)
{
/* no value check needed - u8 exactly matches register size */
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index 3ff5609ecf2e..78fa0b8bab8b 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -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);
diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
index b33a33a85d3b..9dc906124e98 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -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
--
2.34.1
Looks good.
Reviewed-by: Dan Carpenter <[email protected]>
On Sat, Feb 26, 2022 at 11:40:33AM +1300, Paulo Miguel Almeida wrote:
> 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.
>
> 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."
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 069255f023c8..3f3e863e6cc8 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -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);
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
On Mon, Feb 28, 2022 at 09:32:38AM +0300, Dan Carpenter wrote:
> Looks good.
>
> Reviewed-by: Dan Carpenter <[email protected]>
>
Thanks for reviewing my patch Dan.
> > 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."
>
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...
> > - 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.
>
Noted. this patch was already merged into the staging-testing. I will
send another one fixing it.
Best regards,
Paulo Almeida