[PATCH v2] pinctrl: add support for group and pinmux states

[PATCH v2] pinctrl: add support for group and pinmux states

[Linus Walleij]

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

This makes it possible for pin groups and pin muxes to go into
different states, which is especially useful for power management,
both runtime and across suspend/resume.

ChangeLog v1->v2:
- Use a pin controller name instead of the device itself as
  parameter to the state set function, as indicated by Stephens
  similar patch to config this is more helpful.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/pinctrl.txt       |   71 +++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/core.c          |   28 +++++++++++++++
 drivers/pinctrl/pinmux.c        |   34 ++++++++++++++++++
 include/linux/pinctrl/pinctrl.h |   32 +++++++++++++++++
 include/linux/pinctrl/pinmux.h  |    7 ++++
 5 files changed, 172 insertions(+), 0 deletions(-)

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 98aeda6..e76306d 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -194,6 +194,53 @@ just a simple example - in practice you may need more entries in your group
 structure, for example specific register ranges associated with each group
 and so on.
 
+Pin group states
+================
+
+To simplify handling of specific group states, such as when a group of
+pins need to be deactivated or put in a specific sleep state, the pin controller
+may implement a .pin_group_set_state() callback. This will for example be
+called automatically to set a certain pin group in active state when the groups
+are selected for use in a certain pin multiplexing configuration (see below).
+This is a good opportunity to set up any default pin configuration (see below)
+for the pins in a group.
+
+While the driver will have to maintain states for it's pin groups internally
+or in the hardware registers, as well as for setting up the pins on boot,
+it will know through these calls if some special action is needed on behalf
+of the users.
+
+For example, a board file may issue:
+
+pinctrl_set_group_state("foo-dev", "i2c7-group", PIN_GROUP_STATE_DISABLED);
+
+In this case it could be that I2C7 is not used at all in this board, and
+whereas the driver would normally set the I2C pins to pull-up (this happens to
+be required for I2C), in this case since they are not even used on this board,
+it may just as well ground the pins instead and save some power.
+
+The driver will know what to do through this specific callback:
+
+static int foo_set_group_state(struct pinctrl_dev *pctldev,
+				unsigned selector,
+				enum pin_group_state state)
+{
+	switch(state) {
+	...
+	}
+}
+
+static struct pinctrl_ops foo_pctrl_ops = {
+	...
+	.set_group_state = foo_set_group_state,
+};
+
+
+static struct pinctrl_desc foo_desc = {
+	...
+	.pctlops = &foo_pctrl_ops,
+};
+
 
 Pin configuration
 =================
@@ -1066,3 +1113,27 @@ foo_switch()
 }
 
 The above has to be done from process context.
+
+
+Pinmux states
+=============
+
+Pinmuxes can have states just like groups, and in fact setting the state of
+a certain pinmux to a certain enumerator will in practice loop over the pin
+groups used by the pinmux and set the state for each of these groups. Any calls
+to set the pinmux state will be silently ignored if the pin controller does not
+implement handling of group states.
+
+For example a driver, or centralized power management code for the platform
+(wherever the struct pinmux *pmx pointer is handled) can do power saving calls
+like this:
+
+pinmux_set_state(pmx, PIN_GROUP_STATE_ACTIVE);
+pinmux_set_state(pmx, PIN_GROUP_STATE_IDLE);
+pinmux_set_state(pmx, PIN_GROUP_STATE_SLEEP);
+
+Each call may or may not result in the corresponding pin control driver setting
+pins in special low-power modes, disabling pull-ups or anything else (likely
+pin configuration related). It may also just ignore the call, which is useful
+when the same driver is used with different pin controllers, some of which can
+do something meaningful with this information, some which can't.
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 9e32ea3..4a65d83 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -344,6 +344,34 @@ int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
 	return -EINVAL;
 }
 
