[PATCH 2/2] pinctrl: add a generic control interface

[PATCH 2/2] pinctrl: add a generic control interface

[Linus Walleij]

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

This add per-pin and per-group pin control interfaces for biasing,
driving and other such electronic properties. The intention is
clearly to enumerate all things you can do with pins, hoping that
these are enumerable.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/pinctrl.txt       |   55 ++++++++++++++++++-
 drivers/pinctrl/core.c          |   69 ++++++++++++++++++++++++
 include/linux/pinctrl/pinctrl.h |  112 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 232 insertions(+), 4 deletions(-)

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index b04cb7d..328adb7 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -7,8 +7,6 @@ This subsystem deals with:
 
 - Multiplexing of pins, pads, fingers (etc) see below for details
 
-The intention is to also deal with:
-
 - Software-controlled biasing and driving mode specific pins, such as
   pull-up/down, open drain etc, load capacitance configuration when controlled
   by software, etc.
@@ -193,6 +191,59 @@ structure, for example specific register ranges associated with each group
 and so on.
 
 
+Pin configuration
+=================
+
+Pins can sometimes be software-configured in an tremendous amount of ways,
+mostly related to their electronic properties when used as inputs or outputs.
+For example you may be able to make an output pin high impedance, or "tristate"
+meaning it is effectively disconnected. You may be able to connect an input pin
+to VDD or GND using a certain resistor value - pull up and pull down - so that
+the pin has a stable value when nothing is driving the rail it is connected
+to, or when it's unconnected.
+
+The pin control system supports an interface partly abstracting these
+properties while leaving the details to the pin control driver.
+
+For example, a driver can do this:
+
+ret = pin_config(128, PIN_CONFIG_BIAS_PULL_UP, 100000);
+
+To pull up a pin to VDD with a 100KOhm resistor. The driver implements
+callbacks for changing pin configuration in the pin controller ops like this:
+
+int foo_pin_config (struct pinctrl_dev *pctldev,
+		    unsigned pin,
+		    enum pin_config_param param,
+		    unsigned long data)
+{
+	switch (param) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+	     ...
+}
+
+int foo_pin_config_group (struct pinctrl_dev *pctldev,
+		    unsigned selector,
+		    enum pin_config_param param,
+		    unsigned long data)
+{
+	...
+}
+
+static struct pinctrl_ops foo_pctrl_ops = {
+       ...
+       pin_config = foo_pin_config,
+       pin_config_group = foo_pin_config_group,
+};
+
+Since some controllers have special logic for handling entire groups of pins
+they can exploit the special whole-group pin control function. The
+pin_config_group() callback is allowed to return the error code -EAGAIN,
+for groups it does not want to handle, or if it just wants to do some
+group-level handling in which case each individual pin will be handled by
+separate pin_config() calls as well.
+
+
 Interaction with the GPIO subsystem
 ===================================
 
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 478a002..913f760 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -315,6 +315,75 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
 	return -EINVAL;
 }
 
+int pin_config(struct pinctrl_dev *pctldev, int pin,
+	       enum pin_config_param param, unsigned long data)
+{
+	const struct pinctrl_ops *ops = pctldev->desc->pctlops;
+
+	if (!ops->pin_config) {
+		dev_err(&pctldev->dev, "cannot configure pin, missing "
+			"config function in driver\n");
+		return -EINVAL;
+	}
+
+	return ops->pin_config(pctldev, pin, param, data);
+}
+
+int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
+		     enum pin_config_param param, unsigned long data)
+{
+	const struct pinctrl_ops *ops = pctldev->desc->pctlops;
+	int selector;
+	unsigned *pins;
+	unsigned num_pins;
+	int ret;
+	int i;
+
+	if (!ops->pin_config_group && !ops->pin_config) {
+		dev_err(&pctldev->dev, "cannot configure pin group, missing "
+			"config function in driver\n");
+		return -EINVAL;
+	}
+
+	selector = pinctrl_get_group_selector(pctldev, pin_group);
+	if (selector < 0)
+		return selector;
+
+	/*
+	 * If the pin controller supports handling entire groups we use that
+	 * capability.
+	 */
+	if (ops->pin_config_group) {
+		ret = ops->pin_config_group(pctldev, selector, param, data);
+		/*
+		 * If the pin controller prefer that a certain group be handled
+		 * pin-by-pin as well, it returns -EAGAIN.
+		 */
+		if (ret != -EAGAIN)
+			return ret;
+	}
+
+	/*
+	 * If the controller cannot handle entire groups, we configure each pin
+	 * individually.
+	 */
+	ret = ops->get_group_pins(pctldev, selector,
+				  &pins, &num_pins);
+	if (ret) {
+		dev_err(&pctldev->dev, "cannot configure pin group, error "
+			"getting pins\n");
+		return ret;
+	}
+
+	for (i = 0; i < num_pins; i++) {
+		ret = pin_config(pctldev, pins[i], param, data);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_DEBUG_FS
 
 static int pinctrl_pins_show(struct seq_file *s, void *what)
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 4f8d208..189c047 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -19,6 +19,87 @@
 #include <linux/list.h>
 #include <linux/seq_file.h>
 
+/**
+ * enum pin_config_param - possible pin configuration parameters
+ * @PIN_CONFIG_BIAS_UNKNOWN: this bias mode is not known to us
+ * @PIN_CONFIG_BIAS_FLOAT: no specific bias, the pin will float or state
+ *	is not controlled by software
+ * @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_UNKNOWN: we don't know the drive mode of this pin, for
+ *	example since it is controlled by hardware or the information is not
+ *	accessible but we need a meaningful enumerator in e.g. initialization
+ *	code
+ * @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 on a custom format
+ *	as argument when setting pins to this mode
+ * @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 strength
+ *	is given on a custom format as argument when setting pins to this mode
+ * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
+ *	(open emitter) which is the same as open drain mutatis mutandis but
+ *	pulled to ground. If the pin can support different drive strengths for
+ *	the open drain pin, the strength is given on a custom format as
+ *	argument when setting pins to this mode
+ * @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
+ * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
+ *	signals on the pin. The argument gives the rise time in nanoseconds
+ * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
+ *	signals on the pin. The argument gives the fall time in nanoseconds
+ * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
+ *	will deform waveforms when signals are transmitted on them, by
+ *	applying a load capacitance, the waveform can be rectified. The
+ *	argument gives the load capacitance in picofarad (pF)
+ * @PIN_CONFIG_WAKEUP_ENABLE: 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
+ * @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_UNKNOWN,
+	PIN_CONFIG_BIAS_FLOAT,
+	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_UNKNOWN,
+	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_SLEW_RATE_RISING,
+	PIN_CONFIG_SLEW_RATE_FALLING,
+	PIN_CONFIG_LOAD_CAPACITANCE,
+	PIN_CONFIG_WAKEUP_ENABLE,
+	PIN_CONFIG_END,
+};
+
 struct pinctrl_dev;
 struct pinmux_ops;
 struct gpio_chip;
@@ -66,6 +147,8 @@ struct pinctrl_gpio_range {
  * @get_group_name: return the group name of the pin group
  * @get_group_pins: return an array of pins corresponding to a certain
  *	group selector @pins, and the size of the array in @num_pins
+ * @pin_config: configure an individual pin
+ * @pin_config_group: configure all pins in a group
  * @pin_dbg_show: optional debugfs display hook that will provide per-device
  *	info for a certain pin in debugfs
  */
@@ -77,6 +160,14 @@ struct pinctrl_ops {
 			       unsigned selector,
 			       unsigned ** const pins,
 			       unsigned * const num_pins);
+	int (*pin_config) (struct pinctrl_dev *pctldev,
+			   unsigned pin,
+			   enum pin_config_param param,
+			   unsigned long data);
+	int (*pin_config_group) (struct pinctrl_dev *pctldev,
+				 unsigned selector,
+				 enum pin_config_param param,
+				 unsigned long data);
 	void (*pin_dbg_show) (struct pinctrl_dev *pctldev, struct seq_file *s,
 			  unsigned offset);
 };
@@ -113,6 +204,10 @@ extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 				struct device *dev, void *driver_data);
 extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
 extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin);
+extern int pin_config(struct pinctrl_dev *pctldev, int pin,
+		      enum pin_config_param param, unsigned long data);
+extern int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
+			    enum pin_config_param param, unsigned long data);
 extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
 				struct pinctrl_gpio_range *range);
 extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
@@ -121,13 +216,26 @@ extern const char *pinctrl_dev_get_name(struct pinctrl_dev *pctldev);
 extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
 #else
 
-
-/* Sufficiently stupid default function when pinctrl is not in use */
+/* Sufficiently stupid default functions when pinctrl is not in use */
 static inline bool pin_is_valid(struct pinctrl_dev *pctldev, int pin)
 {
 	return pin >= 0;
 }
 
