[PATCH 1/4] pinctrl: introduce generic pin config

[PATCH 1/4] pinctrl: introduce generic pin config

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

This is a split-off from the earlier patch set which adds generic
pin configuration for the pin controllers that want it. Since
we may have a system with mixed generic and custom pin controllers,
we pass a boolean in the pin controller ops vtable to indicate
if it is generic.

ChangeLog v1->v5:
- Follow parent patch versioning number system.
- Document the semantic meaning of return values from pin config
  get functions, so we can iterate over pins and check their
  properties from debugfs as part of the generic config code.
- Use proper cast functions in the generic debugfs pin config
  file.
- Expand generic config to optionally cover groups too.
ChangeLog v5->v6:
- Update to match underlying changes.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/Kconfig                 |    4 +
 drivers/pinctrl/Makefile                |    1 +
 drivers/pinctrl/pinconf-generic.c       |  123 ++++++++++++++++++++++++++
 drivers/pinctrl/pinconf.c               |    6 +-
 drivers/pinctrl/pinconf.h               |   43 +++++++++-
 include/linux/pinctrl/pinconf-generic.h |  147 +++++++++++++++++++++++++++++++
 include/linux/pinctrl/pinconf.h         |    5 +
 7 files changed, 325 insertions(+), 4 deletions(-)
 create mode 100644 drivers/pinctrl/pinconf-generic.c
 create mode 100644 include/linux/pinctrl/pinconf-generic.h

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 3bea79f..ef197cc 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -17,6 +17,10 @@ config PINMUX
 config PINCONF
 	bool "Support pin configuration controllers"
 
+config GENERIC_PINCONF
+	bool
+	select PINCONF
+
 config DEBUG_PINCTRL
 	bool "Debug PINCTRL calls"
 	depends on DEBUG_KERNEL
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index c95d91e..5a46679 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_PINCTRL)	+= -DDEBUG
 obj-$(CONFIG_PINCTRL)		+= core.o
 obj-$(CONFIG_PINMUX)		+= pinmux.o
 obj-$(CONFIG_PINCONF)		+= pinconf.o
+obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
 obj-$(CONFIG_PINCTRL_PXA3xx)	+= pinctrl-pxa3xx.o
 obj-$(CONFIG_PINCTRL_MMP2)	+= pinctrl-mmp2.o
 obj-$(CONFIG_PINCTRL_PXA168)	+= pinctrl-pxa168.o
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
new file mode 100644
index 0000000..594b820
--- /dev/null
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -0,0 +1,123 @@
+/*
+ * Core driver for the generic pin config portions of the pin control subsystem
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#define pr_fmt(fmt) "generic pinconfig core: " fmt
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include "core.h"
+#include "pinconf.h"
+
+#ifdef CONFIG_DEBUG_FS
+
+struct pin_config_item {
+	const enum pin_config_param param;
+	const char * const display;
+	const char * const format;
+};
+
+#define PCONFDUMP(a, b, c) { .param = a, .display = b, .format = c }
+
+struct pin_config_item conf_items[] = {
+	PCONFDUMP(PIN_CONFIG_BIAS_DISABLE, "input bias disabled", NULL),
+	PCONFDUMP(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, "input bias high impedance", NULL),
+	PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", "Ohm"),
+	PCONFDUMP(PIN_CONFIG_BIAS_PULL_DOWN, "input bias pull down", "Ohm"),
+	PCONFDUMP(PIN_CONFIG_BIAS_HIGH, "input bias high", NULL),
+	PCONFDUMP(PIN_CONFIG_BIAS_GROUND, "input bias ground", NULL),
+	PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", "X stages"),
+	PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_DRAIN, "output drive open drain", "X stages"),
+	PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", "X stages"),
+	PCONFDUMP(PIN_CONFIG_DRIVE_OFF, "output drive off", NULL),
+	PCONFDUMP(PIN_CONFIG_INPUT_SCHMITT, "input schmitt trigger", NULL),
+	PCONFDUMP(PIN_CONFIG_INPUT_DEBOUNCE, "input debounce", "time units"),
+	PCONFDUMP(PIN_CONFIG_SLEW_RATE_RISING, "output slew rate rising", "of max"),
+	PCONFDUMP(PIN_CONFIG_SLEW_RATE_FALLING, "output slew rate falling", "of max"),
+	PCONFDUMP(PIN_CONFIG_POWER_SOURCE, "pin power source", "selector"),
+	PCONFDUMP(PIN_CONFIG_LOW_POWER_MODE, "pin low power", "mode"),
+	PCONFDUMP(PIN_CONFIG_WAKEUP, "input wakeup", NULL),
+};
+
+void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, unsigned pin)
+{
+	const struct pinconf_ops *ops = pctldev->desc->confops;
+	int i;
+
+	if (!ops->is_generic)
+		return;
+
+	for(i = 0; i < ARRAY_SIZE(conf_items); i++) {
+		unsigned long config;
+		int ret;
+
+		/* We want to check out this parameter */
+		config = to_config_packed(conf_items[i].param, 0);
+		ret = pin_config_get_for_pin(pctldev, pin, &config);
+		/* These are legal errors */
+		if (ret == -EINVAL || ret == -ENOTSUPP)
+			continue;
+		if (ret) {
+			seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
+			continue;
+		}
+		/* Space between multiple configs */
+		seq_puts(s, " ");
+		seq_puts(s, conf_items[i].display);
+		/* Print unit if available */
+		if (conf_items[i].format && to_config_argument(config) != 0)
+			seq_printf(s, " (%u %s)", to_config_argument(config),
+				   conf_items[i].format);
+	}
+}
+
+void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, const char *gname)
+{
+	const struct pinconf_ops *ops = pctldev->desc->confops;
+	int i;
+
+	if (!ops->is_generic)
+		return;
+
+	for(i = 0; i < ARRAY_SIZE(conf_items); i++) {
+		unsigned long config;
+		int ret;
+
+		/* We want to check out this parameter */
+		config = to_config_packed(conf_items[i].param, 0);
+		ret = pin_config_group_get(dev_name(pctldev->dev), gname,
+					   &config);
+		/* These are legal errors */
+		if (ret == -EINVAL || ret == -ENOTSUPP)
+			continue;
+		if (ret) {
+			seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
+			continue;
+		}
+		/* Space between multiple configs */
+		seq_puts(s, " ");
+		seq_puts(s, conf_items[i].display);
+		/* Print unit if available */
+		if (conf_items[i].format && config != 0)
+			seq_printf(s, " (%u %s)", to_config_argument(config),
+				   conf_items[i].format);
+	}
+}
+
+#endif
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 84869f2..247b9f2 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -54,7 +54,7 @@ int pinconf_validate_map(struct pinctrl_map const *map, int i)
 	return 0;
 }
 
