[PATCH 0/7] gpio: mockup: extensions for testing purposes

[PATCH 0/7] gpio: mockup: extensions for testing purposes

[Bartosz Golaszewski]

I would like to create an automated test-suite for libgpiod, but
the gpio-mockup driver is quite limited when it comes to current
user space functionality - I can't test neither line event
notifications nor finding GPIO lines by name.

This series proposes to extend the gpio framework by allowing to
inject line events from the kernel code and by providing a debugfs
interface for that to the gpio-mockup driver. We also allow the
user to request that the mockup driver name the lines.

The first patch contains only minor clean-ups. Patches 2/7-4/7
implement the in-kernel mechanism for injecting events, while patch
5/7 extends the mockup driver to provide a relevant debugfs interface.

Patch 6/7 adds a new module param allowing to request naming of the
lines.

The last patch contains some coding style tweaks.

Bartosz Golaszewski (7):
  gpiolib: clean up includes
  gpiolib: support monitoring mockup devices
  gpio: mockup: set the mockup flag in struct gpio_chip
  gpiolib: allow injecting line events
  gpio: mockup: implement injecting events over debugfs
  gpio: mockup: implement naming the GPIO lines
  gpio: mockup: readability tweaks

 drivers/gpio/gpio-dwapb.c      |   1 -
 drivers/gpio/gpio-mockup.c     | 147 +++++++++++++++++++++++++++++++++++++----
 drivers/gpio/gpio-xgene-sb.c   |   1 -
 drivers/gpio/gpiolib-acpi.c    |   2 -
 drivers/gpio/gpiolib-devprop.c |   2 -
 drivers/gpio/gpiolib-legacy.c  |   3 -
 drivers/gpio/gpiolib-of.c      |   1 -
 drivers/gpio/gpiolib-sysfs.c   |   2 -
 drivers/gpio/gpiolib.c         |  44 +++++++++---
 drivers/gpio/gpiolib.h         |   5 ++
 include/linux/gpio/driver.h    |   4 ++
 11 files changed, 175 insertions(+), 37 deletions(-)

-- 
2.9.3

[PATCH 1/7] gpiolib: clean up includes

[Bartosz Golaszewski]

gpiolib.h uses enum gpiod_flags, but doesn't pull in gpio/consumer.h.
Include it in gpiolib.h and don't include neither consumer.h nor
driver.h from modules that already include gpiolib.h.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-dwapb.c      | 1 -
 drivers/gpio/gpio-xgene-sb.c   | 1 -
 drivers/gpio/gpiolib-acpi.c    | 2 --
 drivers/gpio/gpiolib-devprop.c | 2 --
 drivers/gpio/gpiolib-legacy.c  | 3 ---
 drivers/gpio/gpiolib-of.c      | 1 -
 drivers/gpio/gpiolib-sysfs.c   | 2 --
 drivers/gpio/gpiolib.c         | 1 -
 drivers/gpio/gpiolib.h         | 1 +
 9 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 6193f62..e486633 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -8,7 +8,6 @@
  * All enquiries to support@picochip.com
  */
 #include <linux/acpi.h>
-#include <linux/gpio/driver.h>
 /* FIXME: for gpio_get_value(), replace this with direct register read */
 #include <linux/gpio.h>
 #include <linux/err.h>
diff --git a/drivers/gpio/gpio-xgene-sb.c b/drivers/gpio/gpio-xgene-sb.c
index 0332586..eb156c3 100644
--- a/drivers/gpio/gpio-xgene-sb.c
+++ b/drivers/gpio/gpio-xgene-sb.c
@@ -24,7 +24,6 @@
 #include <linux/io.h>
 #include <linux/platform_device.h>
 #include <linux/of_gpio.h>
-#include <linux/gpio/driver.h>
 #include <linux/acpi.h>
 
 #include "gpiolib.h"
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 9b37a36..aebd072 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -12,8 +12,6 @@
 
 #include <linux/errno.h>
 #include <linux/gpio.h>
