at86rf230: cleanups

PATCH Superseded Alexander Aring 2 participants 16 messages Dec 14, 2014 – Dec 15, 2014

Subsystems (2)

Network drivers IEEE 802.15.4

lore

Share & tools

lore

Alexander Aring

author maintainer

2014-12-14 23:20 UTC #0 lore raw

cover at86rf230: cleanups

This patch series contains various cleanups for the at86rf230. Except patch 3/5 which contains a fix for a rarely error handling. Because this error handling is very rarely and should only happens if something is wrong with the spi controller, it's okay to fix it in next.

- Alex

Alexander Aring (5):
at86rf230: remove if branch
at86rf230: make at86rf230_async_error inline
at86rf230: fix context pointer handling
at86rf230: cleanup check on trac status
at86rf230: remove unnecessary assign

 drivers/net/ieee802154/at86rf230.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

Signature · 1 line

2.1.3

Alexander Aring

author maintainer

2014-12-14 23:20 UTC #1 lore raw

PATCH 1/5 at86rf230: remove if branch

This patch removes an unnecessary if branch inside the tx complete handler.

Signed-off-by Alexander Aring <[email protected]>

---
 drivers/net/ieee802154/at86rf230.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/net/ieee802154/at86rf230.c +1 -4

index 1c01356..4e983b3 100644
@@ -715,10 +715,7 @@ at86rf230_tx_complete(void *context)
 
 	enable_irq(lp->spi->irq);
 
-	if (lp->max_frame_retries <= 0)
-		ieee802154_xmit_complete(lp->hw, skb, true);
-	else
-		ieee802154_xmit_complete(lp->hw, skb, false);
+	ieee802154_xmit_complete(lp->hw, skb, lp->max_frame_retries <= 0);
 }
 
 static void

Signature · 1 line

2.1.3

Stefan Schmidt

Reviewed-by on v1 1/5

2014-12-15 08:29 UTC #2 lore raw

Re: [PATCH bluetooth-next 1/5] at86rf230: remove if branch

Hello.

Quoted 14 lines · Alexander Aring

This patch removes an unnecessary if branch inside the tx complete handler.
Signed-off-by Alexander Aring <[email protected]>
---
  drivers/net/ieee802154/at86rf230.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)
drivers/net/ieee802154/at86rf230.c +0 -0
index 1c01356..4e983b3 100644

drivers/net/ieee802154/at86rf230.c at86rf230_tx_complete(void *context) · line 719

Quoted 9 lines

drivers/net/ieee802154/at86rf230.c +1 -4

@@ -715,10 +715,7 @@ at86rf230_tx_complete(void *context)
 
  	enable_irq(lp->spi->irq);
 
-	if (lp->max_frame_retries <= 0)
-		ieee802154_xmit_complete(lp->hw, skb, true);
-	else
-		ieee802154_xmit_complete(lp->hw, skb, false);
+	ieee802154_xmit_complete(lp->hw, skb, lp->max_frame_retries <= 0);
  }

It surely saves us some lines but personally I find it harder to read this way. Having the condition inside the function call breaks the reading flow for me. Is this the preferred coding style in the kernel?

Reviewed-by Stefan Schmidt <[email protected]>

regards
Stefan Schmidt

Alexander Aring

author maintainer

2014-12-15 08:45 UTC #3 lore raw

Re: [PATCH bluetooth-next 1/5] at86rf230: remove if branch

Quoted 20 lines · Stefan Schmidt

Hello.
Quoted 17 lines · Alexander Aring
This patch removes an unnecessary if branch inside the tx complete handler.
Signed-off-by Alexander Aring <[email protected]>
---
 drivers/net/ieee802154/at86rf230.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
drivers/net/ieee802154/at86rf230.c +0 -0
index 1c01356..4e983b3 100644
@@ -715,10 +715,7 @@ at86rf230_tx_complete(void *context)
 
 	enable_irq(lp->spi->irq);
 

Quoted 11 lines