-static int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
+int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
 			   unsigned long *config)
 {
 	const struct pinconf_ops *ops = pctldev->desc->confops;
@@ -432,6 +432,8 @@ static void pinconf_dump_pin(struct pinctrl_dev *pctldev,
 {
 	const struct pinconf_ops *ops = pctldev->desc->confops;
 
+	/* no-op when not using generic pin config */
+	pinconf_generic_dump_pin(pctldev, s, pin);
 	if (ops && ops->pin_config_dbg_show)
 		ops->pin_config_dbg_show(pctldev, s, pin);
 }
@@ -475,6 +477,8 @@ static void pinconf_dump_group(struct pinctrl_dev *pctldev,
 {
 	const struct pinconf_ops *ops = pctldev->desc->confops;
 
+	/* no-op when not using generic pin config */
+	pinconf_generic_dump_group(pctldev, s, gname);
 	if (ops && ops->pin_config_group_dbg_show)
 		ops->pin_config_group_dbg_show(pctldev, s, selector);
 }
diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
index 0ded227..54510de 100644
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -14,20 +14,26 @@
 #ifdef CONFIG_PINCONF
 
 int pinconf_check_ops(struct pinctrl_dev *pctldev);
-
 int pinconf_validate_map(struct pinctrl_map const *map, int i);
-
 int pinconf_map_to_setting(struct pinctrl_map const *map,
 			  struct pinctrl_setting *setting);
 void pinconf_free_setting(struct pinctrl_setting const *setting);
 int pinconf_apply_setting(struct pinctrl_setting const *setting);
-
 void pinconf_show_map(struct seq_file *s, struct pinctrl_map const *map);
 void pinconf_show_setting(struct seq_file *s,
 			  struct pinctrl_setting const *setting);
 void pinconf_init_device_debugfs(struct dentry *devroot,
 				 struct pinctrl_dev *pctldev);
 
+/*
+ * You will only be interested in these if you're using PINCONF
+ * so don't supply any stubs for these.
+ */
+int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
+			   unsigned long *config);
+int pin_config_group_get(const char *dev_name, const char *pin_group,
+			 unsigned long *config);
+
 #else
 
 static inline int pinconf_check_ops(struct pinctrl_dev *pctldev)
@@ -71,3 +77,34 @@ static inline void pinconf_init_device_debugfs(struct dentry *devroot,
 }
 
 #endif
+
+/*
+ * The following functions are available if the driver uses the generic
+ * pin config.
+ */
+
+#ifdef CONFIG_GENERIC_PINCONF
+
+void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, unsigned pin);
+
+void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, const char *gname);
+
+#else
+
+static inline void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
+					    struct seq_file *s,
+					    unsigned pin)
+{
+	return;
+}
+
+static inline void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
+					      struct seq_file *s,
+					      const char *gname)
+{
+	return;
+}
+
+#endif
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
new file mode 100644
index 0000000..a47613b
--- /dev/null
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -0,0 +1,147 @@
+/*
+ * Interface the generic pinconfig portions of the pinctrl subsystem
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * This interface is used in the core to keep track of pins.
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef __LINUX_PINCTRL_PINCONF_GENERIC_H
+#define __LINUX_PINCTRL_PINCONF_GENERIC_H
+
+/*
+ * You shouldn't even be able to compile with these enums etc unless you're
+ * using generic pin config. That is why this is defined out.
+ */
+#ifdef CONFIG_GENERIC_PINCONF
+
+/**
+ * enum pin_config_param - possible pin configuration parameters
+ * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
+ *	transition from say pull-up to pull-down implies that you disable
+ *	pull-up in the process, this setting disables all biasing.
+ * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
+ *	mode, also know as "third-state" (tristate) or "high-Z" or "floating".
+ *	On output pins this effectively disconnects the pin, which is useful
+ *	if for example some other pin is going to drive the signal connected
+ *	to it for a while. Pins used for input are usually always high
+ *	impedance.
+ * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
+ *	impedance to VDD), if the controller supports specifying a certain
+ *	pull-up resistance, this is given as an argument (in Ohms) when
+ *	setting this parameter.
+ * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
+ *	impedance to GROUND), if the controller supports specifying a certain
+ *	pull-down resistance, this is given as an argument (in Ohms) when
+ *	setting this parameter.
+ * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
+ * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND
+ * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
+ *	low, this is the most typical case and is typically achieved with two
+ *	active transistors on the output. If the pin can support different
+ *	drive strengths for push/pull, the strength is given in the argument
+ *	as the number of driving stages vs nominal load impedance, so say
+ *	quadruple driving stages (usually 8 transistors rather than two) will
+ *	be configured with the 8 passed as argument.
+ * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
+ *	collector) which means it is usually wired with other output ports
+ *	which are then pulled up with an external resistor. If the pin can
+ *	support different drive strengths for the open drain pin, the format
+ *	is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
+ * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
+ *	(open emitter) which is the same as open drain but pulled to ground.
+ *	If the pin can support different drive strengths for the open drain
+ *	pin, the format is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
+ * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off.
+ * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
+ *	schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
+ *	the threshold value is given on a custom format as argument when
+ *	setting pins to this mode. The argument zero turns the schmitt trigger
+ *	off.
+ * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
+ *	which means it will wait for signals to settle when reading inputs. The
+ *	argument gives the debounce time on a custom format. Setting the
+ *	argument to zero turns debouncing off.
+ * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
+ *	signals on the pin. The argument gives the rise time in fractions
+ *	compared to maximum rise time, 0 means nominal rise time. If you can
+ *	control slew rate in 4 steps these will likely be equidistant like
+ *	1/4, 1/2, 3/4 or full nominal slew rate, which means argument 4 gives
+ *	you 1/4 of nominal slew rate and the argument 4 has the same meaning
+ *	as 0 - nominal slew rate (fastest possible, steep edges). You may want
+ *	to adjust slew rates so that signal edges don't get too steep, causing
+ *	disturbances in surrounding electronics known as electromagnetic
+ *	interference (EMI) for example.
+ * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
+ *	signals on the pin. The argument gives the fall time in fractions
+ *	compared to nominal fall time.
+ * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
+ *	supplies, the argument to this parameter (on a custom format) tells
+ *	the driver which alternative power source to use.
+ * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
+ *	operation, if several modes of operation are supported these can be
+ *	passed in the argument on a custom form, else just use argument 1
+ *	to indicate low power mode, argument 0 turns low power mode off.
+ * @PIN_CONFIG_WAKEUP: this will configure an input pin such that if a
+ *	signal transition arrives at the pin when the pin controller/system
+ *	is sleeping, it will wake up the system if argument 1 is passed along.
+ *	Pass argument 0 to turn wakeup enablement off.
+ * @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.
+ */
+enum pin_config_param {
+	PIN_CONFIG_BIAS_DISABLE,
+	PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
+	PIN_CONFIG_BIAS_PULL_UP,
+	PIN_CONFIG_BIAS_PULL_DOWN,
+	PIN_CONFIG_BIAS_HIGH,
+	PIN_CONFIG_BIAS_GROUND,
+	PIN_CONFIG_DRIVE_PUSH_PULL,
+	PIN_CONFIG_DRIVE_OPEN_DRAIN,
+	PIN_CONFIG_DRIVE_OPEN_SOURCE,
+	PIN_CONFIG_DRIVE_OFF,
+	PIN_CONFIG_INPUT_SCHMITT,
+	PIN_CONFIG_INPUT_DEBOUNCE,
+	PIN_CONFIG_SLEW_RATE_RISING,
+	PIN_CONFIG_SLEW_RATE_FALLING,
+	PIN_CONFIG_POWER_SOURCE,
+	PIN_CONFIG_LOW_POWER_MODE,
+	PIN_CONFIG_WAKEUP,
+	PIN_CONFIG_END,
+};
+
+/*
+ * The following inlines stuffs a configuration parameter and data value
+ * into and out of an unsigned long argument, as used by the generic pin config
+ * system. We put the parameter in the lower 16 bits and the argument in the
+ * upper 16 bits.
+ */
+
+static inline enum pin_config_param to_config_param(unsigned long config)
+{
+	return (enum pin_config_param) (config & 0xffffUL);
+}
+
+static inline u16 to_config_argument(unsigned long config)
+{
+	return (enum pin_config_param) ((config >> 16) & 0xffffUL);
+}
+
+static inline unsigned long to_config_packed(enum pin_config_param param,
+					     u16 argument)
+{
+	return (argument << 16) | ((unsigned long) param & 0xffffUL);
+}
+
+/*
+ * Helpful configuration macro to be used in tables etc.
+ */
+#define PIN_CONF_PACKED(p, a) ((a << 16) | ((unsigned long) p & 0xffffUL))
+
+#endif /* CONFIG_GENERIC_PINCONF */
+
+#endif /* __LINUX_PINCTRL_PINCONF_GENERIC_H */
diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h
index f8cf156..ec431f0 100644
--- a/include/linux/pinctrl/pinconf.h
+++ b/include/linux/pinctrl/pinconf.h
@@ -20,6 +20,8 @@ struct seq_file;
 /**
  * struct pinconf_ops - pin config operations, to be implemented by
  * pin configuration capable drivers.
+ * @is_generic: for pin controllers that want to use the generic interface,
+ *	this flag tells the framework that it's generic.
  * @pin_config_get: get the config of a certain pin, if the requested config
  *	is not available on this controller this should return -ENOTSUPP
  *	and if it is available but disabled it should return -EINVAL
@@ -33,6 +35,9 @@ struct seq_file;
  *	per-device info for a certain group in debugfs
  */
 struct pinconf_ops {
+#ifdef CONFIG_GENERIC_PINCONF
+	bool is_generic;
+#endif
 	int (*pin_config_get) (struct pinctrl_dev *pctldev,
 			       unsigned pin,
 			       unsigned long *config);
-- 
1.7.8

[PATCH 1/4] pinctrl: introduce generic pin config

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

This is a split-off from the earlier patch set which adds generic
pin configuration for the pin controllers that want it. Since
we may have a system with mixed generic and custom pin controllers,
we pass a boolean in the pin controller ops vtable to indicate
if it is generic.

ChangeLog v1->v5:
- Follow parent patch versioning number system.
- Document the semantic meaning of return values from pin config
  get functions, so we can iterate over pins and check their
  properties from debugfs as part of the generic config code.
- Use proper cast functions in the generic debugfs pin config
  file.
- Expand generic config to optionally cover groups too.
ChangeLog v5->v6:
- Update to match underlying changes.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/Kconfig                 |    4 +
 drivers/pinctrl/Makefile                |    1 +
 drivers/pinctrl/pinconf-generic.c       |  123 ++++++++++++++++++++++++++
 drivers/pinctrl/pinconf.c               |    6 +-
 drivers/pinctrl/pinconf.h               |   43 +++++++++-
 include/linux/pinctrl/pinconf-generic.h |  147 +++++++++++++++++++++++++++++++
 include/linux/pinctrl/pinconf.h         |    5 +
 7 files changed, 325 insertions(+), 4 deletions(-)
 create mode 100644 drivers/pinctrl/pinconf-generic.c
 create mode 100644 include/linux/pinctrl/pinconf-generic.h

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 3bea79f..ef197cc 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -17,6 +17,10 @@ config PINMUX
 config PINCONF
 	bool "Support pin configuration controllers"
 
+config GENERIC_PINCONF
+	bool
+	select PINCONF
+
 config DEBUG_PINCTRL
 	bool "Debug PINCTRL calls"
 	depends on DEBUG_KERNEL
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index c95d91e..5a46679 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_PINCTRL)	+= -DDEBUG
 obj-$(CONFIG_PINCTRL)		+= core.o
 obj-$(CONFIG_PINMUX)		+= pinmux.o
 obj-$(CONFIG_PINCONF)		+= pinconf.o
+obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
 obj-$(CONFIG_PINCTRL_PXA3xx)	+= pinctrl-pxa3xx.o
 obj-$(CONFIG_PINCTRL_MMP2)	+= pinctrl-mmp2.o
 obj-$(CONFIG_PINCTRL_PXA168)	+= pinctrl-pxa168.o
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
new file mode 100644
index 0000000..594b820
--- /dev/null
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -0,0 +1,123 @@
+/*
+ * Core driver for the generic pin config portions of the pin control subsystem
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#define pr_fmt(fmt) "generic pinconfig core: " fmt
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include "core.h"
+#include "pinconf.h"
+
+#ifdef CONFIG_DEBUG_FS
+
+struct pin_config_item {
+	const enum pin_config_param param;
+	const char * const display;
+	const char * const format;
+};
+
+#define PCONFDUMP(a, b, c) { .param = a, .display = b, .format = c }
+
+struct pin_config_item conf_items[] = {
+	PCONFDUMP(PIN_CONFIG_BIAS_DISABLE, "input bias disabled", NULL),
+	PCONFDUMP(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, "input bias high impedance", NULL),
+	PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", "Ohm"),
+	PCONFDUMP(PIN_CONFIG_BIAS_PULL_DOWN, "input bias pull down", "Ohm"),
+	PCONFDUMP(PIN_CONFIG_BIAS_HIGH, "input bias high", NULL),
+	PCONFDUMP(PIN_CONFIG_BIAS_GROUND, "input bias ground", NULL),
+	PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", "X stages"),
+	PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_DRAIN, "output drive open drain", "X stages"),
+	PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", "X stages"),
+	PCONFDUMP(PIN_CONFIG_DRIVE_OFF, "output drive off", NULL),
+	PCONFDUMP(PIN_CONFIG_INPUT_SCHMITT, "input schmitt trigger", NULL),
+	PCONFDUMP(PIN_CONFIG_INPUT_DEBOUNCE, "input debounce", "time units"),
+	PCONFDUMP(PIN_CONFIG_SLEW_RATE_RISING, "output slew rate rising", "of max"),
+	PCONFDUMP(PIN_CONFIG_SLEW_RATE_FALLING, "output slew rate falling", "of max"),
+	PCONFDUMP(PIN_CONFIG_POWER_SOURCE, "pin power source", "selector"),
+	PCONFDUMP(PIN_CONFIG_LOW_POWER_MODE, "pin low power", "mode"),
+	PCONFDUMP(PIN_CONFIG_WAKEUP, "input wakeup", NULL),
+};
+
+void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, unsigned pin)
+{
+	const struct pinconf_ops *ops = pctldev->desc->confops;
+	int i;
+
+	if (!ops->is_generic)
+		return;
+
+	for(i = 0; i < ARRAY_SIZE(conf_items); i++) {
+		unsigned long config;
+		int ret;
+
+		/* We want to check out this parameter */
+		config = to_config_packed(conf_items[i].param, 0);
+		ret = pin_config_get_for_pin(pctldev, pin, &config);
+		/* These are legal errors */
+		if (ret == -EINVAL || ret == -ENOTSUPP)
+			continue;
+		if (ret) {
+			seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
+			continue;
+		}
+		/* Space between multiple configs */
+		seq_puts(s, " ");
+		seq_puts(s, conf_items[i].display);
+		/* Print unit if available */
+		if (conf_items[i].format && to_config_argument(config) != 0)
+			seq_printf(s, " (%u %s)", to_config_argument(config),
+				   conf_items[i].format);
+	}
+}
+
+void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, const char *gname)
+{
+	const struct pinconf_ops *ops = pctldev->desc->confops;
+	int i;
+
+	if (!ops->is_generic)
+		return;
+
+	for(i = 0; i < ARRAY_SIZE(conf_items); i++) {
+		unsigned long config;
+		int ret;
+
+		/* We want to check out this parameter */
+		config = to_config_packed(conf_items[i].param, 0);
+		ret = pin_config_group_get(dev_name(pctldev->dev), gname,
+					   &config);
+		/* These are legal errors */
+		if (ret == -EINVAL || ret == -ENOTSUPP)
+			continue;
+		if (ret) {
+			seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
+			continue;
+		}
+		/* Space between multiple configs */
+		seq_puts(s, " ");
+		seq_puts(s, conf_items[i].display);
+		/* Print unit if available */
+		if (conf_items[i].format && config != 0)
+			seq_printf(s, " (%u %s)", to_config_argument(config),
+				   conf_items[i].format);
+	}
+}
+
+#endif
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 84869f2..247b9f2 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -54,7 +54,7 @@ int pinconf_validate_map(struct pinctrl_map const *map, int i)
 	return 0;
 }
 
