diff mbox

[2/3] Add support for monitoring gpio switches

Message ID 1449250275-23435-3-git-send-email-martyn.welch@collabora.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Martyn Welch Dec. 4, 2015, 5:31 p.m. UTC
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

Comments

kernel test robot Dec. 4, 2015, 6:14 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH Dec. 4, 2015, 6:57 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martyn Welch Dec. 5, 2015, 10:42 a.m. UTC | #3
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Dec. 11, 2015, 9:08 a.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martyn Welch Dec. 16, 2015, 10:11 a.m. UTC | #5
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Dec. 22, 2015, 9:25 a.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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");