lkml.org 
[lkml]   [2014]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCHv7 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller.
On Tue 2014-07-08 13:54:58, Borislav Petkov wrote:
> On Tue, Jul 08, 2014 at 01:52:05PM +0200, Pavel Machek wrote:
> > I'm not joking. Try to understand and verify the code above. You
> > can't. The "descriptive macro names" are useless; all the code does is
> > split register in pieces. With the numbers it would be very obvious.
>
> No, you need to fix the names not switch to naked numbers.

Hiding numbers that are used just once into defines to put them out of
sight does not really help readability.

Compare:

+#define DRAMADDRW_COLBIT_MASK 0x001F
+#define DRAMADDRW_COLBIT_LSB 0
+#define DRAMADDRW_ROWBIT_MASK 0x02E0
+#define DRAMADDRW_ROWBIT_LSB 5
+#define DRAMADDRW_BANKBIT_MASK 0x1C00
+#define DRAMADDRW_BANKBIT_LSB 10
+#define DRAMADDRW_CSBIT_MASK 0xE000
+#define DRAMADDRW_CSBIT_LSB 13

(Insert few screens of code so that you have to scroll or split
window).

What information do the define names have? None, I already know how
hardware registers work.

+ col = (read_reg & DRAMADDRW_COLBIT_MASK) >>
+ DRAMADDRW_COLBIT_LSB;
+ row = (read_reg & DRAMADDRW_ROWBIT_MASK) >>
+ DRAMADDRW_ROWBIT_LSB;
+ bank = (read_reg & DRAMADDRW_BANKBIT_MASK) >>
+ DRAMADDRW_BANKBIT_LSB;
+ cs = (read_reg & DRAMADDRW_CSBIT_MASK) >>
+ DRAMADDRW_CSBIT_LSB;

Instead, we could have:

col = read_reg & 0x001F;
row = (read_reg & 0x02E0) >> 5;
bank = (read_reg & 0x1C00) >> 10;
cs = (read_reg & 0xE000) >> 13;

All information is preserved, code is shorter and easier to understand.

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


\
 
 \ /
  Last update: 2014-07-08 14:41    [W:0.077 / U:0.504 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site