-static int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
+int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
 			   unsigned long *config)
 {
 	const struct pinconf_ops *ops = pctldev->desc->confops;
@@ -432,6 +432,8 @@ static void pinconf_dump_pin(struct pinctrl_dev *pctldev,
 {
 	const struct pinconf_ops *ops = pctldev->desc->confops;
 
+	/* no-op when not using generic pin config */
+	pinconf_generic_dump_pin(pctldev, s, pin);
 	if (ops && ops->pin_config_dbg_show)
 		ops->pin_config_dbg_show(pctldev, s, pin);
 }
@@ -475,6 +477,8 @@ static void pinconf_dump_group(struct pinctrl_dev *pctldev,
 {
 	const struct pinconf_ops *ops = pctldev->desc->confops;
 
+	/* no-op when not using generic pin config */
+	pinconf_generic_dump_group(pctldev, s, gname);
 	if (ops && ops->pin_config_group_dbg_show)
 		ops->pin_config_group_dbg_show(pctldev, s, selector);
 }
diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
index 0ded227..54510de 100644
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -14,20 +14,26 @@
 #ifdef CONFIG_PINCONF
 
 int pinconf_check_ops(struct pinctrl_dev *pctldev);
-
 int pinconf_validate_map(struct pinctrl_map const *map, int i);
-
 int pinconf_map_to_setting(struct pinctrl_map const *map,
 			  struct pinctrl_setting *setting);
 void pinconf_free_setting(struct pinctrl_setting const *setting);
 int pinconf_apply_setting(struct pinctrl_setting const *setting);
-
 void pinconf_show_map(struct seq_file *s, struct pinctrl_map const *map);
 void pinconf_show_setting(struct seq_file *s,
 			  struct pinctrl_setting const *setting);
 void pinconf_init_device_debugfs(struct dentry *devroot,
 				 struct pinctrl_dev *pctldev);
 
+/*
+ * You will only be interested in these if you're using PINCONF
+ * so don't supply any stubs for these.
+ */
+int pin_config_get_for_pin(struct pinctrl_dev *pctldev, unsigned pin,
+			   unsigned long *config);
+int pin_config_group_get(const char *dev_name, const char *pin_group,
+			 unsigned long *config);
+
 #else
 
 static inline int pinconf_check_ops(struct pinctrl_dev *pctldev)
@@ -71,3 +77,34 @@ static inline void pinconf_init_device_debugfs(struct dentry *devroot,
 }
 
 #endif
+
+/*
+ * The following functions are available if the driver uses the generic
+ * pin config.
+ */
+
+#ifdef CONFIG_GENERIC_PINCONF
+
+void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, unsigned pin);
+
+void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, const char *gname);
+
+#else
+
+static inline void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
+					    struct seq_file *s,
+					    unsigned pin)
+{
+	return;
+}
+
+static inline void pinconf_generic_dump_group(struct pinctrl_dev *pctldev,
+					      struct seq_file *s,
+					      const char *gname)
+{
+	return;
+}
+
+#endif
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
new file mode 100644
index 0000000..a47613b
--- /dev/null
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -0,0 +1,147 @@
+/*
+ * Interface the generic pinconfig portions of the pinctrl subsystem
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * This interface is used in the core to keep track of pins.
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef __LINUX_PINCTRL_PINCONF_GENERIC_H
+#define __LINUX_PINCTRL_PINCONF_GENERIC_H
+
+/*
+ * You shouldn't even be able to compile with these enums etc unless you're
+ * using generic pin config. That is why this is defined out.
+ */
+#ifdef CONFIG_GENERIC_PINCONF
+
+/**
+ * enum pin_config_param - possible pin configuration parameters
+ * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
+ *	transition from say pull-up to pull-down implies that you disable
+ *	pull-up in the process, this setting disables all biasing.
+ * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
+ *	mode, also know as "third-state" (tristate) or "high-Z" or "floating".
+ *	On output pins this effectively disconnects the pin, which is useful
+ *	if for example some other pin is going to drive the signal connected
+ *	to it for a while. Pins used for input are usually always high
+ *	impedance.
+ * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
+ *	impedance to VDD), if the controller supports specifying a certain
+ *	pull-up resistance, this is given as an argument (in Ohms) when
+ *	setting this parameter.
+ * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
+ *	impedance to GROUND), if the controller supports specifying a certain
+ *	pull-down resistance, this is given as an argument (in Ohms) when
+ *	setting this parameter.
+ * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
+ * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND
+ * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
+ *	low, this is the most typical case and is typically achieved with two
+ *	active transistors on the output. If the pin can support different
+ *	drive strengths for push/pull, the strength is given in the argument
+ *	as the number of driving stages vs nominal load impedance, so say
+ *	quadruple driving stages (usually 8 transistors rather than two) will
+ *	be configured with the 8 passed as argument.
+ * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
+ *	collector) which means it is usually wired with other output ports
+ *	which are then pulled up with an external resistor. If the pin can
+ *	support different drive strengths for the open drain pin, the format
+ *	is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
+ * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
+ *	(open emitter) which is the same as open drain but pulled to ground.
+ *	If the pin can support different drive strengths for the open drain
+ *	pin, the format is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
+ * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off.
+ * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
+ *	schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
+ *	the threshold value is given on a custom format as argument when
+ *	setting pins to this mode. The argument zero turns the schmitt trigger
+ *	off.
+ * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
+ *	which means it will wait for signals to settle when reading inputs. The
+ *	argument gives the debounce time on a custom format. Setting the
+ *	argument to zero turns debouncing off.
+ * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
+ *	signals on the pin. The argument gives the rise time in fractions
+ *	compared to maximum rise time, 0 means nominal rise time. If you can
+ *	control slew rate in 4 steps these will likely be equidistant like
+ *	1/4, 1/2, 3/4 or full nominal slew rate, which means argument 4 gives
+ *	you 1/4 of nominal slew rate and the argument 4 has the same meaning
+ *	as 0 - nominal slew rate (fastest possible, steep edges). You may want
+ *	to adjust slew rates so that signal edges don't get too steep, causing
+ *	disturbances in surrounding electronics known as electromagnetic
+ *	interference (EMI) for example.
+ * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
+ *	signals on the pin. The argument gives the fall time in fractions
+ *	compared to nominal fall time.
+ * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
+ *	supplies, the argument to this parameter (on a custom format) tells
+ *	the driver which alternative power source to use.
+ * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
+ *	operation, if several modes of operation are supported these can be
+ *	passed in the argument on a custom form, else just use argument 1
+ *	to indicate low power mode, argument 0 turns low power mode off.
+ * @PIN_CONFIG_WAKEUP: this will configure an input pin such that if a
+ *	signal transition arrives at the pin when the pin controller/system
+ *	is sleeping, it will wake up the system if argument 1 is passed along.
+ *	Pass argument 0 to turn wakeup enablement off.
+ * @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.
+ */
+enum pin_config_param {
+	PIN_CONFIG_BIAS_DISABLE,
+	PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
+	PIN_CONFIG_BIAS_PULL_UP,
+	PIN_CONFIG_BIAS_PULL_DOWN,
+	PIN_CONFIG_BIAS_HIGH,
+	PIN_CONFIG_BIAS_GROUND,
+	PIN_CONFIG_DRIVE_PUSH_PULL,
+	PIN_CONFIG_DRIVE_OPEN_DRAIN,
+	PIN_CONFIG_DRIVE_OPEN_SOURCE,
+	PIN_CONFIG_DRIVE_OFF,
+	PIN_CONFIG_INPUT_SCHMITT,
+	PIN_CONFIG_INPUT_DEBOUNCE,
+	PIN_CONFIG_SLEW_RATE_RISING,
+	PIN_CONFIG_SLEW_RATE_FALLING,
+	PIN_CONFIG_POWER_SOURCE,
+	PIN_CONFIG_LOW_POWER_MODE,
+	PIN_CONFIG_WAKEUP,
+	PIN_CONFIG_END,
+};
+
+/*
+ * The following inlines stuffs a configuration parameter and data value
+ * into and out of an unsigned long argument, as used by the generic pin config
+ * system. We put the parameter in the lower 16 bits and the argument in the
+ * upper 16 bits.
+ */
+
+static inline enum pin_config_param to_config_param(unsigned long config)
+{
+	return (enum pin_config_param) (config & 0xffffUL);
+}
+
+static inline u16 to_config_argument(unsigned long config)
+{
+	return (enum pin_config_param) ((config >> 16) & 0xffffUL);
+}
+
+static inline unsigned long to_config_packed(enum pin_config_param param,
+					     u16 argument)
+{
+	return (argument << 16) | ((unsigned long) param & 0xffffUL);
+}
+
+/*
+ * Helpful configuration macro to be used in tables etc.
+ */
+#define PIN_CONF_PACKED(p, a) ((a << 16) | ((unsigned long) p & 0xffffUL))
+
+#endif /* CONFIG_GENERIC_PINCONF */
+
+#endif /* __LINUX_PINCTRL_PINCONF_GENERIC_H */
diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h
index f8cf156..ec431f0 100644
--- a/include/linux/pinctrl/pinconf.h
+++ b/include/linux/pinctrl/pinconf.h
@@ -20,6 +20,8 @@ struct seq_file;
 /**
  * struct pinconf_ops - pin config operations, to be implemented by
  * pin configuration capable drivers.
+ * @is_generic: for pin controllers that want to use the generic interface,
+ *	this flag tells the framework that it's generic.
  * @pin_config_get: get the config of a certain pin, if the requested config
  *	is not available on this controller this should return -ENOTSUPP
  *	and if it is available but disabled it should return -EINVAL
@@ -33,6 +35,9 @@ struct seq_file;
  *	per-device info for a certain group in debugfs
  */
 struct pinconf_ops {
+#ifdef CONFIG_GENERIC_PINCONF
+	bool is_generic;
+#endif
 	int (*pin_config_get) (struct pinctrl_dev *pctldev,
 			       unsigned pin,
 			       unsigned long *config);
-- 
1.7.8

Re: [PATCH 1/4] pinctrl: introduce generic pin config

[Stephen Warren]

On 03/06/2012 03:05 PM, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This is a split-off from the earlier patch set which adds generic
> pin configuration for the pin controllers that want it. Since
> we may have a system with mixed generic and custom pin controllers,
> we pass a boolean in the pin controller ops vtable to indicate
> if it is generic.
> 
> ChangeLog v1->v5:
> - Follow parent patch versioning number system.
> - Document the semantic meaning of return values from pin config
>   get functions, so we can iterate over pins and check their
>   properties from debugfs as part of the generic config code.
> - Use proper cast functions in the generic debugfs pin config
>   file.
> - Expand generic config to optionally cover groups too.
> ChangeLog v5->v6:
> - Update to match underlying changes.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
...
> +/**
> + * enum pin_config_param - possible pin configuration parameters
> + * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
> + *     transition from say pull-up to pull-down implies that you disable
> + *     pull-up in the process, this setting disables all biasing.

What's the use-case for that? It seems like if one state's mapping table
entry says:

PIN_CONFIG_BIAS_PULL_UP, value

and another state wants to switch to pull-down, it'd be best to just be
explicit, and say:

PIN_CONFIG_BIAS_PULL_UP, disabled
PIN_CONFIG_BIAS_PULL_DOWN, value

This is somewhat related to my comments on PULL_UP/DOWN below...

> + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
> + *     mode, also know as "third-state" (tristate) or "high-Z" or "floating".
> + *     On output pins this effectively disconnects the pin, which is useful
> + *     if for example some other pin is going to drive the signal connected
> + *     to it for a while. Pins used for input are usually always high
> + *     impedance.

Isn't this the same as DRIVE_OFF?

> + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
> + *     impedance to VDD), if the controller supports specifying a certain
> + *     pull-up resistance, this is given as an argument (in Ohms) when
> + *     setting this parameter.
> + * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
> + *     impedance to GROUND), if the controller supports specifying a certain
> + *     pull-down resistance, this is given as an argument (in Ohms) when
> + *     setting this parameter.

I don't think its a good idea to combine enabling/disabling features
with configuring values for those features. If an SoC only has on/off
control for pull-up, and the resistance isn't documented, what value
does the driver author guess and require as the value here?

Even when multiple pull-up/down strengths are available, it's quite
plausible they're just documented as "weak" and "strong", and requiring
someone to go find out what the exact values are rather than just using
an enum disabled/enabled,weak=0/enabled,strong=1/ is going to be annoying.

In other words, I think we'd be better off with:

PIN_CONFIG_BIAS_PULL_UP = enabled/disabled 0/1
PIN_CONFIG_BIAS_PULL_DOWN = enabled/disabled 0/1
PIN_CONFIG_PULL_UP_STRENGTH = enumerated value 0..n-1
PIN_CONFIG_PULL_DOWN_STRENGTH = enumerated value 0..n-1

> + * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
> + * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND

Aren't these special cases of pull-up/down, with nominally zero
resistance? I suppose I could see these having separate control bits
though, so I suppose having explicit PIN_CONFIG_* values is reasonable.

> + * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
> + *     low, this is the most typical case and is typically achieved with two
> + *     active transistors on the output. If the pin can support different
> + *     drive strengths for push/pull, the strength is given in the argument
> + *     as the number of driving stages vs nominal load impedance, so say
> + *     quadruple driving stages (usually 8 transistors rather than two) will
> + *     be configured with the 8 passed as argument.
> + * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
> + *     collector) which means it is usually wired with other output ports
> + *     which are then pulled up with an external resistor. If the pin can
> + *     support different drive strengths for the open drain pin, the format
> + *     is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
> + *     (open emitter) which is the same as open drain but pulled to ground.
> + *     If the pin can support different drive strengths for the open drain
> + *     pin, the format is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
> + * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off.

Again, I don't think that explicitly documenting the units (# of
transistors) here is helpful. Datasheets may just be written in terms of
weak/normal/strong...

Again, this mixes selection of drive type with configuring that drive
type. For example, on some SoCs, PUSH_PULL might have a different set of
available drive strengths for PUSH and PULL hence need two parameters.
Equally, the parameter for OPEN_DRAIN is the same as for PUSH_PULL, so
why not separate selecting the drive type and setting the drive strength
so that the setting of the drive strength can be shared between the
drive types?

So, I think this should be:

PIN_CONFIG_DRIVE = push_pull/open_drain/open_source/tristate
PIN_CONFIG_DRIVE_STRENGTH_UP = enumerated value 0..n-1
PIN_CONFIG_DRIVE_STRENGTH_DOWN = enumerated value 0..n-1

Note that PIN_CONFIG_SLEW_RATE_* are essentially parameterizations of
the drive type just like drive strength is, and they're already separate
config parameters.

> + * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
> + *     schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
> + *     the threshold value is given on a custom format as argument when
> + *     setting pins to this mode. The argument zero turns the schmitt trigger
> + *     off.
> + * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
> + *     which means it will wait for signals to settle when reading inputs. The
> + *     argument gives the debounce time on a custom format. Setting the
> + *     argument to zero turns debouncing off.

> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
> + *     signals on the pin. The argument gives the rise time in fractions
> + *     compared to maximum rise time, 0 means nominal rise time. If you can
> + *     control slew rate in 4 steps these will likely be equidistant like
> + *     1/4, 1/2, 3/4 or full nominal slew rate, which means argument 4 gives
> + *     you 1/4 of nominal slew rate and the argument 4 has the same meaning
> + *     as 0 - nominal slew rate (fastest possible, steep edges). You may want
> + *     to adjust slew rates so that signal edges don't get too steep, causing
> + *     disturbances in surrounding electronics known as electromagnetic
> + *     interference (EMI) for example.
> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
> + *     signals on the pin. The argument gives the fall time in fractions
> + *     compared to nominal fall time.

The same comment here; I think the value of those two should just be an
enumeration 0..n-1 to avoid documenting specific units.

> + * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
> + *     supplies, the argument to this parameter (on a custom format) tells
> + *     the driver which alternative power source to use.
> + * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
> + *     operation, if several modes of operation are supported these can be
> + *     passed in the argument on a custom form, else just use argument 1
> + *     to indicate low power mode, argument 0 turns low power mode off.

> + * @PIN_CONFIG_WAKEUP: this will configure an input pin such that if a
> + *     signal transition arrives at the pin when the pin controller/system
> + *     is sleeping, it will wake up the system if argument 1 is passed along.
> + *     Pass argument 0 to turn wakeup enablement off.

Is this something that's controlled at the pinmux level? I would have
expected it to be controlled by whatever HW input signals were routed to
- i.e. a GPIO controller could control which GPIO inputs provided
wakeup, or a USB controller would wake if some bus condition was seen
etc. I suppose it's possible that a pin controller itself could wake the
system, but how would the wake reason be mapped to something that could
interpret that? If co-ordination between e.g. the GPIO driver an pinctrl
driver is needed to achieve this, shouldn't the GPIO driver call into
some private API in the pinctrl driver to set this up, similar to how
pinctrl_gpio_direction_input() works?

Still, if there's a specific reason for this I don't see any need to
oppose it; it just seems surprising it'd be needed.

> + * @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.
> + */
> +enum pin_config_param {
> +       PIN_CONFIG_BIAS_DISABLE,
> +       PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> +       PIN_CONFIG_BIAS_PULL_UP,
> +       PIN_CONFIG_BIAS_PULL_DOWN,
> +       PIN_CONFIG_BIAS_HIGH,
> +       PIN_CONFIG_BIAS_GROUND,
> +       PIN_CONFIG_DRIVE_PUSH_PULL,
> +       PIN_CONFIG_DRIVE_OPEN_DRAIN,
> +       PIN_CONFIG_DRIVE_OPEN_SOURCE,
> +       PIN_CONFIG_DRIVE_OFF,
> +       PIN_CONFIG_INPUT_SCHMITT,
> +       PIN_CONFIG_INPUT_DEBOUNCE,
> +       PIN_CONFIG_SLEW_RATE_RISING,
> +       PIN_CONFIG_SLEW_RATE_FALLING,
> +       PIN_CONFIG_POWER_SOURCE,
> +       PIN_CONFIG_LOW_POWER_MODE,
> +       PIN_CONFIG_WAKEUP,
> +       PIN_CONFIG_END,
> +};

I think this should be given a large static value, e.g. 0x8000. That way
the value will never change, and hence the values of custom enumerations
will never change.

That isn't important for mapping tables/... provided by data in the
kernel since a recompile would trivially adapt to changes, but it might
be for device tree; someone might want to represent the raw custom
pin_config_param enumeration values directly in device tree (so they
could read an int from device tree and cast it directly to an enum
pin_config_param without using a lookup table to do the conversion), and
the meanings of values in device tree need to stay static.

Of course, you could argue that drivers parsing DT and creating mapping
tables from it must always do an explicit mapping, in which case having
PIN_CONFIG_END floating is fine.

> +static inline enum pin_config_param to_config_param(unsigned long config)
> +{
> +       return (enum pin_config_param) (config & 0xffffUL);
> +}
> +
> +static inline u16 to_config_argument(unsigned long config)
> +{
> +       return (enum pin_config_param) ((config >> 16) & 0xffffUL);
> +}
> +
> +static inline unsigned long to_config_packed(enum pin_config_param param,
> +                                            u16 argument)
> +{
> +       return (argument << 16) | ((unsigned long) param & 0xffffUL);
> +}

Those function names all have very generic names; shouldn't they be
name-spaced e.g. pinconfig_to_param()/pinconfig_to_argument()?

Shouldn't to_config_packed() either be removed, or rewritten to use
PIN_CONF_PACKED() to remove the duplication?

> +/*
> + * Helpful configuration macro to be used in tables etc.
> + */
> +#define PIN_CONF_PACKED(p, a) ((a << 16) | ((unsigned long) p & 0xffffUL))

> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c

> +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> +                             struct seq_file *s, unsigned pin)
...
> +               /* We want to check out this parameter */
> +               config = to_config_packed(conf_items[i].param, 0);
> +               ret = pin_config_get_for_pin(pctldev, pin, &config);
> +               /* These are legal errors */
> +               if (ret == -EINVAL || ret == -ENOTSUPP)
> +                       continue;

