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

[Marek Vasut <marex@denx.de>]

On 03/03/2016 09:18 PM, Oliver Hartkopp wrote:
> Hi Marek,

Hi Oliver,

> 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.

My understanding (and tests) indicate that this is correct. For CAN2.0
and CANFD-BOSCH (non-ISO), we use the later part of the condition. Only
for CANFD-ISO we use the former part.

> 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?

See above, I think the code is correct as is.

btw can you pick the remaining patches (2,3,4) if they're fine with you,
so I don't have to repost the whole series ?

-- 
Best regards,
Marek Vasut