[PATCH 00/23] gpio: mockup: support dynamically created and removed chips

[PATCH 00/23] gpio: mockup: support dynamically created and removed chips

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We're about to merge w V2 user API for GPIO. In user-space we're using the
gpio-mockup driver for testing but it's quite cumbersome (needs unloading
and reloading to change chip configuration) and not very extensible (config
is passed over module params).

This series proposes to extend the debugfs interface to support dynamic
creation and removal of dummy chips, with extensible options.

First 3 patches add some lib functionality we'll use later on. Next 3 contain
general gpiolib refactoring and can be picked up independently.

Next we refactor gpio-mockup and finally add the delete_device and new_device
attributes. Last patch adds documentation for gpio-mockup so I'm not going into
detail on how the new interface works - the doc describes it pretty well.

Bartosz Golaszewski (23):
  lib: cmdline: export next_arg()
  lib: string_helpers: provide kfree_strarray()
  lib: uaccess: provide getline_from_user()
  gpiolib: generalize devprop_gpiochip_set_names() for device properties
  gpiolib: unexport devprop_gpiochip_set_names()
  gpiolib: switch to simpler IDA interface
  gpio: mockup: drop unneeded includes
  gpio: mockup: use pr_fmt()
  gpio: mockup: use KBUILD_MODNAME
  gpio: mockup: fix resource leak in error path
  gpio: mockup: remove the limit on number of dummy chips
  gpio: mockup: define a constant for chip label size
  gpio: mockup: pass the chip label as device property
  gpio: mockup: use the generic 'gpio-line-names' property
  gpio: mockup: use dynamic device IDs
  gpio: mockup: refactor the module init function
  gpio: mockup: rename and move around debugfs callbacks
  gpio: mockup: require debugfs to build
  gpio: mockup: add a symlink for the per-chip debugfs directory
  gpio: mockup: add a lock for dummy device list
  gpio: mockup: provide a way to delete dummy chips
  gpio: mockup: provide a way to create new dummy chips
  Documentation: gpio: add documentation for gpio-mockup

 .../admin-guide/gpio/gpio-mockup.rst          |  87 +++
 drivers/gpio/Kconfig                          |   1 +
 drivers/gpio/Makefile                         |   1 -
 drivers/gpio/gpio-mockup.c                    | 614 ++++++++++++++----
 drivers/gpio/gpiolib-acpi.c                   |   3 -
 drivers/gpio/gpiolib-devprop.c                |  63 --
 drivers/gpio/gpiolib-of.c                     |   5 -
 drivers/gpio/gpiolib.c                        |  62 +-
 include/linux/gpio/driver.h                   |   3 -
 include/linux/string_helpers.h                |   2 +
 include/linux/uaccess.h                       |   3 +
 lib/cmdline.c                                 |   1 +
 lib/string_helpers.c                          |  22 +
 lib/usercopy.c                                |  37 ++
 14 files changed, 705 insertions(+), 199 deletions(-)
 create mode 100644 Documentation/admin-guide/gpio/gpio-mockup.rst
 delete mode 100644 drivers/gpio/gpiolib-devprop.c

-- 
2.26.1

[PATCH 01/23] lib: cmdline: export next_arg()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We want to use this helper in a module (gpio-mockup) for parsing user
input when creating dummy devices. Let's export it.

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

diff --git a/lib/cmdline.c b/lib/cmdline.c
index fbb9981a04a4..e194f76d546e 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -247,3 +247,4 @@ char *next_arg(char *args, char **param, char **val)
 	/* Chew up trailing spaces. */
 	return skip_spaces(next);
 }
+EXPORT_SYMBOL(next_arg);
-- 
2.26.1

[PATCH 02/23] lib: string_helpers: provide kfree_strarray()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

There's a common pattern of dynamically allocating an array of char
pointers and then also dynamically allocating each string in this
array. Provide a helper for freeing such a string array with one call.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/string_helpers.h |  2 ++
 lib/string_helpers.c           | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 86f150c2a6b6..55b25120a1c6 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -94,4 +94,6 @@ char *kstrdup_quotable(const char *src, gfp_t gfp);
 char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp);
 char *kstrdup_quotable_file(struct file *file, gfp_t gfp);
 
+void kfree_strarray(char **str_array, size_t num_str);
+
 #endif
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 963050c0283e..22505efc6aae 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -649,3 +649,25 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp)
 	return pathname;
 }
 EXPORT_SYMBOL_GPL(kstrdup_quotable_file);
+
+/**
+ * kfree_strarray - free a number of dynamically allocated strings contained
+ *                  in an array and the array itself
+ *
+ * @str_array: Dynamically allocated array of strings to free. If NULL - the
+ *             function does nothing.
+ * @num_str: Number of strings (starting from the beginning of the array) to
+ *           free.
+ */
+void kfree_strarray(char **str_array, size_t num_str)
+{
+	unsigned int i;
+
+	if (!str_array)
+		return;
+
+	for (i = 0; i < num_str; i++)
+		kfree(str_array[i]);
+	kfree(str_array);
+}
+EXPORT_SYMBOL_GPL(kfree_strarray);
-- 
2.26.1

[PATCH 03/23] lib: uaccess: provide getline_from_user()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Provide a uaccess helper that allows callers to copy a single line from
user memory. This is useful for debugfs write callbacks.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/uaccess.h |  3 +++
 lib/usercopy.c          | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 94b285411659..5aedd8ac5c31 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -333,6 +333,9 @@ long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
 		long count);
 long strnlen_user_nofault(const void __user *unsafe_addr, long count);
 
+ssize_t getline_from_user(char *dst, size_t dst_size,
+			  const char __user *src, size_t src_size);
+
 /**
  * get_kernel_nofault(): safely attempt to read from a location
  * @val: read into this variable
diff --git a/lib/usercopy.c b/lib/usercopy.c
index b26509f112f9..55aaaf93d847 100644
--- a/lib/usercopy.c
+++ b/lib/usercopy.c
@@ -87,3 +87,40 @@ int check_zeroed_user(const void __user *from, size_t size)
 	return -EFAULT;
 }
 EXPORT_SYMBOL(check_zeroed_user);
+
+/**
+ * getline_from_user - Copy a single line from user
+ * @dst: Where to copy the line to
+ * @dst_size: Size of the destination buffer
+ * @src: Where to copy the line from
+ * @src_size: Size of the source user buffer
+ *
+ * Copies a number of characters from given user buffer into the dst buffer.
+ * The number of bytes is limited to the lesser of the sizes of both buffers.
+ * If the copied string contains a newline, its first occurrence is replaced
+ * by a NULL byte in the destination buffer. Otherwise the function ensures
+ * the copied string is NULL-terminated.
+ *
+ * Returns the number of copied bytes or a negative error number on failure.
+ */
+
+ssize_t getline_from_user(char *dst, size_t dst_size,
+			  const char __user *src, size_t src_size)
+{
+	size_t size = min_t(size_t, dst_size, src_size);
+	char *c;
+	int ret;
+
+	ret = copy_from_user(dst, src, size);
+	if (ret)
+		return -EFAULT;
+
+	dst[size - 1] = '\0';
+
+	c = strchrnul(dst, '\n');
+	if (*c)
+		*c = '\0';
+
+	return c - dst;
+}
+EXPORT_SYMBOL(getline_from_user);
-- 
2.26.1

[PATCH 04/23] gpiolib: generalize devprop_gpiochip_set_names() for device properties

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

devprop_gpiochip_set_names() is overly complicated with taking the
fwnode argument (which requires using dev_fwnode() & of_fwnode_handle()
in ACPI and OF GPIO code respectively). Let's just switch to using the
generic device properties.

This allows us to pull the code setting line names directly into
gpiochip_add_data_with_key() instead of handling it separately for
ACPI and OF.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib-acpi.c    |  3 ---
 drivers/gpio/gpiolib-devprop.c | 19 ++++++++++---------
 drivers/gpio/gpiolib-of.c      |  5 -----
 drivers/gpio/gpiolib.c         |  8 ++++----
 include/linux/gpio/driver.h    |  3 +--
 5 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 54ca3c18b291..834a12f3219e 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1221,9 +1221,6 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
 		return;
 	}
 
-	if (!chip->names)
-		devprop_gpiochip_set_names(chip, dev_fwnode(chip->parent));
-
 	acpi_gpiochip_request_regions(acpi_gpio);
 	acpi_gpiochip_scan_gpios(acpi_gpio);
 	acpi_walk_dep_device_list(handle);
diff --git a/drivers/gpio/gpiolib-devprop.c b/drivers/gpio/gpiolib-devprop.c
index 26741032fa9e..a28659b4f9c9 100644
--- a/drivers/gpio/gpiolib-devprop.c
+++ b/drivers/gpio/gpiolib-devprop.c
@@ -17,25 +17,24 @@
 /**
  * devprop_gpiochip_set_names - Set GPIO line names using device properties
  * @chip: GPIO chip whose lines should be named, if possible
- * @fwnode: Property Node containing the gpio-line-names property
  *
  * Looks for device property "gpio-line-names" and if it exists assigns
  * GPIO line names for the chip. The memory allocated for the assigned
- * names belong to the underlying firmware node and should not be released
+ * names belong to the underlying software node and should not be released
  * by the caller.
  */