What's the meaning of -EINVAL and -ENOTSUPP in this context? I don't
understand why you'd need two "legal errors" rather than just -ENOTSUPP.

...
> +               seq_puts(s, conf_items[i].display);
> +               /* Print unit if available */
> +               if (conf_items[i].format && to_config_argument(config) != 0)
> +                       seq_printf(s, " (%u %s)", to_config_argument(config),
> +                                  conf_items[i].format);

Wouldn't you always want to dump the value, or if not, wouldn't you only
want to print conf_items[i].display if you're going to print the value too?

[PATCH 1/4] pinctrl: introduce generic pin config

[Stephen Warren]

On 03/06/2012 03:05 PM, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This is a split-off from the earlier patch set which adds generic
> pin configuration for the pin controllers that want it. Since
> we may have a system with mixed generic and custom pin controllers,
> we pass a boolean in the pin controller ops vtable to indicate
> if it is generic.
> 
> ChangeLog v1->v5:
> - Follow parent patch versioning number system.
> - Document the semantic meaning of return values from pin config
>   get functions, so we can iterate over pins and check their
>   properties from debugfs as part of the generic config code.
> - Use proper cast functions in the generic debugfs pin config
>   file.
> - Expand generic config to optionally cover groups too.
> ChangeLog v5->v6:
> - Update to match underlying changes.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
...
> +/**
> + * enum pin_config_param - possible pin configuration parameters
> + * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
> + *     transition from say pull-up to pull-down implies that you disable
> + *     pull-up in the process, this setting disables all biasing.

What's the use-case for that? It seems like if one state's mapping table
entry says:

PIN_CONFIG_BIAS_PULL_UP, value

and another state wants to switch to pull-down, it'd be best to just be
explicit, and say:

PIN_CONFIG_BIAS_PULL_UP, disabled
PIN_CONFIG_BIAS_PULL_DOWN, value

This is somewhat related to my comments on PULL_UP/DOWN below...

> + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
> + *     mode, also know as "third-state" (tristate) or "high-Z" or "floating".
> + *     On output pins this effectively disconnects the pin, which is useful
> + *     if for example some other pin is going to drive the signal connected
> + *     to it for a while. Pins used for input are usually always high
> + *     impedance.

Isn't this the same as DRIVE_OFF?

> + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
> + *     impedance to VDD), if the controller supports specifying a certain
> + *     pull-up resistance, this is given as an argument (in Ohms) when
> + *     setting this parameter.
> + * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
> + *     impedance to GROUND), if the controller supports specifying a certain
> + *     pull-down resistance, this is given as an argument (in Ohms) when
> + *     setting this parameter.

I don't think its a good idea to combine enabling/disabling features
with configuring values for those features. If an SoC only has on/off
control for pull-up, and the resistance isn't documented, what value
does the driver author guess and require as the value here?

Even when multiple pull-up/down strengths are available, it's quite
plausible they're just documented as "weak" and "strong", and requiring
someone to go find out what the exact values are rather than just using
an enum disabled/enabled,weak=0/enabled,strong=1/ is going to be annoying.

In other words, I think we'd be better off with:

PIN_CONFIG_BIAS_PULL_UP = enabled/disabled 0/1
PIN_CONFIG_BIAS_PULL_DOWN = enabled/disabled 0/1
PIN_CONFIG_PULL_UP_STRENGTH = enumerated value 0..n-1
PIN_CONFIG_PULL_DOWN_STRENGTH = enumerated value 0..n-1

> + * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
> + * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND

Aren't these special cases of pull-up/down, with nominally zero
resistance? I suppose I could see these having separate control bits
though, so I suppose having explicit PIN_CONFIG_* values is reasonable.

> + * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
> + *     low, this is the most typical case and is typically achieved with two
> + *     active transistors on the output. If the pin can support different
> + *     drive strengths for push/pull, the strength is given in the argument
> + *     as the number of driving stages vs nominal load impedance, so say
> + *     quadruple driving stages (usually 8 transistors rather than two) will
> + *     be configured with the 8 passed as argument.
> + * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
> + *     collector) which means it is usually wired with other output ports
> + *     which are then pulled up with an external resistor. If the pin can
> + *     support different drive strengths for the open drain pin, the format
> + *     is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
> + *     (open emitter) which is the same as open drain but pulled to ground.
> + *     If the pin can support different drive strengths for the open drain
> + *     pin, the format is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
> + * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off.

Again, I don't think that explicitly documenting the units (# of
transistors) here is helpful. Datasheets may just be written in terms of
weak/normal/strong...

Again, this mixes selection of drive type with configuring that drive
type. For example, on some SoCs, PUSH_PULL might have a different set of
available drive strengths for PUSH and PULL hence need two parameters.
Equally, the parameter for OPEN_DRAIN is the same as for PUSH_PULL, so
why not separate selecting the drive type and setting the drive strength
so that the setting of the drive strength can be shared between the
drive types?

So, I think this should be:

PIN_CONFIG_DRIVE = push_pull/open_drain/open_source/tristate
PIN_CONFIG_DRIVE_STRENGTH_UP = enumerated value 0..n-1
PIN_CONFIG_DRIVE_STRENGTH_DOWN = enumerated value 0..n-1

Note that PIN_CONFIG_SLEW_RATE_* are essentially parameterizations of
the drive type just like drive strength is, and they're already separate
config parameters.

> + * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
> + *     schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
> + *     the threshold value is given on a custom format as argument when
> + *     setting pins to this mode. The argument zero turns the schmitt trigger
> + *     off.
> + * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
> + *     which means it will wait for signals to settle when reading inputs. The
> + *     argument gives the debounce time on a custom format. Setting the
> + *     argument to zero turns debouncing off.

> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
> + *     signals on the pin. The argument gives the rise time in fractions
> + *     compared to maximum rise time, 0 means nominal rise time. If you can
> + *     control slew rate in 4 steps these will likely be equidistant like
> + *     1/4, 1/2, 3/4 or full nominal slew rate, which means argument 4 gives
> + *     you 1/4 of nominal slew rate and the argument 4 has the same meaning
> + *     as 0 - nominal slew rate (fastest possible, steep edges). You may want
> + *     to adjust slew rates so that signal edges don't get too steep, causing
> + *     disturbances in surrounding electronics known as electromagnetic
> + *     interference (EMI) for example.
> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
> + *     signals on the pin. The argument gives the fall time in fractions
> + *     compared to nominal fall time.

The same comment here; I think the value of those two should just be an
enumeration 0..n-1 to avoid documenting specific units.

> + * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
> + *     supplies, the argument to this parameter (on a custom format) tells
> + *     the driver which alternative power source to use.
> + * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
> + *     operation, if several modes of operation are supported these can be
> + *     passed in the argument on a custom form, else just use argument 1
> + *     to indicate low power mode, argument 0 turns low power mode off.

> + * @PIN_CONFIG_WAKEUP: this will configure an input pin such that if a
> + *     signal transition arrives at the pin when the pin controller/system
> + *     is sleeping, it will wake up the system if argument 1 is passed along.
> + *     Pass argument 0 to turn wakeup enablement off.

Is this something that's controlled at the pinmux level? I would have
expected it to be controlled by whatever HW input signals were routed to
- i.e. a GPIO controller could control which GPIO inputs provided
wakeup, or a USB controller would wake if some bus condition was seen
etc. I suppose it's possible that a pin controller itself could wake the
system, but how would the wake reason be mapped to something that could
interpret that? If co-ordination between e.g. the GPIO driver an pinctrl
driver is needed to achieve this, shouldn't the GPIO driver call into
some private API in the pinctrl driver to set this up, similar to how
pinctrl_gpio_direction_input() works?

Still, if there's a specific reason for this I don't see any need to
oppose it; it just seems surprising it'd be needed.

> + * @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.
> + */
> +enum pin_config_param {
> +       PIN_CONFIG_BIAS_DISABLE,
> +       PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> +       PIN_CONFIG_BIAS_PULL_UP,
> +       PIN_CONFIG_BIAS_PULL_DOWN,
> +       PIN_CONFIG_BIAS_HIGH,
> +       PIN_CONFIG_BIAS_GROUND,
> +       PIN_CONFIG_DRIVE_PUSH_PULL,
> +       PIN_CONFIG_DRIVE_OPEN_DRAIN,
> +       PIN_CONFIG_DRIVE_OPEN_SOURCE,
> +       PIN_CONFIG_DRIVE_OFF,
> +       PIN_CONFIG_INPUT_SCHMITT,
> +       PIN_CONFIG_INPUT_DEBOUNCE,
> +       PIN_CONFIG_SLEW_RATE_RISING,
> +       PIN_CONFIG_SLEW_RATE_FALLING,
> +       PIN_CONFIG_POWER_SOURCE,
> +       PIN_CONFIG_LOW_POWER_MODE,
> +       PIN_CONFIG_WAKEUP,
> +       PIN_CONFIG_END,
> +};