+/**
+ * pinctrl_set_group_state() - set pin group state
+ * @dev_name: name of the pin controller holding the group to change state for
+ * @name: the name of the pin group to change the state of
+ * @state: the new state for the pin group
+ */
+int pinctrl_set_group_state(const char *dev_name,
+			    const char *name,
+			    enum pin_group_state state)
+{
+	struct pinctrl_dev *pctldev;
+	const struct pinctrl_ops *ops;
+	int selector;
+
+	pctldev = get_pinctrl_dev_from_dev(NULL, dev_name);
+	if (!pctldev)
+		return -EINVAL;
+	ops = pctldev->desc->pctlops;
+
+	selector = pinctrl_get_group_selector(pctldev, name);
+	if (selector < 0)
+		return selector;
+	if (!ops->set_group_state)
+		return 0;
+	return ops->set_group_state(pctldev, selector, state);
+}
+EXPORT_SYMBOL_GPL(pinctrl_set_group_state);
+
 #ifdef CONFIG_DEBUG_FS
 
 static int pinctrl_pins_show(struct seq_file *s, void *what)
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 3bcc641..0a0b05f 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -817,6 +817,9 @@ struct pinmux *pinmux_get(struct device *dev, const char *name)
 	list_add(&pmx->node, &pinmux_list);
 	mutex_unlock(&pinmux_list_mutex);
 
+	/* Set to default state, if any */
+	pinmux_set_state(pmx, PIN_GROUP_STATE_ACTIVE);
+
 	return pmx;
 }
 EXPORT_SYMBOL_GPL(pinmux_get);
@@ -904,6 +907,37 @@ void pinmux_disable(struct pinmux *pmx)
 }
 EXPORT_SYMBOL_GPL(pinmux_disable);
 
+int pinmux_set_state(struct pinmux *pmx, enum pin_group_state state)
+{
+	struct pinctrl_dev *pctldev = pmx->pctldev;
+	const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
+	struct pinmux_group *grp;
+	int ret;
+
+	/* Does not implement group states, bail out */
+	if (!pctlops->set_group_state)
+		return 0;
+
+	/*
+	 * Loop over the currently used groups for this pinmux and set the
+	 * state for each and every one of them.
+	 */
+	list_for_each_entry(grp, &pmx->groups, node) {
+		ret = pctlops->set_group_state(pctldev, grp->group_selector,
+					       state);
+		if (ret) {
+			dev_err(pctldev->dev, "unable to set state %d for "
+				"group %u on %s\n",
+				state, grp->group_selector,
+				pinctrl_dev_get_name(pctldev));
+			return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinmux_set_state);
+
 int pinmux_check_ops(const struct pinmux_ops *ops)
 {
 	/* Check that we implement required operations */
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 9809a94..ac22827 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -12,6 +12,23 @@
 #ifndef __LINUX_PINCTRL_PINCTRL_H
 #define __LINUX_PINCTRL_PINCTRL_H
 
+/**
+ * enum pin_group_state - different pin group states
+ * @PIN_GROUP_STATE_DISABLED: disabled state
+ * @PIN_GROUP_STATE_ACTIVE: active state, pins are in active use for their
+ *	purpose, this should be the default state when pins are muxed in for
+ *	example
+ * @PIN_GROUP_STATE_IDLE: the pin group is not actively used now
+ * @PIN_GROUP_STATE_SLEEP: the pin group is in low-power mode for a longer
+ *	period of time
+ */
+enum pin_group_state {
+	PIN_GROUP_STATE_DISABLED,
+	PIN_GROUP_STATE_ACTIVE,
+	PIN_GROUP_STATE_IDLE,
+	PIN_GROUP_STATE_SLEEP,
+};
+
 #ifdef CONFIG_PINCTRL
 
 #include <linux/radix-tree.h>
@@ -69,6 +86,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
+ * @set_group_state: optional callback to set the state of a certain pin
+ *	group. Not defining this will make all calls to set state succeed.
  * @pin_dbg_show: optional debugfs display hook that will provide per-device
  *	info for a certain pin in debugfs
  */
@@ -80,6 +99,9 @@ struct pinctrl_ops {
 			       unsigned selector,
 			       const unsigned **pins,
 			       unsigned *num_pins);
+	int (*set_group_state) (struct pinctrl_dev *pctldev,
+				unsigned selector,
+				enum pin_group_state state);
 	void (*pin_dbg_show) (struct pinctrl_dev *pctldev, struct seq_file *s,
 			  unsigned offset);
 };
@@ -125,6 +147,9 @@ extern void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 				struct pinctrl_gpio_range *range);
 extern const char *pinctrl_dev_get_name(struct pinctrl_dev *pctldev);
 extern void *pinctrl_dev_get_drvdata(struct pinctrl_dev *pctldev);
+extern int pinctrl_set_group_state(const char *dev_name,
+				   const char *name,
+				   enum pin_group_state state);
 #else
 
 struct pinctrl_dev;
@@ -135,6 +160,13 @@ static inline bool pin_is_valid(struct pinctrl_dev *pctldev, int pin)
 	return pin >= 0;
 }
 
