Re: [RFC v2 GPIO lines [was: GPIO User I/O]

[Rodolfo Giometti <giometti@enneenne.com>]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On 13/07/2020 23:26, Linus Walleij wrote:
> On Mon, Jul 13, 2020 at 4:20 PM Rodolfo Giometti <giometti@enneenne.com> wrote:
>> Maybe I can do something similar to hog-gpio as below, if you prefer...
>>
>> bypass0 {
>>         gpios = <&gpionb 10 GPIO_ACTIVE_LOW>;
>>         output-low;
> 
> Yes this is better, just boolean flags is not natural than strings
> for this.

OK, changed.

> However it addresses in a way an issue we have had popping
> up from time to time which is assignment of default values to
> lines before they are used overall.
> 
> I think that would be a bit of thing that should be proper to
> solve as part of this.
> 
> The discussion has often stopped short due to different
> opinions on the device tree bindings for that.

I see... however attached is a new version of my proposal patch with the
following changelog:

- GPIO line modes are now decoded as boolean properties (as for gpio-hogs).
  Allowed values are" "input", "output-low" and "output-high". If nothing is
  specified defaults to "as-is";

- At boot a more descriptive message for each line is displayed as below:

[    1.834901] line bypass0: GPIO486 added as output-low

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

[-- Attachment #2: gpio-line.patch --]
[-- Type: text/x-patch, Size: 7919 bytes --]

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 4f52c3a8ec99..f117b0b9d33e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -73,6 +73,16 @@ config GPIO_SYSFS
 	  Kernel drivers may also request that a particular GPIO be
 	  exported to userspace; this can be useful when debugging.
 
+config GPIO_LINE
+	bool "/sys/class/line/... (GPIO lines interface)"
+	depends on SYSFS
+	help
+	  Say Y here to add a sysfs interface to manage system's GPIO lines.
+
+	  Instead of the GPIO_SYSFS support, by using this support, you'll be
+	  able to use GPIOs from userspace as stated in the device-tree
+	  for well defined pourposes and by using proper names.
+
 config GPIO_GENERIC
 	depends on HAS_IOMEM # Only for IOMEM drivers
 	tristate
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c256aff66a65..033a6b836dec 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_GPIOLIB)		+= gpiolib-legacy.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib-devprop.o
 obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
 obj-$(CONFIG_GPIO_SYSFS)	+= gpiolib-sysfs.o
+obj-$(CONFIG_GPIO_LINE)		+= gpiolib-line.o
 obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
 
 # Device drivers. Generally keep list sorted alphabetically
diff --git a/drivers/gpio/gpiolib-line.c b/drivers/gpio/gpiolib-line.c
new file mode 100644
index 000000000000..640c9971d97e
--- /dev/null
+++ b/drivers/gpio/gpiolib-line.c
@@ -0,0 +1,251 @@
+/*
+ * GPIOlib - userspace I/O line interface
+ *
+ *
+ * Copyright (C) 2020   Rodolfo Giometti <giometti@enneenne.com>
+ *
+ *   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.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/idr.h>
+#include <linux/kdev_t.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+
+#define GPIO_LINE_MAX_SOURCES       128      /* should be enough... */
+
+/*
+ * Local variables
+ */
+
+static dev_t gpio_line_devt;
+static struct class *gpio_line_class;
+
+static DEFINE_MUTEX(gpio_line_idr_lock);
+static DEFINE_IDR(gpio_line_idr);
+
+struct gpio_line_device {
+	struct gpio_desc *gpiod;
+	const char *name;
+	unsigned int id;
+	struct device *dev;
+};
+
+/*
+ * sysfs methods
+ */
+
+static ssize_t state_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct gpio_line_device *gpio_line = dev_get_drvdata(dev);
+	int status, ret;
+
+	ret = sscanf(buf, "%d", &status);
+	if (ret != 1 && status != 0 && status != 1)
+		return -EINVAL;
+
+	gpiod_set_value_cansleep(gpio_line->gpiod, status);
+
+	return count;
+}
+
+static ssize_t state_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct gpio_line_device *gpio_line = dev_get_drvdata(dev);
+	int status = gpiod_get_value_cansleep(gpio_line->gpiod);
+
+	return sprintf(buf, "%d\n", status);
+}
+static DEVICE_ATTR_RW(state);
+
+/*
+ * Class attributes
+ */
+
+static struct attribute *gpio_line_attrs[] = {
+	&dev_attr_state.attr,
+	NULL,
+};
+
+static const struct attribute_group gpio_line_group = {
+	.attrs = gpio_line_attrs,
+};
+
+static const struct attribute_group *gpio_line_groups[] = {
+	&gpio_line_group,
+	NULL,
+};
+
+/*
+ * Driver stuff
+ */
+
+static struct gpio_line_device *gpio_line_create_entry(const char *name,
+				struct gpio_desc *gpiod,
+				struct device *parent)
+{
+	struct gpio_line_device *gpio_line;
+	dev_t devt;
+	int ret;
+
+	/* First allocate a new gpio_line device */
+	gpio_line = kmalloc(sizeof(struct gpio_line_device), GFP_KERNEL);
+	if (!gpio_line)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&gpio_line_idr_lock);
+	/*
+	 * Get new ID for the new gpio_line source.  After idr_alloc() calling
+	 * the new source will be freely available into the kernel.
+	 */
+	ret = idr_alloc(&gpio_line_idr, gpio_line, 0,
+			GPIO_LINE_MAX_SOURCES, GFP_KERNEL);
+	if (ret < 0) {
+		if (ret == -ENOSPC) {
+			pr_err("%s: too many GPIO lines in the system\n",
+			       name);
+			ret = -EBUSY;
+		}
+		goto error_device_create;
+	}
+	gpio_line->id = ret;
+	mutex_unlock(&gpio_line_idr_lock);
+
+	/* Create the device and init the device's data */
+	devt = MKDEV(MAJOR(gpio_line_devt), gpio_line->id);
+	gpio_line->dev = device_create(gpio_line_class, parent, devt, gpio_line,
+				   "%s", name);
+	if (IS_ERR(gpio_line->dev)) {
+		dev_err(gpio_line->dev, "unable to create device %s\n", name);
+		ret = PTR_ERR(gpio_line->dev);
+		goto error_idr_remove;
+	}
+	dev_set_drvdata(gpio_line->dev, gpio_line);
+
+	/* Init the gpio_line data */
+	gpio_line->gpiod = gpiod;
+	gpio_line->name = name;
+
+	return gpio_line;
+
+error_idr_remove:
+	mutex_lock(&gpio_line_idr_lock);
+	idr_remove(&gpio_line_idr, gpio_line->id);
+
+error_device_create:
+	mutex_unlock(&gpio_line_idr_lock);
+	kfree(gpio_line);
+
+	return ERR_PTR(ret);
+}
+
+static int gpio_line_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct fwnode_handle *child;
+	struct gpio_line_device *gpio_line;
+	int ret;
+
+	device_for_each_child_node(dev, child) {
+		struct device_node *np = to_of_node(child);
+		const char *name;
+		enum gpiod_flags flags;
+		struct gpio_desc *gpiod;
+
+		ret = fwnode_property_read_string(child, "line-name", &name);
+		if (ret && IS_ENABLED(CONFIG_OF) && np)
+			name = np->name;
+		if (!name) {
+			dev_err(dev,
+				"name property not defined or invalid!\n");
+			goto skip;
+		}
+
+		flags = GPIOD_ASIS;
+		if (of_property_read_bool(np, "input"))
+			flags = GPIOD_IN;
+		else if (of_property_read_bool(np, "output-low"))
+			flags = GPIOD_OUT_LOW;
+		else if (of_property_read_bool(np, "output-high"))
+			flags = GPIOD_OUT_HIGH;
+		gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL, child,
+							 flags, name);
+		if (IS_ERR(gpiod)) {
+			dev_err(dev, "gpios property not defined!\n");
+			goto skip;
+		}
+
+		gpio_line = gpio_line_create_entry(name, gpiod, dev);
+		if (IS_ERR(gpio_line))
+			goto skip;
+
+		/* Success, go to the next child */
+		dev_info(gpio_line->dev, "GPIO%d added as %s\n",
+			desc_to_gpio(gpiod),
+			flags == GPIOD_ASIS ? "as-is" :
+			  flags == GPIOD_OUT_HIGH ? "output-high" :
+			    flags == GPIOD_OUT_LOW ? "output-low" :
+			      flags == GPIOD_IN ? "input" : "unknow!");
+		continue;
+
+skip:		/* Error, skip the child */
+		fwnode_handle_put(child);
+		dev_err(dev, "failed to register GPIO lines interface\n");
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_gpio_gpio_line_match[] = {
+	{ .compatible = "gpio-line", },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver gpio_line_gpio_driver = {
+	.driver	 = {
+		.name   = "gpio-line",
+		.of_match_table = of_gpio_gpio_line_match,
+	},
+};
+
+builtin_platform_driver_probe(gpio_line_gpio_driver, gpio_line_gpio_probe);
+
+/*
+ * Module stuff
+ */
+
+static int __init gpiolib_line_init(void)
+{
+	/* Create the new class */
+	gpio_line_class = class_create(THIS_MODULE, "line");
+	if (!gpio_line_class) {
+		printk(KERN_ERR "gpio_line: failed to create class\n");
+		return -ENOMEM;
+	}
+	gpio_line_class->dev_groups = gpio_line_groups;
+
+	return 0;
+}
+
+postcore_initcall(gpiolib_line_init);