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? > > 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. Regards Dong Aisheng > > > So i'm not sure define that enum now is really needed. > > [...] > > >>>>> +static int m_can_handle_lec_err(struct net_device *dev, > >>>>> + enum m_can_lec_type lec_type) > >>>>> +{ > >>>>> + struct m_can_priv *priv = netdev_priv(dev); > >>>>> + struct net_device_stats *stats = &dev->stats; > >>>>> + struct can_frame *cf; > >>>>> + struct sk_buff *skb; > >>>>> + > >>>>> + /* early exit if no lec update */ > >>>>> + if (lec_type == LEC_UNUSED) > >>>>> + return 0; > >>>> > >>>> I think this is not needed, as checked by the only caller. > >>> > >>> You mean move it to caller as follows? > >>> /* handle lec errors on the bus */ > >>> if ((psr & LEC_UNUSED) && ((psr & LEC_UNUSED)!= LEC_UNUSED) && > >> > >> yes - or something like this: > >> > >> static inline bool is_lec(u32 psr) > >> { > >> u32 psr &= LEC_UNUSED > >> > >> return psr && (psr != LEC_UNUSED) > >> } > >> > >> if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) && > >> is_lec(psr)) { > >> } > >> > > > > > > Looks fine. > > Maybe is_lec_err(u32 psr) better? :-) > > Yes, is_lec() was just a random placeholder :) Descriptive function > names are always preferred. > > 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 | > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html