Messages in this thread | | | Date | Wed, 03 Nov 2010 17:15:42 +0100 | From | Wolfgang Grandegger <> | Subject | Re: [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/
| |