[PATCH bluetooth-next 0/5] at86rf230: cleanups

[PATCH bluetooth-next 0/5] at86rf230: cleanups

[Alexander Aring]

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(-)

-- 
2.1.3

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

[Alexander Aring]

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

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 1c01356..4e983b3 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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
-- 
2.1.3

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

[Alexander Aring]

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

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 4e983b3..430d3bd 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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)
 {
-- 
2.1.3

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

[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 <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 430d3bd..08b2f9f 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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);
 	}
 }
 
-- 
2.1.3

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

[Alexander Aring]

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 08b2f9f..7b0a17e 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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);
 }
 
 
-- 
2.1.3

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

[Alexander Aring]

The attribute extra_tx_headroom should already be zero.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 7b0a17e..89589d8 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -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;
-- 
2.1.3

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

[Stefan Schmidt]

Hello.

On 15/12/14 00:20, Alexander Aring wrote:
> This patch removes an unnecessary if branch inside the tx complete
> handler.
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c
> b/drivers/net/ieee802154/at86rf230.c
> index 1c01356..4e983b3 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -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 <s.schmidt@samsung.com>

regards
Stefan Schmidt

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

[Stefan Schmidt]

Hello.

On 15/12/14 00:20, Alexander Aring wrote:
> This patch makes the at86rf230_async_error inline. This function is
> small enough to handle inline.
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c
> b/drivers/net/ieee802154/at86rf230.c
> index 4e983b3..430d3bd 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -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 <s.schmidt@samsung.com>

regards
Stefan Schmidt

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

[Stefan Schmidt]

Hello.

On 15/12/14 00:20, Alexander Aring wrote:
> 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 <alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c
> b/drivers/net/ieee802154/at86rf230.c
> index 430d3bd..08b2f9f 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -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 <s.schmidt@samsung.com>

regards
Stefan Schmidt

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

[Stefan Schmidt]

Hello.

On 15/12/14 00:20, Alexander Aring wrote:
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c
> b/drivers/net/ieee802154/at86rf230.c
> index 08b2f9f..7b0a17e 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -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);
>   }
>

Reviewed-by: Stefan Schmidt <s.schmidt@samsung.com>

regards
Stefan Schmidt

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

[Stefan Schmidt]

Hello.

On 15/12/14 00:20, Alexander Aring wrote:
> The attribute extra_tx_headroom should already be zero.
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c
> b/drivers/net/ieee802154/at86rf230.c
> index 7b0a17e..89589d8 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -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 <s.schmidt@samsung.com>

regards
Stefan Schmidt

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

[Alexander Aring]

On Mon, Dec 15, 2014 at 08:29:26AM +0000, Stefan Schmidt wrote:
> Hello.
> 
> On 15/12/14 00:20, Alexander Aring wrote:
> >This patch makes the at86rf230_async_error inline. This function is
> >small enough to handle inline.
> >
> >Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> >---
> >  drivers/net/ieee802154/at86rf230.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/ieee802154/at86rf230.c
> >b/drivers/net/ieee802154/at86rf230.c
> >index 4e983b3..430d3bd 100644
> >--- a/drivers/net/ieee802154/at86rf230.c
> >+++ b/drivers/net/ieee802154/at86rf230.c
> >@@ -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.

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

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

[Alexander Aring]

On Mon, Dec 15, 2014 at 08:29:24AM +0000, Stefan Schmidt wrote:
> Hello.
> 
> On 15/12/14 00:20, Alexander Aring wrote:
> >This patch removes an unnecessary if branch inside the tx complete
> >handler.
> >
> >Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> >---
> >  drivers/net/ieee802154/at86rf230.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> >diff --git a/drivers/net/ieee802154/at86rf230.c
> >b/drivers/net/ieee802154/at86rf230.c
> >index 1c01356..4e983b3 100644
> >--- a/drivers/net/ieee802154/at86rf230.c
> >+++ b/drivers/net/ieee802154/at86rf230.c
> >@@ -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?
> 

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

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

[Alexander Aring]

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

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

[Stefan Schmidt]

Hello.

On 15/12/14 09:39, Alexander Aring wrote:
> On Mon, Dec 15, 2014 at 08:29:26AM +0000, Stefan Schmidt wrote:
>> Hello.
>>
>> On 15/12/14 00:20, Alexander Aring wrote:
>>> This patch makes the at86rf230_async_error inline. This function is
>>> small enough to handle inline.
>>>
>>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
>>> ---
>>>   drivers/net/ieee802154/at86rf230.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ieee802154/at86rf230.c
>>> b/drivers/net/ieee802154/at86rf230.c
>>> index 4e983b3..430d3bd 100644
>>> --- a/drivers/net/ieee802154/at86rf230.c
>>> +++ b/drivers/net/ieee802154/at86rf230.c
>>> @@ -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.
>
> 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.

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

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

[Alexander Aring]

On Mon, Dec 15, 2014 at 09:01:18AM +0000, Stefan Schmidt wrote:
> Hello.
> 
> On 15/12/14 09:39, Alexander Aring wrote:
> >On Mon, Dec 15, 2014 at 08:29:26AM +0000, Stefan Schmidt wrote:
> >>Hello.
> >>
> >>On 15/12/14 00:20, Alexander Aring wrote:
> >>>This patch makes the at86rf230_async_error inline. This function is
> >>>small enough to handle inline.
> >>>
> >>>Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> >>>---
> >>>  drivers/net/ieee802154/at86rf230.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>>diff --git a/drivers/net/ieee802154/at86rf230.c
> >>>b/drivers/net/ieee802154/at86rf230.c
> >>>index 4e983b3..430d3bd 100644
> >>>--- a/drivers/net/ieee802154/at86rf230.c
> >>>+++ b/drivers/net/ieee802154/at86rf230.c
> >>>@@ -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.
> >
> >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.
> 
> 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