Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings

[Wolfgang Grandegger <wg@grandegger.com>]

On 11/01/2010 12:05 PM, Marc Kleine-Budde wrote:
> Hello,
> 
> On 10/29/2010 09:32 PM, Wolfgang Grandegger wrote:
> 
> some comments inline.
> 
> [...]
...
>>>> +	if (status & PCH_LEC_ALL) {
>>>> +		priv->can.can_stats.bus_error++;
>>>> +		stats->rx_errors++;
>>>> +		switch (status & PCH_LEC_ALL) {
>>>
>>> I suggest to convert to a if-bit-set because there might be more than
>>> one bit set.
>>
>> Marc, what do you mean here. It's an enumeraton. Maybe the following
>> code is more clear:
> 
> Oh, I haven't looked this one up in the datasheet.
>>
>> 	lec = status & PCH_LEC_ALL;
>> 	if (lec > 0) {
> 
> or "if (lec)", YMMV
> 
>> 		switch (lec) {
>>
>>>> +		case PCH_STUF_ERR:
>>>> +			cf->data[2] |= CAN_ERR_PROT_STUFF;
>>>> +			break;
>>>> +		case PCH_FORM_ERR:
>>>> +			cf->data[2] |= CAN_ERR_PROT_FORM;
>>>> +			break;
>>>> +		case PCH_ACK_ERR:
>>>> +			cf->data[2] |= CAN_ERR_PROT_LOC_ACK |
>>>> +				       CAN_ERR_PROT_LOC_ACK_DEL;
>>
>> Could you check what that type of bus error that is? Usually it's a ack
>> lost error.
>>
>>>> +			break;
>>>> +		case PCH_BIT1_ERR:
>>>> +		case PCH_BIT0_ERR:
>>>> +			cf->data[2] |= CAN_ERR_PROT_BIT;
>>>> +			break;
>>>> +		case PCH_CRC_ERR:
>>>> +			cf->data[2] |= CAN_ERR_PROT_LOC_CRC_SEQ |
>>>> +				       CAN_ERR_PROT_LOC_CRC_DEL;
>>>> +			break;
>>>> +		default:
>>>> +			iowrite32(status | PCH_LEC_ALL, &priv->regs->stat);
>>>> +			break;
>>>> +		}
>>>> +
>>>> +	}

At a closer look, I doubt that the above LEC handling is correct.
According to the manual: "when the LEC shows the value “7,” this value
was written to the LEC by the CPU; thus, no CAN bus event was detected"
Therefore the LEC bits should be properly *initialized* and *reset* to
0x7, which I do not find in the code.

Wolfgang.