[RFC PATCH 0/5] gpio: Expose reset tolerance capability

[RFC PATCH 0/5] gpio: Expose reset tolerance capability

[Andrew Jeffery]

Hello,

This series exposes a "reset tolerant" property for GPIOs. For example, the
controller implemented in Aspeed BMCs provides such a feature to allow the BMC
to be reset whilst maintaining necessary state to keep host systems alive or
status LEDs in-tact.

I'm sending it as an RFC because I'm not sure using pinconf is the right way
to go about it, or that expanding the sysfs interface is a good idea, or that
the approach taken is right in the context of the existing suspend support.
pinconf just ended up being a convenient abstraction whilst supporting the
sysfs change, and didn't feel unreasonable to use for devicetree or the chardev
interface either. My concern with using pinconf is that the reset-tolerant
property is (currently) GPIO-centric, but maybe that's not a worry.

So the patches in the series support configuring the property via devicetree,
the chardev interface and the sysfs interface. The sysfs interface also exposes
the ability to configure the suspend tolerance, though there are some ordering
requirements with respect to setting the direction (the suspend tolerance will
only take if configured before setting the pin direction on the Arizona
controller).

Please review!

Cheers,

Andrew

Andrew Jeffery (5):
  gpio: gpiolib: Add core support for maintaining GPIO values on reset
  gpio: gpiolib: Add OF support for maintaining GPIO values on reset
  gpio: gpiolib: Add chardev support for maintaining GPIO values on
    reset
  gpio: gpiolib: Add sysfs support for maintaining GPIO values on reset
  gpio: aspeed: Add support for reset tolerance

 Documentation/gpio/sysfs.txt            |  9 ++++
 drivers/gpio/gpio-arizona.c             |  4 +-
 drivers/gpio/gpio-aspeed.c              | 39 ++++++++++++++-
 drivers/gpio/gpiolib-of.c               |  2 +
 drivers/gpio/gpiolib-sysfs.c            | 88 +++++++++++++++++++++++++++++++--
 drivers/gpio/gpiolib.c                  | 74 +++++++++++++++++++++++++--
 drivers/gpio/gpiolib.h                  |  1 +
 include/dt-bindings/gpio/gpio.h         |  4 ++
 include/linux/gpio/consumer.h           |  9 ++++
 include/linux/gpio/driver.h             |  5 +-
 include/linux/gpio/machine.h            |  2 +
 include/linux/of_gpio.h                 |  1 +
 include/linux/pinctrl/pinconf-generic.h |  2 +
 include/uapi/linux/gpio.h               | 11 +++--
 14 files changed, 234 insertions(+), 17 deletions(-)

-- 
2.11.0

[RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset

[Andrew Jeffery]

GPIO state reset tolerance is implemented in gpiolib through the
addition of a new pinconf parameter. With that, some renaming of helpers
is done to clarify the scope of the already existing
gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
means on suspend, reset or both. This in-turn impacts gpio-arizona, but
not in any complicated way.

This change lays the groundwork for implementing reset tolerance support
in all of the external interfaces that can influence GPIOs.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/gpio/gpio-arizona.c             |  4 +--
 drivers/gpio/gpiolib.c                  | 55 +++++++++++++++++++++++++++++++--
 drivers/gpio/gpiolib.h                  |  1 +
 include/linux/gpio/consumer.h           |  9 ++++++
 include/linux/gpio/driver.h             |  5 ++-
 include/linux/gpio/machine.h            |  2 ++
 include/linux/pinctrl/pinconf-generic.h |  2 ++
 7 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-arizona.c b/drivers/gpio/gpio-arizona.c
index d4e6ba0301bc..d3fe23569811 100644
--- a/drivers/gpio/gpio-arizona.c
+++ b/drivers/gpio/gpio-arizona.c
@@ -33,7 +33,7 @@ static int arizona_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
 {
 	struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
 	struct arizona *arizona = arizona_gpio->arizona;
-	bool persistent = gpiochip_line_is_persistent(chip, offset);
+	bool persistent = gpiochip_line_is_persistent_suspend(chip, offset);
 	bool change;
 	int ret;
 
@@ -99,7 +99,7 @@ static int arizona_gpio_direction_out(struct gpio_chip *chip,
 {
 	struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
 	struct arizona *arizona = arizona_gpio->arizona;
-	bool persistent = gpiochip_line_is_persistent(chip, offset);
+	bool persistent = gpiochip_line_is_persistent_suspend(chip, offset);
 	unsigned int val;
 	int ret;
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a56b29fd8bb1..d9dc7e588699 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2414,6 +2414,40 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 EXPORT_SYMBOL_GPL(gpiod_set_debounce);
 
 /**
+ * gpiod_set_reset_tolerant - Hold state across controller reset
+ * @desc: descriptor of the GPIO for which to set debounce time
+ * @tolerant: True to hold state across a controller reset, false otherwise
+ *
+ * Returns:
+ * 0 on success, %-ENOTSUPP if the controller doesn't support setting the
+ * reset tolerance or less than zero on other failures.
+ */
+int gpiod_set_reset_tolerant(struct gpio_desc *desc, bool tolerant)
+{
+	struct gpio_chip *chip;
+	unsigned long packed;
+	int rc;
+
+	chip = desc->gdev->chip;
+	if (!chip->set_config)
+		return -ENOTSUPP;
+
+	packed = pinconf_to_config_packed(PIN_CONFIG_RESET_TOLERANT, tolerant);
+
+	rc = chip->set_config(chip, gpio_chip_hwgpio(desc), packed);
+	if (rc < 0)
+		return rc;
+
+	if (tolerant)
+		set_bit(FLAG_RESET_TOLERANT, &desc->flags);
+	else
+		clear_bit(FLAG_RESET_TOLERANT, &desc->flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpiod_set_reset_tolerant);
+
+/**
  * gpiod_is_active_low - test whether a GPIO is active-low or not
  * @desc: the gpio descriptor to test
  *
@@ -2885,7 +2919,8 @@ bool gpiochip_line_is_open_source(struct gpio_chip *chip, unsigned int offset)
 }
 EXPORT_SYMBOL_GPL(gpiochip_line_is_open_source);
 
-bool gpiochip_line_is_persistent(struct gpio_chip *chip, unsigned int offset)
+bool gpiochip_line_is_persistent_suspend(struct gpio_chip *chip,
+					 unsigned int offset)
 {
 	if (offset >= chip->ngpio)
 		return false;
@@ -2893,7 +2928,18 @@ bool gpiochip_line_is_persistent(struct gpio_chip *chip, unsigned int offset)
 	return !test_bit(FLAG_SLEEP_MAY_LOSE_VALUE,
 			 &chip->gpiodev->descs[offset].flags);
 }
-EXPORT_SYMBOL_GPL(gpiochip_line_is_persistent);
+EXPORT_SYMBOL_GPL(gpiochip_line_is_persistent_suspend);
+
+bool gpiochip_line_is_persistent_reset(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	if (offset >= chip->ngpio)
+		return false;
+
+	return test_bit(FLAG_RESET_TOLERANT,
+			&chip->gpiodev->descs[offset].flags);
+}
+EXPORT_SYMBOL_GPL(gpiochip_line_is_persistent_reset);
 
 /**
  * gpiod_get_raw_value_cansleep() - return a gpio's raw value
@@ -3271,6 +3317,11 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 	if (lflags & GPIO_SLEEP_MAY_LOSE_VALUE)
 		set_bit(FLAG_SLEEP_MAY_LOSE_VALUE, &desc->flags);
 
+	status = gpiod_set_reset_tolerant(desc,
+					  !!(lflags & GPIO_RESET_TOLERANT));
+	if (status < 0)
+		return status;
+
 	/* No particular flag request, return here... */
 	if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
 		pr_debug("no flags found for %s\n", con_id);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 799208fc189a..4ce36f14c1ad 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -202,6 +202,7 @@ struct gpio_desc {
 #define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
 #define FLAG_IS_HOGGED	11	/* GPIO is hogged */
 #define FLAG_SLEEP_MAY_LOSE_VALUE 12	/* GPIO may lose value in sleep */
+#define FLAG_RESET_TOLERANT 13	/* GPIO to maintain state at shutdown */
 
 	/* Connection label */
 	const char		*label;
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 8f702fcbe485..ee5e125c8fec 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -121,6 +121,7 @@ void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 					int *value_array);
 
 int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
+int gpiod_set_reset_tolerant(struct gpio_desc *desc, bool tolerant);
 
 int gpiod_is_active_low(const struct gpio_desc *desc);
 int gpiod_cansleep(const struct gpio_desc *desc);
@@ -381,6 +382,14 @@ static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 	return -ENOSYS;
 }
 
+static inline int gpiod_set_reset_tolerant(struct gpio_desc *desc,
+					   bool tolerant)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+	return -ENOSYS;
+}
+
 static inline int gpiod_is_active_low(const struct gpio_desc *desc)
 {
 	/* GPIO can never have been requested */
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index c97f8325e8bf..6e9d32bdab55 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -233,7 +233,10 @@ bool gpiochip_line_is_open_drain(struct gpio_chip *chip, unsigned int offset);
 bool gpiochip_line_is_open_source(struct gpio_chip *chip, unsigned int offset);
 
 /* Sleep persistence inquiry for drivers */
-bool gpiochip_line_is_persistent(struct gpio_chip *chip, unsigned int offset);
+bool gpiochip_line_is_persistent_suspend(struct gpio_chip *chip,
+					 unsigned int offset);
+bool gpiochip_line_is_persistent_reset(struct gpio_chip *chip,
+				       unsigned int offset);
 
 /* get driver data */
 void *gpiochip_get_data(struct gpio_chip *chip);
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 5e9f294c29eb..11b7209c7ab9 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -11,6 +11,8 @@ enum gpio_lookup_flags {
 	GPIO_OPEN_SOURCE = (1 << 2),
 	GPIO_SLEEP_MAINTAIN_VALUE = (0 << 3),
 	GPIO_SLEEP_MAY_LOSE_VALUE = (1 << 3),
+	GPIO_RESET_INTOLERANT = (0 << 4),
+	GPIO_RESET_TOLERANT = (1 << 4),
 };
 
 /**
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 5d8bc7f21c2a..487cc863cb36 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -90,6 +90,7 @@
  * @PIN_CONFIG_SLEW_RATE: if the pin can select slew rate, the argument to
  *	this parameter (on a custom format) tells the driver which alternative
  *	slew rate to use.
+ * @PIN_CONFIG_RESET_TOLERANT: retains the pin state across a system reset
  * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
  *	you need to pass in custom configurations to the pin controller, use
  *	PIN_CONFIG_END+1 as the base offset.
@@ -117,6 +118,7 @@ enum pin_config_param {
 	PIN_CONFIG_POWER_SOURCE,
 	PIN_CONFIG_SLEEP_HARDWARE_STATE,
 	PIN_CONFIG_SLEW_RATE,
+	PIN_CONFIG_RESET_TOLERANT,
 	PIN_CONFIG_END = 0x7F,
 	PIN_CONFIG_MAX = 0xFF,
 };
-- 
2.11.0

[RFC PATCH 2/5] gpio: gpiolib: Add OF support for maintaining GPIO values on reset

[Andrew Jeffery]

Add flags and the associated flag mappings between interfaces to enable
GPIO reset tolerance to be specified via devicetree.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/gpio/gpiolib-of.c       | 2 ++
 drivers/gpio/gpiolib.c          | 5 +++++
 include/dt-bindings/gpio/gpio.h | 4 ++++
 include/linux/of_gpio.h         | 1 +
 4 files changed, 12 insertions(+)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index e0d59e61b52f..4a268ba52998 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -155,6 +155,8 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 
 	if (of_flags & OF_GPIO_SLEEP_MAY_LOSE_VALUE)
 		*flags |= GPIO_SLEEP_MAY_LOSE_VALUE;
+	if (of_flags & OF_GPIO_RESET_TOLERANT)
+		*flags |= GPIO_RESET_TOLERANT;
 
 	return desc;
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d9dc7e588699..6b4c5df10e84 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3434,6 +3434,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	bool active_low = false;
 	bool single_ended = false;
 	bool open_drain = false;
+	bool reset_tolerant = false;
 	int ret;
 
 	if (!fwnode)
@@ -3448,6 +3449,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 			active_low = flags & OF_GPIO_ACTIVE_LOW;
 			single_ended = flags & OF_GPIO_SINGLE_ENDED;
 			open_drain = flags & OF_GPIO_OPEN_DRAIN;
+			reset_tolerant = flags & OF_GPIO_RESET_TOLERANT;
 		}
 	} else if (is_acpi_node(fwnode)) {
 		struct acpi_gpio_info info;
@@ -3478,6 +3480,9 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 			lflags |= GPIO_OPEN_SOURCE;
 	}
 
+	if (reset_tolerant)
+		lflags |= GPIO_RESET_TOLERANT;
+
 	ret = gpiod_configure_flags(desc, propname, lflags, dflags);
 	if (ret < 0) {
 		gpiod_put(desc);
diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h
index 70de5b7a6c9b..01c75d9e308e 100644
--- a/include/dt-bindings/gpio/gpio.h
+++ b/include/dt-bindings/gpio/gpio.h
@@ -32,4 +32,8 @@
 #define GPIO_SLEEP_MAINTAIN_VALUE 0
 #define GPIO_SLEEP_MAY_LOSE_VALUE 8
 
+/* Bit 4 express GPIO persistence on reset */
+#define GPIO_RESET_INTOLERANT 0
+#define GPIO_RESET_TOLERANT 16
+
 #endif
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 1fe205582111..9b34737706a7 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -32,6 +32,7 @@ enum of_gpio_flags {
 	OF_GPIO_SINGLE_ENDED = 0x2,
 	OF_GPIO_OPEN_DRAIN = 0x4,
 	OF_GPIO_SLEEP_MAY_LOSE_VALUE = 0x8,
+	OF_GPIO_RESET_TOLERANT = 0x16,
 };
 
 #ifdef CONFIG_OF_GPIO
-- 
2.11.0

[RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset

[Andrew Jeffery]

Similar to devicetree support, add flags and mappings to expose reset
tolerance configuration through the chardev interface.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/gpio/gpiolib.c    | 14 +++++++++++++-
 include/uapi/linux/gpio.h | 11 ++++++-----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6b4c5df10e84..442ee5ceee08 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -357,7 +357,8 @@ struct linehandle_state {
 	GPIOHANDLE_REQUEST_OUTPUT | \
 	GPIOHANDLE_REQUEST_ACTIVE_LOW | \
 	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
-	GPIOHANDLE_REQUEST_OPEN_SOURCE)
+	GPIOHANDLE_REQUEST_OPEN_SOURCE | \
+	GPIOHANDLE_REQUEST_RESET_TOLERANT)
 
 static long linehandle_ioctl(struct file *filep, unsigned int cmd,
 			     unsigned long arg)
@@ -498,6 +499,17 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 
 		/*
+		 * Unconditionally configure reset tolerance, as it's possible
+		 * that the tolerance flag itself becomes tolerant to resets.
+		 * Thus it could remain set from a previous environment, but
+		 * the current environment may not expect it so.
+		 */
+		ret = gpiod_set_reset_tolerant(desc,
+				!!(lflags & GPIOHANDLE_REQUEST_RESET_TOLERANT));
+		if (ret < 0)
+			goto out_free_descs;
+
+		/*
 		 * Lines have to be requested explicitly for input
 		 * or output, else the line will be treated "as is".
 		 */
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 333d3544c964..1b1ce1af8653 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -56,11 +56,12 @@ struct gpioline_info {
 #define GPIOHANDLES_MAX 64
 
 /* Linerequest flags */
-#define GPIOHANDLE_REQUEST_INPUT	(1UL << 0)
-#define GPIOHANDLE_REQUEST_OUTPUT	(1UL << 1)
-#define GPIOHANDLE_REQUEST_ACTIVE_LOW	(1UL << 2)
-#define GPIOHANDLE_REQUEST_OPEN_DRAIN	(1UL << 3)
-#define GPIOHANDLE_REQUEST_OPEN_SOURCE	(1UL << 4)
+#define GPIOHANDLE_REQUEST_INPUT		(1UL << 0)
+#define GPIOHANDLE_REQUEST_OUTPUT		(1UL << 1)
+#define GPIOHANDLE_REQUEST_ACTIVE_LOW		(1UL << 2)
+#define GPIOHANDLE_REQUEST_OPEN_DRAIN		(1UL << 3)
+#define GPIOHANDLE_REQUEST_OPEN_SOURCE		(1UL << 4)
+#define GPIOHANDLE_REQUEST_RESET_TOLERANT	(1UL << 5)
 
 /**
  * struct gpiohandle_request - Information about a GPIO handle request
-- 
2.11.0

[RFC PATCH 4/5] gpio: gpiolib: Add sysfs support for maintaining GPIO values on reset

[Andrew Jeffery]

Expose a new 'maintain' sysfs attribute to control both suspend and
reset tolerance.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/gpio/sysfs.txt |  9 +++++
 drivers/gpio/gpiolib-sysfs.c | 88 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
index aeab01aa4d00..f447f0746884 100644
--- a/Documentation/gpio/sysfs.txt
+++ b/Documentation/gpio/sysfs.txt
@@ -96,6 +96,15 @@ and have the following read/write attributes:
 		for "rising" and "falling" edges will follow this
 		setting.
 
+	"maintain" ... displays and controls whether the state of the GPIO is
+		maintained or lost on suspend or reset. The valid values take
+	        the following meanings:
+
+	        0: Do not maintain state on either suspend or reset
+	        1: Maintain state for suspend only
+	        2: Maintain state for reset only
+	        3: Maintain state for both suspend and reset
+
 GPIO controllers have paths like /sys/class/gpio/gpiochip42/ (for the
 controller implementing GPIOs starting at #42) and have the following
 read-only attributes:
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 3f454eaf2101..bfa186e73e26 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -289,6 +289,74 @@ static ssize_t edge_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(edge);
 
+#define GPIOLIB_SYSFS_MAINTAIN_SUSPEND	BIT(0)
+#define GPIOLIB_SYSFS_MAINTAIN_RESET	BIT(1)
+#define GPIOLIB_SYSFS_MAINTAIN_ALL	GENMASK(1, 0)
+static ssize_t maintain_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	ssize_t	status = 0;
+	int val = 0;
+
+	mutex_lock(&data->mutex);
+
+	if (!test_bit(FLAG_SLEEP_MAY_LOSE_VALUE, &data->desc->flags))
+		val |= GPIOLIB_SYSFS_MAINTAIN_SUSPEND;
+
+	if (test_bit(FLAG_RESET_TOLERANT, &data->desc->flags))
+		val |= GPIOLIB_SYSFS_MAINTAIN_RESET;
+
+	status = sprintf(buf, "%d\n", val);
+
+	mutex_unlock(&data->mutex);
+
+	return status;
+}
+
+static ssize_t maintain_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf,
+			      size_t size)
+{
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_chip *chip;
+	ssize_t	status;
+	long provided;
+
+	mutex_lock(&data->mutex);
+
+	chip = data->desc->gdev->chip;
+
+	if (!chip->set_config)
+		return -ENOTSUPP;
+
+	status = kstrtol(buf, 0, &provided);
+	if (status < 0)
+		goto out;
+
+	if (provided & ~GPIOLIB_SYSFS_MAINTAIN_ALL) {
+		status = -EINVAL;
+		goto out;
+	}
+
+	if (!(provided & GPIOLIB_SYSFS_MAINTAIN_SUSPEND))
+		set_bit(FLAG_SLEEP_MAY_LOSE_VALUE, &data->desc->flags);
+	else
+		clear_bit(FLAG_SLEEP_MAY_LOSE_VALUE,
+			  &data->desc->flags);
+
+	/* Configure reset tolerance */
+	status = gpiod_set_reset_tolerant(data->desc,
+			!!(provided & GPIOLIB_SYSFS_MAINTAIN_RESET));
+out:
+	mutex_unlock(&data->mutex);
+
+	return status ? : size;
+
+}
+static DEVICE_ATTR_RW(maintain);
+
 /* Caller holds gpiod-data mutex. */
 static int gpio_sysfs_set_active_low(struct device *dev, int value)
 {
@@ -378,6 +446,7 @@ static struct attribute *gpio_attrs[] = {
 	&dev_attr_edge.attr,
 	&dev_attr_value.attr,
 	&dev_attr_active_low.attr,
+	&dev_attr_maintain.attr,
 	NULL,
 };
 
@@ -474,11 +543,22 @@ static ssize_t export_store(struct class *class,
 			status = -ENODEV;
 		goto done;
 	}
-	status = gpiod_export(desc, true);
-	if (status < 0)
+
+	/*
+	 * If userspace is requesting the GPIO via sysfs, make them explicitly
+	 * configure reset tolerance each time by unconditionally disabling it
+	 * here, as the export and configuration steps are not atomic.
+	 */
+	status = gpiod_set_reset_tolerant(desc, false);
+	if (status < 0) {
 		gpiod_free(desc);
-	else
-		set_bit(FLAG_SYSFS, &desc->flags);
+	} else {
+		status = gpiod_export(desc, true);
+		if (status < 0)
+			gpiod_free(desc);
+		else
+			set_bit(FLAG_SYSFS, &desc->flags);
+	}
 
 done:
 	if (status)
-- 
2.11.0

[RFC PATCH 5/5] gpio: aspeed: Add support for reset tolerance

[Andrew Jeffery]

Use the new pinconf parameter for reset tolerance to expose the
associated capability of the Aspeed GPIO controller.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/gpio/gpio-aspeed.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index bfc53995064a..0492cd917178 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -60,6 +60,7 @@ struct aspeed_gpio_bank {
 	uint16_t	val_regs;
 	uint16_t	irq_regs;
 	uint16_t	debounce_regs;
+	uint16_t	tolerance_regs;
 	const char	names[4][3];
 };
 
@@ -70,48 +71,56 @@ static const struct aspeed_gpio_bank aspeed_gpio_banks[] = {
 		.val_regs = 0x0000,
 		.irq_regs = 0x0008,
 		.debounce_regs = 0x0040,
+		.tolerance_regs = 0x001c,
 		.names = { "A", "B", "C", "D" },
 	},
 	{
 		.val_regs = 0x0020,
 		.irq_regs = 0x0028,
 		.debounce_regs = 0x0048,
+		.tolerance_regs = 0x003c,
 		.names = { "E", "F", "G", "H" },
 	},
 	{
 		.val_regs = 0x0070,
 		.irq_regs = 0x0098,
 		.debounce_regs = 0x00b0,
+		.tolerance_regs = 0x00ac,
 		.names = { "I", "J", "K", "L" },
 	},
 	{
 		.val_regs = 0x0078,
 		.irq_regs = 0x00e8,
 		.debounce_regs = 0x0100,
+		.tolerance_regs = 0x00fc,
 		.names = { "M", "N", "O", "P" },
 	},
 	{
 		.val_regs = 0x0080,
 		.irq_regs = 0x0118,
 		.debounce_regs = 0x0130,
+		.tolerance_regs = 0x012c,
 		.names = { "Q", "R", "S", "T" },
 	},
 	{
 		.val_regs = 0x0088,
 		.irq_regs = 0x0148,
 		.debounce_regs = 0x0160,
+		.tolerance_regs = 0x015c,
 		.names = { "U", "V", "W", "X" },
 	},
 	{
 		.val_regs = 0x01E0,
 		.irq_regs = 0x0178,
 		.debounce_regs = 0x0190,
+		.tolerance_regs = 0x018c,
 		.names = { "Y", "Z", "AA", "AB" },
 	},
 	{
-		.val_regs = 0x01E8,
-		.irq_regs = 0x01A8,
+		.val_regs = 0x01e8,
+		.irq_regs = 0x01a8,
 		.debounce_regs = 0x01c0,
+		.tolerance_regs = 0x01bc,
 		.names = { "AC", "", "", "" },
 	},
 };
@@ -531,6 +540,30 @@ static int aspeed_gpio_setup_irqs(struct aspeed_gpio *gpio,
 	return 0;
 }
 
+static int aspeed_gpio_reset_tolerance(struct gpio_chip *chip,
+					unsigned int offset, bool enable)
+{
+	struct aspeed_gpio *gpio = gpiochip_get_data(chip);
+	const struct aspeed_gpio_bank *bank;
+	unsigned long flags;
+	u32 val;
+
+	bank = to_bank(offset);
+
+	spin_lock_irqsave(&gpio->lock, flags);
+	val = readl(gpio->base + bank->tolerance_regs);
+
+	if (enable)
+		val |= GPIO_BIT(offset);
+	else
+		val &= ~GPIO_BIT(offset);
+
+	writel(val, gpio->base + bank->tolerance_regs);
+	spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return 0;
+}
+
 static int aspeed_gpio_request(struct gpio_chip *chip, unsigned int offset)
 {
 	if (!have_gpio(gpiochip_get_data(chip), offset))
@@ -768,6 +801,8 @@ static int aspeed_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 			param == PIN_CONFIG_DRIVE_OPEN_SOURCE)
 		/* Return -ENOTSUPP to trigger emulation, as per datasheet */
 		return -ENOTSUPP;
+	else if (param == PIN_CONFIG_RESET_TOLERANT)
+		return aspeed_gpio_reset_tolerance(chip, offset, arg);
 
 	return -ENOTSUPP;
 }
-- 
2.11.0

Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset

[Linus Walleij]

On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:

> GPIO state reset tolerance is implemented in gpiolib through the
> addition of a new pinconf parameter. With that, some renaming of helpers
> is done to clarify the scope of the already existing
> gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
> means on suspend, reset or both.

Isn't it most reasonable to say persistance covers both cases, reset
and/or sleep? This seems a bit like overdefined.

So can we say that is this flag is set, the hardware and driver should
do its best to preserve the value across any system disruptions.

We can change the wording of course, patches welcome for that.

But do we really need to distinguish the cases of disruption and
whether we cover up for them or not?

I would say we can deal with that the day we have a system with
two register bits (or similar) where you can select to preserve across
sleep, reset, one or the other, AND there is also a usecase such that
a user wants to preserve the value across reset but not suspend or
vice versa.

I suspect that will not happen.

Yours,
Linus Walleij

Re: [RFC PATCH 2/5] gpio: gpiolib: Add OF support for maintaining GPIO values on reset

[Linus Walleij]

On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:

> @@ -32,6 +32,7 @@ enum of_gpio_flags {
>         OF_GPIO_SINGLE_ENDED = 0x2,
>         OF_GPIO_OPEN_DRAIN = 0x4,
>         OF_GPIO_SLEEP_MAY_LOSE_VALUE = 0x8,
> +       OF_GPIO_RESET_TOLERANT = 0x16,

Now you're mixing up decimal and hex.

Anyways, I do not think this is necessary.

Yours,
Linus Walleij

Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset

[Linus Walleij]

I paged Bartosz and Michael on this, they are experts on the use cases for
the character device and their opinions are likely more valuable than mine.

On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> Similar to devicetree support, add flags and mappings to expose reset
> tolerance configuration through the chardev interface.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
(...)

> +                * Unconditionally configure reset tolerance, as it's possible
> +                * that the tolerance flag itself becomes tolerant to resets.
> +                * Thus it could remain set from a previous environment, but
> +                * the current environment may not expect it so.
> +                */
> +               ret = gpiod_set_reset_tolerant(desc,
> +                               !!(lflags & GPIOHANDLE_REQUEST_RESET_TOLERANT));
> +               if (ret < 0)
> +                       goto out_free_descs;

First, as noted in the first patch, IMO we should just go for persistance,
i.e. you want to flag to the system to keep the line persistent in any case,
no matter if the system goes to sleep or resets.

So the usecase is going to be a control system or similar, a makerspace
project, an industrial product of some kind, driving GPIO from userspace.

I don't see it as helpful to give userspace control over whether the line
is persistent or not. It is more reasonable to assume persistance for
userspace use cases, don't you think? Whether the system goes to sleep
or the gpiochip resets should not make a door suddenly close or the
lights in the christmas tree go out, right? I think if the gpiochip supports
persistance of any kind, we should try to use it and not have userspace
provide flags for that.

Yours,
Linus Walleij

Re: [RFC PATCH 4/5] gpio: gpiolib: Add sysfs support for maintaining GPIO values on reset

[Linus Walleij]

On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:

> Expose a new 'maintain' sysfs attribute to control both suspend and
> reset tolerance.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

NAK. You will find the actual ABI documentation in
Documentation/ABI/obsolete/sysfs-gpio
that's why. This is being phased out and should not be extended.
Everyone should use the character device, especially for new
functionality.

Yours,
Linus Walleij

Re: [RFC PATCH 2/5] gpio: gpiolib: Add OF support for maintaining GPIO values on reset

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]

On Fri, 2017-10-20 at 09:18 +0200, Linus Walleij wrote:
> On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au>
> wrote:
> 
> > @@ -32,6 +32,7 @@ enum of_gpio_flags {
> >         OF_GPIO_SINGLE_ENDED = 0x2,
> >         OF_GPIO_OPEN_DRAIN = 0x4,
> >         OF_GPIO_SLEEP_MAY_LOSE_VALUE = 0x8,
> > +       OF_GPIO_RESET_TOLERANT = 0x16,
> 
> Now you're mixing up decimal and hex.

Ugh. Whoops.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 4/5] gpio: gpiolib: Add sysfs support for maintaining GPIO values on reset

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On Fri, 2017-10-20 at 09:29 +0200, Linus Walleij wrote:
> On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au>
> wrote:
> 
> > Expose a new 'maintain' sysfs attribute to control both suspend and
> > reset tolerance.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> 
> NAK. You will find the actual ABI documentation in
> Documentation/ABI/obsolete/sysfs-gpio

Right, I did a quick grep to find an attribute description in order to
judge what documentation to change. Unfortunately my grep didn't pick
up this file.

> that's why. This is being phased out and should not be extended.
> Everyone should use the character device, especially for new
> functionality.

Yeah, I expected this (and the NAK) would be the response but figured I
should ask the question.

Thanks,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset

[Linus Walleij]

On Fri, Oct 20, 2017 at 9:17 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>> GPIO state reset tolerance is implemented in gpiolib through the
>> addition of a new pinconf parameter. With that, some renaming of helpers
>> is done to clarify the scope of the already existing
>> gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
>> means on suspend, reset or both.
>
> Isn't it most reasonable to say persistance covers both cases, reset
> and/or sleep? This seems a bit like overdefined.

I should also add: right now persistance is defined in negative terms,
you can supply the flag "may lose value", which means the subsystem
by default, and driver by default, will try to keep values persistent across
sleep.

Then it is possible to opt in for not doing so. (Usually to save power I
think.)

I think that especially for userspace use cases, saving power should
not really be the concern, but correct me if I'm wrong. I am thinking
of a box with a DC plug wired up to a factory line here.

What we have in the Arizona driver is an opt-in where the DT can
say "don't preserve the value  this line during system sleep" i.e. "lay lose
value" and we can extend that flag to mean "don't preserve this line
during reset either" but by default assume that we should.

Yours,
Linus Walleij

Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1550 bytes --]

On Fri, 2017-10-20 at 09:17 +0200, Linus Walleij wrote:
> > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> 
> > GPIO state reset tolerance is implemented in gpiolib through the
> > addition of a new pinconf parameter. With that, some renaming of helpers
> > is done to clarify the scope of the already existing
> > gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
> > means on suspend, reset or both.
> 
> Isn't it most reasonable to say persistance covers both cases, reset
> and/or sleep? This seems a bit like overdefined.

I definitely had some internal debate about that. I erred on the side of
avoiding potential change in expectations for the arizona. If you consider that
overdefined then I'm happy to go the other way.

> 
> So can we say that is this flag is set, the hardware and driver should
> do its best to preserve the value across any system disruptions.
> 
> We can change the wording of course, patches welcome for that.

Yep.

> 
> But do we really need to distinguish the cases of disruption and
> whether we cover up for them or not?
> 
> I would say we can deal with that the day we have a system with
> two register bits (or similar) where you can select to preserve across
> sleep, reset, one or the other, AND there is also a usecase such that
> a user wants to preserve the value across reset but not suspend or
> vice versa.
> 
> I suspect that will not happen.

A very reasonable approach.

Cheers for the feedback.

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

On Fri, 2017-10-20 at 09:43 +0200, Linus Walleij wrote:
> On Fri, Oct 20, 2017 at 9:17 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > 
> > > GPIO state reset tolerance is implemented in gpiolib through the
> > > addition of a new pinconf parameter. With that, some renaming of helpers
> > > is done to clarify the scope of the already existing
> > > gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
> > > means on suspend, reset or both.
> > 
> > Isn't it most reasonable to say persistance covers both cases, reset
> > and/or sleep? This seems a bit like overdefined.
> 
> I should also add: right now persistance is defined in negative terms,
> you can supply the flag "may lose value", which means the subsystem
> by default, and driver by default, will try to keep values persistent across
> sleep.
> 
> Then it is possible to opt in for not doing so. (Usually to save power I
> think.)
> 
> I think that especially for userspace use cases, saving power should
> not really be the concern, but correct me if I'm wrong. I am thinking
> of a box with a DC plug wired up to a factory line here.
> 
> What we have in the Arizona driver is an opt-in where the DT can
> say "don't preserve the value  this line during system sleep" i.e. "lay lose
> value" and we can extend that flag to mean "don't preserve this line
> during reset either" but by default assume that we should.

Yeah, the preserve polarity was another thing I debated given the
current example with the Arizona driver. Not preserving is the default
for the Aspeed hardware, so that ended up influencing my choice. Not
that implementation details should necessarily influence interface
design, but it was at least more than a coin toss.

I don't have anything specific against preserving by default, just my
gut instinct and the hardware went the other way. As long as we expose
the option to opt out, which the additions for the Arizona already do.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> I paged Bartosz and Michael on this, they are experts on the use cases for
> the character device and their opinions are likely more valuable than mine.
> 
> > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > Similar to devicetree support, add flags and mappings to expose reset
> > tolerance configuration through the chardev interface.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> 
> (...)
> 
> > +                * Unconditionally configure reset tolerance, as it's possible
> > +                * that the tolerance flag itself becomes tolerant to resets.
> > +                * Thus it could remain set from a previous environment, but
> > +                * the current environment may not expect it so.
> > +                */
> > +               ret = gpiod_set_reset_tolerant(desc,
> > +                               !!(lflags & GPIOHANDLE_REQUEST_RESET_TOLERANT));
> > +               if (ret < 0)
> > +                       goto out_free_descs;
> 
> First, as noted in the first patch, IMO we should just go for persistance,
> i.e. you want to flag to the system to keep the line persistent in any case,
> no matter if the system goes to sleep or resets.
> 
> So the usecase is going to be a control system or similar, a makerspace
> project, an industrial product of some kind, driving GPIO from userspace.
> 
> I don't see it as helpful to give userspace control over whether the line
> is persistent or not. It is more reasonable to assume persistance for
> userspace use cases, don't you think? Whether the system goes to sleep
> or the gpiochip resets should not make a door suddenly close or the
> lights in the christmas tree go out, right? I think if the gpiochip supports
> persistance of any kind, we should try to use it and not have userspace
> provide flags for that.

Right. I guess the counter argument to your examples is if the gpio is
controlling any active process that we don't want to continue if we've
lost the capacity to monitor some other inputs (some kind of dead-man's 
switch). But maybe the argument is that should be implemented in the
kernel anyway?

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset

[Charles Keepax]

On Fri, Oct 20, 2017 at 07:02:27PM +1030, Andrew Jeffery wrote:
> On Fri, 2017-10-20 at 09:43 +0200, Linus Walleij wrote:
> > On Fri, Oct 20, 2017 at 9:17 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > > 
> > > > GPIO state reset tolerance is implemented in gpiolib through the
> > > > addition of a new pinconf parameter. With that, some renaming of helpers
> > > > is done to clarify the scope of the already existing
> > > > gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
> > > > means on suspend, reset or both.
> > > 
> > > Isn't it most reasonable to say persistance covers both cases, reset
> > > and/or sleep? This seems a bit like overdefined.
> > 

Seems reasonable to me to just expand the existing stuff to cover
reset as well, I don't think that should cause any issues for the
Arizona stuff.

> > I should also add: right now persistance is defined in negative terms,
> > you can supply the flag "may lose value", which means the subsystem
> > by default, and driver by default, will try to keep values persistent across
> > sleep.
> > 
> > Then it is possible to opt in for not doing so. (Usually to save power I
> > think.)
> > 
> > I think that especially for userspace use cases, saving power should
> > not really be the concern, but correct me if I'm wrong. I am thinking
> > of a box with a DC plug wired up to a factory line here.
> > 
> > What we have in the Arizona driver is an opt-in where the DT can
> > say "don't preserve the value  this line during system sleep" i.e. "lay lose
> > value" and we can extend that flag to mean "don't preserve this line
> > during reset either" but by default assume that we should.
> 
> Yeah, the preserve polarity was another thing I debated given the
> current example with the Arizona driver. Not preserving is the default
> for the Aspeed hardware, so that ended up influencing my choice. Not
> that implementation details should necessarily influence interface
> design, but it was at least more than a coin toss.
> 

The way I think we ended up looking at this was when a user
requests a GPIO the least surprising thing for them is the value
maintains until they change it again. So it made sense to make
the allowing the value to be dropped the opt in side.

> I don't have anything specific against preserving by default, just my
> gut instinct and the hardware went the other way. As long as we expose
> the option to opt out, which the additions for the Arizona already do.
> 
> Cheers,
> 
> Andrew

Thanks,
Charles

Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset

[Charles Keepax]

On Fri, Oct 20, 2017 at 07:32:53PM +1030, Andrew Jeffery wrote:
> On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> > I don't see it as helpful to give userspace control over whether the line
> > is persistent or not. It is more reasonable to assume persistance for
> > userspace use cases, don't you think? Whether the system goes to sleep
> > or the gpiochip resets should not make a door suddenly close or the
> > lights in the christmas tree go out, right? I think if the gpiochip supports
> > persistance of any kind, we should try to use it and not have userspace
> > provide flags for that.
> 
> Right. I guess the counter argument to your examples is if the gpio is
> controlling any active process that we don't want to continue if we've
> lost the capacity to monitor some other inputs (some kind of dead-man's 
> switch). But maybe the argument is that should be implemented in the
> kernel anyway?
> 

To me it certainly feels like decisions like this should live in
the kernel, your talking about things that could cause very weird
hardware behaviour if set wrong, so it makes sense to me to have
that responsibility guarded in the kernel.

Thanks,
Charles

Re: [RFC PATCH 1/5] gpio: gpiolib: Add core support for maintaining GPIO values on reset

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

On Wed, 2017-10-25 at 09:11 +0100, Charles Keepax wrote:
> On Fri, Oct 20, 2017 at 07:02:27PM +1030, Andrew Jeffery wrote:
> > On Fri, 2017-10-20 at 09:43 +0200, Linus Walleij wrote:
> > > On Fri, Oct 20, 2017 at 9:17 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > > > On Fri, Oct 20, 2017 at 5:37 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > > GPIO state reset tolerance is implemented in gpiolib through the
> > > > > addition of a new pinconf parameter. With that, some renaming of helpers
> > > > > is done to clarify the scope of the already existing
> > > > > gpiochip_line_is_persistent(), as it's now ambiguous as to whether that
> > > > > means on suspend, reset or both.
> > > > 
> > > > Isn't it most reasonable to say persistance covers both cases, reset
> > > > and/or sleep? This seems a bit like overdefined.
> 
> Seems reasonable to me to just expand the existing stuff to cover
> reset as well, I don't think that should cause any issues for the
> Arizona stuff.

Great. I addressed this in the non-RFC series.

Thanks for the feedback.

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On Wed, 2017-10-25 at 09:14 +0100, Charles Keepax wrote:
> On Fri, Oct 20, 2017 at 07:32:53PM +1030, Andrew Jeffery wrote:
> > On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> > > I don't see it as helpful to give userspace control over whether the line
> > > is persistent or not. It is more reasonable to assume persistance for
> > > userspace use cases, don't you think? Whether the system goes to sleep
> > > or the gpiochip resets should not make a door suddenly close or the
> > > lights in the christmas tree go out, right? I think if the gpiochip supports
> > > persistance of any kind, we should try to use it and not have userspace
> > > provide flags for that.
> > 
> > Right. I guess the counter argument to your examples is if the gpio is
> > controlling any active process that we don't want to continue if we've
> > lost the capacity to monitor some other inputs (some kind of dead-man's 
> > switch). But maybe the argument is that should be implemented in the
> > kernel anyway?
> > 
> 
> To me it certainly feels like decisions like this should live in
> the kernel, your talking about things that could cause very weird
> hardware behaviour if set wrong, so it makes sense to me to have
> that responsibility guarded in the kernel.

I feel that taking this argument to its logical conclusion leads to
never exporting any GPIOs to userspace and doing everything in the
kernel. If userspace has exported the GPIO and is managing its state,
then it can *already* cause very weird hardware behaviour if set wrong.
The fact that userspace is controlling the GPIO state and not the
kernel already says that the kernel doesn't know how to manage it, so
why not expose the option for userspace to set the persistence, given
that it should know what it's doing?

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset

[Charles Keepax]

On Thu, Oct 26, 2017 at 10:35:39AM +1030, Andrew Jeffery wrote:
> On Wed, 2017-10-25 at 09:14 +0100, Charles Keepax wrote:
> > On Fri, Oct 20, 2017 at 07:32:53PM +1030, Andrew Jeffery wrote:
> > > On Fri, 2017-10-20 at 09:27 +0200, Linus Walleij wrote:
> > > > I don't see it as helpful to give userspace control over whether the line
> > > > is persistent or not. It is more reasonable to assume persistance for
> > > > userspace use cases, don't you think? Whether the system goes to sleep
> > > > or the gpiochip resets should not make a door suddenly close or the
> > > > lights in the christmas tree go out, right? I think if the gpiochip supports
> > > > persistance of any kind, we should try to use it and not have userspace
> > > > provide flags for that.
> > > 
> > > Right. I guess the counter argument to your examples is if the gpio is
> > > controlling any active process that we don't want to continue if we've
> > > lost the capacity to monitor some other inputs (some kind of dead-man's 
> > > switch). But maybe the argument is that should be implemented in the
> > > kernel anyway?
> > > 
> > 
> > To me it certainly feels like decisions like this should live in
> > the kernel, your talking about things that could cause very weird
> > hardware behaviour if set wrong, so it makes sense to me to have
> > that responsibility guarded in the kernel.
> 
> I feel that taking this argument to its logical conclusion leads to
> never exporting any GPIOs to userspace and doing everything in the
> kernel. If userspace has exported the GPIO and is managing its state,
> then it can *already* cause very weird hardware behaviour if set wrong.
> The fact that userspace is controlling the GPIO state and not the
> kernel already says that the kernel doesn't know how to manage it, so
> why not expose the option for userspace to set the persistence, given
> that it should know what it's doing?

Admittedly yes, I guess it really comes down to use-cases.  There
are fairly strong use-cases to control GPIOs from user-space
that justify the risks. The use-cases for being able to set
non-persistent GPIOs from user-space seem less clear to me, but
if they exist I certainly don't have any objection.

Thanks,
Charles

Re: [RFC PATCH 3/5] gpio: gpiolib: Add chardev support for maintaining GPIO values on reset

[Linus Walleij]

On Thu, Oct 26, 2017 at 2:05 AM, Andrew Jeffery <andrew@aj.id.au> wrote:

> I feel that taking this argument to its logical conclusion leads to
> never exporting any GPIOs to userspace and doing everything in the
> kernel.

That is very much how I feel about things anyways.
In a recent presentation:
https://dflund.se/~triad/papers/GPIO-for-Engineers-and-Makers.pdf

I had the following text:

The Rules of Linux Userspace GPIO
1. You do not access GPIOs from userspace
2. YOU DO NOT ACCESS GPIOS FROM USERSPACE
3. Read Documentation/gpio/drivers-on-gpio.txt
4. Use the character device

> If userspace has exported the GPIO and is managing its state,
> then it can *already* cause very weird hardware behaviour if set wrong.
> The fact that userspace is controlling the GPIO state and not the
> kernel already says that the kernel doesn't know how to manage it, so
> why not expose the option for userspace to set the persistence, given
> that it should know what it's doing?

People do need to access GPIOs from userspace for things
like one-off makerspace projects, relays on factory lines,
fire alarms, door openers etc etc.

One-off projects is fine, the user likely has an idea about the
whole system that is comprehensive. They use the random
raspberry Pi (etc) development board for this. OK.

When we are talking about adding GPIO in mass-market goods
such as phones and tablets and laptops userspace GPIO
access become more and more dubious.

Yours,
Linus Walleij