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.

[...]

...

quoted

quoted

quoted

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

quoted

	lec = status & PCH_LEC_ALL;
	if (lec > 0) {

or "if (lec)", YMMV

quoted

		switch (lec) {

quoted

quoted

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

quoted

quoted

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