netdev - RE: [RESEND PATCH v5 1/2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 7 Jun 2016 13:18:38 +0000
From:	Ramesh Shanmugasundaram <ramesh.shanmugasundaram@...renesas.com>
To:	David Miller <davem@...emloft.net>,
	"socketcan@...tkopp.net" <socketcan@...tkopp.net>
CC:	"ulrich.hecht@...il.com" <ulrich.hecht@...il.com>,
	"mkl@...gutronix.de" <mkl@...gutronix.de>,
	"wg@...ndegger.com" <wg@...ndegger.com>,
	"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Chris Paterson <Chris.Paterson2@...esas.com>,
	"horms@...ge.net.au" <horms@...ge.net.au>,
	"magnus.damm@...il.com" <magnus.damm@...il.com>,
	"linux-renesas-soc@...r.kernel.org" 
	<linux-renesas-soc@...r.kernel.org>
Subject: RE: [RESEND PATCH v5 1/2] can: rcar_canfd: Add Renesas R-Car CAN FD
 driver

> > On 06/03/2016 07:03 PM, Ulrich Hecht wrote:
> >
> >> Thanks; I missed that every register is described twice.
> >>
> >> Nevertheless, names often vary more or less subtly between your patch
> >> and the specs, making it very hard to review. Some have letters
> >> added, some have letters removed, and some are just plain confusing.
> >> For instance, RCANFD_DCFG_* apparently does not describe, as one
> >> might think, RSCFDnCFDCmDCFG, but RSCFDnCFDCmFDCFG. These names are,
> >> of course, completely ridiculous, but inventing a new set makes
> >> things even worse, IMO.
> >
> > ???
> >
> > You suggest to use 'completely ridiculous' definitions in favor to
> > definitions that have a proper name space RCANFD_ ?
> >
> > When there is a more readable way that maintains proper readable code
> > there's no reason to adopt crappy definitions just because some chip
> > designer has no clue how to design proper register names.
> >
> > When there's some mapping from RSCFDnCFDCmFDCFG to RCANFD_DCFG_* this
> > could be mentioned in the comments.
> >
> > But I'm totally against these blurry upper/lower case letter stuff for
> > register definitions.
> 
> I agree with Oliver, these StuDlyCaPS names used in the spec should not be
> used in the driver, they are completely unreadable.

Thanks Oliver, Dave for your inputs.

Hi Uli,

I have added comments to the readable register set mapping them to the clumsy name in h/w manual to aid your review. I have kept the bit name definitions within register exactly as in h/w manual. E.g.

+/* RSCFDnCFDGSTS */
+#define RCANFD_GSTS_GRAMINIT		BIT(3)
+#define RCANFD_GSTS_GSLPSTS		BIT(2)
+#define RCANFD_GSTS_GHLTSTS		BIT(1)
+#define RCANFD_GSTS_GRSTSTS		BIT(0)

I have also sorted the bit definition ordering from MSB to LSB. I will send out the updated patch set next. I hope this addresses your concern.

Please note that I am planning to add classical CAN only mode support next (after the current patch set is accepted). The code is designed to support both modes and hence I have classified the register definitions as "common register set" & "CAN FD specific register set". In future, I will just add the "Classical CAN specific register set" alone without much code re-org.

Thanks,
Ramesh