-void devprop_gpiochip_set_names(struct gpio_chip *chip,
-				const struct fwnode_handle *fwnode)
+int devprop_gpiochip_set_names(struct gpio_chip *chip)
 {
 	struct gpio_device *gdev = chip->gpiodev;
+	struct device *dev = chip->parent;
 	const char **names;
 	int ret, i;
 	int count;
 
-	count = fwnode_property_read_string_array(fwnode, "gpio-line-names",
+	count = device_property_read_string_array(dev, "gpio-line-names",
 						  NULL, 0);
 	if (count < 0)
-		return;
+		return 0;
 
 	if (count > gdev->ngpio) {
 		dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d",
@@ -45,19 +44,21 @@ void devprop_gpiochip_set_names(struct gpio_chip *chip,
 
 	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
 	if (!names)
-		return;
+		return -ENOMEM;
 
-	ret = fwnode_property_read_string_array(fwnode, "gpio-line-names",
+	ret = device_property_read_string_array(dev, "gpio-line-names",
 						names, count);
 	if (ret < 0) {
 		dev_warn(&gdev->dev, "failed to read GPIO line names\n");
 		kfree(names);
-		return;
+		return ret;
 	}
 
 	for (i = 0; i < count; i++)
 		gdev->descs[i].name = names[i];
 
 	kfree(names);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(devprop_gpiochip_set_names);
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index bd31dd3b6a75..2f895a2b8411 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -1026,11 +1026,6 @@ int of_gpiochip_add(struct gpio_chip *chip)
 	if (ret)
 		return ret;
 
-	/* If the chip defines names itself, these take precedence */
-	if (!chip->names)
-		devprop_gpiochip_set_names(chip,
-					   of_fwnode_handle(chip->of_node));
-
 	of_node_get(chip->of_node);
 
 	ret = of_gpiochip_scan_gpios(chip);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 80137c1b3cdc..0d390f0ec32c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -340,9 +340,6 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
 	struct gpio_device *gdev = gc->gpiodev;
 	int i;
 
-	if (!gc->names)
-		return 0;
-
 	/* First check all names if they are unique */
 	for (i = 0; i != gc->ngpio; ++i) {
 		struct gpio_desc *gpio;
@@ -621,7 +618,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	INIT_LIST_HEAD(&gdev->pin_ranges);
 #endif
 
-	ret = gpiochip_set_desc_names(gc);
+	if (gc->names)
+		ret = gpiochip_set_desc_names(gc);
+	else
+		ret = devprop_gpiochip_set_names(gc);
 	if (ret)
 		goto err_remove_from_list;
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index d1cef5c2715c..56485a040b82 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -756,8 +756,7 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 					    enum gpiod_flags dflags);
 void gpiochip_free_own_desc(struct gpio_desc *desc);
 
-void devprop_gpiochip_set_names(struct gpio_chip *gc,
-				const struct fwnode_handle *fwnode);
+int devprop_gpiochip_set_names(struct gpio_chip *gc);
 
 #ifdef CONFIG_GPIOLIB
 
-- 
2.26.1

[PATCH 05/23] gpiolib: unexport devprop_gpiochip_set_names()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Now that devprop_gpiochip_set_names() is only used in a single place
inside drivers/gpio/gpiolib.c, there's no need anymore for it to be
exported or to even live in its own source file. Pull this function into
the core source file for gpiolib.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/Makefile          |  1 -
 drivers/gpio/gpiolib-devprop.c | 64 ----------------------------------
 drivers/gpio/gpiolib.c         | 48 +++++++++++++++++++++++++
 include/linux/gpio/driver.h    |  2 --
 4 files changed, 48 insertions(+), 67 deletions(-)
 delete mode 100644 drivers/gpio/gpiolib-devprop.c

diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 4f9abff4f2dc..639275eb4e4d 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -6,7 +6,6 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
 obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib-devres.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib-legacy.o
-obj-$(CONFIG_GPIOLIB)		+= gpiolib-devprop.o
 obj-$(CONFIG_GPIOLIB)		+= gpiolib-cdev.o
 obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
 obj-$(CONFIG_GPIO_SYSFS)	+= gpiolib-sysfs.o
diff --git a/drivers/gpio/gpiolib-devprop.c b/drivers/gpio/gpiolib-devprop.c
deleted file mode 100644
index a28659b4f9c9..000000000000
--- a/drivers/gpio/gpiolib-devprop.c
+++ /dev/null
@@ -1,64 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Device property helpers for GPIO chips.
- *
- * Copyright (C) 2016, Intel Corporation
- * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
- */
-
-#include <linux/property.h>
-#include <linux/slab.h>
-#include <linux/gpio/consumer.h>
-#include <linux/gpio/driver.h>
-#include <linux/export.h>
-
-#include "gpiolib.h"
-
-/**
- * devprop_gpiochip_set_names - Set GPIO line names using device properties
- * @chip: GPIO chip whose lines should be named, if possible
- *
- * Looks for device property "gpio-line-names" and if it exists assigns
- * GPIO line names for the chip. The memory allocated for the assigned
- * names belong to the underlying software node and should not be released
- * by the caller.
- */
-int devprop_gpiochip_set_names(struct gpio_chip *chip)
-{
-	struct gpio_device *gdev = chip->gpiodev;
-	struct device *dev = chip->parent;
-	const char **names;
-	int ret, i;
-	int count;
-
-	count = device_property_read_string_array(dev, "gpio-line-names",
-						  NULL, 0);
-	if (count < 0)
-		return 0;
-
-	if (count > gdev->ngpio) {
-		dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d",
-			 count, gdev->ngpio);
-		count = gdev->ngpio;
-	}
-
-	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
-	if (!names)
-		return -ENOMEM;
-
-	ret = device_property_read_string_array(dev, "gpio-line-names",
-						names, count);
-	if (ret < 0) {
-		dev_warn(&gdev->dev, "failed to read GPIO line names\n");
-		kfree(names);
-		return ret;
-	}
-
-	for (i = 0; i < count; i++)
-		gdev->descs[i].name = names[i];
-
-	kfree(names);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(devprop_gpiochip_set_names);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0d390f0ec32c..15c99cf560ee 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -358,6 +358,54 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
 	return 0;
 }
 
+/*
+ * devprop_gpiochip_set_names - Set GPIO line names using device properties
+ * @chip: GPIO chip whose lines should be named, if possible
+ *
+ * Looks for device property "gpio-line-names" and if it exists assigns
+ * GPIO line names for the chip. The memory allocated for the assigned
+ * names belong to the underlying software node and should not be released
+ * by the caller.
+ */
+static int devprop_gpiochip_set_names(struct gpio_chip *chip)
+{
+	struct gpio_device *gdev = chip->gpiodev;
+	struct device *dev = chip->parent;
+	const char **names;
+	int ret, i;
+	int count;
+
+	count = device_property_read_string_array(dev, "gpio-line-names",
+						  NULL, 0);
+	if (count < 0)
+		return 0;
+
+	if (count > gdev->ngpio) {
+		dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d",
+			 count, gdev->ngpio);
+		count = gdev->ngpio;
+	}
+
+	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
+	if (!names)
+		return -ENOMEM;
+
+	ret = device_property_read_string_array(dev, "gpio-line-names",
+						names, count);
+	if (ret < 0) {
+		dev_warn(&gdev->dev, "failed to read GPIO line names\n");
+		kfree(names);
+		return ret;
+	}
+
+	for (i = 0; i < count; i++)
+		gdev->descs[i].name = names[i];
+
+	kfree(names);
+
+	return 0;
+}
+
 static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc)
 {
 	unsigned long *p;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 56485a040b82..4a7e295c3640 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -756,8 +756,6 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 					    enum gpiod_flags dflags);
 void gpiochip_free_own_desc(struct gpio_desc *desc);
 
-int devprop_gpiochip_set_names(struct gpio_chip *gc);
-
 #ifdef CONFIG_GPIOLIB
 
 /* lock/unlock as IRQ */
-- 
2.26.1

[PATCH 06/23] gpiolib: switch to simpler IDA interface

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We don't need to specify any ranges when allocating IDs so we can switch
to ida_alloc() and ida_free() instead of the ida_simple_ counterparts.

ida_simple_get(ida, 0, 0, gfp) is equivalent to
ida_alloc_range(ida, 0, UINT_MAX, gfp) which is equivalent to
ida_alloc(ida, gfp). Note: IDR will never actually allocate an ID
larger than INT_MAX.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 15c99cf560ee..591777bc2285 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -471,7 +471,7 @@ static void gpiodevice_release(struct device *dev)
 	struct gpio_device *gdev = dev_get_drvdata(dev);
 
 	list_del(&gdev->list);
-	ida_simple_remove(&gpio_ida, gdev->id);
+	ida_free(&gpio_ida, gdev->id);
 	kfree_const(gdev->label);
 	kfree(gdev->descs);
 	kfree(gdev);
@@ -582,7 +582,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		gc->of_node = gdev->dev.of_node;
 #endif
 
-	gdev->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL);
+	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
 	if (gdev->id < 0) {
 		ret = gdev->id;
 		goto err_free_gdev;
@@ -753,7 +753,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 err_free_descs:
 	kfree(gdev->descs);
 err_free_ida:
-	ida_simple_remove(&gpio_ida, gdev->id);
+	ida_free(&gpio_ida, gdev->id);
 err_free_gdev:
 	/* failures here can mean systems won't boot... */
 	pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,
-- 
2.26.1

[PATCH 07/23] gpio: mockup: drop unneeded includes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This module doesn't need gpio/consumer.h - it's a provider. It also
doesn't use any symbols from init.h so let's remove both includes.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index bc345185db26..349782cdb4d7 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -8,9 +8,7 @@
  */
 
 #include <linux/debugfs.h>
-#include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
-#include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
-- 
2.26.1

[PATCH 08/23] gpio: mockup: use pr_fmt()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We don't need a custom logging helper. Let's use the standard pr_fmt()
macro which allows us to use all pr_*() routines with custom format.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 349782cdb4d7..73cd51459c2a 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -21,6 +21,9 @@
 
 #include "gpiolib.h"
 
+#undef pr_fmt
+#define pr_fmt(fmt)		GPIO_MOCKUP_NAME ": " fmt
+
 #define GPIO_MOCKUP_NAME	"gpio-mockup"
 #define GPIO_MOCKUP_MAX_GC	10
 /*
@@ -31,8 +34,6 @@
 /* Maximum of three properties + the sentinel. */
 #define GPIO_MOCKUP_MAX_PROP	4
 
-#define gpio_mockup_err(...)	pr_err(GPIO_MOCKUP_NAME ": " __VA_ARGS__)
-
 /*
  * struct gpio_pin_status - structure describing a GPIO status
  * @dir:       Configures direction of gpio as "in" or "out"
@@ -549,7 +550,7 @@ static int __init gpio_mockup_init(void)
 
 	err = platform_driver_register(&gpio_mockup_driver);
 	if (err) {
-		gpio_mockup_err("error registering platform driver\n");
+		pr_err("error registering platform driver\n");
 		return err;
 	}
 
@@ -577,7 +578,7 @@ static int __init gpio_mockup_init(void)
 
 		pdev = platform_device_register_full(&pdevinfo);
 		if (IS_ERR(pdev)) {
-			gpio_mockup_err("error registering device");
+			pr_err("error registering device");
 			platform_driver_unregister(&gpio_mockup_driver);
 			gpio_mockup_unregister_pdevs();
 			return PTR_ERR(pdev);
-- 
2.26.1

[PATCH 09/23] gpio: mockup: use KBUILD_MODNAME

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Drop the definition for the driver name. Let's use KBUILD_MODNAME for
the log format and use the "gpio-mockup" value directly in the only
place where it's relevant: in the name of the device.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 73cd51459c2a..78c97f7b6893 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -22,9 +22,8 @@
 #include "gpiolib.h"
 
 #undef pr_fmt
-#define pr_fmt(fmt)		GPIO_MOCKUP_NAME ": " fmt
+#define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
 
-#define GPIO_MOCKUP_NAME	"gpio-mockup"
 #define GPIO_MOCKUP_MAX_GC	10
 /*
  * We're storing two values per chip: the GPIO base and the number
@@ -501,7 +500,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 
 static struct platform_driver gpio_mockup_driver = {
 	.driver = {
-		.name = GPIO_MOCKUP_NAME,
+		.name = "gpio-mockup",
 	},
 	.probe = gpio_mockup_probe,
 };
@@ -572,7 +571,7 @@ static int __init gpio_mockup_init(void)
 			properties[prop++] = PROPERTY_ENTRY_BOOL(
 						"named-gpio-lines");
 
-		pdevinfo.name = GPIO_MOCKUP_NAME;
+		pdevinfo.name = "gpio-mockup";
 		pdevinfo.id = i;
 		pdevinfo.properties = properties;
 
-- 
2.26.1

[PATCH 10/23] gpio: mockup: fix resource leak in error path

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

If the module init function fails after creating the debugs directory,
it's never removed. Add proper cleanup calls to avoid this resource
leak.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 78c97f7b6893..19c092f814fd 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -550,6 +550,7 @@ static int __init gpio_mockup_init(void)
 	err = platform_driver_register(&gpio_mockup_driver);
 	if (err) {
 		pr_err("error registering platform driver\n");
+		debugfs_remove_recursive(gpio_mockup_dbg_dir);
 		return err;
 	}
 
@@ -580,6 +581,7 @@ static int __init gpio_mockup_init(void)
 			pr_err("error registering device");
 			platform_driver_unregister(&gpio_mockup_driver);
 			gpio_mockup_unregister_pdevs();
+			debugfs_remove_recursive(gpio_mockup_dbg_dir);
 			return PTR_ERR(pdev);
 		}
 
-- 
2.26.1

[PATCH 11/23] gpio: mockup: remove the limit on number of dummy chips

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

As a preparation for implementing dynamically created dummy GPIO chips:
drop the limit on the number of chips. Let's use a linked list to store
the chip context structures.

Let's rename gpio_mockup_unregister_pdevs() to
gpio_mockup_unregister_devices() as we're now handling structures in
which struct platform_device is embedded instead of operating on
platform devices directly.

Note: this does not remove the limit on the number of ranges passed as a
module parameter. For now nothing changes in how the module works for
user-space. This patch will however allow us to create a higher number
of chips once we can do this dynamically.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 19c092f814fd..801fba6496a4 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -13,6 +13,7 @@
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
 #include <linux/irqdomain.h>
+#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
@@ -24,12 +25,11 @@
 #undef pr_fmt
 #define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
 
-#define GPIO_MOCKUP_MAX_GC	10
 /*
  * We're storing two values per chip: the GPIO base and the number
  * of GPIO lines.
  */
-#define GPIO_MOCKUP_MAX_RANGES	(GPIO_MOCKUP_MAX_GC * 2)
+#define GPIO_MOCKUP_MAX_RANGES	(10 * 2)
 /* Maximum of three properties + the sentinel. */
 #define GPIO_MOCKUP_MAX_PROP	4
 
@@ -505,27 +505,37 @@ static struct platform_driver gpio_mockup_driver = {
 	.probe = gpio_mockup_probe,
 };
 
-static struct platform_device *gpio_mockup_pdevs[GPIO_MOCKUP_MAX_GC];
+struct gpio_mockup_device {
+	struct list_head list;
+	struct platform_device *pdev;
+};
 
-static void gpio_mockup_unregister_pdevs(void)
+static LIST_HEAD(gpio_mockup_devices);
+
+static void gpio_mockup_unregister_one_device(struct gpio_mockup_device *dev)
 {
-	struct platform_device *pdev;
-	int i;
+	list_del(&dev->list);
+	platform_device_unregister(dev->pdev);
+	kfree(dev);
+}
 
-	for (i = 0; i < GPIO_MOCKUP_MAX_GC; i++) {
-		pdev = gpio_mockup_pdevs[i];
+static void gpio_mockup_unregister_devices(void)
+{
+	struct gpio_mockup_device *mockup_dev;
+	struct list_head *curr, *next;
 
-		if (pdev)
-			platform_device_unregister(pdev);
+	list_for_each_safe(curr, next, &gpio_mockup_devices) {
+		mockup_dev = list_entry(curr, struct gpio_mockup_device, list);
+		gpio_mockup_unregister_one_device(mockup_dev);
 	}
 }
 
 static int __init gpio_mockup_init(void)
 {
 	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
+	struct gpio_mockup_device *mockup_dev;
 	int i, prop, num_chips, err = 0, base;
 	struct platform_device_info pdevinfo;
-	struct platform_device *pdev;
 	u16 ngpio;
 
 	if ((gpio_mockup_num_ranges < 2) ||
@@ -576,26 +586,37 @@ static int __init gpio_mockup_init(void)
 		pdevinfo.id = i;
 		pdevinfo.properties = properties;
 
-		pdev = platform_device_register_full(&pdevinfo);
-		if (IS_ERR(pdev)) {
+		mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL);
+		if (!mockup_dev) {
+			err = -ENOMEM;
+			goto err_out;
+		}
+
+		mockup_dev->pdev = platform_device_register_full(&pdevinfo);
+		if (IS_ERR(mockup_dev->pdev)) {
 			pr_err("error registering device");
-			platform_driver_unregister(&gpio_mockup_driver);
-			gpio_mockup_unregister_pdevs();
-			debugfs_remove_recursive(gpio_mockup_dbg_dir);
-			return PTR_ERR(pdev);
+			kfree(mockup_dev);
+			err = PTR_ERR(mockup_dev->pdev);
+			goto err_out;
 		}
 
-		gpio_mockup_pdevs[i] = pdev;
+		list_add(&mockup_dev->list, &gpio_mockup_devices);
 	}
 
 	return 0;
+
+err_out:
+	platform_driver_unregister(&gpio_mockup_driver);
+	gpio_mockup_unregister_devices();
+	debugfs_remove_recursive(gpio_mockup_dbg_dir);
+	return err;
 }
 
 static void __exit gpio_mockup_exit(void)
 {
 	debugfs_remove_recursive(gpio_mockup_dbg_dir);
 	platform_driver_unregister(&gpio_mockup_driver);
-	gpio_mockup_unregister_pdevs();
+	gpio_mockup_unregister_devices();
 }
 
 module_init(gpio_mockup_init);
-- 
2.26.1

[PATCH 12/23] gpio: mockup: define a constant for chip label size

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We'll be using this value in many places in this driver soon so define
a constant to avoid using magic values.

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 801fba6496a4..e8a19a28ed13 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -32,6 +32,7 @@
 #define GPIO_MOCKUP_MAX_RANGES	(10 * 2)
 /* Maximum of three properties + the sentinel. */
 #define GPIO_MOCKUP_MAX_PROP	4
+#define GPIO_MOCKUP_LABEL_SIZE	32
 
 /*
  * struct gpio_pin_status - structure describing a GPIO status
-- 
2.26.1

[PATCH 13/23] gpio: mockup: pass the chip label as device property

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

While we do check the "chip-name" property in probe(), we never actually
use it. Let's pass the chip label to the driver using device properties
as we'll want to allow users to define their own once dynamically
created chips are supported.

The property is renamed to "chip-label" to not cause any confusion with
the actual chip name which is of the form: "gpiochipX".

If the "chip-label" property is missing, let's do what most devices in
drivers/gpio/ do and use dev_name().

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index e8a19a28ed13..ce83f1df1933 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -433,21 +433,14 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 	if (rv)
 		return rv;
 
-	rv = device_property_read_string(dev, "chip-name", &name);
+	rv = device_property_read_string(dev, "chip-label", &name);
 	if (rv)
-		name = NULL;
+		name = dev_name(dev);
 
 	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
-	if (!name) {
-		name = devm_kasprintf(dev, GFP_KERNEL,
-				      "%s-%c", pdev->name, pdev->id + 'A');
-		if (!name)
-			return -ENOMEM;
-	}
-
 	mutex_init(&chip->lock);
 
 	gc = &chip->gc;
@@ -534,6 +527,7 @@ static void gpio_mockup_unregister_devices(void)
 static int __init gpio_mockup_init(void)
 {
 	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
+	char chip_label[GPIO_MOCKUP_LABEL_SIZE];
 	struct gpio_mockup_device *mockup_dev;
 	int i, prop, num_chips, err = 0, base;
 	struct platform_device_info pdevinfo;
@@ -570,6 +564,11 @@ static int __init gpio_mockup_init(void)
 		memset(&pdevinfo, 0, sizeof(pdevinfo));
 		prop = 0;
 
+		snprintf(chip_label, sizeof(chip_label),
+			 "gpio-mockup-%c", i + 'A');
+		properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
+							   chip_label);
+
 		base = gpio_mockup_range_base(i);
 		if (base >= 0)
 			properties[prop++] = PROPERTY_ENTRY_U32("gpio-base",
-- 
2.26.1

[PATCH 14/23] gpio: mockup: use the generic 'gpio-line-names' property

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

GPIO line names are currently created by the driver from the chip label.
We'll want to support custom formats for line names (for instance: to
name all lines the same) for user-space tests so create them in the
module init function and pass them to the driver using the standard
'gpio-line-names' property.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index ce83f1df1933..96976ba66598 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -18,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include <linux/string_helpers.h>
 #include <linux/uaccess.h>
 
 #include "gpiolib.h"
@@ -378,29 +379,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
 	return;
 }
 
-static int gpio_mockup_name_lines(struct device *dev,
-				  struct gpio_mockup_chip *chip)
-{
-	struct gpio_chip *gc = &chip->gc;
-	char **names;
-	int i;
-
-	names = devm_kcalloc(dev, gc->ngpio, sizeof(char *), GFP_KERNEL);
-	if (!names)
-		return -ENOMEM;
-
-	for (i = 0; i < gc->ngpio; i++) {
-		names[i] = devm_kasprintf(dev, GFP_KERNEL,
-					  "%s-%d", gc->label, i);
-		if (!names[i])
-			return -ENOMEM;
-	}
-
-	gc->names = (const char *const *)names;
-
-	return 0;
-}
-
 static void gpio_mockup_dispose_mappings(void *data)
 {
 	struct gpio_mockup_chip *chip = data;
@@ -468,12 +446,6 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 	for (i = 0; i < gc->ngpio; i++)
 		chip->lines[i].dir = GPIO_LINE_DIRECTION_IN;
 
-	if (device_property_read_bool(dev, "named-gpio-lines")) {
-		rv = gpio_mockup_name_lines(dev, chip);
-		if (rv)
-			return rv;
-	}
-
 	chip->irq_sim_domain = devm_irq_domain_create_sim(dev, NULL,
 							  gc->ngpio);
 	if (IS_ERR(chip->irq_sim_domain))
@@ -524,6 +496,27 @@ static void gpio_mockup_unregister_devices(void)
 	}
 }
 
+static __init char **gpio_mockup_make_line_names(const char *label,
+						 unsigned int num_lines)
+{
+	unsigned int i;
+	char **names;
+
+	names = kcalloc(num_lines + 1, sizeof(char *), GFP_KERNEL);
+	if (!names)
+		return NULL;
+
+	for (i = 0; i < num_lines; i++) {
+		names[i] = kasprintf(GFP_KERNEL, "%s-%u", label, i);
+		if (!names[i]) {
+			kfree_strarray(names, i);
+			return NULL;
+		}
+	}
+
+	return names;
+}
+
 static int __init gpio_mockup_init(void)
 {
 	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
@@ -531,6 +524,7 @@ static int __init gpio_mockup_init(void)
 	struct gpio_mockup_device *mockup_dev;
 	int i, prop, num_chips, err = 0, base;
 	struct platform_device_info pdevinfo;
+	char **line_names;
 	u16 ngpio;
 
 	if ((gpio_mockup_num_ranges < 2) ||
@@ -563,6 +557,7 @@ static int __init gpio_mockup_init(void)
 		memset(properties, 0, sizeof(properties));
 		memset(&pdevinfo, 0, sizeof(pdevinfo));
 		prop = 0;
+		line_names = NULL;
 
 		snprintf(chip_label, sizeof(chip_label),
 			 "gpio-mockup-%c", i + 'A');
@@ -578,9 +573,18 @@ static int __init gpio_mockup_init(void)
 				 : gpio_mockup_range_ngpio(i) - base;
 		properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio);
 
-		if (gpio_mockup_named_lines)
-			properties[prop++] = PROPERTY_ENTRY_BOOL(
-						"named-gpio-lines");
+		if (gpio_mockup_named_lines) {
+			line_names = gpio_mockup_make_line_names(chip_label,
+								 ngpio);
+			if (!line_names) {
+				err = -ENOMEM;
+				goto err_out;
+			}
+
+			properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
+						"gpio-line-names",
+						line_names, ngpio);
+		}
 
 		pdevinfo.name = "gpio-mockup";
 		pdevinfo.id = i;
@@ -588,11 +592,13 @@ static int __init gpio_mockup_init(void)
 
 		mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL);
 		if (!mockup_dev) {
+			kfree_strarray(line_names, ngpio);
 			err = -ENOMEM;
 			goto err_out;
 		}
 
 		mockup_dev->pdev = platform_device_register_full(&pdevinfo);
+		kfree_strarray(line_names, ngpio);
 		if (IS_ERR(mockup_dev->pdev)) {
 			pr_err("error registering device");
 			kfree(mockup_dev);
-- 
2.26.1

[PATCH 15/23] gpio: mockup: use dynamic device IDs

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We're currently creating chips at module init time only so using a
static index for dummy devices is fine. We want to support dynamically
created chips however so we need to switch to dynamic device IDs.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 96976ba66598..995e37fef9c5 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -9,6 +9,7 @@
 
 #include <linux/debugfs.h>
 #include <linux/gpio/driver.h>
+#include <linux/idr.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
@@ -70,6 +71,8 @@ module_param_named(gpio_mockup_named_lines,
 
 static struct dentry *gpio_mockup_dbg_dir;
 
+static DEFINE_IDA(gpio_mockup_ida);
+
 static int gpio_mockup_range_base(unsigned int index)
 {
 	return gpio_mockup_ranges[index * 2];
@@ -480,8 +483,12 @@ static LIST_HEAD(gpio_mockup_devices);
 
 static void gpio_mockup_unregister_one_device(struct gpio_mockup_device *dev)
 {
+	int id;
+
 	list_del(&dev->list);
+	id = dev->pdev->id;
 	platform_device_unregister(dev->pdev);
+	ida_free(&gpio_mockup_ida, id);
 	kfree(dev);
 }
 
@@ -587,12 +594,19 @@ static int __init gpio_mockup_init(void)
 		}
 
 		pdevinfo.name = "gpio-mockup";
-		pdevinfo.id = i;
 		pdevinfo.properties = properties;
 
+		pdevinfo.id = ida_alloc(&gpio_mockup_ida, GFP_KERNEL);
+		if (pdevinfo.id < 0) {
+			kfree_strarray(line_names, ngpio);
+			err = pdevinfo.id;
+			goto err_out;
+		}
+
 		mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL);
 		if (!mockup_dev) {
 			kfree_strarray(line_names, ngpio);
+			ida_free(&gpio_mockup_ida, pdevinfo.id);
 			err = -ENOMEM;
 			goto err_out;
 		}
@@ -601,6 +615,7 @@ static int __init gpio_mockup_init(void)
 		kfree_strarray(line_names, ngpio);
 		if (IS_ERR(mockup_dev->pdev)) {
 			pr_err("error registering device");
+			ida_free(&gpio_mockup_ida, pdevinfo.id);
 			kfree(mockup_dev);
 			err = PTR_ERR(mockup_dev->pdev);
 			goto err_out;
-- 
2.26.1

[PATCH 16/23] gpio: mockup: refactor the module init function

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This is in preparation for dynamically created chips.

Let's split out the code that can be reused when creating chips at
run-time. Let's also move the code preparing the device properties into
a separate routine. This has the advantage of simplifying the error
handling.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 995e37fef9c5..eb94ddac5fee 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -524,16 +524,78 @@ static __init char **gpio_mockup_make_line_names(const char *label,
 	return names;
 }
 
-static int __init gpio_mockup_init(void)
+static int __init gpio_mockup_register_device(struct property_entry *properties)
 {
-	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
-	char chip_label[GPIO_MOCKUP_LABEL_SIZE];
 	struct gpio_mockup_device *mockup_dev;
-	int i, prop, num_chips, err = 0, base;
 	struct platform_device_info pdevinfo;
-	char **line_names;
+
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+
+	mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL);
+	if (!mockup_dev)
+		return -ENOMEM;
+
+	pdevinfo.name = "gpio-mockup";
+	pdevinfo.properties = properties;
+
+	pdevinfo.id = ida_alloc(&gpio_mockup_ida, GFP_KERNEL);
+	if (pdevinfo.id < 0) {
+		kfree(mockup_dev);
+		return pdevinfo.id;
+	}
+
+	mockup_dev->pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(mockup_dev->pdev)) {
+		ida_free(&gpio_mockup_ida, pdevinfo.id);
+		kfree(mockup_dev);
+		return PTR_ERR(mockup_dev->pdev);
+	}
+
+	list_add(&mockup_dev->list, &gpio_mockup_devices);
+
+	return 0;
+}
+
+static int __init gpio_mockup_register_one_chip_from_params(int idx)
+{
+	char chip_label[GPIO_MOCKUP_LABEL_SIZE], **line_names = NULL;
+	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
+	int prop = 0, base, ret;
 	u16 ngpio;
 
+	memset(properties, 0, sizeof(properties));
+
+	snprintf(chip_label, sizeof(chip_label), "gpio-mockup-%c", idx + 'A');
+	properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
+						   chip_label);
+
+	base = gpio_mockup_range_base(idx);
+	if (base >= 0)
+		properties[prop++] = PROPERTY_ENTRY_U32("gpio-base",
+							base);
+
+	ngpio = base < 0 ? gpio_mockup_range_ngpio(idx)
+			 : gpio_mockup_range_ngpio(idx) - base;
+	properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio);
+
+	if (gpio_mockup_named_lines) {
+		line_names = gpio_mockup_make_line_names(chip_label, ngpio);
+		if (!line_names)
+			return -ENOMEM;
+
+		properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
+					"gpio-line-names", line_names, ngpio);
+	}
+
+	ret = gpio_mockup_register_device(properties);
+	kfree_strarray(line_names, ngpio);
+	return ret;
+}
+
+static int __init gpio_mockup_register_chips_from_params(void)
+{
+	int num_chips, i, ret;
+
 	if ((gpio_mockup_num_ranges < 2) ||
 	    (gpio_mockup_num_ranges % 2) ||
 	    (gpio_mockup_num_ranges > GPIO_MOCKUP_MAX_RANGES))
@@ -551,86 +613,39 @@ static int __init gpio_mockup_init(void)
 			return -EINVAL;
 	}
 
-	gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL);
-
-	err = platform_driver_register(&gpio_mockup_driver);
-	if (err) {
-		pr_err("error registering platform driver\n");
-		debugfs_remove_recursive(gpio_mockup_dbg_dir);
-		return err;
-	}
-
 	for (i = 0; i < num_chips; i++) {
-		memset(properties, 0, sizeof(properties));
-		memset(&pdevinfo, 0, sizeof(pdevinfo));
-		prop = 0;
-		line_names = NULL;
-
-		snprintf(chip_label, sizeof(chip_label),
-			 "gpio-mockup-%c", i + 'A');
-		properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
-							   chip_label);
-
-		base = gpio_mockup_range_base(i);
-		if (base >= 0)
-			properties[prop++] = PROPERTY_ENTRY_U32("gpio-base",
-								base);
-
-		ngpio = base < 0 ? gpio_mockup_range_ngpio(i)
-				 : gpio_mockup_range_ngpio(i) - base;
-		properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio);
-
-		if (gpio_mockup_named_lines) {
-			line_names = gpio_mockup_make_line_names(chip_label,
-								 ngpio);
-			if (!line_names) {
-				err = -ENOMEM;
-				goto err_out;
-			}
-
-			properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
-						"gpio-line-names",
-						line_names, ngpio);
+		ret = gpio_mockup_register_one_chip_from_params(i);
+		if (ret) {
+			gpio_mockup_unregister_devices();
+			return ret;
 		}
+	}
 
-		pdevinfo.name = "gpio-mockup";
-		pdevinfo.properties = properties;
+	return 0;
+}
 
-		pdevinfo.id = ida_alloc(&gpio_mockup_ida, GFP_KERNEL);
-		if (pdevinfo.id < 0) {
-			kfree_strarray(line_names, ngpio);
-			err = pdevinfo.id;
-			goto err_out;
-		}
+static int __init gpio_mockup_init(void)
+{
+	int ret;
 
-		mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL);
-		if (!mockup_dev) {
-			kfree_strarray(line_names, ngpio);
-			ida_free(&gpio_mockup_ida, pdevinfo.id);
-			err = -ENOMEM;
-			goto err_out;
-		}
+	gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL);
 
-		mockup_dev->pdev = platform_device_register_full(&pdevinfo);
-		kfree_strarray(line_names, ngpio);
-		if (IS_ERR(mockup_dev->pdev)) {
-			pr_err("error registering device");
-			ida_free(&gpio_mockup_ida, pdevinfo.id);
-			kfree(mockup_dev);
-			err = PTR_ERR(mockup_dev->pdev);
-			goto err_out;
-		}
+	ret = platform_driver_register(&gpio_mockup_driver);
+	if (ret) {
+		pr_err("error registering platform driver\n");
+		debugfs_remove_recursive(gpio_mockup_dbg_dir);
+		return ret;
+	}
 
-		list_add(&mockup_dev->list, &gpio_mockup_devices);
+	ret = gpio_mockup_register_chips_from_params();
+	if (ret) {
+		pr_err("error registering device");
+		debugfs_remove_recursive(gpio_mockup_dbg_dir);
+		platform_driver_unregister(&gpio_mockup_driver);
+		return ret;
 	}
 
 	return 0;
-
-err_out:
-	platform_driver_unregister(&gpio_mockup_driver);
-	gpio_mockup_unregister_devices();
-	debugfs_remove_recursive(gpio_mockup_dbg_dir);
-	return err;
 }
 
 static void __exit gpio_mockup_exit(void)
-- 
2.26.1

[PATCH 17/23] gpio: mockup: rename and move around debugfs callbacks

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We will soon introduce new debugfs attributes for dynamically created
chips. We'll reuse the gpio_mockup_debugfs_open() helper for them but
the relevant write callbacks will of course be different.

Let's rename gpio_mockup_debugfs_write() to
gpio_mockup_debugfs_pull_write() to avoid confusion with new write
callbacks and move gpio_mockup_debugfs_open() higher up in the code to
separate it from the pull/value attribute which will no longer be the
only user.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index eb94ddac5fee..29fbf007ab26 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -262,9 +262,14 @@ static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset)
 	__gpio_mockup_set(chip, offset, chip->lines[offset].pull);
 }
 
-static ssize_t gpio_mockup_debugfs_read(struct file *file,
-					char __user *usr_buf,
-					size_t size, loff_t *ppos)
+static int gpio_mockup_debugfs_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, NULL, inode->i_private);
+}
+
+static ssize_t gpio_mockup_debugfs_value_read(struct file *file,
+					      char __user *usr_buf,
+					      size_t size, loff_t *ppos)
 {
 	struct gpio_mockup_dbgfs_private *priv;
 	struct gpio_mockup_chip *chip;
@@ -287,9 +292,9 @@ static ssize_t gpio_mockup_debugfs_read(struct file *file,
 	return simple_read_from_buffer(usr_buf, size, ppos, buf, cnt);
 }
 
-static ssize_t gpio_mockup_debugfs_write(struct file *file,
-					 const char __user *usr_buf,
-					 size_t size, loff_t *ppos)
+static ssize_t gpio_mockup_debugfs_pull_write(struct file *file,
+					      const char __user *usr_buf,
+					      size_t size, loff_t *ppos)
 {
 	struct gpio_mockup_dbgfs_private *priv;
 	int rv, val;
@@ -313,11 +318,6 @@ static ssize_t gpio_mockup_debugfs_write(struct file *file,
 	return size;
 }
 
-static int gpio_mockup_debugfs_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, NULL, inode->i_private);
-}
-
 /*
  * Each mockup chip is represented by a directory named after the chip's device
  * name under /sys/kernel/debug/gpio-mockup/. Each line is represented by
@@ -342,8 +342,8 @@ static int gpio_mockup_debugfs_open(struct inode *inode, struct file *file)
 static const struct file_operations gpio_mockup_debugfs_ops = {
 	.owner = THIS_MODULE,
 	.open = gpio_mockup_debugfs_open,
-	.read = gpio_mockup_debugfs_read,
-	.write = gpio_mockup_debugfs_write,
+	.read = gpio_mockup_debugfs_value_read,
+	.write = gpio_mockup_debugfs_pull_write,
 	.llseek = no_llseek,
 	.release = single_release,
 };
-- 
2.26.1

[PATCH 18/23] gpio: mockup: require debugfs to build

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Debugfs has become the standard way of interfacing with gpio-mockup to
the point where the module is not very useful without it anymore. Let's
make it a hard requirement to build gpio-mockup.

Let's also add error checks whenever calling debugfs routines as we now
don't expect them to fail.

The device sub-directories must now be removed when the device is
detached to correctly support dynamically created chips.

The call to debugfs_remove_recursive() in module exit must be moved to
the bottom or we'd risk to remove the root directory before devices can
unregister their own sub-directories.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/Kconfig       |  1 +
 drivers/gpio/gpio-mockup.c | 41 ++++++++++++++++++++++++++++----------
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8030fd91a3cc..515f345757d8 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1567,6 +1567,7 @@ config GPIO_AGGREGATOR
 
 config GPIO_MOCKUP
 	tristate "GPIO Testing Driver"
+	depends on DEBUG_FS
 	select IRQ_SIM
 	help
 	  This enables GPIO Testing driver, which provides a way to test GPIO
diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 29fbf007ab26..7df990662c17 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -348,38 +348,55 @@ static const struct file_operations gpio_mockup_debugfs_ops = {
 	.release = single_release,
 };
 
-static void gpio_mockup_debugfs_setup(struct device *dev,
-				      struct gpio_mockup_chip *chip)
+static void gpio_mockup_remove_chip_debugfs_entry(void *data)
+{
+	struct dentry *entry = data;
+
+	debugfs_remove_recursive(entry);
+}
+
+static int gpio_mockup_debugfs_setup(struct device *dev,
+				     struct gpio_mockup_chip *chip)
 {
 	struct gpio_mockup_dbgfs_private *priv;
 	struct gpio_chip *gc;
+	struct dentry *attr;
 	const char *devname;
 	char *name;
-	int i;
+	int i, ret;
 
 	gc = &chip->gc;
 	devname = dev_name(&gc->gpiodev->dev);
 
 	chip->dbg_dir = debugfs_create_dir(devname, gpio_mockup_dbg_dir);
+	if (IS_ERR(chip->dbg_dir))
+		return PTR_ERR(chip->dbg_dir);
+
+	ret = devm_add_action_or_reset(dev,
+			gpio_mockup_remove_chip_debugfs_entry, chip->dbg_dir);
+	if (ret)
+		return ret;
 
 	for (i = 0; i < gc->ngpio; i++) {
 		name = devm_kasprintf(dev, GFP_KERNEL, "%d", i);
 		if (!name)
-			return;
+			return -ENOMEM;
 
 		priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 		if (!priv)
-			return;
+			return -ENOMEM;
 
 		priv->chip = chip;
 		priv->offset = i;
 		priv->desc = &gc->gpiodev->descs[i];
 
-		debugfs_create_file(name, 0200, chip->dbg_dir, priv,
-				    &gpio_mockup_debugfs_ops);
+		attr = debugfs_create_file(name, 0200, chip->dbg_dir, priv,
+					   &gpio_mockup_debugfs_ops);
+		if (IS_ERR(attr))
+			return PTR_ERR(attr);
 	}
 
-	return;
+	return 0;
 }
 
 static void gpio_mockup_dispose_mappings(void *data)
@@ -462,7 +479,9 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 	if (rv)
 		return rv;
 
-	gpio_mockup_debugfs_setup(dev, chip);
+	rv = gpio_mockup_debugfs_setup(dev, chip);
+	if (rv)
+		return rv;
 
 	return 0;
 }
@@ -629,6 +648,8 @@ static int __init gpio_mockup_init(void)
 	int ret;
 
 	gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL);
+	if (IS_ERR(gpio_mockup_dbg_dir))
+		return PTR_ERR(gpio_mockup_dbg_dir);
 
 	ret = platform_driver_register(&gpio_mockup_driver);
 	if (ret) {
@@ -650,9 +671,9 @@ static int __init gpio_mockup_init(void)
 
 static void __exit gpio_mockup_exit(void)
 {
-	debugfs_remove_recursive(gpio_mockup_dbg_dir);
 	platform_driver_unregister(&gpio_mockup_driver);
 	gpio_mockup_unregister_devices();
+	debugfs_remove_recursive(gpio_mockup_dbg_dir);
 }
 
 module_init(gpio_mockup_init);
-- 
2.26.1

[PATCH 19/23] gpio: mockup: add a symlink for the per-chip debugfs directory

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We used to have a symlink named after the chip's label linking to the
per-chip directory named after the chip's name. This was removed by
commit d51ee07a8de7 ("gpio: mockup: don't create the debugfs link named
after the label") because there were no users of it.

This changeset proposes to reintroduce debugfs symlinks but inverted:
the link named after the device name points to the directory named after
the label. This way user-space can dynamically create a chip (once that
functionality is available), detect its creation over uevent and match
the device name to the label by resolving the link.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 7df990662c17..bc4609e047ef 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -52,6 +52,7 @@ struct gpio_mockup_chip {
 	struct gpio_mockup_line_status *lines;
 	struct irq_domain *irq_sim_domain;
 	struct dentry *dbg_dir;
+	struct dentry *dbg_link;
 	struct mutex lock;
 };
 
@@ -355,6 +356,13 @@ static void gpio_mockup_remove_chip_debugfs_entry(void *data)
 	debugfs_remove_recursive(entry);
 }
 
+static void gpio_mockup_remove_chip_debugfs_link(void *data)
+{
+	struct dentry *link = data;
+
+	debugfs_remove(link);
+}
+
 static int gpio_mockup_debugfs_setup(struct device *dev,
 				     struct gpio_mockup_chip *chip)
 {
@@ -368,7 +376,7 @@ static int gpio_mockup_debugfs_setup(struct device *dev,
 	gc = &chip->gc;
 	devname = dev_name(&gc->gpiodev->dev);
 
-	chip->dbg_dir = debugfs_create_dir(devname, gpio_mockup_dbg_dir);
+	chip->dbg_dir = debugfs_create_dir(gc->label, gpio_mockup_dbg_dir);
 	if (IS_ERR(chip->dbg_dir))
 		return PTR_ERR(chip->dbg_dir);
 
@@ -377,6 +385,16 @@ static int gpio_mockup_debugfs_setup(struct device *dev,
 	if (ret)
 		return ret;
 
+	chip->dbg_link = debugfs_create_symlink(devname, gpio_mockup_dbg_dir,
+						gc->label);
+	if (IS_ERR(chip->dbg_link))
+		return PTR_ERR(chip->dbg_link);
+
+	ret = devm_add_action_or_reset(dev,
+			gpio_mockup_remove_chip_debugfs_link, chip->dbg_link);
+	if (ret)
+		return ret;
+
 	for (i = 0; i < gc->ngpio; i++) {
 		name = devm_kasprintf(dev, GFP_KERNEL, "%d", i);
 		if (!name)
-- 
2.26.1

[PATCH 20/23] gpio: mockup: add a lock for dummy device list

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We'll soon add the possibility to create chips dynamically over debugfs
attributes. Since multiple threads will be able to create devices at
once: add a mutex to protect the device list.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index bc4609e047ef..1353239dc315 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -517,6 +517,7 @@ struct gpio_mockup_device {
 };
 
 static LIST_HEAD(gpio_mockup_devices);
+static DEFINE_MUTEX(gpio_mockup_devices_lock);
 
 static void gpio_mockup_unregister_one_device(struct gpio_mockup_device *dev)
 {
@@ -534,10 +535,14 @@ static void gpio_mockup_unregister_devices(void)
 	struct gpio_mockup_device *mockup_dev;
 	struct list_head *curr, *next;
 
+	mutex_lock(&gpio_mockup_devices_lock);
+
 	list_for_each_safe(curr, next, &gpio_mockup_devices) {
 		mockup_dev = list_entry(curr, struct gpio_mockup_device, list);
 		gpio_mockup_unregister_one_device(mockup_dev);
 	}
+
+	mutex_unlock(&gpio_mockup_devices_lock);
 }
 
 static __init char **gpio_mockup_make_line_names(const char *label,
@@ -588,7 +593,9 @@ static int __init gpio_mockup_register_device(struct property_entry *properties)
 		return PTR_ERR(mockup_dev->pdev);
 	}
 
+	mutex_lock(&gpio_mockup_devices_lock);
 	list_add(&mockup_dev->list, &gpio_mockup_devices);
+	mutex_unlock(&gpio_mockup_devices_lock);
 
 	return 0;
 }
-- 
2.26.1

[PATCH 21/23] gpio: mockup: provide a way to delete dummy chips

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add a new debugfs attribute 'delete_device' to which the chip label can
be written to dynamically remove the associated dummy device.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 1353239dc315..9d2de78a45c2 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <linux/string_helpers.h>
 #include <linux/uaccess.h>
 
@@ -668,14 +669,81 @@ static int __init gpio_mockup_register_chips_from_params(void)
 	return 0;
 }
 
-static int __init gpio_mockup_init(void)
+static ssize_t gpio_mockup_debugfs_delete_device_write(struct file *file,
+						const char __user *usr_buf,
+						size_t size, loff_t *ppos)
 {
+	struct gpio_mockup_device *mockup_dev;
+	char label[GPIO_MOCKUP_LABEL_SIZE];
+	struct list_head *curr;
+	struct device *dev;
+	const char *prop;
 	int ret;
 
+	if (*ppos != 0)
+		return -EINVAL;
+
+	ret = getline_from_user(label, sizeof(label), usr_buf, size);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&gpio_mockup_devices_lock);
+
+	list_for_each(curr, &gpio_mockup_devices) {
+		mockup_dev = list_entry(curr, struct gpio_mockup_device, list);
+		dev = &mockup_dev->pdev->dev;
+
+		ret = device_property_read_string(dev, "chip-label", &prop);
+		if (ret) {
+			mutex_unlock(&gpio_mockup_devices_lock);
+			return ret;
+		}
+
+		if (sysfs_streq(label, prop)) {
+			gpio_mockup_unregister_one_device(mockup_dev);
+			mutex_unlock(&gpio_mockup_devices_lock);
+			return size;
+		}
+	}
+
+	mutex_unlock(&gpio_mockup_devices_lock);
+	return -ENODEV;
+}
+
+static const struct file_operations gpio_mockup_debugfs_delete_device_ops = {
+	.owner = THIS_MODULE,
+	.open = gpio_mockup_debugfs_open,
+	.write = gpio_mockup_debugfs_delete_device_write,
+	.llseek = no_llseek,
+	.release = single_release,
+};
+
+static int __init gpio_mockup_debugfs_init(void)
+{
+	struct dentry *entry;
+
 	gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL);
 	if (IS_ERR(gpio_mockup_dbg_dir))
 		return PTR_ERR(gpio_mockup_dbg_dir);
 
+	entry = debugfs_create_file("delete_device", 0200, gpio_mockup_dbg_dir,
+				NULL, &gpio_mockup_debugfs_delete_device_ops);
+	if (IS_ERR(entry)) {
+		debugfs_remove_recursive(gpio_mockup_dbg_dir);
+		return PTR_ERR(entry);
+	}
+
+	return 0;
+}
+
+static int __init gpio_mockup_init(void)
+{
+	int ret;
+
+	ret = gpio_mockup_debugfs_init();
+	if (ret)
+		return ret;
+
 	ret = platform_driver_register(&gpio_mockup_driver);
 	if (ret) {
 		pr_err("error registering platform driver\n");
-- 
2.26.1

[PATCH 22/23] gpio: mockup: provide a way to create new dummy chips

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add a new debugfs attribute 'new_device' that allows to dynamically
create new dummy chips according to specification.

New chips are created by writing a number of supported parameters to the
new attribute of which 'label' and 'num_lines' are mandatory. The new
attribute is designed to be easily exstensible with new parameters. For
now we simply provide the same functionality that the module params
expose but with the intention of introducing new options (such as custom
line name formats).

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 9d2de78a45c2..6577d18671df 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2014  Kamlakant Patel <kamlakant.patel@broadcom.com>
  * Copyright (C) 2015-2016  Bamvor Jian Zhang <bamv2005@gmail.com>
  * Copyright (C) 2017 Bartosz Golaszewski <brgl@bgdev.pl>
+ * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@baylibre.com>
  */
 
 #include <linux/debugfs.h>
@@ -14,6 +15,7 @@
 #include <linux/irq.h>
 #include <linux/irq_sim.h>
 #include <linux/irqdomain.h>
+#include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -546,8 +548,8 @@ static void gpio_mockup_unregister_devices(void)
 	mutex_unlock(&gpio_mockup_devices_lock);
 }
 
-static __init char **gpio_mockup_make_line_names(const char *label,
-						 unsigned int num_lines)
+static char **gpio_mockup_make_line_names(const char *label,
+					  unsigned int num_lines)
 {
 	unsigned int i;
 	char **names;
@@ -567,7 +569,7 @@ static __init char **gpio_mockup_make_line_names(const char *label,
 	return names;
 }
 
-static int __init gpio_mockup_register_device(struct property_entry *properties)
+static int gpio_mockup_register_device(struct property_entry *properties)
 {
 	struct gpio_mockup_device *mockup_dev;
 	struct platform_device_info pdevinfo;
@@ -641,8 +643,7 @@ static int __init gpio_mockup_register_chips_from_params(void)
 {
 	int num_chips, i, ret;
 
-	if ((gpio_mockup_num_ranges < 2) ||
-	    (gpio_mockup_num_ranges % 2) ||
+	if ((gpio_mockup_num_ranges % 2) ||
 	    (gpio_mockup_num_ranges > GPIO_MOCKUP_MAX_RANGES))
 		return -EINVAL;
 
@@ -669,6 +670,205 @@ static int __init gpio_mockup_register_chips_from_params(void)
 	return 0;
 }
 
+/*
+ * We store all data associated with device properties in this structure. It's
+ * only needed until we register the platform device at which point the driver
+ * core makes a deep copy of all property data (even the string arrays).
+ *
+ * The reason to keep them bunched up is simple: we can have a single function
+ * to free all resources which simplifies error handling.
+ */
+struct gpio_mockup_prop_data {
+	char chip_label[GPIO_MOCKUP_LABEL_SIZE];
+	u16 ngpio;
+	bool named_lines;
+	char **line_names;
+};
+
+/* We don't free the structure itself - it's expected to live on the stack. */
+static void
+gpio_mockup_free_property_data(struct gpio_mockup_prop_data *prop_data)
+{
+	kfree_strarray(prop_data->line_names, prop_data->ngpio);
+}
+
+/*
+ * Each supported option is parsed by a separate callback - this way the
+ * 'new_device' attribute is easily exstensible.
+ */
+struct gpio_mockup_new_device_opt {
+	char *name;
+	int (*func)(const char *val, struct gpio_mockup_prop_data *prop_data,
+		    struct property_entry *properties, int *prop_idx);
+	bool has_val;
+};
+
+static int
+gpio_mockup_parse_label(const char *val,
+		struct gpio_mockup_prop_data *prop_data,
+		struct property_entry *properties, int *prop_idx)
+{
+	snprintf(prop_data->chip_label, sizeof(prop_data->chip_label), val);
+	properties[(*prop_idx)++] =
+		PROPERTY_ENTRY_STRING("chip-label", prop_data->chip_label);
+
+	return 0;
+}
+
+static int gpio_mockup_parse_num_lines(const char *val,
+			struct gpio_mockup_prop_data *prop_data,
+			struct property_entry *properties, int *prop_idx)
+{
+	int ret;
+
+	ret = kstrtou16(val, 10, &prop_data->ngpio);
+	if (ret) {
+		pr_err("invalid new_lines format: %s\n", val);
+		return ret;
+	}
+
+	properties[(*prop_idx)++] = PROPERTY_ENTRY_U16("nr-gpios",
+						     prop_data->ngpio);
+
+	return 0;
+}
+
+static int gpio_mockup_parse_named_lines(const char *val,
+			struct gpio_mockup_prop_data *prop_data,
+			struct property_entry *properties, int *prop_idx)
+{
+	prop_data->named_lines = true;
+
+	return 0;
+}
+
+static struct gpio_mockup_new_device_opt gpio_mockup_new_device_opts[] = {
+	{
+		.name = "label",
+		.func = gpio_mockup_parse_label,
+		.has_val = true,
+	},
+	{
+		.name = "num_lines",
+		.func = gpio_mockup_parse_num_lines,
+		.has_val = true,
+	},
+	{
+		.name = "named_lines",
+		.func = gpio_mockup_parse_named_lines,
+		.has_val = false,
+	},
+};
+
+static int
+gpio_mockup_parse_one_opt(const char *key, const char *val,
+			  struct gpio_mockup_prop_data *prop_data,
+			  struct property_entry *properties, int *prop_idx)
+{
+	struct gpio_mockup_new_device_opt *opt;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(gpio_mockup_new_device_opts); i++) {
+		opt = &gpio_mockup_new_device_opts[i];
+
+		if (strcmp(key, opt->name) == 0) {
+			if (opt->has_val && !val) {
+				pr_err("%s option requires an argument\n",
+				       opt->name);
+				return -EINVAL;
+			}
+
+			if (!opt->has_val && val) {
+				pr_err("%s option doesn't take any arguments\n",
+				       opt->name);
+				return -EINVAL;
+			}
+
+			return opt->func(val, prop_data, properties, prop_idx);
+		}
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int
+gpio_mockup_new_device_from_opts(char *opts,
+				 struct gpio_mockup_prop_data *prop_data)
+{
+	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
+	int prop_idx = 0, ret;
+	char *key, *val;
+
+	memset(properties, 0, sizeof(properties));
+
+	while (*opts) {
+		if (prop_idx >= GPIO_MOCKUP_MAX_PROP)
+			return -EINVAL;
+
+		opts = next_arg(opts, &key, &val);
+
+		ret = gpio_mockup_parse_one_opt(key, val, prop_data,
+						properties, &prop_idx);
+		if (ret)
+			return ret;
+	}
+
+	/* This is the only mandatory property. */
+	if (!prop_data->ngpio) {
+		pr_err("number of lines must be specified\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Line names must be created at the end - once we know how
+	 * many GPIOs there are.
+	 */
+	if (prop_data->named_lines) {
+		prop_data->line_names = gpio_mockup_make_line_names(
+							prop_data->chip_label,
+							prop_data->ngpio);
+		if (!prop_data->line_names)
+			return -ENOMEM;
+
+		properties[prop_idx++] =
+			PROPERTY_ENTRY_STRING_ARRAY_LEN("gpio-line-names",
+							prop_data->line_names,
+							prop_data->ngpio);
+	}
+
+	return gpio_mockup_register_device(properties);
+}
+
+static ssize_t gpio_mockup_debugfs_new_device_write(struct file *file,
+						    const char __user *usr_buf,
+						    size_t size, loff_t *ppos)
+{
+	struct gpio_mockup_prop_data prop_data;
+	char opts[128];
+	int ret;
+
+	if (*ppos != 0 || size > sizeof(opts))
+		return -EINVAL;
+
+	ret = getline_from_user(opts, sizeof(opts), usr_buf, size);
+	if (ret < 0)
+		return ret;
+
+	memset(&prop_data, 0, sizeof(prop_data));
+
+	ret = gpio_mockup_new_device_from_opts(opts, &prop_data);
+	gpio_mockup_free_property_data(&prop_data);
+	return ret < 0 ? ret : size;
+}
+
+static const struct file_operations gpio_mockup_debugfs_new_device_ops = {
+	.owner = THIS_MODULE,
+	.open = gpio_mockup_debugfs_open,
+	.write = gpio_mockup_debugfs_new_device_write,
+	.llseek = no_llseek,
+	.release = single_release,
+};
+
 static ssize_t gpio_mockup_debugfs_delete_device_write(struct file *file,
 						const char __user *usr_buf,
 						size_t size, loff_t *ppos)
@@ -726,6 +926,13 @@ static int __init gpio_mockup_debugfs_init(void)
 	if (IS_ERR(gpio_mockup_dbg_dir))
 		return PTR_ERR(gpio_mockup_dbg_dir);
 
+	entry = debugfs_create_file("new_device", 0200, gpio_mockup_dbg_dir,
+				NULL, &gpio_mockup_debugfs_new_device_ops);
+	if (IS_ERR(entry)) {
+		debugfs_remove_recursive(gpio_mockup_dbg_dir);
+		return PTR_ERR(entry);
+	}
+
 	entry = debugfs_create_file("delete_device", 0200, gpio_mockup_dbg_dir,
 				NULL, &gpio_mockup_debugfs_delete_device_ops);
 	if (IS_ERR(entry)) {
@@ -751,12 +958,14 @@ static int __init gpio_mockup_init(void)
 		return ret;
 	}
 
-	ret = gpio_mockup_register_chips_from_params();
-	if (ret) {
-		pr_err("error registering device");
-		debugfs_remove_recursive(gpio_mockup_dbg_dir);
-		platform_driver_unregister(&gpio_mockup_driver);
-		return ret;
+	if (gpio_mockup_ranges > 0) {
+		ret = gpio_mockup_register_chips_from_params();
+		if (ret) {
+			pr_err("error registering device");
+			debugfs_remove_recursive(gpio_mockup_dbg_dir);
+			platform_driver_unregister(&gpio_mockup_driver);
+			return ret;
+		}
 	}
 
 	return 0;
-- 
2.26.1

[PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

There's some documentation for gpio-mockup's debugfs interface in the
driver's source but it's not much. Add proper documentation for this
testing module.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../admin-guide/gpio/gpio-mockup.rst          | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/admin-guide/gpio/gpio-mockup.rst

diff --git a/Documentation/admin-guide/gpio/gpio-mockup.rst b/Documentation/admin-guide/gpio/gpio-mockup.rst
new file mode 100644
index 000000000000..1d452ee55f8d
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-mockup.rst
@@ -0,0 +1,87 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+GPIO Testing Driver
+===================
+
+The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
+chips for testing purposes. There are two ways of configuring the chips exposed
+by the module. The lines can be accessed using the standard GPIO character
+device interface as well as manipulated using the dedicated debugfs directory
+structure.
+
+Creating simulated chips using debugfs
+--------------------------------------
+
+When the gpio-mockup module is loaded (or builtin) it creates its own directory
+in debugfs. Assuming debugfs is mounted at /sys/kernel/debug/, the directory
+will be located at /sys/kernel/debug/gpio-mockup/. Inside this directory there
+are two attributes: new_device and delete_device.
+
+New chips can be created by writing a single line containing a number of
+options to "new_device". For example:
+
+.. code-block:: sh
+
+    $ echo "label=my-mockup num_lines=4 named_lines" > /sys/kernel/debug/gpio-mockup/new_device
+
+Supported options:
+
+    num_lines=<num_lines> - number of GPIO lines to expose
+
+    label=<label> - label of the dummy chip
+
+    named_lines - defines whether dummy lines should be named, the names are
+                  of the form X-Y where X is the chip's label and Y is the
+                  line's offset
+
+Note: only num_lines is mandatory.
+
+Chips can be dynamically removed by writing the chip's label to
+"delete_device". For example:
+
+.. code-block:: sh
+
+    echo "gpio-mockup.0" > /sys/kernel/debug/gpio-mockup/delete_device
+
+Creating simulated chips using module params
+--------------------------------------------
+
+Note: this is an older, now deprecated method kept for backward compatibility
+for user-space tools.
+
+When loading the gpio-mockup driver a number of parameters can be passed to the
+module.
+
+    gpio_mockup_ranges
+
+        This parameter takes an argument in the form of an array of integer
+        pairs. Each pair defines the base GPIO number (if any) and the number
+        of lines exposed by the chip. If the base GPIO is -1, the gpiolib
+        will assign it automatically.
+
+        Example: gpio_mockup_ranges=-1,8,-1,16,405,4
+
+        The line above creates three chips. The first one will expose 8 lines,
+        the second 16 and the third 4. The base GPIO for the third chip is set
+        to 405 while for two first chips it will be assigned automatically.
+
+    gpio_named_lines
+
+        This parameter doesn't take any arguments. It lets the driver know that
+        GPIO lines exposed by it should be named.
+
+        The name format is: gpio-mockup-X-Y where X is the letter associated
+        with the mockup chip and Y is the line offset.
+
+Manipulating simulated lines
+----------------------------
+
+Each mockup chip creates its own subdirectory in /sys/kernel/debug/gpio-mockup/.
+The directory is named after the chip's label. A symlink is also created, named
+after the chip's name, which points to the label directory.
+
+Inside each subdirectory, there's a separate attribute for each GPIO line. The
+name of the attribute represents the line's offset in the chip.
+
+Reading from a line attribute returns the current value. Writing to it (0 or 1)
+changes its pull.
-- 
2.26.1

Re: [PATCH 08/23] gpio: mockup: use pr_fmt()

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:32PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We don't need a custom logging helper. Let's use the standard pr_fmt()
> macro which allows us to use all pr_*() routines with custom format.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-mockup.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index 349782cdb4d7..73cd51459c2a 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -21,6 +21,9 @@
>  
>  #include "gpiolib.h"

> +#undef pr_fmt
> +#define pr_fmt(fmt)		GPIO_MOCKUP_NAME ": " fmt
> +

Just put definition to be first line of code before other inclusions and drop
unnecessary #undef.

>  #define GPIO_MOCKUP_NAME	"gpio-mockup"
>  #define GPIO_MOCKUP_MAX_GC	10
>  /*
> @@ -31,8 +34,6 @@
>  /* Maximum of three properties + the sentinel. */
>  #define GPIO_MOCKUP_MAX_PROP	4
>  
> -#define gpio_mockup_err(...)	pr_err(GPIO_MOCKUP_NAME ": " __VA_ARGS__)
> -
>  /*
>   * struct gpio_pin_status - structure describing a GPIO status
>   * @dir:       Configures direction of gpio as "in" or "out"
> @@ -549,7 +550,7 @@ static int __init gpio_mockup_init(void)
>  
>  	err = platform_driver_register(&gpio_mockup_driver);
>  	if (err) {
> -		gpio_mockup_err("error registering platform driver\n");
> +		pr_err("error registering platform driver\n");
>  		return err;
>  	}
>  
> @@ -577,7 +578,7 @@ static int __init gpio_mockup_init(void)
>  
>  		pdev = platform_device_register_full(&pdevinfo);
>  		if (IS_ERR(pdev)) {
> -			gpio_mockup_err("error registering device");
> +			pr_err("error registering device");
>  			platform_driver_unregister(&gpio_mockup_driver);
>  			gpio_mockup_unregister_pdevs();
>  			return PTR_ERR(pdev);
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 09/23] gpio: mockup: use KBUILD_MODNAME

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:33PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Drop the definition for the driver name. Let's use KBUILD_MODNAME for
> the log format and use the "gpio-mockup" value directly in the only
> place where it's relevant: in the name of the device.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-mockup.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index 73cd51459c2a..78c97f7b6893 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -22,9 +22,8 @@
>  #include "gpiolib.h"

>  #undef pr_fmt
> -#define pr_fmt(fmt)		GPIO_MOCKUP_NAME ": " fmt
> +#define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt

This should be part of previous patch with something like "this is equivalent
right now".

> -#define GPIO_MOCKUP_NAME	"gpio-mockup"
>  #define GPIO_MOCKUP_MAX_GC	10
>  /*
>   * We're storing two values per chip: the GPIO base and the number
> @@ -501,7 +500,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
>  
>  static struct platform_driver gpio_mockup_driver = {
>  	.driver = {
> -		.name = GPIO_MOCKUP_NAME,
> +		.name = "gpio-mockup",
>  	},
>  	.probe = gpio_mockup_probe,
>  };
> @@ -572,7 +571,7 @@ static int __init gpio_mockup_init(void)
>  			properties[prop++] = PROPERTY_ENTRY_BOOL(
>  						"named-gpio-lines");
>  
> -		pdevinfo.name = GPIO_MOCKUP_NAME;
> +		pdevinfo.name = "gpio-mockup";
>  		pdevinfo.id = i;
>  		pdevinfo.properties = properties;
>  
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/23] lib: string_helpers: provide kfree_strarray()

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:26PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> There's a common pattern of dynamically allocating an array of char
> pointers and then also dynamically allocating each string in this
> array. Provide a helper for freeing such a string array with one call.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/string_helpers.h |  2 ++
>  lib/string_helpers.c           | 22 ++++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index 86f150c2a6b6..55b25120a1c6 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -94,4 +94,6 @@ char *kstrdup_quotable(const char *src, gfp_t gfp);
>  char *kstrdup_quotable_cmdline(struct task_struct *task, gfp_t gfp);
>  char *kstrdup_quotable_file(struct file *file, gfp_t gfp);
>  
> +void kfree_strarray(char **str_array, size_t num_str);
> +
>  #endif
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 963050c0283e..22505efc6aae 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -649,3 +649,25 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp)
>  	return pathname;
>  }
>  EXPORT_SYMBOL_GPL(kstrdup_quotable_file);
> +
> +/**
> + * kfree_strarray - free a number of dynamically allocated strings contained
> + *                  in an array and the array itself
> + *
> + * @str_array: Dynamically allocated array of strings to free. If NULL - the
> + *             function does nothing.
> + * @num_str: Number of strings (starting from the beginning of the array) to
> + *           free.
> + */
> +void kfree_strarray(char **str_array, size_t num_str)
> +{
> +	unsigned int i;

> +	if (!str_array)
> +		return;

Shouldn't we expect that caller will supply NULL, 0 and above check is not
needed?

> +	for (i = 0; i < num_str; i++)
> +		kfree(str_array[i]);
> +	kfree(str_array);

> +}
> +EXPORT_SYMBOL_GPL(kfree_strarray);
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/23] lib: uaccess: provide getline_from_user()

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Provide a uaccess helper that allows callers to copy a single line from
> user memory. This is useful for debugfs write callbacks.

Doesn't mm/util.c provides us something like this?
strndup_user()?

> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/uaccess.h |  3 +++
>  lib/usercopy.c          | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 94b285411659..5aedd8ac5c31 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -333,6 +333,9 @@ long strncpy_from_user_nofault(char *dst, const void __user *unsafe_addr,
>  		long count);
>  long strnlen_user_nofault(const void __user *unsafe_addr, long count);
>  
> +ssize_t getline_from_user(char *dst, size_t dst_size,
> +			  const char __user *src, size_t src_size);
> +
>  /**
>   * get_kernel_nofault(): safely attempt to read from a location
>   * @val: read into this variable
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> index b26509f112f9..55aaaf93d847 100644
> --- a/lib/usercopy.c
> +++ b/lib/usercopy.c
> @@ -87,3 +87,40 @@ int check_zeroed_user(const void __user *from, size_t size)
>  	return -EFAULT;
>  }
>  EXPORT_SYMBOL(check_zeroed_user);
> +
> +/**
> + * getline_from_user - Copy a single line from user
> + * @dst: Where to copy the line to
> + * @dst_size: Size of the destination buffer
> + * @src: Where to copy the line from
> + * @src_size: Size of the source user buffer
> + *
> + * Copies a number of characters from given user buffer into the dst buffer.
> + * The number of bytes is limited to the lesser of the sizes of both buffers.
> + * If the copied string contains a newline, its first occurrence is replaced
> + * by a NULL byte in the destination buffer. Otherwise the function ensures
> + * the copied string is NULL-terminated.
> + *
> + * Returns the number of copied bytes or a negative error number on failure.
> + */
> +
> +ssize_t getline_from_user(char *dst, size_t dst_size,
> +			  const char __user *src, size_t src_size)
> +{
> +	size_t size = min_t(size_t, dst_size, src_size);
> +	char *c;
> +	int ret;
> +
> +	ret = copy_from_user(dst, src, size);
> +	if (ret)
> +		return -EFAULT;
> +
> +	dst[size - 1] = '\0';
> +
> +	c = strchrnul(dst, '\n');
> +	if (*c)
> +		*c = '\0';
> +
> +	return c - dst;
> +}
> +EXPORT_SYMBOL(getline_from_user);
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 04/23] gpiolib: generalize devprop_gpiochip_set_names() for device properties

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:28PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> devprop_gpiochip_set_names() is overly complicated with taking the
> fwnode argument (which requires using dev_fwnode() & of_fwnode_handle()
> in ACPI and OF GPIO code respectively). Let's just switch to using the
> generic device properties.
> 
> This allows us to pull the code setting line names directly into
> gpiochip_add_data_with_key() instead of handling it separately for
> ACPI and OF.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpiolib-acpi.c    |  3 ---
>  drivers/gpio/gpiolib-devprop.c | 19 ++++++++++---------
>  drivers/gpio/gpiolib-of.c      |  5 -----
>  drivers/gpio/gpiolib.c         |  8 ++++----
>  include/linux/gpio/driver.h    |  3 +--
>  5 files changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 54ca3c18b291..834a12f3219e 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1221,9 +1221,6 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
>  		return;
>  	}
>  
> -	if (!chip->names)
> -		devprop_gpiochip_set_names(chip, dev_fwnode(chip->parent));
> -
>  	acpi_gpiochip_request_regions(acpi_gpio);
>  	acpi_gpiochip_scan_gpios(acpi_gpio);
>  	acpi_walk_dep_device_list(handle);
> diff --git a/drivers/gpio/gpiolib-devprop.c b/drivers/gpio/gpiolib-devprop.c
> index 26741032fa9e..a28659b4f9c9 100644
> --- a/drivers/gpio/gpiolib-devprop.c
> +++ b/drivers/gpio/gpiolib-devprop.c
> @@ -17,25 +17,24 @@
>  /**
>   * devprop_gpiochip_set_names - Set GPIO line names using device properties
>   * @chip: GPIO chip whose lines should be named, if possible
> - * @fwnode: Property Node containing the gpio-line-names property
>   *
>   * Looks for device property "gpio-line-names" and if it exists assigns
>   * GPIO line names for the chip. The memory allocated for the assigned
> - * names belong to the underlying firmware node and should not be released
> + * names belong to the underlying software node and should not be released
>   * by the caller.
>   */
> -void devprop_gpiochip_set_names(struct gpio_chip *chip,
> -				const struct fwnode_handle *fwnode)
> +int devprop_gpiochip_set_names(struct gpio_chip *chip)
>  {
>  	struct gpio_device *gdev = chip->gpiodev;
> +	struct device *dev = chip->parent;
>  	const char **names;
>  	int ret, i;
>  	int count;
>  
> -	count = fwnode_property_read_string_array(fwnode, "gpio-line-names",
> +	count = device_property_read_string_array(dev, "gpio-line-names",
>  						  NULL, 0);
>  	if (count < 0)
> -		return;
> +		return 0;

Can we introduce a followup to 33ee09cd59ce ("device property: Add helpers to
count items in an array") for strings?

>  	if (count > gdev->ngpio) {
>  		dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d",
> @@ -45,19 +44,21 @@ void devprop_gpiochip_set_names(struct gpio_chip *chip,
>  
>  	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
>  	if (!names)
> -		return;
> +		return -ENOMEM;
>  
> -	ret = fwnode_property_read_string_array(fwnode, "gpio-line-names",
> +	ret = device_property_read_string_array(dev, "gpio-line-names",
>  						names, count);
>  	if (ret < 0) {
>  		dev_warn(&gdev->dev, "failed to read GPIO line names\n");
>  		kfree(names);
> -		return;
> +		return ret;
>  	}
>  
>  	for (i = 0; i < count; i++)
>  		gdev->descs[i].name = names[i];
>  
>  	kfree(names);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(devprop_gpiochip_set_names);
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index bd31dd3b6a75..2f895a2b8411 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -1026,11 +1026,6 @@ int of_gpiochip_add(struct gpio_chip *chip)
>  	if (ret)
>  		return ret;
>  
> -	/* If the chip defines names itself, these take precedence */
> -	if (!chip->names)
> -		devprop_gpiochip_set_names(chip,
> -					   of_fwnode_handle(chip->of_node));
> -
>  	of_node_get(chip->of_node);
>  
>  	ret = of_gpiochip_scan_gpios(chip);
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 80137c1b3cdc..0d390f0ec32c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -340,9 +340,6 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
>  	struct gpio_device *gdev = gc->gpiodev;
>  	int i;
>  
> -	if (!gc->names)
> -		return 0;
> -
>  	/* First check all names if they are unique */
>  	for (i = 0; i != gc->ngpio; ++i) {
>  		struct gpio_desc *gpio;
> @@ -621,7 +618,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  	INIT_LIST_HEAD(&gdev->pin_ranges);
>  #endif
>  
> -	ret = gpiochip_set_desc_names(gc);
> +	if (gc->names)
> +		ret = gpiochip_set_desc_names(gc);
> +	else
> +		ret = devprop_gpiochip_set_names(gc);
>  	if (ret)
>  		goto err_remove_from_list;
>  
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index d1cef5c2715c..56485a040b82 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -756,8 +756,7 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
>  					    enum gpiod_flags dflags);
>  void gpiochip_free_own_desc(struct gpio_desc *desc);
>  
> -void devprop_gpiochip_set_names(struct gpio_chip *gc,
> -				const struct fwnode_handle *fwnode);
> +int devprop_gpiochip_set_names(struct gpio_chip *gc);
>  
>  #ifdef CONFIG_GPIOLIB
>  
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 05/23] gpiolib: unexport devprop_gpiochip_set_names()

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:29PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Now that devprop_gpiochip_set_names() is only used in a single place
> inside drivers/gpio/gpiolib.c, there's no need anymore for it to be
> exported or to even live in its own source file. Pull this function into
> the core source file for gpiolib.

I have mixed feelings about this. We may simply unexport and attach object file
to gpiolib.o. Would it be expected to see more functions in this file in the
future?

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/Makefile          |  1 -
>  drivers/gpio/gpiolib-devprop.c | 64 ----------------------------------
>  drivers/gpio/gpiolib.c         | 48 +++++++++++++++++++++++++
>  include/linux/gpio/driver.h    |  2 --
>  4 files changed, 48 insertions(+), 67 deletions(-)
>  delete mode 100644 drivers/gpio/gpiolib-devprop.c
> 
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 4f9abff4f2dc..639275eb4e4d 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -6,7 +6,6 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
>  obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
>  obj-$(CONFIG_GPIOLIB)		+= gpiolib-devres.o
>  obj-$(CONFIG_GPIOLIB)		+= gpiolib-legacy.o
> -obj-$(CONFIG_GPIOLIB)		+= gpiolib-devprop.o
>  obj-$(CONFIG_GPIOLIB)		+= gpiolib-cdev.o
>  obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
>  obj-$(CONFIG_GPIO_SYSFS)	+= gpiolib-sysfs.o
> diff --git a/drivers/gpio/gpiolib-devprop.c b/drivers/gpio/gpiolib-devprop.c
> deleted file mode 100644
> index a28659b4f9c9..000000000000
> --- a/drivers/gpio/gpiolib-devprop.c
> +++ /dev/null
> @@ -1,64 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Device property helpers for GPIO chips.
> - *
> - * Copyright (C) 2016, Intel Corporation
> - * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> - */
> -
> -#include <linux/property.h>
> -#include <linux/slab.h>
> -#include <linux/gpio/consumer.h>
> -#include <linux/gpio/driver.h>
> -#include <linux/export.h>
> -
> -#include "gpiolib.h"
> -
> -/**
> - * devprop_gpiochip_set_names - Set GPIO line names using device properties
> - * @chip: GPIO chip whose lines should be named, if possible
> - *
> - * Looks for device property "gpio-line-names" and if it exists assigns
> - * GPIO line names for the chip. The memory allocated for the assigned
> - * names belong to the underlying software node and should not be released
> - * by the caller.
> - */
> -int devprop_gpiochip_set_names(struct gpio_chip *chip)
> -{
> -	struct gpio_device *gdev = chip->gpiodev;
> -	struct device *dev = chip->parent;
> -	const char **names;
> -	int ret, i;
> -	int count;
> -
> -	count = device_property_read_string_array(dev, "gpio-line-names",
> -						  NULL, 0);
> -	if (count < 0)
> -		return 0;
> -
> -	if (count > gdev->ngpio) {
> -		dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d",
> -			 count, gdev->ngpio);
> -		count = gdev->ngpio;
> -	}
> -
> -	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
> -	if (!names)
> -		return -ENOMEM;
> -
> -	ret = device_property_read_string_array(dev, "gpio-line-names",
> -						names, count);
> -	if (ret < 0) {
> -		dev_warn(&gdev->dev, "failed to read GPIO line names\n");
> -		kfree(names);
> -		return ret;
> -	}
> -
> -	for (i = 0; i < count; i++)
> -		gdev->descs[i].name = names[i];
> -
> -	kfree(names);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(devprop_gpiochip_set_names);
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 0d390f0ec32c..15c99cf560ee 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -358,6 +358,54 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
>  	return 0;
>  }
>  
> +/*
> + * devprop_gpiochip_set_names - Set GPIO line names using device properties
> + * @chip: GPIO chip whose lines should be named, if possible
> + *
> + * Looks for device property "gpio-line-names" and if it exists assigns
> + * GPIO line names for the chip. The memory allocated for the assigned
> + * names belong to the underlying software node and should not be released
> + * by the caller.
> + */
> +static int devprop_gpiochip_set_names(struct gpio_chip *chip)
> +{
> +	struct gpio_device *gdev = chip->gpiodev;
> +	struct device *dev = chip->parent;
> +	const char **names;
> +	int ret, i;
> +	int count;
> +
> +	count = device_property_read_string_array(dev, "gpio-line-names",
> +						  NULL, 0);
> +	if (count < 0)
> +		return 0;
> +
> +	if (count > gdev->ngpio) {
> +		dev_warn(&gdev->dev, "gpio-line-names is length %d but should be at most length %d",
> +			 count, gdev->ngpio);
> +		count = gdev->ngpio;
> +	}
> +
> +	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
> +	if (!names)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_string_array(dev, "gpio-line-names",
> +						names, count);
> +	if (ret < 0) {
> +		dev_warn(&gdev->dev, "failed to read GPIO line names\n");
> +		kfree(names);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < count; i++)
> +		gdev->descs[i].name = names[i];
> +
> +	kfree(names);
> +
> +	return 0;
> +}
> +
>  static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc)
>  {
>  	unsigned long *p;
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 56485a040b82..4a7e295c3640 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -756,8 +756,6 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
>  					    enum gpiod_flags dflags);
>  void gpiochip_free_own_desc(struct gpio_desc *desc);
>  
> -int devprop_gpiochip_set_names(struct gpio_chip *gc);
> -
>  #ifdef CONFIG_GPIOLIB
>  
>  /* lock/unlock as IRQ */
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/23] gpiolib: switch to simpler IDA interface

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:30PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We don't need to specify any ranges when allocating IDs so we can switch
> to ida_alloc() and ida_free() instead of the ida_simple_ counterparts.
> 
> ida_simple_get(ida, 0, 0, gfp) is equivalent to
> ida_alloc_range(ida, 0, UINT_MAX, gfp) which is equivalent to
> ida_alloc(ida, gfp). Note: IDR will never actually allocate an ID
> larger than INT_MAX.

Have you considered switching to XArray API?

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpiolib.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 15c99cf560ee..591777bc2285 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -471,7 +471,7 @@ static void gpiodevice_release(struct device *dev)
>  	struct gpio_device *gdev = dev_get_drvdata(dev);
>  
>  	list_del(&gdev->list);
> -	ida_simple_remove(&gpio_ida, gdev->id);
> +	ida_free(&gpio_ida, gdev->id);
>  	kfree_const(gdev->label);
>  	kfree(gdev->descs);
>  	kfree(gdev);
> @@ -582,7 +582,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  		gc->of_node = gdev->dev.of_node;
>  #endif
>  
> -	gdev->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL);
> +	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
>  	if (gdev->id < 0) {
>  		ret = gdev->id;
>  		goto err_free_gdev;
> @@ -753,7 +753,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  err_free_descs:
>  	kfree(gdev->descs);
>  err_free_ida:
> -	ida_simple_remove(&gpio_ida, gdev->id);
> +	ida_free(&gpio_ida, gdev->id);
>  err_free_gdev:
>  	/* failures here can mean systems won't boot... */
>  	pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 14/23] gpio: mockup: use the generic 'gpio-line-names' property

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:38PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> GPIO line names are currently created by the driver from the chip label.
> We'll want to support custom formats for line names (for instance: to
> name all lines the same) for user-space tests so create them in the
> module init function and pass them to the driver using the standard
> 'gpio-line-names' property.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-mockup.c | 70 +++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index ce83f1df1933..96976ba66598 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -18,6 +18,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
> +#include <linux/string_helpers.h>
>  #include <linux/uaccess.h>
>  
>  #include "gpiolib.h"
> @@ -378,29 +379,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
>  	return;
>  }
>  
> -static int gpio_mockup_name_lines(struct device *dev,
> -				  struct gpio_mockup_chip *chip)
> -{
> -	struct gpio_chip *gc = &chip->gc;
> -	char **names;
> -	int i;
> -
> -	names = devm_kcalloc(dev, gc->ngpio, sizeof(char *), GFP_KERNEL);
> -	if (!names)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < gc->ngpio; i++) {
> -		names[i] = devm_kasprintf(dev, GFP_KERNEL,
> -					  "%s-%d", gc->label, i);
> -		if (!names[i])
> -			return -ENOMEM;
> -	}
> -
> -	gc->names = (const char *const *)names;
> -
> -	return 0;
> -}
> -
>  static void gpio_mockup_dispose_mappings(void *data)
>  {
>  	struct gpio_mockup_chip *chip = data;
> @@ -468,12 +446,6 @@ static int gpio_mockup_probe(struct platform_device *pdev)
>  	for (i = 0; i < gc->ngpio; i++)
>  		chip->lines[i].dir = GPIO_LINE_DIRECTION_IN;
>  
> -	if (device_property_read_bool(dev, "named-gpio-lines")) {
> -		rv = gpio_mockup_name_lines(dev, chip);
> -		if (rv)
> -			return rv;
> -	}
> -
>  	chip->irq_sim_domain = devm_irq_domain_create_sim(dev, NULL,
>  							  gc->ngpio);
>  	if (IS_ERR(chip->irq_sim_domain))
> @@ -524,6 +496,27 @@ static void gpio_mockup_unregister_devices(void)
>  	}
>  }
>  
> +static __init char **gpio_mockup_make_line_names(const char *label,
> +						 unsigned int num_lines)
> +{
> +	unsigned int i;
> +	char **names;
> +
> +	names = kcalloc(num_lines + 1, sizeof(char *), GFP_KERNEL);
> +	if (!names)
> +		return NULL;
> +
> +	for (i = 0; i < num_lines; i++) {
> +		names[i] = kasprintf(GFP_KERNEL, "%s-%u", label, i);
> +		if (!names[i]) {
> +			kfree_strarray(names, i);
> +			return NULL;
> +		}
> +	}
> +
> +	return names;
> +}
> +
>  static int __init gpio_mockup_init(void)
>  {
>  	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
> @@ -531,6 +524,7 @@ static int __init gpio_mockup_init(void)
>  	struct gpio_mockup_device *mockup_dev;
>  	int i, prop, num_chips, err = 0, base;
>  	struct platform_device_info pdevinfo;
> +	char **line_names;
>  	u16 ngpio;
>  
>  	if ((gpio_mockup_num_ranges < 2) ||
> @@ -563,6 +557,7 @@ static int __init gpio_mockup_init(void)
>  		memset(properties, 0, sizeof(properties));
>  		memset(&pdevinfo, 0, sizeof(pdevinfo));
>  		prop = 0;
> +		line_names = NULL;
>  
>  		snprintf(chip_label, sizeof(chip_label),
>  			 "gpio-mockup-%c", i + 'A');
> @@ -578,9 +573,18 @@ static int __init gpio_mockup_init(void)
>  				 : gpio_mockup_range_ngpio(i) - base;
>  		properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio);
>  
> -		if (gpio_mockup_named_lines)
> -			properties[prop++] = PROPERTY_ENTRY_BOOL(
> -						"named-gpio-lines");
> +		if (gpio_mockup_named_lines) {
> +			line_names = gpio_mockup_make_line_names(chip_label,
> +								 ngpio);
> +			if (!line_names) {
> +				err = -ENOMEM;
> +				goto err_out;
> +			}
> +
> +			properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
> +						"gpio-line-names",
> +						line_names, ngpio);
> +		}

Indentation here looks quite deep. Maybe introduce a helper in between where
you assign properties?

>  		pdevinfo.name = "gpio-mockup";
>  		pdevinfo.id = i;
> @@ -588,11 +592,13 @@ static int __init gpio_mockup_init(void)
>  
>  		mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL);
>  		if (!mockup_dev) {
> +			kfree_strarray(line_names, ngpio);
>  			err = -ENOMEM;
>  			goto err_out;
>  		}
>  
>  		mockup_dev->pdev = platform_device_register_full(&pdevinfo);
> +		kfree_strarray(line_names, ngpio);
>  		if (IS_ERR(mockup_dev->pdev)) {
>  			pr_err("error registering device");
>  			kfree(mockup_dev);
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 13/23] gpio: mockup: pass the chip label as device property

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:37PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> While we do check the "chip-name" property in probe(), we never actually
> use it. Let's pass the chip label to the driver using device properties
> as we'll want to allow users to define their own once dynamically
> created chips are supported.
> 
> The property is renamed to "chip-label" to not cause any confusion with
> the actual chip name which is of the form: "gpiochipX".
> 
> If the "chip-label" property is missing, let's do what most devices in
> drivers/gpio/ do and use dev_name().

Just wondering if we have a documentation in kernel how this mockup mechanism
works and what kind of properties it uses.

Side note: moving to software nodes would make some advantages in future such
as visibility properties and their values (not yet implemented, but there is an
idea to move forward).

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-mockup.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index e8a19a28ed13..ce83f1df1933 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -433,21 +433,14 @@ static int gpio_mockup_probe(struct platform_device *pdev)
>  	if (rv)
>  		return rv;
>  
> -	rv = device_property_read_string(dev, "chip-name", &name);
> +	rv = device_property_read_string(dev, "chip-label", &name);
>  	if (rv)
> -		name = NULL;
> +		name = dev_name(dev);
>  
>  	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
>  	if (!chip)
>  		return -ENOMEM;
>  
> -	if (!name) {
> -		name = devm_kasprintf(dev, GFP_KERNEL,
> -				      "%s-%c", pdev->name, pdev->id + 'A');
> -		if (!name)
> -			return -ENOMEM;
> -	}
> -
>  	mutex_init(&chip->lock);
>  
>  	gc = &chip->gc;
> @@ -534,6 +527,7 @@ static void gpio_mockup_unregister_devices(void)
>  static int __init gpio_mockup_init(void)
>  {
>  	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
> +	char chip_label[GPIO_MOCKUP_LABEL_SIZE];
>  	struct gpio_mockup_device *mockup_dev;
>  	int i, prop, num_chips, err = 0, base;
>  	struct platform_device_info pdevinfo;
> @@ -570,6 +564,11 @@ static int __init gpio_mockup_init(void)
>  		memset(&pdevinfo, 0, sizeof(pdevinfo));
>  		prop = 0;
>  
> +		snprintf(chip_label, sizeof(chip_label),
> +			 "gpio-mockup-%c", i + 'A');
> +		properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
> +							   chip_label);
> +
>  		base = gpio_mockup_range_base(i);
>  		if (base >= 0)
>  			properties[prop++] = PROPERTY_ENTRY_U32("gpio-base",
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 15/23] gpio: mockup: use dynamic device IDs

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:39PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We're currently creating chips at module init time only so using a
> static index for dummy devices is fine. We want to support dynamically
> created chips however so we need to switch to dynamic device IDs.

It misses ida_destroy().

What about XArray API?

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-mockup.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index 96976ba66598..995e37fef9c5 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/debugfs.h>
>  #include <linux/gpio/driver.h>
> +#include <linux/idr.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irq_sim.h>
> @@ -70,6 +71,8 @@ module_param_named(gpio_mockup_named_lines,
>  
>  static struct dentry *gpio_mockup_dbg_dir;
>  
> +static DEFINE_IDA(gpio_mockup_ida);
> +
>  static int gpio_mockup_range_base(unsigned int index)
>  {
>  	return gpio_mockup_ranges[index * 2];
> @@ -480,8 +483,12 @@ static LIST_HEAD(gpio_mockup_devices);
>  
>  static void gpio_mockup_unregister_one_device(struct gpio_mockup_device *dev)
>  {
> +	int id;
> +
>  	list_del(&dev->list);
> +	id = dev->pdev->id;
>  	platform_device_unregister(dev->pdev);
> +	ida_free(&gpio_mockup_ida, id);
>  	kfree(dev);
>  }
>  
> @@ -587,12 +594,19 @@ static int __init gpio_mockup_init(void)
>  		}
>  
>  		pdevinfo.name = "gpio-mockup";
> -		pdevinfo.id = i;
>  		pdevinfo.properties = properties;
>  
> +		pdevinfo.id = ida_alloc(&gpio_mockup_ida, GFP_KERNEL);
> +		if (pdevinfo.id < 0) {
> +			kfree_strarray(line_names, ngpio);
> +			err = pdevinfo.id;
> +			goto err_out;
> +		}
> +
>  		mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL);
>  		if (!mockup_dev) {
>  			kfree_strarray(line_names, ngpio);
> +			ida_free(&gpio_mockup_ida, pdevinfo.id);
>  			err = -ENOMEM;
>  			goto err_out;
>  		}
> @@ -601,6 +615,7 @@ static int __init gpio_mockup_init(void)
>  		kfree_strarray(line_names, ngpio);
>  		if (IS_ERR(mockup_dev->pdev)) {
>  			pr_err("error registering device");
> +			ida_free(&gpio_mockup_ida, pdevinfo.id);
>  			kfree(mockup_dev);
>  			err = PTR_ERR(mockup_dev->pdev);
>  			goto err_out;
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 16/23] gpio: mockup: refactor the module init function

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:40PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> This is in preparation for dynamically created chips.
> 
> Let's split out the code that can be reused when creating chips at
> run-time. Let's also move the code preparing the device properties into
> a separate routine. This has the advantage of simplifying the error
> handling.

Almost all contents of this patch should go to proposed helper as I mentioned
before. Will make this patch quite small and understandable.

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-mockup.c | 165 ++++++++++++++++++++-----------------
>  1 file changed, 90 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index 995e37fef9c5..eb94ddac5fee 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -524,16 +524,78 @@ static __init char **gpio_mockup_make_line_names(const char *label,
>  	return names;
>  }
>  
> -static int __init gpio_mockup_init(void)
> +static int __init gpio_mockup_register_device(struct property_entry *properties)
>  {
> -	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
> -	char chip_label[GPIO_MOCKUP_LABEL_SIZE];
>  	struct gpio_mockup_device *mockup_dev;
> -	int i, prop, num_chips, err = 0, base;
>  	struct platform_device_info pdevinfo;
> -	char **line_names;
> +
> +	memset(&pdevinfo, 0, sizeof(pdevinfo));
> +
> +	mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL);
> +	if (!mockup_dev)
> +		return -ENOMEM;
> +
> +	pdevinfo.name = "gpio-mockup";
> +	pdevinfo.properties = properties;
> +
> +	pdevinfo.id = ida_alloc(&gpio_mockup_ida, GFP_KERNEL);
> +	if (pdevinfo.id < 0) {
> +		kfree(mockup_dev);
> +		return pdevinfo.id;
> +	}
> +
> +	mockup_dev->pdev = platform_device_register_full(&pdevinfo);
> +	if (IS_ERR(mockup_dev->pdev)) {
> +		ida_free(&gpio_mockup_ida, pdevinfo.id);
> +		kfree(mockup_dev);
> +		return PTR_ERR(mockup_dev->pdev);
> +	}
> +
> +	list_add(&mockup_dev->list, &gpio_mockup_devices);
> +
> +	return 0;
> +}
> +
> +static int __init gpio_mockup_register_one_chip_from_params(int idx)
> +{
> +	char chip_label[GPIO_MOCKUP_LABEL_SIZE], **line_names = NULL;
> +	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
> +	int prop = 0, base, ret;
>  	u16 ngpio;
>  
> +	memset(properties, 0, sizeof(properties));
> +
> +	snprintf(chip_label, sizeof(chip_label), "gpio-mockup-%c", idx + 'A');
> +	properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
> +						   chip_label);
> +
> +	base = gpio_mockup_range_base(idx);
> +	if (base >= 0)
> +		properties[prop++] = PROPERTY_ENTRY_U32("gpio-base",
> +							base);
> +
> +	ngpio = base < 0 ? gpio_mockup_range_ngpio(idx)
> +			 : gpio_mockup_range_ngpio(idx) - base;
> +	properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio);
> +
> +	if (gpio_mockup_named_lines) {
> +		line_names = gpio_mockup_make_line_names(chip_label, ngpio);
> +		if (!line_names)
> +			return -ENOMEM;
> +
> +		properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
> +					"gpio-line-names", line_names, ngpio);
> +	}
> +
> +	ret = gpio_mockup_register_device(properties);
> +	kfree_strarray(line_names, ngpio);
> +	return ret;
> +}
> +
> +static int __init gpio_mockup_register_chips_from_params(void)
> +{
> +	int num_chips, i, ret;
> +
>  	if ((gpio_mockup_num_ranges < 2) ||
>  	    (gpio_mockup_num_ranges % 2) ||
>  	    (gpio_mockup_num_ranges > GPIO_MOCKUP_MAX_RANGES))
> @@ -551,86 +613,39 @@ static int __init gpio_mockup_init(void)
>  			return -EINVAL;
>  	}
>  
> -	gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL);
> -
> -	err = platform_driver_register(&gpio_mockup_driver);
> -	if (err) {
> -		pr_err("error registering platform driver\n");
> -		debugfs_remove_recursive(gpio_mockup_dbg_dir);
> -		return err;
> -	}
> -
>  	for (i = 0; i < num_chips; i++) {
> -		memset(properties, 0, sizeof(properties));
> -		memset(&pdevinfo, 0, sizeof(pdevinfo));
> -		prop = 0;
> -		line_names = NULL;
> -
> -		snprintf(chip_label, sizeof(chip_label),
> -			 "gpio-mockup-%c", i + 'A');
> -		properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
> -							   chip_label);
> -
> -		base = gpio_mockup_range_base(i);
> -		if (base >= 0)
> -			properties[prop++] = PROPERTY_ENTRY_U32("gpio-base",
> -								base);
> -
> -		ngpio = base < 0 ? gpio_mockup_range_ngpio(i)
> -				 : gpio_mockup_range_ngpio(i) - base;
> -		properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio);
> -
> -		if (gpio_mockup_named_lines) {
> -			line_names = gpio_mockup_make_line_names(chip_label,
> -								 ngpio);
> -			if (!line_names) {
> -				err = -ENOMEM;
> -				goto err_out;
> -			}
> -
> -			properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
> -						"gpio-line-names",
> -						line_names, ngpio);
> +		ret = gpio_mockup_register_one_chip_from_params(i);
> +		if (ret) {
> +			gpio_mockup_unregister_devices();
> +			return ret;
>  		}
> +	}
>  
> -		pdevinfo.name = "gpio-mockup";
> -		pdevinfo.properties = properties;
> +	return 0;
> +}
>  
> -		pdevinfo.id = ida_alloc(&gpio_mockup_ida, GFP_KERNEL);
> -		if (pdevinfo.id < 0) {
> -			kfree_strarray(line_names, ngpio);
> -			err = pdevinfo.id;
> -			goto err_out;
> -		}
> +static int __init gpio_mockup_init(void)
> +{
> +	int ret;
>  
> -		mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL);
> -		if (!mockup_dev) {
> -			kfree_strarray(line_names, ngpio);
> -			ida_free(&gpio_mockup_ida, pdevinfo.id);
> -			err = -ENOMEM;
> -			goto err_out;
> -		}
> +	gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL);
>  
> -		mockup_dev->pdev = platform_device_register_full(&pdevinfo);
> -		kfree_strarray(line_names, ngpio);
> -		if (IS_ERR(mockup_dev->pdev)) {
> -			pr_err("error registering device");
> -			ida_free(&gpio_mockup_ida, pdevinfo.id);
> -			kfree(mockup_dev);
> -			err = PTR_ERR(mockup_dev->pdev);
> -			goto err_out;
> -		}
> +	ret = platform_driver_register(&gpio_mockup_driver);
> +	if (ret) {
> +		pr_err("error registering platform driver\n");
> +		debugfs_remove_recursive(gpio_mockup_dbg_dir);
> +		return ret;
> +	}
>  
> -		list_add(&mockup_dev->list, &gpio_mockup_devices);
> +	ret = gpio_mockup_register_chips_from_params();
> +	if (ret) {
> +		pr_err("error registering device");
> +		debugfs_remove_recursive(gpio_mockup_dbg_dir);
> +		platform_driver_unregister(&gpio_mockup_driver);
> +		return ret;
>  	}
>  
>  	return 0;
> -
> -err_out:
> -	platform_driver_unregister(&gpio_mockup_driver);
> -	gpio_mockup_unregister_devices();
> -	debugfs_remove_recursive(gpio_mockup_dbg_dir);
> -	return err;
>  }
>  
>  static void __exit gpio_mockup_exit(void)
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 21/23] gpio: mockup: provide a way to delete dummy chips

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:45PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Add a new debugfs attribute 'delete_device' to which the chip label can
> be written to dynamically remove the associated dummy device.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-mockup.c | 70 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index 1353239dc315..9d2de78a45c2 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -19,6 +19,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
> +#include <linux/string.h>
>  #include <linux/string_helpers.h>
>  #include <linux/uaccess.h>
>  
> @@ -668,14 +669,81 @@ static int __init gpio_mockup_register_chips_from_params(void)
>  	return 0;
>  }
>  
> -static int __init gpio_mockup_init(void)
> +static ssize_t gpio_mockup_debugfs_delete_device_write(struct file *file,
> +						const char __user *usr_buf,
> +						size_t size, loff_t *ppos)
>  {
> +	struct gpio_mockup_device *mockup_dev;
> +	char label[GPIO_MOCKUP_LABEL_SIZE];
> +	struct list_head *curr;
> +	struct device *dev;
> +	const char *prop;
>  	int ret;
>  
> +	if (*ppos != 0)
> +		return -EINVAL;
> +
> +	ret = getline_from_user(label, sizeof(label), usr_buf, size);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&gpio_mockup_devices_lock);
> +
> +	list_for_each(curr, &gpio_mockup_devices) {
> +		mockup_dev = list_entry(curr, struct gpio_mockup_device, list);

Why not list_for_each_entry()?

> +		dev = &mockup_dev->pdev->dev;
> +
> +		ret = device_property_read_string(dev, "chip-label", &prop);
> +		if (ret) {
> +			mutex_unlock(&gpio_mockup_devices_lock);
> +			return ret;
> +		}
> +
> +		if (sysfs_streq(label, prop)) {
> +			gpio_mockup_unregister_one_device(mockup_dev);
> +			mutex_unlock(&gpio_mockup_devices_lock);
> +			return size;
> +		}
> +	}
> +
> +	mutex_unlock(&gpio_mockup_devices_lock);
> +	return -ENODEV;
> +}
> +
> +static const struct file_operations gpio_mockup_debugfs_delete_device_ops = {
> +	.owner = THIS_MODULE,
> +	.open = gpio_mockup_debugfs_open,
> +	.write = gpio_mockup_debugfs_delete_device_write,
> +	.llseek = no_llseek,
> +	.release = single_release,
> +};
> +
> +static int __init gpio_mockup_debugfs_init(void)
> +{
> +	struct dentry *entry;
> +
>  	gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL);
>  	if (IS_ERR(gpio_mockup_dbg_dir))
>  		return PTR_ERR(gpio_mockup_dbg_dir);
>  
> +	entry = debugfs_create_file("delete_device", 0200, gpio_mockup_dbg_dir,
> +				NULL, &gpio_mockup_debugfs_delete_device_ops);
> +	if (IS_ERR(entry)) {
> +		debugfs_remove_recursive(gpio_mockup_dbg_dir);
> +		return PTR_ERR(entry);
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init gpio_mockup_init(void)
> +{
> +	int ret;
> +
> +	ret = gpio_mockup_debugfs_init();
> +	if (ret)
> +		return ret;
> +
>  	ret = platform_driver_register(&gpio_mockup_driver);
>  	if (ret) {
>  		pr_err("error registering platform driver\n");
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:47PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> There's some documentation for gpio-mockup's debugfs interface in the
> driver's source but it's not much. Add proper documentation for this
> testing module.

I always for the documentation!

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  .../admin-guide/gpio/gpio-mockup.rst          | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/admin-guide/gpio/gpio-mockup.rst
> 
> diff --git a/Documentation/admin-guide/gpio/gpio-mockup.rst b/Documentation/admin-guide/gpio/gpio-mockup.rst
> new file mode 100644
> index 000000000000..1d452ee55f8d
> --- /dev/null
> +++ b/Documentation/admin-guide/gpio/gpio-mockup.rst
> @@ -0,0 +1,87 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +GPIO Testing Driver
> +===================
> +
> +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> +chips for testing purposes. There are two ways of configuring the chips exposed
> +by the module. The lines can be accessed using the standard GPIO character
> +device interface as well as manipulated using the dedicated debugfs directory
> +structure.
> +
> +Creating simulated chips using debugfs
> +--------------------------------------
> +
> +When the gpio-mockup module is loaded (or builtin) it creates its own directory
> +in debugfs. Assuming debugfs is mounted at /sys/kernel/debug/, the directory
> +will be located at /sys/kernel/debug/gpio-mockup/. Inside this directory there
> +are two attributes: new_device and delete_device.
> +
> +New chips can be created by writing a single line containing a number of
> +options to "new_device". For example:
> +
> +.. code-block:: sh
> +
> +    $ echo "label=my-mockup num_lines=4 named_lines" > /sys/kernel/debug/gpio-mockup/new_device
> +
> +Supported options:
> +
> +    num_lines=<num_lines> - number of GPIO lines to expose
> +
> +    label=<label> - label of the dummy chip
> +
> +    named_lines - defines whether dummy lines should be named, the names are
> +                  of the form X-Y where X is the chip's label and Y is the
> +                  line's offset
> +
> +Note: only num_lines is mandatory.
> +
> +Chips can be dynamically removed by writing the chip's label to
> +"delete_device". For example:
> +
> +.. code-block:: sh
> +
> +    echo "gpio-mockup.0" > /sys/kernel/debug/gpio-mockup/delete_device
> +
> +Creating simulated chips using module params
> +--------------------------------------------
> +
> +Note: this is an older, now deprecated method kept for backward compatibility
> +for user-space tools.
> +
> +When loading the gpio-mockup driver a number of parameters can be passed to the
> +module.
> +
> +    gpio_mockup_ranges
> +
> +        This parameter takes an argument in the form of an array of integer
> +        pairs. Each pair defines the base GPIO number (if any) and the number
> +        of lines exposed by the chip. If the base GPIO is -1, the gpiolib
> +        will assign it automatically.
> +
> +        Example: gpio_mockup_ranges=-1,8,-1,16,405,4
> +
> +        The line above creates three chips. The first one will expose 8 lines,
> +        the second 16 and the third 4. The base GPIO for the third chip is set
> +        to 405 while for two first chips it will be assigned automatically.
> +
> +    gpio_named_lines
> +
> +        This parameter doesn't take any arguments. It lets the driver know that
> +        GPIO lines exposed by it should be named.
> +
> +        The name format is: gpio-mockup-X-Y where X is the letter associated
> +        with the mockup chip and Y is the line offset.
> +
> +Manipulating simulated lines
> +----------------------------
> +
> +Each mockup chip creates its own subdirectory in /sys/kernel/debug/gpio-mockup/.
> +The directory is named after the chip's label. A symlink is also created, named
> +after the chip's name, which points to the label directory.
> +
> +Inside each subdirectory, there's a separate attribute for each GPIO line. The
> +name of the attribute represents the line's offset in the chip.
> +
> +Reading from a line attribute returns the current value. Writing to it (0 or 1)
> +changes its pull.
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 10/23] gpio: mockup: fix resource leak in error path

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 05:45:34PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> If the module init function fails after creating the debugs directory,
> it's never removed. Add proper cleanup calls to avoid this resource
> leak.

Does it fix existing bug?

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-mockup.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index 78c97f7b6893..19c092f814fd 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -550,6 +550,7 @@ static int __init gpio_mockup_init(void)
>  	err = platform_driver_register(&gpio_mockup_driver);
>  	if (err) {
>  		pr_err("error registering platform driver\n");
> +		debugfs_remove_recursive(gpio_mockup_dbg_dir);
>  		return err;
>  	}
>  
> @@ -580,6 +581,7 @@ static int __init gpio_mockup_init(void)
>  			pr_err("error registering device");
>  			platform_driver_unregister(&gpio_mockup_driver);
>  			gpio_mockup_unregister_pdevs();
> +			debugfs_remove_recursive(gpio_mockup_dbg_dir);
>  			return PTR_ERR(pdev);
>  		}
>  
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Randy Dunlap]

