* [PATCH v3 2/4] can: sja1000: Consolidate and unify state change handling
@ 2014-09-26 17:35 Andri Yngvason
2014-11-25 20:55 ` Wolfgang Grandegger
0 siblings, 1 reply; 4+ messages in thread
From: Andri Yngvason @ 2014-09-26 17:35 UTC (permalink / raw)
To: linux-can; +Cc: mkl, wg
Replacing error state change handling with the new mechanism.
Changes made since last proposal:
can: sja1000: move bus-off handling and fix state transitions.
can: sja1000: made one line more readable
Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
---
drivers/net/can/sja1000/sja1000.c | 49 ++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index b27ac60..ee94e2c 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -392,12 +392,20 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
struct can_frame *cf;
struct sk_buff *skb;
enum can_state state = priv->can.state;
+ enum can_state rx_state, tx_state;
+ unsigned int rxerr, txerr;
uint8_t ecc, alc;
skb = alloc_can_err_skb(dev, &cf);
if (skb == NULL)
return -ENOMEM;
+ txerr = priv->read_reg(priv, SJA1000_TXERR);
+ rxerr = priv->read_reg(priv, SJA1000_RXERR);
+
+ cf->data[6] = txerr;
+ cf->data[7] = rxerr;
+
if (isrc & IRQ_DOI) {
/* data overrun interrupt */
netdev_dbg(dev, "data overrun interrupt\n");
@@ -412,13 +420,11 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
/* error warning interrupt */
netdev_dbg(dev, "error warning interrupt\n");
- if (status & SR_BS) {
+ if (status & SR_BS)
state = CAN_STATE_BUS_OFF;
- cf->can_id |= CAN_ERR_BUSOFF;
- can_bus_off(dev);
- } else if (status & SR_ES) {
+ else if (status & SR_ES)
state = CAN_STATE_ERROR_WARNING;
- } else
+ else
state = CAN_STATE_ERROR_ACTIVE;
}
if (isrc & IRQ_BEI) {
@@ -452,10 +458,11 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
if (isrc & IRQ_EPI) {
/* error passive interrupt */
netdev_dbg(dev, "error passive interrupt\n");
- if (status & SR_ES)
- state = CAN_STATE_ERROR_PASSIVE;
+
+ if (state == CAN_STATE_ERROR_PASSIVE)
+ state = CAN_STATE_ERROR_WARNING;
else
- state = CAN_STATE_ERROR_ACTIVE;
+ state = CAN_STATE_ERROR_PASSIVE;
}
if (isrc & IRQ_ALI) {
/* arbitration lost interrupt */
@@ -467,27 +474,15 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
cf->data[0] = alc & 0x1f;
}
- if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING ||
- state == CAN_STATE_ERROR_PASSIVE)) {
- uint8_t rxerr = priv->read_reg(priv, SJA1000_RXERR);
- uint8_t txerr = priv->read_reg(priv, SJA1000_TXERR);
- cf->can_id |= CAN_ERR_CRTL;
- if (state == CAN_STATE_ERROR_WARNING) {
- priv->can.can_stats.error_warning++;
- cf->data[1] = (txerr > rxerr) ?
- CAN_ERR_CRTL_TX_WARNING :
- CAN_ERR_CRTL_RX_WARNING;
- } else {
- priv->can.can_stats.error_passive++;
- cf->data[1] = (txerr > rxerr) ?
- CAN_ERR_CRTL_TX_PASSIVE :
- CAN_ERR_CRTL_RX_PASSIVE;
- }
- cf->data[6] = txerr;
- cf->data[7] = rxerr;
+ if (state != priv->can.state) {
+ tx_state = txerr >= rxerr ? state : 0;
+ rx_state = txerr <= rxerr ? state : 0;
+
+ can_change_state(dev, cf, state, tx_state, rx_state);
}
- priv->can.state = state;
+ if(state == CAN_STATE_BUS_OFF)
+ can_bus_off(dev);
netif_rx(skb);
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 2/4] can: sja1000: Consolidate and unify state change handling
2014-09-26 17:35 [PATCH v3 2/4] can: sja1000: Consolidate and unify state change handling Andri Yngvason
@ 2014-11-25 20:55 ` Wolfgang Grandegger
2014-11-26 10:44 ` Andri Yngvason
0 siblings, 1 reply; 4+ messages in thread
From: Wolfgang Grandegger @ 2014-11-25 20:55 UTC (permalink / raw)
To: Andri Yngvason, linux-can; +Cc: mkl
On 09/26/2014 07:35 PM, Andri Yngvason wrote:
> Replacing error state change handling with the new mechanism.
>
> Changes made since last proposal:
> can: sja1000: move bus-off handling and fix state transitions.
> can: sja1000: made one line more readable
See my previous comment.
> Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> ---
> drivers/net/can/sja1000/sja1000.c | 49 ++++++++++++++++++---------------------
> 1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> index b27ac60..ee94e2c 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -392,12 +392,20 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
> struct can_frame *cf;
> struct sk_buff *skb;
> enum can_state state = priv->can.state;
> + enum can_state rx_state, tx_state;
> + unsigned int rxerr, txerr;
> uint8_t ecc, alc;
>
> skb = alloc_can_err_skb(dev, &cf);
> if (skb == NULL)
> return -ENOMEM;
>
> + txerr = priv->read_reg(priv, SJA1000_TXERR);
> + rxerr = priv->read_reg(priv, SJA1000_RXERR);
> +
> + cf->data[6] = txerr;
> + cf->data[7] = rxerr;
> +
> if (isrc & IRQ_DOI) {
> /* data overrun interrupt */
> netdev_dbg(dev, "data overrun interrupt\n");
> @@ -412,13 +420,11 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
> /* error warning interrupt */
> netdev_dbg(dev, "error warning interrupt\n");
>
> - if (status & SR_BS) {
> + if (status & SR_BS)
> state = CAN_STATE_BUS_OFF;
> - cf->can_id |= CAN_ERR_BUSOFF;
> - can_bus_off(dev);
> - } else if (status & SR_ES) {
> + else if (status & SR_ES)
> state = CAN_STATE_ERROR_WARNING;
> - } else
> + else
> state = CAN_STATE_ERROR_ACTIVE;
> }
> if (isrc & IRQ_BEI) {
> @@ -452,10 +458,11 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
> if (isrc & IRQ_EPI) {
> /* error passive interrupt */
> netdev_dbg(dev, "error passive interrupt\n");
> - if (status & SR_ES)
> - state = CAN_STATE_ERROR_PASSIVE;
> +
> + if (state == CAN_STATE_ERROR_PASSIVE)
> + state = CAN_STATE_ERROR_WARNING;
> else
> - state = CAN_STATE_ERROR_ACTIVE;
> + state = CAN_STATE_ERROR_PASSIVE;
> }
> if (isrc & IRQ_ALI) {
> /* arbitration lost interrupt */
> @@ -467,27 +474,15 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
> cf->data[0] = alc & 0x1f;
> }
>
> - if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING ||
> - state == CAN_STATE_ERROR_PASSIVE)) {
> - uint8_t rxerr = priv->read_reg(priv, SJA1000_RXERR);
> - uint8_t txerr = priv->read_reg(priv, SJA1000_TXERR);
> - cf->can_id |= CAN_ERR_CRTL;
> - if (state == CAN_STATE_ERROR_WARNING) {
> - priv->can.can_stats.error_warning++;
> - cf->data[1] = (txerr > rxerr) ?
> - CAN_ERR_CRTL_TX_WARNING :
> - CAN_ERR_CRTL_RX_WARNING;
> - } else {
> - priv->can.can_stats.error_passive++;
> - cf->data[1] = (txerr > rxerr) ?
> - CAN_ERR_CRTL_TX_PASSIVE :
> - CAN_ERR_CRTL_RX_PASSIVE;
> - }
> - cf->data[6] = txerr;
> - cf->data[7] = rxerr;
> + if (state != priv->can.state) {
> + tx_state = txerr >= rxerr ? state : 0;
> + rx_state = txerr <= rxerr ? state : 0;
> +
> + can_change_state(dev, cf, state, tx_state, rx_state);
priv->can.state = can_change_state(dev, cf, state, tx_state, rx_state);
Is maybe more transparent.
> }
>
> - priv->can.state = state;
> + if(state == CAN_STATE_BUS_OFF)
> + can_bus_off(dev);
This could go inside the "if (state != priv->can.state)" block above.
>
> netif_rx(skb);
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 2/4] can: sja1000: Consolidate and unify state change handling
2014-11-25 20:55 ` Wolfgang Grandegger
@ 2014-11-26 10:44 ` Andri Yngvason
2014-11-26 11:24 ` Wolfgang Grandegger
0 siblings, 1 reply; 4+ messages in thread
From: Andri Yngvason @ 2014-11-26 10:44 UTC (permalink / raw)
To: Wolfgang Grandegger, linux-can; +Cc: mkl
Quoting Wolfgang Grandegger (2014-11-25 20:55:43)
> On 09/26/2014 07:35 PM, Andri Yngvason wrote:
> > Replacing error state change handling with the new mechanism.
> >
> > Changes made since last proposal:
> > can: sja1000: move bus-off handling and fix state transitions.
> > can: sja1000: made one line more readable
>
> See my previous comment.
>
> > Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> > ---
> > drivers/net/can/sja1000/sja1000.c | 49 ++++++++++++++++++---------------------
> > 1 file changed, 22 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> > index b27ac60..ee94e2c 100644
> > --- a/drivers/net/can/sja1000/sja1000.c
> > +++ b/drivers/net/can/sja1000/sja1000.c
> > @@ -392,12 +392,20 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
> > struct can_frame *cf;
> > struct sk_buff *skb;
> > enum can_state state = priv->can.state;
> > + enum can_state rx_state, tx_state;
> > + unsigned int rxerr, txerr;
> > uint8_t ecc, alc;
> >
> > skb = alloc_can_err_skb(dev, &cf);
> > if (skb == NULL)
> > return -ENOMEM;
> >
> > + txerr = priv->read_reg(priv, SJA1000_TXERR);
> > + rxerr = priv->read_reg(priv, SJA1000_RXERR);
> > +
> > + cf->data[6] = txerr;
> > + cf->data[7] = rxerr;
> > +
> > if (isrc & IRQ_DOI) {
> > /* data overrun interrupt */
> > netdev_dbg(dev, "data overrun interrupt\n");
> > @@ -412,13 +420,11 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
> > /* error warning interrupt */
> > netdev_dbg(dev, "error warning interrupt\n");
> >
> > - if (status & SR_BS) {
> > + if (status & SR_BS)
> > state = CAN_STATE_BUS_OFF;
> > - cf->can_id |= CAN_ERR_BUSOFF;
> > - can_bus_off(dev);
> > - } else if (status & SR_ES) {
> > + else if (status & SR_ES)
> > state = CAN_STATE_ERROR_WARNING;
> > - } else
> > + else
> > state = CAN_STATE_ERROR_ACTIVE;
> > }
> > if (isrc & IRQ_BEI) {
> > @@ -452,10 +458,11 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
> > if (isrc & IRQ_EPI) {
> > /* error passive interrupt */
> > netdev_dbg(dev, "error passive interrupt\n");
> > - if (status & SR_ES)
> > - state = CAN_STATE_ERROR_PASSIVE;
> > +
> > + if (state == CAN_STATE_ERROR_PASSIVE)
> > + state = CAN_STATE_ERROR_WARNING;
> > else
> > - state = CAN_STATE_ERROR_ACTIVE;
> > + state = CAN_STATE_ERROR_PASSIVE;
> > }
> > if (isrc & IRQ_ALI) {
> > /* arbitration lost interrupt */
> > @@ -467,27 +474,15 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
> > cf->data[0] = alc & 0x1f;
> > }
> >
> > - if (state != priv->can.state && (state == CAN_STATE_ERROR_WARNING ||
> > - state == CAN_STATE_ERROR_PASSIVE)) {
> > - uint8_t rxerr = priv->read_reg(priv, SJA1000_RXERR);
> > - uint8_t txerr = priv->read_reg(priv, SJA1000_TXERR);
> > - cf->can_id |= CAN_ERR_CRTL;
> > - if (state == CAN_STATE_ERROR_WARNING) {
> > - priv->can.can_stats.error_warning++;
> > - cf->data[1] = (txerr > rxerr) ?
> > - CAN_ERR_CRTL_TX_WARNING :
> > - CAN_ERR_CRTL_RX_WARNING;
> > - } else {
> > - priv->can.can_stats.error_passive++;
> > - cf->data[1] = (txerr > rxerr) ?
> > - CAN_ERR_CRTL_TX_PASSIVE :
> > - CAN_ERR_CRTL_RX_PASSIVE;
> > - }
> > - cf->data[6] = txerr;
> > - cf->data[7] = rxerr;
> > + if (state != priv->can.state) {
> > + tx_state = txerr >= rxerr ? state : 0;
> > + rx_state = txerr <= rxerr ? state : 0;
> > +
> > + can_change_state(dev, cf, state, tx_state, rx_state);
>
> priv->can.state = can_change_state(dev, cf, state, tx_state, rx_state);
>
> Is maybe more transparent.
>
Yes, or maybe:
priv->can.state = new_state;
can_update_state_error_stats(dev, tx_state, rx_state);
can_compose_state_error_frame(dev, cf, tx_state, rx_state);
I can't really think of a function name that describes those three things
accurately enough, so this is probably the most explicit way to do it.
>
> > }
> >
> > - priv->can.state = state;
> > + if(state == CAN_STATE_BUS_OFF)
> > + can_bus_off(dev);
>
> This could go inside the "if (state != priv->can.state)" block above.
>
OK.
>
> >
> > netif_rx(skb);
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 2/4] can: sja1000: Consolidate and unify state change handling
2014-11-26 10:44 ` Andri Yngvason
@ 2014-11-26 11:24 ` Wolfgang Grandegger
0 siblings, 0 replies; 4+ messages in thread
From: Wolfgang Grandegger @ 2014-11-26 11:24 UTC (permalink / raw)
To: Andri Yngvason; +Cc: linux-can, mkl
On Wed, 26 Nov 2014 10:44:14 +0000, Andri Yngvason
<andri.yngvason@marel.com> wrote:
> Quoting Wolfgang Grandegger (2014-11-25 20:55:43)
>> On 09/26/2014 07:35 PM, Andri Yngvason wrote:
>> > Replacing error state change handling with the new mechanism.
>> >
>> > Changes made since last proposal:
>> > can: sja1000: move bus-off handling and fix state transitions.
>> > can: sja1000: made one line more readable
>>
>> See my previous comment.
>>
>> > Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
>> > ---
>> > drivers/net/can/sja1000/sja1000.c | 49
>> > ++++++++++++++++++---------------------
>> > 1 file changed, 22 insertions(+), 27 deletions(-)
>> >
>> > diff --git a/drivers/net/can/sja1000/sja1000.c
>> > b/drivers/net/can/sja1000/sja1000.c
>> > index b27ac60..ee94e2c 100644
>> > --- a/drivers/net/can/sja1000/sja1000.c
>> > +++ b/drivers/net/can/sja1000/sja1000.c
>> > @@ -392,12 +392,20 @@ static int sja1000_err(struct net_device *dev,
>> > uint8_t isrc, uint8_t status)
>> > struct can_frame *cf;
>> > struct sk_buff *skb;
>> > enum can_state state = priv->can.state;
>> > + enum can_state rx_state, tx_state;
>> > + unsigned int rxerr, txerr;
>> > uint8_t ecc, alc;
>> >
>> > skb = alloc_can_err_skb(dev, &cf);
>> > if (skb == NULL)
>> > return -ENOMEM;
>> >
>> > + txerr = priv->read_reg(priv, SJA1000_TXERR);
>> > + rxerr = priv->read_reg(priv, SJA1000_RXERR);
>> > +
>> > + cf->data[6] = txerr;
>> > + cf->data[7] = rxerr;
>> > +
>> > if (isrc & IRQ_DOI) {
>> > /* data overrun interrupt */
>> > netdev_dbg(dev, "data overrun interrupt\n");
>> > @@ -412,13 +420,11 @@ static int sja1000_err(struct net_device *dev,
>> > uint8_t isrc, uint8_t status)
>> > /* error warning interrupt */
>> > netdev_dbg(dev, "error warning interrupt\n");
>> >
>> > - if (status & SR_BS) {
>> > + if (status & SR_BS)
>> > state = CAN_STATE_BUS_OFF;
>> > - cf->can_id |= CAN_ERR_BUSOFF;
>> > - can_bus_off(dev);
>> > - } else if (status & SR_ES) {
>> > + else if (status & SR_ES)
>> > state = CAN_STATE_ERROR_WARNING;
>> > - } else
>> > + else
>> > state = CAN_STATE_ERROR_ACTIVE;
>> > }
>> > if (isrc & IRQ_BEI) {
>> > @@ -452,10 +458,11 @@ static int sja1000_err(struct net_device *dev,
>> > uint8_t isrc, uint8_t status)
>> > if (isrc & IRQ_EPI) {
>> > /* error passive interrupt */
>> > netdev_dbg(dev, "error passive interrupt\n");
>> > - if (status & SR_ES)
>> > - state = CAN_STATE_ERROR_PASSIVE;
>> > +
>> > + if (state == CAN_STATE_ERROR_PASSIVE)
>> > + state = CAN_STATE_ERROR_WARNING;
>> > else
>> > - state = CAN_STATE_ERROR_ACTIVE;
>> > + state = CAN_STATE_ERROR_PASSIVE;
>> > }
>> > if (isrc & IRQ_ALI) {
>> > /* arbitration lost interrupt */
>> > @@ -467,27 +474,15 @@ static int sja1000_err(struct net_device *dev,
>> > uint8_t isrc, uint8_t status)
>> > cf->data[0] = alc & 0x1f;
>> > }
>> >
>> > - if (state != priv->can.state && (state ==
>> > CAN_STATE_ERROR_WARNING ||
>> > - state ==
>> > CAN_STATE_ERROR_PASSIVE)) {
>> > - uint8_t rxerr = priv->read_reg(priv, SJA1000_RXERR);
>> > - uint8_t txerr = priv->read_reg(priv, SJA1000_TXERR);
>> > - cf->can_id |= CAN_ERR_CRTL;
>> > - if (state == CAN_STATE_ERROR_WARNING) {
>> > - priv->can.can_stats.error_warning++;
>> > - cf->data[1] = (txerr > rxerr) ?
>> > - CAN_ERR_CRTL_TX_WARNING :
>> > - CAN_ERR_CRTL_RX_WARNING;
>> > - } else {
>> > - priv->can.can_stats.error_passive++;
>> > - cf->data[1] = (txerr > rxerr) ?
>> > - CAN_ERR_CRTL_TX_PASSIVE :
>> > - CAN_ERR_CRTL_RX_PASSIVE;
>> > - }
>> > - cf->data[6] = txerr;
>> > - cf->data[7] = rxerr;
>> > + if (state != priv->can.state) {
>> > + tx_state = txerr >= rxerr ? state : 0;
>> > + rx_state = txerr <= rxerr ? state : 0;
>> > +
>> > + can_change_state(dev, cf, state, tx_state, rx_state);
>>
>> priv->can.state = can_change_state(dev, cf, state,
>> tx_state, rx_state);
>>
>> Is maybe more transparent.
>>
> Yes, or maybe:
> priv->can.state = new_state;
> can_update_state_error_stats(dev, tx_state, rx_state);
> can_compose_state_error_frame(dev, cf, tx_state, rx_state);
>
> I can't really think of a function name that describes those three
things
> accurately enough, so this is probably the most explicit way to do it.
Well, thinking more about it, just leave it as is. can_change_state() is
pretty
clear also that it will update priv->can.state.
Wolfgang.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-11-26 11:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-26 17:35 [PATCH v3 2/4] can: sja1000: Consolidate and unify state change handling Andri Yngvason
2014-11-25 20:55 ` Wolfgang Grandegger
2014-11-26 10:44 ` Andri Yngvason
2014-11-26 11:24 ` Wolfgang Grandegger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.