From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Kleine-Budde Subject: Re: [PATCH v2 1/1] can: m_can: add Bosch M_CAN controller support Date: Tue, 08 Jul 2014 12:41:41 +0200 Message-ID: <53BBCAE5.7040404@pengutronix.de> References: <1404474812-16855-1-git-send-email-b29396@freescale.com> <53B69C55.8010901@pengutronix.de> <20140707071040.GA18505@shlinux1.ap.freescale.net> <53BA7543.70901@pengutronix.de> <20140708103009.GA21754@shlinux1.ap.freescale.net> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="UCHoFrhDcuJdhrwOPfmvl2U67a26lcpfO" Return-path: In-Reply-To: <20140708103009.GA21754@shlinux1.ap.freescale.net> Sender: linux-can-owner@vger.kernel.org To: Dong Aisheng Cc: linux-can@vger.kernel.org, wg@grandegger.com, socketcan@hartkopp.net, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org List-Id: devicetree@vger.kernel.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --UCHoFrhDcuJdhrwOPfmvl2U67a26lcpfO Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/08/2014 12:30 PM, Dong Aisheng wrote: >> Regarding the mram and the offsets: >> >>> fifo_addr =3D priv->mram_base + priv->rxf0_off + fgi * RXF0_ELEMENT_= SIZE; >>> fifo_addr =3D priv->mram_base + priv->mram_off + priv->txb_off; >> >> Why is rxf0_off used without the mram_off and txb_off with the mram_of= f? >> Can you please test your driver with a mram offset !=3D 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. >=20 > Good catch! > You're right! I aslo found this recently! > txb_off already includes the mram_off so should not plus mram_off again= =2E > The former test did not find it because it's still not exceed the 16K r= am > size for m_can0. But m_can1 has such issue. >=20 >> I even think mram_off should be removed from the priv. >=20 > Right, i also think so. >=20 > It is used for debug information formerly that we need mram_off > to calculate each element address in the fifo. >=20 > 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); >=20 > The annoying thing is the line has to be a much bigger one to avoid > checkpatch warning of "WARNING: quoted string split across lines". >=20 > What's your suggestion for such issue? > Keeping the big line or split into two lines and leave checkpatch warni= ng 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. >> >=20 > 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. >=20 >> What about putting the offset and the number of elements into a struct= >> and make use an array for rxf{0,1}? >> >=20 > You mean something like below? > struct mram_cfg { > u16 off; > u8 elements; > }; >=20 > struct m_can_priv { > ........ >=20 > 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 --=20 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 | --UCHoFrhDcuJdhrwOPfmvl2U67a26lcpfO Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Using GnuPG with Icedove - http://www.enigmail.net/ iEYEARECAAYFAlO7yuUACgkQjTAFq1RaXHOA6QCghfzpdimUOFm0C59x3mWizPvb euUAoJcoaeZUuhGRHPsLOxOKfb/PP7Gj =tKj1 -----END PGP SIGNATURE----- --UCHoFrhDcuJdhrwOPfmvl2U67a26lcpfO--