Hi,

On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> There's some documentation for gpio-mockup's debugfs interface in the
> driver's source but it's not much. Add proper documentation for this
> testing module.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  .../admin-guide/gpio/gpio-mockup.rst          | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/admin-guide/gpio/gpio-mockup.rst
> 
> diff --git a/Documentation/admin-guide/gpio/gpio-mockup.rst b/Documentation/admin-guide/gpio/gpio-mockup.rst
> new file mode 100644
> index 000000000000..1d452ee55f8d
> --- /dev/null
> +++ b/Documentation/admin-guide/gpio/gpio-mockup.rst
> @@ -0,0 +1,87 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +GPIO Testing Driver
> +===================
> +
> +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> +chips for testing purposes. There are two ways of configuring the chips exposed
> +by the module. The lines can be accessed using the standard GPIO character
> +device interface as well as manipulated using the dedicated debugfs directory
> +structure.

Could configfs be used for this instead of debugfs?
debugfs is ad hoc.

> +
> +Creating simulated chips using debugfs
> +--------------------------------------
> +
> +When the gpio-mockup module is loaded (or builtin) it creates its own directory
> +in debugfs. Assuming debugfs is mounted at /sys/kernel/debug/, the directory
> +will be located at /sys/kernel/debug/gpio-mockup/. Inside this directory there
> +are two attributes: new_device and delete_device.
> +
> +New chips can be created by writing a single line containing a number of
> +options to "new_device". For example:
> +
> +.. code-block:: sh
> +
> +    $ echo "label=my-mockup num_lines=4 named_lines" > /sys/kernel/debug/gpio-mockup/new_device
> +
> +Supported options:
> +
> +    num_lines=<num_lines> - number of GPIO lines to expose
> +
> +    label=<label> - label of the dummy chip
> +
> +    named_lines - defines whether dummy lines should be named, the names are
> +                  of the form X-Y where X is the chip's label and Y is the
> +                  line's offset
> +
> +Note: only num_lines is mandatory.
> +
> +Chips can be dynamically removed by writing the chip's label to
> +"delete_device". For example:
> +
> +.. code-block:: sh
> +
> +    echo "gpio-mockup.0" > /sys/kernel/debug/gpio-mockup/delete_device
> +
> +Creating simulated chips using module params
> +--------------------------------------------
> +
> +Note: this is an older, now deprecated method kept for backward compatibility
> +for user-space tools.
> +
> +When loading the gpio-mockup driver a number of parameters can be passed to the
> +module.
> +
> +    gpio_mockup_ranges
> +
> +        This parameter takes an argument in the form of an array of integer
> +        pairs. Each pair defines the base GPIO number (if any) and the number
> +        of lines exposed by the chip. If the base GPIO is -1, the gpiolib
> +        will assign it automatically.
> +
> +        Example: gpio_mockup_ranges=-1,8,-1,16,405,4
> +
> +        The line above creates three chips. The first one will expose 8 lines,
> +        the second 16 and the third 4. The base GPIO for the third chip is set
> +        to 405 while for two first chips it will be assigned automatically.
> +
> +    gpio_named_lines
> +
> +        This parameter doesn't take any arguments. It lets the driver know that
> +        GPIO lines exposed by it should be named.
> +
> +        The name format is: gpio-mockup-X-Y where X is the letter associated
> +        with the mockup chip and Y is the line offset.