I think this should be given a large static value, e.g. 0x8000. That way
the value will never change, and hence the values of custom enumerations
will never change.

That isn't important for mapping tables/... provided by data in the
kernel since a recompile would trivially adapt to changes, but it might
be for device tree; someone might want to represent the raw custom
pin_config_param enumeration values directly in device tree (so they
could read an int from device tree and cast it directly to an enum
pin_config_param without using a lookup table to do the conversion), and
the meanings of values in device tree need to stay static.

Of course, you could argue that drivers parsing DT and creating mapping
tables from it must always do an explicit mapping, in which case having
PIN_CONFIG_END floating is fine.

> +static inline enum pin_config_param to_config_param(unsigned long config)
> +{
> +       return (enum pin_config_param) (config & 0xffffUL);
> +}
> +
> +static inline u16 to_config_argument(unsigned long config)
> +{
> +       return (enum pin_config_param) ((config >> 16) & 0xffffUL);
> +}
> +
> +static inline unsigned long to_config_packed(enum pin_config_param param,
> +                                            u16 argument)
> +{
> +       return (argument << 16) | ((unsigned long) param & 0xffffUL);
> +}

Those function names all have very generic names; shouldn't they be
name-spaced e.g. pinconfig_to_param()/pinconfig_to_argument()?