+static inline int pinctrl_set_group_state(const char *dev_name,
+					  const char *name,
+					  enum pin_group_state state)
+{
+	return 0;
+}
+
 #endif /* !CONFIG_PINCTRL */
 
 #endif /* __LINUX_PINCTRL_PINCTRL_H */
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
index 937b3e2..8d084ad 100644
--- a/include/linux/pinctrl/pinmux.h
+++ b/include/linux/pinctrl/pinmux.h
@@ -97,6 +97,7 @@ extern struct pinmux * __must_check pinmux_get(struct device *dev, const char *n
 extern void pinmux_put(struct pinmux *pmx);
 extern int pinmux_enable(struct pinmux *pmx);
 extern void pinmux_disable(struct pinmux *pmx);
+extern int pinmux_set_state(struct pinmux *pmx, enum pin_group_state state);
 
 #else /* !CONFIG_PINMUX */
 
@@ -137,6 +138,12 @@ static inline void pinmux_disable(struct pinmux *pmx)
 {
 }
 
+static inline int pinmux_set_state(struct pinmux *pmx,
+				   enum pin_group_state state)
+{
+	return 0;
+}
+
 #endif /* CONFIG_PINMUX */
 
 #endif /* __LINUX_PINCTRL_PINMUX_H */
-- 
1.7.8

RE: [PATCH v2] pinctrl: add support for group and pinmux states

[Stephen Warren]

Linus Walleij wrote at Sunday, December 18, 2011 4:07 PM:
> This makes it possible for pin groups and pin muxes to go into
> different states, which is especially useful for power management,
> both runtime and across suspend/resume.

> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt

> +Pin group states
> +================
> +
> +To simplify handling of specific group states, such as when a group of
> +pins need to be deactivated or put in a specific sleep state, the pin controller
> +may implement a .pin_group_set_state() callback. This will for example be
> +called automatically to set a certain pin group in active state when the groups
> +are selected for use in a certain pin multiplexing configuration (see below).
> +This is a good opportunity to set up any default pin configuration (see below)
> +for the pins in a group.

I don't think it's possible for a pinctrl driver to implement such a
callback; the appropriate state for a pin may well be board-specific,
and the pinctrl driver itself can't possibly know what that state is.

For example, for some random pin that a board happens to be using as a
GPIO - how does the pinctrl driver know whether that GPIO actually does
something while in suspend; if the GPIO isn't used, the SoC can probably
just turn off all drivers and pull-up/downs on the pin, but if the GPIO
is used to control some aspect of a PMIC, it probably needs to be in a
specific state during suspend. This kind of thing is entirely board-
specific.

Similarly, the sequencing of reprogramming of the various pins when
entering/leaving suspend is most likely board specific, e.g. there may
be a need to assert a "speaker mute" GPIO before de-asserting a GPIO
that enables power to speaker amplifiers etc.

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

> +enum pin_group_state {
> +	PIN_GROUP_STATE_DISABLED,
> +	PIN_GROUP_STATE_ACTIVE,
> +	PIN_GROUP_STATE_IDLE,
> +	PIN_GROUP_STATE_SLEEP,
> +};

I'm unsure that having the pinctrl core explicitly define the set of
legal states is the right approach. This prevents domain-specific states
(e.g. PCIe power states say) from being defined without revising the
core. That said, I'm less adamant about this than about my first point
above.

I'd like to suggest an alternative:

a) The mapping table should be enhanced to cover all configuration that
   pinctrl can program:
   * Mux function selection
   * Whether a pin a used as a GPIO, if gpiolib doesn't interact with
     pinctrl to set this up.
   * Any pin config parameters that should be set.
   * Define all the above for each state that a driver can be in.
   * Name legal states as strings in the mapping table, so they're just
     data and any given driver can define the names of states it expects
     to have available.
   * The table can have a defined execution ordering this allowing board-
     specific sequencing requirements to be defined. Perhaps the mapping
     table can define the set of parameters to program for each legal
     to/from state transition combination, rather than target
     configuration for each state, so all sequencing is fully under
     control of the mapping table.