drivers/net/ieee802154/at86rf230.c at86rf230_tx_complete(void *context) · line 719
Quoted 6 lines
drivers/net/ieee802154/at86rf230.c +1 -4
-	if (lp->max_frame_retries <= 0)
-		ieee802154_xmit_complete(lp->hw, skb, true);
-	else
-		ieee802154_xmit_complete(lp->hw, skb, false);
+	ieee802154_xmit_complete(lp->hw, skb, lp->max_frame_retries <= 0);
 }
It surely saves us some lines but personally I find it harder to read this way. Having the condition inside the function call breaks the reading flow for me. Is this the preferred coding style in the kernel?

I agree this is more readable, maybe this is because I wrote this at first time in this way.

But that reminds me to doing something like this:

if (bool)

	return true;
else
	return false;

instead:

return bool;

which is usually some beginners failure. I don't know if the kernel conding style really discuss about that. The checkpatch script doesn't report anything about this.

Maybe we can put something on the stack like:

bool ifs_handling = lp->max_frame_retries <= 0;

...

ieee802154_xmit_complete(lp->hw, skb, ifs_handling);

is that more readable?

- Alex

Alexander Aring

author maintainer

2014-12-15 08:49 UTC #4 lore raw

Re: [PATCH bluetooth-next 1/5] at86rf230: remove if branch

I think, I found a better solution:

static void
at86rf230_tx_complete(void *context)

{                                                
        struct at86rf230_state_change *ctx = context;
        struct at86rf230_local *lp = ctx->lp;     
        struct sk_buff *skb = lp->tx_skb;         
                                                  
        enable_irq(lp->spi->irq);                 
                                                  
        ieee802154_xmit_complete(lp->hw, skb, !lp->tx_aret);
}

lifs handling is only _inactive_ in ARET mode.

I will change it do this solution.

- Alex

Alexander Aring

author maintainer

2014-12-14 23:20 UTC #5 lore raw

PATCH 2/5 at86rf230: make at86rf230_async_error inline

In reply to Alexander Aring

This patch makes the at86rf230_async_error inline. This function is small enough to handle inline.

Signed-off-by Alexander Aring <[email protected]>

---
 drivers/net/ieee802154/at86rf230.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ieee802154/at86rf230.c +1 -1

index 4e983b3..430d3bd 100644
@@ -450,7 +450,7 @@ at86rf230_async_error_recover(void *context)
 	ieee802154_wake_queue(lp->hw);
 }
 
-static void
+static inline void
 at86rf230_async_error(struct at86rf230_local *lp,
 		      struct at86rf230_state_change *ctx, int rc)
 {

Signature · 1 line

2.1.3

Stefan Schmidt

Reviewed-by on v1 2/5

2014-12-15 08:29 UTC #6 lore raw

Re: [PATCH bluetooth-next 2/5] at86rf230: make at86rf230_async_error inline

Hello.

Quoted 14 lines · Alexander Aring

This patch makes the at86rf230_async_error inline. This function is small enough to handle inline.
Signed-off-by Alexander Aring <[email protected]>
---
  drivers/net/ieee802154/at86rf230.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ieee802154/at86rf230.c +0 -0
index 4e983b3..430d3bd 100644

drivers/net/ieee802154/at86rf230.c at86rf230_async_error_recover(void *context) · line 456

Quoted 8 lines

drivers/net/ieee802154/at86rf230.c +1 -1

@@ -450,7 +450,7 @@ at86rf230_async_error_recover(void *context)
  	ieee802154_wake_queue(lp->hw);
  }
 
-static void
+static inline void
  at86rf230_async_error(struct at86rf230_local *lp,
  		      struct at86rf230_state_change *ctx, int rc)
  {

Hopefully we would not need this error function often enough to have a real benefit for inline but with only two function calls it should be small enough anyway for inline.

Reviewed-by Stefan Schmidt <[email protected]>

regards
Stefan Schmidt

Alexander Aring

author maintainer

2014-12-15 08:39 UTC #7 lore raw

Re: [PATCH bluetooth-next 2/5] at86rf230: make at86rf230_async_error inline

Quoted 20 lines · Stefan Schmidt

Hello.
Quoted 17 lines · Alexander Aring
This patch makes the at86rf230_async_error inline. This function is small enough to handle inline.
Signed-off-by Alexander Aring <[email protected]>
---
 drivers/net/ieee802154/at86rf230.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ieee802154/at86rf230.c +0 -0
index 4e983b3..430d3bd 100644
@@ -450,7 +450,7 @@ at86rf230_async_error_recover(void *context)
 	ieee802154_wake_queue(lp->hw);
 }
 

Quoted 9 lines

drivers/net/ieee802154/at86rf230.c at86rf230_async_error_recover(void *context) · line 456
Quoted 5 lines
drivers/net/ieee802154/at86rf230.c +1 -1
-static void
+static inline void
 at86rf230_async_error(struct at86rf230_local *lp,
 		      struct at86rf230_state_change *ctx, int rc)
 {
Hopefully we would not need this error function often enough to have a real benefit for inline but with only two function calls it should be small enough anyway for inline.

With Werner Almesberger words "If this fails something goes really wrong with your spi controller and you can only save that the kernel doesn't run amok" or something like that.

I also heard that we don't need to check errors for the spi calls.

For now I don't know what happens if an error occurs here, I activate the irq again (if disabled before) and try to run some TRX_OFF to RX_AACK_ON recover, so we can receive some frames again.

But I think it depends on "error case" if this mechanism really helps.

Nevertheless, still better than doing nothing.

- Alex

Stefan Schmidt

2014-12-15 09:01 UTC #8 lore raw

Re: [PATCH bluetooth-next 2/5] at86rf230: make at86rf230_async_error inline

Hello.

Quoted 38 lines · Alexander Aring

Quoted 20 lines · Stefan Schmidt
Hello.
Quoted 17 lines · Alexander Aring
This patch makes the at86rf230_async_error inline. This function is small enough to handle inline.
Signed-off-by Alexander Aring <[email protected]>
---
  drivers/net/ieee802154/at86rf230.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ieee802154/at86rf230.c +0 -0
index 4e983b3..430d3bd 100644
@@ -450,7 +450,7 @@ at86rf230_async_error_recover(void *context)
  	ieee802154_wake_queue(lp->hw);
  }
 
Quoted 10 lines
drivers/net/ieee802154/at86rf230.c at86rf230_async_error_recover(void *context) · line 456
Quoted 5 lines
drivers/net/ieee802154/at86rf230.c +1 -1
-static void
+static inline void
  at86rf230_async_error(struct at86rf230_local *lp,
  		      struct at86rf230_state_change *ctx, int rc)
  {
Hopefully we would not need this error function often enough to have a real
benefit for inline but with only two function calls it should be small enough anyway for inline.
With Werner Almesberger words "If this fails something goes really wrong with your spi controller and you can only save that the kernel doesn't run amok" or something like that.
I also heard that we don't need to check errors for the spi calls.

Quoted 7 lines

For now I don't know what happens if an error occurs here, I activate the irq again (if disabled before) and try to run some TRX_OFF to RX_AACK_ON recover, so we can receive some frames again.
But I think it depends on "error case" if this mechanism really helps.
Nevertheless, still better than doing nothing.

Sure, handling the case is good. Just wondered about the need for inline here but as I wrote with two calls this functions is small enough I would say.

regards
Stefan Schmidt

Alexander Aring

author maintainer

2014-12-15 09:04 UTC #9 lore raw

Re: [PATCH bluetooth-next 2/5] at86rf230: make at86rf230_async_error inline

Quoted 45 lines · Stefan Schmidt

Hello.
Quoted 42 lines · Alexander Aring
Quoted 20 lines · Stefan Schmidt
Hello.
Quoted 17 lines · Alexander Aring
This patch makes the at86rf230_async_error inline. This function is small enough to handle inline.
Signed-off-by Alexander Aring <[email protected]>
---
 drivers/net/ieee802154/at86rf230.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ieee802154/at86rf230.c +0 -0
index 4e983b3..430d3bd 100644
@@ -450,7 +450,7 @@ at86rf230_async_error_recover(void *context)
 	ieee802154_wake_queue(lp->hw);
 }
 