Shouldn't to_config_packed() either be removed, or rewritten to use
PIN_CONF_PACKED() to remove the duplication?

> +/*
> + * Helpful configuration macro to be used in tables etc.
> + */
> +#define PIN_CONF_PACKED(p, a) ((a << 16) | ((unsigned long) p & 0xffffUL))

> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c

> +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> +                             struct seq_file *s, unsigned pin)
...
> +               /* We want to check out this parameter */
> +               config = to_config_packed(conf_items[i].param, 0);
> +               ret = pin_config_get_for_pin(pctldev, pin, &config);
> +               /* These are legal errors */
> +               if (ret == -EINVAL || ret == -ENOTSUPP)
> +                       continue;

What's the meaning of -EINVAL and -ENOTSUPP in this context? I don't
understand why you'd need two "legal errors" rather than just -ENOTSUPP.

...
> +               seq_puts(s, conf_items[i].display);
> +               /* Print unit if available */
> +               if (conf_items[i].format && to_config_argument(config) != 0)
> +                       seq_printf(s, " (%u %s)", to_config_argument(config),
> +                                  conf_items[i].format);

Wouldn't you always want to dump the value, or if not, wouldn't you only
want to print conf_items[i].display if you're going to print the value too?

Re: [PATCH 1/4] pinctrl: introduce generic pin config

[Linus Walleij]

On Wed, Mar 7, 2012 at 11:31 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/06/2012 03:05 PM, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This is a split-off from the earlier patch set which adds generic
>> pin configuration for the pin controllers that want it. Since
>> we may have a system with mixed generic and custom pin controllers,
>> we pass a boolean in the pin controller ops vtable to indicate
>> if it is generic.
>>
>> ChangeLog v1->v5:
>> - Follow parent patch versioning number system.
>> - Document the semantic meaning of return values from pin config
>>   get functions, so we can iterate over pins and check their
>>   properties from debugfs as part of the generic config code.
>> - Use proper cast functions in the generic debugfs pin config
>>   file.
>> - Expand generic config to optionally cover groups too.
>> ChangeLog v5->v6:
>> - Update to match underlying changes.
>>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
>> diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
> ...
>> +/**
>> + * enum pin_config_param - possible pin configuration parameters
>> + * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
>> + *     transition from say pull-up to pull-down implies that you disable
>> + *     pull-up in the process, this setting disables all biasing.
>
> What's the use-case for that? It seems like if one state's mapping table
> entry says:
>
> PIN_CONFIG_BIAS_PULL_UP, value

The mapping table only has an unsigned long really, but yes it
might look something like that syntactically speaking (albeit being
compiled into a constant)

> and another state wants to switch to pull-down, it'd be best to just be
> explicit, and say:
>
> PIN_CONFIG_BIAS_PULL_UP, disabled
> PIN_CONFIG_BIAS_PULL_DOWN, value

I tried to avoid ging state-switching capabilities to arguments.
So going put of bias mode is a state changes and thus has its
own enumerator. Conflating parameters and their arguments
is less elegant IMO.

>> + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
>> + *     mode, also know as "third-state" (tristate) or "high-Z" or "floating".
>> + *     On output pins this effectively disconnects the pin, which is useful
>> + *     if for example some other pin is going to drive the signal connected
>> + *     to it for a while. Pins used for input are usually always high
>> + *     impedance.
>
> Isn't this the same as DRIVE_OFF?

Yeah probably, so I'll drop DRIVE_OFF and keeå high impedance, because
it's more precise.

>> + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
>> + *     impedance to VDD), if the controller supports specifying a certain
>> + *     pull-up resistance, this is given as an argument (in Ohms) when
>> + *     setting this parameter.
>> + * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
>> + *     impedance to GROUND), if the controller supports specifying a certain
>> + *     pull-down resistance, this is given as an argument (in Ohms) when
>> + *     setting this parameter.
>
> I don't think its a good idea to combine enabling/disabling features
> with configuring values for those features. If an SoC only has on/off
> control for pull-up, and the resistance isn't documented, what value
> does the driver author guess and require as the value here?
>
> Even when multiple pull-up/down strengths are available, it's quite
> plausible they're just documented as "weak" and "strong", and requiring
> someone to go find out what the exact values are rather than just using
> an enum disabled/enabled,weak=0/enabled,strong=1/ is going to be annoying.
>
> In other words, I think we'd be better off with:
>
> PIN_CONFIG_BIAS_PULL_UP = enabled/disabled 0/1
> PIN_CONFIG_BIAS_PULL_DOWN = enabled/disabled 0/1
> PIN_CONFIG_PULL_UP_STRENGTH = enumerated value 0..n-1
> PIN_CONFIG_PULL_DOWN_STRENGTH = enumerated value 0..n-1

Hm! I like the looks of this. I'm going to split them anyway.

But what's wrong with just using Ohms as the parameter?
That's what it's all about in the end is it not?
(The U300 GPIOs are 50 kOhm BTW, clearly specified.)

(Here I'm vaguely remembering some implementers not having a
clue what their enumerators really meant and me being alien to
the concept of incomplete hardware docs.)

I'm taking strength out for adding later, I only need
enable/disable for the U300 driver anyway.

>> + * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
>> + * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND
>
> Aren't these special cases of pull-up/down, with nominally zero
> resistance? I suppose I could see these having separate control bits
> though, so I suppose having explicit PIN_CONFIG_* values is reasonable.

Hm right, following my own argument above to encode pull up/down
arguments as Ohms I'll just delete them. Can be added back if we
encounter a HW like that.

>> + * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
>> + *     low, this is the most typical case and is typically achieved with two
>> + *     active transistors on the output. If the pin can support different
>> + *     drive strengths for push/pull, the strength is given in the argument
>> + *     as the number of driving stages vs nominal load impedance, so say
>> + *     quadruple driving stages (usually 8 transistors rather than two) will
>> + *     be configured with the 8 passed as argument.
>> + * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
>> + *     collector) which means it is usually wired with other output ports
>> + *     which are then pulled up with an external resistor. If the pin can
>> + *     support different drive strengths for the open drain pin, the format
>> + *     is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
>> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
>> + *     (open emitter) which is the same as open drain but pulled to ground.
>> + *     If the pin can support different drive strengths for the open drain
>> + *     pin, the format is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
>> + * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off.
>
> Again, I don't think that explicitly documenting the units (# of
> transistors) here is helpful. Datasheets may just be written in terms of
> weak/normal/strong...

Since I don't need these for this implementation I'll just delete them
and postpone the inclusion to the day we have a hardware that needs
them.

> Again, this mixes selection of drive type with configuring that drive
> type. For example, on some SoCs, PUSH_PULL might have a different set of
> available drive strengths for PUSH and PULL hence need two parameters.

Hm, interesting! But that very fact again underscores the underlying
silicon construct as a number of driver stages.

> Equally, the parameter for OPEN_DRAIN is the same as for PUSH_PULL, so
> why not separate selecting the drive type and setting the drive strength
> so that the setting of the drive strength can be shared between the
> drive types?

OK atleast I'm taking the argument out and replacing with just
enable/disable for now, so we can discuss that separately.

> So, I think this should be:
>
> PIN_CONFIG_DRIVE = push_pull/open_drain/open_source/tristate

Tristate is not a drive mode but equivalent to
BIAS_HIGH_IMPEDANCE.

I'll go for three parameters for each drive mode and the
argument ignored, I'm not copying the schem from pull up/down
with an enable/disable argument since you probably cannot "disable"
push-pull but rather have to set the pin to high impedance or OD, or
OS rather i.e. an implicit state transition, then it's better to have
that explicit.

> PIN_CONFIG_DRIVE_STRENGTH_UP = enumerated value 0..n-1
> PIN_CONFIG_DRIVE_STRENGTH_DOWN = enumerated value 0..n-1

Saving this stuff for later. Let's agree on a starting
point first.

> Note that PIN_CONFIG_SLEW_RATE_* are essentially parameterizations of
> the drive type just like drive strength is, and they're already separate
> config parameters.

True.

>> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
>> + *     signals on the pin. The argument gives the rise time in fractions
>> + *     compared to maximum rise time, 0 means nominal rise time. If you can
>> + *     control slew rate in 4 steps these will likely be equidistant like
>> + *     1/4, 1/2, 3/4 or full nominal slew rate, which means argument 4 gives
>> + *     you 1/4 of nominal slew rate and the argument 4 has the same meaning
>> + *     as 0 - nominal slew rate (fastest possible, steep edges). You may want
>> + *     to adjust slew rates so that signal edges don't get too steep, causing
>> + *     disturbances in surrounding electronics known as electromagnetic
>> + *     interference (EMI) for example.
>> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
>> + *     signals on the pin. The argument gives the fall time in fractions
>> + *     compared to nominal fall time.
>
> The same comment here; I think the value of those two should just be an
> enumeration 0..n-1 to avoid documenting specific units.

I'm taking them out for later discussion alongside the first driver that
need them.

>> + * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
>> + *     supplies, the argument to this parameter (on a custom format) tells
>> + *     the driver which alternative power source to use.
>> + * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
>> + *     operation, if several modes of operation are supported these can be
>> + *     passed in the argument on a custom form, else just use argument 1
>> + *     to indicate low power mode, argument 0 turns low power mode off.
>
>> + * @PIN_CONFIG_WAKEUP: this will configure an input pin such that if a
>> + *     signal transition arrives at the pin when the pin controller/system
>> + *     is sleeping, it will wake up the system if argument 1 is passed along.
>> + *     Pass argument 0 to turn wakeup enablement off.
>
> Is this something that's controlled at the pinmux level? I would have
> expected it to be controlled by whatever HW input signals were routed to
> - i.e. a GPIO controller could control which GPIO inputs provided
> wakeup, or a USB controller would wake if some bus condition was seen
> etc. I suppose it's possible that a pin controller itself could wake the
> system, but how would the wake reason be mapped to something that could
> interpret that? If co-ordination between e.g. the GPIO driver an pinctrl
> driver is needed to achieve this, shouldn't the GPIO driver call into
> some private API in the pinctrl driver to set this up, similar to how
> pinctrl_gpio_direction_input() works?
>
> Still, if there's a specific reason for this I don't see any need to
> oppose it; it just seems surprising it'd be needed.

No I think it's plain wrong actually.

The struct irq_chip already provides methods for setting pins to
wakeup, so I'm just deleteing this.

There could be a usecase for a complex pin controller that can wake
up the system on a pin which however isn't going to generate an IRQ,
but that sounds backwards to me, and we can add it the day we
need it in that case.

>> + * @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.
>> + */
>> +enum pin_config_param {
>> +       PIN_CONFIG_BIAS_DISABLE,
>> +       PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
>> +       PIN_CONFIG_BIAS_PULL_UP,
>> +       PIN_CONFIG_BIAS_PULL_DOWN,
>> +       PIN_CONFIG_BIAS_HIGH,
>> +       PIN_CONFIG_BIAS_GROUND,
>> +       PIN_CONFIG_DRIVE_PUSH_PULL,
>> +       PIN_CONFIG_DRIVE_OPEN_DRAIN,
>> +       PIN_CONFIG_DRIVE_OPEN_SOURCE,
>> +       PIN_CONFIG_DRIVE_OFF,
>> +       PIN_CONFIG_INPUT_SCHMITT,
>> +       PIN_CONFIG_INPUT_DEBOUNCE,
>> +       PIN_CONFIG_SLEW_RATE_RISING,
>> +       PIN_CONFIG_SLEW_RATE_FALLING,
>> +       PIN_CONFIG_POWER_SOURCE,
>> +       PIN_CONFIG_LOW_POWER_MODE,
>> +       PIN_CONFIG_WAKEUP,
>> +       PIN_CONFIG_END,
>> +};
>
> I think this should be given a large static value, e.g. 0x8000. That way
> the value will never change, and hence the values of custom enumerations
> will never change.

