lkml.org 
[lkml]   [2010]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2010-11-03 17:17    [W:0.090 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site