-#include <linux/gpio/consumer.h>
-#include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
 #include <linux/export.h>
 #include <linux/acpi.h>
diff --git a/drivers/gpio/gpiolib-devprop.c b/drivers/gpio/gpiolib-devprop.c
index 27f383b..cf4c677 100644
--- a/drivers/gpio/gpiolib-devprop.c
+++ b/drivers/gpio/gpiolib-devprop.c
@@ -11,8 +11,6 @@
 
 #include <linux/property.h>
 #include <linux/slab.h>
-#include <linux/gpio/consumer.h>
-#include <linux/gpio/driver.h>
 
 #include "gpiolib.h"
 
diff --git a/drivers/gpio/gpiolib-legacy.c b/drivers/gpio/gpiolib-legacy.c
index 8b83099..f5e7ce8 100644
--- a/drivers/gpio/gpiolib-legacy.c
+++ b/drivers/gpio/gpiolib-legacy.c
@@ -1,6 +1,3 @@
-#include <linux/gpio/consumer.h>
-#include <linux/gpio/driver.h>
-
 #include <linux/gpio.h>
 
 #include "gpiolib.h"
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 975b9f6..064d51f 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -16,7 +16,6 @@
 #include <linux/errno.h>
 #include <linux/module.h>
 #include <linux/io.h>
-#include <linux/gpio/consumer.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_gpio.h>
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 4b44dd9..844a7cd 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -2,8 +2,6 @@
 #include <linux/mutex.h>
 #include <linux/device.h>
 #include <linux/sysfs.h>
-#include <linux/gpio/consumer.h>
-#include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/kdev_t.h>
 #include <linux/slab.h>
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 59d3d96..6722579 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -14,7 +14,6 @@
 #include <linux/idr.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
-#include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/cdev.h>
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 2495b7e..067e5e5 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -13,6 +13,7 @@
 #define GPIOLIB_H
 
 #include <linux/gpio/driver.h>
+#include <linux/gpio/consumer.h>
 #include <linux/err.h>
 #include <linux/device.h>
 #include <linux/module.h>
-- 
2.9.3

Re: [PATCH 1/7] gpiolib: clean up includes

[Linus Walleij]

On Wed, Jan 25, 2017 at 4:34 PM, Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> gpiolib.h uses enum gpiod_flags, but doesn't pull in gpio/consumer.h.
> Include it in gpiolib.h and don't include neither consumer.h nor
> driver.h from modules that already include gpiolib.h.

Nah.... .c files should include the headers representing the
symbols they use. Not rely on secondary inclusions.

Drivers should ideally *always* and *only*
include <linux/gpio/driver.h>. If they include other stuff,
that is just tautological icing on the cake.

So, I'm gonna drop this.

Yours,
Linus Walleij

[PATCH 2/7] gpiolib: support monitoring mockup devices

[Bartosz Golaszewski]

Add a new flag to struct gpio_chip indicating that the chip doesn't
model a real device. When setting up the line event queue, check if
a device is a mockup chip and don't actually request the interrupt.