PIN_CONFIG_END you mean?

> That isn't important for mapping tables/... provided by data in the
> kernel since a recompile would trivially adapt to changes, but it might
> be for device tree; someone might want to represent the raw custom
> pin_config_param enumeration values directly in device tree (so they
> could read an int from device tree and cast it directly to an enum
> pin_config_param without using a lookup table to do the conversion), and
> the meanings of values in device tree need to stay static.
>
> Of course, you could argue that drivers parsing DT and creating mapping
> tables from it must always do an explicit mapping, in which case having
> PIN_CONFIG_END floating is fine.

We can have both. But I will set it to 0x7fff and let 0x8000 be the first
custom parameter...

>> +static inline enum pin_config_param to_config_param(unsigned long config)
>> +{
>> +       return (enum pin_config_param) (config & 0xffffUL);
>> +}
>> +
>> +static inline u16 to_config_argument(unsigned long config)
>> +{
>> +       return (enum pin_config_param) ((config >> 16) & 0xffffUL);
>> +}
>> +
>> +static inline unsigned long to_config_packed(enum pin_config_param param,
>> +                                            u16 argument)
>> +{
>> +       return (argument << 16) | ((unsigned long) param & 0xffffUL);
>> +}
>
> Those function names all have very generic names; shouldn't they be
> name-spaced e.g. pinconfig_to_param()/pinconfig_to_argument()?

Yep, I'll fix.

> Shouldn't to_config_packed() either be removed, or rewritten to use
> PIN_CONF_PACKED() to remove the duplication?

Ah. The latter was for tables so needed to be compile-time
resolvable. Didn't see the duplication. Will move the macro
above the function and use the macro in the static inline!

>> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
>
>> +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
>> +                             struct seq_file *s, unsigned pin)
> ...
>> +               /* We want to check out this parameter */
>> +               config = to_config_packed(conf_items[i].param, 0);
>> +               ret = pin_config_get_for_pin(pctldev, pin, &config);
>> +               /* These are legal errors */
>> +               if (ret == -EINVAL || ret == -ENOTSUPP)
>> +                       continue;
>
> What's the meaning of -EINVAL and -ENOTSUPP in this context? I don't
> understand why you'd need two "legal errors" rather than just -ENOTSUPP.

The reasoning is that -ENOTSUPP cannot be done by this
pin controller at all while -EINVAL is returned when the pin controller
does support the parameter, bit it has no meaning right now.

For example the pin controller might not support pull down
at all and then returns -ENOTSUPP for any attempt to read that
code.

However it might be that it does support pull-down, but right now
it is in output (drive) mode so there is nothing relevant to say
about pull-down right now.

I gave these two different error codes so we could tell them
apart, semantically speaking, practically you can write code doing
the same thing with just -ENOTSUPP but then this return code
means one of two things and you don't know which one it is.

The use cases in debugfs is:

(A) Show all settings right now (current implementation)
   iterate/skip over all error codes like above

(B) Show all *possible* settings (can be added if we can
  tell these two apart) show "available" for anything
  -EINVAL but do not show at all anything reporting
  -ENOTSUPP.

I think I had this documented at some point and it got lost..
will reinsert it somewhere.

> ...
>> +               seq_puts(s, conf_items[i].display);
>> +               /* Print unit if available */
>> +               if (conf_items[i].format && to_config_argument(config) != 0)
>> +                       seq_printf(s, " (%u %s)", to_config_argument(config),
>> +                                  conf_items[i].format);
>
> Wouldn't you always want to dump the value, or if not, wouldn't you only
> want to print conf_items[i].display if you're going to print the value too?

We have some parameters where the value is meaningless so
we don't want to show these at all.

Let me post v7 so we can discuss from there.

Yours,
Linus Walleij

[PATCH 1/4] pinctrl: introduce generic pin config

[Linus Walleij]

