[PATCH v2 0/9] gpio: mockup: refactoring + documentation

[PATCH v2 0/9] gpio: mockup: refactoring + documentation

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

These patches were part of the bigger overhaul of gpio-mockup but since
the initial idea was dropped in favor of using configfs + sysfs in the
future I thought I'd resent just the refactoring of the existing code
+ documentation patches. I think it's good to apply them since we don't
really know when the new interface will be ready (configfs needs a new
functionality - commitable items - to support mockup chip instantiation).

v1 -> v2:
- check for NULL pointer in kfree_strarray() to avoid having to always pass
  a zeroed string count when the array pointer is NULL
- collect review tags from Andy

Bartosz Golaszewski (9):
  lib: string_helpers: provide kfree_strarray()
  Documentation: gpio: add documentation for gpio-mockup
  gpio: mockup: drop unneeded includes
  gpio: mockup: use KBUILD_MODNAME
  gpio: mockup: use pr_fmt()
  gpio: mockup: remove unneeded return statement
  gpio: mockup: pass the chip label as device property
  gpio: mockup: use the generic 'gpio-line-names' property
  gpio: mockup: refactor the module init function

 .../admin-guide/gpio/gpio-mockup.rst          |  50 ++++++
 drivers/gpio/gpio-mockup.c                    | 154 +++++++++---------
 include/linux/string_helpers.h                |   2 +
 lib/string_helpers.c                          |  25 +++
 4 files changed, 155 insertions(+), 76 deletions(-)
 create mode 100644 Documentation/admin-guide/gpio/gpio-mockup.rst

-- 
2.26.1

[PATCH v2 1/9] 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           | 25 +++++++++++++++++++++++++
 2 files changed, 27 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..bfa4c9f3ca0a 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -649,3 +649,28 @@ 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.
+ *
+ * Passing a non-null str_array and num_str == 0 as well as NULL str_array
+ * are valid use-cases.
+ */
+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 v2 2/9] 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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../admin-guide/gpio/gpio-mockup.rst          | 50 +++++++++++++++++++
 1 file changed, 50 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..9fa1618b3adc
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-mockup.rst
@@ -0,0 +1,50 @@
+.. 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. The lines exposed by these chips can be accessed
+using the standard GPIO character device interface as well as manipulated
+using the dedicated debugfs directory structure.
+
+Creating simulated chips using module params
+--------------------------------------------
+
+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 mockup chip's ID
+        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 the configuration of the simulated pull-up/pull-down resistor
+(1 - pull-up, 0 - pull-down).
-- 
2.26.1

[PATCH v2 3/9] 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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 1652897fdf90..c5092773afd8 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 v2 4/9] 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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 c5092773afd8..90a1d6c2775f 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -21,7 +21,6 @@
 
 #include "gpiolib.h"
 
-#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
@@ -31,7 +30,7 @@
 /* Maximum of three properties + the sentinel. */
 #define GPIO_MOCKUP_MAX_PROP	4
 
-#define gpio_mockup_err(...)	pr_err(GPIO_MOCKUP_NAME ": " __VA_ARGS__)
+#define gpio_mockup_err(...)	pr_err(KBUILD_MODNAME ": " __VA_ARGS__)
 
 /*
  * struct gpio_pin_status - structure describing a GPIO status
@@ -500,7 +499,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 v2 5/9] 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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mockup.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 90a1d6c2775f..c2b2f7d5ff34 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -7,6 +7,8 @@
  * Copyright (C) 2017 Bartosz Golaszewski <brgl@bgdev.pl>
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/debugfs.h>
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
@@ -30,8 +32,6 @@
 /* Maximum of three properties + the sentinel. */
 #define GPIO_MOCKUP_MAX_PROP	4
 
-#define gpio_mockup_err(...)	pr_err(KBUILD_MODNAME ": " __VA_ARGS__)
-
 /*
  * struct gpio_pin_status - structure describing a GPIO status
  * @dir:       Configures direction of gpio as "in" or "out"
@@ -548,7 +548,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");
 		debugfs_remove_recursive(gpio_mockup_dbg_dir);
 		return err;
 	}
@@ -577,7 +577,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();
 			debugfs_remove_recursive(gpio_mockup_dbg_dir);
-- 
2.26.1

[PATCH v2 6/9] gpio: mockup: remove unneeded return statement

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

There's a return; at the end of a void function. This is not needed so
remove it.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 c2b2f7d5ff34..de778b52f355 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -372,8 +372,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
 		debugfs_create_file(name, 0200, chip->dbg_dir, priv,
 				    &gpio_mockup_debugfs_ops);
 	}
-
-	return;
 }
 
 static int gpio_mockup_name_lines(struct device *dev,
-- 
2.26.1

[PATCH v2 7/9] 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 de778b52f355..5b2686f9e07d 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -429,21 +429,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;
@@ -523,6 +516,7 @@ static int __init gpio_mockup_init(void)
 	int i, prop, num_chips, err = 0, base;
 	struct platform_device_info pdevinfo;
 	struct platform_device *pdev;
+	char chip_label[32];
 	u16 ngpio;
 
 	if ((gpio_mockup_num_ranges < 2) ||
@@ -556,6 +550,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 v2 8/9] 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 5b2686f9e07d..47b7de6d5ab1 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_helpers.h>
 #include <linux/uaccess.h>
 
 #include "gpiolib.h"
@@ -374,29 +375,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
 	}
 }
 
-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;
@@ -464,12 +442,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))
@@ -510,6 +482,27 @@ static void gpio_mockup_unregister_pdevs(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];
@@ -517,6 +510,7 @@ static int __init gpio_mockup_init(void)
 	struct platform_device_info pdevinfo;
 	struct platform_device *pdev;
 	char chip_label[32];
+	char **line_names;
 	u16 ngpio;
 
 	if ((gpio_mockup_num_ranges < 2) ||
@@ -549,6 +543,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');
@@ -564,15 +559,26 @@ 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) {
+				platform_driver_unregister(&gpio_mockup_driver);
+				gpio_mockup_unregister_pdevs();
+				return -ENOMEM;
+			}
+
+			properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
+						"gpio-line-names",
+						line_names, ngpio);
+		}
 
 		pdevinfo.name = "gpio-mockup";
 		pdevinfo.id = i;
 		pdevinfo.properties = properties;
 
 		pdev = platform_device_register_full(&pdevinfo);
+		kfree_strarray(line_names, ngpio);
 		if (IS_ERR(pdev)) {
 			pr_err("error registering device");
 			platform_driver_unregister(&gpio_mockup_driver);
-- 
2.26.1

[PATCH v2 9/9] gpio: mockup: refactor the module init function

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Let's move the code preparing the device properties into a separate
routine. This has the advantage of simplifying the error handling and
makes the indentation less deep.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-mockup.c | 96 +++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 47b7de6d5ab1..02eea05a09dd 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -503,16 +503,59 @@ 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_chip(int idx)
 {
 	struct property_entry properties[GPIO_MOCKUP_MAX_PROP];
-	int i, prop, num_chips, err = 0, base;
 	struct platform_device_info pdevinfo;
 	struct platform_device *pdev;
+	char **line_names = NULL;
 	char chip_label[32];
-	char **line_names;
+	int prop = 0, base;
 	u16 ngpio;
 
+	memset(properties, 0, sizeof(properties));
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+
+	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);
+	}
+
+	pdevinfo.name = "gpio-mockup";
+	pdevinfo.id = idx;
+	pdevinfo.properties = properties;
+
+	pdev = platform_device_register_full(&pdevinfo);
+	kfree_strarray(line_names, ngpio);
+	if (IS_ERR(pdev)) {
+		pr_err("error registering device");
+		return PTR_ERR(pdev);
+	}
+
+	gpio_mockup_pdevs[idx] = pdev;
+
+	return 0;
+}
+
+static int __init gpio_mockup_init(void)
+{
+	int i, num_chips, err;
+
 	if ((gpio_mockup_num_ranges < 2) ||
 	    (gpio_mockup_num_ranges % 2) ||
 	    (gpio_mockup_num_ranges > GPIO_MOCKUP_MAX_RANGES))
@@ -540,54 +583,13 @@ static int __init gpio_mockup_init(void)
 	}
 
 	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) {
-				platform_driver_unregister(&gpio_mockup_driver);
-				gpio_mockup_unregister_pdevs();
-				return -ENOMEM;
-			}
-
-			properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
-						"gpio-line-names",
-						line_names, ngpio);
-		}
-
-		pdevinfo.name = "gpio-mockup";
-		pdevinfo.id = i;
-		pdevinfo.properties = properties;
-
-		pdev = platform_device_register_full(&pdevinfo);
-		kfree_strarray(line_names, ngpio);
-		if (IS_ERR(pdev)) {
-			pr_err("error registering device");
+		err = gpio_mockup_register_chip(i);
+		if (err) {
 			platform_driver_unregister(&gpio_mockup_driver);
 			gpio_mockup_unregister_pdevs();
 			debugfs_remove_recursive(gpio_mockup_dbg_dir);
-			return PTR_ERR(pdev);
+			return err;
 		}
-
-		gpio_mockup_pdevs[i] = pdev;
 	}
 
 	return 0;
-- 
2.26.1

Re: [PATCH v2 1/9] lib: string_helpers: provide kfree_strarray()

[Andy Shevchenko]

On Mon, Sep 28, 2020 at 12:41:47PM +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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
But see below.

> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/string_helpers.h |  2 ++
>  lib/string_helpers.c           | 25 +++++++++++++++++++++++++
>  2 files changed, 27 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..bfa4c9f3ca0a 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -649,3 +649,28 @@ 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.
> + *
> + * Passing a non-null str_array and num_str == 0 as well as NULL str_array
> + * are valid use-cases.
> + */
> +void kfree_strarray(char **str_array, size_t num_str)

