Messages in this thread | | | Date | Fri, 12 Nov 2010 12:45:42 +0100 | From | Wolfgang Grandegger <> | Subject | Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings |
| |
On 11/12/2010 12:10 PM, Tomoya MORINAGA wrote: > On Saturday, October 30, 2010 4:32 AM, Wolfgang Grandegger wrote : > > Sorry, for my late. > >>>> + >>>> +#define PCH_RX_OK 0x00000010 >>>> +#define PCH_TX_OK 0x00000008 >>>> +#define PCH_BUS_OFF 0x00000080 >>>> +#define PCH_EWARN 0x00000040 >>>> +#define PCH_EPASSIV 0x00000020 >>>> +#define PCH_LEC0 0x00000001 >>>> +#define PCH_LEC1 0x00000002 >>>> +#define PCH_LEC2 0x00000004 >>> >>> These are just single set bit, please use BIT() >>> Consider adding the name of the corresponding register to the define's >>> name. > > I agree. > >>> >>>> +#define PCH_LEC_ALL (PCH_LEC0 | PCH_LEC1 | PCH_LEC2) >>>> +#define PCH_STUF_ERR PCH_LEC0 >>>> +#define PCH_FORM_ERR PCH_LEC1 >>>> +#define PCH_ACK_ERR (PCH_LEC0 | PCH_LEC1) >>>> +#define PCH_BIT1_ERR PCH_LEC2 >>>> +#define PCH_BIT0_ERR (PCH_LEC0 | PCH_LEC2) >>>> +#define PCH_CRC_ERR (PCH_LEC1 | PCH_LEC2) >> >> This is an enumeration: >> >> enum { >> PCH_STUF_ERR = 1, >> PCH_FORM_ERR, >> PCH_ACK_ERR, >> PCH_BIT1_ERR; >> PCH_BIT0_ERR, >> PCH_CRC_ERR, >> PCH_LEC_ALL; >> } > > No, > LEC is for bit assignment. > Thus, "enum" can't be used.
Why? For me it's a classical enum because the value matters, and *not* the individual bit. Do you agree?
>>> 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: >> >> lec = status & PCH_LEC_ALL; >> if (lec > 0) { >> switch (lec) { > > No. > LEC is not enum.
See also my sub-sequent comment here:
http://marc.info/?l=linux-netdev&m=128880088907148&w=2
>>>> + 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. > > I will modify. > BTW, I can see ti_hecc also has the above the same code.
Yes, also the AT91 driver uses a somehow incorrect error mask. I will check and provide a patch a.s.a.p.
>> >>>> + 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; >>>> + } >>>> + >>>> + } >> >> Also, could you please add the TEC and REC: >> >> cf->data[6] = ioread32(&priv->regs->errc) & CAN_TEC; >> cf->data[7] = (ioread32(&priv->regs->errc) & CAN_REC) >> 8; > > I will add.
BTW: it could be done with one I/O call:
errc = ioread32(&priv->regs->errc); cf->data[6] = errc & CAN_TEC; cf->data[7] = (errc & CAN_REC) >> 8;
> But I couldn't find
Don't understand? It's also implemented for the SJA1000 driver:
http://lxr.linux.no/#linux+v2.6.36/drivers/net/can/sja1000/sja1000.c#L466
Wolfgang.
| |