Where does this 'X' letter associated with the mockup chip come from?

> +
> +Manipulating simulated lines
> +----------------------------
> +
> +Each mockup chip creates its own subdirectory in /sys/kernel/debug/gpio-mockup/.
> +The directory is named after the chip's label. A symlink is also created, named
> +after the chip's name, which points to the label directory.
> +
> +Inside each subdirectory, there's a separate attribute for each GPIO line. The
> +name of the attribute represents the line's offset in the chip.
> +
> +Reading from a line attribute returns the current value. Writing to it (0 or 1)
> +changes its pull.

What does "pull" mean here?


thanks.

-- 
~Randy

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Andy Shevchenko]

On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:

...

> > +GPIO Testing Driver
> > +===================
> > +
> > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > +chips for testing purposes. There are two ways of configuring the chips exposed
> > +by the module. The lines can be accessed using the standard GPIO character
> > +device interface as well as manipulated using the dedicated debugfs directory
> > +structure.
> 
> Could configfs be used for this instead of debugfs?
> debugfs is ad hoc.

Actually sounds like a good idea.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/23] lib: uaccess: provide getline_from_user()

[Bartosz Golaszewski]

On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Provide a uaccess helper that allows callers to copy a single line from
> > user memory. This is useful for debugfs write callbacks.
>
> Doesn't mm/util.c provides us something like this?
> strndup_user()?
>