Hmm... I have missed your answer to 
 str_array -> array
 num_str -> n

The rationale behind dropping str is to avoid duplicates in the name of the
function and its parameters. 'array' is harder to avoid, but also possible,
though I leave it to you.

> +{
> +	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
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/9] gpio: mockup: pass the chip label as device property

[Andy Shevchenko]

On Mon, Sep 28, 2020 at 12:41:53PM +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().

...

> +		snprintf(chip_label, sizeof(chip_label),
> +			 "gpio-mockup-%c", i + 'A');
> +		properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
> +							   chip_label);

You added new property, now count is up to 4. But at the same time

	#define GPIO_MOCKUP_MAX_PROP  4

how do you avoid overflow?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 8/9] gpio: mockup: use the generic 'gpio-line-names' property

[Andy Shevchenko]

On Mon, Sep 28, 2020 at 12:41:54PM +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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 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 5b2686f9e07d..47b7de6d5ab1 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_helpers.h>
>  #include <linux/uaccess.h>
>  
>  #include "gpiolib.h"
> @@ -374,29 +375,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
>  	}
>  }
>  
> -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;
> @@ -464,12 +442,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))
> @@ -510,6 +482,27 @@ static void gpio_mockup_unregister_pdevs(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];
> @@ -517,6 +510,7 @@ static int __init gpio_mockup_init(void)
>  	struct platform_device_info pdevinfo;
>  	struct platform_device *pdev;
>  	char chip_label[32];
> +	char **line_names;
>  	u16 ngpio;
>  
>  	if ((gpio_mockup_num_ranges < 2) ||
> @@ -549,6 +543,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');
> @@ -564,15 +559,26 @@ 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) {
> +				platform_driver_unregister(&gpio_mockup_driver);
> +				gpio_mockup_unregister_pdevs();
> +				return -ENOMEM;
> +			}
> +
> +			properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
> +						"gpio-line-names",
> +						line_names, ngpio);
> +		}
>  
>  		pdevinfo.name = "gpio-mockup";
>  		pdevinfo.id = i;
>  		pdevinfo.properties = properties;
>  
>  		pdev = platform_device_register_full(&pdevinfo);
> +		kfree_strarray(line_names, ngpio);
>  		if (IS_ERR(pdev)) {
>  			pr_err("error registering device");
>  			platform_driver_unregister(&gpio_mockup_driver);
> -- 
> 2.26.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/9] lib: string_helpers: provide kfree_strarray()

