RE: Enable buttons GPIO passthrough

["Wang, Kuiying" <kuiying.wang@xxxxxxxxx>]

Hi Linus,
I got your points.
Below patch is based gpio character device and extend .set_config() property to support pass-through. If you have more comments please feel free to reply me.
And I plan to add passthrough setting for gpioset of libgpiod, patch is attached. If you agree I will upstream it together w/ this pass-through patch.

BTW: I have 2 questions hope could get your answer:
	1. why we have to switch to gpio character device and what's the advantage of it?
	2. As you mentioned " sysfs ABI is deprecated since over three years ", why sysfs ABI is still in use and most of cases are still using sysfs ABI? And when sysfs ABI is removed totally?
	    Please share all the related information (including web page/video/PPT/etc.)  ASAP. :)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 2342e154029b..bd917dca48a6 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -988,12 +988,19 @@ static int set_debounce(struct gpio_chip *chip, unsigned int offset,
 	return disable_debounce(chip, offset);
 }
 
+static int aspeed_gpio_pass_through(struct gpio_chip *chip, unsigned int offset,
+				    unsigned long usecs)
+{
+	printk("kwin::aspeed_gpio_pass_through is called");
+	return 0;
+}
+
 static int aspeed_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 				  unsigned long config)
 {
 	unsigned long param = pinconf_to_config_param(config);
 	u32 arg = pinconf_to_config_argument(config);
-
+	printk("kwin:: aspeed_gpio_set_config");
 	if (param == PIN_CONFIG_INPUT_DEBOUNCE)
 		return set_debounce(chip, offset, arg);
 	else if (param == PIN_CONFIG_BIAS_DISABLE ||
@@ -1006,6 +1013,8 @@ static int aspeed_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 		return -ENOTSUPP;
 	else if (param == PIN_CONFIG_PERSIST_STATE)
 		return aspeed_gpio_reset_tolerance(chip, offset, arg);
+	else if (param == PIN_CONFIG_PASS_THROUGH)
+		return aspeed_gpio_pass_through((chip, offset, arg);
 
 	return -ENOTSUPP;
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a8e01d99919c..1d2a2f0eb144 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -579,6 +579,8 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 			set_bit(FLAG_OPEN_DRAIN, &desc->flags);
 		if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
 			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+		if (lflags & GPIOHANDLE_REQUEST_PASS_THROUGH)
+			set_bit(FLAG_PASS_THROUGH, &desc->flags);
 
 		ret = gpiod_set_transitory(desc, false);
 		if (ret < 0)
@@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 			ret = gpiod_direction_input(desc);
 			if (ret)
 				goto out_free_descs;
+		} else if (lflags & GPIOHANDLE_REQUEST_PASS_THROUGH) {
+			ret = gpiod_direction_pass_through(desc);
+			if (ret)
+				goto out_free_descs;
 		}
 		dev_dbg(&gdev->dev, "registered chardev handle for line %d\n",
 			offset);
@@ -2643,6 +2649,41 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_output);
 
+/**
+ * gpiod_direction_pass_through - set the GPIO direction to pass-through
+ * @desc:	GPIO to set to pass-through
+ *
+ * Set the direction of the passed GPIO to passthrough.
+ *
+ * Return 0 in case of success, else an error code.
+ */
+int gpiod_direction_pass_through(struct gpio_desc *desc)
+{
+	struct gpio_chip *gc;
+	printk("kwin:: gpiod_direction_pass_through");
+	VALIDATE_DESC(desc);
+	/* GPIOs used for IRQs shall not be set as pass-through */
+	if (test_bit(FLAG_USED_AS_IRQ, &desc->flags)) {
+		gpiod_err(desc,
+			  "%s: tried to set a GPIO tied to an IRQ as pass-through\n",
+			  __func__);
+		return -EIO;
+	}
+	gc = desc->gdev->chip;
+	if (test_bit(FLAG_PASS_THROUGH, &desc->flags)) {
+		gpio_set_drive_single_ended(gc, gpio_chip_hwgpio(desc),
+					    PIN_CONFIG_PASS_THROUGH);
+	} else {
+		gpiod_err(desc,
+			  "%s: desc->flags is not set to FLAG_PASS_THROUGH\n",
+			  __func__);
+		return -EIO;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpiod_direction_pass_through);
+
 /**
  * gpiod_set_debounce - sets @debounce time for a GPIO
  * @desc: descriptor of the GPIO for which to set debounce time
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index a7e49fef73d4..b143ee47870a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -210,6 +210,7 @@ struct gpio_desc {
 #define FLAG_IS_OUT	1
 #define FLAG_EXPORT	2	/* protected by sysfs_lock */
 #define FLAG_SYSFS	3	/* exported via /sys/class/gpio/control */
+#define FLAG_PASS_THROUGH 4 /*Gpio is passthrough type*/
 #define FLAG_ACTIVE_LOW	6	/* value has active low */
 #define FLAG_OPEN_DRAIN	7	/* Gpio is open drain type */
 #define FLAG_OPEN_SOURCE 8	/* Gpio is open source type */
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 21ddbe440030..f0231dd09a2c 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -99,6 +99,7 @@ void devm_gpiod_put_array(struct device *dev, struct gpio_descs *descs);
 int gpiod_get_direction(struct gpio_desc *desc);
 int gpiod_direction_input(struct gpio_desc *desc);
 int gpiod_direction_output(struct gpio_desc *desc, int value);
+int gpiod_direction_pass_through(struct gpio_desc *desc);
 int gpiod_direction_output_raw(struct gpio_desc *desc, int value);
 
 /* Value get/set from non-sleeping context */
@@ -314,6 +315,12 @@ static inline int gpiod_direction_output(struct gpio_desc *desc, int value)
 	WARN_ON(1);
 	return -ENOSYS;
 }
+static inline int gpiod_direction_pass_through(struct gpio_desc *desc)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+	return -ENOSYS;
+}
 static inline int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
 {
 	/* GPIO can never have been requested */
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 6c0680641108..b2cdea81d6c9 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -124,6 +124,7 @@ enum pin_config_param {
 	PIN_CONFIG_SLEW_RATE,
 	PIN_CONFIG_SKEW_DELAY,
 	PIN_CONFIG_PERSIST_STATE,
+	PIN_CONFIG_PASS_THROUGH,
 	PIN_CONFIG_END = 0x7F,
 	PIN_CONFIG_MAX = 0xFF,
 };
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 1bf6e6df084b..44d9876b4aa2 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -62,6 +62,7 @@ struct gpioline_info {
 #define GPIOHANDLE_REQUEST_ACTIVE_LOW	(1UL << 2)
 #define GPIOHANDLE_REQUEST_OPEN_DRAIN	(1UL << 3)
 #define GPIOHANDLE_REQUEST_OPEN_SOURCE	(1UL << 4)
+#define GPIOHANDLE_REQUEST_PASS_THROUGH (1UL << 5)
 
 /**
  * struct gpiohandle_request - Information about a GPIO handle request
--

diff --git a/aclocal.m4 b/aclocal.m4
index b0db596..a5e28ad 100644
--- a/aclocal.m4
+++ b/aclocal.m4
@@ -911,7 +911,7 @@ AS_VAR_IF([$1], [""], [$5], [$4])dnl
 # generated from the m4 files accompanying Automake X.Y.
 # (This private macro should not be called outside this file.)
 AC_DEFUN([AM_AUTOMAKE_VERSION],
-[am__api_version='1.15'
+[am__api_version='1.16'
 dnl Some users find AM_AUTOMAKE_VERSION and mistake it for a way to
 dnl require some minimum version.  Point them to the right macro.
 m4_if([$1], [1.15], [],
diff --git a/configure b/configure
index 3d44cd1..d218338 100755
--- a/configure
+++ b/configure
@@ -2477,7 +2477,7 @@ ac_configure="$SHELL $ac_aux_dir/configure"  # Please don't use this var.
 
 
 
-am__api_version='1.15'
+am__api_version='1.16'
 
 # Find a good install program.  We prefer a C program (faster),
 # so one script is as good as another.  But avoid the broken or
diff --git a/include/gpiod.h b/include/gpiod.h
index ccff977..0f935e6 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -121,6 +121,7 @@ typedef void (*gpiod_ctxless_set_value_cb)(void *);
  * @param offset The offset of the GPIO line.
  * @param value New value (0 or 1).
  * @param active_low The active state of this line - true if low.
+ * @param pass_through The pass-through state of the lines - true if enabled.
  * @param consumer Name of the consumer.
  * @param cb Optional callback function that will be called right after setting
  *           the value. Users can use this, for example, to pause the execution
@@ -129,7 +130,7 @@ typedef void (*gpiod_ctxless_set_value_cb)(void *);
  * @return 0 if the operation succeeds, -1 on error.
  */
 int gpiod_ctxless_set_value(const char *device, unsigned int offset, int value,
-			    bool active_low, const char *consumer,
+			    bool active_low, bool pass_through, const char *consumer,
 			    gpiod_ctxless_set_value_cb cb,
 			    void *data) GPIOD_API;
 
@@ -140,6 +141,7 @@ int gpiod_ctxless_set_value(const char *device, unsigned int offset, int value,
  * @param values Array of integers containing new values.
  * @param num_lines Number of lines, must be > 0.
  * @param active_low The active state of the lines - true if low.
+ * @param pass_through The pass-through state of the lines - true if enabled.
  * @param consumer Name of the consumer.
  * @param cb Optional callback function that will be called right after setting
  *           all values. Works the same as in ::gpiod_ctxless_set_value.
@@ -149,7 +151,7 @@ int gpiod_ctxless_set_value(const char *device, unsigned int offset, int value,
 int gpiod_ctxless_set_value_multiple(const char *device,
 				     const unsigned int *offsets,
 				     const int *values, unsigned int num_lines,
-				     bool active_low, const char *consumer,
+				     bool active_low, bool pass_through, const char *consumer,
 				     gpiod_ctxless_set_value_cb cb,
 				     void *data) GPIOD_API;
 
@@ -766,6 +768,8 @@ enum {
 	/**< Request the line(s) for reading the GPIO line state. */
 	GPIOD_LINE_REQUEST_DIRECTION_OUTPUT,
 	/**< Request the line(s) for setting the GPIO line state. */
+	GPIOD_LINE_REQUEST_DIRECTION_PASS_THROUGH,
+	/**< Request the line(s) for setting the GPIO line state. */
 	GPIOD_LINE_REQUEST_EVENT_FALLING_EDGE,
 	/**< Monitor both types of events. */
 	GPIOD_LINE_REQUEST_EVENT_RISING_EDGE,
@@ -784,6 +788,8 @@ enum {
 	/**< The line is an open-source port. */
 	GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW	= GPIOD_BIT(2),
 	/**< The active state of the line is low (high is the default). */
+	GPIOD_LINE_REQUEST_FLAG_PASS_THROUGH	= GPIOD_BIT(5),
+	/**< The line is a pass-through port*/
 };
 
 /**
diff --git a/src/lib/core.c b/src/lib/core.c
index 4f273e3..74139a0 100644
--- a/src/lib/core.c
+++ b/src/lib/core.c
@@ -617,7 +617,8 @@ static bool line_request_is_direction(int request)
 {
 	return request == GPIOD_LINE_REQUEST_DIRECTION_AS_IS ||
 	       request == GPIOD_LINE_REQUEST_DIRECTION_INPUT ||
-	       request == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT;
+	       request == GPIOD_LINE_REQUEST_DIRECTION_OUTPUT ||
+	       request == GPIOD_LINE_REQUEST_DIRECTION_PASS_THROUGH;
 }
 
 static bool line_request_is_events(int request)
diff --git a/src/lib/ctxless.c b/src/lib/ctxless.c
index 0009504..c48e739 100644
--- a/src/lib/ctxless.c
+++ b/src/lib/ctxless.c
@@ -76,17 +76,17 @@ int gpiod_ctxless_get_value_multiple(const char *device,
 }
 
 int gpiod_ctxless_set_value(const char *device, unsigned int offset, int value,
-			    bool active_low, const char *consumer,
+			    bool active_low, bool pass_through, const char *consumer,
 			    gpiod_ctxless_set_value_cb cb, void *data)
 {
 	return gpiod_ctxless_set_value_multiple(device, &offset, &value, 1,
-						active_low, consumer, cb, data);
+						active_low, pass_through, consumer, cb, data);
 }
 
 int gpiod_ctxless_set_value_multiple(const char *device,
 				     const unsigned int *offsets,
 				     const int *values, unsigned int num_lines,
-				     bool active_low, const char *consumer,
+				     bool active_low, bool pass_through, const char *consumer,
 				     gpiod_ctxless_set_value_cb cb, void *data)
 {
 	struct gpiod_line_bulk bulk;
@@ -117,6 +117,7 @@ int gpiod_ctxless_set_value_multiple(const char *device,
 	}
 
 	flags = active_low ? GPIOD_LINE_REQUEST_FLAG_ACTIVE_LOW : 0;
+	flags |= pass_through ? GPIOD_LINE_REQUEST_FLAG_PASS_THROUGH : 0;
 
 	status = gpiod_line_request_bulk_output_flags(&bulk, consumer,
 						      flags, values);
diff --git a/src/tools/gpioset.c b/src/tools/gpioset.c
index fb012fa..d5f0b77 100644
--- a/src/tools/gpioset.c
+++ b/src/tools/gpioset.c
@@ -22,7 +22,8 @@
 static const struct option longopts[] = {
 	{ "help",		no_argument,		NULL,	'h' },
 	{ "version",		no_argument,		NULL,	'v' },
-	{ "active-low",		no_argument,		NULL,	'l' },
+	{ "active-low",		no_argument,		NULL,   'l' },
+	{ "pass-through",	no_argument,		NULL,	'p' },
 	{ "mode",		required_argument,	NULL,	'm' },
 	{ "sec",		required_argument,	NULL,	's' },
 	{ "usec",		required_argument,	NULL,	'u' },
@@ -30,7 +31,7 @@ static const struct option longopts[] = {
 	{ GETOPT_NULL_LONGOPT },
 };
 
-static const char *const shortopts = "+hvlm:s:u:b";
+static const char *const shortopts = "+hvlpm:s:u:b";
 
 static void print_help(void)
 {
@@ -40,8 +41,9 @@ static void print_help(void)
 	printf("\n");
 	printf("Options:\n");
 	printf("  -h, --help:\t\tdisplay this message and exit\n");
-	printf("  -v, --version:\tdisplay the version and exit\n");
 	printf("  -l, --active-low:\tset the line active state to low\n");
+	printf("  -v, --version:\tdisplay the version and exit\n");
+	printf("  -p, --pass-through:\tset it to pass through mode\n");
 	printf("  -m, --mode=[exit|wait|time|signal] (defaults to 'exit'):\n");
 	printf("		tell the program what to do after setting values\n");
 	printf("  -s, --sec=SEC:\tspecify the number of seconds to wait (only valid for --mode=time)\n");
@@ -179,6 +181,7 @@ int main(int argc, char **argv)
 	int *values, status, optc, opti;
 	struct callback_data cbdata;
 	bool active_low = false;
+	bool pass_through = false;
 	char *device, *end;
 
 	memset(&cbdata, 0, sizeof(cbdata));
@@ -197,6 +200,8 @@ int main(int argc, char **argv)
 			return EXIT_SUCCESS;
 		case 'l':
 			active_low = true;
+		case 'p':
+			pass_through = true;
 			break;
 		case 'm':
 			mode = parse_mode(optarg);
@@ -263,7 +268,7 @@ int main(int argc, char **argv)
 	}
 
 	status = gpiod_ctxless_set_value_multiple(device, offsets, values,
-						  num_lines, active_low,
+						  num_lines, active_low, pass_through,
 						  "gpioset", mode->callback,
 						  &cbdata);
 	if (status < 0)
diff --git a/tests/tests-ctxless.c b/tests/tests-ctxless.c
index ea9403d..228c49d 100644
--- a/tests/tests-ctxless.c
+++ b/tests/tests-ctxless.c
@@ -20,7 +20,7 @@ static void ctxless_set_get_value(void)
 	TEST_ASSERT_EQ(ret, 0);
 
 	ret = gpiod_ctxless_set_value(test_chip_name(0), 3, 1,
-				      false, TEST_CONSUMER, NULL, NULL);
+				      false, false, TEST_CONSUMER, NULL, NULL);
 	TEST_ASSERT_RET_OK(ret);
 
 	ret = gpiod_ctxless_get_value(test_chip_name(0), 3,


Thanks,
Kwin.


-----Original Message-----
From: Linus Walleij [mailto:linus.walleij@xxxxxxxxxx] 
Sent: Monday, January 28, 2019 9:25 PM
To: Wang, Kuiying <kuiying.wang@xxxxxxxxx>
Cc: Joel Stanley <joel@xxxxxxxxx>; Andrew Jeffery <andrew@xxxxxxxx>; Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxx>; open list:GPIO SUBSYSTEM <linux-gpio@xxxxxxxxxxxxxxx>; Andrew Geissler <geissonator@xxxxxxxxx>; OpenBMC Maillist <openbmc@xxxxxxxxxxxxxxxx>; Mauery, Vernon <vernon.mauery@xxxxxxxxx>; Feist, James <james.feist@xxxxxxxxx>; Yoo, Jae Hyun <jae.hyun.yoo@xxxxxxxxx>; Nguyen, Hai V <hai.v.nguyen@xxxxxxxxx>; Khetan, Sharad <sharad.khetan@xxxxxxxxx>
Subject: Re: Enable buttons GPIO passthrough

Hi Kwin!

On Tue, Jan 22, 2019 at 11:39 AM Wang, Kuiying <kuiying.wang@xxxxxxxxx> wrote:
>
> Hi Linus,
> Let me attach a draft patch, it will be easy to understand.
>
> if someone wanna enable passthrough just need to override like " gpio->chip.direction_passthrough = aspeed_gpio_dir_passthrough;"
> else passthrough is disabled.
>
> In app level, just need to echo "passthrough" to enable passthrough like "echo passthrough > /sys/class/gpio/gpio35/direction"

Sorry, but the sysfs ABI is deprecated since over three years and we will not add any new interesting features to it. Most major distributions don't even compile it in to the kernel anymore.

Your only option to control anything like this from userspace would be through the new gpio character device ABI, see examples in tools/gpio to see how to use this new ABI.

>  static int aspeed_gpio_get_direction(struct gpio_chip *gc, unsigned 
> int offset)  {
>         struct aspeed_gpio *gpio = gpiochip_get_data(gc); @@ -1188,6 
> +1215,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
>         gpio->chip.parent = &pdev->dev;
>         gpio->chip.direction_input = aspeed_gpio_dir_in;
>         gpio->chip.direction_output = aspeed_gpio_dir_out;
> +       gpio->chip.direction_passthrough = 
> + aspeed_gpio_dir_passthrough;

As stated earlier, do not add new callbacks for this. Use the existing .set_config, extend generic config options with a passthrough attribute.

> +       else if (sysfs_streq(buf, "passthrough"))
> +               status = gpiod_direction_passthrough(desc);

NACK on this sorry. This is the wrong design approach.

The Aspeed GPIO driver already contains a .set_config callback. Use this.

It is this function:

static int aspeed_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
                                  unsigned long config) {
        unsigned long param = pinconf_to_config_param(config);
        u32 arg = pinconf_to_config_argument(config);

        if (param == PIN_CONFIG_INPUT_DEBOUNCE)
                return set_debounce(chip, offset, arg);
        else if (param == PIN_CONFIG_BIAS_DISABLE ||
                        param == PIN_CONFIG_BIAS_PULL_DOWN ||
                        param == PIN_CONFIG_DRIVE_STRENGTH)
                return pinctrl_gpio_set_config(offset, config);
        else if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN ||
                        param == PIN_CONFIG_DRIVE_OPEN_SOURCE)
                /* Return -ENOTSUPP to trigger emulation, as per datasheet */
                return -ENOTSUPP;
        else if (param == PIN_CONFIG_PERSIST_STATE)
                return aspeed_gpio_reset_tolerance(chip, offset, arg);

Here add

else if (param == PIN_CONFIG_PASSTHROUGH)
   ....

And work from there.

Yours,
Linus Walleij