On Sat, Jun 16, 2012 at 04:30:24PM +0200, Marc Dietrich wrote:
> 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.
Why? Why hide something that makes it easier to determine what is going
on?
> 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.
What's wrong with a structure:
struct message {
u8 target;
u8 command;
u8 payload[0];
};
And setting the fields and going from there? That's the way all other
driver subsystems do this...
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