[Bartosz Golaszewski]

On Mon, Sep 28, 2020 at 2:55 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 28, 2020 at 12:41:47PM +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.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> But see below.
>
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >  include/linux/string_helpers.h |  2 ++
> >  lib/string_helpers.c           | 25 +++++++++++++++++++++++++
> >  2 files changed, 27 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..bfa4c9f3ca0a 100644
> > --- a/lib/string_helpers.c
> > +++ b/lib/string_helpers.c
> > @@ -649,3 +649,28 @@ 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.
> > + *
> > + * Passing a non-null str_array and num_str == 0 as well as NULL str_array
> > + * are valid use-cases.
> > + */
> > +void kfree_strarray(char **str_array, size_t num_str)
>
> Hmm... I have missed your answer to
>  str_array -> array
>  num_str -> n
>
> The rationale behind dropping str is to avoid duplicates in the name of the
> function and its parameters. 'array' is harder to avoid, but also possible,
> though I leave it to you.
>

Are you fine with me fixing this when applying the patches?

Bart

Re: [PATCH v2 7/9] gpio: mockup: pass the chip label as device property

[Bartosz Golaszewski]

On Mon, Sep 28, 2020 at 3:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 28, 2020 at 12:41:53PM +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".
> >

^^^ here, see below

> > If the "chip-label" property is missing, let's do what most devices in
> > drivers/gpio/ do and use dev_name().
>
> ...
>
> > +             snprintf(chip_label, sizeof(chip_label),
> > +                      "gpio-mockup-%c", i + 'A');
> > +             properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
> > +                                                        chip_label);
>
> You added new property, now count is up to 4. But at the same time
>
>         #define GPIO_MOCKUP_MAX_PROP  4
>
> how do you avoid overflow?
>

I renamed the property, the previous "chip-name" is no longer used. In
fact it was never used but was accounted for in GPIO_MOCKUP_MAX_PROP.

Bart

Re: [PATCH v2 1/9] lib: string_helpers: provide kfree_strarray()

[Andy Shevchenko]

On Mon, Sep 28, 2020 at 03:04:05PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 28, 2020 at 2:55 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Sep 28, 2020 at 12:41:47PM +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.
> >
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > But see below.
> >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ---
> > >  include/linux/string_helpers.h |  2 ++
> > >  lib/string_helpers.c           | 25 +++++++++++++++++++++++++
> > >  2 files changed, 27 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..bfa4c9f3ca0a 100644
> > > --- a/lib/string_helpers.c
> > > +++ b/lib/string_helpers.c
> > > @@ -649,3 +649,28 @@ 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.
> > > + *
> > > + * Passing a non-null str_array and num_str == 0 as well as NULL str_array
> > > + * are valid use-cases.
> > > + */
> > > +void kfree_strarray(char **str_array, size_t num_str)
> >
> > Hmm... I have missed your answer to
> >  str_array -> array
> >  num_str -> n
> >
> > The rationale behind dropping str is to avoid duplicates in the name of the
> > function and its parameters. 'array' is harder to avoid, but also possible,
> > though I leave it to you.
> >
> 
> Are you fine with me fixing this when applying the patches?

Sure!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/9] gpio: mockup: pass the chip label as device property

[Andy Shevchenko]

On Mon, Sep 28, 2020 at 03:13:53PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 28, 2020 at 3:00 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Sep 28, 2020 at 12:41:53PM +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".
> > >
> 
> ^^^ here, see below
> 
> > > If the "chip-label" property is missing, let's do what most devices in
> > > drivers/gpio/ do and use dev_name().
> >
> > ...
> >
> > > +             snprintf(chip_label, sizeof(chip_label),
> > > +                      "gpio-mockup-%c", i + 'A');
> > > +             properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
> > > +                                                        chip_label);
> >
> > You added new property, now count is up to 4. But at the same time
> >
> >         #define GPIO_MOCKUP_MAX_PROP  4
> >
> > how do you avoid overflow?
> >
> 
> I renamed the property, the previous "chip-name" is no longer used. In
> fact it was never used but was accounted for in GPIO_MOCKUP_MAX_PROP.