This way we can monitor mockup GPIOs.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c      | 27 +++++++++++++++++----------
 include/linux/gpio/driver.h |  4 ++++
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6722579..19b4b8b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -735,6 +735,7 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
 
 static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 {
+	struct gpio_chip *chip = gdev->chip;
 	struct gpioevent_request eventreq;
 	struct lineevent_state *le;
 	struct gpio_desc *desc;
@@ -807,7 +808,8 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 		goto out_free_desc;
 
 	le->irq = gpiod_to_irq(desc);
-	if (le->irq <= 0) {
+	/* Mockup gpiochips don't have to support this. */
+	if (le->irq <= 0 && !chip->mockup) {
 		ret = -ENODEV;
 		goto out_free_desc;
 	}
@@ -823,15 +825,20 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	init_waitqueue_head(&le->wait);
 	mutex_init(&le->read_lock);
 
-	/* Request a thread to read the events */
-	ret = request_threaded_irq(le->irq,
-			NULL,
-			lineevent_irq_thread,
-			irqflags,
-			le->label,
-			le);
-	if (ret)
-		goto out_free_desc;
+	if (!chip->mockup) {
+		/*
+		 * Request a thread to read the events unless we're dealing
+		 * with a mockup gpiochip.
+		 */
+		ret = request_threaded_irq(le->irq,
+				NULL,
+				lineevent_irq_thread,
+				irqflags,
+				le->label,
+				le);
+		if (ret)
+			goto out_free_desc;
+	}
 
 	fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
 	if (fd < 0) {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index e973fab..912eae1 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -82,6 +82,9 @@ enum single_ended_mode {
  *	implies that if the chip supports IRQs, these IRQs need to be threaded
  *	as the chip access may sleep when e.g. reading out the IRQ status
  *	registers.
+ * @mockup: if set, the flag signifies that this gpiochip does not model a
+ *      real device; this can affect the way the chip is handled internally
+ *      by gpiolib.
  * @read_reg: reader function for generic GPIO
  * @write_reg: writer function for generic GPIO
  * @pin2mask: some generic GPIO controllers work with the big-endian bits
@@ -166,6 +169,7 @@ struct gpio_chip {
 	u16			ngpio;
 	const char		*const *names;
 	bool			can_sleep;
+	bool			mockup;
 
 #if IS_ENABLED(CONFIG_GPIO_GENERIC)
 	unsigned long (*read_reg)(void __iomem *reg);
-- 
2.9.3

Re: [PATCH 2/7] gpiolib: support monitoring mockup devices

[Linus Walleij]

On Wed, Jan 25, 2017 at 4:34 PM, Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> Add a new flag to struct gpio_chip indicating that the chip doesn't
> model a real device. When setting up the line event queue, check if
> a device is a mockup chip and don't actually request the interrupt.
>
> This way we can monitor mockup GPIOs.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Hm. I'm not so happy about adding code that gets compiled into every
system on the planet but is only used for testing with the mockup driver.

Atleast it should be #ifdef:ed (one of the few cases when we should
do that).

But I would prefer if there was some more elegant way. Like getting
a mockup irqchip! But I don't know how hard that is indeed.

Yours,
Linus Walleij

[PATCH 3/7] gpio: mockup: set the mockup flag in struct gpio_chip

[Bartosz Golaszewski]

gpio-mockup creates dummy devices, so indicate it.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-mockup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 82a9efd..cf8b1b2 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -101,6 +101,7 @@ static int mockup_gpio_add(struct device *dev,
 	cntr->gc.direction_output = mockup_gpio_dirout;
 	cntr->gc.direction_input = mockup_gpio_dirin;
 	cntr->gc.get_direction = mockup_gpio_get_direction;
+	cntr->gc.mockup = true;
 	cntr->stats = devm_kzalloc(dev, sizeof(*cntr->stats) * cntr->gc.ngpio,
 				   GFP_KERNEL);
 	if (!cntr->stats) {
-- 
2.9.3

[PATCH 4/7] gpiolib: allow injecting line events

[Bartosz Golaszewski]

Implement a routine visible inside the drivers/gpio directory, that
allows to inject line events for testing/debugging purposes.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 16 ++++++++++++++++
 drivers/gpio/gpiolib.h |  4 ++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 19b4b8b..8dde5f1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -643,6 +643,7 @@ static int lineevent_release(struct inode *inode, struct file *filep)
 	struct gpio_device *gdev = le->gdev;
 
 	free_irq(le->irq, le);
+	le->desc->le = NULL;
 	gpiod_free(le->desc);
 	kfree(le->label);
 	kfree(le);
@@ -733,6 +734,20 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+void gpiod_inject_event(struct gpio_desc *desc)
+{
+	struct lineevent_state *le = desc->le;
+	unsigned long flags;
+
+	if (le) {
+		/* Act as if we were in interrupt context. */
+		local_irq_save(flags);
+		lineevent_irq_thread(-1 /* unused */, le);
+		local_irq_restore(flags);
+	}
+}
+EXPORT_SYMBOL(gpiod_inject_event);
+
 static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 {
 	struct gpio_chip *chip = gdev->chip;
@@ -794,6 +809,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	if (ret)
 		goto out_free_desc;
 	le->desc = desc;
+	desc->le = le;
 	le->eflags = eflags;
 
 	if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 067e5e5..3c87222 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -23,6 +23,7 @@ enum of_gpio_flags;
 enum gpiod_flags;
 enum gpio_lookup_flags;
 struct acpi_device;
+struct lineevent_state;
 
 /**
  * struct gpio_device - internal state container for GPIO devices
@@ -196,12 +197,15 @@ struct gpio_desc {
 	const char		*label;
 	/* Name of the GPIO */
 	const char		*name;
+	/* Private: line event context for the GPIO */
+	struct lineevent_state  *le;
 };
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 		unsigned long lflags, enum gpiod_flags dflags);
+void gpiod_inject_event(struct gpio_desc *desc);
 
 /*
  * Return the GPIO number of the passed descriptor relative to its chip
-- 
2.9.3

Re: [PATCH 4/7] gpiolib: allow injecting line events

[Linus Walleij]

On Wed, Jan 25, 2017 at 4:34 PM, Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> Implement a routine visible inside the drivers/gpio directory, that
> allows to inject line events for testing/debugging purposes.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Hm OK it is clever, but would also need #ifdef.

Also this kludge would go away if we had a real mockup
irqchip...

Yours,
Linus Walleij

[PATCH 5/7] gpio: mockup: implement injecting events over debugfs

[Bartosz Golaszewski]

When probing gpio-mockup, create a debugfs directory structure that
allows the user to inject line events by writing either 0
(falling-edge) or 1 (rising-edge) to debugfs files.

The gpio-mockup-event directory has one sub-directory per chip and
a single event file per line.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-mockup.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index cf8b1b2..c5eb530 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -15,6 +15,10 @@
 #include <linux/module.h>
 #include <linux/gpio/driver.h>
 #include <linux/platform_device.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
+
+#include "gpiolib.h"
 
 #define GPIO_NAME	"gpio-mockup"
 #define	MAX_GC		10
@@ -37,6 +41,7 @@ struct gpio_pin_status {
 struct mockup_gpio_controller {
 	struct gpio_chip gc;
 	struct gpio_pin_status *stats;
+	struct dentry *dbg_dir;
 };
 
 static int gpio_mockup_ranges[MAX_GC << 1];
@@ -44,6 +49,7 @@ static int gpio_mockup_params_nr;
 module_param_array(gpio_mockup_ranges, int, &gpio_mockup_params_nr, 0400);
 
 static const char pins_name_start = 'A';
+static struct dentry *dbg_dir;
 
 static int mockup_gpio_get(struct gpio_chip *gc, unsigned int offset)
 {
@@ -85,6 +91,80 @@ static int mockup_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
 	return cntr->stats[offset].dir;
 }
 
+static ssize_t mockup_gpio_event_write(struct file *file,
+				       const char __user *usr_buf,
+				       size_t size, loff_t *ppos)
+{
+	struct gpio_desc *desc;
+	struct seq_file *sfile;
+	int status, val;
+	char buf;
+
+	sfile = file->private_data;
+	desc = sfile->private;
+
+	status = copy_from_user(&buf, usr_buf, 1);
+	if (status)
+		return status;
+
+	if (buf == '0')
+		val = 0;
+	else if (buf == '1')
+		val = 1;
+	else
+		return -EINVAL;
+
+	gpiod_set_value_cansleep(desc, val);
+	gpiod_inject_event(desc);
+
+	return size;
+}
+
+static int mockup_gpio_event_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, NULL, inode->i_private);
+}
+
+static const struct file_operations mockup_gpio_event_ops = {
+	.owner = THIS_MODULE,
+	.open = mockup_gpio_event_open,
+	.write = mockup_gpio_event_write,
+	.llseek = no_llseek,
+};
+
+static void mockup_gpio_debugfs_setup(struct mockup_gpio_controller *cntr)
+{
+	struct dentry *evfile;
+	struct gpio_chip *gc;
+	struct device *dev;
+	char *name;
+	int i;
+
+	gc = &cntr->gc;
+	dev = &gc->gpiodev->dev;
+
+	cntr->dbg_dir = debugfs_create_dir(gc->label, dbg_dir);
+	if (!cntr->dbg_dir)
+		goto err;
+
+	for (i = 0; i < gc->ngpio; i++) {
+		name = devm_kasprintf(dev, GFP_KERNEL, "%d", i);
+		if (!name)
+			goto err;
+
+		evfile = debugfs_create_file(name, 0200, cntr->dbg_dir,
+					     &gc->gpiodev->descs[i],
+					     &mockup_gpio_event_ops);
+		if (!evfile)
+			goto err;
+	}
+
+	return;
+
+err:
+	dev_err(dev, "error creating debugfs directory\n");
+}
+
 static int mockup_gpio_add(struct device *dev,
 			   struct mockup_gpio_controller *cntr,
 			   const char *name, int base, int ngpio)
@@ -112,6 +192,9 @@ static int mockup_gpio_add(struct device *dev,
 	if (ret)
 		goto err;
 
+	if (dbg_dir)
+		mockup_gpio_debugfs_setup(cntr);
+
 	dev_info(dev, "gpio<%d..%d> add successful!", base, base + ngpio);
 	return 0;
 err:
@@ -182,6 +265,10 @@ static int __init mock_device_init(void)
 {
 	int err;
 
+	dbg_dir = debugfs_create_dir("gpio-mockup-event", NULL);
+	if (!dbg_dir)
+		pr_err("%s: error creating debugfs directory\n", GPIO_NAME);
+
 	pdev = platform_device_alloc(GPIO_NAME, -1);
 	if (!pdev)
 		return -ENOMEM;
@@ -203,6 +290,7 @@ static int __init mock_device_init(void)
 
 static void __exit mock_device_exit(void)
 {
+	debugfs_remove_recursive(dbg_dir);
 	platform_driver_unregister(&mockup_gpio_driver);
 	platform_device_unregister(pdev);
 }
-- 
2.9.3

[PATCH 6/7] gpio: mockup: implement naming the GPIO lines

[Bartosz Golaszewski]

Implement a new boolean module argument that indicates that the mockup
lines should be named. The names are created by appending the line
offset to the chip label.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-mockup.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index c5eb530..dabf3ec 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -17,6 +17,7 @@
 #include <linux/platform_device.h>
 #include <linux/debugfs.h>
 #include <linux/uaccess.h>
+#include <linux/slab.h>
 
 #include "gpiolib.h"
 
@@ -48,6 +49,10 @@ static int gpio_mockup_ranges[MAX_GC << 1];
 static int gpio_mockup_params_nr;
 module_param_array(gpio_mockup_ranges, int, &gpio_mockup_params_nr, 0400);
 
+static bool gpio_mockup_named_lines;
+module_param_named(gpio_mockup_named_lines,
+		   gpio_mockup_named_lines, bool, 0400);
+
 static const char pins_name_start = 'A';
 static struct dentry *dbg_dir;
 
@@ -169,7 +174,8 @@ static int mockup_gpio_add(struct device *dev,
 			   struct mockup_gpio_controller *cntr,
 			   const char *name, int base, int ngpio)
 {
-	int ret;
+	char **names;
+	int ret, i;
 
 	cntr->gc.base = base;
 	cntr->gc.ngpio = ngpio;
@@ -188,6 +194,28 @@ static int mockup_gpio_add(struct device *dev,
 		ret = -ENOMEM;
 		goto err;
 	}
+
+	if (gpio_mockup_named_lines) {
+		names = devm_kzalloc(dev,
+				     sizeof(char *) * cntr->gc.ngpio,
+				     GFP_KERNEL);
+		if (!names) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		for (i = 0; i < cntr->gc.ngpio; i++) {
+			names[i] = devm_kasprintf(dev, GFP_KERNEL,
+						  "%s-%d", cntr->gc.label, i);
+			if (!names[i]) {
+				ret = -ENOMEM;
+				goto err;
+			}
+		}
+
+		cntr->gc.names = (const char *const*)names;
+	}
+
 	ret = devm_gpiochip_add_data(dev, &cntr->gc, cntr);
 	if (ret)
 		goto err;
-- 
2.9.3

[PATCH 7/7] gpio: mockup: readability tweaks

[Bartosz Golaszewski]

Add some newlines and use a temporary pointer in mockup_gpio_add().

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-mockup.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index dabf3ec..4289bbd 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -78,6 +78,7 @@ static int mockup_gpio_dirout(struct gpio_chip *gc, unsigned int offset,
 
 	mockup_gpio_set(gc, offset, value);
 	cntr->stats[offset].dir = OUT;
+
 	return 0;
 }
 
@@ -86,6 +87,7 @@ static int mockup_gpio_dirin(struct gpio_chip *gc, unsigned int offset)
 	struct mockup_gpio_controller *cntr = gpiochip_get_data(gc);
 
 	cntr->stats[offset].dir = IN;
+
 	return 0;
 }
 
@@ -174,21 +176,22 @@ static int mockup_gpio_add(struct device *dev,
 			   struct mockup_gpio_controller *cntr,
 			   const char *name, int base, int ngpio)
 {
+	struct gpio_chip *gc = &cntr->gc;
 	char **names;
 	int ret, i;
 
-	cntr->gc.base = base;
-	cntr->gc.ngpio = ngpio;
-	cntr->gc.label = name;
-	cntr->gc.owner = THIS_MODULE;
-	cntr->gc.parent = dev;
-	cntr->gc.get = mockup_gpio_get;
-	cntr->gc.set = mockup_gpio_set;
-	cntr->gc.direction_output = mockup_gpio_dirout;
-	cntr->gc.direction_input = mockup_gpio_dirin;
-	cntr->gc.get_direction = mockup_gpio_get_direction;
-	cntr->gc.mockup = true;
-	cntr->stats = devm_kzalloc(dev, sizeof(*cntr->stats) * cntr->gc.ngpio,
+	gc->base = base;
+	gc->ngpio = ngpio;
+	gc->label = name;
+	gc->owner = THIS_MODULE;
+	gc->parent = dev;
+	gc->get = mockup_gpio_get;
+	gc->set = mockup_gpio_set;
+	gc->direction_output = mockup_gpio_dirout;
+	gc->direction_input = mockup_gpio_dirin;
+	gc->get_direction = mockup_gpio_get_direction;
+	gc->mockup = true;
+	cntr->stats = devm_kzalloc(dev, sizeof(*cntr->stats) * gc->ngpio,
 				   GFP_KERNEL);
 	if (!cntr->stats) {
 		ret = -ENOMEM;
@@ -197,26 +200,25 @@ static int mockup_gpio_add(struct device *dev,
 
 	if (gpio_mockup_named_lines) {
 		names = devm_kzalloc(dev,
-				     sizeof(char *) * cntr->gc.ngpio,
-				     GFP_KERNEL);
+				     sizeof(char *) * gc->ngpio, GFP_KERNEL);
 		if (!names) {
 			ret = -ENOMEM;
 			goto err;
 		}
 
-		for (i = 0; i < cntr->gc.ngpio; i++) {
+		for (i = 0; i < gc->ngpio; i++) {
 			names[i] = devm_kasprintf(dev, GFP_KERNEL,
-						  "%s-%d", cntr->gc.label, i);
+						  "%s-%d", gc->label, i);
 			if (!names[i]) {
 				ret = -ENOMEM;
 				goto err;
 			}
 		}
 
-		cntr->gc.names = (const char *const*)names;
+		gc->names = (const char *const*)names;
 	}
 
-	ret = devm_gpiochip_add_data(dev, &cntr->gc, cntr);
+	ret = devm_gpiochip_add_data(dev, gc, cntr);
 	if (ret)
 		goto err;
 
-- 
2.9.3

Re: [PATCH 0/7] gpio: mockup: extensions for testing purposes

[Linus Walleij]

On Wed, Jan 25, 2017 at 4:34 PM, Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> This series proposes to extend the gpio framework by allowing to
> inject line events from the kernel code and by providing a debugfs
> interface for that to the gpio-mockup driver. We also allow the
> user to request that the mockup driver name the lines.

I sympathize fully with the goal and intentions of the series, I
agree: this is awesome to have for testing and validation of
GPIO.

I'm reluctant about the changes to gpiolib and want to make that
code as optional as possible, definately #ifdef if nothing else
works. Otherwise the memory footprint people will get me for this,
haha. ;)

The absolutely best would be if the driver could inject "real"
irqs and also exercise the gpiolib irqchip helpers. I have been
vaguely thinking that sofware interrupts should be able to do this
but I'm not very versed in that kind of stuff.

The changes to gpio-mockup.c are entirely uncontroversial, it
is for testing so I'm willing to accept almost anything if it looks
maintainable and helps in testing.

Yours,
Linus Walleij

Re: [PATCH 0/7] gpio: mockup: extensions for testing purposes

[Bartosz Golaszewski]

2017-01-31 14:28 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
> On Wed, Jan 25, 2017 at 4:34 PM, Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
>
>> This series proposes to extend the gpio framework by allowing to
>> inject line events from the kernel code and by providing a debugfs
>> interface for that to the gpio-mockup driver. We also allow the
>> user to request that the mockup driver name the lines.
>
> I sympathize fully with the goal and intentions of the series, I
> agree: this is awesome to have for testing and validation of
> GPIO.
>
> I'm reluctant about the changes to gpiolib and want to make that
> code as optional as possible, definately #ifdef if nothing else
> works. Otherwise the memory footprint people will get me for this,
> haha. ;)
>
> The absolutely best would be if the driver could inject "real"
> irqs and also exercise the gpiolib irqchip helpers. I have been
> vaguely thinking that sofware interrupts should be able to do this
> but I'm not very versed in that kind of stuff.
>

This was my initial idea, but I thought it's not very likely that
Thomas Gleixner would allow me to allocate a new software interrupt
just for the sake of testing gpiolib. Also: the handling of softirqs
seems to be a bit different than regular IRQs, but I'm too not an
expert.

> The changes to gpio-mockup.c are entirely uncontroversial, it
> is for testing so I'm willing to accept almost anything if it looks
> maintainable and helps in testing.
>

How about creating a new config option GPIOLIB_LINE_EVENT_DEBUG that
would be selected by GPIO_MOCKUP and enclosing the relevant gpiolib
code with ifdefs (+ potentially comments explaining what's being done
and why)?

Thanks,
Bartosz

Re: [PATCH 0/7] gpio: mockup: extensions for testing purposes

[Lars-Peter Clausen]

On 01/31/2017 03:05 PM, Bartosz Golaszewski wrote:
> 2017-01-31 14:28 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
>> On Wed, Jan 25, 2017 at 4:34 PM, Bartosz Golaszewski
>> <bgolaszewski@baylibre.com> wrote:
>>
>>> This series proposes to extend the gpio framework by allowing to
>>> inject line events from the kernel code and by providing a debugfs
>>> interface for that to the gpio-mockup driver. We also allow the
>>> user to request that the mockup driver name the lines.
>>
>> I sympathize fully with the goal and intentions of the series, I
>> agree: this is awesome to have for testing and validation of
>> GPIO.
>>
>> I'm reluctant about the changes to gpiolib and want to make that
>> code as optional as possible, definately #ifdef if nothing else
>> works. Otherwise the memory footprint people will get me for this,
>> haha. ;)
>>
>> The absolutely best would be if the driver could inject "real"
>> irqs and also exercise the gpiolib irqchip helpers. I have been
>> vaguely thinking that sofware interrupts should be able to do this
>> but I'm not very versed in that kind of stuff.
>>
> 
> This was my initial idea, but I thought it's not very likely that
> Thomas Gleixner would allow me to allocate a new software interrupt
> just for the sake of testing gpiolib. Also: the handling of softirqs
> seems to be a bit different than regular IRQs, but I'm too not an
> expert.

FWIW, we also need this in IIO. We currently inject our software IRQs for
testing using irq_work and handle_simple_irq()[1]. This has the advantage
that it goes the normal route through the IRQ subsystem and is even running
in IRQ context rather than application context (which is what happens if you
emulate the IRQ directly from the sysfs/debugfs callbacks).

- Lars

[1] http://lxr.free-electrons.com/source/drivers/iio/dummy/iio_dummy_evgen.c#L84

Re: [PATCH 0/7] gpio: mockup: extensions for testing purposes

[Bartosz Golaszewski]

2017-01-31 15:11 GMT+01:00 Lars-Peter Clausen <lars@metafoo.de>:
> On 01/31/2017 03:05 PM, Bartosz Golaszewski wrote:
>> 2017-01-31 14:28 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
>>> On Wed, Jan 25, 2017 at 4:34 PM, Bartosz Golaszewski
>>> <bgolaszewski@baylibre.com> wrote:
>>>
>>>> This series proposes to extend the gpio framework by allowing to
>>>> inject line events from the kernel code and by providing a debugfs
>>>> interface for that to the gpio-mockup driver. We also allow the
>>>> user to request that the mockup driver name the lines.
>>>
>>> I sympathize fully with the goal and intentions of the series, I
>>> agree: this is awesome to have for testing and validation of
>>> GPIO.
>>>
>>> I'm reluctant about the changes to gpiolib and want to make that
>>> code as optional as possible, definately #ifdef if nothing else
>>> works. Otherwise the memory footprint people will get me for this,
>>> haha. ;)
>>>
>>> The absolutely best would be if the driver could inject "real"
>>> irqs and also exercise the gpiolib irqchip helpers. I have been
>>> vaguely thinking that sofware interrupts should be able to do this
>>> but I'm not very versed in that kind of stuff.
>>>
>>
>> This was my initial idea, but I thought it's not very likely that
>> Thomas Gleixner would allow me to allocate a new software interrupt
>> just for the sake of testing gpiolib. Also: the handling of softirqs
>> seems to be a bit different than regular IRQs, but I'm too not an
>> expert.
>
> FWIW, we also need this in IIO. We currently inject our software IRQs for
> testing using irq_work and handle_simple_irq()[1]. This has the advantage
> that it goes the normal route through the IRQ subsystem and is even running
> in IRQ context rather than application context (which is what happens if you
> emulate the IRQ directly from the sysfs/debugfs callbacks).
>
> - Lars
>
> [1] http://lxr.free-electrons.com/source/drivers/iio/dummy/iio_dummy_evgen.c#L84

Nice! I didn't know about that. I think this is what we need in gpiolib.

Thanks,
Bartosz