Messages in this thread | | | Date | Fri, 4 Apr 2008 21:36:32 +0200 | From | Jean Delvare <> | Subject | Re: [patch/rfc 2/4] pcf875x I2C GPIO expander driver |
| |
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 in comparison to the whole chain from the user-space process to the GPIO chip.
Not that it really matters anyway, this is in no way a hot path so clarity and correctness definitely take over efficiency. And the code above is actually incorrect: as I mentioned elsewhere in this 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.
> (...) > >> +static void __exit gpio_class_exit(void) > >> +{ > >> + /* 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.
> (...) > >> @@ -118,6 +290,22 @@ int gpiochip_add(struct gpio_chip *chip) > >> } > >> > >> spin_unlock_irqrestore(&gpio_lock, flags); > >> + > >> + if (status) > >> + goto fail; > >> + > >> + if (chip->dev) { > >> + /* can sleep, so can't call with the spinlock held */ > > > > You don't actually hold the spinlock at this point. > > It would be easier to call gpiochip_classdev_register() earlier when the spin > lock is held, but one can't because it could sleep. The comment was a warning > to anyone who thought they could simplify the logic.
Oops, sorry. I totally missed the "can't" in the comment.
-- Jean Delvare
| |