* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).