Yes, there's both strndup_user() as well as strncpy_from_user(). The
problem is that they rely on the strings being NULL-terminated. This
is not guaranteed for debugfs file_operations write callbacks. We need
some helper that takes the minimum of bytes provided by userspace and
the buffer size and figure out how many bytes to actually copy IMO.

Bart

Re: [PATCH 03/23] lib: uaccess: provide getline_from_user()

[Andy Shevchenko]

On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > Doesn't mm/util.c provides us something like this?
> > strndup_user()?
> >
>
> Yes, there's both strndup_user() as well as strncpy_from_user(). The
> problem is that they rely on the strings being NULL-terminated. This
> is not guaranteed for debugfs file_operations write callbacks. We need
> some helper that takes the minimum of bytes provided by userspace and
> the buffer size and figure out how many bytes to actually copy IMO.

Wouldn't this [1] approach work?

[1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Bartosz Golaszewski]

On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
>
> ...
>
> > > +GPIO Testing Driver
> > > +===================
> > > +
> > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > +by the module. The lines can be accessed using the standard GPIO character
> > > +device interface as well as manipulated using the dedicated debugfs directory
> > > +structure.
> >
> > Could configfs be used for this instead of debugfs?
> > debugfs is ad hoc.
>
> Actually sounds like a good idea.
>

