On 07/15/2014 11:07 AM, Dong Aisheng wrote: > On Tue, Jul 15, 2014 at 10:46:32AM +0200, Marc Kleine-Budde wrote: >> On 07/15/2014 10:26 AM, Dong Aisheng wrote: >>>>>>> +static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf, >>>>>>> + u32 rxfs) >>>>>>> +{ >>>>>>> + struct m_can_priv *priv = netdev_priv(dev); >>>>>>> + u32 flags, fgi; >>>>>>> + >>>>>>> + /* calculate the fifo get index for where to read data */ >>>>>>> + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF; >>>>>>> + flags = m_can_fifo_read(priv, fgi, 0x0); >>>>>> ^^^ >>>>>> >>>>>> Can you introduce an enum for the offsets, please adjust the signature >>>>>> of m_can_fifo_read() accordingly. >>>>>> >>>>> >>>>> I wonder enum may not be suitable. >>>>> The Rx Buffer and FIFO Element is as follows: >>>>> 31 24 23 16 15 8 7 0 >>>>> R0 ESI XTD RTR ID[28:0] >>>> >>>> M_CAN_FIFO_ID >>>> >>>>> R1 ANMF FIDX[6:0] res EDL BRS DLC[3:0] RXTS[15:0] >>>> >>>> M_CAN_FIFO_DLC >>>> >>>>> R2 DB3[7:0] DB2[7:0] DB1[7:0] DB0[7:0] >>>>> R3 DB7[7:0] DB6[7:0] DB5[7:0] DB4[7:0] >>>> >>>> M_CAN_FIFO_DATA0 >>>> M_CAN_FIFO_DATA1 >>>> >>> >>> You mean as follows? >>> enum m_can_fifo { >>> M_CAN_FIFO_ID = 0, >>> M_CAN_FIFO_DLC, >> = 0x4, >>> M_CAN_FIFO_DATA0, >> = 0x8, >>> M_CAN_FIFO_DATA1, >> = 0xc, >>> }; >>> >>> static inline u32 m_can_fifo_read(const struct m_can_priv *priv, >>> u32 fgi, enum m_can_fifo fifo) >>> { >>> return readl(priv->mram_base + priv->mcfg[MRAM_RXF0].off + >>> fgi * RXF0_ELEMENT_SIZE + fifo * 0x4); >>> } >> >> without the * 0x4 >> >>> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID); >>> >>> The problem is when adding long frames support, it becomes: >>> enum m_can_fifo { >>> M_CAN_FIFO_ID = 0, >>> M_CAN_FIFO_DLC, >>> M_CAN_FIFO_DATA0, >>> M_CAN_FIFO_DATA1, >>> .... >>> M_CAN_FIFO_DATA15, >>> }; >> >> #define M_CAN_FIFO_DATA(n) >> (enum m_can_fifo)(M_CAN_FIFO_DATA_0 + (n) << 2) >> > > This is a bit strange using and we may still have to define other M_CAN_FIFO_DATAx > to avoid the enum value exceeds the defined range. > enum m_can_fifo { > M_CAN_FIFO_ID = 0, > M_CAN_FIFO_DLC = 0x4, > M_CAN_FIFO_DATA0 = 0x8, > M_CAN_FIFO_DATA1 = 0xc, > .... > M_CAN_FIFO_DATA15 = 0xc, > }; > > However, actually we will not use them at all after introducing M_CAN_FIFO_DATA(n). > If that, why we still need define them in enum? > > Comparing to this way, why not simply just do as follows: > #define M_CAN_FIFO_ID 0x0 > #define M_CAN_FIFO_DLC 0x4 > #define M_CAN_FIFO_DATA(n) (0x8 + (n) << 2) > > What do you think? Looks good. > >>> But it's useless because we may not use enum to read fifo data anymore. >>> It's not suitable to read fifo one by one: >>> m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA0); >>> m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA1); >>> .. >>> m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA15); >>> >>> >>> Instead, we may read data according to real dlc value within a for loop like: >>> #define M_CAN_FIFO(n) (n * 0x4) >>> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO(0)); >>> dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO(1)); >>> for (i = 0; dlc > 0; dlc -= 0x4, i++) { >>> .... >>> data[i] = m_can_fifo_read(priv, fgi, M_CAN_FIFO(i + 2)); >>> } >> >> id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID); >> dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC); >> for (i = 0; i <= dlc; i++) >> data[i] = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DATA(i)); > > Does it work? > The dlc is in bytes while m_can_fifo_read is read in words. Doh! probably not :) But should work with something like this: int len = DIV_ROUND_UP(dlc, 4); Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Attachment:
signature.asc
Description: OpenPGP digital signature