Re: [PATCH 4/7] staging: nvec: add NVEC_CALL helper macro

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Friday 15 June 2012 15:55:59 Greg KH wrote:
> On Sat, Jun 16, 2012 at 12:00:03AM +0200, Julian Andres Klode wrote:
> > On Fri, Jun 15, 2012 at 02:45:33PM -0700, Greg KH wrote:
> > > On Fri, Jun 15, 2012 at 08:40:26AM +0200, Julian Andres Klode wrote:
> > > > On Thu, Jun 14, 2012 at 03:09:38PM -0700, Greg KH wrote:
> > > > > On Thu, Jun 14, 2012 at 11:57:38PM +0200, Marc Dietrich wrote:
> > > > > > Add a helper macro to wrap nvec_{a}sync_writes and to get rid of
> > > > > > the various strings distributed all over the nvec code.
> > > > > 
> > > > > Why can't these be inline functions instead?  That will catch errors
> > > > > easier, and make it a bit more "obvious" as to what is going on
> > > > > (hint, I
> > > > > have no idea in reading these what they are doing...)
> > > > 
> > > > They are not really obvious, but they kind of catch more errors and
> > > > are shorter to writer.
> > > 
> > > Shorter to write where?
> > > 
> > > And obvious is good, we want obvious in the kernel.  Non-obvious is bad,
> > > bugs live there...
> > > 
> > > > A nvec "call" consists of a type, a subtype and a payload, so
> > > > 
> > > > the calls expand like this:
> > > >     nvec_write_async(nvec, {NVEC_FOO, NVEC_FOO_BAR, ...}, length of
> > > >     ... + 2)
> > > >     NVEC_CALL(nvec, FOO, BAR, ...)
> > > > 
> > > > With an inline function, you would have to use arrays (as
> > > > nvec_write_async() does) or variable arguments lists which are not as
> > > > optimizable, and you would need to repeat NVEC all over the place.
> > > > For example, with an inline function> > > 
> > > > instead of a macro:
> > > >     nvec_call(nvec, NVEC_FOO, NVEC_FOO_BAR, [size here?], ...)
> > > > 
> > > > This manual size tracking also makes it less reliable.
> > > 
> > > I'm sorry, but I still don't understand.  Why would you ever want any
> > > NVEC_CALL macros either?
> > > 
> > > What exactly is the driver doing here that is so "odd" from other data
> > > streams that need to be written to devices that it has to go through
> > > wierd gyrations like this?
> > 
> > The core point was probably that we currently have various sequences
> > like
> > 
> > 	char blah[] = {some hex value, other hex value, ...}
> > 	nvec_write_async(nvec, blah, sizeof(blah))
> > 
> > which is a bit long to write or distracting under some circumstances (and
> > not very obvious, as we did not use named constants for those hex values
> > 
> > most of the time). With a macro you could just write it:
> > 	NVEC_CALL(nvec, FOO, BAR)
> > 
> > And save a line. Would you prefer us to use:
> > 	{
> > 	
> > 		char msg[] = {NVEC_FOO, NVEC_FOO_BAR};
> > 		nvec_write_async(nvec, msg, sizeof(msg));
> > 	
> > 	}
> > 
> > instead?
> 
> Yes, don't you agree that this is more readable?
> 
> > And yes, macro-less code is much more readable than the
> > macro code for non-insiders, which is probably helpful.
> 
> Ah, you do agree, nice :)
> 
> Remember, write kernel code so that others can fix it 10 years in the
> future.  Don't be cute, play tricks, or do fancy things if at all
> possible.  Kernel code is written to be debugged by someone you have
> never met, whose native language is not your own, who does not have your
> device, and who you don't want having to email you asking questions.

The whole intention is to hide the various char arrays somehow. I agree that 
the call to send it to the embedded controller could be separated. 

We have something like 

	static const unsigned char EC_ENABLE_EVENT_REPORTING[3]  = "\x04\x00\x01";

now, and we want to replace this 3 byte string (which is in fact a smbus 
message) into something more understandable (or maintainable).

So the message consists of a target (0x04), a command (0x00), and a playload 
(0x01). Note that these strings can vary in length depending on the payload. 
We could add an enum for every byte here:

NVEC_SLEEP = 0x04
NVEC_SLEEP_GLOBAL_EVENTS = 0x00
NVEC_ENABLE = 0x01

so char buf[] = { NVEC_SLEEP, NVEC_SLEEP_GLOBAL_EVENTS, NVEC_ENABLE }

or use a macro to shorten this to

char buf[] = NVEC_CMD_STR(SLEEP, GLOBAL_EVENTS, NVEC_ENABLE)

using the same enums, but in my opinion more readable.

If macros are no go in this case, we could hide this in a function with 
variadic parameters also, but this adds more code/runtime and isn't more 
readable:

nvec_write_async(nvec, NVEC_SLEEP, NVEC_SLEEP_GLOBAL_EVENTS, NVEC_ENABLE, -1)

the "-1" is the arg terminator.

Greg, what do you prefer here?


Marc

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux