| Message ID | 1350069085-13283-3-git-send-email-stigge@antcom.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13/10/12 06:11, Roland Stigge wrote: > This patch adds sysfs support to the block GPIO API. > > Signed-off-by: Roland Stigge <stigge@antcom.de> Hi Roland, Some comments below, ~Ryan > > --- > Documentation/ABI/testing/sysfs-gpio | 6 > drivers/gpio/gpiolib.c | 226 ++++++++++++++++++++++++++++++++++- > include/asm-generic/gpio.h | 11 + > include/linux/gpio.h | 13 ++ > 4 files changed, 254 insertions(+), 2 deletions(-) > > --- linux-2.6.orig/Documentation/ABI/testing/sysfs-gpio > +++ linux-2.6/Documentation/ABI/testing/sysfs-gpio > @@ -24,4 +24,8 @@ Description: > /base ... (r/o) same as N > /label ... (r/o) descriptive, not necessarily unique > /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1) > - > + /blockN ... for each GPIO block #N > + /ngpio ... (r/o) number of GPIOs in this group > + /exported ... sysfs export state of this group (0, 1) > + /value ... current value as 32 or 64 bit integer in decimal > + (only available if /exported is 1) > --- linux-2.6.orig/drivers/gpio/gpiolib.c > +++ linux-2.6/drivers/gpio/gpiolib.c > @@ -974,6 +974,218 @@ static void gpiochip_unexport(struct gpi > chip->label, status); > } > > +static ssize_t gpio_block_ngpio_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + const struct gpio_block *block = dev_get_drvdata(dev); > + > + return sprintf(buf, "%u\n", block->ngpio); > +} > +static struct device_attribute > +dev_attr_block_ngpio = __ATTR(ngpio, 0444, gpio_block_ngpio_show, NULL); static DEVICE_ATTR(ngpio, S_IRUGO, gpio_block_ngpio_show, NULL); > + > +static ssize_t gpio_block_value_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + const struct gpio_block *block = dev_get_drvdata(dev); > + > + return sprintf(buf, "%u\n", gpio_block_get(block)); Printing the value of a bunch of pins as a decimal is a bit odd. Hex, or a bitmap would be more appropriate. > +} > + > +static bool gpio_block_is_output(struct gpio_block *block) > +{ > + int i; > + > + for (i = 0; i < block->ngpio; i++) > + if (!test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags)) > + return false; Shouldn't a block force all of the pins to be the same direction? Or at least have gpio_block_set skip pins which aren't outputs. > + return true; > +} > + > +static ssize_t gpio_block_value_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + ssize_t status; > + struct gpio_block *block = dev_get_drvdata(dev); > + unsigned long value; > + > + mutex_lock(&sysfs_lock); > + > + status = kstrtoul(buf, 0, &value); > + if (status == 0) { You don't need to do the kstrtoul under the lock: err = kstrtoul(buf, 0, &value); if (err) return err; mutex_lock(&sysfs_lock); ... Global lock is a bit lame, it serialises all of your bitbanged busses against each other. Why is it not part of the gpio_block structure? > + if (gpio_block_is_output(block)) { > + gpio_block_set(block, value); > + status = size; > + } else { > + status = -EPERM; > + } > + } > + > + mutex_unlock(&sysfs_lock); > + return status; > +} > + > +static struct device_attribute > +dev_attr_block_value = __ATTR(value, 0644, gpio_block_value_show, > + gpio_block_value_store); Use DEVICE_ATTR and S_IWUSR | S_IRUGO permission macros. > + > +static int gpio_block_value_is_exported(struct gpio_block *block) > +{ > + struct device *dev; > + struct sysfs_dirent *sd = NULL; > + > + mutex_lock(&sysfs_lock); > + dev = class_find_device(&gpio_class, NULL, block, match_export); > + if (!dev) > + goto out; > + > + sd = sysfs_get_dirent(dev->kobj.sd, NULL, "value"); > + > +out: > + mutex_unlock(&sysfs_lock); > + return sd ? 1 : 0; return sd; or: return !!sd; > +} > + > +static ssize_t gpio_block_exported_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct gpio_block *block = dev_get_drvdata(dev); > + > + return sprintf(buf, "%u\n", gpio_block_value_is_exported(block)); > +} > + > +static int gpio_block_value_export(struct gpio_block *block) > +{ > + struct device *dev; > + int status; > + int i; > + > + mutex_lock(&sysfs_lock); > + > + for (i = 0; i < block->ngpio; i++) { > + status = gpio_request(block->gpio[i], "sysfs"); > + if (status) > + goto out; > + } > + > + dev = class_find_device(&gpio_class, NULL, block, match_export); > + if (!dev) { > + status = -ENODEV; > + goto out; > + } > + > + status = device_create_file(dev, &dev_attr_block_value); > + if (status) > + goto out; > + > + mutex_unlock(&sysfs_lock); > + return 0; > + > +out: > + while (i > 0) { > + i--; > + gpio_free(block->gpio[i]); > + } Nitpick: while (--i >= 0) gpio_free(block->gpio[i]); > + > + mutex_unlock(&sysfs_lock); > + return status; > +} > + > +static int gpio_block_value_unexport(struct gpio_block *block) > +{ > + struct device *dev; > + int i; > + > + dev = class_find_device(&gpio_class, NULL, block, match_export); > + if (!dev) > + return -ENODEV; > + > + for (i = 0; i < block->ngpio; i++) > + gpio_free(block->gpio[i]); > + > + device_remove_file(dev, &dev_attr_block_value); > + > + return 0; > +} > + > +static ssize_t gpio_block_exported_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + long value; > + int status; > + struct gpio_block *block = dev_get_drvdata(dev); > + int exported = gpio_block_value_is_exported(block); > + > + status = kstrtoul(buf, 0, &value); > + if (status < 0) > + goto err; > + > + if (value != exported) { > + if (value) > + status = gpio_block_value_export(block); > + else > + status = gpio_block_value_unexport(block); > + if (!status) > + status = size; > + } else { > + status = size; > + } > +err: > + return status; > +} > + > +static DEVICE_ATTR(exported, 0644, gpio_block_exported_show, > + gpio_block_exported_store); > + > +static const struct attribute *gpio_block_attrs[] = { > + &dev_attr_block_ngpio.attr, > + &dev_attr_exported.attr, > + NULL, > +}; > + > +static const struct attribute_group gpio_block_attr_group = { > + .attrs = (struct attribute **) gpio_block_attrs, > +}; > + > +int gpio_block_export(struct gpio_block *block) > +{ > + int status; > + struct device *dev; > + > + /* can't export until sysfs is available ... */ > + if (!gpio_class.p) { > + pr_debug("%s: called too early!\n", __func__); > + return -ENOENT; > + } > + > + mutex_lock(&sysfs_lock); > + dev = device_create(&gpio_class, NULL, MKDEV(0, 0), block, > + block->name); > + if (!IS_ERR(dev)) > + status = sysfs_create_group(&dev->kobj, &gpio_block_attr_group); > + else > + status = PTR_ERR(dev); > + mutex_unlock(&sysfs_lock); > + > + return status; > +} > +EXPORT_SYMBOL_GPL(gpio_block_export); > + > +void gpio_block_unexport(struct gpio_block *block) > +{ > + struct device *dev; > + > + mutex_lock(&sysfs_lock); > + dev = class_find_device(&gpio_class, NULL, block, match_export); > + if (dev) > + device_unregister(dev); > + mutex_unlock(&sysfs_lock); > +} > +EXPORT_SYMBOL_GPL(gpio_block_unexport); > + > static int __init gpiolib_sysfs_init(void) > { > int status; > @@ -1882,7 +2094,14 @@ int gpio_block_register(struct gpio_bloc > break; > } > } > - return i == NR_GPIO_BLOCKS ? -EBUSY : 0; > + if (i == NR_GPIO_BLOCKS) > + goto err; > + > + gpio_block_export(block); > + > + return 0; > +err: > + return -EBUSY; > } > EXPORT_SYMBOL_GPL(gpio_block_register); > > @@ -1896,6 +2115,11 @@ void gpio_block_unregister(struct gpio_b > break; > } > } > + > + if (i == NR_GPIO_BLOCKS) > + return; > + > + gpio_block_unexport(block); > } > EXPORT_SYMBOL_GPL(gpio_block_unregister); > > --- linux-2.6.orig/include/asm-generic/gpio.h > +++ linux-2.6/include/asm-generic/gpio.h > @@ -210,6 +210,8 @@ extern int gpio_export_link(struct devic > unsigned gpio); > extern int gpio_sysfs_set_active_low(unsigned gpio, int value); > extern void gpio_unexport(unsigned gpio); > +extern int gpio_block_export(struct gpio_block *block); > +extern void gpio_block_unexport(struct gpio_block *block); > > #endif /* CONFIG_GPIO_SYSFS */ > > @@ -269,6 +271,15 @@ static inline int gpio_sysfs_set_active_ > static inline void gpio_unexport(unsigned gpio) > { > } > + > +static inline int gpio_block_export(struct gpio_block *block) > +{ > + return -ENOSYS; > +} > + > +static inline void gpio_block_unexport(struct gpio_block *block) > +{ > +} > #endif /* CONFIG_GPIO_SYSFS */ > > #endif /* _ASM_GENERIC_GPIO_H */ > --- linux-2.6.orig/include/linux/gpio.h > +++ linux-2.6/include/linux/gpio.h > @@ -278,6 +278,19 @@ static inline void gpio_unexport(unsigne > WARN_ON(1); > } > > +static inline int gpio_block_export(struct gpio_block *block) > +{ > + /* GPIO block can never have been requested or set as {in,out}put */ > + WARN_ON(1); > + return -EINVAL; > +} > + > +static inline void gpio_block_unexport(struct gpio_block *block) > +{ > + /* GPIO block can never have been exported */ > + WARN_ON(1); > +} > + > static inline int gpio_to_irq(unsigned gpio) > { > /* GPIO can never have been requested or set as input */ > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
On 10/15/2012 07:35 AM, Ryan Mallon wrote: >> --- linux-2.6.orig/Documentation/ABI/testing/sysfs-gpio >> +++ linux-2.6/Documentation/ABI/testing/sysfs-gpio >> @@ -24,4 +24,8 @@ Description: >> /base ... (r/o) same as N >> /label ... (r/o) descriptive, not necessarily unique >> /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1) >> - >> + /blockN ... for each GPIO block #N >> + /ngpio ... (r/o) number of GPIOs in this group >> + /exported ... sysfs export state of this group (0, 1) >> + /value ... current value as 32 or 64 bit integer in decimal >> + (only available if /exported is 1) >> --- linux-2.6.orig/drivers/gpio/gpiolib.c >> +++ linux-2.6/drivers/gpio/gpiolib.c >> @@ -974,6 +974,218 @@ static void gpiochip_unexport(struct gpi >> chip->label, status); >> } >> >> +static ssize_t gpio_block_ngpio_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + const struct gpio_block *block = dev_get_drvdata(dev); >> + >> + return sprintf(buf, "%u\n", block->ngpio); >> +} >> +static struct device_attribute >> +dev_attr_block_ngpio = __ATTR(ngpio, 0444, gpio_block_ngpio_show, NULL); > > static DEVICE_ATTR(ngpio, S_IRUGO, gpio_block_ngpio_show, NULL); Of course I would have used this. :-) But DEVICE_ATTR isn't flexible enough here. There is already another "ngpio" in this file (resulting in its name "dev_attr_ngpio") for gpio chips, colliding with this attribute here (only on C source level - for sysfs it's fine). Using S_IRUGO - thanks. >> +} >> + >> +static bool gpio_block_is_output(struct gpio_block *block) >> +{ >> + int i; >> + >> + for (i = 0; i < block->ngpio; i++) >> + if (!test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags)) >> + return false; > > Shouldn't a block force all of the pins to be the same direction? Or at > least have gpio_block_set skip pins which aren't outputs. It is again analogous to GPIOs themselves: The sysfs interface prevents Oopses by checking for the direction with the above function. Otherwise, the user is responsible for requesting and setting direction. >> +static ssize_t gpio_block_value_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t size) >> +{ >> + ssize_t status; >> + struct gpio_block *block = dev_get_drvdata(dev); >> + unsigned long value; >> + >> + mutex_lock(&sysfs_lock); >> + >> + status = kstrtoul(buf, 0, &value); >> + if (status == 0) { > > You don't need to do the kstrtoul under the lock: > > err = kstrtoul(buf, 0, &value); > if (err) > return err; > > mutex_lock(&sysfs_lock); > ... > > Global lock is a bit lame, it serialises all of your bitbanged busses > against each other. Why is it not part of the gpio_block structure? It's the same strategy as for GPIO value get/set. More importantly maybe: Consider 32 GPIO lines on a single GPIO controller. Several defined, say, 8 bit buses defined on this single hardware word actually need to be locked against each other. So sticking with it for now. >> + if (gpio_block_is_output(block)) { >> + gpio_block_set(block, value); >> + status = size; >> + } else { >> + status = -EPERM; >> + } >> + } >> + >> + mutex_unlock(&sysfs_lock); >> + return status; >> +} >> + >> +static struct device_attribute >> +dev_attr_block_value = __ATTR(value, 0644, gpio_block_value_show, >> + gpio_block_value_store); > > Use DEVICE_ATTR and S_IWUSR | S_IRUGO permission macros. Regarding DEVICE_ATTR as above. But adopting S_IWUSR | S_IRUGO - thanks. Again, including all the other suggestions in the next update. Thanks, Roland
On 21:11 Fri 12 Oct , Roland Stigge wrote: > This patch adds sysfs support to the block GPIO API. > > Signed-off-by: Roland Stigge <stigge@antcom.de> > > --- > Documentation/ABI/testing/sysfs-gpio | 6 > drivers/gpio/gpiolib.c | 226 ++++++++++++++++++++++++++++++++++- > include/asm-generic/gpio.h | 11 + > include/linux/gpio.h | 13 ++ > 4 files changed, 254 insertions(+), 2 deletions(-) I really don't like this sysfs we need to add a specific device with ioctl I put Greg in Cc Best Regards, J. > > --- linux-2.6.orig/Documentation/ABI/testing/sysfs-gpio > +++ linux-2.6/Documentation/ABI/testing/sysfs-gpio > @@ -24,4 +24,8 @@ Description: > /base ... (r/o) same as N > /label ... (r/o) descriptive, not necessarily unique > /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1) > - > + /blockN ... for each GPIO block #N > + /ngpio ... (r/o) number of GPIOs in this group > + /exported ... sysfs export state of this group (0, 1) > + /value ... current value as 32 or 64 bit integer in decimal > + (only available if /exported is 1) > --- linux-2.6.orig/drivers/gpio/gpiolib.c > +++ linux-2.6/drivers/gpio/gpiolib.c > @@ -974,6 +974,218 @@ static void gpiochip_unexport(struct gpi > chip->label, status); > } > > +static ssize_t gpio_block_ngpio_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + const struct gpio_block *block = dev_get_drvdata(dev); > + > + return sprintf(buf, "%u\n", block->ngpio); > +} > +static struct device_attribute > +dev_attr_block_ngpio = __ATTR(ngpio, 0444, gpio_block_ngpio_show, NULL); > + > +static ssize_t gpio_block_value_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + const struct gpio_block *block = dev_get_drvdata(dev); > + > + return sprintf(buf, "%u\n", gpio_block_get(block)); > +} > + > +static bool gpio_block_is_output(struct gpio_block *block) > +{ > + int i; > + > + for (i = 0; i < block->ngpio; i++) > + if (!test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags)) > + return false; > + return true; > +} > + > +static ssize_t gpio_block_value_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + ssize_t status; > + struct gpio_block *block = dev_get_drvdata(dev); > + unsigned long value; > + > + mutex_lock(&sysfs_lock); > + > + status = kstrtoul(buf, 0, &value); > + if (status == 0) { > + if (gpio_block_is_output(block)) { > + gpio_block_set(block, value); > + status = size; > + } else { > + status = -EPERM; > + } > + } > + > + mutex_unlock(&sysfs_lock); > + return status; > +} > + > +static struct device_attribute > +dev_attr_block_value = __ATTR(value, 0644, gpio_block_value_show, > + gpio_block_value_store); > + > +static int gpio_block_value_is_exported(struct gpio_block *block) > +{ > + struct device *dev; > + struct sysfs_dirent *sd = NULL; > + > + mutex_lock(&sysfs_lock); > + dev = class_find_device(&gpio_class, NULL, block, match_export); > + if (!dev) > + goto out; > + > + sd = sysfs_get_dirent(dev->kobj.sd, NULL, "value"); > + > +out: > + mutex_unlock(&sysfs_lock); > + return sd ? 1 : 0; > +} > + > +static ssize_t gpio_block_exported_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct gpio_block *block = dev_get_drvdata(dev); > + > + return sprintf(buf, "%u\n", gpio_block_value_is_exported(block)); > +} > + > +static int gpio_block_value_export(struct gpio_block *block) > +{ > + struct device *dev; > + int status; > + int i; > + > + mutex_lock(&sysfs_lock); > + > + for (i = 0; i < block->ngpio; i++) { > + status = gpio_request(block->gpio[i], "sysfs"); > + if (status) > + goto out; > + } > + > + dev = class_find_device(&gpio_class, NULL, block, match_export); > + if (!dev) { > + status = -ENODEV; > + goto out; > + } > + > + status = device_create_file(dev, &dev_attr_block_value); > + if (status) > + goto out; > + > + mutex_unlock(&sysfs_lock); > + return 0; > + > +out: > + while (i > 0) { > + i--; > + gpio_free(block->gpio[i]); > + } > + > + mutex_unlock(&sysfs_lock); > + return status; > +} > + > +static int gpio_block_value_unexport(struct gpio_block *block) > +{ > + struct device *dev; > + int i; > + > + dev = class_find_device(&gpio_class, NULL, block, match_export); > + if (!dev) > + return -ENODEV; > + > + for (i = 0; i < block->ngpio; i++) > + gpio_free(block->gpio[i]); > + > + device_remove_file(dev, &dev_attr_block_value); > + > + return 0; > +} > + > +static ssize_t gpio_block_exported_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + long value; > + int status; > + struct gpio_block *block = dev_get_drvdata(dev); > + int exported = gpio_block_value_is_exported(block); > + > + status = kstrtoul(buf, 0, &value); > + if (status < 0) > + goto err; > + > + if (value != exported) { > + if (value) > + status = gpio_block_value_export(block); > + else > + status = gpio_block_value_unexport(block); > + if (!status) > + status = size; > + } else { > + status = size; > + } > +err: > + return status; > +} > + > +static DEVICE_ATTR(exported, 0644, gpio_block_exported_show, > + gpio_block_exported_store); > + > +static const struct attribute *gpio_block_attrs[] = { > + &dev_attr_block_ngpio.attr, > + &dev_attr_exported.attr, > + NULL, > +}; > + > +static const struct attribute_group gpio_block_attr_group = { > + .attrs = (struct attribute **) gpio_block_attrs, > +}; > + > +int gpio_block_export(struct gpio_block *block) > +{ > + int status; > + struct device *dev; > + > + /* can't export until sysfs is available ... */ > + if (!gpio_class.p) { > + pr_debug("%s: called too early!\n", __func__); > + return -ENOENT; > + } > + > + mutex_lock(&sysfs_lock); > + dev = device_create(&gpio_class, NULL, MKDEV(0, 0), block, > + block->name); > + if (!IS_ERR(dev)) > + status = sysfs_create_group(&dev->kobj, &gpio_block_attr_group); > + else > + status = PTR_ERR(dev); > + mutex_unlock(&sysfs_lock); > + > + return status; > +} > +EXPORT_SYMBOL_GPL(gpio_block_export); > + > +void gpio_block_unexport(struct gpio_block *block) > +{ > + struct device *dev; > + > + mutex_lock(&sysfs_lock); > + dev = class_find_device(&gpio_class, NULL, block, match_export); > + if (dev) > + device_unregister(dev); > + mutex_unlock(&sysfs_lock); > +} > +EXPORT_SYMBOL_GPL(gpio_block_unexport); > + > static int __init gpiolib_sysfs_init(void) > { > int status; > @@ -1882,7 +2094,14 @@ int gpio_block_register(struct gpio_bloc > break; > } > } > - return i == NR_GPIO_BLOCKS ? -EBUSY : 0; > + if (i == NR_GPIO_BLOCKS) > + goto err; > + > + gpio_block_export(block); > + > + return 0; > +err: > + return -EBUSY; > } > EXPORT_SYMBOL_GPL(gpio_block_register); > > @@ -1896,6 +2115,11 @@ void gpio_block_unregister(struct gpio_b > break; > } > } > + > + if (i == NR_GPIO_BLOCKS) > + return; > + > + gpio_block_unexport(block); > } > EXPORT_SYMBOL_GPL(gpio_block_unregister); > > --- linux-2.6.orig/include/asm-generic/gpio.h > +++ linux-2.6/include/asm-generic/gpio.h > @@ -210,6 +210,8 @@ extern int gpio_export_link(struct devic > unsigned gpio); > extern int gpio_sysfs_set_active_low(unsigned gpio, int value); > extern void gpio_unexport(unsigned gpio); > +extern int gpio_block_export(struct gpio_block *block); > +extern void gpio_block_unexport(struct gpio_block *block); > > #endif /* CONFIG_GPIO_SYSFS */ > > @@ -269,6 +271,15 @@ static inline int gpio_sysfs_set_active_ > static inline void gpio_unexport(unsigned gpio) > { > } > + > +static inline int gpio_block_export(struct gpio_block *block) > +{ > + return -ENOSYS; > +} > + > +static inline void gpio_block_unexport(struct gpio_block *block) > +{ > +} > #endif /* CONFIG_GPIO_SYSFS */ > > #endif /* _ASM_GENERIC_GPIO_H */ > --- linux-2.6.orig/include/linux/gpio.h > +++ linux-2.6/include/linux/gpio.h > @@ -278,6 +278,19 @@ static inline void gpio_unexport(unsigne > WARN_ON(1); > } > > +static inline int gpio_block_export(struct gpio_block *block) > +{ > + /* GPIO block can never have been requested or set as {in,out}put */ > + WARN_ON(1); > + return -EINVAL; > +} > + > +static inline void gpio_block_unexport(struct gpio_block *block) > +{ > + /* GPIO block can never have been exported */ > + WARN_ON(1); > +} > + > static inline int gpio_to_irq(unsigned gpio) > { > /* GPIO can never have been requested or set as input */
On 10/15/2012 08:07 PM, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 21:11 Fri 12 Oct , Roland Stigge wrote: >> This patch adds sysfs support to the block GPIO API. >> >> Signed-off-by: Roland Stigge <stigge@antcom.de> >> >> --- >> Documentation/ABI/testing/sysfs-gpio | 6 >> drivers/gpio/gpiolib.c | 226 ++++++++++++++++++++++++++++++++++- >> include/asm-generic/gpio.h | 11 + >> include/linux/gpio.h | 13 ++ >> 4 files changed, 254 insertions(+), 2 deletions(-) > I really don't like this sysfs we need to add a specific device with ioctl The good thing about the current style is that you have a single value for the whole block in a single file just as for single GPIOs. You can consider it as a generalized GPIO. And I like it. :-) Thanks for considering, Roland
On Mon, Oct 15, 2012 at 08:07:02PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 21:11 Fri 12 Oct , Roland Stigge wrote: > > This patch adds sysfs support to the block GPIO API. > > > > Signed-off-by: Roland Stigge <stigge@antcom.de> > > > > --- > > Documentation/ABI/testing/sysfs-gpio | 6 > > drivers/gpio/gpiolib.c | 226 ++++++++++++++++++++++++++++++++++- > > include/asm-generic/gpio.h | 11 + > > include/linux/gpio.h | 13 ++ > > 4 files changed, 254 insertions(+), 2 deletions(-) > I really don't like this sysfs we need to add a specific device with ioctl Why? > I put Greg in Cc Again, why? greg k-h
I really request Grant to comment on this...too. On Mon, Oct 15, 2012 at 8:19 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Mon, Oct 15, 2012 at 08:07:02PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: >> On 21:11 Fri 12 Oct , Roland Stigge wrote: >> > This patch adds sysfs support to the block GPIO API. >> > >> > Signed-off-by: Roland Stigge <stigge@antcom.de> >> > >> > --- >> > Documentation/ABI/testing/sysfs-gpio | 6 >> > drivers/gpio/gpiolib.c | 226 ++++++++++++++++++++++++++++++++++- >> > include/asm-generic/gpio.h | 11 + >> > include/linux/gpio.h | 13 ++ >> > 4 files changed, 254 insertions(+), 2 deletions(-) >> I really don't like this sysfs we need to add a specific device with ioctl > > Why? I don't like it either, basically because the GPIO sysfs is not entirely sound. Another patch that is circulating concerns edge triggers and similar, and it appear that some parts of the GPIO sysfs is for example redefining and exporting IRQchip properties like trigger edge in sysfs, while the settings of the irqchip actually used by the driver is not reflected in the other direction. So you can *set* these things by writing in the GPIO sysfs, but never trust what you *read* from there. And you can set what edges an IRQ will trigger on a certain GPIO, and the way to handle the IRQs from usespace is to poll on a value. This is not really documented but well ... Part of me just want to delete that, but I can't because it's now an ABI. The "devices" that the sysfs files are tied to are not real devices, instead the code look like this: whenever a gpio is exported to sysfs, the code calls (drivers/gpio/gpiolib.c): dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0), desc, ioname ? ioname : "gpio%u", gpio); Mock device just to get a sysfs opening. And once device for every GPIO with no hierarchical correspondence to anything in the system. The thing is that struct gpio_chip is not a device at all, it's something else. This inconsistency in the GPIO sysfs implementation make me fear adding new stuff to it. The other problems need fixing first. The reason an ioctl() IMO is better suited to do the job is that it can properly represent a multiple-value operation on several GPIOs at the same time in a struct, and it can conversely inform userspace about which GPIOs may be a block of signals that can be fired simultaneously instead of going to string parsing and binary values in sysfs which look like worse hacks to me. The last thing I'm a bit softer on though. Mainly I fear of this sysfs ABI growing into a beast. It was all merged prior to Grant becoming maintainer and me becoming co-maintainer of it, so this is legacy business. Sadly the main creator of this ABI is David Brownell who is not able to respond nor maintain it from where he is now. But we need to think hard about what we shall do with this particular piece of legacy. Some of the stuff was added by Daniel Glöckner so requesting advice from him. Daniel: are you interested in helping us fixing the GPIOlib sysfs ABI and kernel internals? I'm a bit afraid of it. Yours, Linus Walleij
On 15/10/12 22:30, Linus Walleij wrote: > I don't like it either, basically because the GPIO sysfs is not > entirely sound. > > Another patch that is circulating concerns edge triggers and similar, > and it appear that some parts of the GPIO sysfs is for example > redefining and exporting IRQchip properties like trigger edge > in sysfs, while the settings of the irqchip actually used by the driver > is not reflected in the other direction. So you can *set* these things > by writing in the GPIO sysfs, but never trust what you *read* from > there. And you can set what edges an IRQ will trigger on a certain > GPIO, and the way to handle the IRQs from usespace is to poll > on a value. This is not really documented but well ... I'm not convinced this generally also applies to the block GPIO patches. Or that it can't be fixed. > The reason an ioctl() IMO is better suited to do the job is that > it can properly represent a multiple-value operation on several > GPIOs at the same time in a struct, and it can conversely inform > userspace about which GPIOs may be a block of signals that > can be fired simultaneously instead of going to string parsing > and binary values in sysfs which look like worse hacks to me. There is no binary values in the sysfs for the block GPIO patches, and regarding string parsing - yes, that's sysfs. :-) Roland
On 22:30 Mon 15 Oct , Linus Walleij wrote: > I really request Grant to comment on this...too. > > On Mon, Oct 15, 2012 at 8:19 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Mon, Oct 15, 2012 at 08:07:02PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > >> On 21:11 Fri 12 Oct , Roland Stigge wrote: > >> > This patch adds sysfs support to the block GPIO API. > >> > > >> > Signed-off-by: Roland Stigge <stigge@antcom.de> > >> > > >> > --- > >> > Documentation/ABI/testing/sysfs-gpio | 6 > >> > drivers/gpio/gpiolib.c | 226 ++++++++++++++++++++++++++++++++++- > >> > include/asm-generic/gpio.h | 11 + > >> > include/linux/gpio.h | 13 ++ > >> > 4 files changed, 254 insertions(+), 2 deletions(-) > >> I really don't like this sysfs we need to add a specific device with ioctl > > > > Why? > > I don't like it either, basically because the GPIO sysfs is not > entirely sound. > > Another patch that is circulating concerns edge triggers and similar, > and it appear that some parts of the GPIO sysfs is for example > redefining and exporting IRQchip properties like trigger edge > in sysfs, while the settings of the irqchip actually used by the driver > is not reflected in the other direction. So you can *set* these things > by writing in the GPIO sysfs, but never trust what you *read* from > there. And you can set what edges an IRQ will trigger on a certain > GPIO, and the way to handle the IRQs from usespace is to poll > on a value. This is not really documented but well ... > > Part of me just want to delete that, but I can't because it's now > an ABI. > > The "devices" that the sysfs files are tied to are not real devices, > instead the code look like this: whenever a gpio is exported to > sysfs, the code calls (drivers/gpio/gpiolib.c): > > dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0), > desc, ioname ? ioname : "gpio%u", gpio); > > Mock device just to get a sysfs opening. And once device for > every GPIO with no hierarchical correspondence to anything > in the system. > > The thing is that struct gpio_chip is not a device at all, it's something > else. > > This inconsistency in the GPIO sysfs implementation make me > fear adding new stuff to it. The other problems need fixing first. > > The reason an ioctl() IMO is better suited to do the job is that > it can properly represent a multiple-value operation on several > GPIOs at the same time in a struct, and it can conversely inform > userspace about which GPIOs may be a block of signals that > can be fired simultaneously instead of going to string parsing > and binary values in sysfs which look like worse hacks to me. > > The last thing I'm a bit softer on though. Mainly I fear of this > sysfs ABI growing into a beast. > > It was all merged prior to Grant becoming maintainer and > me becoming co-maintainer of it, so this is legacy business. > > Sadly the main creator of this ABI is David Brownell who is > not able to respond nor maintain it from where he is now. But > we need to think hard about what we shall do with this particular > piece of legacy. Some of the stuff was added by Daniel > Glöckner so requesting advice from him. > > Daniel: are you interested in helping us fixing the GPIOlib > sysfs ABI and kernel internals? I'm a bit afraid of it. My 0.02$ too Best Regards, J.
Hi, sorry for the late reply. I'm currently on vacation and it is no fun to use SSH with a 1s latency while Entel Chile injects RST/ACK packets. I'll read the remaining related mails that have accumulated in my inbox when I'm back home. On Mon, Oct 15, 2012 at 10:30:15PM +0200, Linus Walleij wrote: > Another patch that is circulating concerns edge triggers and similar, > and it appear that some parts of the GPIO sysfs is for example > redefining and exporting IRQchip properties like trigger edge > in sysfs, while the settings of the irqchip actually used by the driver > is not reflected in the other direction. So you can *set* these things > by writing in the GPIO sysfs, but never trust what you *read* from > there. And you can set what edges an IRQ will trigger on a certain > GPIO, and the way to handle the IRQs from usespace is to poll > on a value. This is not really documented but well ... Part of this sounds like you are not familiar with the GPIOlib sysfs IRQ stuff. The trigger edge set in sysfs is only used when userspace polls the GPIO via sysfs. Drivers that want to register a gpio IRQ should IMHO always explicitly request the edge/level to trigger on and they should request the gpio beforehand. This prevents the gpio from being exported to userspace. Only IRQ triggers accepted by the irq chip are settable in sysfs, so you can trust the value read from that file. > Sadly the main creator of this ABI is David Brownell who is > not able to respond nor maintain it from where he is now. But > we need to think hard about what we shall do with this particular > piece of legacy. Some of the stuff was added by Daniel > Glöckner so requesting advice from him. I'm only guilty of adding the IRQ sysfs interface. > Daniel: are you interested in helping us fixing the GPIOlib > sysfs ABI and kernel internals? I'm a bit afraid of it. Actually I don't know what you want to change to fix the existing sysfs ABI. Personally I'd like to see the following things changed: - /sys/gpio/.../direction does not correspond to hardware before first use of gpio_direction_* due to lack of gpio_get_direction. - Names given to gpios by the chip should just result in symlinks to the usual gpioX directories or (un)exporting of gpios should accept names. Best regards, Daniel
On Thu, Oct 18, 2012 at 6:38 AM, Daniel Glöckner <daniel-gl@gmx.net> wrote: > On Mon, Oct 15, 2012 at 10:30:15PM +0200, Linus Walleij wrote: >> >> Another patch that is circulating concerns edge triggers and similar, >> and it appear that some parts of the GPIO sysfs is for example >> redefining and exporting IRQchip properties like trigger edge >> in sysfs, while the settings of the irqchip actually used by the driver >> is not reflected in the other direction. So you can *set* these things >> by writing in the GPIO sysfs, but never trust what you *read* from >> there. And you can set what edges an IRQ will trigger on a certain >> GPIO, and the way to handle the IRQs from usespace is to poll >> on a value. This is not really documented but well ... > > Part of this sounds like you are not familiar with the GPIOlib sysfs > IRQ stuff. Yeah you bet :-) I'm just trying to maintain it, I think I need your help with this. Do you think it'd be possible for you to augment the Documentation/gpio.txt file with some userspace code examples? I think this could be very useful. > The trigger edge set in sysfs is only used when userspace > polls the GPIO via sysfs. Drivers that want to register a gpio IRQ > should IMHO always explicitly request the edge/level to trigger on and > they should request the gpio beforehand. This prevents the gpio from > being exported to userspace. Only IRQ triggers accepted by the irq chip > are settable in sysfs, so you can trust the value read from that file. OK. >> Daniel: are you interested in helping us fixing the GPIOlib >> sysfs ABI and kernel internals? I'm a bit afraid of it. > > Actually I don't know what you want to change to fix the existing sysfs > ABI. Personally I'd like to see the following things changed: > - /sys/gpio/.../direction does not correspond to hardware before first > use of gpio_direction_* due to lack of gpio_get_direction. > - Names given to gpios by the chip should just result in symlinks to > the usual gpioX directories or (un)exporting of gpios should accept > names. Please send a patch! I'll merge. Actually I'm mostly referring to another floating patch, I will try to dig it up and CC you. Yours, Linus Walleij
--- linux-2.6.orig/Documentation/ABI/testing/sysfs-gpio +++ linux-2.6/Documentation/ABI/testing/sysfs-gpio @@ -24,4 +24,8 @@ Description: /base ... (r/o) same as N /label ... (r/o) descriptive, not necessarily unique /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1) - + /blockN ... for each GPIO block #N + /ngpio ... (r/o) number of GPIOs in this group + /exported ... sysfs export state of this group (0, 1) + /value ... current value as 32 or 64 bit integer in decimal + (only available if /exported is 1) --- linux-2.6.orig/drivers/gpio/gpiolib.c +++ linux-2.6/drivers/gpio/gpiolib.c @@ -974,6 +974,218 @@ static void gpiochip_unexport(struct gpi chip->label, status); } +static ssize_t gpio_block_ngpio_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + const struct gpio_block *block = dev_get_drvdata(dev); + + return sprintf(buf, "%u\n", block->ngpio); +} +static struct device_attribute +dev_attr_block_ngpio = __ATTR(ngpio, 0444, gpio_block_ngpio_show, NULL); + +static ssize_t gpio_block_value_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + const struct gpio_block *block = dev_get_drvdata(dev); + + return sprintf(buf, "%u\n", gpio_block_get(block)); +} + +static bool gpio_block_is_output(struct gpio_block *block) +{ + int i; + + for (i = 0; i < block->ngpio; i++) + if (!test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags)) + return false; + return true; +} + +static ssize_t gpio_block_value_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + ssize_t status; + struct gpio_block *block = dev_get_drvdata(dev); + unsigned long value; + + mutex_lock(&sysfs_lock); + + status = kstrtoul(buf, 0, &value); + if (status == 0) { + if (gpio_block_is_output(block)) { + gpio_block_set(block, value); + status = size; + } else { + status = -EPERM; + } + } + + mutex_unlock(&sysfs_lock); + return status; +} + +static struct device_attribute +dev_attr_block_value = __ATTR(value, 0644, gpio_block_value_show, + gpio_block_value_store); + +static int gpio_block_value_is_exported(struct gpio_block *block) +{ + struct device *dev; + struct sysfs_dirent *sd = NULL; + + mutex_lock(&sysfs_lock); + dev = class_find_device(&gpio_class, NULL, block, match_export); + if (!dev) + goto out; + + sd = sysfs_get_dirent(dev->kobj.sd, NULL, "value"); + +out: + mutex_unlock(&sysfs_lock); + return sd ? 1 : 0; +} + +static ssize_t gpio_block_exported_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct gpio_block *block = dev_get_drvdata(dev); + + return sprintf(buf, "%u\n", gpio_block_value_is_exported(block)); +} + +static int gpio_block_value_export(struct gpio_block *block) +{ + struct device *dev; + int status; + int i; + + mutex_lock(&sysfs_lock); + + for (i = 0; i < block->ngpio; i++) { + status = gpio_request(block->gpio[i], "sysfs"); + if (status) + goto out; + } + + dev = class_find_device(&gpio_class, NULL, block, match_export); + if (!dev) { + status = -ENODEV; + goto out; + } + + status = device_create_file(dev, &dev_attr_block_value); + if (status) + goto out; + + mutex_unlock(&sysfs_lock); + return 0; + +out: + while (i > 0) { + i--; + gpio_free(block->gpio[i]); + } + + mutex_unlock(&sysfs_lock); + return status; +} + +static int gpio_block_value_unexport(struct gpio_block *block) +{ + struct device *dev; + int i; + + dev = class_find_device(&gpio_class, NULL, block, match_export); + if (!dev) + return -ENODEV; + + for (i = 0; i < block->ngpio; i++) + gpio_free(block->gpio[i]); + + device_remove_file(dev, &dev_attr_block_value); + + return 0; +} + +static ssize_t gpio_block_exported_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + long value; + int status; + struct gpio_block *block = dev_get_drvdata(dev); + int exported = gpio_block_value_is_exported(block); + + status = kstrtoul(buf, 0, &value); + if (status < 0) + goto err; + + if (value != exported) { + if (value) + status = gpio_block_value_export(block); + else + status = gpio_block_value_unexport(block); + if (!status) + status = size; + } else { + status = size; + } +err: + return status; +} + +static DEVICE_ATTR(exported, 0644, gpio_block_exported_show, + gpio_block_exported_store); + +static const struct attribute *gpio_block_attrs[] = { + &dev_attr_block_ngpio.attr, + &dev_attr_exported.attr, + NULL, +}; + +static const struct attribute_group gpio_block_attr_group = { + .attrs = (struct attribute **) gpio_block_attrs, +}; + +int gpio_block_export(struct gpio_block *block) +{ + int status; + struct device *dev; + + /* can't export until sysfs is available ... */ + if (!gpio_class.p) { + pr_debug("%s: called too early!\n", __func__); + return -ENOENT; + } + + mutex_lock(&sysfs_lock); + dev = device_create(&gpio_class, NULL, MKDEV(0, 0), block, + block->name); + if (!IS_ERR(dev)) + status = sysfs_create_group(&dev->kobj, &gpio_block_attr_group); + else + status = PTR_ERR(dev); + mutex_unlock(&sysfs_lock); + + return status; +} +EXPORT_SYMBOL_GPL(gpio_block_export); + +void gpio_block_unexport(struct gpio_block *block) +{ + struct device *dev; + + mutex_lock(&sysfs_lock); + dev = class_find_device(&gpio_class, NULL, block, match_export); + if (dev) + device_unregister(dev); + mutex_unlock(&sysfs_lock); +} +EXPORT_SYMBOL_GPL(gpio_block_unexport); + static int __init gpiolib_sysfs_init(void) { int status; @@ -1882,7 +2094,14 @@ int gpio_block_register(struct gpio_bloc break; } } - return i == NR_GPIO_BLOCKS ? -EBUSY : 0; + if (i == NR_GPIO_BLOCKS) + goto err; + + gpio_block_export(block); + + return 0; +err: + return -EBUSY; } EXPORT_SYMBOL_GPL(gpio_block_register); @@ -1896,6 +2115,11 @@ void gpio_block_unregister(struct gpio_b break; } } + + if (i == NR_GPIO_BLOCKS) + return; + + gpio_block_unexport(block); } EXPORT_SYMBOL_GPL(gpio_block_unregister); --- linux-2.6.orig/include/asm-generic/gpio.h +++ linux-2.6/include/asm-generic/gpio.h @@ -210,6 +210,8 @@ extern int gpio_export_link(struct devic unsigned gpio); extern int gpio_sysfs_set_active_low(unsigned gpio, int value); extern void gpio_unexport(unsigned gpio); +extern int gpio_block_export(struct gpio_block *block); +extern void gpio_block_unexport(struct gpio_block *block); #endif /* CONFIG_GPIO_SYSFS */ @@ -269,6 +271,15 @@ static inline int gpio_sysfs_set_active_ static inline void gpio_unexport(unsigned gpio) { } + +static inline int gpio_block_export(struct gpio_block *block) +{ + return -ENOSYS; +} + +static inline void gpio_block_unexport(struct gpio_block *block) +{ +} #endif /* CONFIG_GPIO_SYSFS */ #endif /* _ASM_GENERIC_GPIO_H */ --- linux-2.6.orig/include/linux/gpio.h +++ linux-2.6/include/linux/gpio.h @@ -278,6 +278,19 @@ static inline void gpio_unexport(unsigne WARN_ON(1); } +static inline int gpio_block_export(struct gpio_block *block) +{ + /* GPIO block can never have been requested or set as {in,out}put */ + WARN_ON(1); + return -EINVAL; +} + +static inline void gpio_block_unexport(struct gpio_block *block) +{ + /* GPIO block can never have been exported */ + WARN_ON(1); +} + static inline int gpio_to_irq(unsigned gpio) { /* GPIO can never have been requested or set as input */
This patch adds sysfs support to the block GPIO API. Signed-off-by: Roland Stigge <stigge@antcom.de> --- Documentation/ABI/testing/sysfs-gpio | 6 drivers/gpio/gpiolib.c | 226 ++++++++++++++++++++++++++++++++++- include/asm-generic/gpio.h | 11 + include/linux/gpio.h | 13 ++ 4 files changed, 254 insertions(+), 2 deletions(-)