On Tue, Jul 08, 2014 at 01:20:18PM +0200, Marc Kleine-Budde wrote: > On 07/08/2014 01:08 PM, Dong Aisheng wrote: > > On Tue, Jul 08, 2014 at 12:41:41PM +0200, Marc Kleine-Budde wrote: > >> On 07/08/2014 12:30 PM, Dong Aisheng wrote: > >>>> Regarding the mram and the offsets: > >>>> > >>>>> fifo_addr = priv->mram_base + priv->rxf0_off + fgi * RXF0_ELEMENT_SIZE; > >>>>> fifo_addr = priv->mram_base + priv->mram_off + priv->txb_off; > >>>> > >>>> Why is rxf0_off used without the mram_off and txb_off with the mram_off? > >>>> Can you please test your driver with a mram offset != in your DT. > >>>> > >>>> If I understand the code in m_can_of_parse_mram() correctly the > >>>> individual *_off are already offsets to the *mram_base, so mram_off > >>>> should not be used within the driver. > >>> > >>> Good catch! > >>> You're right! I aslo found this recently! > >>> txb_off already includes the mram_off so should not plus mram_off again. > >>> The former test did not find it because it's still not exceed the 16K ram > >>> size for m_can0. But m_can1 has such issue. > >>> > >>>> I even think mram_off should be removed from the priv. > >>> > >>> Right, i also think so. > >>> > >>> It is used for debug information formerly that we need mram_off > >>> to calculate each element address in the fifo. > >>> > >>> By removing mram_off, i'm going to change the debug information to: > >>> dev_dbg(&pdev->dev, "mram_base %p sidf 0x%x %d xidf 0x%x %d rxf0 %x %d rxf1 %x %d rxb %x %d txe %x %d txb %x %d\n", > >>> priv->mram_base, priv->sidf_off, priv->sidf_elems, > >>> priv->xidf_off, priv->xidf_elems, priv->rxf0_off, > >>> priv->rxf0_elems, priv->rxf1_off, priv->rxf1_elems, > >>> priv->rxb_off, priv->rxb_elems, priv->txe_off, > >>> priv->txe_elems, priv->txb_off, priv->txb_elems); > >>> > >>> The annoying thing is the line has to be a much bigger one to avoid > >>> checkpatch warning of "WARNING: quoted string split across lines". > >>> > >>> What's your suggestion for such issue? > >>> Keeping the big line or split into two lines and leave checkpatch warning there? > >> > >> The idea behind the warning is, that you can grep for error messages > >> better, as normal grep wouldn't find an error string which spans two > >> lines. So make it a long line. > >> > >>>> Do the *_off and *_elems fit into a u8 or u16? If > >>>> so it makes sense to convert the priv accordingly. > >>>> > >>> > >>> Yes, *_off fit into u16 since MRAM has a maximum of 4352 words(17K). > >>> And *_elems fit into u8 since the max number is 128. > >>> I will change them accordingly. > >>> > >>>> What about putting the offset and the number of elements into a struct > >>>> and make use an array for rxf{0,1}? > >>>> > >>> > >>> You mean something like below? > >>> struct mram_cfg { > >>> u16 off; > >>> u8 elements; > >>> }; > >>> > >>> struct m_can_priv { > >>> ........ > >>> > >>> struct mram_cfg sidf; > >>> struct mram_cfg xidf; > >>> struct mram_cfg rxf0; > >>> struct mram_cfg rxf1; > >> > >> struct mram_cfg rxf[2]; > >> > > > > It does not help too much and a bit strange for only make > > rxf0/rxf1 into array, > > > > How about making them all: > > enum m_can_mram_cfg { > > SIDF = 0, > > XIDF, > > RXF0, > > RXF1, > > RXB, > > TXE, > > TXB, > > CFG_NUM, > > }; > > > > struct m_can_priv { > > ........ > > struct mram_cfg mcfg[CFG_NUM]; > > }; > > > > Then in code: > > > > priv->cfg[SIDF].off = > > priv->cfg[SIDF].elements = > > > > But it could make code become much longer... > > I like the idea, but can you add a common prefix to the enums. Though > makes the code even longer :) > Okay, got it. :-) Regards Dong Aisheng > 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