Well, then we can go on and write an entirely new mockup driver
(ditching module params and dropping any backwards compatibility)
because we're already using debugfs for line values.

How would we pass the device properties to configfs created GPIO chips
anyway? Devices seem to only be created using mkdir. Am I missing
something?

Bart

Re: [PATCH 03/23] lib: uaccess: provide getline_from_user()

[Bartosz Golaszewski]

On Mon, Sep 7, 2020 at 12:19 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> > On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> > > Doesn't mm/util.c provides us something like this?
> > > strndup_user()?
> > >
> >
> > Yes, there's both strndup_user() as well as strncpy_from_user(). The
> > problem is that they rely on the strings being NULL-terminated. This
> > is not guaranteed for debugfs file_operations write callbacks. We need
> > some helper that takes the minimum of bytes provided by userspace and
> > the buffer size and figure out how many bytes to actually copy IMO.
>
> Wouldn't this [1] approach work?
>
> [1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93
>

Sure, but this is pretty much what I do in getline_from_user(). If
anything we should port mtrr_write() to using getline_from_user() once
it's available upstream, no?

Bart

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Bartosz Golaszewski]

On Sat, Sep 5, 2020 at 5:16 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> Hi,
>
> On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > There's some documentation for gpio-mockup's debugfs interface in the
> > driver's source but it's not much. Add proper documentation for this
> > testing module.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >  .../admin-guide/gpio/gpio-mockup.rst          | 87 +++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >  create mode 100644 Documentation/admin-guide/gpio/gpio-mockup.rst
> >
> > diff --git a/Documentation/admin-guide/gpio/gpio-mockup.rst b/Documentation/admin-guide/gpio/gpio-mockup.rst
> > new file mode 100644
> > index 000000000000..1d452ee55f8d
> > --- /dev/null
> > +++ b/Documentation/admin-guide/gpio/gpio-mockup.rst
> > @@ -0,0 +1,87 @@
> > +.. SPDX-License-Identifier: GPL-2.0-only
> > +
> > +GPIO Testing Driver
> > +===================
> > +
> > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > +chips for testing purposes. There are two ways of configuring the chips exposed
> > +by the module. The lines can be accessed using the standard GPIO character
> > +device interface as well as manipulated using the dedicated debugfs directory
> > +structure.
>
> Could configfs be used for this instead of debugfs?
> debugfs is ad hoc.
>
> > +
> > +Creating simulated chips using debugfs
> > +--------------------------------------
> > +
> > +When the gpio-mockup module is loaded (or builtin) it creates its own directory
> > +in debugfs. Assuming debugfs is mounted at /sys/kernel/debug/, the directory
> > +will be located at /sys/kernel/debug/gpio-mockup/. Inside this directory there
> > +are two attributes: new_device and delete_device.
> > +
> > +New chips can be created by writing a single line containing a number of
> > +options to "new_device". For example:
> > +
> > +.. code-block:: sh
> > +
> > +    $ echo "label=my-mockup num_lines=4 named_lines" > /sys/kernel/debug/gpio-mockup/new_device
> > +
> > +Supported options:
> > +
> > +    num_lines=<num_lines> - number of GPIO lines to expose
> > +
> > +    label=<label> - label of the dummy chip
> > +
> > +    named_lines - defines whether dummy lines should be named, the names are
> > +                  of the form X-Y where X is the chip's label and Y is the
> > +                  line's offset
> > +
> > +Note: only num_lines is mandatory.
> > +
> > +Chips can be dynamically removed by writing the chip's label to
> > +"delete_device". For example:
> > +
> > +.. code-block:: sh
> > +
> > +    echo "gpio-mockup.0" > /sys/kernel/debug/gpio-mockup/delete_device
> > +
> > +Creating simulated chips using module params
> > +--------------------------------------------
> > +
> > +Note: this is an older, now deprecated method kept for backward compatibility
> > +for user-space tools.
> > +
> > +When loading the gpio-mockup driver a number of parameters can be passed to the
> > +module.
> > +
> > +    gpio_mockup_ranges
> > +
> > +        This parameter takes an argument in the form of an array of integer
> > +        pairs. Each pair defines the base GPIO number (if any) and the number
> > +        of lines exposed by the chip. If the base GPIO is -1, the gpiolib
> > +        will assign it automatically.
> > +
> > +        Example: gpio_mockup_ranges=-1,8,-1,16,405,4
> > +
> > +        The line above creates three chips. The first one will expose 8 lines,
> > +        the second 16 and the third 4. The base GPIO for the third chip is set
> > +        to 405 while for two first chips it will be assigned automatically.
> > +
> > +    gpio_named_lines
> > +
> > +        This parameter doesn't take any arguments. It lets the driver know that
> > +        GPIO lines exposed by it should be named.
> > +
> > +        The name format is: gpio-mockup-X-Y where X is the letter associated
> > +        with the mockup chip and Y is the line offset.
>
> Where does this 'X' letter associated with the mockup chip come from?
>
> > +
> > +Manipulating simulated lines
> > +----------------------------
> > +
> > +Each mockup chip creates its own subdirectory in /sys/kernel/debug/gpio-mockup/.
> > +The directory is named after the chip's label. A symlink is also created, named
> > +after the chip's name, which points to the label directory.
> > +
> > +Inside each subdirectory, there's a separate attribute for each GPIO line. The
> > +name of the attribute represents the line's offset in the chip.
> > +
> > +Reading from a line attribute returns the current value. Writing to it (0 or 1)
> > +changes its pull.
>
> What does "pull" mean here?
>

