> > 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