Quoted 10 lines
drivers/net/ieee802154/at86rf230.c at86rf230_async_error_recover(void *context) · line 456
Quoted 5 lines
drivers/net/ieee802154/at86rf230.c +1 -1
-static void
+static inline void
 at86rf230_async_error(struct at86rf230_local *lp,
 		      struct at86rf230_state_change *ctx, int rc)
 {
Hopefully we would not need this error function often enough to have a real
benefit for inline but with only two function calls it should be small enough anyway for inline.
With Werner Almesberger words "If this fails something goes really wrong with your spi controller and you can only save that the kernel doesn't run amok" or something like that.
I also heard that we don't need to check errors for the spi calls.
For now I don't know what happens if an error occurs here, I activate the irq again (if disabled before) and try to run some TRX_OFF to RX_AACK_ON recover, so we can receive some frames again.

Quoted 8 lines

Quoted 3 lines
But I think it depends on "error case" if this mechanism really helps.
Nevertheless, still better than doing nothing.
Sure, handling the case is good. Just wondered about the need for inline here but as I wrote with two calls this functions is small enough I would say.

ok. :-)

- Alex

Alexander Aring

author maintainer

2014-12-14 23:20 UTC #10 lore raw

PATCH 3/5 at86rf230: fix context pointer handling

In reply to Alexander Aring

This patch changes the context pointer to the parameter given one inside function at86rf230_async_state_change_start. This could occur problem if context isn't pointed to lp->state.

Signed-off-by Alexander Aring <[email protected]>

---
 drivers/net/ieee802154/at86rf230.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ieee802154/at86rf230.c +1 -1

index 430d3bd..08b2f9f 100644
@@ -655,7 +655,7 @@ at86rf230_async_state_change_start(void *context)
 		if (ctx->irq_enable)
 			enable_irq(lp->spi->irq);
 
-		at86rf230_async_error(lp, &lp->state, rc);
+		at86rf230_async_error(lp, ctx, rc);
 	}
 }

Signature · 1 line

2.1.3

Stefan Schmidt

Reviewed-by on v1 3/5

2014-12-15 08:29 UTC #11 lore raw

Re: [PATCH bluetooth-next 3/5] at86rf230: fix context pointer handling

Hello.

Quoted 16 lines · Alexander Aring

This patch changes the context pointer to the parameter given one inside function at86rf230_async_state_change_start. This could occur problem if context isn't pointed to lp->state.
Signed-off-by Alexander Aring <[email protected]>
---
  drivers/net/ieee802154/at86rf230.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ieee802154/at86rf230.c +0 -0
index 430d3bd..08b2f9f 100644

drivers/net/ieee802154/at86rf230.c at86rf230_async_state_change_start(void *context) · line 661

Quoted 8 lines

drivers/net/ieee802154/at86rf230.c +1 -1

@@ -655,7 +655,7 @@ at86rf230_async_state_change_start(void *context)
  		if (ctx->irq_enable)
  			enable_irq(lp->spi->irq);
 
-		at86rf230_async_error(lp, &lp->state, rc);
+		at86rf230_async_error(lp, ctx, rc);
  	}
  }

Reviewed-by Stefan Schmidt <[email protected]>

regards
Stefan Schmidt

Alexander Aring

author maintainer

2014-12-14 23:20 UTC #12 lore raw

PATCH 4/5 at86rf230: cleanup check on trac status

In reply to Alexander Aring

Signed-off-by Alexander Aring <[email protected]>

---
 drivers/net/ieee802154/at86rf230.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/net/ieee802154/at86rf230.c +3 -5

index 08b2f9f..7b0a17e 100644
@@ -750,13 +750,11 @@ at86rf230_tx_trac_check(void *context)
 	 * to STATE_FORCE_TRX_OFF then STATE_TX_ON to recover the transceiver
 	 * state to TX_ON.
 	 */
-	if (trac) {
+	if (trac)
 		at86rf230_async_state_change(lp, ctx, STATE_FORCE_TRX_OFF,
 					     at86rf230_tx_trac_error, true);
-		return;
-	}
-
-	at86rf230_tx_on(context);
+	else
+		at86rf230_tx_on(context);
 }