Yeah I should probably clarify this. "Pull" here means a simulated
pull-down/up resistor basically.

Bart

Re: [PATCH 06/23] gpiolib: switch to simpler IDA interface

[Bartosz Golaszewski]

On Fri, Sep 4, 2020 at 6:41 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 05:45:30PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > We don't need to specify any ranges when allocating IDs so we can switch
> > to ida_alloc() and ida_free() instead of the ida_simple_ counterparts.
> >
> > ida_simple_get(ida, 0, 0, gfp) is equivalent to
> > ida_alloc_range(ida, 0, UINT_MAX, gfp) which is equivalent to
> > ida_alloc(ida, gfp). Note: IDR will never actually allocate an ID
> > larger than INT_MAX.
>
> Have you considered switching to XArray API?
>

IDA uses an xarray internally but wraps it in a more straightforward
API. No need to use the low-level API IMO.

Bart

Re: [PATCH 05/23] gpiolib: unexport devprop_gpiochip_set_names()

[Bartosz Golaszewski]

On Fri, Sep 4, 2020 at 6:42 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 05:45:29PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Now that devprop_gpiochip_set_names() is only used in a single place
> > inside drivers/gpio/gpiolib.c, there's no need anymore for it to be
> > exported or to even live in its own source file. Pull this function into
> > the core source file for gpiolib.
>
> I have mixed feelings about this. We may simply unexport and attach object file
> to gpiolib.o. Would it be expected to see more functions in this file in the
> future?
>

This file was created for this function alone over 4 years ago and
never saw any new routines. Personally I think there's no reason to
keep a single helper in its own file. We usually only split big chunks
of code out of gpiolib.c.

Bart

Re: [PATCH 04/23] gpiolib: generalize devprop_gpiochip_set_names() for device properties

[Bartosz Golaszewski]

On Fri, Sep 4, 2020 at 6:44 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 05:45:28PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > devprop_gpiochip_set_names() is overly complicated with taking the
> > fwnode argument (which requires using dev_fwnode() & of_fwnode_handle()
> > in ACPI and OF GPIO code respectively). Let's just switch to using the
> > generic device properties.
> >
> > This allows us to pull the code setting line names directly into
> > gpiochip_add_data_with_key() instead of handling it separately for
> > ACPI and OF.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >  drivers/gpio/gpiolib-acpi.c    |  3 ---
> >  drivers/gpio/gpiolib-devprop.c | 19 ++++++++++---------
> >  drivers/gpio/gpiolib-of.c      |  5 -----
> >  drivers/gpio/gpiolib.c         |  8 ++++----
> >  include/linux/gpio/driver.h    |  3 +--
> >  5 files changed, 15 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > index 54ca3c18b291..834a12f3219e 100644
> > --- a/drivers/gpio/gpiolib-acpi.c
> > +++ b/drivers/gpio/gpiolib-acpi.c
> > @@ -1221,9 +1221,6 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
> >               return;
> >       }
> >
> > -     if (!chip->names)
> > -             devprop_gpiochip_set_names(chip, dev_fwnode(chip->parent));
> > -
> >       acpi_gpiochip_request_regions(acpi_gpio);
> >       acpi_gpiochip_scan_gpios(acpi_gpio);
> >       acpi_walk_dep_device_list(handle);
> > diff --git a/drivers/gpio/gpiolib-devprop.c b/drivers/gpio/gpiolib-devprop.c
> > index 26741032fa9e..a28659b4f9c9 100644
> > --- a/drivers/gpio/gpiolib-devprop.c
> > +++ b/drivers/gpio/gpiolib-devprop.c
> > @@ -17,25 +17,24 @@
> >  /**
> >   * devprop_gpiochip_set_names - Set GPIO line names using device properties
> >   * @chip: GPIO chip whose lines should be named, if possible
> > - * @fwnode: Property Node containing the gpio-line-names property
> >   *
> >   * Looks for device property "gpio-line-names" and if it exists assigns
> >   * GPIO line names for the chip. The memory allocated for the assigned
> > - * names belong to the underlying firmware node and should not be released
> > + * names belong to the underlying software node and should not be released
> >   * by the caller.
> >   */
> > -void devprop_gpiochip_set_names(struct gpio_chip *chip,
> > -                             const struct fwnode_handle *fwnode)
> > +int devprop_gpiochip_set_names(struct gpio_chip *chip)
> >  {
> >       struct gpio_device *gdev = chip->gpiodev;
> > +     struct device *dev = chip->parent;
> >       const char **names;
> >       int ret, i;
> >       int count;
> >
> > -     count = fwnode_property_read_string_array(fwnode, "gpio-line-names",
> > +     count = device_property_read_string_array(dev, "gpio-line-names",
> >                                                 NULL, 0);
> >       if (count < 0)
> > -             return;
> > +             return 0;
>
> Can we introduce a followup to 33ee09cd59ce ("device property: Add helpers to
> count items in an array") for strings?
>

Sure, I'll do this in v2.

Bart

Re: [PATCH 14/23] gpio: mockup: use the generic 'gpio-line-names' property

[Bartosz Golaszewski]

On Fri, Sep 4, 2020 at 6:46 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 05:45:38PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > GPIO line names are currently created by the driver from the chip label.
> > We'll want to support custom formats for line names (for instance: to
> > name all lines the same) for user-space tests so create them in the
> > module init function and pass them to the driver using the standard
> > 'gpio-line-names' property.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >  drivers/gpio/gpio-mockup.c | 70 +++++++++++++++++++++-----------------
> >  1 file changed, 38 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> > index ce83f1df1933..96976ba66598 100644
> > --- a/drivers/gpio/gpio-mockup.c
> > +++ b/drivers/gpio/gpio-mockup.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/property.h>
> >  #include <linux/slab.h>
> > +#include <linux/string_helpers.h>
> >  #include <linux/uaccess.h>
> >
> >  #include "gpiolib.h"
> > @@ -378,29 +379,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
> >       return;
> >  }
> >
> > -static int gpio_mockup_name_lines(struct device *dev,
> > -                               struct gpio_mockup_chip *chip)
> > -{
> > -     struct gpio_chip *gc = &chip->gc;
> > -     char **names;
> > -     int i;
> > -
> > -     names = devm_kcalloc(dev, gc->ngpio, sizeof(char *), GFP_KERNEL);
> > -     if (!names)
> > -             return -ENOMEM;
> > -
> > -     for (i = 0; i < gc->ngpio; i++) {
> > -             names[i] = devm_kasprintf(dev, GFP_KERNEL,
> > -                                       "%s-%d", gc->label, i);
> > -             if (!names[i])
> > -                     return -ENOMEM;
> > -     }
> > -
> > -     gc->names = (const char *const *)names;
> > -
> > -     return 0;
> > -}
> > -
> >  static void gpio_mockup_dispose_mappings(void *data)
> >  {
> >       struct gpio_mockup_chip *chip = data;
> > @@ -468,12 +446,6 @@ static int gpio_mockup_probe(struct platform_device *pdev)
> >       for (i = 0; i < gc->ngpio; i++)
> >               chip->lines[i].dir = GPIO_LINE_DIRECTION_IN;
> >
> > -     if (device_property_read_bool(dev, "named-gpio-lines")) {
> > -             rv = gpio_mockup_name_lines(dev, chip);
> > -             if (rv)
> > -                     return rv;
> > -     }
> > -
> >       chip->irq_sim_domain = devm_irq_domain_create_sim(dev, NULL,
> >                                                         gc->ngpio);
> >       if (IS_ERR(chip->irq_sim_domain))
> > @@ -524,6 +496,27 @@ static void gpio_mockup_unregister_devices(void)
> >       }
> >  }
> >
> > +static __init char **gpio_mockup_make_line_names(const char *label,
> > +                                              unsigned int num_lines)
> > +{
> > +     unsigned int i;
> > +     char **names;
> > +
> > +     names = kcalloc(num_lines + 1, sizeof(char *), GFP_KERNEL);
> > +     if (!names)
> > +             return NULL;
> > +
> > +     for (i = 0; i < num_lines; i++) {
> > +             names[i] = kasprintf(GFP_KERNEL, "%s-%u", label, i);
> > +             if (!names[i]) {
> > +                     kfree_strarray(names, i);
> > +                     return NULL;
> > +             }
> > +     }
> > +
> > +     return names;
> > +}
> > +
> >  static int __init gpio_mockup_init(void)
> >  {
> >       struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
> > @@ -531,6 +524,7 @@ static int __init gpio_mockup_init(void)
> >       struct gpio_mockup_device *mockup_dev;
> >       int i, prop, num_chips, err = 0, base;
> >       struct platform_device_info pdevinfo;
> > +     char **line_names;
> >       u16 ngpio;
> >
> >       if ((gpio_mockup_num_ranges < 2) ||
> > @@ -563,6 +557,7 @@ static int __init gpio_mockup_init(void)
> >               memset(properties, 0, sizeof(properties));
> >               memset(&pdevinfo, 0, sizeof(pdevinfo));
> >               prop = 0;
> > +             line_names = NULL;
> >
> >               snprintf(chip_label, sizeof(chip_label),
> >                        "gpio-mockup-%c", i + 'A');
> > @@ -578,9 +573,18 @@ static int __init gpio_mockup_init(void)
> >                                : gpio_mockup_range_ngpio(i) - base;
> >               properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio);
> >
> > -             if (gpio_mockup_named_lines)
> > -                     properties[prop++] = PROPERTY_ENTRY_BOOL(
> > -                                             "named-gpio-lines");
> > +             if (gpio_mockup_named_lines) {
> > +                     line_names = gpio_mockup_make_line_names(chip_label,
> > +                                                              ngpio);
> > +                     if (!line_names) {
> > +                             err = -ENOMEM;
> > +                             goto err_out;
> > +                     }
> > +
> > +                     properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
> > +                                             "gpio-line-names",
> > +                                             line_names, ngpio);
> > +             }
>
> Indentation here looks quite deep. Maybe introduce a helper in between where
> you assign properties?
>

No need for that - this gets flattened later on in the series.

Bart

Re: [PATCH 13/23] gpio: mockup: pass the chip label as device property

[Bartosz Golaszewski]

On Fri, Sep 4, 2020 at 6:48 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 05:45:37PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > While we do check the "chip-name" property in probe(), we never actually
> > use it. Let's pass the chip label to the driver using device properties
> > as we'll want to allow users to define their own once dynamically
> > created chips are supported.
> >
> > The property is renamed to "chip-label" to not cause any confusion with
> > the actual chip name which is of the form: "gpiochipX".
> >
> > If the "chip-label" property is missing, let's do what most devices in
> > drivers/gpio/ do and use dev_name().
>
> Just wondering if we have a documentation in kernel how this mockup mechanism
> works and what kind of properties it uses.
>
> Side note: moving to software nodes would make some advantages in future such
> as visibility properties and their values (not yet implemented, but there is an
> idea to move forward).

Seems like we're implicitly using software nodes already:
fwnode_create_software_node() is called when adding device properties.

Bart

Re: [PATCH 15/23] gpio: mockup: use dynamic device IDs

[Bartosz Golaszewski]

On Fri, Sep 4, 2020 at 6:49 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 05:45:39PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > We're currently creating chips at module init time only so using a
> > static index for dummy devices is fine. We want to support dynamically
> > created chips however so we need to switch to dynamic device IDs.
>
> It misses ida_destroy().
>

No, we always call ida_free() for separate IDs when removing devices
and we remove all devices at module exit so no need to call
ida_destroy().

> What about XArray API?
>

Answered that somewhere - xarray is already used internally by IDA.

Bart

Re: [PATCH 10/23] gpio: mockup: fix resource leak in error path

[Bartosz Golaszewski]

On Fri, Sep 4, 2020 at 7:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 05:45:34PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > If the module init function fails after creating the debugs directory,
> > it's never removed. Add proper cleanup calls to avoid this resource
> > leak.
>
> Does it fix existing bug?

You mean - should it go into stable? The bug is quite unlikely but
yeah, probably.

Bart

Re: [PATCH 16/23] gpio: mockup: refactor the module init function

[Bartosz Golaszewski]

On Fri, Sep 4, 2020 at 6:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 04, 2020 at 05:45:40PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > This is in preparation for dynamically created chips.
> >
> > Let's split out the code that can be reused when creating chips at
> > run-time. Let's also move the code preparing the device properties into
> > a separate routine. This has the advantage of simplifying the error
> > handling.
>
> Almost all contents of this patch should go to proposed helper as I mentioned
> before. Will make this patch quite small and understandable.
>

Sorry, I'm not sure what you're referring to.

Bart

Re: [PATCH 03/23] lib: uaccess: provide getline_from_user()

[Andy Shevchenko]

On Mon, Sep 07, 2020 at 12:28:05PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 7, 2020 at 12:19 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > > On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > > > Doesn't mm/util.c provides us something like this?
> > > > strndup_user()?
> > > >
> > >
> > > Yes, there's both strndup_user() as well as strncpy_from_user(). The
> > > problem is that they rely on the strings being NULL-terminated. This
> > > is not guaranteed for debugfs file_operations write callbacks. We need
> > > some helper that takes the minimum of bytes provided by userspace and
> > > the buffer size and figure out how many bytes to actually copy IMO.
> >
> > Wouldn't this [1] approach work?
> >
> > [1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93
> >
> 
> Sure, but this is pretty much what I do in getline_from_user(). If
> anything we should port mtrr_write() to using getline_from_user() once
> it's available upstream, no?

But you may provide getline_from_user() as inline in the same header where
strncpy_from_user() is declared. It will be like 3 LOCs?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 10/23] gpio: mockup: fix resource leak in error path

[Andy Shevchenko]

On Mon, Sep 07, 2020 at 01:04:59PM +0200, Bartosz Golaszewski wrote:
> On Fri, Sep 4, 2020 at 7:00 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 04, 2020 at 05:45:34PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > If the module init function fails after creating the debugs directory,
> > > it's never removed. Add proper cleanup calls to avoid this resource
> > > leak.
> >
> > Does it fix existing bug?
> 
> You mean - should it go into stable? The bug is quite unlikely but
> yeah, probably.

Yes. That sounds to me like Fixes: is needed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 13/23] gpio: mockup: pass the chip label as device property

[Andy Shevchenko]

On Mon, Sep 07, 2020 at 01:01:02PM +0200, Bartosz Golaszewski wrote:
> On Fri, Sep 4, 2020 at 6:48 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 04, 2020 at 05:45:37PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > While we do check the "chip-name" property in probe(), we never actually
> > > use it. Let's pass the chip label to the driver using device properties
> > > as we'll want to allow users to define their own once dynamically
> > > created chips are supported.
> > >
> > > The property is renamed to "chip-label" to not cause any confusion with
> > > the actual chip name which is of the form: "gpiochipX".
> > >
> > > If the "chip-label" property is missing, let's do what most devices in
> > > drivers/gpio/ do and use dev_name().
> >
> > Just wondering if we have a documentation in kernel how this mockup mechanism
> > works and what kind of properties it uses.
> >
> > Side note: moving to software nodes would make some advantages in future such
> > as visibility properties and their values (not yet implemented, but there is an
> > idea to move forward).
> 
> Seems like we're implicitly using software nodes already:
> fwnode_create_software_node() is called when adding device properties.

It's not the same APIs, though the former is called from the latter.
But maybe for now it's okay.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 15/23] gpio: mockup: use dynamic device IDs

[Andy Shevchenko]

On Mon, Sep 07, 2020 at 01:04:29PM +0200, Bartosz Golaszewski wrote:
> On Fri, Sep 4, 2020 at 6:49 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 04, 2020 at 05:45:39PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > We're currently creating chips at module init time only so using a
> > > static index for dummy devices is fine. We want to support dynamically
> > > created chips however so we need to switch to dynamic device IDs.
> >
> > It misses ida_destroy().
> 
> No, we always call ida_free() for separate IDs when removing devices
> and we remove all devices at module exit so no need to call
> ida_destroy().

Perhaps couple of words about this in the commit message?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 16/23] gpio: mockup: refactor the module init function

[Andy Shevchenko]

On Mon, Sep 07, 2020 at 01:05:54PM +0200, Bartosz Golaszewski wrote:
> On Fri, Sep 4, 2020 at 6:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Sep 04, 2020 at 05:45:40PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > This is in preparation for dynamically created chips.
> > >
> > > Let's split out the code that can be reused when creating chips at
> > > run-time. Let's also move the code preparing the device properties into
> > > a separate routine. This has the advantage of simplifying the error
> > > handling.
> >
> > Almost all contents of this patch should go to proposed helper as I mentioned
> > before. Will make this patch quite small and understandable.

> Sorry, I'm not sure what you're referring to.

I meant if you move changes done here to the patch where I complained about
indentation you might have better series. Have you considered that?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Andy Shevchenko]

On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
> >
> > ...
> >
> > > > +GPIO Testing Driver
> > > > +===================
> > > > +
> > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > > +by the module. The lines can be accessed using the standard GPIO character
> > > > +device interface as well as manipulated using the dedicated debugfs directory
> > > > +structure.
> > >
> > > Could configfs be used for this instead of debugfs?
> > > debugfs is ad hoc.
> >
> > Actually sounds like a good idea.
> >
> 
> Well, then we can go on and write an entirely new mockup driver
> (ditching module params and dropping any backwards compatibility)
> because we're already using debugfs for line values.
> 
> How would we pass the device properties to configfs created GPIO chips
> anyway? Devices seem to only be created using mkdir. Am I missing
> something?

Same way how USB composite works, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/23] lib: uaccess: provide getline_from_user()

[Bartosz Golaszewski]

On Mon, Sep 7, 2020 at 1:45 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Sep 07, 2020 at 12:28:05PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 7, 2020 at 12:19 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski
> > > <bgolaszewski@baylibre.com> wrote:
> > > > On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote:
> > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > > > Doesn't mm/util.c provides us something like this?
> > > > > strndup_user()?
> > > > >
> > > >
> > > > Yes, there's both strndup_user() as well as strncpy_from_user(). The
> > > > problem is that they rely on the strings being NULL-terminated. This
> > > > is not guaranteed for debugfs file_operations write callbacks. We need
> > > > some helper that takes the minimum of bytes provided by userspace and
> > > > the buffer size and figure out how many bytes to actually copy IMO.
> > >
> > > Wouldn't this [1] approach work?
> > >
> > > [1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93
> > >
> >
> > Sure, but this is pretty much what I do in getline_from_user(). If
> > anything we should port mtrr_write() to using getline_from_user() once
> > it's available upstream, no?
>
> But you may provide getline_from_user() as inline in the same header where
> strncpy_from_user() is declared. It will be like 3 LOCs?
>

May be more than that. I'll see what I can do.

Bart

Re: [PATCH 15/23] gpio: mockup: use dynamic device IDs

[Bartosz Golaszewski]

On Mon, Sep 7, 2020 at 1:50 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 07, 2020 at 01:04:29PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Sep 4, 2020 at 6:49 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Sep 04, 2020 at 05:45:39PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > We're currently creating chips at module init time only so using a
> > > > static index for dummy devices is fine. We want to support dynamically
> > > > created chips however so we need to switch to dynamic device IDs.
> > >
> > > It misses ida_destroy().
> >
> > No, we always call ida_free() for separate IDs when removing devices
> > and we remove all devices at module exit so no need to call
> > ida_destroy().
>
> Perhaps couple of words about this in the commit message?
>

But ida_destroy() and ida_free() are already well documented. It's
clear that we remove all devices at exit and that every device removes
its ID, there's really no point in mentioning it again.

Bart

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Bartosz Golaszewski]

On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
> > >
> > > ...
> > >
> > > > > +GPIO Testing Driver
> > > > > +===================
> > > > > +
> > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > > > +by the module. The lines can be accessed using the standard GPIO character
> > > > > +device interface as well as manipulated using the dedicated debugfs directory
> > > > > +structure.
> > > >
> > > > Could configfs be used for this instead of debugfs?
> > > > debugfs is ad hoc.
> > >
> > > Actually sounds like a good idea.
> > >
> >
> > Well, then we can go on and write an entirely new mockup driver
> > (ditching module params and dropping any backwards compatibility)
> > because we're already using debugfs for line values.
> >
> > How would we pass the device properties to configfs created GPIO chips
> > anyway? Devices seem to only be created using mkdir. Am I missing
> > something?
>
> Same way how USB composite works, no?
>

OK, so create a new chip directory in configfs, configure it using
some defined configfs attributes and then finally instantiate it from
sysfs?

Makes sense and is probably the right way to go. Now the question is:
is it fine to just entirely remove the previous gpio-mockup? Should we
keep some backwards compatibility? Should we introduce an entirely new
module and have a transition period before removing previous
gpio-mockup?

