On Thu, 3 Apr 2008 19:06:27 -0700 (PDT), Trent Piepho wrote:From c65d0fb239b79de7f595e47edb2fb641217e7309 Mon Sep 17 00:00:00 2001
From: Trent Piepho <tpiepho@xxxxxxxxxxxxx>
Date: Thu, 3 Apr 2008 18:37:23 -0700
Subject: GPIO: Create a sysfs gpio class for messing with GPIOs from userspace
struct gpio_chip gets a new field, dev, which points to the struct device
of whatever the gpio lines are part of. If this is non-NULL, gpio class
entries are created for this device when gpiochip_add() is called, by a new
function gpiochip_classdev_register().
It creates a sysfs directory for each gpio the chip defines. Each device
has three attributes so far, direction, value, and label. label is
read-only, and will only be present if DEBUG_FS is on, as without DEBUG_FS
the gpio layer doesn't keep track of any labels.
Purely technical review (I can't say whether this interface is better
than what has been proposed elsewhere or not):
+ 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.
+ } else {
+ d = simple_strtoul(buf, NULL, 0);
This exposes to user-space the so far internal-only decision to encode
output as 1 and input as 0. I don't see much benefit in doing this, in
particular when you don't check for errors so for example an input of
"Out" would result in value 0 i.e. input mode. I'd rather support only
"in" and "out" as valid values and return -EINVAL otherwise.
+ const struct gpio_desc *gdesc = dev_get_drvdata(dev);
+ int n = gdesc - gpio_desc;
+
+ if (test_bit(FLAG_IS_OUT, &gdesc->flags)) {
+ return -EINVAL;
+ /* strcpy(buf, "-1\n"); return 4; */ /* Or this? */
Why not just return gpio_get_value(n) in all cases? User might want to
know which value is currently being output.
+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 ;)
@@ -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.
* state (such as pullup/pulldown configuration).
+ * @dev: optional device for the GPIO chip. Used to create sysfs files.
Broken indentation.