Signature · 1 line

2.1.3

Stefan Schmidt

Reviewed-by on v1 4/5

2014-12-15 08:29 UTC #13 lore raw

Re: [PATCH bluetooth-next 4/5] at86rf230: cleanup check on trac status

Hello.

Quoted 21 lines · Alexander Aring

Signed-off-by Alexander Aring <[email protected]>

---
  drivers/net/ieee802154/at86rf230.c | 8 +++-----
  1 file changed, 3 insertions(+), 5 deletions(-)

drivers/net/ieee802154/at86rf230.c +0 -0

index 08b2f9f..7b0a17e 100644
@@ -750,13 +750,11 @@ at86rf230_tx_trac_check(void *context)
  	 * to STATE_FORCE_TRX_OFF then STATE_TX_ON to recover the

transceiver
* state to TX_ON.
*/

drivers/net/ieee802154/at86rf230.c +1 -1

-	if (trac) {
+	if (trac)
  		at86rf230_async_state_change(lp, ctx, STATE_FORCE_TRX_OFF,
  					     at86rf230_tx_trac_error,

true);

drivers/net/ieee802154/at86rf230.c at86rf230_tx_trac_check(void *context) · line 758

Quoted 7 lines

drivers/net/ieee802154/at86rf230.c +2 -4

-		return;
-	}
-
-	at86rf230_tx_on(context);
+	else
+		at86rf230_tx_on(context);
  }

Reviewed-by Stefan Schmidt <[email protected]>

regards
Stefan Schmidt

Alexander Aring

author maintainer

2014-12-14 23:20 UTC #14 lore raw

PATCH 5/5 at86rf230: remove unnecessary assign

In reply to Alexander Aring

The attribute extra_tx_headroom should already be zero.

Signed-off-by Alexander Aring <[email protected]>

---
 drivers/net/ieee802154/at86rf230.c | 1 -
 1 file changed, 1 deletion(-)

drivers/net/ieee802154/at86rf230.c +0 -1

index 7b0a17e..89589d8 100644
@@ -1405,7 +1405,6 @@ at86rf230_detect_device(struct at86rf230_local *lp)
 		return -EINVAL;
 	}
 
-	lp->hw->extra_tx_headroom = 0;
 	lp->hw->flags = IEEE802154_HW_TX_OMIT_CKSUM | IEEE802154_HW_AACK |
 			IEEE802154_HW_TXPOWER | IEEE802154_HW_ARET |
 			IEEE802154_HW_AFILT | IEEE802154_HW_PROMISCUOUS;

Signature · 1 line

2.1.3

Stefan Schmidt

Reviewed-by on v1 5/5

2014-12-15 08:29 UTC #15 lore raw

Re: [PATCH bluetooth-next 5/5] at86rf230: remove unnecessary assign

Hello.

Quoted 14 lines · Alexander Aring

The attribute extra_tx_headroom should already be zero.
Signed-off-by Alexander Aring <[email protected]>
---
  drivers/net/ieee802154/at86rf230.c | 1 -
  1 file changed, 1 deletion(-)
drivers/net/ieee802154/at86rf230.c +0 -0
index 7b0a17e..89589d8 100644

drivers/net/ieee802154/at86rf230.c at86rf230_detect_device(struct at86rf230_local *lp) · line 1410

Quoted 7 lines

drivers/net/ieee802154/at86rf230.c +0 -1

@@ -1405,7 +1405,6 @@ at86rf230_detect_device(struct at86rf230_local *lp)
  		return -EINVAL;
  	}
 
-	lp->hw->extra_tx_headroom = 0;
  	lp->hw->flags = IEEE802154_HW_TX_OMIT_CKSUM | IEEE802154_HW_AACK |
  			IEEE802154_HW_TXPOWER | IEEE802154_HW_ARET |
  			IEEE802154_HW_AFILT | IEEE802154_HW_PROMISCUOUS;

Reviewed-by Stefan Schmidt <[email protected]>

regards
Stefan Schmidt