Also: this is a testing module so to me debugfs is just fine. Is
configfs considered stable ABI like sysfs?

Bart

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Greg Kroah-Hartman]

On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
> > > >
> > > > ...
> > > >
> > > > > > +GPIO Testing Driver
> > > > > > +===================
> > > > > > +
> > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > > > > +by the module. The lines can be accessed using the standard GPIO character
> > > > > > +device interface as well as manipulated using the dedicated debugfs directory
> > > > > > +structure.
> > > > >
> > > > > Could configfs be used for this instead of debugfs?
> > > > > debugfs is ad hoc.
> > > >
> > > > Actually sounds like a good idea.
> > > >
> > >
> > > Well, then we can go on and write an entirely new mockup driver
> > > (ditching module params and dropping any backwards compatibility)
> > > because we're already using debugfs for line values.
> > >
> > > How would we pass the device properties to configfs created GPIO chips
> > > anyway? Devices seem to only be created using mkdir. Am I missing
> > > something?
> >
> > Same way how USB composite works, no?
> >
> 
> OK, so create a new chip directory in configfs, configure it using
> some defined configfs attributes and then finally instantiate it from
> sysfs?
> 
> Makes sense and is probably the right way to go. Now the question is:
> is it fine to just entirely remove the previous gpio-mockup? Should we
> keep some backwards compatibility? Should we introduce an entirely new
> module and have a transition period before removing previous
> gpio-mockup?
> 
> Also: this is a testing module so to me debugfs is just fine. Is
> configfs considered stable ABI like sysfs?

Yes it is.  Or at least until you fix all existing users so that if you
do change it, no one notices it happening :)

thanks,

greg k-h

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Andy Shevchenko]

On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:

...

> > > > > > +GPIO Testing Driver
> > > > > > +===================
> > > > > > +
> > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > > > > +by the module. The lines can be accessed using the standard GPIO character
> > > > > > +device interface as well as manipulated using the dedicated debugfs directory
> > > > > > +structure.
> > > > >
> > > > > Could configfs be used for this instead of debugfs?
> > > > > debugfs is ad hoc.
> > > >
> > > > Actually sounds like a good idea.
> > > >
> > >
> > > Well, then we can go on and write an entirely new mockup driver
> > > (ditching module params and dropping any backwards compatibility)
> > > because we're already using debugfs for line values.
> > >
> > > How would we pass the device properties to configfs created GPIO chips
> > > anyway? Devices seem to only be created using mkdir. Am I missing
> > > something?
> >
> > Same way how USB composite works, no?
> >
> 
> OK, so create a new chip directory in configfs, configure it using
> some defined configfs attributes and then finally instantiate it from
> sysfs?
> 
> Makes sense and is probably the right way to go. Now the question is:
> is it fine to just entirely remove the previous gpio-mockup?

Since, for example, I never saw device property bindings for that driver I
assume that it was never considered as an ABI, so feel free to hack it in
either direction.

> Should we
> keep some backwards compatibility?

I wouldn't probably spend time on this.

> Should we introduce an entirely new
> module and have a transition period before removing previous
> gpio-mockup?

Neither transition period.

> Also: this is a testing module so to me debugfs is just fine. Is
> configfs considered stable ABI like sysfs?

But this one is a good question. I think ConfigFS is stricter than DebugFS,
up to being an ABI. But never did myself such a thing, so would like to hear
experienced developers.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Bartosz Golaszewski]

On Mon, Sep 7, 2020 at 2:38 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
>
> ...
>
> > > > > > > +GPIO Testing Driver
> > > > > > > +===================
> > > > > > > +
> > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > > > > > +by the module. The lines can be accessed using the standard GPIO character
> > > > > > > +device interface as well as manipulated using the dedicated debugfs directory
> > > > > > > +structure.
> > > > > >
> > > > > > Could configfs be used for this instead of debugfs?
> > > > > > debugfs is ad hoc.
> > > > >
> > > > > Actually sounds like a good idea.
> > > > >
> > > >
> > > > Well, then we can go on and write an entirely new mockup driver
> > > > (ditching module params and dropping any backwards compatibility)
> > > > because we're already using debugfs for line values.
> > > >
> > > > How would we pass the device properties to configfs created GPIO chips
> > > > anyway? Devices seem to only be created using mkdir. Am I missing
> > > > something?
> > >
> > > Same way how USB composite works, no?
> > >
> >
> > OK, so create a new chip directory in configfs, configure it using
> > some defined configfs attributes and then finally instantiate it from
> > sysfs?
> >
> > Makes sense and is probably the right way to go. Now the question is:
> > is it fine to just entirely remove the previous gpio-mockup?
>
> Since, for example, I never saw device property bindings for that driver I
> assume that it was never considered as an ABI, so feel free to hack it in
> either direction.
>
> > Should we
> > keep some backwards compatibility?
>
> I wouldn't probably spend time on this.
>
> > Should we introduce an entirely new
> > module and have a transition period before removing previous
> > gpio-mockup?
>
> Neither transition period.
>

I wouldn't rush this actually. gpio-mockup is used a lot by libgpiod
and probably by Kent's Go library. My main goal with this series is to
extend it to allow for more advanced testing like simulating spurious
irqs to test the software debouncer or custom line name formats to
test name lookups.

I need to think about it some more. An entirely new configfs interface
would take time too.

Bart

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Bartosz Golaszewski]

On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > +GPIO Testing Driver
> > > > > > > +===================
> > > > > > > +
> > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > > > > > +by the module. The lines can be accessed using the standard GPIO character
> > > > > > > +device interface as well as manipulated using the dedicated debugfs directory
> > > > > > > +structure.
> > > > > >
> > > > > > Could configfs be used for this instead of debugfs?
> > > > > > debugfs is ad hoc.
> > > > >
> > > > > Actually sounds like a good idea.
> > > > >
> > > >
> > > > Well, then we can go on and write an entirely new mockup driver
> > > > (ditching module params and dropping any backwards compatibility)
> > > > because we're already using debugfs for line values.
> > > >
> > > > How would we pass the device properties to configfs created GPIO chips
> > > > anyway? Devices seem to only be created using mkdir. Am I missing
> > > > something?
> > >
> > > Same way how USB composite works, no?
> > >
> >
> > OK, so create a new chip directory in configfs, configure it using
> > some defined configfs attributes and then finally instantiate it from
> > sysfs?
> >
> > Makes sense and is probably the right way to go. Now the question is:
> > is it fine to just entirely remove the previous gpio-mockup? Should we
> > keep some backwards compatibility? Should we introduce an entirely new
> > module and have a transition period before removing previous
> > gpio-mockup?
> >
> > Also: this is a testing module so to me debugfs is just fine. Is
> > configfs considered stable ABI like sysfs?
>
> Yes it is.  Or at least until you fix all existing users so that if you
> do change it, no one notices it happening :)
>

Then another question is: do we really want to commit to a stable ABI
for a module we only use for testing purposes and which doesn't
interact with any real hardware.

Rewriting this module without any legacy cruft is tempting though. :)

Bart

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Andy Shevchenko]

On Mon, Sep 07, 2020 at 02:57:29PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 7, 2020 at 2:38 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote:
> > > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
> >
> > ...
> >
> > > > > > > > +GPIO Testing Driver
> > > > > > > > +===================
> > > > > > > > +
> > > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > > > > > > +by the module. The lines can be accessed using the standard GPIO character
> > > > > > > > +device interface as well as manipulated using the dedicated debugfs directory
> > > > > > > > +structure.
> > > > > > >
> > > > > > > Could configfs be used for this instead of debugfs?
> > > > > > > debugfs is ad hoc.
> > > > > >
> > > > > > Actually sounds like a good idea.
> > > > > >
> > > > >
> > > > > Well, then we can go on and write an entirely new mockup driver
> > > > > (ditching module params and dropping any backwards compatibility)
> > > > > because we're already using debugfs for line values.
> > > > >
> > > > > How would we pass the device properties to configfs created GPIO chips
> > > > > anyway? Devices seem to only be created using mkdir. Am I missing
> > > > > something?
> > > >
> > > > Same way how USB composite works, no?
> > > >
> > >
> > > OK, so create a new chip directory in configfs, configure it using
> > > some defined configfs attributes and then finally instantiate it from
> > > sysfs?
> > >
> > > Makes sense and is probably the right way to go. Now the question is:
> > > is it fine to just entirely remove the previous gpio-mockup?
> >
> > Since, for example, I never saw device property bindings for that driver I
> > assume that it was never considered as an ABI, so feel free to hack it in
> > either direction.
> >
> > > Should we
> > > keep some backwards compatibility?
> >
> > I wouldn't probably spend time on this.
> >
> > > Should we introduce an entirely new
> > > module and have a transition period before removing previous
> > > gpio-mockup?
> >
> > Neither transition period.
> >
> 
> I wouldn't rush this actually. gpio-mockup is used a lot by libgpiod
> and probably by Kent's Go library. My main goal with this series is to
> extend it to allow for more advanced testing like simulating spurious
> irqs to test the software debouncer or custom line name formats to
> test name lookups.

When I wrote above I didn't mean this should be done in a hurry. Just during
one release to make all parties ready for the change.

> I need to think about it some more. An entirely new configfs interface
> would take time too.

Sure!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Andy Shevchenko]

On Mon, Sep 07, 2020 at 03:49:23PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:

...

> > Yes it is.  Or at least until you fix all existing users so that if you
> > do change it, no one notices it happening :)
> >
> 
> Then another question is: do we really want to commit to a stable ABI
> for a module we only use for testing purposes and which doesn't
> interact with any real hardware.
> 
> Rewriting this module without any legacy cruft is tempting though. :)

Another thought spoken loudly: maybe it can be unified with GPIO aggregator
code? In that case it makes sense.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Bartosz Golaszewski]

On Mon, Sep 7, 2020 at 4:08 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 07, 2020 at 03:49:23PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > > Yes it is.  Or at least until you fix all existing users so that if you
> > > do change it, no one notices it happening :)
> > >
> >
> > Then another question is: do we really want to commit to a stable ABI
> > for a module we only use for testing purposes and which doesn't
> > interact with any real hardware.
> >
> > Rewriting this module without any legacy cruft is tempting though. :)
>
> Another thought spoken loudly: maybe it can be unified with GPIO aggregator
> code? In that case it makes sense.
>

Cc'ing Geert but I don't quite see how this would make sense. :)

Also one thing I'm not sure about re configfs is the interface we use
to read values/set pull i.e. the line attributes in debugfs, do you
think configfs allows this type of attributes?

Bart

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Geert Uytterhoeven]

Hi Andy,

On Mon, Sep 7, 2020 at 4:14 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Sep 07, 2020 at 03:49:23PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > > Yes it is.  Or at least until you fix all existing users so that if you
> > > do change it, no one notices it happening :)
> > >
> >
> > Then another question is: do we really want to commit to a stable ABI
> > for a module we only use for testing purposes and which doesn't
> > interact with any real hardware.
> >
> > Rewriting this module without any legacy cruft is tempting though. :)
>
> Another thought spoken loudly: maybe it can be unified with GPIO aggregator
> code? In that case it makes sense.

You want to aggregate GPIOs out of thin air?

From DT, that would be something like

    gpios = <&gpio1 2>, <0>, <0>, <&gpio2, 5>;

?

For writing into ".../new_device", we could agree on something like "0"
means not backed by an existing GPIO?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Bartosz Golaszewski]

On Mon, Sep 7, 2020 at 5:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Andy,
>
> On Mon, Sep 7, 2020 at 4:14 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Sep 07, 2020 at 03:49:23PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
> >
> > ...
> >
> > > > Yes it is.  Or at least until you fix all existing users so that if you
> > > > do change it, no one notices it happening :)
> > > >
> > >
> > > Then another question is: do we really want to commit to a stable ABI
> > > for a module we only use for testing purposes and which doesn't
> > > interact with any real hardware.
> > >
> > > Rewriting this module without any legacy cruft is tempting though. :)
> >
> > Another thought spoken loudly: maybe it can be unified with GPIO aggregator
> > code? In that case it makes sense.
>
> You want to aggregate GPIOs out of thin air?
>
> From DT, that would be something like
>
>     gpios = <&gpio1 2>, <0>, <0>, <&gpio2, 5>;
>
> ?
>
> For writing into ".../new_device", we could agree on something like "0"
> means not backed by an existing GPIO?
>

I'm really not sure this makes any sense. Why complicate an otherwise
elegant module that is gpio-aggregator with functionalities that
obviously don't belong here? I want to add various parameters that
would affect the way the simulated chips work - this really doesn't
need to go into the aggregator.

Bart

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Bartosz Golaszewski]

On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > +GPIO Testing Driver
> > > > > > > +===================
> > > > > > > +
> > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > > > > > +by the module. The lines can be accessed using the standard GPIO character
> > > > > > > +device interface as well as manipulated using the dedicated debugfs directory
> > > > > > > +structure.
> > > > > >
> > > > > > Could configfs be used for this instead of debugfs?
> > > > > > debugfs is ad hoc.
> > > > >
> > > > > Actually sounds like a good idea.
> > > > >
> > > >
> > > > Well, then we can go on and write an entirely new mockup driver
> > > > (ditching module params and dropping any backwards compatibility)
> > > > because we're already using debugfs for line values.
> > > >
> > > > How would we pass the device properties to configfs created GPIO chips
> > > > anyway? Devices seem to only be created using mkdir. Am I missing
> > > > something?
> > >
> > > Same way how USB composite works, no?
> > >
> >
> > OK, so create a new chip directory in configfs, configure it using
> > some defined configfs attributes and then finally instantiate it from
> > sysfs?
> >
> > Makes sense and is probably the right way to go. Now the question is:
> > is it fine to just entirely remove the previous gpio-mockup? Should we
> > keep some backwards compatibility? Should we introduce an entirely new
> > module and have a transition period before removing previous
> > gpio-mockup?
> >
> > Also: this is a testing module so to me debugfs is just fine. Is
> > configfs considered stable ABI like sysfs?
>
> Yes it is.  Or at least until you fix all existing users so that if you
> do change it, no one notices it happening :)
>

Got it. One more question: the current debugfs interface we're using
in gpio-mockup exists to allow to read current values of GPIO lines in
output mode (check how the user drives dummy lines) and to set their
simulated pull-up/pull-down resistors (what values the user reads in
input mode).

This works like this: in /sys/kernel/debug/gpio-mockup every dummy
chip creates its own directory (e.g.
/sys/kernel/debug/gpio-mockup/gpiochip0) and inside this directory
there's an attribute per line named after the line's offset (e.g.
/sys/kernel/debug/gpio-mockup/gpiochip0/4). Writing 0 or 1 to this
attribute sets the pull resistor. Reading from it yields the current
value (0 or 1 as well).

This is pretty non-standard so I proposed to put it in debugfs. If we
were to use configfs - is this where something like this should go? Or
rather sysfs? Is it even suitable/acceptable for sysfs?

Thanks,
Bartosz

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Greg Kroah-Hartman]

On Tue, Sep 08, 2020 at 07:03:30PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote:
> > > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > >
> > > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > > +GPIO Testing Driver
> > > > > > > > +===================
> > > > > > > > +
> > > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > > > > > > +by the module. The lines can be accessed using the standard GPIO character
> > > > > > > > +device interface as well as manipulated using the dedicated debugfs directory
> > > > > > > > +structure.
> > > > > > >
> > > > > > > Could configfs be used for this instead of debugfs?
> > > > > > > debugfs is ad hoc.
> > > > > >
> > > > > > Actually sounds like a good idea.
> > > > > >
> > > > >
> > > > > Well, then we can go on and write an entirely new mockup driver
> > > > > (ditching module params and dropping any backwards compatibility)
> > > > > because we're already using debugfs for line values.
> > > > >
> > > > > How would we pass the device properties to configfs created GPIO chips
> > > > > anyway? Devices seem to only be created using mkdir. Am I missing
> > > > > something?
> > > >
> > > > Same way how USB composite works, no?
> > > >
> > >
> > > OK, so create a new chip directory in configfs, configure it using
> > > some defined configfs attributes and then finally instantiate it from
> > > sysfs?
> > >
> > > Makes sense and is probably the right way to go. Now the question is:
> > > is it fine to just entirely remove the previous gpio-mockup? Should we
> > > keep some backwards compatibility? Should we introduce an entirely new
> > > module and have a transition period before removing previous
> > > gpio-mockup?
> > >
> > > Also: this is a testing module so to me debugfs is just fine. Is
> > > configfs considered stable ABI like sysfs?
> >
> > Yes it is.  Or at least until you fix all existing users so that if you
> > do change it, no one notices it happening :)
> >
> 
> Got it. One more question: the current debugfs interface we're using
> in gpio-mockup exists to allow to read current values of GPIO lines in
> output mode (check how the user drives dummy lines) and to set their
> simulated pull-up/pull-down resistors (what values the user reads in
> input mode).
> 
> This works like this: in /sys/kernel/debug/gpio-mockup every dummy
> chip creates its own directory (e.g.
> /sys/kernel/debug/gpio-mockup/gpiochip0) and inside this directory
> there's an attribute per line named after the line's offset (e.g.
> /sys/kernel/debug/gpio-mockup/gpiochip0/4). Writing 0 or 1 to this
> attribute sets the pull resistor. Reading from it yields the current
> value (0 or 1 as well).
> 
> This is pretty non-standard so I proposed to put it in debugfs. If we
> were to use configfs - is this where something like this should go? Or
> rather sysfs? Is it even suitable/acceptable for sysfs?

That sounds like it would work in sysfs just fine as-is, why don't you
all want to use that?  configfs is good for "set a bunch of attributes
to different values and then do a 'create/go/work'" type action.


thanks,

greg k-h

Re: [PATCH 23/23] Documentation: gpio: add documentation for gpio-mockup

[Bartosz Golaszewski]

On Fri, Sep 11, 2020 at 3:01 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Sep 08, 2020 at 07:03:30PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote:
> > > > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > >
> > > > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote:
> > > > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote:
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > > > +GPIO Testing Driver
> > > > > > > > > +===================
> > > > > > > > > +
> > > > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO
> > > > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed
> > > > > > > > > +by the module. The lines can be accessed using the standard GPIO character
> > > > > > > > > +device interface as well as manipulated using the dedicated debugfs directory
> > > > > > > > > +structure.
> > > > > > > >
> > > > > > > > Could configfs be used for this instead of debugfs?
> > > > > > > > debugfs is ad hoc.
> > > > > > >
> > > > > > > Actually sounds like a good idea.
> > > > > > >
> > > > > >
> > > > > > Well, then we can go on and write an entirely new mockup driver
> > > > > > (ditching module params and dropping any backwards compatibility)
> > > > > > because we're already using debugfs for line values.
> > > > > >
> > > > > > How would we pass the device properties to configfs created GPIO chips
> > > > > > anyway? Devices seem to only be created using mkdir. Am I missing
> > > > > > something?
> > > > >
> > > > > Same way how USB composite works, no?
> > > > >
> > > >
> > > > OK, so create a new chip directory in configfs, configure it using
> > > > some defined configfs attributes and then finally instantiate it from
> > > > sysfs?
> > > >
> > > > Makes sense and is probably the right way to go. Now the question is:
> > > > is it fine to just entirely remove the previous gpio-mockup? Should we
> > > > keep some backwards compatibility? Should we introduce an entirely new
> > > > module and have a transition period before removing previous
> > > > gpio-mockup?
> > > >
> > > > Also: this is a testing module so to me debugfs is just fine. Is
> > > > configfs considered stable ABI like sysfs?
> > >
> > > Yes it is.  Or at least until you fix all existing users so that if you
> > > do change it, no one notices it happening :)
> > >
> >
> > Got it. One more question: the current debugfs interface we're using
> > in gpio-mockup exists to allow to read current values of GPIO lines in
> > output mode (check how the user drives dummy lines) and to set their
> > simulated pull-up/pull-down resistors (what values the user reads in
> > input mode).
> >
> > This works like this: in /sys/kernel/debug/gpio-mockup every dummy
> > chip creates its own directory (e.g.
> > /sys/kernel/debug/gpio-mockup/gpiochip0) and inside this directory
> > there's an attribute per line named after the line's offset (e.g.
> > /sys/kernel/debug/gpio-mockup/gpiochip0/4). Writing 0 or 1 to this
> > attribute sets the pull resistor. Reading from it yields the current
> > value (0 or 1 as well).
> >
> > This is pretty non-standard so I proposed to put it in debugfs. If we
> > were to use configfs - is this where something like this should go? Or
> > rather sysfs? Is it even suitable/acceptable for sysfs?
>
> That sounds like it would work in sysfs just fine as-is, why don't you
> all want to use that?  configfs is good for "set a bunch of attributes
> to different values and then do a 'create/go/work'" type action.
>

I've started looking into it. I need to first implement committable
items for configfs because mockup GPIO chips need to be configured
before they're instantiated. It'll be configfs to configure and
instantiate each chip and a set of sysfs attributes to manipulate
existing chips.

Bartosz