Re: [patch/rfc 2/4] pcf875x I2C GPIO expander driver

[Trent Piepho]

On Fri, 4 Apr 2008, Jean Delvare wrote:
> On Fri, 4 Apr 2008 12:07:12 -0700 (PDT), Trent Piepho wrote:
> > On Fri, 4 Apr 2008, Jean Delvare wrote:
> > > On Thu, 3 Apr 2008 19:06:27 -0700 (PDT), Trent Piepho wrote:
> > > > +	strcpy(buf, test_bit(FLAG_IS_OUT, &gdesc->flags) ? "out\n" : "in\n\0");
> > > > +
> > > > +	return 5;
> > >
> > > Confusing construct... I suggest using sprintf instead, which will
> > > automatically return the correct number of bytes for you.
> >
> > But it's less efficient!  Will nobody think of the wasted cycles?
>
> Can you prove that it is actually less efficient, and if so, by how
> much? The time spent in this single function if probably insignificant

I think sprintf will parse the format string, and then end up calling the same
strcpy() call to handle a %s.  Since sprintf() contains the strcpy(), it has
to be slower than strcpy alone.

But I should have put a :) in there, as I changed this code.

> thread, you aren't support to include trailing \0s in the buffer you
> pass back to sysfs. Not all programming languages use \0 for string
> termination.

I fixed all those.  I wasn't clear on that when I wrote this code and forgot I
had done it incorrectly.

> > > > +	/* FIXME:  Code to remove all the sysfs devices and files created
> > > > +	 * should go here */
> > >
> > > Oh yes it really should ;)
> >
> > I know, but I'm not using modules for the system this is in, so it will never
> > get called.  What's the point of writing code I'll never use if this isn't
> > useful for the kernel?
>
> Because most certainly your code won't be accepted upstream until this
> is fixed, and presumably you posted this patch in the hope that it
> would go upstream ;) Just because it isn't useful to you doesn't mean
> it won't be useful to others. Otherwise this particular piece of code
> couldn't be built as a module at all.

I guess I was waiting for a "this could go upstream if you fix this" or "this
won't go upstream even if you fix it" so I don't waste time writing code no
one is interested in.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/