| Message ID | 1449250275-23435-3-git-send-email-martyn.welch@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Martyn,
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on v4.4-rc3 next-20151203]
url: https://github.com/0day-ci/linux/commits/Martyn-Welch/Device-tree-binding-documentation-for-gpio-switch/20151205-014105
coccinelle warnings: (new ones prefixed by >>)
>> drivers/misc/gpio-switch.c:98:34-40: ERROR: application of sizeof to pointer
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, Dec 04, 2015 at 05:31:14PM +0000, Martyn Welch wrote: > Select Chromebooks have gpio attached to switches used to cause the > firmware to enter alternative modes of operation and/or control other > device characteristics (such as write protection on flash devices). This > patch adds a driver that exposes a read-only interface to allow these > signals to be read from user space. > > This functionality has been generalised to provide support for any device > with device tree support which needs to identify a gpio as being used for a > specific task. > > Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk> > --- > drivers/misc/Kconfig | 11 ++++ > drivers/misc/Makefile | 1 + > drivers/misc/gpio-switch.c | 160 +++++++++++++++++++++++++++++++++++++++++++++ Why isn't this in drivers/gpio/ ? why make it a misc driver? thanks, greg k-h
On 04/12/15 18:57, Greg Kroah-Hartman wrote: > On Fri, Dec 04, 2015 at 05:31:14PM +0000, Martyn Welch wrote: >> Select Chromebooks have gpio attached to switches used to cause the >> firmware to enter alternative modes of operation and/or control other >> device characteristics (such as write protection on flash devices). This >> patch adds a driver that exposes a read-only interface to allow these >> signals to be read from user space. >> >> This functionality has been generalised to provide support for any device >> with device tree support which needs to identify a gpio as being used for a >> specific task. >> >> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk> >> --- >> drivers/misc/Kconfig | 11 ++++ >> drivers/misc/Makefile | 1 + >> drivers/misc/gpio-switch.c | 160 +++++++++++++++++++++++++++++++++++++++++++++ > > Why isn't this in drivers/gpio/ ? > > why make it a misc driver? > I thought all the drivers in /drivers/gpio were gpio drivers, rather than users of the gpio framework. Is that not the case? Happy to move it if the consensus is that that's the correct place to put it. Martyn > thanks, > > greg k-h >
On Fri, Dec 4, 2015 at 6:31 PM, Martyn Welch <martyn.welch@collabora.co.uk> wrote: > Select Chromebooks have gpio attached to switches used to cause the > firmware to enter alternative modes of operation and/or control other > device characteristics (such as write protection on flash devices). This > patch adds a driver that exposes a read-only interface to allow these > signals to be read from user space. > > This functionality has been generalised to provide support for any device > with device tree support which needs to identify a gpio as being used for a > specific task. > > Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk> If you want to do this thing, also propose a device tree binding document for "gpio-switch". But first (from Documentation/gpio/drivers-on-gpio.txt): - gpio-keys: drivers/input/keyboard/gpio_keys.c is used when your GPIO line can generate interrupts in response to a key press. Also supports debounce. - gpio-keys-polled: drivers/input/keyboard/gpio_keys_polled.c is used when your GPIO line cannot generate interrupts, so it needs to be periodically polled by a timer. - extcon-gpio: drivers/extcon/extcon-gpio.c is used when you need to read an external connector status, such as a headset line for an audio driver or an HDMI connector. It will provide a better userspace sysfs interface than GPIO. So you mean none of these apply for this case? Second: what you want to do is export a number of GPIOs with certain names to userspace. This is something very generic and should be implemented as such, not as something Chromebook-specific. Patches like that has however already been suggested, and I have NACKed them because the GPIO sysfs ABI is insane, and that is why I am refactoring the world to create a proper chardev ABI for GPIO instead. See: http://marc.info/?l=linux-gpio&m=144550276512673&w=2 So for the moment, NACK on this, please participate in creating the *right* ABI for GPIO instead of trying to shoehorn stuff into the dying sysfs ABI. Yours, Linus Walleij
On 11/12/15 09:08, Linus Walleij wrote: > On Fri, Dec 4, 2015 at 6:31 PM, Martyn Welch > <martyn.welch@collabora.co.uk> wrote: > >> Select Chromebooks have gpio attached to switches used to cause the >> firmware to enter alternative modes of operation and/or control other >> device characteristics (such as write protection on flash devices). This >> patch adds a driver that exposes a read-only interface to allow these >> signals to be read from user space. >> >> This functionality has been generalised to provide support for any device >> with device tree support which needs to identify a gpio as being used for a >> specific task. >> >> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk> > > If you want to do this thing, also propose a device tree binding document > for "gpio-switch". > > But first (from Documentation/gpio/drivers-on-gpio.txt): > > - gpio-keys: drivers/input/keyboard/gpio_keys.c is used when your GPIO line > can generate interrupts in response to a key press. Also supports debounce. > This one generates input events from gpio. I'm not looking to generate events. > - gpio-keys-polled: drivers/input/keyboard/gpio_keys_polled.c is used when your > GPIO line cannot generate interrupts, so it needs to be periodically polled > by a timer. > Ditto (but using a polled mechanism rather than interrupts) > - extcon-gpio: drivers/extcon/extcon-gpio.c is used when you need to read an > external connector status, such as a headset line for an audio driver or an > HDMI connector. It will provide a better userspace sysfs interface than GPIO. > This appears to be exclusively for monitoring insertion events, or am I missing something? > So you mean none of these apply for this case? > No, I'm looking for a mechanism to identify GPIO as connected to a specific signal, which is provided across multiple devices, but which might be implemented subtly differently on different platforms (i.e. active high/low) and on different GPIO lines. > Second: what you want to do is export a number of GPIOs with certain names > to userspace. This is something very generic and should be implemented > as such, not as something Chromebook-specific. > I'd agree that my first implementation was ChromeBook specific, but I'm fairly sure that my last attempt wasn't. I've mentioned ChromeBooks as an example of an existing use case. > Patches like that has however already been suggested, and I have NACKed > them because the GPIO sysfs ABI is insane, and that is why I am refactoring > the world to create a proper chardev ABI for GPIO instead. See: > http://marc.info/?l=linux-gpio&m=144550276512673&w=2 > I can certainly understand the rationale for the changes that you are proposing, though do worry that it does make it a bit tougher to use from scripting languages. I see that the question of how to provide functionality equivalent to the above was raised and no answer was forthcoming. Is there a plan for supporting the identification of a GPIO line serving a specific purpose? What is the status of the mentioned patch series? Martyn > So for the moment, NACK on this, please participate in creating the > *right* ABI for GPIO instead of trying to shoehorn stuff into the dying > sysfs ABI. > > Yours, > Linus Walleij >
On Wed, Dec 16, 2015 at 11:11 AM, Martyn Welch <martyn.welch@collabora.co.uk> wrote: >> Patches like that has however already been suggested, and I have NACKed >> them because the GPIO sysfs ABI is insane, and that is why I am >> refactoring >> the world to create a proper chardev ABI for GPIO instead. See: >> http://marc.info/?l=linux-gpio&m=144550276512673&w=2 > > I can certainly understand the rationale for the changes that you are > proposing, though do worry that it does make it a bit tougher to use from > scripting languages. The idea is to provide commandline tools in the kernel tools/gpio subdir to satisfy the needs of scripting. "lsgpio" today is just one example, nothing stops us from having a tool called just "gpio-sak" (GPIO swiss army knife) that will be tailored for scripting. > I see that the question of how to provide functionality > equivalent to the above was raised and no answer was forthcoming. Is there a > plan for supporting the identification of a GPIO line serving a specific > purpose? Yes by name. > What is the status of the mentioned patch series? They stubled into a few dozen architecture issues in the GPIO subsystem so I am busy refactoring the whole know universe :D But I still intend to persue the series. Yours, Linus Walleij
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 22892c7..d24367c 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -525,6 +525,17 @@ config VEXPRESS_SYSCFG bus. System Configuration interface is one of the possible means of generating transactions on this bus. +config GPIO_SWITCH + tristate "GPIO Switch driver" + depends on GPIO_SYSFS + ---help--- + Some devices have gpio attached to dedicated switches, an example of + this are chromebooks (where connection to some switches for predefined + purposes are provided on the generic development card). This driver + provides the ability to create consistently named sysfs entries to the + functionally equivalent signals across a range of devices. This + functionality currently requires a device which supports device tree. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig" diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 537d7f3..7a7e11a 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -56,3 +56,4 @@ obj-$(CONFIG_GENWQE) += genwqe/ obj-$(CONFIG_ECHO) += echo/ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o obj-$(CONFIG_CXL_BASE) += cxl/ +obj-$(CONFIG_GPIO_SWITCH) += gpio-switch.o diff --git a/drivers/misc/gpio-switch.c b/drivers/misc/gpio-switch.c new file mode 100644 index 0000000..1f381d6 --- /dev/null +++ b/drivers/misc/gpio-switch.c @@ -0,0 +1,160 @@ +/* + * Copyright (C) 2015 Collabora Ltd. + * + * based on vendor driver, + * + * Copyright (C) 2011 The Chromium OS Authors + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include <linux/bcd.h> +#include <linux/gpio.h> +#include <linux/notifier.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_gpio.h> +#include <linux/platform_device.h> +#include <linux/slab.h> + +struct gpio_switch_gpio_info { + int gpio; + const char *link; +}; + +static int dt_gpio_init(struct platform_device *pdev, struct device_node *child, + struct gpio_switch_gpio_info *gpio) +{ + int err; + enum of_gpio_flags of_flags; + unsigned long flags = GPIOF_DIR_IN | GPIOF_EXPORT; + const char *name; + + err = of_property_read_string(child, "label", &name); + if (err) + return err; + + gpio->gpio = of_get_named_gpio_flags(child, "gpios", 0, &of_flags); + if (!gpio_is_valid(gpio->gpio)) { + err = -EINVAL; + goto err_prop; + } + + if (of_flags & OF_GPIO_ACTIVE_LOW) + flags |= GPIOF_ACTIVE_LOW; + + if (!of_property_read_bool(child, "read-only")) + flags |= GPIOF_EXPORT_CHANGEABLE; + + err = gpio_request_one(gpio->gpio, flags, name); + if (err) + goto err_prop; + + err = gpio_export_link(&pdev->dev, name, gpio->gpio); + if (err) + goto err_gpio; + + gpio->link = name; + + return 0; + +err_gpio: + gpio_free(gpio->gpio); +err_prop: + of_node_put(child); + + return err; +} + +static void gpio_switch_rem(struct device *dev, + struct gpio_switch_gpio_info *gpio) +{ + sysfs_remove_link(&dev->kobj, gpio->link); + + gpio_unexport(gpio->gpio); + + gpio_free(gpio->gpio); +} + +static int gpio_switch_probe(struct platform_device *pdev) +{ + struct gpio_switch_gpio_info *gpios; + struct device_node *child; + struct device_node *np = pdev->dev.of_node; + int ret; + int i; + + i = of_get_child_count(np); + if (i < 1) + return i; + + gpios = devm_kmalloc(&pdev->dev, sizeof(gpios) * i, GFP_KERNEL); + if (!gpios) + return -ENOMEM; + + i = 0; + + for_each_child_of_node(np, child) { + ret = dt_gpio_init(pdev, child, &gpios[i]); + if (ret) { + dev_err(&pdev->dev, "Failed to init child node %d.\n", + i); + goto err; + } + + i++; + } + + platform_set_drvdata(pdev, gpios); + + return 0; + +err: + while (i > 0) { + i--; + gpio_switch_rem(&pdev->dev, &gpios[i]); + } + + return ret; +} + +static int gpio_switch_remove(struct platform_device *pdev) +{ + struct gpio_switch_gpio_info *gpios = platform_get_drvdata(pdev); + struct device_node *np = pdev->dev.of_node; + int i; + + i = of_get_child_count(np); + + while (i > 0) { + i--; + gpio_switch_rem(&pdev->dev, &gpios[i]); + } + + return 0; +} + +static const struct of_device_id gpio_switch_of_match[] = { + { .compatible = "gpio-switch" }, + { } +}; +MODULE_DEVICE_TABLE(of, gpio_switch_of_match); + +static struct platform_driver gpio_switch_driver = { + .probe = gpio_switch_probe, + .remove = gpio_switch_remove, + .driver = { + .name = "gpio_switch", + .of_match_table = gpio_switch_of_match, + }, +}; +module_platform_driver(gpio_switch_driver); + +MODULE_LICENSE("GPL");
Select Chromebooks have gpio attached to switches used to cause the firmware to enter alternative modes of operation and/or control other device characteristics (such as write protection on flash devices). This patch adds a driver that exposes a read-only interface to allow these signals to be read from user space. This functionality has been generalised to provide support for any device with device tree support which needs to identify a gpio as being used for a specific task. Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk> --- drivers/misc/Kconfig | 11 ++++ drivers/misc/Makefile | 1 + drivers/misc/gpio-switch.c | 160 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 172 insertions(+) create mode 100644 drivers/misc/gpio-switch.c