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];
> ......
> struct mram_cfg txb;
> };
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 |
signature.asc
Description: OpenPGP digital signature
