[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-can
Subject:    Re: [PATCH v4 4/6] can: flexcan: Consolidate and unify state change handling.
From:       Wolfgang Grandegger <wg () grandegger ! com>
Date:       2014-11-30 20:23:57
Message-ID: 547B7CDD.2080202 () grandegger ! com
[Download RAW message or body]

On 11/28/2014 01:12 PM, Andri Yngvason wrote:
> Replacing error state change handling with the new mechanism.
> 
> Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> ---
> Changes made since last proposal:
> can: flexcan: add FLEXCAN_HAS_BROKEN_ERR_STATE for i.MX6
> can: flexcan: adapt to newer can_change_state
> 
>  drivers/net/can/flexcan.c | 103 +++++++++-------------------------------------
>  1 file changed, 19 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 60f86bd..9f91735 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -266,7 +266,7 @@ static struct flexcan_devtype_data fsl_p1010_devtype_data = {
>  };
>  static struct flexcan_devtype_data fsl_imx28_devtype_data;
>  static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
> -	.features = FLEXCAN_HAS_V10_FEATURES,
> +	.features = FLEXCAN_HAS_V10_FEATURES | FLEXCAN_HAS_BROKEN_ERR_STATE,

Oops, this change is not related to the subject! Anyway, did it cure
your problems with state handling. Is it required for all i.MX6 cores then?

>  };
>  static struct flexcan_devtype_data fsl_vf610_devtype_data = {
>  	.features = FLEXCAN_HAS_V10_FEATURES | FLEXCAN_HAS_MECR_FEATURES,
> @@ -577,98 +577,30 @@ static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr)
>  	return 1;
>  }
>  
> -static void do_state(struct net_device *dev,
> -		     struct can_frame *cf, enum can_state new_state)
> -{
> -	struct flexcan_priv *priv = netdev_priv(dev);
> -	struct can_berr_counter bec;
> -
> -	__flexcan_get_berr_counter(dev, &bec);
> -
> -	switch (priv->can.state) {
> -	case CAN_STATE_ERROR_ACTIVE:
> -		/*
> -		 * from: ERROR_ACTIVE
> -		 * to  : ERROR_WARNING, ERROR_PASSIVE, BUS_OFF
> -		 * =>  : there was a warning int
> -		 */
> -		if (new_state >= CAN_STATE_ERROR_WARNING &&
> -		    new_state <= CAN_STATE_BUS_OFF) {
> -			netdev_dbg(dev, "Error Warning IRQ\n");
> -			priv->can.can_stats.error_warning++;
> -
> -			cf->can_id |= CAN_ERR_CRTL;
> -			cf->data[1] = (bec.txerr > bec.rxerr) ?
> -				CAN_ERR_CRTL_TX_WARNING :
> -				CAN_ERR_CRTL_RX_WARNING;
> -		}
> -	case CAN_STATE_ERROR_WARNING:	/* fallthrough */
> -		/*
> -		 * from: ERROR_ACTIVE, ERROR_WARNING
> -		 * to  : ERROR_PASSIVE, BUS_OFF
> -		 * =>  : error passive int
> -		 */
> -		if (new_state >= CAN_STATE_ERROR_PASSIVE &&
> -		    new_state <= CAN_STATE_BUS_OFF) {
> -			netdev_dbg(dev, "Error Passive IRQ\n");

Maybe it's a good idea to have netdev_dbg's for state changes in
can_change_state() as well. Other opinions? Marc?

Apart from these to comments the patches 1-5 look good (but you should
drop the 6th).

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]