Either I'm missing something or...

Current code in linux-next has 3 properties to be possible

PROPERTY_ENTRY_U32("gpio-base", base);
PROPERTY_ENTRY_U16("nr-gpios", ngpio);
PROPERTY_ENTRY_BOOL("named-gpio-lines");

You adding here
PROPERTY_ENTRY_STRING("chip-label", chip_label);

Altogether after this patch is 4 which is maximum, but since array is passed by
a solely pointer, the terminator is a must.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/9] gpio: mockup: pass the chip label as device property

[Bartosz Golaszewski]

On Mon, Sep 28, 2020 at 4:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 28, 2020 at 03:13:53PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 28, 2020 at 3:00 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Mon, Sep 28, 2020 at 12:41:53PM +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".
> > > >
> >
> > ^^^ here, see below
> >
> > > > If the "chip-label" property is missing, let's do what most devices in
> > > > drivers/gpio/ do and use dev_name().
> > >
> > > ...
> > >
> > > > +             snprintf(chip_label, sizeof(chip_label),
> > > > +                      "gpio-mockup-%c", i + 'A');
> > > > +             properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
> > > > +                                                        chip_label);
> > >
> > > You added new property, now count is up to 4. But at the same time
> > >
> > >         #define GPIO_MOCKUP_MAX_PROP  4
> > >
> > > how do you avoid overflow?
> > >
> >
> > I renamed the property, the previous "chip-name" is no longer used. In
> > fact it was never used but was accounted for in GPIO_MOCKUP_MAX_PROP.
>
> Either I'm missing something or...
>
> Current code in linux-next has 3 properties to be possible
>
> PROPERTY_ENTRY_U32("gpio-base", base);
> PROPERTY_ENTRY_U16("nr-gpios", ngpio);
> PROPERTY_ENTRY_BOOL("named-gpio-lines");
>
> You adding here
> PROPERTY_ENTRY_STRING("chip-label", chip_label);
>
> Altogether after this patch is 4 which is maximum, but since array is passed by
> a solely pointer, the terminator is a must.
>

Thanks for explaining my code to me. Yes you're right and I'm not sure
why I missed this. :)

I'll fix this in v3.

Actually this means the code is wrong even before this series - it's
just that we don't use the "chip-name" property.

Bartosz

Re: [PATCH v2 1/9] lib: string_helpers: provide kfree_strarray()

[Joe Perches]

On Mon, 2020-09-28 at 12:41 +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.

Isn't this also common for things like ring buffers?
Why limit this to char *[]?

Re: [PATCH v2 1/9] lib: string_helpers: provide kfree_strarray()

[Bartosz Golaszewski]

On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2020-09-28 at 12:41 +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.
>
> Isn't this also common for things like ring buffers?
> Why limit this to char *[]?
>

I don't want to add APIs nobody is using. What do you suggest?

Bartosz

Re: [PATCH v2 1/9] lib: string_helpers: provide kfree_strarray()

[Joe Perches]

On Mon, 2020-09-28 at 18:02 +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2020-09-28 at 12:41 +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.
> > 
> > Isn't this also common for things like ring buffers?
> > Why limit this to char *[]?
> > 
> 
> I don't want to add APIs nobody is using. What do you suggest?

Change the argument to void** and call it

void kfree_array(void **array, int count);

Re: [PATCH v2 7/9] gpio: mockup: pass the chip label as device property

[Andy Shevchenko]

On Mon, Sep 28, 2020 at 04:52:25PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 28, 2020 at 4:00 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Sep 28, 2020 at 03:13:53PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Sep 28, 2020 at 3:00 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Sep 28, 2020 at 12:41:53PM +0200, Bartosz Golaszewski wrote:

...

