[PATCH RFD 0/3] Add gpio test framework

[PATCH RFD 0/3] Add gpio test framework

[Bamvor Jian Zhang]

These series of patches try to add support for testing of gpio
subsystem based on the proposal from Linus Walleij.

The basic idea is implement a virtual gpio device(gpio-mockup) base
on gpiolib. Tester could test the gpiolib by manipulating gpio-mockup
device through sysfs and check the result from debugfs. Both sysfs
and debugfs are provided by gpiolib. Reference the following figure:

   sysfs  debugfs
     |       |
  gpiolib---/
     |
 gpio-mockup

Find two issues with my patch series[1]

Futher work and discussion:
1.  Test other code path(if exists).

2   Add pinctrl and interrupt support(Linus suggest trying the
    eventfd) in next steps.

3.  I feel that we could rework the debugfs in other gpiolib based
    drivers to the code in gpiolib.c with generic gpiolib_dbg_show or
    chip->dbg_show.

4.  Currently, gpio-mockup does not support the device tree. There are
    pros and cons if we do not support device tree:
    Pros: do not need to mix with the real hardware dts.
    Cons: could not test the dt_gpio_count, of_find_gpio which rely on
    device tree. And other function such like gpiod_get_index which
    rely on the correctness of the above functions.

    If we want to test the above functions, we could provide a
    dedicated device tree for gpio-mockup device which could be
    included by other device tree. And we could use device tree
    overlay to provide multiple device tree testcases.

5.  Given that this gpio test framework is based on sysfs and debugfs.
    I feel that it could be a generic gpio test script other then a
    dedicated script for gpio-mockup driver. Although, my script only
    support gpio-mockup driver at this monment.

[1] http://article.gmane.org/gmane.linux.kernel.gpio/11878

Bamvor Jian Zhang (3):
  gpio/mockup: add virtual gpio device
  selftest/gpio: add gpio test case
  gpio: MAINTAINERS: Add an entry for GPIO mockup driver

 MAINTAINERS                          |   9 ++
 drivers/gpio/Kconfig                 |   9 ++
 drivers/gpio/Makefile                |   1 +
 drivers/gpio/gpio-mockup.c           | 216 +++++++++++++++++++++++++++++++
 tools/testing/selftests/Makefile     |   1 +
 tools/testing/selftests/gpio/gpio.sh | 238 +++++++++++++++++++++++++++++++++++
 6 files changed, 474 insertions(+)
 create mode 100644 drivers/gpio/gpio-mockup.c
 create mode 100755 tools/testing/selftests/gpio/gpio.sh

-- 
2.1.4

[PATCH RFD 1/3] gpio/mockup: add virtual gpio device

[Bamvor Jian Zhang]

This patch add basic structure of a virtual gpio device(gpio-mockup)
for testing gpio subsystem. The tester could manipulate such device
through sysfs and check the result from debugfs.

Currently, it support one or more gpiochip(determined by module
parameters with base,ngpio pair). One could test the overlap of
different gpiochip and test the direction and/or output values of
these chips.

Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
---
 drivers/gpio/Kconfig       |   9 ++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-mockup.c | 216 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 226 insertions(+)
 create mode 100644 drivers/gpio/gpio-mockup.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b18bea0..41d8421 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -268,6 +268,15 @@ config GPIO_MM_LANTIQ
 	  (EBU) found on Lantiq SoCs. The gpios are output only as they are
 	  created by attaching a 16bit latch to the bus.
 
+config GPIO_MOCKUP
+	tristate "GPIO Testing Driver"
+	depends on GPIOLIB
+	select GPIO_SYSFS
+	help
+	  This enables GPIO Testing driver, which provides a way to test GPIO
+	  subsystem through sysfs and debugfs. GPIO_SYSFS must be selected for
+	  this test.
+
 config GPIO_MOXART
 	bool "MOXART GPIO support"
 	depends on ARCH_MOXART
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 986dbd8..b91b66c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_GPIO_MC9S08DZ60)	+= gpio-mc9s08dz60.o
 obj-$(CONFIG_GPIO_MCP23S08)	+= gpio-mcp23s08.o
 obj-$(CONFIG_GPIO_ML_IOH)	+= gpio-ml-ioh.o
 obj-$(CONFIG_GPIO_MM_LANTIQ)	+= gpio-mm-lantiq.o
+obj-$(CONFIG_GPIO_MOCKUP)      += gpio-mockup.o
 obj-$(CONFIG_GPIO_MOXART)	+= gpio-moxart.o
 obj-$(CONFIG_GPIO_MPC5200)	+= gpio-mpc5200.o
 obj-$(CONFIG_GPIO_MPC8XXX)	+= gpio-mpc8xxx.o
diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
new file mode 100644
index 0000000..b689389
--- /dev/null
+++ b/drivers/gpio/gpio-mockup.c
@@ -0,0 +1,216 @@
+/*
+ * GPIO Testing Device Driver
+ *
+ * Copyright (C) 2014  Kamlakant Patel <kamlakant.patel@linaro.org>
+ * Copyright (C) 2015  Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/gpio/driver.h>
+#include <linux/platform_device.h>
+
+#define GPIO_NAME	"gpio-mockup"
+#define	MAX_GC		5
+
+enum direction {
+	IN,
+	OUT
+};
+
+/*
+ * struct gpio_pin_status - structure describing a GPIO status
+ * @dir:       Configures direction of gpio as "in" or "out", 0=in, 1=out
+ * @value:     Configures status of the gpio as 0(low) or 1(high)
+ */
+struct gpio_pin_status {
+	enum direction	dir;
+	int		value;
+};
+
+struct mockup_gpio_controller {
+	struct gpio_chip	gc;
+	struct gpio_pin_status	*stats;
+};
+
+int conf[MAX_GC << 1];
+int params_nr;
+module_param_array(conf, int, &params_nr, 0400);
+
+const char *pins_name[MAX_GC] = {"A", "B", "C", "D", "E"};
+
+static int
+mockup_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+	struct mockup_gpio_controller *cntr = container_of(gc,
+			struct mockup_gpio_controller, gc);
+
+	return cntr->stats[offset].value;
+}
+
+static void
+mockup_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
+{
+	struct mockup_gpio_controller *cntr = container_of(gc,
+			struct mockup_gpio_controller, gc);
+
+	if (value)
+		cntr->stats[offset].value = 1;
+	else
+		cntr->stats[offset].value = 0;
+
+}
+
+static int mockup_gpio_dirout(struct gpio_chip *gc, unsigned offset,
+		int value)
+{
+	struct mockup_gpio_controller *cntr = container_of(gc,
+			struct mockup_gpio_controller, gc);
+
+	mockup_gpio_set(gc, offset, value);
+	cntr->stats[offset].dir = OUT;
+	return 0;
+}
+
+static int mockup_gpio_dirin(struct gpio_chip *gc, unsigned offset)
+{
+	struct mockup_gpio_controller *cntr = container_of(gc,
+			struct mockup_gpio_controller, gc);
+
+	cntr->stats[offset].dir = IN;
+	return 0;
+}
+
+int mockup_gpio_add(struct device *dev, struct mockup_gpio_controller *cntr,
+		const char *name, int base, int ngpio)
+{
+	int ret;
+
+	cntr->gc.base = base;
+	cntr->gc.ngpio = ngpio;
+	cntr->gc.label = name;
+	cntr->gc.owner = THIS_MODULE;
+	cntr->gc.dev = dev;
+	cntr->gc.get = mockup_gpio_get;
+	cntr->gc.set = mockup_gpio_set;
+	cntr->gc.direction_output = mockup_gpio_dirout;
+	cntr->gc.direction_input = mockup_gpio_dirin;
+	cntr->stats = devm_kzalloc(dev, sizeof(*cntr->stats) * cntr->gc.ngpio,
+			GFP_KERNEL);
+	if (!cntr->stats) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	ret = gpiochip_add(&cntr->gc);
+	if (ret)
+		goto err;
+
+	dev_info(dev, "gpio<%d..%d> add successful!", base, base + ngpio);
+	return 0;
+err:
+	dev_err(dev, "gpio<%d..%d> add failed!", base, base + ngpio);
+	return ret;
+}
+
+static int
+mockup_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mockup_gpio_controller *cntr;
+	int ret;
+	int i, j;
+	int base;
+	int ngpio;
+
+	if (params_nr == 0) {
+		params_nr = 2;
+		conf[0] = 0;
+		conf[1] = 32;
+	}
+
+	cntr = devm_kzalloc(dev, sizeof(*cntr) * params_nr, GFP_KERNEL);
+	if (!cntr)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, cntr);
+
+	for (i = 0; i < params_nr >> 1; i++) {
+		base = conf[i << 1];
+		ngpio = conf[(i << 1) + 1];
+		ret = mockup_gpio_add(dev, &cntr[i], pins_name[i], base, ngpio);
+		if (ret) {
+			dev_err(dev, "gpio<%d..%d> add failed, remove added gpio\n",
+					base, base + ngpio);
+			for (j = 0; j < i; j++)
+				gpiochip_remove(&cntr[j].gc);
+
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int mockup_gpio_remove(struct platform_device *pdev)
+{
+	struct mockup_gpio_controller *cntr = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < params_nr >> 1; i++)
+		gpiochip_remove(&cntr[i].gc);
+
+	return 0;
+}
+
+static struct platform_driver mockup_gpio_driver = {
+	.driver = {
+		.name           = GPIO_NAME,
+	},
+	.probe = mockup_gpio_probe,
+	.remove = mockup_gpio_remove,
+};
+
+static struct platform_device *pdev;
+static int __init mock_device_init(void)
+{
+	int err;
+
+	pdev = platform_device_alloc(GPIO_NAME, -1);
+	if (!pdev)
+		return -ENOMEM;
+
+	err = platform_device_add(pdev);
+	if (err) {
+		platform_device_put(pdev);
+		return err;
+	}
+
+	err = platform_driver_register(&mockup_gpio_driver);
+	if (err) {
+		platform_device_unregister(pdev);
+		return err;
+	}
+
+	return 0;
+}
+
+static void __exit
+mock_device_exit(void)
+{
+	platform_driver_unregister(&mockup_gpio_driver);
+	platform_device_unregister(pdev);
+}
+
+module_init(mock_device_init);
+module_exit(mock_device_exit);
+
+MODULE_AUTHOR("Kamlakant Patel <kamlakant.patel@linaro.org>");
+MODULE_AUTHOR("Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>");
+MODULE_DESCRIPTION("GPIO Testing driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

[PATCH RFD 2/3] selftest/gpio: add gpio test case

[Bamvor Jian Zhang]

This test script try to do whitebox testing for gpio subsystem(
based on gpiolib). It manipulate gpio-mockup device through sysfs
provided and check the result from debugfs.

Test the following things:
1.  Add single, multi gpiochip with the checking of overlap.
2.  Test direction and output value for valid pin.
3.  Test dynamic allocation of gpio base.

Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
---
 tools/testing/selftests/Makefile     |   1 +
 tools/testing/selftests/gpio/gpio.sh | 238 +++++++++++++++++++++++++++++++++++
 2 files changed, 239 insertions(+)
 create mode 100755 tools/testing/selftests/gpio/gpio.sh

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index cfe1213..df0b81d 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -5,6 +5,7 @@ TARGETS += exec
 TARGETS += firmware
 TARGETS += ftrace
 TARGETS += futex
+TARGETS += gpio
 TARGETS += kcmp
 TARGETS += membarrier
 TARGETS += memfd
diff --git a/tools/testing/selftests/gpio/gpio.sh b/tools/testing/selftests/gpio/gpio.sh
new file mode 100755
index 0000000..05e109e
--- /dev/null
+++ b/tools/testing/selftests/gpio/gpio.sh
@@ -0,0 +1,238 @@
+#!/bin/bash
+
+module="gpio-mockup"
+
+SYSFS=
+GPIO_SYSFS=
+GPIO_DRV_SYSFS=
+DEBUGFS=
+GPIO_DEBUGFS=
+
+prerequisite()
+{
+	msg="skip all tests:"
+
+	if [ $UID != 0 ]; then
+		echo $msg must be run as root >&2
+		exit 0
+	fi
+
+	SYSFS=`mount -t sysfs | head -1 | awk '{ print $3 }'`
+	if [ ! -d "$SYSFS" ]; then
+		echo $msg sysfs is not mounted >&2
+		exit 0
+	fi
+	GPIO_SYSFS=`echo $SYSFS/class/gpio`
+	GPIO_DRV_SYSFS=`echo $SYSFS/devices/platform/$module/gpio`
+
+	DEBUGFS=`mount -t debugfs | head -1 | awk '{ print $3 }'`
+	if [ ! -d "$DEBUGFS" ]; then
+		echo $msg debugfs is not mounted >&2
+		exit 0
+	fi
+	GPIO_DEBUGFS=`echo $DEBUGFS/gpio`
+
+}
+
+insert_module()
+{
+	if modprobe -q $module $1; then
+		echo -n ""
+	else
+		echo $msg insmod $module failed >&2
+		exit 0
+	fi
+}
+
+remove_module()
+{
+	modprobe -r -q $module
+}
+
+die()
+{
+	remove_module
+	exit 1
+}
+
+gpio_test()
+{
+	param=$1
+	if [ X$2 != X ]; then
+		invalid_pin=$2
+	fi
+	if [ X$param = X ]; then
+		insert_module
+	else
+		insert_module "conf=$param"
+	fi
+
+	echo -n "GPIO $module test with base,ngpio configure<"
+	if [ X$param = X ]; then
+		echo "(default)>: ";
+	else
+		echo "$param>: "
+	fi
+	printf "%-10s %-5s %-5s\n" name base ngpio
+	gpiochip=`ls -d $GPIO_DRV_SYSFS/gpiochip* 2>/dev/null`
+	if [ X"$gpiochip" = X ]; then
+		echo "no gpiochip"
+	else
+		for chip in $gpiochip; do
+			name=`basename $chip`
+			base=`cat $chip/base`
+			ngpio=`cat $chip/ngpio`
+			printf "%-10s %-5s %-5s\n" $name $base $ngpio
+			if [ $ngpio = "0" ]; then
+				echo "number of gpio is zero is not allowed".
+			fi
+			test_one_pin $base
+			test_one_pin $(($base + $ngpio - 1))
+			test_one_pin $((( RANDOM % $ngpio )  + $base ))
+			if [ X$invalid_pin != X ]; then
+				test_one_pin_fail $invalid_pin
+			fi
+		done
+	fi
+	remove_module
+}
+
+gpio_test_fail()
+{
+	param=$1
+
+	insert_module "conf=$param"
+	echo -n "GPIO $module fail test with base,ngpio configure<$param>: "
+	gpiochip=`ls -d $GPIO_DRV_SYSFS/gpiochip* 2>/dev/null`
+	if [ X"$gpiochip" = X ]; then
+		echo "successful"
+	else
+		echo "fail"
+		die
+	fi
+	remove_module
+}
+
+is_consistent()
+{
+	val=
+
+	active_low_sysfs=`cat $GPIO_SYSFS/gpio$nr/active_low`
+	val_sysfs=`cat $GPIO_SYSFS/gpio$nr/value`
+	dir_sysfs=`cat $GPIO_SYSFS/gpio$nr/direction`
+
+	gpio_this_debugfs=`cat $GPIO_DEBUGFS |grep "gpio-$nr" | sed "s/(.*)//g"`
+	dir_debugfs=`echo $gpio_this_debugfs | awk '{print $2}'`
+	val_debugfs=`echo $gpio_this_debugfs | awk '{print $3}'`
+	if [ $val_debugfs = "lo" ]; then
+		val=0
+	elif [ $val_debugfs = "hi" ]; then
+		val=1
+	fi
+
+	if [ $active_low_sysfs = "1" ]; then
+		if [ $val = "0" ]; then
+			val="1"
+		else
+			val="0"
+		fi
+	fi
+
+	if [ $val_sysfs = $val ] && [ $dir_sysfs = $dir_debugfs ]; then
+		echo -n "."
+	else
+		echo "test fail, exit"
+		die
+	fi
+}
+
+test_pin_logic()
+{
+	nr=$1
+	direction=$2
+	active_low=$3
+	value=$4
+
+	echo $direction > $GPIO_SYSFS/gpio$nr/direction
+	echo $active_low > $GPIO_SYSFS/gpio$nr/active_low
+	if [ $direction = "out" ]; then
+		echo $value > $GPIO_SYSFS/gpio$nr/value
+	fi
+	is_consistent $nr
+}
+
+test_one_pin()
+{
+	nr=$1
+
+	echo -n "test pin<$nr>"
+
+	echo $nr > $GPIO_SYSFS/export 2>/dev/null
+
+	if [ X$? != X0 ]; then
+		echo "test GPIO pin $nr failed"
+		die
+	fi
+
+	#"Checking if the sysfs is consistent with debugfs: "
+	is_consistent $nr
+
+	#"Checking the logic of active_low: "
+	test_pin_logic $nr out 1 1
+	test_pin_logic $nr out 1 0
+	test_pin_logic $nr out 0 1
+	test_pin_logic $nr out 0 0
+
+	#"Checking the logic of direction: "
+	test_pin_logic $nr in 1 1
+	test_pin_logic $nr out 1 0
+	test_pin_logic $nr low 0 1
+	test_pin_logic $nr high 0 0
+
+	echo $nr > $GPIO_SYSFS/unexport
+
+	echo "successful"
+}
+
+test_one_pin_fail()
+{
+	nr=$1
+
+	echo $nr > $GPIO_SYSFS/export 2>/dev/null
+
+	if [ X$? != X0 ]; then
+		echo "test invalid pin $nr successful"
+	else
+		echo "test invalid pin $nr failed"
+		echo $nr > $GPIO_SYSFS/unexport 2>/dev/null
+		die
+	fi
+}
+
+prerequisite
+
+#basically
+gpio_test
+gpio_test "0,32" 33
+gpio_test "0,32,32,16" 50
+gpio_test "0,32,40,16,32,5" 38
+#test dynamic allocation of gpio
+gpio_test "-1,32"
+gpio_test "-1,32,32,16" 31
+gpio_test "-1,32,40,16,-1,5" 38
+#error test
+#zero number of gpio
+gpio_test_fail "0,0"
+#base conflict
+gpio_test_fail "0,32,0,1"
+#range overlap
+gpio_test_fail "0,32,30,5"
+gpio_test_fail "0,32,1,5"
+gpio_test_fail "10,32,9,5"
+gpio_test_fail "10,32,30,5"
+gpio_test_fail "0,32,40,16,39,5"
+gpio_test_fail "0,32,40,16,30,5"
+gpio_test_fail "0,32,40,16,30,11"
+gpio_test_fail "0,32,40,16,20,1"
+
+echo GPIO test PASS
-- 
2.1.4

[PATCH RFD 3/3] gpio: MAINTAINERS: Add an entry for GPIO mockup driver

[Bamvor Jian Zhang]

Add an entry for the GPIO mockup driver with myself as maintainer.

Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6ad9d5c..ac648f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4649,6 +4649,15 @@ L:	linux-input@vger.kernel.org
 S:	Maintained
 F:	drivers/input/touchscreen/goodix.c
 
+GPIO MOCKUP DRIVER
+M:	Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
+L:	linux-gpio@vger.kernel.org
+W:	https://github.com/bjzhang/linux
+T:	git git://github.com/bjzhang/linux
+S:	Maintained
+F:	drivers/gpio/gpio-mockup.c
+F:	tools/testing/selftests/gpio/
+
 GPIO SUBSYSTEM
 M:	Linus Walleij <linus.walleij@linaro.org>
 M:	Alexandre Courbot <gnurou@gmail.com>
-- 
2.1.4

Re: [PATCH RFD 1/3] gpio/mockup: add virtual gpio device

[Linus Walleij]

On Sat, Nov 14, 2015 at 9:51 AM, Bamvor Jian Zhang
<bamvor.zhangjian@linaro.org> wrote:

> This patch add basic structure of a virtual gpio device(gpio-mockup)
> for testing gpio subsystem. The tester could manipulate such device
> through sysfs and check the result from debugfs.
>
> Currently, it support one or more gpiochip(determined by module
> parameters with base,ngpio pair). One could test the overlap of
> different gpiochip and test the direction and/or output values of
> these chips.
>
> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>

This is looking quite nice. Just minor nitpicks:

> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/platform_device.h>
> +
> +#define GPIO_NAME      "gpio-mockup"
> +#define        MAX_GC          5
> +
> +enum direction {
> +       IN,
> +       OUT
> +};
> +
> +/*
> + * struct gpio_pin_status - structure describing a GPIO status
> + * @dir:       Configures direction of gpio as "in" or "out", 0=in, 1=out
> + * @value:     Configures status of the gpio as 0(low) or 1(high)
> + */
> +struct gpio_pin_status {
> +       enum direction  dir;
> +       int             value;

bool value

Since it is only 0/1.

> +struct mockup_gpio_controller {
> +       struct gpio_chip        gc;
> +       struct gpio_pin_status  *stats;
> +};
> +
> +int conf[MAX_GC << 1];
> +int params_nr;
> +module_param_array(conf, int, &params_nr, 0400);

I don't really understand why we need these.

"params_nr" is a way too generic name, plus such module parameters
should be documented in Documentation/kernel-parameters.txt
gpio_test_chips = n is better, but I don't really see why we need to
specify this at all, can't we just decide how many we need? Why
do we need more than one?

> +const char *pins_name[MAX_GC] = {"A", "B", "C", "D", "E"};

So this has 5 pins. Any special reason why?
I think 32 is more typical, but there may be a point in having some
odd number.

> +
> +static int
> +mockup_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct mockup_gpio_controller *cntr = container_of(gc,
> +                       struct mockup_gpio_controller, gc);
> +
> +       return cntr->stats[offset].value;

This should still work with a bool there I think.

> +static void
> +mockup_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
> +{
> +       struct mockup_gpio_controller *cntr = container_of(gc,
> +                       struct mockup_gpio_controller, gc);
> +
> +       if (value)
> +               cntr->stats[offset].value = 1;
> +       else
> +               cntr->stats[offset].value = 0;

Assign like this to do boolean clamping:

cntr->stats[offset].value = !!value;


> +int mockup_gpio_add(struct device *dev, struct mockup_gpio_controller *cntr,
> +               const char *name, int base, int ngpio)
> +{
> +       int ret;
> +
> +       cntr->gc.base = base;

I don't think base should be specified for the test chip.
We should set this to -1 and get a dynamic base, whatever
it is. Then the test scripts should deal with that.

Doing this also makes the conf array go away.

> +static int
> +mockup_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct mockup_gpio_controller *cntr;
> +       int ret;
> +       int i, j;
> +       int base;
> +       int ngpio;
> +
> +       if (params_nr == 0) {
> +               params_nr = 2;
> +               conf[0] = 0;
> +               conf[1] = 32;
> +       }

This needs explaining. Why do we want 2 test gpio chips by default?
Why not just one?

Why is the base of the second one 32 and not 5 since there is 5 pins
on each chip?

I would think adding just one chip or two with very varying characteristics
is the way to go. Like one 32 line chip and one 5 line chip or
something.

> +
> +       cntr = devm_kzalloc(dev, sizeof(*cntr) * params_nr, GFP_KERNEL);
> +       if (!cntr)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, cntr);
> +
> +       for (i = 0; i < params_nr >> 1; i++) {
> +               base = conf[i << 1];
> +               ngpio = conf[(i << 1) + 1];

These should go away I think.

The rest looks good.

Yours,
Linus Walleij

Re: [PATCH RFD 2/3] selftest/gpio: add gpio test case

[Linus Walleij]

On Sat, Nov 14, 2015 at 9:51 AM, Bamvor Jian Zhang
<bamvor.zhangjian@linaro.org> wrote:

> This test script try to do whitebox testing for gpio subsystem(
> based on gpiolib). It manipulate gpio-mockup device through sysfs
> provided and check the result from debugfs.
>
> Test the following things:
> 1.  Add single, multi gpiochip with the checking of overlap.
> 2.  Test direction and output value for valid pin.
> 3.  Test dynamic allocation of gpio base.
>
> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>

I like what this test is doing, but at some point we will need tests
written in C.

The reason is that we are working on a character device for gpio,
and once we have that, we want to be able to run these tests using
the same program.

But maybe this suffice for the sysfs tests of the current ABI.

> +++ b/tools/testing/selftests/gpio/gpio.sh

Rename it gpio-mockup-sysfs.sh please.

> +#!/bin/bash
> +
> +module="gpio-mockup"

This is ABI so not changing, you can just encode it into the
program, but OK.

> +
> +SYSFS=
> +GPIO_SYSFS=
> +GPIO_DRV_SYSFS=
> +DEBUGFS=
> +GPIO_DEBUGFS=
> +
> +prerequisite()
> +{
> +       msg="skip all tests:"
> +
> +       if [ $UID != 0 ]; then
> +               echo $msg must be run as root >&2
> +               exit 0
> +       fi
> +
> +       SYSFS=`mount -t sysfs | head -1 | awk '{ print $3 }'`
> +       if [ ! -d "$SYSFS" ]; then
> +               echo $msg sysfs is not mounted >&2
> +               exit 0
> +       fi
> +       GPIO_SYSFS=`echo $SYSFS/class/gpio`
> +       GPIO_DRV_SYSFS=`echo $SYSFS/devices/platform/$module/gpio`
> +
> +       DEBUGFS=`mount -t debugfs | head -1 | awk '{ print $3 }'`
> +       if [ ! -d "$DEBUGFS" ]; then
> +               echo $msg debugfs is not mounted >&2
> +               exit 0
> +       fi
> +       GPIO_DEBUGFS=`echo $DEBUGFS/gpio`
> +
> +}

Looks good.

> +insert_module()
> +{
> +       if modprobe -q $module $1; then
> +               echo -n ""
> +       else
> +               echo $msg insmod $module failed >&2
> +               exit 0
> +       fi
> +}
> +
> +remove_module()
> +{
> +       modprobe -r -q $module
> +}
> +
> +die()
> +{
> +       remove_module
> +       exit 1
> +}

So this inserting/removing modules work fine unless you want to
run the tests with the module compiled-in. We should support that
usecase too. So these functions must bail out silently if the module
is already loaded (compiled in).

> +gpio_test()
> +{
> +       param=$1
> +       if [ X$2 != X ]; then
> +               invalid_pin=$2
> +       fi
> +       if [ X$param = X ]; then
> +               insert_module
> +       else
> +               insert_module "conf=$param"
> +       fi

As mentioned in the previous patch I'm suspicious about the parameters.
Let's just have one or two mock chips and cut down on the
parameterization.

> +       echo -n "GPIO $module test with base,ngpio configure<"
> +       if [ X$param = X ]; then
> +               echo "(default)>: ";
> +       else
> +               echo "$param>: "
> +       fi
> +       printf "%-10s %-5s %-5s\n" name base ngpio
> +       gpiochip=`ls -d $GPIO_DRV_SYSFS/gpiochip* 2>/dev/null`
> +       if [ X"$gpiochip" = X ]; then
> +               echo "no gpiochip"
> +       else
> +               for chip in $gpiochip; do
> +                       name=`basename $chip`
> +                       base=`cat $chip/base`
> +                       ngpio=`cat $chip/ngpio`

This is nice, because it relies on the chip informing us about the
base and ngpio, meaning we can use a dynamic base, as pointed
out in the previous patch.

> +#test dynamic allocation of gpio
> +gpio_test "-1,32"
> +gpio_test "-1,32,32,16" 31
> +gpio_test "-1,32,40,16,-1,5" 38

I think these are the only tests we should have, really.

Using base 0 as some tests are doing is not going to work on
any system that already has a built-in GPIO (SoC) controller.
I think it is better to just use and test dynamic allocations.

Yours,
Linus Walleij

Re: [PATCH RFD 1/3] gpio/mockup: add virtual gpio device

[Bamvor Jian Zhang]

Hi, Linus

On 11/26/2015 05:34 PM, Linus Walleij wrote:
> On Sat, Nov 14, 2015 at 9:51 AM, Bamvor Jian Zhang
> <bamvor.zhangjian@linaro.org> wrote:
> 
>> This patch add basic structure of a virtual gpio device(gpio-mockup)
>> for testing gpio subsystem. The tester could manipulate such device
>> through sysfs and check the result from debugfs.
>>
>> Currently, it support one or more gpiochip(determined by module
>> parameters with base,ngpio pair). One could test the overlap of
>> different gpiochip and test the direction and/or output values of
>> these chips.
>>
>> Signed-off-by: Kamlakant Patel <kamlakant.patel@linaro.org>
>> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
> 
> This is looking quite nice. Just minor nitpicks:
Thanks.
> 
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define GPIO_NAME      "gpio-mockup"
>> +#define        MAX_GC          5
>> +
>> +enum direction {
>> +       IN,
>> +       OUT
>> +};
>> +
>> +/*
>> + * struct gpio_pin_status - structure describing a GPIO status
>> + * @dir:       Configures direction of gpio as "in" or "out", 0=in, 1=out
>> + * @value:     Configures status of the gpio as 0(low) or 1(high)
>> + */
>> +struct gpio_pin_status {
>> +       enum direction  dir;
>> +       int             value;
> 
> bool value
> 
> Since it is only 0/1.
Ok, I will change it in next version.
> 
>> +struct mockup_gpio_controller {
>> +       struct gpio_chip        gc;
>> +       struct gpio_pin_status  *stats;
>> +};
>> +
>> +int conf[MAX_GC << 1];
>> +int params_nr;
>> +module_param_array(conf, int, &params_nr, 0400);
> 
> I don't really understand why we need these.
> 
> "params_nr" is a way too generic name, plus such module parameters
> should be documented in Documentation/kernel-parameters.txt
> gpio_test_chips = n is better, but I don't really see why we need to
> specify this at all, can't we just decide how many we need? Why
> do we need more than one?
Firstly, one or more gpiochip(s) exist in real driver. And I found the
number of gpiochip may be the corner case. Just like the fix in 
Commit 5433aed ("gpiolib: improve overlap check of range of gpio").
Left this parameter to userspace make the test script easier to
different corner case.
In fact, one or two or more gpiochip is different cases in
gpiochip_add_to_list().
> 
>> +const char *pins_name[MAX_GC] = {"A", "B", "C", "D", "E"};
> 
> So this has 5 pins. Any special reason why?
There is no special reason. Like I said above, I found that there 
are the corner cases when I insert different number of gpiochip. And I
do not want to fix to 3 gpiochips.
And it is not the name of each pin. It is the name of each gpiochip
(cntr->gc.label = name).
> I think 32 is more typical, but there may be a point in having some
> odd number.
Do you mean the gc->names? I do not support it at this time. It seems
that the code of handling gc->names(aka gpiochip_set_desc_names) is
not important. How about do it later?
> 
>> +
>> +static int
>> +mockup_gpio_get(struct gpio_chip *gc, unsigned offset)
>> +{
>> +       struct mockup_gpio_controller *cntr = container_of(gc,
>> +                       struct mockup_gpio_controller, gc);
>> +
>> +       return cntr->stats[offset].value;
> 
> This should still work with a bool there I think.
Ok. 
> 
>> +static void
>> +mockup_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
>> +{
>> +       struct mockup_gpio_controller *cntr = container_of(gc,
>> +                       struct mockup_gpio_controller, gc);
>> +
>> +       if (value)
>> +               cntr->stats[offset].value = 1;
>> +       else
>> +               cntr->stats[offset].value = 0;
> 
> Assign like this to do boolean clamping:
> 
> cntr->stats[offset].value = !!value;
Got you.
> 
> 
>> +int mockup_gpio_add(struct device *dev, struct mockup_gpio_controller *cntr,
>> +               const char *name, int base, int ngpio)
>> +{
>> +       int ret;
>> +
>> +       cntr->gc.base = base;
> 
> I don't think base should be specified for the test chip.
> We should set this to -1 and get a dynamic base, whatever
> it is. 
Fix base to -1 means we could only test the path of code in dynamic
gpio base allocation. I had look at the drivers in drivers/gpio,
it seems that the dynamic allocation is not usual. So, I feel test
with the base gave by user(dt, acpi or something else) is useful.
> Then the test scripts should deal with that.

> 
> Doing this also makes the conf array go away.
> 
>> +static int
>> +mockup_gpio_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct mockup_gpio_controller *cntr;
>> +       int ret;
>> +       int i, j;
>> +       int base;
>> +       int ngpio;
>> +
>> +       if (params_nr == 0) {
>> +               params_nr = 2;
>> +               conf[0] = 0;
>> +               conf[1] = 32;
>> +       }
> 
> This needs explaining. Why do we want 2 test gpio chips by default?
> Why not just one?
Oh, it is too tricky. The default 2 parameter means one gpiochip(one
base and one ngpio). It is indeed just one gpiochip by default.
When I add the module parameter, I was thinking I should pass the
parameter in one array(base1, ngpio1, base2, ngpio2 ...) or two arrays
((base1, base2, ...), (ngpio1, ngpio2, ...). Having two arrays is more
clear but hard to read in test script.
> 
> Why is the base of the second one 32 and not 5 since there is 5 pins
> on each chip?
The '5' is the series of name of each gpio label.
> 
> I would think adding just one chip or two with very varying characteristics
> is the way to go.
Like I mentioned above, one, two or more is different cases.
> Like one 32 line chip and one 5 line chip or
> something.
Yes, it is what I am doing in the script. I will insert one, two or
more gpiochip with different number of lines each.
> 
>> +
>> +       cntr = devm_kzalloc(dev, sizeof(*cntr) * params_nr, GFP_KERNEL);
>> +       if (!cntr)
>> +               return -ENOMEM;
>> +
>> +       platform_set_drvdata(pdev, cntr);
>> +
>> +       for (i = 0; i < params_nr >> 1; i++) {
>> +               base = conf[i << 1];
>> +               ngpio = conf[(i << 1) + 1];
> 
> These should go away I think.
> 
> The rest looks good.
Thanks

Bamvor
> 
> Yours,
> Linus Walleij
> 

Re: [PATCH RFD 2/3] selftest/gpio: add gpio test case

[Bamvor Jian Zhang]

Hi, Linus

On 11/26/2015 05:50 PM, Linus Walleij wrote:
> On Sat, Nov 14, 2015 at 9:51 AM, Bamvor Jian Zhang
> <bamvor.zhangjian@linaro.org> wrote:
> 
>> This test script try to do whitebox testing for gpio subsystem(
>> based on gpiolib). It manipulate gpio-mockup device through sysfs
>> provided and check the result from debugfs.
>>
>> Test the following things:
>> 1.  Add single, multi gpiochip with the checking of overlap.
>> 2.  Test direction and output value for valid pin.
>> 3.  Test dynamic allocation of gpio base.
>>
>> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
> 
> I like what this test is doing, but at some point we will need tests
> written in C.
> 
> The reason is that we are working on a character device for gpio,
> and once we have that, we want to be able to run these tests using
> the same program.
Understand, Written in c is easy to support both of them.
> But maybe this suffice for the sysfs tests of the current ABI.
Yeap.
> 
>> +++ b/tools/testing/selftests/gpio/gpio.sh
> 
> Rename it gpio-mockup-sysfs.sh please.
> 
>> +#!/bin/bash
>> +
>> +module="gpio-mockup"
> 
> This is ABI so not changing, you can just encode it into the
> program, but OK.
I was thinking if we could use this script to test gpio hardware
which support the 'standard' debugfs interface in gpiolib. It
rely on the feature that gpio could read back the output value.
I assume that lots of hw support this feature, if so, we could
use this script this gpio framework and gpio hw at the same
time.
> 
>> +
>> +SYSFS=
>> +GPIO_SYSFS=
>> +GPIO_DRV_SYSFS=
>> +DEBUGFS=
>> +GPIO_DEBUGFS=
>> +
>> +prerequisite()
>> +{
>> +       msg="skip all tests:"
>> +
>> +       if [ $UID != 0 ]; then
>> +               echo $msg must be run as root >&2
>> +               exit 0
>> +       fi
>> +
>> +       SYSFS=`mount -t sysfs | head -1 | awk '{ print $3 }'`
>> +       if [ ! -d "$SYSFS" ]; then
>> +               echo $msg sysfs is not mounted >&2
>> +               exit 0
>> +       fi
>> +       GPIO_SYSFS=`echo $SYSFS/class/gpio`
>> +       GPIO_DRV_SYSFS=`echo $SYSFS/devices/platform/$module/gpio`
>> +
>> +       DEBUGFS=`mount -t debugfs | head -1 | awk '{ print $3 }'`
>> +       if [ ! -d "$DEBUGFS" ]; then
>> +               echo $msg debugfs is not mounted >&2
>> +               exit 0
>> +       fi
>> +       GPIO_DEBUGFS=`echo $DEBUGFS/gpio`
>> +
>> +}
> 
> Looks good.
> 
>> +insert_module()
>> +{
>> +       if modprobe -q $module $1; then
>> +               echo -n ""
>> +       else
>> +               echo $msg insmod $module failed >&2
>> +               exit 0
>> +       fi
>> +}
>> +
>> +remove_module()
>> +{
>> +       modprobe -r -q $module
>> +}
>> +
>> +die()
>> +{
>> +       remove_module
>> +       exit 1
>> +}
> 
> So this inserting/removing modules work fine unless you want to
> run the tests with the module compiled-in. We should support that
> usecase too. So these functions must bail out silently if the module
> is already loaded (compiled in).
Yeap, understand. I try to do this by passing the '-q' to modprobe.
"With this flag, modprobe won't print an error message if you try to
remove or insert a module it can't find (and isn't an alias or
install/remove command).".
> 
>> +gpio_test()
>> +{
>> +       param=$1
>> +       if [ X$2 != X ]; then
>> +               invalid_pin=$2
>> +       fi
>> +       if [ X$param = X ]; then
>> +               insert_module
>> +       else
>> +               insert_module "conf=$param"
>> +       fi
> 
> As mentioned in the previous patch I'm suspicious about the parameters.
> Let's just have one or two mock chips and cut down on the
> parameterization.
Maybe we could discuss it in the previous patch?
> 
>> +       echo -n "GPIO $module test with base,ngpio configure<"
>> +       if [ X$param = X ]; then
>> +               echo "(default)>: ";
>> +       else
>> +               echo "$param>: "
>> +       fi
>> +       printf "%-10s %-5s %-5s\n" name base ngpio
>> +       gpiochip=`ls -d $GPIO_DRV_SYSFS/gpiochip* 2>/dev/null`
>> +       if [ X"$gpiochip" = X ]; then
>> +               echo "no gpiochip"
>> +       else
>> +               for chip in $gpiochip; do
>> +                       name=`basename $chip`
>> +                       base=`cat $chip/base`
>> +                       ngpio=`cat $chip/ngpio`
> 
> This is nice, because it relies on the chip informing us about the
> base and ngpio, meaning we can use a dynamic base, as pointed
> out in the previous patch.
yes.
>> +#test dynamic allocation of gpio
>> +gpio_test "-1,32"
>> +gpio_test "-1,32,32,16" 31
>> +gpio_test "-1,32,40,16,-1,5" 38
> 
> I think these are the only tests we should have, really.
Nope. If we only use the dynamic base, we could not test the corner
case in gpiochip_add_to_list. How about we mix the dynamic and
static base: insert a dynamic gpiochip and read back the base
allocated, then insert a static base to do the overlap test.
Is it make sense to you?
> 
> Using base 0 as some tests are doing is not going to work on
> any system that already has a built-in GPIO (SoC) controller.
> I think it is better to just use and test dynamic allocations.
Oh, yes. Insert base 0 without condition may fail in above case.
How about read back the current gpio range. And find out the
empty range to test? I could default to test the range in the
middle of the [0, MAX].

Regards

Bamvor
> 
> Yours,
> Linus Walleij
>