b) Add a pinctrl API that transitions a given pmx (from pinmux_get())
   between states as defined in the mapping table.

That should allow this to be fully generic.

For standard states (e.g. fully active, system suspend) we could define
(de-facto?) standardized names that drivers can use if appropriate.
That'd prevent too many custom state strings, yet allow extensibility
when needed.

IIRC, some of my previous emails have touched on this idea, and included
example mapping tables with multiple states per device.

-- 
nvpublic

Re: [PATCH v2] pinctrl: add support for group and pinmux states

[Haojian Zhuang]

On Tue, Dec 20, 2011 at 8:24 AM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Sunday, December 18, 2011 4:07 PM:
>> This makes it possible for pin groups and pin muxes to go into
>> different states, which is especially useful for power management,
>> both runtime and across suspend/resume.
>
>> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
>
>> +Pin group states
>> +================
>> +
>> +To simplify handling of specific group states, such as when a group of
>> +pins need to be deactivated or put in a specific sleep state, the pin controller
>> +may implement a .pin_group_set_state() callback. This will for example be
>> +called automatically to set a certain pin group in active state when the groups
>> +are selected for use in a certain pin multiplexing configuration (see below).
>> +This is a good opportunity to set up any default pin configuration (see below)
>> +for the pins in a group.
>
> I don't think it's possible for a pinctrl driver to implement such a
> callback; the appropriate state for a pin may well be board-specific,
> and the pinctrl driver itself can't possibly know what that state is.
>
> For example, for some random pin that a board happens to be using as a
> GPIO - how does the pinctrl driver know whether that GPIO actually does
> something while in suspend; if the GPIO isn't used, the SoC can probably
> just turn off all drivers and pull-up/downs on the pin, but if the GPIO
> is used to control some aspect of a PMIC, it probably needs to be in a
> specific state during suspend. This kind of thing is entirely board-
> specific.
>
> Similarly, the sequencing of reprogramming of the various pins when
> entering/leaving suspend is most likely board specific, e.g. there may
> be a need to assert a "speaker mute" GPIO before de-asserting a GPIO
> that enables power to speaker amplifiers etc.
>

Do we really need to configure low power mode in device driver? In most time,
user will set and tune the low power mode of pins. If the low power setting
could be configured via sysfs, it could save a lot of time to rebuild kernel.

Especially GPIO pins are board specific. I think that configure all low power
setting in board specific code is more reasonable.

Thanks
Haojian

Re: [PATCH v2] pinctrl: add support for group and pinmux states

[Linus Walleij]

Hi Stephen,

many thanks for the comments as usual!

On Tue, Dec 20, 2011 at 1:24 AM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Sunday, December 18, 2011 4:07 PM:
>>
>> +To simplify handling of specific group states, such as when a group of
>> +pins need to be deactivated or put in a specific sleep state, the pin controller
>> +may implement a .pin_group_set_state() callback. This will for example be
>> +called automatically to set a certain pin group in active state when the groups
>> +are selected for use in a certain pin multiplexing configuration (see below).
>> +This is a good opportunity to set up any default pin configuration (see below)
>> +for the pins in a group.
>
> I don't think it's possible for a pinctrl driver to implement such a
> callback; the appropriate state for a pin may well be board-specific,
> and the pinctrl driver itself can't possibly know what that state is.

I don't see the conflict. The driver knows this from its supplied
platform data or device tree nodes. It is indeed board-specific,
as is many things a pinctrl driver may be using.

> For example, for some random pin that a board happens to be using as a
> GPIO - how does the pinctrl driver know whether that GPIO actually does
> something while in suspend; if the GPIO isn't used, the SoC can probably
> just turn off all drivers and pull-up/downs on the pin, but if the GPIO
> is used to control some aspect of a PMIC, it probably needs to be in a
> specific state during suspend. This kind of thing is entirely board-
> specific.

