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