> > > > how do you avoid overflow?
> > >
> > > I renamed the property, the previous "chip-name" is no longer used. In
> > > fact it was never used but was accounted for in GPIO_MOCKUP_MAX_PROP.
> >
> > Either I'm missing something or...
> >
> > Current code in linux-next has 3 properties to be possible
> >
> > PROPERTY_ENTRY_U32("gpio-base", base);
> > PROPERTY_ENTRY_U16("nr-gpios", ngpio);
> > PROPERTY_ENTRY_BOOL("named-gpio-lines");
> >
> > You adding here
> > PROPERTY_ENTRY_STRING("chip-label", chip_label);
> >
> > Altogether after this patch is 4 which is maximum, but since array is passed by
> > a solely pointer, the terminator is a must.
> >
> 
> Thanks for explaining my code to me. Yes you're right and I'm not sure
> why I missed this. :)
> 
> I'll fix this in v3.
> 
> Actually this means the code is wrong even before this series - it's
> just that we don't use the "chip-name" property.

Right, you patch just exposed it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/9] lib: string_helpers: provide kfree_strarray()

[Andy Shevchenko]

On Mon, Sep 28, 2020 at 09:06:34AM -0700, Joe Perches wrote:
> On Mon, 2020-09-28 at 18:02 +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2020-09-28 at 12:41 +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.
> > > 
> > > Isn't this also common for things like ring buffers?
> > > Why limit this to char *[]?
> > > 
> > 
> > I don't want to add APIs nobody is using. What do you suggest?
> 
> Change the argument to void** and call it
> 
> void kfree_array(void **array, int count);

Bart, if you go for this, I'm fine. You may keep my tag.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 1/9] lib: string_helpers: provide kfree_strarray()

[David Laight]

From: Joe Perches
> Sent: 28 September 2020 17:07
> 
> On Mon, 2020-09-28 at 18:02 +0200, Bartosz Golaszewski wrote:
> > On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2020-09-28 at 12:41 +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.
> > >
> > > Isn't this also common for things like ring buffers?
> > > Why limit this to char *[]?
> > >
> >
> > I don't want to add APIs nobody is using. What do you suggest?
> 
> Change the argument to void** and call it
> 
> void kfree_array(void **array, int count);

Does help, void doesn't work that way.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 1/9] lib: string_helpers: provide kfree_strarray()

[Andy Shevchenko]

On Tue, Sep 29, 2020 at 08:10:10AM +0000, David Laight wrote:
> From: Joe Perches
> > Sent: 28 September 2020 17:07
> > 
> > On Mon, 2020-09-28 at 18:02 +0200, Bartosz Golaszewski wrote:
> > > On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote:
> > > > On Mon, 2020-09-28 at 12:41 +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.
> > > >
> > > > Isn't this also common for things like ring buffers?
> > > > Why limit this to char *[]?
> > > >
> > >
> > > I don't want to add APIs nobody is using. What do you suggest?
> > 
> > Change the argument to void** and call it
> > 
> > void kfree_array(void **array, int count);
> 
> Does help, void doesn't work that way.

Actually good catch. void * and void ** have a big difference in the implicit
casting behaviour. I was stumbled over this while playing with some
experimental stuff locally.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/9] lib: string_helpers: provide kfree_strarray()

[Bartosz Golaszewski]

On Tue, Sep 29, 2020 at 10:49 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 29, 2020 at 08:10:10AM +0000, David Laight wrote:
> > From: Joe Perches
> > > Sent: 28 September 2020 17:07
> > >
> > > On Mon, 2020-09-28 at 18:02 +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Sep 28, 2020 at 5:59 PM Joe Perches <joe@perches.com> wrote:
> > > > > On Mon, 2020-09-28 at 12:41 +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.
> > > > >
> > > > > Isn't this also common for things like ring buffers?
> > > > > Why limit this to char *[]?
> > > > >
> > > >
> > > > I don't want to add APIs nobody is using. What do you suggest?
> > >
> > > Change the argument to void** and call it
> > >
> > > void kfree_array(void **array, int count);
> >
> > Does help, void doesn't work that way.
>
> Actually good catch. void * and void ** have a big difference in the implicit
> casting behaviour. I was stumbled over this while playing with some
> experimental stuff locally.
>

I'll keep kfree_strarray() then.

Bart