On Wed, Mar 7, 2012 at 11:31 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/06/2012 03:05 PM, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This is a split-off from the earlier patch set which adds generic
>> pin configuration for the pin controllers that want it. Since
>> we may have a system with mixed generic and custom pin controllers,
>> we pass a boolean in the pin controller ops vtable to indicate
>> if it is generic.
>>
>> ChangeLog v1->v5:
>> - Follow parent patch versioning number system.
>> - Document the semantic meaning of return values from pin config
>> ? get functions, so we can iterate over pins and check their
>> ? properties from debugfs as part of the generic config code.
>> - Use proper cast functions in the generic debugfs pin config
>> ? file.
>> - Expand generic config to optionally cover groups too.
>> ChangeLog v5->v6:
>> - Update to match underlying changes.
>>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
>> diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
> ...
>> +/**
>> + * enum pin_config_param - possible pin configuration parameters
>> + * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
>> + * ? ? transition from say pull-up to pull-down implies that you disable
>> + * ? ? pull-up in the process, this setting disables all biasing.
>
> What's the use-case for that? It seems like if one state's mapping table
> entry says:
>
> PIN_CONFIG_BIAS_PULL_UP, value

The mapping table only has an unsigned long really, but yes it
might look something like that syntactically speaking (albeit being
compiled into a constant)

> and another state wants to switch to pull-down, it'd be best to just be
> explicit, and say:
>
> PIN_CONFIG_BIAS_PULL_UP, disabled
> PIN_CONFIG_BIAS_PULL_DOWN, value

I tried to avoid ging state-switching capabilities to arguments.
So going put of bias mode is a state changes and thus has its
own enumerator. Conflating parameters and their arguments
is less elegant IMO.

>> + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
>> + * ? ? mode, also know as "third-state" (tristate) or "high-Z" or "floating".
>> + * ? ? On output pins this effectively disconnects the pin, which is useful
>> + * ? ? if for example some other pin is going to drive the signal connected
>> + * ? ? to it for a while. Pins used for input are usually always high
>> + * ? ? impedance.
>
> Isn't this the same as DRIVE_OFF?

Yeah probably, so I'll drop DRIVE_OFF and kee? high impedance, because
it's more precise.

>> + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
>> + * ? ? impedance to VDD), if the controller supports specifying a certain
>> + * ? ? pull-up resistance, this is given as an argument (in Ohms) when
>> + * ? ? setting this parameter.
>> + * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
>> + * ? ? impedance to GROUND), if the controller supports specifying a certain
>> + * ? ? pull-down resistance, this is given as an argument (in Ohms) when
>> + * ? ? setting this parameter.
>
> I don't think its a good idea to combine enabling/disabling features
> with configuring values for those features. If an SoC only has on/off
> control for pull-up, and the resistance isn't documented, what value
> does the driver author guess and require as the value here?
>
> Even when multiple pull-up/down strengths are available, it's quite
> plausible they're just documented as "weak" and "strong", and requiring
> someone to go find out what the exact values are rather than just using
> an enum disabled/enabled,weak=0/enabled,strong=1/ is going to be annoying.
>
> In other words, I think we'd be better off with:
>
> PIN_CONFIG_BIAS_PULL_UP = enabled/disabled 0/1
> PIN_CONFIG_BIAS_PULL_DOWN = enabled/disabled 0/1
> PIN_CONFIG_PULL_UP_STRENGTH = enumerated value 0..n-1
> PIN_CONFIG_PULL_DOWN_STRENGTH = enumerated value 0..n-1

Hm! I like the looks of this. I'm going to split them anyway.

But what's wrong with just using Ohms as the parameter?
That's what it's all about in the end is it not?
(The U300 GPIOs are 50 kOhm BTW, clearly specified.)

(Here I'm vaguely remembering some implementers not having a
clue what their enumerators really meant and me being alien to
the concept of incomplete hardware docs.)

I'm taking strength out for adding later, I only need
enable/disable for the U300 driver anyway.

>> + * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
>> + * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND
>
> Aren't these special cases of pull-up/down, with nominally zero
> resistance? I suppose I could see these having separate control bits
> though, so I suppose having explicit PIN_CONFIG_* values is reasonable.

Hm right, following my own argument above to encode pull up/down
arguments as Ohms I'll just delete them. Can be added back if we
encounter a HW like that.

>> + * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
>> + * ? ? low, this is the most typical case and is typically achieved with two
>> + * ? ? active transistors on the output. If the pin can support different
>> + * ? ? drive strengths for push/pull, the strength is given in the argument
>> + * ? ? as the number of driving stages vs nominal load impedance, so say
>> + * ? ? quadruple driving stages (usually 8 transistors rather than two) will
>> + * ? ? be configured with the 8 passed as argument.
>> + * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
>> + * ? ? collector) which means it is usually wired with other output ports
>> + * ? ? which are then pulled up with an external resistor. If the pin can
>> + * ? ? support different drive strengths for the open drain pin, the format
>> + * ? ? is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
>> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
>> + * ? ? (open emitter) which is the same as open drain but pulled to ground.
>> + * ? ? If the pin can support different drive strengths for the open drain
>> + * ? ? pin, the format is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
>> + * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off.
>
> Again, I don't think that explicitly documenting the units (# of
> transistors) here is helpful. Datasheets may just be written in terms of
> weak/normal/strong...

Since I don't need these for this implementation I'll just delete them
and postpone the inclusion to the day we have a hardware that needs
them.

> Again, this mixes selection of drive type with configuring that drive
> type. For example, on some SoCs, PUSH_PULL might have a different set of
> available drive strengths for PUSH and PULL hence need two parameters.

Hm, interesting! But that very fact again underscores the underlying
silicon construct as a number of driver stages.

> Equally, the parameter for OPEN_DRAIN is the same as for PUSH_PULL, so
> why not separate selecting the drive type and setting the drive strength
> so that the setting of the drive strength can be shared between the
> drive types?

OK atleast I'm taking the argument out and replacing with just
enable/disable for now, so we can discuss that separately.

> So, I think this should be:
>
> PIN_CONFIG_DRIVE = push_pull/open_drain/open_source/tristate

Tristate is not a drive mode but equivalent to
BIAS_HIGH_IMPEDANCE.

I'll go for three parameters for each drive mode and the
argument ignored, I'm not copying the schem from pull up/down
with an enable/disable argument since you probably cannot "disable"
push-pull but rather have to set the pin to high impedance or OD, or
OS rather i.e. an implicit state transition, then it's better to have
that explicit.

> PIN_CONFIG_DRIVE_STRENGTH_UP = enumerated value 0..n-1
> PIN_CONFIG_DRIVE_STRENGTH_DOWN = enumerated value 0..n-1

Saving this stuff for later. Let's agree on a starting
point first.

> Note that PIN_CONFIG_SLEW_RATE_* are essentially parameterizations of
> the drive type just like drive strength is, and they're already separate
> config parameters.

True.

>> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
>> + * ? ? signals on the pin. The argument gives the rise time in fractions
>> + * ? ? compared to maximum rise time, 0 means nominal rise time. If you can
>> + * ? ? control slew rate in 4 steps these will likely be equidistant like
>> + * ? ? 1/4, 1/2, 3/4 or full nominal slew rate, which means argument 4 gives
>> + * ? ? you 1/4 of nominal slew rate and the argument 4 has the same meaning
>> + * ? ? as 0 - nominal slew rate (fastest possible, steep edges). You may want
>> + * ? ? to adjust slew rates so that signal edges don't get too steep, causing
>> + * ? ? disturbances in surrounding electronics known as electromagnetic
>> + * ? ? interference (EMI) for example.
>> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
>> + * ? ? signals on the pin. The argument gives the fall time in fractions
>> + * ? ? compared to nominal fall time.
>
> The same comment here; I think the value of those two should just be an
> enumeration 0..n-1 to avoid documenting specific units.

I'm taking them out for later discussion alongside the first driver that
need them.

>> + * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
>> + * ? ? supplies, the argument to this parameter (on a custom format) tells
>> + * ? ? the driver which alternative power source to use.
>> + * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
>> + * ? ? operation, if several modes of operation are supported these can be
>> + * ? ? passed in the argument on a custom form, else just use argument 1
>> + * ? ? to indicate low power mode, argument 0 turns low power mode off.
>
>> + * @PIN_CONFIG_WAKEUP: this will configure an input pin such that if a
>> + * ? ? signal transition arrives at the pin when the pin controller/system
>> + * ? ? is sleeping, it will wake up the system if argument 1 is passed along.
>> + * ? ? Pass argument 0 to turn wakeup enablement off.
>
> Is this something that's controlled at the pinmux level? I would have
> expected it to be controlled by whatever HW input signals were routed to
> - i.e. a GPIO controller could control which GPIO inputs provided
> wakeup, or a USB controller would wake if some bus condition was seen
> etc. I suppose it's possible that a pin controller itself could wake the
> system, but how would the wake reason be mapped to something that could
> interpret that? If co-ordination between e.g. the GPIO driver an pinctrl
> driver is needed to achieve this, shouldn't the GPIO driver call into
> some private API in the pinctrl driver to set this up, similar to how
> pinctrl_gpio_direction_input() works?
>
> Still, if there's a specific reason for this I don't see any need to
> oppose it; it just seems surprising it'd be needed.

No I think it's plain wrong actually.

The struct irq_chip already provides methods for setting pins to
wakeup, so I'm just deleteing this.

There could be a usecase for a complex pin controller that can wake
up the system on a pin which however isn't going to generate an IRQ,
but that sounds backwards to me, and we can add it the day we
need it in that case.

>> + * @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.
>> + */
>> +enum pin_config_param {
>> + ? ? ? PIN_CONFIG_BIAS_DISABLE,
>> + ? ? ? PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
>> + ? ? ? PIN_CONFIG_BIAS_PULL_UP,
>> + ? ? ? PIN_CONFIG_BIAS_PULL_DOWN,
>> + ? ? ? PIN_CONFIG_BIAS_HIGH,
>> + ? ? ? PIN_CONFIG_BIAS_GROUND,
>> + ? ? ? PIN_CONFIG_DRIVE_PUSH_PULL,
>> + ? ? ? PIN_CONFIG_DRIVE_OPEN_DRAIN,
>> + ? ? ? PIN_CONFIG_DRIVE_OPEN_SOURCE,
>> + ? ? ? PIN_CONFIG_DRIVE_OFF,
>> + ? ? ? PIN_CONFIG_INPUT_SCHMITT,
>> + ? ? ? PIN_CONFIG_INPUT_DEBOUNCE,
>> + ? ? ? PIN_CONFIG_SLEW_RATE_RISING,
>> + ? ? ? PIN_CONFIG_SLEW_RATE_FALLING,
>> + ? ? ? PIN_CONFIG_POWER_SOURCE,
>> + ? ? ? PIN_CONFIG_LOW_POWER_MODE,
>> + ? ? ? PIN_CONFIG_WAKEUP,
>> + ? ? ? PIN_CONFIG_END,
>> +};
>
> I think this should be given a large static value, e.g. 0x8000. That way
> the value will never change, and hence the values of custom enumerations
> will never change.

PIN_CONFIG_END you mean?

> That isn't important for mapping tables/... provided by data in the
> kernel since a recompile would trivially adapt to changes, but it might
> be for device tree; someone might want to represent the raw custom
> pin_config_param enumeration values directly in device tree (so they
> could read an int from device tree and cast it directly to an enum
> pin_config_param without using a lookup table to do the conversion), and
> the meanings of values in device tree need to stay static.
>
> Of course, you could argue that drivers parsing DT and creating mapping
> tables from it must always do an explicit mapping, in which case having
> PIN_CONFIG_END floating is fine.

We can have both. But I will set it to 0x7fff and let 0x8000 be the first
custom parameter...

>> +static inline enum pin_config_param to_config_param(unsigned long config)
>> +{
>> + ? ? ? return (enum pin_config_param) (config & 0xffffUL);
>> +}
>> +
>> +static inline u16 to_config_argument(unsigned long config)
>> +{
>> + ? ? ? return (enum pin_config_param) ((config >> 16) & 0xffffUL);
>> +}
>> +
>> +static inline unsigned long to_config_packed(enum pin_config_param param,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?u16 argument)
>> +{
>> + ? ? ? return (argument << 16) | ((unsigned long) param & 0xffffUL);
>> +}
>
> Those function names all have very generic names; shouldn't they be
> name-spaced e.g. pinconfig_to_param()/pinconfig_to_argument()?

Yep, I'll fix.

> Shouldn't to_config_packed() either be removed, or rewritten to use
> PIN_CONF_PACKED() to remove the duplication?

Ah. The latter was for tables so needed to be compile-time
resolvable. Didn't see the duplication. Will move the macro
above the function and use the macro in the static inline!

>> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
>
>> +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct seq_file *s, unsigned pin)
> ...
>> + ? ? ? ? ? ? ? /* We want to check out this parameter */
>> + ? ? ? ? ? ? ? config = to_config_packed(conf_items[i].param, 0);
>> + ? ? ? ? ? ? ? ret = pin_config_get_for_pin(pctldev, pin, &config);
>> + ? ? ? ? ? ? ? /* These are legal errors */
>> + ? ? ? ? ? ? ? if (ret == -EINVAL || ret == -ENOTSUPP)
>> + ? ? ? ? ? ? ? ? ? ? ? continue;
>
> What's the meaning of -EINVAL and -ENOTSUPP in this context? I don't
> understand why you'd need two "legal errors" rather than just -ENOTSUPP.

The reasoning is that -ENOTSUPP cannot be done by this
pin controller at all while -EINVAL is returned when the pin controller
does support the parameter, bit it has no meaning right now.

For example the pin controller might not support pull down
at all and then returns -ENOTSUPP for any attempt to read that
code.

However it might be that it does support pull-down, but right now
it is in output (drive) mode so there is nothing relevant to say
about pull-down right now.

I gave these two different error codes so we could tell them
apart, semantically speaking, practically you can write code doing
the same thing with just -ENOTSUPP but then this return code
means one of two things and you don't know which one it is.

The use cases in debugfs is:

(A) Show all settings right now (current implementation)
   iterate/skip over all error codes like above

(B) Show all *possible* settings (can be added if we can
  tell these two apart) show "available" for anything
  -EINVAL but do not show at all anything reporting
  -ENOTSUPP.

I think I had this documented at some point and it got lost..
will reinsert it somewhere.

> ...
>> + ? ? ? ? ? ? ? seq_puts(s, conf_items[i].display);
>> + ? ? ? ? ? ? ? /* Print unit if available */
>> + ? ? ? ? ? ? ? if (conf_items[i].format && to_config_argument(config) != 0)
>> + ? ? ? ? ? ? ? ? ? ? ? seq_printf(s, " (%u %s)", to_config_argument(config),
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?conf_items[i].format);
>
> Wouldn't you always want to dump the value, or if not, wouldn't you only
> want to print conf_items[i].display if you're going to print the value too?

We have some parameters where the value is meaningless so
we don't want to show these at all.

Let me post v7 so we can discuss from there.

Yours,
Linus Walleij