Re: [PATCH V4 1/4] net: can: ifi: Fix clock generator configuration

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Marek,

On 03/03/2016 08:45 PM, Marek Vasut wrote:

> @@ -545,32 +545,34 @@ static void ifi_canfd_set_bittiming(struct net_device *ndev)
>  	u32 noniso_arg = 0;
>  	u32 time_off;
>  
> -	if (priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO) {
> +	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
> +	    !(priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)) {
> +		time_off = IFI_CANFD_TIME_SJW_OFF_ISO;
> +	} else {
>  		noniso_arg = IFI_CANFD_TIME_SET_TIMEB_BOSCH |
>  			     IFI_CANFD_TIME_SET_TIMEA_BOSCH |
>  			     IFI_CANFD_TIME_SET_PRESC_BOSCH |
>  			     IFI_CANFD_TIME_SET_SJW_BOSCH;
>  		time_off = IFI_CANFD_TIME_SJW_OFF_BOSCH;
> -	} else {
> -		time_off = IFI_CANFD_TIME_SJW_OFF_ISO;
>  	}
>  

This may set time_off to IFI_CANFD_TIME_SJW_OFF_BOSCH in the case of

	!(priv->can.ctrlmode & CAN_CTRLMODE_FD)

(== CAN2.0) right?

I assume this is not intended.

I would suggest to initialize time_off first:

	u32 noniso_arg = 0;
	u32 time_off = IFI_CANFD_TIME_SJW_OFF_ISO;

	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
	    (priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)) {
		noniso_arg = IFI_CANFD_TIME_SET_TIMEB_BOSCH |
			     IFI_CANFD_TIME_SET_TIMEA_BOSCH |
			     IFI_CANFD_TIME_SET_PRESC_BOSCH |
			     IFI_CANFD_TIME_SET_SJW_BOSCH;
		time_off = IFI_CANFD_TIME_SJW_OFF_BOSCH;
	}

Is my assumption correct? Or does the manual require other settings?

Regards,
Oliver