+static inline int pin_config(struct pinctrl_dev *pctldev, int pin,
+			     enum pin_config_param param, unsigned long data)
+{
+	return 0;
+}
+
+static inline int pin_config_group(struct pinctrl_dev *pctldev,
+				   const char *pin_group,
+				   enum pin_config_param param,
+				   unsigned long data)
+{
+	return 0;
+}
+
 #endif /* !CONFIG_PINCTRL */
 
 #endif /* __LINUX_PINCTRL_PINCTRL_H */
-- 
1.7.3.2

RE: [PATCH 2/2] pinctrl: add a generic control interface

[Stephen Warren]

Linus Walleij wrote at Wednesday, October 19, 2011 10:21 AM:
> This add per-pin and per-group pin control interfaces for biasing,
> driving and other such electronic properties. The intention is
> clearly to enumerate all things you can do with pins, hoping that
> these are enumerable.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h

> +/**
> + * enum pin_config_param - possible pin configuration parameters
> + * @PIN_CONFIG_BIAS_UNKNOWN: this bias mode is not known to us

I'm not sure what use that would be; shouldn't we remove that, and add
new PIN_CONFIG_BIAS_* values as/when they're needed?

> + * @PIN_CONFIG_BIAS_FLOAT: no specific bias, the pin will float or state
> + *	is not controlled by software

"float" and "not controlled by SW" are very different things. How is float
different to high impedance?

> + * @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

What value should be used to disable a pull-up; 0? What value should be
used when requesting pull-up where the SoC doesn't have a configurable
pull-up resistor; anything non-zero? Tegra has separate configurations for
pull-up/down strength (0..31) and pull-up/down enable (up/down/none), though
I suppose we could map 0 to off, 1..32 to on with strength 0..31, although
that'd prevent a driver from toggling the enable bit on/off without knowing
what the strength code should be programmed to...

Tegra's pull-up/down strengths aren't documented in terms of ohms, but
rather a "drive strength code" on scale 0..31. I'm not sure what that
truly maps to in hardware.

> + * @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

At least some of these options appear mutually exclusive; I wonder if we
shouldn't have a PIN_CONFIG_BIAS parameter, and have the legal values be
HIGH/GROUND/NONE. Pull up/down are probably separate. HIGH_IMPEDANCE seems
more like a PIN_CONFIG_DRIVE value than a PIN_CONFIG_BIAS value.

> + * @PIN_CONFIG_DRIVE_UNKNOWN: we don't know the drive mode of this pin, for
> + *	example since it is controlled by hardware or the information is not
> + *	accessible but we need a meaningful enumerator in e.g. initialization
> + *	code

Again, I'm not sure this is useful; if this is not a configurable parameter,
surely initialization code would simply skip attempting to set this param?

> + * @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 on a custom format
> + *	as argument when setting pins to this mode
> + * @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 strength
> + *	is given on a custom format as argument when setting pins to this mode
> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
> + *	(open emitter) which is the same as open drain mutatis mutandis but
> + *	pulled to ground. If the pin can support different drive strengths for
> + *	the open drain pin, the strength is given on a custom format as
> + *	argument when setting pins to this mode
> + * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off

These seem like mutually exclusive values for a PIN_CONFIG_DRIVE param.

> + * @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
> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
> + *	signals on the pin. The argument gives the rise time in nanoseconds
> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
> + *	signals on the pin. The argument gives the fall time in nanoseconds
> + * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
> + *	will deform waveforms when signals are transmitted on them, by
> + *	applying a load capacitance, the waveform can be rectified. The
> + *	argument gives the load capacitance in picofarad (pF)
> + * @PIN_CONFIG_WAKEUP_ENABLE: 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
> + * @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

Tegra needs at least some more:

PIN_CONFIG_HIGH_SPEED_MODE (0..1) [tegra_drive_pingroup_config.hsm]
PIN_CONFIG_LOW_POWER_MODE (0..3)  [tegra_drive_pingroup_config.drive]

> @@ -77,6 +160,14 @@ struct pinctrl_ops {
...
> +	int (*pin_config) (struct pinctrl_dev *pctldev,
> +			   unsigned pin,
> +			   enum pin_config_param param,
> +			   unsigned long data);
> +	int (*pin_config_group) (struct pinctrl_dev *pctldev,
> +				 unsigned selector,
> +				 enum pin_config_param param,
> +				 unsigned long data);

That looks good.

> @@ -113,6 +204,10 @@ extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
...
> +extern int pin_config(struct pinctrl_dev *pctldev, int pin,
> +		      enum pin_config_param param, unsigned long data);
> +extern int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
> +			    enum pin_config_param param, unsigned long data);

Hmmm. Do we really want to expose these as public APIs? I suppose it
will allow us to start configuring all these parameters ASAP, but all
previous discussion has been aimed at having the pinctrl core set up an
initial set of values at boot-time using a board-supplied table (so no
external API), and then we were still talking about how to manipulate
the values at run-time. Do we really want to encode all this information
into drivers calling these APIs?

-- 
nvpublic

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Shawn Guo]

On Wed, Oct 19, 2011 at 04:04:29PM -0700, Stephen Warren wrote:
> > @@ -113,6 +204,10 @@ extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
> ...
> > +extern int pin_config(struct pinctrl_dev *pctldev, int pin,
> > +		      enum pin_config_param param, unsigned long data);
> > +extern int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
> > +			    enum pin_config_param param, unsigned long data);
> 
> Hmmm. Do we really want to expose these as public APIs? I suppose it
> will allow us to start configuring all these parameters ASAP, but all
> previous discussion has been aimed at having the pinctrl core set up an
> initial set of values at boot-time using a board-supplied table (so no
> external API), and then we were still talking about how to manipulate
> the values at run-time. Do we really want to encode all this information
> into drivers calling these APIs?
> 
+1

We should not require device driver to call these APIs directly.  There
are so many pinctrl subsystem internal details left to its users.

The Stephen's proposal [1] about adding a new param into pinmux_enable()
to specify pin configuration is much preferred to me.

Regards,
Shawn

[1] http://article.gmane.org/gmane.linux.ports.arm.kernel/137341

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Linus Walleij]

On Thu, Oct 20, 2011 at 4:45 AM, Shawn Guo <shawn.guo@freescale.com> wrote:

> We should not require device driver to call these APIs directly.  There
> are so many pinctrl subsystem internal details left to its users.

As explained I already have drivers that need to do this. OK they
are out-of-tree, but they do exist.

For U5500 we have a SIM card driver which sets pull down resistance
and load capacitance in response to information retrieved from the
card at runtime. This is not static or internal to pinctrl at all. How should
I solve this if pin configuration is not exposed?

Yours,
Linus Walleij

RE: [PATCH 2/2] pinctrl: add a generic control interface

[Stephen Warren]

Linus Walleij wrote at Thursday, October 20, 2011 7:44 AM:
> On Thu, Oct 20, 2011 at 4:45 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> 
> > We should not require device driver to call these APIs directly.  There
> > are so many pinctrl subsystem internal details left to its users.
> 
> As explained I already have drivers that need to do this. OK they
> are out-of-tree, but they do exist.
> 
> For U5500 we have a SIM card driver which sets pull down resistance
> and load capacitance in response to information retrieved from the
> card at runtime. This is not static or internal to pinctrl at all. How should
> I solve this if pin configuration is not exposed?

I think drivers should operate at a higher level than "set pin X's param
Y to value Z".

I imagine the set of states that the SIM card driver needs to set up are
defined by the SIM protocol.

So, the driver communicates with the SIM card, find out that it supports
Certain clock frequencies or capabilities etc., and then needs to program
the SIM card and associated SoC-side HW to operate in the selected mode.

The set of legal modes for the SIM card protocol is presumably pretty
small, so the SIM driver might want to request "basic" or "fast" operation
(note: I have zero knowledge of SIM cars, so those mode names are bogus).

Similarly, SD/MMC drivers might know about a mode name for each clock
frequency (or range) that an SD card is allowed to support by spec.

However, I still think it's a good idea for pinctrl drivers to expose
each of their available configuration options with as little veneer or
abstraction as possible.

To bridge these two levels of abstraction, I think the pinctrl core
needs to be provided with information or an API to map between them.

I imagine this could be implemented as a table rather similar to the
existing mux table, or even part of that mux table.

The table would be at least partially board-specific (since a given SD
instance might be mux'd onto a different set of pins between two boards,
and the values for each param might be determined during board
characterization), but for a given HW module (SIM, SD), at least some
of the data might be the same across any board that use that function,
irrespective of which set of pins is in use or independent of board
characterization. So, we might have to think about how the board and
SoC code (and/or DT board/SoC bindings) could interact to reduce
duplication of common settings, but we should probably defer that
discussion.

-- 
nvpublic

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Linus Walleij]

On Thu, Oct 20, 2011 at 4:45 AM, Shawn Guo <shawn.guo@freescale.com> wrote:

>  There
> are so many pinctrl subsystem internal details left to its users.

The idea is indeed to provide facilities to help with setting up
default configuration
for pins like we set up default pinmux maps. It's not been coded yet though,
atleast not by me :-) so just have some patience...

Linus Walleij

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Linus Walleij]

On Thu, Oct 20, 2011 at 1:04 AM, Stephen Warren <swarren@nvidia.com> wrote:

>> +/**
>> + * enum pin_config_param - possible pin configuration parameters
>> + * @PIN_CONFIG_BIAS_UNKNOWN: this bias mode is not known to us
>
> I'm not sure what use that would be; shouldn't we remove that, and add
> new PIN_CONFIG_BIAS_* values as/when they're needed?

Came from some idea that you could also pin_congif_get() to get the setting
of a pin, but that's overdesigned, so killed it now.

>> + * @PIN_CONFIG_BIAS_FLOAT: no specific bias, the pin will float or state
>> + *   is not controlled by software
>
> "float" and "not controlled by SW" are very different things. How is float
> different to high impedance?

True. And the comment for BIAS_HIGH_IMPEDANCE even says so.
Deleted this.

>> + * @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
>
> What value should be used to disable a pull-up; 0?

A semantic question would also be if pull up is implicitly disabled
if you issue PIN_CONFIG_BIAS_PULL_DOWN when you are
in PULL_UP state.

I added PIN_CONFIG_BIAS_DISABLED  to
set_pin_config(pin, PIN_CONFIG_BIAS_DISABLED);

So we can transition to a state of totally disabled pin bias.

(How to handled them is up to the driver...)

> What value should be
> used when requesting pull-up where the SoC doesn't have a configurable
> pull-up resistor; anything non-zero?

I think the argument should be ignored if it's simply on/off.

Then PIN_CONFIG_BIAS_DISABLE to turn it off.

> Tegra has separate configurations for
> pull-up/down strength (0..31) and pull-up/down enable (up/down/none), though
> I suppose we could map 0 to off, 1..32 to on with strength 0..31, although
> that'd prevent a driver from toggling the enable bit on/off without knowing
> what the strength code should be programmed to...

No a DISABLE enum is cleaner I think.

> Tegra's pull-up/down strengths aren't documented in terms of ohms, but
> rather a "drive strength code" on scale 0..31. I'm not sure what that
> truly maps to in hardware.

Hmmmmmmm can you check it with some hardware engineer?
I have a strong feeling that this is or should be very strictly specified
somewhere in an electrical datasheet. How else can users of the
circuit design the electronics around it? Trial-and-error? :-)

>> + * @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
>
> At least some of these options appear mutually exclusive; I wonder if we
> shouldn't have a PIN_CONFIG_BIAS parameter, and have the legal values be
> HIGH/GROUND/NONE. Pull up/down are probably separate. HIGH_IMPEDANCE seems
> more like a PIN_CONFIG_DRIVE value than a PIN_CONFIG_BIAS value.

Yes some of them are. I think it should be up to the driver to handle
how the mutual exclusiveness of its supported configs are done,
returning the occasional -EINVAL.

Later we may consolidate this, say by letting the core keep track of
config states (it currently doesn't) and reject things that are mutually
exclusive. But I think that's overkill right now?

>> + * @PIN_CONFIG_DRIVE_UNKNOWN: we don't know the drive mode of this pin, for
>> + *   example since it is controlled by hardware or the information is not
>> + *   accessible but we need a meaningful enumerator in e.g. initialization
>> + *   code
>
> Again, I'm not sure this is useful; if this is not a configurable parameter,
> surely initialization code would simply skip attempting to set this param?

Deleted.

>> + * @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 on a custom format
>> + *   as argument when setting pins to this mode
>> + * @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 strength
>> + *   is given on a custom format as argument when setting pins to this mode
>> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
>> + *   (open emitter) which is the same as open drain mutatis mutandis but
>> + *   pulled to ground. If the pin can support different drive strengths for
>> + *   the open drain pin, the strength is given on a custom format as
>> + *   argument when setting pins to this mode
>> + * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off
>
> These seem like mutually exclusive values for a PIN_CONFIG_DRIVE param.

They are. Driver needs to handle that, and say disable OPEN_DRAIN
if it is on when enabling OPEN_SOURCE.

>> + * @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
>> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
>> + *   signals on the pin. The argument gives the rise time in nanoseconds
>> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
>> + *   signals on the pin. The argument gives the fall time in nanoseconds
>> + * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
>> + *   will deform waveforms when signals are transmitted on them, by
>> + *   applying a load capacitance, the waveform can be rectified. The
>> + *   argument gives the load capacitance in picofarad (pF)
>> + * @PIN_CONFIG_WAKEUP_ENABLE: 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
>> + * @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
>
> Tegra needs at least some more:
>
> PIN_CONFIG_HIGH_SPEED_MODE (0..1) [tegra_drive_pingroup_config.hsm]

Is it only for output so it's actually PIN_CONFIG_DRIVE_HIGH_SPEED
or only for input so it's PIN_CONFIG_INPUT_HIGHSPEED?

So this is like the inverse of the slew rate or so?

> PIN_CONFIG_LOW_POWER_MODE (0..3)  [tegra_drive_pingroup_config.drive]

OK added PIN_CONFIG_LOW_POWER_MODE, it could actually
map to say ground setting after a state transition in some cases.

>> @@ -113,6 +204,10 @@ extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
> ...
>> +extern int pin_config(struct pinctrl_dev *pctldev, int pin,
>> +                   enum pin_config_param param, unsigned long data);
>> +extern int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
>> +                         enum pin_config_param param, unsigned long data);
>
> Hmmm. Do we really want to expose these as public APIs? I suppose it
> will allow us to start configuring all these parameters ASAP, but all
> previous discussion has been aimed at having the pinctrl core set up an
> initial set of values at boot-time using a board-supplied table (so no
> external API), and then we were still talking about how to manipulate
> the values at run-time. Do we really want to encode all this information
> into drivers calling these APIs?

I already have such drivers: in the ABx500 there is a SIM card interface,
these set pull up/down and load capacitance (also voltage!) depending
on information obtained after actively communicating with the card
and asking about its electrical characteristics.

Same applies to some eMMC I think.

So we probably cannot avoid exposing this...

However, yes, the simple cases should be for default-config
and then state transitions to things like sleeping or different
"C-states".

Yours,
Linus Walleij

RE: [PATCH 2/2] pinctrl: add a generic control interface

[Stephen Warren]

Linus Walleij wrote at Thursday, October 20, 2011 4:25 AM:
> On Thu, Oct 20, 2011 at 1:04 AM, Stephen Warren <swarren@nvidia.com> wrote:
...
> >> + * @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
> >
> > What value should be used to disable a pull-up; 0?
> 
> A semantic question would also be if pull up is implicitly disabled
> if you issue PIN_CONFIG_BIAS_PULL_DOWN when you are
> in PULL_UP state.
> 
> I added PIN_CONFIG_BIAS_DISABLED  to
> set_pin_config(pin, PIN_CONFIG_BIAS_DISABLED);
> 
> So we can transition to a state of totally disabled pin bias.

I'm not too sure I like that; the core's definition of PIN_CONFIG_BIAS_*
is then imposing semantics that the HW might not have.

So, Tegra's pull configuration is up/down/none, as a register field with
3 values.

Another chip could easily have 1 bit to pull-up-enable and a separate
bit for pull-down-enable. It might be silly to set them both, but the HW
could quite easily be designed such that it'd work as one would exect
electrically.

I'm not convinced that the PIN_CONFIG_BIAS_* definitions should be defined
to force one model over the other. With SoC-defined param names, the
pinctrl driver can expose exactly what the HW supports without abstraction.

And how to hide the abstraction from drivers? Some kind of mapping table
or API; see my other email for details.

-- 
nvpublic

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Shawn Guo]

On Wed, Oct 19, 2011 at 06:21:14PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This add per-pin and per-group pin control interfaces for biasing,
> driving and other such electronic properties. The intention is
> clearly to enumerate all things you can do with pins, hoping that
> these are enumerable.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/pinctrl.txt       |   55 ++++++++++++++++++-
>  drivers/pinctrl/core.c          |   69 ++++++++++++++++++++++++
>  include/linux/pinctrl/pinctrl.h |  112 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 232 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> index b04cb7d..328adb7 100644
> --- a/Documentation/pinctrl.txt
> +++ b/Documentation/pinctrl.txt
> @@ -7,8 +7,6 @@ This subsystem deals with:
>  
>  - Multiplexing of pins, pads, fingers (etc) see below for details
>  
> -The intention is to also deal with:
> -
>  - Software-controlled biasing and driving mode specific pins, such as
>    pull-up/down, open drain etc, load capacitance configuration when controlled
>    by software, etc.
> @@ -193,6 +191,59 @@ structure, for example specific register ranges associated with each group
>  and so on.
>  
>  
> +Pin configuration
> +=================
> +
> +Pins can sometimes be software-configured in an tremendous amount of ways,
> +mostly related to their electronic properties when used as inputs or outputs.
> +For example you may be able to make an output pin high impedance, or "tristate"
> +meaning it is effectively disconnected. You may be able to connect an input pin
> +to VDD or GND using a certain resistor value - pull up and pull down - so that
> +the pin has a stable value when nothing is driving the rail it is connected
> +to, or when it's unconnected.
> +
> +The pin control system supports an interface partly abstracting these
> +properties while leaving the details to the pin control driver.
> +
> +For example, a driver can do this:
> +
> +ret = pin_config(128, PIN_CONFIG_BIAS_PULL_UP, 100000);
> +
> +To pull up a pin to VDD with a 100KOhm resistor. The driver implements
> +callbacks for changing pin configuration in the pin controller ops like this:
> +
> +int foo_pin_config (struct pinctrl_dev *pctldev,
> +		    unsigned pin,
> +		    enum pin_config_param param,
> +		    unsigned long data)
> +{
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +	     ...
> +}
> +
> +int foo_pin_config_group (struct pinctrl_dev *pctldev,
> +		    unsigned selector,
> +		    enum pin_config_param param,
> +		    unsigned long data)
> +{
> +	...
> +}
> +
> +static struct pinctrl_ops foo_pctrl_ops = {
> +       ...
> +       pin_config = foo_pin_config,
> +       pin_config_group = foo_pin_config_group,
> +};
> +
> +Since some controllers have special logic for handling entire groups of pins
> +they can exploit the special whole-group pin control function. The
> +pin_config_group() callback is allowed to return the error code -EAGAIN,
> +for groups it does not want to handle, or if it just wants to do some
> +group-level handling in which case each individual pin will be handled by
> +separate pin_config() calls as well.
> +
> +
>  Interaction with the GPIO subsystem
>  ===================================
>  
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 478a002..913f760 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -315,6 +315,75 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
>  	return -EINVAL;
>  }
>  
> +int pin_config(struct pinctrl_dev *pctldev, int pin,
> +	       enum pin_config_param param, unsigned long data)
> +{
> +	const struct pinctrl_ops *ops = pctldev->desc->pctlops;
> +
> +	if (!ops->pin_config) {
> +		dev_err(&pctldev->dev, "cannot configure pin, missing "
> +			"config function in driver\n");
> +		return -EINVAL;
> +	}
> +
> +	return ops->pin_config(pctldev, pin, param, data);
> +}
> +
> +int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
> +		     enum pin_config_param param, unsigned long data)
> +{
> +	const struct pinctrl_ops *ops = pctldev->desc->pctlops;
> +	int selector;
> +	unsigned *pins;
> +	unsigned num_pins;
> +	int ret;
> +	int i;
> +
> +	if (!ops->pin_config_group && !ops->pin_config) {
> +		dev_err(&pctldev->dev, "cannot configure pin group, missing "
> +			"config function in driver\n");
> +		return -EINVAL;
> +	}
> +
> +	selector = pinctrl_get_group_selector(pctldev, pin_group);
> +	if (selector < 0)
> +		return selector;
> +
> +	/*
> +	 * If the pin controller supports handling entire groups we use that
> +	 * capability.
> +	 */
> +	if (ops->pin_config_group) {
> +		ret = ops->pin_config_group(pctldev, selector, param, data);
> +		/*
> +		 * If the pin controller prefer that a certain group be handled
> +		 * pin-by-pin as well, it returns -EAGAIN.
> +		 */
> +		if (ret != -EAGAIN)
> +			return ret;
> +	}
> +
> +	/*
> +	 * If the controller cannot handle entire groups, we configure each pin
> +	 * individually.
> +	 */
> +	ret = ops->get_group_pins(pctldev, selector,
> +				  &pins, &num_pins);
> +	if (ret) {
> +		dev_err(&pctldev->dev, "cannot configure pin group, error "
> +			"getting pins\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < num_pins; i++) {
> +		ret = pin_config(pctldev, pins[i], param, data);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_DEBUG_FS
>  
>  static int pinctrl_pins_show(struct seq_file *s, void *what)
> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
> index 4f8d208..189c047 100644
> --- a/include/linux/pinctrl/pinctrl.h
> +++ b/include/linux/pinctrl/pinctrl.h
> @@ -19,6 +19,87 @@
>  #include <linux/list.h>
>  #include <linux/seq_file.h>
>  
> +/**
> + * enum pin_config_param - possible pin configuration parameters
> + * @PIN_CONFIG_BIAS_UNKNOWN: this bias mode is not known to us
> + * @PIN_CONFIG_BIAS_FLOAT: no specific bias, the pin will float or state
> + *	is not controlled by software
> + * @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_UNKNOWN: we don't know the drive mode of this pin, for
> + *	example since it is controlled by hardware or the information is not
> + *	accessible but we need a meaningful enumerator in e.g. initialization
> + *	code
> + * @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 on a custom format
> + *	as argument when setting pins to this mode
> + * @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 strength
> + *	is given on a custom format as argument when setting pins to this mode
> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
> + *	(open emitter) which is the same as open drain mutatis mutandis but
> + *	pulled to ground. If the pin can support different drive strengths for
> + *	the open drain pin, the strength is given on a custom format as
> + *	argument when setting pins to this mode
> + * @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
> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
> + *	signals on the pin. The argument gives the rise time in nanoseconds
> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
> + *	signals on the pin. The argument gives the fall time in nanoseconds
> + * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
> + *	will deform waveforms when signals are transmitted on them, by
> + *	applying a load capacitance, the waveform can be rectified. The
> + *	argument gives the load capacitance in picofarad (pF)
> + * @PIN_CONFIG_WAKEUP_ENABLE: 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
> + * @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_UNKNOWN,
> +	PIN_CONFIG_BIAS_FLOAT,
> +	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_UNKNOWN,
> +	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_SLEW_RATE_RISING,
> +	PIN_CONFIG_SLEW_RATE_FALLING,
> +	PIN_CONFIG_LOAD_CAPACITANCE,
> +	PIN_CONFIG_WAKEUP_ENABLE,
> +	PIN_CONFIG_END,
> +};
> +

IMO, trying to enumerate all possible pin_config options is just to
make everyone's life harder.  Firstly, this enumeration is far from
completion, for imx6 iomuxc example, we have 3 options for pull-up,
22, 47 and 100 kOhm, and 7 options for driver-strength, 34, 40, 48,
60, 80, 120, and 240 Ohm.  It's just so hard to make this enumeration
complete.  Secondly, defining this param as enum requires the API
user to call the function multiple times to configure one pin.  For
example, if I want to configure pin_foo as slow-slew, open-drain,
100 kOhm pull-up and 120 Ohm driver-strength, I will have to call
pin_config(pctldev, pin_foo, ...) 4 times to achieve that.

I like Stephen's idea about having 'u32 param' and let pinctrl drivers
to encode/decode this u32 for their pinctrl controller.  It makes
people's life much easier.

-- 
Regards,
Shawn

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Barry Song]

2011/10/20 Shawn Guo <shawn.guo@freescale.com>:
>>  #ifdef CONFIG_DEBUG_FS
>>
>>  static int pinctrl_pins_show(struct seq_file *s, void *what)
>> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
>> index 4f8d208..189c047 100644
>> --- a/include/linux/pinctrl/pinctrl.h
>> +++ b/include/linux/pinctrl/pinctrl.h
>> @@ -19,6 +19,87 @@
>>  #include <linux/list.h>
>>  #include <linux/seq_file.h>
>>
>> +/**
>> + * enum pin_config_param - possible pin configuration parameters
>> + * @PIN_CONFIG_BIAS_UNKNOWN: this bias mode is not known to us
>> + * @PIN_CONFIG_BIAS_FLOAT: no specific bias, the pin will float or state
>> + *   is not controlled by software
>> + * @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_UNKNOWN: we don't know the drive mode of this pin, for
>> + *   example since it is controlled by hardware or the information is not
>> + *   accessible but we need a meaningful enumerator in e.g. initialization
>> + *   code
>> + * @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 on a custom format
>> + *   as argument when setting pins to this mode
>> + * @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 strength
>> + *   is given on a custom format as argument when setting pins to this mode
>> + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
>> + *   (open emitter) which is the same as open drain mutatis mutandis but
>> + *   pulled to ground. If the pin can support different drive strengths for
>> + *   the open drain pin, the strength is given on a custom format as
>> + *   argument when setting pins to this mode
>> + * @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
>> + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
>> + *   signals on the pin. The argument gives the rise time in nanoseconds
>> + * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
>> + *   signals on the pin. The argument gives the fall time in nanoseconds
>> + * @PIN_CONFIG_LOAD_CAPACITANCE: some pins have inductive characteristics and
>> + *   will deform waveforms when signals are transmitted on them, by
>> + *   applying a load capacitance, the waveform can be rectified. The
>> + *   argument gives the load capacitance in picofarad (pF)
>> + * @PIN_CONFIG_WAKEUP_ENABLE: 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
>> + * @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_UNKNOWN,
>> +     PIN_CONFIG_BIAS_FLOAT,
>> +     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_UNKNOWN,
>> +     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_SLEW_RATE_RISING,
>> +     PIN_CONFIG_SLEW_RATE_FALLING,
>> +     PIN_CONFIG_LOAD_CAPACITANCE,
>> +     PIN_CONFIG_WAKEUP_ENABLE,
>> +     PIN_CONFIG_END,
>> +};
>> +
>
> IMO, trying to enumerate all possible pin_config options is just to
> make everyone's life harder.  Firstly, this enumeration is far from
> completion, for imx6 iomuxc example, we have 3 options for pull-up,
> 22, 47 and 100 kOhm, and 7 options for driver-strength, 34, 40, 48,
> 60, 80, 120, and 240 Ohm.  It's just so hard to make this enumeration
> complete.  Secondly, defining this param as enum requires the API
> user to call the function multiple times to configure one pin.  For
> example, if I want to configure pin_foo as slow-slew, open-drain,
> 100 kOhm pull-up and 120 Ohm driver-strength, I will have to call
> pin_config(pctldev, pin_foo, ...) 4 times to achieve that.
>
> I like Stephen's idea about having 'u32 param' and let pinctrl drivers
> to encode/decode this u32 for their pinctrl controller.  It makes
> people's life much easier.

A multifunctional API is actually  a bad and hard-to-use API. i agree
we can make param u32 instead of enum and let specific driver
customizes the param by itself.

But if there are some common params, for example, PULL, OC/OD,
WAKEUP_ENABLE, which almost all platforms need,  why don't we make
them have common definitions like:

#define PIN_CONFIG_PULL                             0
#define PIN_CONFIG_OPEN_DRAIN                1
....
#define PIN_CONFIG_USER                            5 (in case)

if one platform has config not in the up list, then:

#define PIN_CONFIG_TERGA_XXX                PIN_CONFIG_USER
#define PIN_CONFIG_TERGA_YYY                (PIN_CONFIG_USER + 1)

without the common definition from  PIN_CONFIG_PULL to
PIN_CONFIG_USER, many platforms will need to definite them repeatedly.
that is what we hate.

>
> --
> Regards,
> Shawn

-barry

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Shawn Guo]

On Thu, Oct 20, 2011 at 05:17:08PM +0800, Barry Song wrote:
> >> +enum pin_config_param {
> >> +     PIN_CONFIG_BIAS_UNKNOWN,
> >> +     PIN_CONFIG_BIAS_FLOAT,
> >> +     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_UNKNOWN,
> >> +     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_SLEW_RATE_RISING,
> >> +     PIN_CONFIG_SLEW_RATE_FALLING,
> >> +     PIN_CONFIG_LOAD_CAPACITANCE,
> >> +     PIN_CONFIG_WAKEUP_ENABLE,
> >> +     PIN_CONFIG_END,
> >> +};
> >> +
> >
> > IMO, trying to enumerate all possible pin_config options is just to
> > make everyone's life harder.  Firstly, this enumeration is far from
> > completion, for imx6 iomuxc example, we have 3 options for pull-up,
> > 22, 47 and 100 kOhm, and 7 options for driver-strength, 34, 40, 48,
> > 60, 80, 120, and 240 Ohm.  It's just so hard to make this enumeration
> > complete.  Secondly, defining this param as enum requires the API
> > user to call the function multiple times to configure one pin.  For
> > example, if I want to configure pin_foo as slow-slew, open-drain,
> > 100 kOhm pull-up and 120 Ohm driver-strength, I will have to call
> > pin_config(pctldev, pin_foo, ...) 4 times to achieve that.
> >
> > I like Stephen's idea about having 'u32 param' and let pinctrl drivers
> > to encode/decode this u32 for their pinctrl controller.  It makes
> > people's life much easier.
> 
> A multifunctional API is actually  a bad and hard-to-use API. i agree
> we can make param u32 instead of enum and let specific driver
> customizes the param by itself.
> 
> But if there are some common params, for example, PULL, OC/OD,
> WAKEUP_ENABLE, which almost all platforms need,  why don't we make
> them have common definitions like:
> 
> #define PIN_CONFIG_PULL                             0
> #define PIN_CONFIG_OPEN_DRAIN                1
> ....
> #define PIN_CONFIG_USER                            5 (in case)
> 
> if one platform has config not in the up list, then:
> 
> #define PIN_CONFIG_TERGA_XXX                PIN_CONFIG_USER
> #define PIN_CONFIG_TERGA_YYY                (PIN_CONFIG_USER + 1)
> 
> without the common definition from  PIN_CONFIG_PULL to
> PIN_CONFIG_USER, many platforms will need to definite them repeatedly.
> that is what we hate.
> 
I prefer to completely leave the encoding of this 'u32 param' to pinctrl
drivers, so that they can map the bit definition of 'param' to the
controller register as much as they can.  On imx, we have one register
accommodating all pin configuration options for one pin, so we can
write 'param' directly into register with exactly same bit definition
as register.

-- 
Regards,
Shawn

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Linus Walleij]

On Thu, Oct 20, 2011 at 3:10 PM, Shawn Guo <shawn.guo@freescale.com> wrote:

>> without the common definition from  PIN_CONFIG_PULL to
>> PIN_CONFIG_USER, many platforms will need to definite them repeatedly.
>> that is what we hate.
>>
> I prefer to completely leave the encoding of this 'u32 param' to pinctrl
> drivers, so that they can map the bit definition of 'param' to the
> controller register as much as they can. On imx, we have one register
> accommodating all pin configuration options for one pin, so we can
> write 'param' directly into register with exactly same bit definition
> as register.

I understand this argument, because it makes it more convenient to
write drivers and you can cut a few corners.

However I would prefer to keep things more abstract, and the reason
is that we strive to have drivers reused across SoCs. For example
drivers/tty/serial/amba-pl011.c is used on a multitude of SoCs,
U300, Nomadik, Ux500, ARM Versatile, ARM Vexpress, LPC32XX
etc etc.

What if this common driver suddenly need to bias a pin? Is it:

#include <linus/pinctrl/pinctrl.h>

ret = pin_config(pctldev, pin, PIN_CONFIG_BIAS_HIGH, 0);

Or (OK silly example but you get the point):

#include <linus/pinctrl/pinctrl.h>

if (machine_is_u300())
    ret = pin_config(pctldev, pin, PIN_CONFIG_U300_BIAS_VDD, 0);
else if (machine_is_nomadik())
    ret = pin_config(pctldev, pin, PIN_CONFIG_NOMADIK_BIAS_UP, 0);
else if (machine_is_ux500())
    ret = pin_config(pctldev, pin, PIN_CONFIG_UX500_PULLHIGH, 0);
else if (machine_is_versatile())
    ret = pin_config(pctldev, pin, PIN_CONFIG_VERSATILE_POW, 0);
else if (machine_is_vexpress())
    ret = pin_config(pctldev, pin, PIN_CONFIG_VEXPRESS_XTRA, 0);
else if (machine_is_lpc32xx())
    ret = pin_config(pctldev, pin, PIN_CONFIG_LPC_SUPPLY, 0);
...

IMO we have enough strange and custom things in the kernel
already and we need to make things more abstract, not the least
so that people can understand what the code is doing.

Yours,
Linus Walleij

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Shawn Guo]

On Thu, Oct 20, 2011 at 04:18:49PM +0200, Linus Walleij wrote:
> On Thu, Oct 20, 2011 at 3:10 PM, Shawn Guo <shawn.guo@freescale.com> wrote:
> 
> >> without the common definition from  PIN_CONFIG_PULL to
> >> PIN_CONFIG_USER, many platforms will need to definite them repeatedly.
> >> that is what we hate.
> >>
> > I prefer to completely leave the encoding of this 'u32 param' to pinctrl
> > drivers, so that they can map the bit definition of 'param' to the
> > controller register as much as they can. On imx, we have one register
> > accommodating all pin configuration options for one pin, so we can
> > write 'param' directly into register with exactly same bit definition
> > as register.
> 
> I understand this argument, because it makes it more convenient to
> write drivers and you can cut a few corners.
> 
> However I would prefer to keep things more abstract, and the reason
> is that we strive to have drivers reused across SoCs. For example
> drivers/tty/serial/amba-pl011.c is used on a multitude of SoCs,
> U300, Nomadik, Ux500, ARM Versatile, ARM Vexpress, LPC32XX
> etc etc.
> 
> What if this common driver suddenly need to bias a pin? Is it:
> 
> #include <linus/pinctrl/pinctrl.h>
> 
> ret = pin_config(pctldev, pin, PIN_CONFIG_BIAS_HIGH, 0);
> 
> Or (OK silly example but you get the point):
> 
> #include <linus/pinctrl/pinctrl.h>
> 
> if (machine_is_u300())
>     ret = pin_config(pctldev, pin, PIN_CONFIG_U300_BIAS_VDD, 0);
> else if (machine_is_nomadik())
>     ret = pin_config(pctldev, pin, PIN_CONFIG_NOMADIK_BIAS_UP, 0);
> else if (machine_is_ux500())
>     ret = pin_config(pctldev, pin, PIN_CONFIG_UX500_PULLHIGH, 0);
> else if (machine_is_versatile())
>     ret = pin_config(pctldev, pin, PIN_CONFIG_VERSATILE_POW, 0);
> else if (machine_is_vexpress())
>     ret = pin_config(pctldev, pin, PIN_CONFIG_VEXPRESS_XTRA, 0);
> else if (machine_is_lpc32xx())
>     ret = pin_config(pctldev, pin, PIN_CONFIG_LPC_SUPPLY, 0);
> ...
> 
> IMO we have enough strange and custom things in the kernel
> already and we need to make things more abstract, not the least
> so that people can understand what the code is doing.
> 
To me, pin_config() and its parameters should be invisible to client
drivers.  For amba-pl011 example, do you think any of those SoCs will
need multiple sets of uart pin configurations to switch for different
working modes?  If that's case, the individual SoC pinctrl driver
should be responsible for mapping particular pin configuration to
specific pl011 working mode.  Otherwise, the amba-pl011 can simply
call pinmux_enable(...) to have both mux and cfg set up.

-- 
Regards,
Shawn

RE: [PATCH 2/2] pinctrl: add a generic control interface

[Stephen Warren]

Shawn Guo wrote at Sunday, October 23, 2011 2:51 AM:
...
> To me, pin_config() and its parameters should be invisible to client
> drivers.  For amba-pl011 example, do you think any of those SoCs will
> need multiple sets of uart pin configurations to switch for different
> working modes?  If that's case, the individual SoC pinctrl driver
> should be responsible for mapping particular pin configuration to
> specific pl011 working mode.  Otherwise, the amba-pl011 can simply
> call pinmux_enable(...) to have both mux and cfg set up.

I agree.

Somebody at the ARM workshop yesterday (and I'm sorry, I forget who; I'm
missing too much sleep) pointed out another very good reason:

If there's some IP block that's used in a bunch of SoCs, and each SoC has
different sets of params (or values) that can be set, the IP block driver
shouldn't know about each SoC, and set those values; instead, the board
should be telling the pinmux system exactly what parameters to set, and
the IP block driver should simply be telling pinmux "set up the pins".

Now, I suppose that this could also be done by providing all the params
and values to the driver through platform data, however:

a) The driver is already calling pinctrl to set up the muxing in an
abstract way (mapping table hides the details); I'd like to see pin
config work in the same way.

b) If this were implemented through platform data in each driver, then
every driver's DT binding and probe code will need to define the format
of this data and parse it at probe time. Instead, if this is all in some
table handled by the pinctrl system, it's defined once, and parse once,
which seems a lot simpler.

-- 
nvpublic

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Linus Walleij]

On Thu, Oct 20, 2011 at 11:17 AM, Barry Song <21cnbao@gmail.com> wrote:
> [Shawn]
>> I like Stephen's idea about having 'u32 param' and let pinctrl drivers
>> to encode/decode this u32 for their pinctrl controller.  It makes
>> people's life much easier.
>
> A multifunctional API is actually  a bad and hard-to-use API. i agree
> we can make param u32 instead of enum and let specific driver
> customizes the param by itself.

I am hesitant about that idea because it echoes something I have
heard before, about how every system is so very special.

Greg had this very famous quote, "yes you're special, just like
everyone else"...

I think (and of course this may be completely wrong, but it's my
working hypthesis) that the things that software wants to do to
pins are:

- Enumerable, not vast

- Actually often very much the same things, just named
  differently

- Often possible to express in terms of SI-units (Ohms, nanoseconds,
  Farad, Volt per sec, ...)

> But if there are some common params, for example, PULL, OC/OD,
> WAKEUP_ENABLE, which almost all platforms need,  why don't we make
> them have common definitions like:
>
> #define PIN_CONFIG_PULL                             0
> #define PIN_CONFIG_OPEN_DRAIN                1
> ....
> #define PIN_CONFIG_USER                            5 (in case)
>
> if one platform has config not in the up list, then:
>
> #define PIN_CONFIG_TERGA_XXX                PIN_CONFIG_USER
> #define PIN_CONFIG_TERGA_YYY                (PIN_CONFIG_USER + 1)
>
> without the common definition from  PIN_CONFIG_PULL to
> PIN_CONFIG_USER, many platforms will need to definite them repeatedly.
> that is what we hate.

In the patch you're quoting:

>>> + * @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

So you begin your custom enum like this:

#include <linus/pinctrl/pinctrl.h>

enum foo_pin_config {
    PIN_CONFIG_FOO_XXX = PIN_CONFIG_END+1,
    PIN_CONFIG_FOO_YYY,
    ....
};

Enums are good because in theory they give some
type safety. (Maybe not in practice. Hm.) But lecture me a bit
about why this is such a bad idea and I will change them into
#define:s but I want a solid case for it first.

Maybe PIN_CONFIG_END is not such a good name for the
last enum since there are more configs in the expanded cases...

Yet again, can I have some examples of what
PIN_CONFIG_USER may *actually* be, which would be
absolutely impossible to express in some neutral way, and
ridiculous to have in the generic enum?

There is a lot of things with strange names in some current
pin controllers/muxes, but strange names doesn't count,
it has to be a strange concept behind it to be strange for real.

At one point when I was creating pinmux I was told this was
pointless because one platform was doing pinmux, another
padmux, a third "mission modes", a fourth "alternat functions".
But it turns out that these are just different names for one and
the same thing, so I have this maybe naïve idea that pin
control/bias/drive/etc may largely be the same.

Example setting a pin "floating", "high impedance", tristate",
"high-Z" or "off" turns out to often mean the exact same thing.

Yours,
Linus Walleij

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Mark Brown]

On Thu, Oct 20, 2011 at 04:04:47PM +0200, Linus Walleij wrote:

> I think (and of course this may be completely wrong, but it's my
> working hypthesis) that the things that software wants to do to
> pins are:

The other question is if it's worth bouncing through too much of an
abstraction layer when both ends of the API are fixed.

> Yet again, can I have some examples of what
> PIN_CONFIG_USER may *actually* be, which would be
> absolutely impossible to express in some neutral way, and
> ridiculous to have in the generic enum?

One fun example is that we have some devices with pins which have
runtime controllable voltage domains, there's no obvious SI unit mapping
for those.

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Linus Walleij]

On Thu, Oct 20, 2011 at 4:18 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Oct 20, 2011 at 04:04:47PM +0200, Linus Walleij wrote:

>> I think (and of course this may be completely wrong, but it's my
>> working hypthesis) that the things that software wants to do to
>> pins are:
>
> The other question is if it's worth bouncing through too much of an
> abstraction layer when both ends of the API are fixed.

If drivers doing pinctrl are used in more than one SoC
both ends aren't fixed. But maybe I'm wrong in assuming that
such things exist?

I bet someone made a claim about regulators in its
infancy, "I just want to communicate to the regulator to set
voltage number 0x14, the framework shouldn't care what
voltage that actually is." The selectors is a good compromise
that unify expressing voltages and currents with fixed
discrete steps actually, that is why I like it so much.

And the same could probably be said about why the clock
framework insist on expressing frequencies in Hz
including all the trouble with using clk_round_rate() if all my
SoC drivers doing clk_get_rate() and clk_set_rate() already know
very well that rate 0x54 is the frequency it wants to set.

So there is some lesson abou abstraction to be learned
here, and the question is, whatever it is that pin control
is passing around, should the core or anyone else really
care, or should it be opaque in difference from regulators
and clocks?

>> Yet again, can I have some examples of what
>> PIN_CONFIG_USER may *actually* be, which would be
>> absolutely impossible to express in some neutral way, and
>> ridiculous to have in the generic enum?
>
> One fun example is that we have some devices with pins which have
> runtime controllable voltage domains, there's no obvious SI unit mapping
> for those.

If you mean they can only be switched on or off yes,
they rather need some ON vs OFF parameter setting without
any unit argument. The unit argument is already optional for a few
parameters.

(If you mean you can control the voltage I guess Volt is a nice
derived SI unit, but I guess you can't since you put the claim
that way.)

In Ux500 we model our power domain switches as regulators,
does such a property even belong in pin control? Is there
one power domain switch per pin you mean, or a power switch
for a whole group of pins?

On a related key we have the part that gpiolib is handling already,
setting a pin to input or output and driving it low or high -
I have no plans to model that through the pinconfig API by
exposing some PIN_CONFIG_SET_OUTPUT,
PIN_CONFIG_SET_INPUT, PIN_CONFIG_DRIVE_HIGH,
PIN_CONFIG_DRIVE_LOW, that would be madness, what we
do is of course expose a struct gpio_chip interface for that
stuff and register to gpiolib.

Thus it might be that pins should express their voltage domain
switches as a regulator rather than trying to do it in pin control?

Yours,
Linus Walleij

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Mark Brown]

On Thu, Oct 20, 2011 at 04:43:30PM +0200, Linus Walleij wrote:
> On Thu, Oct 20, 2011 at 4:18 PM, Mark Brown

> > The other question is if it's worth bouncing through too much of an
> > abstraction layer when both ends of the API are fixed.

> If drivers doing pinctrl are used in more than one SoC
> both ends aren't fixed. But maybe I'm wrong in assuming that
> such things exist?

There's definitely drivers that will be used over multiple SoCs,
although I'm not sure what the overlap is between them and devices that
need to fiddle with their pin settings at runtime (and how much of the
pin settings they can usefully fiddle with).

> I bet someone made a claim about regulators in its
> infancy, "I just want to communicate to the regulator to set
> voltage number 0x14, the framework shouldn't care what

Not really, actually - with regulators there's a blatantly obvious
abstraction layer to set up as they're physically distinct devices.

> voltage that actually is." The selectors is a good compromise
> that unify expressing voltages and currents with fixed
> discrete steps actually, that is why I like it so much.

That's mostly just a reflection of the reality of how one interacts with
devices of course - at the end of the day you have to translate the
settings into a register write so...

> So there is some lesson abou abstraction to be learned
> here, and the question is, whatever it is that pin control
> is passing around, should the core or anyone else really
> care, or should it be opaque in difference from regulators
> and clocks?

Yes, much of this depends on how much generic drivers (as opposed to
SoCs or boards) are going to need to know about their pin configuration
and talk about it to something else.

> > One fun example is that we have some devices with pins which have
> > runtime controllable voltage domains, there's no obvious SI unit mapping
> > for those.

> If you mean they can only be switched on or off yes,
> they rather need some ON vs OFF parameter setting without
> any unit argument. The unit argument is already optional for a few
> parameters.

> (If you mean you can control the voltage I guess Volt is a nice
> derived SI unit, but I guess you can't since you put the claim
> that way.)

Neither of these things.  As with all pins these are referenced to
particular supply voltages but fairly unusually these devices are able
to change the supply the pin is referenced to.  This changes the level
that outputs are driven at and the levels used for identification of
logic levels on input.

> In Ux500 we model our power domain switches as regulators,
> does such a property even belong in pin control? Is there
> one power domain switch per pin you mean, or a power switch
> for a whole group of pins?

This isn't an on/off control and in this cae it's per pin.

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Linus Walleij]

On Thu, Oct 20, 2011 at 5:42 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Oct 20, 2011 at 04:43:30PM +0200, Linus Walleij wrote:
>> In Ux500 we model our power domain switches as regulators,
>> does such a property even belong in pin control? Is there
>> one power domain switch per pin you mean, or a power switch
>> for a whole group of pins?
>
> This isn't an on/off control and in this cae it's per pin.

If this is a binary output pin, I suspect that
I suspect that what you call "off" is actually implemeted as
in this figure:
http://upload.wikimedia.org/wikipedia/commons/c/c0/Tristate_buffer.svg

In which case:
PIN_CONFIG_DRIVE_PUSH_PULL
PIN_CONFIG_DRIVE_OFF
or PIN_CONFIG_BIAS_HIGH_IMPEDANCE

Is a config pair which most often correspond to
exactly what you're doing here, albeit with a different
terminology.

However, if it is an *analog* pin....
Then it's something else.

Yours,
Linus Walleij

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Mark Brown]

On Fri, Oct 21, 2011 at 02:28:25PM +0200, Linus Walleij wrote:
> On Thu, Oct 20, 2011 at 5:42 PM, Mark Brown
> > On Thu, Oct 20, 2011 at 04:43:30PM +0200, Linus Walleij wrote:

> >> In Ux500 we model our power domain switches as regulators,
> >> does such a property even belong in pin control? Is there
> >> one power domain switch per pin you mean, or a power switch
> >> for a whole group of pins?

> > This isn't an on/off control and in this cae it's per pin.

> If this is a binary output pin, I suspect that
> I suspect that what you call "off" is actually implemeted as
> in this figure:

To repeat once more: this is *not* an on/off control, this is a control
selecting the voltage domain a pin is referenced to.

> http://upload.wikimedia.org/wikipedia/commons/c/c0/Tristate_buffer.svg

No, this is completely orthogonal to voltage domain selection.

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Linus Walleij]

On Fri, Oct 21, 2011 at 2:30 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Oct 21, 2011 at 02:28:25PM +0200, Linus Walleij wrote:
>> If this is a binary output pin, I suspect that
>> I suspect that what you call "off" is actually implemeted as
>> in this figure:
>
> To repeat once more: this is *not* an on/off control, this is a control
> selecting the voltage domain a pin is referenced to.

Sorry, slow learner. So I'll add:
PIN_CONFIG_ENABLE_POWER
PIN_CONFIG_DISABLE_POWER

With the semantics of enabling/disabling the power
supply for the pin logic. Correct now?

Yours,
Linus Walleij

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Mark Brown]

On Fri, Oct 21, 2011 at 02:51:46PM +0200, Linus Walleij wrote:
> On Fri, Oct 21, 2011 at 2:30 PM, Mark Brown

> > To repeat once more: this is *not* an on/off control, this is a control
> > selecting the voltage domain a pin is referenced to.

> Sorry, slow learner. So I'll add:
> PIN_CONFIG_ENABLE_POWER
> PIN_CONFIG_DISABLE_POWER

> With the semantics of enabling/disabling the power
> supply for the pin logic. Correct now?

No, like I keep saying it's nothing to do with enabling or disabling
power.  It's about selecting between multiple reference domains.  For
example, choosing between the DBVDD1 and DBVDD2 domains.

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Linus Walleij]

On Fri, Oct 21, 2011 at 2:55 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Oct 21, 2011 at 02:51:46PM +0200, Linus Walleij wrote:
>> On Fri, Oct 21, 2011 at 2:30 PM, Mark Brown
>
>> > To repeat once more: this is *not* an on/off control, this is a control
>> > selecting the voltage domain a pin is referenced to.
>
>> Sorry, slow learner. So I'll add:
>> PIN_CONFIG_ENABLE_POWER
>> PIN_CONFIG_DISABLE_POWER
>
>> With the semantics of enabling/disabling the power
>> supply for the pin logic. Correct now?
>
> No, like I keep saying it's nothing to do with enabling or disabling
> power.  It's about selecting between multiple reference domains.  For
> example, choosing between the DBVDD1 and DBVDD2 domains.

OK I think I get it now.

So PIN_CONFIG_POWER_SOURCE and then the argument to
that is definately custom...

Yours,
Linus Walleij

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Mark Brown]

On Fri, Oct 21, 2011 at 02:57:19PM +0200, Linus Walleij wrote:
> On Fri, Oct 21, 2011 at 2:55 PM, Mark Brown

> > No, like I keep saying it's nothing to do with enabling or disabling
> > power.  It's about selecting between multiple reference domains.  For
> > example, choosing between the DBVDD1 and DBVDD2 domains.

> OK I think I get it now.

> So PIN_CONFIG_POWER_SOURCE and then the argument to
> that is definately custom...

I'd probably go with _DOMAIN rather than _SOURCE but yes.

RE: [PATCH 2/2] pinctrl: add a generic control interface

[Stephen Warren]

Shawn Guo wrote at Wednesday, October 19, 2011 8:32 PM:
> On Wed, Oct 19, 2011 at 06:21:14PM +0200, Linus Walleij wrote:
...
> > +int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
> > +		     enum pin_config_param param, unsigned long data)
...
> > +enum pin_config_param {
> > +	PIN_CONFIG_BIAS_UNKNOWN,
> > +	PIN_CONFIG_BIAS_FLOAT,
> > +	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_UNKNOWN,
> > +	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_SLEW_RATE_RISING,
> > +	PIN_CONFIG_SLEW_RATE_FALLING,
> > +	PIN_CONFIG_LOAD_CAPACITANCE,
> > +	PIN_CONFIG_WAKEUP_ENABLE,
> > +	PIN_CONFIG_END,
> > +};
> 
> IMO, trying to enumerate all possible pin_config options is just to
> make everyone's life harder.  Firstly, this enumeration is far from
> completion, for imx6 iomuxc example, we have 3 options for pull-up,
> 22, 47 and 100 kOhm, and 7 options for driver-strength, 34, 40, 48,
> 60, 80, 120, and 240 Ohm.  It's just so hard to make this enumeration
> complete.  Secondly, defining this param as enum requires the API
> user to call the function multiple times to configure one pin.  For
> example, if I want to configure pin_foo as slow-slew, open-drain,
> 100 kOhm pull-up and 120 Ohm driver-strength, I will have to call
> pin_config(pctldev, pin_foo, ...) 4 times to achieve that.
> 
> I like Stephen's idea about having 'u32 param' and let pinctrl drivers
> to encode/decode this u32 for their pinctrl controller.  It makes
> people's life much easier.

That's not quite what I meant.

I meant that I thought the types for param and value should be simple
integers, with meanings of the values defined by the individual drivers,
rather than a system-defined enum.

However, I wasn't envisaging packing multiple fields into the "value"
parameter; that would essentially be packing a struct into a u32 for
transport. I still figured that "param" would logically be an enum,
and represent a single modifiable parameter, and "data"/"value" would
be the single value of that one parameter.

Still, perhaps packing stuff is an option that makes sense in some cases,
depending on what API we end up with to manipulate the parameters, and
where the source of "data"/"value" is (encoded into client driver, or
from some hidden table passed to pinmux core by board, with the values
being passed directly to the pinmux drivers without the client drivers
seeing them)

-- 
nvpublic

Re: [PATCH 2/2] pinctrl: add a generic control interface

[Shawn Guo]

On Thu, Oct 20, 2011 at 10:26:43AM -0700, Stephen Warren wrote:
> Shawn Guo wrote at Wednesday, October 19, 2011 8:32 PM:
> > On Wed, Oct 19, 2011 at 06:21:14PM +0200, Linus Walleij wrote:
> ...
> > > +int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
> > > +		     enum pin_config_param param, unsigned long data)
> ...
> > > +enum pin_config_param {
> > > +	PIN_CONFIG_BIAS_UNKNOWN,
> > > +	PIN_CONFIG_BIAS_FLOAT,
> > > +	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_UNKNOWN,
> > > +	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_SLEW_RATE_RISING,
> > > +	PIN_CONFIG_SLEW_RATE_FALLING,
> > > +	PIN_CONFIG_LOAD_CAPACITANCE,
> > > +	PIN_CONFIG_WAKEUP_ENABLE,
> > > +	PIN_CONFIG_END,
> > > +};
> > 
> > IMO, trying to enumerate all possible pin_config options is just to
> > make everyone's life harder.  Firstly, this enumeration is far from
> > completion, for imx6 iomuxc example, we have 3 options for pull-up,
> > 22, 47 and 100 kOhm, and 7 options for driver-strength, 34, 40, 48,
> > 60, 80, 120, and 240 Ohm.  It's just so hard to make this enumeration
> > complete.  Secondly, defining this param as enum requires the API
> > user to call the function multiple times to configure one pin.  For
> > example, if I want to configure pin_foo as slow-slew, open-drain,
> > 100 kOhm pull-up and 120 Ohm driver-strength, I will have to call
> > pin_config(pctldev, pin_foo, ...) 4 times to achieve that.
> > 
> > I like Stephen's idea about having 'u32 param' and let pinctrl drivers
> > to encode/decode this u32 for their pinctrl controller.  It makes
> > people's life much easier.
> 
> That's not quite what I meant.
> 
> I meant that I thought the types for param and value should be simple
> integers, with meanings of the values defined by the individual drivers,
> rather than a system-defined enum.
> 
> However, I wasn't envisaging packing multiple fields into the "value"
> parameter; that would essentially be packing a struct into a u32 for
> transport. I still figured that "param" would logically be an enum,
> and represent a single modifiable parameter, and "data"/"value" would
> be the single value of that one parameter.
> 
Oops, I misread your idea.  Reading it correctly, I do not like it
either :)  It does not resolve my concern that we need to call the API
multiple times to configure one pin.

> Still, perhaps packing stuff is an option that makes sense in some cases,

I feel strongly that this is what we want.

> depending on what API we end up with to manipulate the parameters, and
> where the source of "data"/"value" is (encoded into client driver, or
> from some hidden table passed to pinmux core by board, with the values
> being passed directly to the pinmux drivers without the client drivers
> seeing them)

I do not think client drivers care about the parameters.  For the mmc
example I put earlier, all it needs from pinctrl subsystem is "Hey,
I'm going to switch bus clock to a higher frequency 100 MHz, please
configure mmc pins properly for that."

-- 
Regards,
Shawn

RE: [PATCH 2/2] pinctrl: add a generic control interface

[Stephen Warren]

Shawn Guo wrote at Sunday, October 23, 2011 2:26 AM:
> On Thu, Oct 20, 2011 at 10:26:43AM -0700, Stephen Warren wrote:
> > Shawn Guo wrote at Wednesday, October 19, 2011 8:32 PM:
> > > On Wed, Oct 19, 2011 at 06:21:14PM +0200, Linus Walleij wrote:
> > ...
> > > > +int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
> > > > +		     enum pin_config_param param, unsigned long data)
> > ...
> > > > +enum pin_config_param {
> > > > +	PIN_CONFIG_BIAS_UNKNOWN,
> > > > +	PIN_CONFIG_BIAS_FLOAT,
> > > > +	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_UNKNOWN,
> > > > +	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_SLEW_RATE_RISING,
> > > > +	PIN_CONFIG_SLEW_RATE_FALLING,
> > > > +	PIN_CONFIG_LOAD_CAPACITANCE,
> > > > +	PIN_CONFIG_WAKEUP_ENABLE,
> > > > +	PIN_CONFIG_END,
> > > > +};
> > >
> > > IMO, trying to enumerate all possible pin_config options is just to
> > > make everyone's life harder.  Firstly, this enumeration is far from
> > > completion, for imx6 iomuxc example, we have 3 options for pull-up,
> > > 22, 47 and 100 kOhm, and 7 options for driver-strength, 34, 40, 48,
> > > 60, 80, 120, and 240 Ohm.  It's just so hard to make this enumeration
> > > complete.  Secondly, defining this param as enum requires the API
> > > user to call the function multiple times to configure one pin.  For
> > > example, if I want to configure pin_foo as slow-slew, open-drain,
> > > 100 kOhm pull-up and 120 Ohm driver-strength, I will have to call
> > > pin_config(pctldev, pin_foo, ...) 4 times to achieve that.
> > >
> > > I like Stephen's idea about having 'u32 param' and let pinctrl drivers
> > > to encode/decode this u32 for their pinctrl controller.  It makes
> > > people's life much easier.
> >
> > That's not quite what I meant.
> >
> > I meant that I thought the types for param and value should be simple
> > integers, with meanings of the values defined by the individual drivers,
> > rather than a system-defined enum.
> >
> > However, I wasn't envisaging packing multiple fields into the "value"
> > parameter; that would essentially be packing a struct into a u32 for
> > transport. I still figured that "param" would logically be an enum,
> > and represent a single modifiable parameter, and "data"/"value" would
> > be the single value of that one parameter.
> >
> Oops, I misread your idea.  Reading it correctly, I do not like it
> either :)  It does not resolve my concern that we need to call the API
> multiple times to configure one pin.
> 
> > Still, perhaps packing stuff is an option that makes sense in some cases,
> 
> I feel strongly that this is what we want.

Perhaps the solution is for the pinctrl core -> pinctrl driver API to take
arrays of params and values. A simple driver can simply iterate over the
arrays 1 element at a time, and a more complex driver can read the relevant
regs, apply all the changes, then write all the regs back. This would allow
atomically updating multiple values at once, without much function call
overhead, yet still keep the data for each param a simple unpacked value.

-- 
nvpublic