This is exactly the stuff it is supposed to handle. And the driver
does so by inspecting its platform data/DT.

> Similarly, the sequencing of reprogramming of the various pins when
> entering/leaving suspend is most likely board specific, e.g. there may
> be a need to assert a "speaker mute" GPIO before de-asserting a GPIO
> that enables power to speaker amplifiers etc.

I don't see how that affects pinctrl, you can assert your GPIO
from somewhere in arch/arm/mach-foo/* and following that
you can issue pinmux_set_state() for some other pins.

>> +enum pin_group_state {
>> +     PIN_GROUP_STATE_DISABLED,
>> +     PIN_GROUP_STATE_ACTIVE,
>> +     PIN_GROUP_STATE_IDLE,
>> +     PIN_GROUP_STATE_SLEEP,
>> +};
>
> I'm unsure that having the pinctrl core explicitly define the set of
> legal states is the right approach.

But later you say:

> For standard states (e.g. fully active, system suspend) we could define
> (de-facto?) standardized names that drivers can use if appropriate.
>
> That'd prevent too many custom state strings, yet allow extensibility
> when needed.

So I don't get this to match up... There is a clear advantage in
defining a few standard states.

> This prevents domain-specific states
> (e.g. PCIe power states say) from being defined without revising the
> core.

I don't know much about PCIe, but I've heard about C-states and
such things so I buy this argument.

> I'd like to suggest an alternative:
>
> a) The mapping table should be enhanced to cover all configuration that
>   pinctrl can program:
>   * Mux function selection
>   * Whether a pin a used as a GPIO, if gpiolib doesn't interact with
>     pinctrl to set this up.
>   * Any pin config parameters that should be set.
>   * Define all the above for each state that a driver can be in.
>   * Name legal states as strings in the mapping table, so they're just
>     data and any given driver can define the names of states it expects
>     to have available.
>   * The table can have a defined execution ordering this allowing board-
>     specific sequencing requirements to be defined. Perhaps the mapping
>     table can define the set of parameters to program for each legal
>     to/from state transition combination, rather than target
>     configuration for each state, so all sequencing is fully under
>     control of the mapping table.

As mentioned before I prefer that pin configuration tables be
kept separate from say pinmux tables.

And the last bullet is again a script engine...

While these may be good ideas and very useful I fear that
coding it all up into one big table of states and settings
is going to look complex and become hard to maintain.

I would prefer a divide-and-conquer approach here, so
that tables for mux settings and configs are two things
whereas a optional transition script engine is a third thing
utilizing the first two.

Neither has any implications for group states anyway -
how you realize the (optional) group states, i.e. whether
say a certain scriptlet is engaged, a certain table entry
is activated or not, etc, should be up to the driver IMO.
I.e. if implemented by a script engine you *can* do it
that way but you don't *have to*. In our case and for many
simple systems with a few states it would just add
overhead.

> b) Add a pinctrl API that transitions a given pmx (from pinmux_get())
>   between states as defined in the mapping table.

>From the external API point of view, the only difference from the
outlined approach in this patch would be that instead of defining:

enum pin_group_state {
    PIN_GROUP_STATE_DISABLED,
    PIN_GROUP_STATE_ACTIVE,
    (...)
}

states would instead come from the platform data or so.

So if I just replace this enum with a string or so:
pinmux_set_state(pmx, PIN_GROUP_STATE_ACTIVE);
turns into:
pinmux_set_state(pmx, "my-active-state-name");

> That should allow this to be fully generic.
>
> For standard states (e.g. fully active, system suspend) we could define
> (de-facto?) standardized names that drivers can use if appropriate.
>
> That'd prevent too many custom state strings, yet allow extensibility
> when needed.

That means if I refine the API from:

enum pin_group_state {
    PIN_GROUP_STATE_DISABLED,
    PIN_GROUP_STATE_ACTIVE,
    PIN_GROUP_STATE_IDLE,
    PIN_GROUP_STATE_SLEEP,
};

To:

#define PIN_GROUP_STATE_DISABLED "disable"
#define PIN_GROUP_STATE_ACTICE "active"
#define PIN_GROUP_STATE_IDLE "idle"
#define PIN_GROUP_STATE_SLEEP "sleep"

I have achieved the same thing but with string names for
states, opening up for custom named states.

I can live with that I think, I'll hack something.

> IIRC, some of my previous emails have touched on this idea, and included
> example mapping tables with multiple states per device.

Yes, but I do not (yet) embrace this idea :-)

Yours,
Linus Walleij

Re: [PATCH v2] pinctrl: add support for group and pinmux states

[Linus Walleij]

On Tue, Dec 20, 2011 at 3:22 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:

> Do we really need to configure low power mode in device driver?

Who said anything about the device driver?

It's just an API, I was imagining the group states being triggered
from where it makes sense. In our case it would currently be done
in the central system-wide runtime PM and system idle loop. Not
in device drivers at all.

> In most time,
> user will set and tune the low power mode of pins.
>
> If the low power setting
> could be configured via sysfs, it could save a lot of time to rebuild kernel.

pinctrl does not have a sysfs interface, and as per discussion with
Grant this needs to be designed with care. I think proper device nodes
for the pin controllers and iopctl():s is way more apropriate if we really
want a userspace interface to pinctrl stuff.

> Especially GPIO pins are board specific. I think that configure all low power
> setting in board specific code is more reasonable.

As outlined in my response to Stephen you supply the group states
to your driver along with any other platform data or DT nodes.

Yours,
Linus Walleij

RE: [PATCH v2] pinctrl: add support for group and pinmux states

[Stephen Warren]

Linus Walleij wrote at Monday, January 02, 2012 4:36 AM:
> Hi Stephen,
> 
> many thanks for the comments as usual!
> 
> On Tue, Dec 20, 2011 at 1:24 AM, Stephen Warren <swarren@nvidia.com> wrote:
> > Linus Walleij wrote at Sunday, December 18, 2011 4:07 PM:
> >>
> >> +To simplify handling of specific group states, such as when a group of
> >> +pins need to be deactivated or put in a specific sleep state, the pin controller
> >> +may implement a .pin_group_set_state() callback. This will for example be
> >> +called automatically to set a certain pin group in active state when the groups
> >> +are selected for use in a certain pin multiplexing configuration (see below).
> >> +This is a good opportunity to set up any default pin configuration (see below)
> >> +for the pins in a group.
> >
> > I don't think it's possible for a pinctrl driver to implement such a
> > callback; the appropriate state for a pin may well be board-specific,
> > and the pinctrl driver itself can't possibly know what that state is.
> 
> I don't see the conflict. The driver knows this from its supplied
> platform data or device tree nodes. It is indeed board-specific,
> as is many things a pinctrl driver may be using.

The main issue I see is that:

* With the current pin mux API, the driver is simply defining what the
SoC can do, and the mapping table (outside the driver) is selecting which
of those things to do. There's zero knowledge (or need) of board-specific
data in the pin mux driver.

* With your proposal for pin configuration, the driver still of course
defines what the SoC can do since it must implement it, but also the
(e.g. platform) data that defines which of those things to do is now
Handled by the driver rather than the pinctrl/pinconfig core.

That seems like a needless semantic difference.

While I agree it's most likely perfectly possible for the driver to
parse platform data or DT to obtain this board-specific pin config
information, I think that'd be a lot of work that all drivers have to
implement identically, so it might be better moved into the pinctrl
core.

Thinking more about this and putting it another way: Having the pinctrl
driver rather than the pinctrl core know what "suspend" means for a pin/
group, there's still the table of pin config settings that I'm asking
for, it's just that each individual pin ctrl driver is implementing the
table; we may as well move that common code to a central place.

> > For example, for some random pin that a board happens to be using as a
> > GPIO - how does the pinctrl driver know whether that GPIO actually does
> > something while in suspend; if the GPIO isn't used, the SoC can probably
> > just turn off all drivers and pull-up/downs on the pin, but if the GPIO
> > is used to control some aspect of a PMIC, it probably needs to be in a
> > specific state during suspend. This kind of thing is entirely board-
> > specific.
> 
> This is exactly the stuff it is supposed to handle. And the driver
> does so by inspecting its platform data/DT.
> 
> > Similarly, the sequencing of reprogramming of the various pins when
> > entering/leaving suspend is most likely board specific, e.g. there may
> > be a need to assert a "speaker mute" GPIO before de-asserting a GPIO
> > that enables power to speaker amplifiers etc.
> 
> I don't see how that affects pinctrl, you can assert your GPIO
> from somewhere in arch/arm/mach-foo/* and following that
> you can issue pinmux_set_state() for some other pins.

What if you need to do something like:

Put pin A into "suspend" state.

Reprogram GPIO X

Put pin B into "suspend" state.

It's possible to open-code that in a driver, but the sequence and even
need to do it is most likely board-specific. I feel it'd be best to
drive all this by data rather than code, and just loop over a list of
N pin config settings to apply. Handling that data and iterating over
the n settings seems like the domain of pinctrl?

> >> +enum pin_group_state {
> >> +     PIN_GROUP_STATE_DISABLED,
> >> +     PIN_GROUP_STATE_ACTIVE,
> >> +     PIN_GROUP_STATE_IDLE,
> >> +     PIN_GROUP_STATE_SLEEP,
> >> +};
> >
> > I'm unsure that having the pinctrl core explicitly define the set of
> > legal states is the right approach.
> 
> But later you say:
> 
> > For standard states (e.g. fully active, system suspend) we could define
> > (de-facto?) standardized names that drivers can use if appropriate.
> >
> > That'd prevent too many custom state strings, yet allow extensibility
> > when needed.
> 
> So I don't get this to match up... There is a clear advantage in
> defining a few standard states.

IIRC, I was talking about two different kinds of states.

The PIN_GROUP_STATE_* you quoted above are individual pin group states.

The "standard states" in the second chunk I wrote above was regarding
states of an entire device (a pmx entry I think)

A totally made up example might be the need to wake from sleep due to
SD card insertion, detected by the SD devices' CD (Change Detect) GPIO.
Here, the SD device's board-specific suspend state would perhaps include
the SD pins (clk, data) being in some suspend-oriented pin state, but
the GPIO pin would be in a full-power state to detect the plug-in.

> > This prevents domain-specific states
> > (e.g. PCIe power states say) from being defined without revising the
> > core.
> 
> I don't know much about PCIe, but I've heard about C-states and
> such things so I buy this argument.
> 
> > I'd like to suggest an alternative:
> >
> > a) The mapping table should be enhanced to cover all configuration that
> >   pinctrl can program:
> >   * Mux function selection
> >   * Whether a pin a used as a GPIO, if gpiolib doesn't interact with
> >     pinctrl to set this up.
> >   * Any pin config parameters that should be set.
> >   * Define all the above for each state that a driver can be in.
> >   * Name legal states as strings in the mapping table, so they're just
> >     data and any given driver can define the names of states it expects
> >     to have available.
> >   * The table can have a defined execution ordering this allowing board-
> >     specific sequencing requirements to be defined. Perhaps the mapping
> >     table can define the set of parameters to program for each legal
> >     to/from state transition combination, rather than target
> >     configuration for each state, so all sequencing is fully under
> >     control of the mapping table.
> 
> As mentioned before I prefer that pin configuration tables be
> kept separate from say pinmux tables.

Separate tables to store the data is one thing, and potentially
reasonable; I'm more concerned whether the table for pin mux and pin
config are similar and both handled the same way by pinctrl core, or
whether the pin config table/... has to be open-coded in a pinctrl
driver.

> And the last bullet is again a script engine...

A very simple one. I think all that's needed is blindly iterating over
each entry in an ordered list in a defined order. To me, "scripting
engine" implies state/variables, conditionals, loops, etc.

> While these may be good ideas and very useful I fear that
> coding it all up into one big table of states and settings
> is going to look complex and become hard to maintain.

Could you expand upon that? I'm not sure I agree.

If the individual pinctrl driver needs to know what "suspend" means for
a pin/group, I'd imagine the platform data or DT for it would just be a
table of settings for each pin anyway. I don't think moving the
responsibility between pinctrl core and driver is going to affect whether
there is this "one big table of states" or not.

> I would prefer a divide-and-conquer approach here, so
> that tables for mux settings and configs are two things
> whereas a optional transition script engine is a third thing
> utilizing the first two.
> 
> Neither has any implications for group states anyway -
> how you realize the (optional) group states, i.e. whether
> say a certain scriptlet is engaged, a certain table entry
> is activated or not, etc, should be up to the driver IMO.
> I.e. if implemented by a script engine you *can* do it
> that way but you don't *have to*. In our case and for many
> simple systems with a few states it would just add
> overhead.
>
> > b) Add a pinctrl API that transitions a given pmx (from pinmux_get())
> >   between states as defined in the mapping table.
> 
> From the external API point of view, the only difference from the
> outlined approach in this patch would be that instead of defining:
> 
> enum pin_group_state {
>     PIN_GROUP_STATE_DISABLED,
>     PIN_GROUP_STATE_ACTIVE,
>     (...)
> }
> 
> states would instead come from the platform data or so.
> 
> So if I just replace this enum with a string or so:
> pinmux_set_state(pmx, PIN_GROUP_STATE_ACTIVE);
> turns into:
> pinmux_set_state(pmx, "my-active-state-name");

I think the issue isn't so much about int/enum vs. string, but rather
about whether the core set of values defined by the pinctrl core can
be expanded to include more options for certain devices or classes of
device.

> > That should allow this to be fully generic.
> >
> > For standard states (e.g. fully active, system suspend) we could define
> > (de-facto?) standardized names that drivers can use if appropriate.
> >
> > That'd prevent too many custom state strings, yet allow extensibility
> > when needed.
> 
> That means if I refine the API from:
> 
> enum pin_group_state {
>     PIN_GROUP_STATE_DISABLED,
>     PIN_GROUP_STATE_ACTIVE,
>     PIN_GROUP_STATE_IDLE,
>     PIN_GROUP_STATE_SLEEP,
> };
> 
> To:
> 
> #define PIN_GROUP_STATE_DISABLED "disable"
> #define PIN_GROUP_STATE_ACTICE "active"
> #define PIN_GROUP_STATE_IDLE "idle"
> #define PIN_GROUP_STATE_SLEEP "sleep"
> 
> I have achieved the same thing but with string names for
> states, opening up for custom named states.
> 
> I can live with that I think, I'll hack something.
> 
> > IIRC, some of my previous emails have touched on this idea, and included
> > example mapping tables with multiple states per device.
> 
> Yes, but I do not (yet) embrace this idea :-)

-- 
nvpublic

Re: [PATCH v2] pinctrl: add support for group and pinmux states

[Linus Walleij]

On Sat, Jan 7, 2012 at 12:45 AM, Stephen Warren <swarren@nvidia.com> wrote:

> Linus Walleij wrote at Monday, January 02, 2012 4:36 AM:

> The main issue I see is that:
>
> * With the current pin mux API, the driver is simply defining what the
> SoC can do, and the mapping table (outside the driver) is selecting which
> of those things to do. There's zero knowledge (or need) of board-specific
> data in the pin mux driver.
>
> * With your proposal for pin configuration, the driver still of course
> defines what the SoC can do since it must implement it, but also the
> (e.g. platform) data that defines which of those things to do is now
> Handled by the driver rather than the pinctrl/pinconfig core.
>
> That seems like a needless semantic difference.

I'm  buying this and the following argument. I need to come up with
something better. Apparently this idea of states need to be paired
with a proper state configuration table like you suggest, so no
shortcuts (I was hoping to do one, then the other) I will have to
code it all in one go.

One minor thing:

> IIRC, I was talking about two different kinds of states.
>
> The PIN_GROUP_STATE_* you quoted above are individual pin group states.
>
> The "standard states" in the second chunk I wrote above was regarding
> states of an entire device (a pmx entry I think)

They were to use the same states.

Since a PMX corresponds to one or more groups these are easy
to couple, that's what it's all about. So setting the state of a pmx just
falls through to the state of it's mapped in groups. And if these
share the same syntax, it's a quick thing to do.

This will work with string-named states too I think, we'll see.

Yours,
Linus Walleij