This provides a single centralized name for the default state.
Update PIN_MAP_* macros to use this state name, instead of requiring the
user to pass a state name in.
With this change, hog entries in the mapping table are defined as those
with state name PINCTRL_STATE_DEFAULT, i.e. all entries have the same
name. This interacts badly with the nested iteration over mapping table
entries in pinctrl_hog_maps() and pinctrl_hog_map() which would now
attempt to claim each hog mapping table entry multiple times. Replacing
the custom hog code with a simple pinctrl_get()/pinctrl_enable().
Update documentation and mapping tables to use this.
Signed-off-by: Stephen Warren <[email protected]>
---
v2: Added all the hogging rework now described above; hopefully will solve
U300 runtime issues.
---
Documentation/pinctrl.txt | 8 +-
arch/arm/mach-u300/core.c | 6 +-
drivers/pinctrl/core.c | 145 ++-------------------------------------
drivers/pinctrl/core.h | 4 +-
include/linux/pinctrl/machine.h | 13 ++--
include/linux/pinctrl/pinctrl.h | 2 +
6 files changed, 26 insertions(+), 152 deletions(-)
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 5e314ce..6fe3232 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -814,7 +814,7 @@ it even more compact which assumes you want to use pinctrl-foo and position
0 for mapping, for example:
static struct pinctrl_map __initdata mapping[] = {
- PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
+ PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
};
@@ -930,7 +930,7 @@ foo_probe()
/* Allocate a state holder named "state" etc */
struct pinctrl p;
- p = pinctrl_get(&device, NULL);
+ p = pinctrl_get(&device, PINCTRL_STATE_DEFAULT);
if IS_ERR(p)
return PTR_ERR(p);
pinctrl_enable(p);
@@ -989,7 +989,7 @@ of the pin controller itself, like this:
{
.dev_name = "pinctrl-foo",
- .name = "POWERMAP"
+ .name = PINCTRL_STATE_DEFAULT,
.ctrl_dev_name = "pinctrl-foo",
.function = "power_func",
},
@@ -998,7 +998,7 @@ Since it may be common to request the core to hog a few always-applicable
mux settings on the primary pin controller, there is a convenience macro for
this:
-PIN_MAP_PRIMARY_SYS_HOG("POWERMAP", "pinctrl-foo", "power_func")
+PIN_MAP_SYS_HOG("pinctrl-foo", "power_func")
This gives the exact same result as the above construction.
diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
index 336526a..c9050dd 100644
--- a/arch/arm/mach-u300/core.c
+++ b/arch/arm/mach-u300/core.c
@@ -1569,9 +1569,9 @@ static struct platform_device dma_device = {
/* Pinmux settings */
static struct pinctrl_map __initdata u300_pinmux_map[] = {
/* anonymous maps for chip power and EMIFs */
- PIN_MAP_SYS_HOG("POWER", "pinctrl-u300", "power"),
- PIN_MAP_SYS_HOG("EMIF0", "pinctrl-u300", "emif0"),
- PIN_MAP_SYS_HOG("EMIF1", "pinctrl-u300", "emif1"),
+ PIN_MAP_SYS_HOG("pinctrl-u300", "power"),
+ PIN_MAP_SYS_HOG("pinctrl-u300", "emif0"),
+ PIN_MAP_SYS_HOG("pinctrl-u300", "emif1"),
/* per-device maps for MMC/SD, SPI and UART */
PIN_MAP("MMCSD", "pinctrl-u300", "mmc0", "mmci"),
PIN_MAP("SPI", "pinctrl-u300", "spi0", "pl022"),
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 376cede..f25307b 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -44,18 +44,6 @@ struct pinctrl_maps {
unsigned num_maps;
};
-/**
- * struct pinctrl_hog - a list item to stash control hogs
- * @node: pin control hog list node
- * @map: map entry responsible for this hogging
- * @pmx: the pin control hogged by this item
- */
-struct pinctrl_hog {
- struct list_head node;
- struct pinctrl_map const *map;
- struct pinctrl *p;
-};
-
/* Global list of pin control devices */
static DEFINE_MUTEX(pinctrldev_list_mutex);
static LIST_HEAD(pinctrldev_list);
@@ -702,103 +690,6 @@ int pinctrl_register_mappings(struct pinctrl_map const *maps,
return 0;
}
-/* Hog a single map entry and add to the hoglist */
-static int pinctrl_hog_map(struct pinctrl_dev *pctldev,
- struct pinctrl_map const *map)
-{
- struct pinctrl_hog *hog;
- struct pinctrl *p;
- int ret;
-
- hog = kzalloc(sizeof(*hog), GFP_KERNEL);
- if (!hog) {
- dev_err(pctldev->dev, "failed to alloc struct pinctrl_hog\n");
- return -ENOMEM;
- }
-
- p = pinctrl_get_locked(pctldev->dev, map->name);
- if (IS_ERR(p)) {
- kfree(hog);
- dev_err(pctldev->dev,
- "could not get the %s pin control mapping for hogging\n",
- map->name);
- return PTR_ERR(p);
- }
-
- ret = pinctrl_enable(p);
- if (ret) {
- pinctrl_put(p);
- kfree(hog);
- dev_err(pctldev->dev,
- "could not enable the %s pin control mapping for hogging\n",
- map->name);
- return ret;
- }
-
- hog->map = map;
- hog->p = p;
-
- dev_info(pctldev->dev, "hogged map %s, function %s\n", map->name,
- map->function);
- list_add_tail(&hog->node, &pctldev->pinctrl_hogs);
-
- return 0;
-}
-
-/**
- * pinctrl_hog_maps() - hog specific map entries on controller device
- * @pctldev: the pin control device to hog entries on
- *
- * When the pin controllers are registered, there may be some specific pinmux
- * map entries that need to be hogged, i.e. get+enabled until the system shuts
- * down.
- */
-static int pinctrl_hog_maps(struct pinctrl_dev *pctldev)
-{
- struct device *dev = pctldev->dev;
- const char *devname = dev_name(dev);
- int ret;
- struct pinctrl_maps *maps_node;
- int i;
- struct pinctrl_map const *map;
-
- INIT_LIST_HEAD(&pctldev->pinctrl_hogs);
-
- mutex_lock(&pinctrl_maps_mutex);
- for_each_maps(maps_node, i, map) {
- if (!strcmp(map->ctrl_dev_name, devname) &&
- !strcmp(map->dev_name, devname)) {
- /* OK time to hog! */
- ret = pinctrl_hog_map(pctldev, map);
- if (ret) {
- mutex_unlock(&pinctrl_maps_mutex);
- return ret;
- }
- }
- }
- mutex_unlock(&pinctrl_maps_mutex);
-
- return 0;
-}
-
-/**
- * pinctrl_unhog_maps() - unhog specific map entries on controller device
- * @pctldev: the pin control device to unhog entries on
- */
-static void pinctrl_unhog_maps(struct pinctrl_dev *pctldev)
-{
- struct list_head *node, *tmp;
-
- list_for_each_safe(node, tmp, &pctldev->pinctrl_hogs) {
- struct pinctrl_hog *hog =
- list_entry(node, struct pinctrl_hog, node);
- pinctrl_disable(hog->p);
- pinctrl_put(hog->p);
- list_del(node);
- kfree(hog);
- }
-}
-
#ifdef CONFIG_DEBUG_FS
static int pinctrl_pins_show(struct seq_file *s, void *what)
@@ -889,19 +780,6 @@ static int pinctrl_gpioranges_show(struct seq_file *s, void *what)
return 0;
}
-static int pinmux_hogs_show(struct seq_file *s, void *what)
-{
- struct pinctrl_dev *pctldev = s->private;
- struct pinctrl_hog *hog;
-
- seq_puts(s, "Pin control map hogs held by device\n");
-
- list_for_each_entry(hog, &pctldev->pinctrl_hogs, node)
- seq_printf(s, "%s\n", hog->map->name);
-
- return 0;
-}
-
static int pinctrl_devices_show(struct seq_file *s, void *what)
{
struct pinctrl_dev *pctldev;
@@ -988,11 +866,6 @@ static int pinctrl_gpioranges_open(struct inode *inode, struct file *file)
return single_open(file, pinctrl_gpioranges_show, inode->i_private);
}
-static int pinmux_hogs_open(struct inode *inode, struct file *file)
-{
- return single_open(file, pinmux_hogs_show, inode->i_private);
-}
-
static int pinctrl_devices_open(struct inode *inode, struct file *file)
{
return single_open(file, pinctrl_devices_show, NULL);
@@ -1029,13 +902,6 @@ static const struct file_operations pinctrl_gpioranges_ops = {
.release = single_release,
};
-static const struct file_operations pinmux_hogs_ops = {
- .open = pinmux_hogs_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release = single_release,
-};
-
static const struct file_operations pinctrl_devices_ops = {
.open = pinctrl_devices_open,
.read = seq_read,
@@ -1078,8 +944,6 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
device_root, pctldev, &pinctrl_groups_ops);
debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
device_root, pctldev, &pinctrl_gpioranges_ops);
- debugfs_create_file("pinmux-hogs", S_IFREG | S_IRUGO,
- device_root, pctldev, &pinmux_hogs_ops);
pinmux_init_device_debugfs(device_root, pctldev);
pinconf_init_device_debugfs(device_root, pctldev);
}
@@ -1188,7 +1052,9 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
mutex_lock(&pinctrldev_list_mutex);
list_add_tail(&pctldev->node, &pinctrldev_list);
mutex_unlock(&pinctrldev_list_mutex);
- pinctrl_hog_maps(pctldev);
+ pctldev->p = pinctrl_get(pctldev->dev, PINCTRL_STATE_DEFAULT);
+ if (!IS_ERR(pctldev->p))
+ pinctrl_enable(pctldev->p);
pinctrl_init_device_debugfs(pctldev);
return pctldev;
@@ -1211,7 +1077,10 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
return;
pinctrl_remove_device_debugfs(pctldev);
- pinctrl_unhog_maps(pctldev);
+ if (!IS_ERR(pctldev->p)) {
+ pinctrl_disable(pctldev->p);
+ pinctrl_put(pctldev->p);
+ }
/* TODO: check that no pinmuxes are still active? */
mutex_lock(&pinctrldev_list_mutex);
list_del(&pctldev->node);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 7551611..2981b73 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -27,7 +27,7 @@ struct pinctrl_gpio_range;
* @owner: module providing the pin controller, used for refcounting
* @driver_data: driver data for drivers registering to the pin controller
* subsystem
- * @pinctrl_hogs: list of pin control maps hogged by this device
+ * @p: result of pinctrl_get() for this device
* @device_root: debugfs root for this device
*/
struct pinctrl_dev {
@@ -39,7 +39,7 @@ struct pinctrl_dev {
struct device *dev;
struct module *owner;
void *driver_data;
- struct list_head pinctrl_hogs;
+ struct pinctrl *p;
#ifdef CONFIG_DEBUG_FS
struct dentry *device_root;
#endif
diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
index 73fbb27..20e9735 100644
--- a/include/linux/pinctrl/machine.h
+++ b/include/linux/pinctrl/machine.h
@@ -12,6 +12,8 @@
#ifndef __LINUX_PINCTRL_MACHINE_H
#define __LINUX_PINCTRL_MACHINE_H
+#include "pinctrl.h"
+
/**
* struct pinctrl_map - boards/machines shall provide this map for devices
* @dev_name: the name of the device using this specific mapping, the name
@@ -49,17 +51,18 @@ struct pinctrl_map {
* Convenience macro to map a system function onto a certain pinctrl device,
* to be hogged by the pin control core until the system shuts down.
*/
-#define PIN_MAP_SYS_HOG(a, b, c) \
- { .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, }
+#define PIN_MAP_SYS_HOG(a, b) \
+ { .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
+ .function = b, }
/*
* Convenience macro to map a system function onto a certain pinctrl device
* using a specified group, to be hogged by the pin control core until the
* system shuts down.
*/
-#define PIN_MAP_SYS_HOG_GROUP(a, b, c, d) \
- { .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, \
- .group = d, }
+#define PIN_MAP_SYS_HOG_GROUP(a, b, c) \
+ { .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
+ .function = b, .group = c, }
#ifdef CONFIG_PINMUX
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 8bd22ee..411fe23 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -19,6 +19,8 @@
#include <linux/list.h>
#include <linux/seq_file.h>
+#define PINCTRL_STATE_DEFAULT "default"
+
struct pinctrl_dev;
struct pinmux_ops;
struct pinconf_ops;
--
1.7.0.4
pinctrl_register_mappings() already requires that every mapping table
entry have a non-NULL name field.
Logically, this makes sense too; drivers should always request a specific
named state so they know what they're getting. Relying on getting the
first mentioned state in the mapping table is error-prone, and a nasty
special case to implement, given that a given the mapping table may define
multiple states for a device.
Remove a small part of the documentation that talked about optionally
requesting a specific state; it's mandatory now.
Signed-off-by: Stephen Warren <[email protected]>
---
v2: Update mach-u300/core.c and sirfsoc_uart.c not to request NULL state
names. Use PINCTRL_STATE_DEFAULT from previous patch.
This is now conceptually ack'd by Dong Aisheng, although I didn't actually
include the ack above since I've reworked the patch a little since I last
posted it (per the v2 changelog above).
---
Documentation/pinctrl.txt | 7 +++----
arch/arm/mach-u300/core.c | 8 ++++----
drivers/pinctrl/core.c | 17 +++++------------
drivers/tty/serial/sirfsoc_uart.c | 2 +-
4 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 6fe3232..558aac5 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -782,16 +782,19 @@ spi on the second function mapping:
static const struct pinctrl_map __initdata mapping[] = {
{
.dev_name = "foo-spi.0",
+ .name = PINCTRL_STATE_DEFAULT,
.ctrl_dev_name = "pinctrl-foo",
.function = "spi0",
},
{
.dev_name = "foo-i2c.0",
+ .name = PINCTRL_STATE_DEFAULT,
.ctrl_dev_name = "pinctrl-foo",
.function = "i2c0",
},
{
.dev_name = "foo-mmc.0",
+ .name = PINCTRL_STATE_DEFAULT,
.ctrl_dev_name = "pinctrl-foo",
.function = "mmc0",
},
@@ -944,10 +947,6 @@ foo_remove()
pinctrl_put(state->p);
}
-If you want to grab a specific control mapping and not just the first one
-found for this device you can specify a specific mapping name, for example in
-the above example the second i2c0 setting: pinctrl_get(&device, "spi0-pos-B");
-
This get/enable/disable/put sequence can just as well be handled by bus drivers
if you don't want each and every driver to handle it and you know the
arrangement on your bus.
diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
index c9050dd..5b37d84 100644
--- a/arch/arm/mach-u300/core.c
+++ b/arch/arm/mach-u300/core.c
@@ -1573,9 +1573,9 @@ static struct pinctrl_map __initdata u300_pinmux_map[] = {
PIN_MAP_SYS_HOG("pinctrl-u300", "emif0"),
PIN_MAP_SYS_HOG("pinctrl-u300", "emif1"),
/* per-device maps for MMC/SD, SPI and UART */
- PIN_MAP("MMCSD", "pinctrl-u300", "mmc0", "mmci"),
- PIN_MAP("SPI", "pinctrl-u300", "spi0", "pl022"),
- PIN_MAP("UART0", "pinctrl-u300", "uart0", "uart0"),
+ PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "mmc0", "mmci"),
+ PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "spi0", "pl022"),
+ PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "uart0", "uart0"),
};
struct u300_mux_hog {
@@ -1607,7 +1607,7 @@ static int __init u300_pinctrl_fetch(void)
struct pinctrl *p;
int ret;
- p = pinctrl_get(u300_mux_hogs[i].dev, NULL);
+ p = pinctrl_get(u300_mux_hogs[i].dev, PINCTRL_STATE_DEFAULT);
if (IS_ERR(p)) {
pr_err("u300: could not get pinmux hog %s\n",
u300_mux_hogs[i].name);
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index f25307b..6af6d8d 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -461,8 +461,8 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
int i;
struct pinctrl_map const *map;
- /* We must have a dev name */
- if (WARN_ON(!dev))
+ /* We must have both a dev and state name */
+ if (WARN_ON(!dev || !name))
return ERR_PTR(-EINVAL);
devname = dev_name(dev);
@@ -504,16 +504,9 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
if (strcmp(map->dev_name, devname))
continue;
- /*
- * If we're looking for a specific named map, this must match,
- * else we loop and look for the next.
- */
- if (name != NULL) {
- if (map->name == NULL)
- continue;
- if (strcmp(map->name, name))
- continue;
- }
+ /* State name must be the one we're looking for */
+ if (strcmp(map->name, name))
+ continue;
ret = pinmux_apply_muxmap(pctldev, p, dev, devname, map);
if (ret) {
diff --git a/drivers/tty/serial/sirfsoc_uart.c b/drivers/tty/serial/sirfsoc_uart.c
index c1a871e..3cabb65 100644
--- a/drivers/tty/serial/sirfsoc_uart.c
+++ b/drivers/tty/serial/sirfsoc_uart.c
@@ -673,7 +673,7 @@ int sirfsoc_uart_probe(struct platform_device *pdev)
port->irq = res->start;
if (sirfport->hw_flow_ctrl) {
- sirfport->p = pinctrl_get(&pdev->dev, NULL);
+ sirfport->p = pinctrl_get(&pdev->dev, PINCTRL_STATE_DEFAULT);
ret = IS_ERR(sirfport->p);
if (ret)
goto pin_err;
--
1.7.0.4
On Mon, Feb 27, 2012 at 04:55:08PM -0700, Stephen Warren wrote:
> This provides a single centralized name for the default state.
>
> Update PIN_MAP_* macros to use this state name, instead of requiring the
> user to pass a state name in.
>
> With this change, hog entries in the mapping table are defined as those
> with state name PINCTRL_STATE_DEFAULT, i.e. all entries have the same
> name. This interacts badly with the nested iteration over mapping table
> entries in pinctrl_hog_maps() and pinctrl_hog_map() which would now
> attempt to claim each hog mapping table entry multiple times. Replacing
> the custom hog code with a simple pinctrl_get()/pinctrl_enable().
>
> Update documentation and mapping tables to use this.
>
> Signed-off-by: Stephen Warren <[email protected]>
> ---
The whole patch looks good to me.
A few trivial comments:
> v2: Added all the hogging rework now described above; hopefully will solve
> U300 runtime issues.
> ---
> Documentation/pinctrl.txt | 8 +-
> arch/arm/mach-u300/core.c | 6 +-
> drivers/pinctrl/core.c | 145 ++-------------------------------------
> drivers/pinctrl/core.h | 4 +-
> include/linux/pinctrl/machine.h | 13 ++--
> include/linux/pinctrl/pinctrl.h | 2 +
> 6 files changed, 26 insertions(+), 152 deletions(-)
>
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> index 5e314ce..6fe3232 100644
> --- a/Documentation/pinctrl.txt
> +++ b/Documentation/pinctrl.txt
> @@ -814,7 +814,7 @@ it even more compact which assumes you want to use pinctrl-foo and position
> 0 for mapping, for example:
>
> static struct pinctrl_map __initdata mapping[] = {
> - PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
> + PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
It seemed Linus already applied your "re-order struct pinctrl_map".
This may need change a bit according to that patch.
>
>
> @@ -930,7 +930,7 @@ foo_probe()
> /* Allocate a state holder named "state" etc */
> struct pinctrl p;
>
> - p = pinctrl_get(&device, NULL);
> + p = pinctrl_get(&device, PINCTRL_STATE_DEFAULT);
> if IS_ERR(p)
> return PTR_ERR(p);
> pinctrl_enable(p);
> @@ -989,7 +989,7 @@ of the pin controller itself, like this:
>
> {
> .dev_name = "pinctrl-foo",
> - .name = "POWERMAP"
> + .name = PINCTRL_STATE_DEFAULT,
> .ctrl_dev_name = "pinctrl-foo",
> .function = "power_func",
> },
ditto
> @@ -998,7 +998,7 @@ Since it may be common to request the core to hog a few always-applicable
> mux settings on the primary pin controller, there is a convenience macro for
> this:
>
> -PIN_MAP_PRIMARY_SYS_HOG("POWERMAP", "pinctrl-foo", "power_func")
> +PIN_MAP_SYS_HOG("pinctrl-foo", "power_func")
>
> This gives the exact same result as the above construction.
>
> diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
> index 336526a..c9050dd 100644
> --- a/arch/arm/mach-u300/core.c
> +++ b/arch/arm/mach-u300/core.c
> @@ -1569,9 +1569,9 @@ static struct platform_device dma_device = {
> /* Pinmux settings */
> static struct pinctrl_map __initdata u300_pinmux_map[] = {
> /* anonymous maps for chip power and EMIFs */
> - PIN_MAP_SYS_HOG("POWER", "pinctrl-u300", "power"),
> - PIN_MAP_SYS_HOG("EMIF0", "pinctrl-u300", "emif0"),
> - PIN_MAP_SYS_HOG("EMIF1", "pinctrl-u300", "emif1"),
> + PIN_MAP_SYS_HOG("pinctrl-u300", "power"),
> + PIN_MAP_SYS_HOG("pinctrl-u300", "emif0"),
> + PIN_MAP_SYS_HOG("pinctrl-u300", "emif1"),
> /* per-device maps for MMC/SD, SPI and UART */
> PIN_MAP("MMCSD", "pinctrl-u300", "mmc0", "mmci"),
> PIN_MAP("SPI", "pinctrl-u300", "spi0", "pl022"),
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 376cede..f25307b 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -44,18 +44,6 @@ struct pinctrl_maps {
> unsigned num_maps;
> };
>
> -/**
> - * struct pinctrl_hog - a list item to stash control hogs
> - * @node: pin control hog list node
> - * @map: map entry responsible for this hogging
> - * @pmx: the pin control hogged by this item
> - */
> -struct pinctrl_hog {
> - struct list_head node;
> - struct pinctrl_map const *map;
> - struct pinctrl *p;
> -};
> -
> /* Global list of pin control devices */
> static DEFINE_MUTEX(pinctrldev_list_mutex);
> static LIST_HEAD(pinctrldev_list);
> @@ -702,103 +690,6 @@ int pinctrl_register_mappings(struct pinctrl_map const *maps,
> return 0;
> }
>
> -/* Hog a single map entry and add to the hoglist */
> -static int pinctrl_hog_map(struct pinctrl_dev *pctldev,
> - struct pinctrl_map const *map)
> -{
> - struct pinctrl_hog *hog;
> - struct pinctrl *p;
> - int ret;
> -
> - hog = kzalloc(sizeof(*hog), GFP_KERNEL);
> - if (!hog) {
> - dev_err(pctldev->dev, "failed to alloc struct pinctrl_hog\n");
> - return -ENOMEM;
> - }
> -
> - p = pinctrl_get_locked(pctldev->dev, map->name);
> - if (IS_ERR(p)) {
> - kfree(hog);
> - dev_err(pctldev->dev,
> - "could not get the %s pin control mapping for hogging\n",
> - map->name);
> - return PTR_ERR(p);
> - }
> -
> - ret = pinctrl_enable(p);
> - if (ret) {
> - pinctrl_put(p);
> - kfree(hog);
> - dev_err(pctldev->dev,
> - "could not enable the %s pin control mapping for hogging\n",
> - map->name);
> - return ret;
> - }
> -
> - hog->map = map;
> - hog->p = p;
> -
> - dev_info(pctldev->dev, "hogged map %s, function %s\n", map->name,
> - map->function);
> - list_add_tail(&hog->node, &pctldev->pinctrl_hogs);
> -
> - return 0;
> -}
> -
> -/**
> - * pinctrl_hog_maps() - hog specific map entries on controller device
> - * @pctldev: the pin control device to hog entries on
> - *
> - * When the pin controllers are registered, there may be some specific pinmux
> - * map entries that need to be hogged, i.e. get+enabled until the system shuts
> - * down.
> - */
> -static int pinctrl_hog_maps(struct pinctrl_dev *pctldev)
> -{
> - struct device *dev = pctldev->dev;
> - const char *devname = dev_name(dev);
> - int ret;
> - struct pinctrl_maps *maps_node;
> - int i;
> - struct pinctrl_map const *map;
> -
> - INIT_LIST_HEAD(&pctldev->pinctrl_hogs);
> -
> - mutex_lock(&pinctrl_maps_mutex);
> - for_each_maps(maps_node, i, map) {
> - if (!strcmp(map->ctrl_dev_name, devname) &&
> - !strcmp(map->dev_name, devname)) {
> - /* OK time to hog! */
> - ret = pinctrl_hog_map(pctldev, map);
> - if (ret) {
> - mutex_unlock(&pinctrl_maps_mutex);
> - return ret;
> - }
> - }
> - }
> - mutex_unlock(&pinctrl_maps_mutex);
> -
> - return 0;
> -}
> -
> -/**
> - * pinctrl_unhog_maps() - unhog specific map entries on controller device
> - * @pctldev: the pin control device to unhog entries on
> - */
> -static void pinctrl_unhog_maps(struct pinctrl_dev *pctldev)
> -{
> - struct list_head *node, *tmp;
> -
> - list_for_each_safe(node, tmp, &pctldev->pinctrl_hogs) {
> - struct pinctrl_hog *hog =
> - list_entry(node, struct pinctrl_hog, node);
> - pinctrl_disable(hog->p);
> - pinctrl_put(hog->p);
> - list_del(node);
> - kfree(hog);
> - }
> -}
> -
> #ifdef CONFIG_DEBUG_FS
>
> static int pinctrl_pins_show(struct seq_file *s, void *what)
> @@ -889,19 +780,6 @@ static int pinctrl_gpioranges_show(struct seq_file *s, void *what)
> return 0;
> }
>
> -static int pinmux_hogs_show(struct seq_file *s, void *what)
> -{
> - struct pinctrl_dev *pctldev = s->private;
> - struct pinctrl_hog *hog;
> -
> - seq_puts(s, "Pin control map hogs held by device\n");
> -
> - list_for_each_entry(hog, &pctldev->pinctrl_hogs, node)
> - seq_printf(s, "%s\n", hog->map->name);
> -
> - return 0;
> -}
> -
> static int pinctrl_devices_show(struct seq_file *s, void *what)
> {
> struct pinctrl_dev *pctldev;
> @@ -988,11 +866,6 @@ static int pinctrl_gpioranges_open(struct inode *inode, struct file *file)
> return single_open(file, pinctrl_gpioranges_show, inode->i_private);
> }
>
> -static int pinmux_hogs_open(struct inode *inode, struct file *file)
> -{
> - return single_open(file, pinmux_hogs_show, inode->i_private);
> -}
> -
> static int pinctrl_devices_open(struct inode *inode, struct file *file)
> {
> return single_open(file, pinctrl_devices_show, NULL);
> @@ -1029,13 +902,6 @@ static const struct file_operations pinctrl_gpioranges_ops = {
> .release = single_release,
> };
>
> -static const struct file_operations pinmux_hogs_ops = {
> - .open = pinmux_hogs_open,
> - .read = seq_read,
> - .llseek = seq_lseek,
> - .release = single_release,
> -};
> -
> static const struct file_operations pinctrl_devices_ops = {
> .open = pinctrl_devices_open,
> .read = seq_read,
> @@ -1078,8 +944,6 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
> device_root, pctldev, &pinctrl_groups_ops);
> debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
> device_root, pctldev, &pinctrl_gpioranges_ops);
> - debugfs_create_file("pinmux-hogs", S_IFREG | S_IRUGO,
> - device_root, pctldev, &pinmux_hogs_ops);
I see you remove the pinnmux-hogs sysfs file here.
I guess you may want to merge it into pinctrl-maps sysfs file since
they're almost same(only difference is device name).
Do you think if we can add a special flag for that type of map in sysfs
(e.g. a "Hog" flag behind the regular map debug info)?
Then users do not need to check device's name to see if it's a hogged function.
(Anyway, it should be in another patch)
> pinmux_init_device_debugfs(device_root, pctldev);
> pinconf_init_device_debugfs(device_root, pctldev);
> }
> @@ -1188,7 +1052,9 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
> mutex_lock(&pinctrldev_list_mutex);
> list_add_tail(&pctldev->node, &pinctrldev_list);
> mutex_unlock(&pinctrldev_list_mutex);
> - pinctrl_hog_maps(pctldev);
> + pctldev->p = pinctrl_get(pctldev->dev, PINCTRL_STATE_DEFAULT);
> + if (!IS_ERR(pctldev->p))
> + pinctrl_enable(pctldev->p);
This change looks good to me.
> pinctrl_init_device_debugfs(pctldev);
>
> return pctldev;
> @@ -1211,7 +1077,10 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
> return;
>
> pinctrl_remove_device_debugfs(pctldev);
> - pinctrl_unhog_maps(pctldev);
> + if (!IS_ERR(pctldev->p)) {
> + pinctrl_disable(pctldev->p);
> + pinctrl_put(pctldev->p);
> + }
> /* TODO: check that no pinmuxes are still active? */
> mutex_lock(&pinctrldev_list_mutex);
> list_del(&pctldev->node);
> diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
> index 7551611..2981b73 100644
> --- a/drivers/pinctrl/core.h
> +++ b/drivers/pinctrl/core.h
> @@ -27,7 +27,7 @@ struct pinctrl_gpio_range;
> * @owner: module providing the pin controller, used for refcounting
> * @driver_data: driver data for drivers registering to the pin controller
> * subsystem
> - * @pinctrl_hogs: list of pin control maps hogged by this device
> + * @p: result of pinctrl_get() for this device
> * @device_root: debugfs root for this device
> */
> struct pinctrl_dev {
> @@ -39,7 +39,7 @@ struct pinctrl_dev {
> struct device *dev;
> struct module *owner;
> void *driver_data;
> - struct list_head pinctrl_hogs;
> + struct pinctrl *p;
> #ifdef CONFIG_DEBUG_FS
> struct dentry *device_root;
> #endif
> diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
> index 73fbb27..20e9735 100644
> --- a/include/linux/pinctrl/machine.h
> +++ b/include/linux/pinctrl/machine.h
> @@ -12,6 +12,8 @@
> #ifndef __LINUX_PINCTRL_MACHINE_H
> #define __LINUX_PINCTRL_MACHINE_H
>
> +#include "pinctrl.h"
> +
> /**
> * struct pinctrl_map - boards/machines shall provide this map for devices
> * @dev_name: the name of the device using this specific mapping, the name
> @@ -49,17 +51,18 @@ struct pinctrl_map {
> * Convenience macro to map a system function onto a certain pinctrl device,
> * to be hogged by the pin control core until the system shuts down.
> */
> -#define PIN_MAP_SYS_HOG(a, b, c) \
> - { .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, }
> +#define PIN_MAP_SYS_HOG(a, b) \
> + { .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
> + .function = b, }
>
> /*
> * Convenience macro to map a system function onto a certain pinctrl device
> * using a specified group, to be hogged by the pin control core until the
> * system shuts down.
> */
> -#define PIN_MAP_SYS_HOG_GROUP(a, b, c, d) \
> - { .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, \
> - .group = d, }
> +#define PIN_MAP_SYS_HOG_GROUP(a, b, c) \
> + { .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
> + .function = b, .group = c, }
>
In your v1 patch, we discussed about the possible requirement of different state
support for hog functions(it seemed still no conclusion, right?).
If we decide to support it, i'd suggest change here macro a bit to align with
the exist PIN_MAP macro like:
#define PIN_MAP_SYS_HOG_DEFAULT(a, b) \
{ .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
.function = b, }
#define PIN_MAP_SYS_HOG_GROUP_DEFAULT(a, b, c) \
{ .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
.function = b, .group = c, }
And reserve the original PIN_MAP_SYS_HOG for users to add the state name as they
want.
However, since this is still undetermined, i'm also ok if you want to use this
macro first.
Acked-by: Dong Aisheng <[email protected]>
Regards
Dong Aisheng
On Mon, Feb 27, 2012 at 04:55:09PM -0700, Stephen Warren wrote:
> pinctrl_register_mappings() already requires that every mapping table
> entry have a non-NULL name field.
>
> Logically, this makes sense too; drivers should always request a specific
> named state so they know what they're getting. Relying on getting the
> first mentioned state in the mapping table is error-prone, and a nasty
> special case to implement, given that a given the mapping table may define
> multiple states for a device.
>
> Remove a small part of the documentation that talked about optionally
> requesting a specific state; it's mandatory now.
>
> Signed-off-by: Stephen Warren <[email protected]>
> ---
> v2: Update mach-u300/core.c and sirfsoc_uart.c not to request NULL state
> names. Use PINCTRL_STATE_DEFAULT from previous patch.
>
> This is now conceptually ack'd by Dong Aisheng, although I didn't actually
> include the ack above since I've reworked the patch a little since I last
> posted it (per the v2 changelog above).
> ---
It looks very good to me.
Acked-by: Dong Aisheng <[email protected]>
Regards
Dong Aisheng
> Documentation/pinctrl.txt | 7 +++----
> arch/arm/mach-u300/core.c | 8 ++++----
> drivers/pinctrl/core.c | 17 +++++------------
> drivers/tty/serial/sirfsoc_uart.c | 2 +-
> 4 files changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> index 6fe3232..558aac5 100644
> --- a/Documentation/pinctrl.txt
> +++ b/Documentation/pinctrl.txt
> @@ -782,16 +782,19 @@ spi on the second function mapping:
> static const struct pinctrl_map __initdata mapping[] = {
> {
> .dev_name = "foo-spi.0",
> + .name = PINCTRL_STATE_DEFAULT,
> .ctrl_dev_name = "pinctrl-foo",
> .function = "spi0",
> },
> {
> .dev_name = "foo-i2c.0",
> + .name = PINCTRL_STATE_DEFAULT,
> .ctrl_dev_name = "pinctrl-foo",
> .function = "i2c0",
> },
> {
> .dev_name = "foo-mmc.0",
> + .name = PINCTRL_STATE_DEFAULT,
> .ctrl_dev_name = "pinctrl-foo",
> .function = "mmc0",
> },
> @@ -944,10 +947,6 @@ foo_remove()
> pinctrl_put(state->p);
> }
>
> -If you want to grab a specific control mapping and not just the first one
> -found for this device you can specify a specific mapping name, for example in
> -the above example the second i2c0 setting: pinctrl_get(&device, "spi0-pos-B");
> -
> This get/enable/disable/put sequence can just as well be handled by bus drivers
> if you don't want each and every driver to handle it and you know the
> arrangement on your bus.
> diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
> index c9050dd..5b37d84 100644
> --- a/arch/arm/mach-u300/core.c
> +++ b/arch/arm/mach-u300/core.c
> @@ -1573,9 +1573,9 @@ static struct pinctrl_map __initdata u300_pinmux_map[] = {
> PIN_MAP_SYS_HOG("pinctrl-u300", "emif0"),
> PIN_MAP_SYS_HOG("pinctrl-u300", "emif1"),
> /* per-device maps for MMC/SD, SPI and UART */
> - PIN_MAP("MMCSD", "pinctrl-u300", "mmc0", "mmci"),
> - PIN_MAP("SPI", "pinctrl-u300", "spi0", "pl022"),
> - PIN_MAP("UART0", "pinctrl-u300", "uart0", "uart0"),
> + PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "mmc0", "mmci"),
> + PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "spi0", "pl022"),
> + PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-u300", "uart0", "uart0"),
> };
>
> struct u300_mux_hog {
> @@ -1607,7 +1607,7 @@ static int __init u300_pinctrl_fetch(void)
> struct pinctrl *p;
> int ret;
>
> - p = pinctrl_get(u300_mux_hogs[i].dev, NULL);
> + p = pinctrl_get(u300_mux_hogs[i].dev, PINCTRL_STATE_DEFAULT);
> if (IS_ERR(p)) {
> pr_err("u300: could not get pinmux hog %s\n",
> u300_mux_hogs[i].name);
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index f25307b..6af6d8d 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -461,8 +461,8 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
> int i;
> struct pinctrl_map const *map;
>
> - /* We must have a dev name */
> - if (WARN_ON(!dev))
> + /* We must have both a dev and state name */
> + if (WARN_ON(!dev || !name))
> return ERR_PTR(-EINVAL);
>
> devname = dev_name(dev);
> @@ -504,16 +504,9 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
> if (strcmp(map->dev_name, devname))
> continue;
>
> - /*
> - * If we're looking for a specific named map, this must match,
> - * else we loop and look for the next.
> - */
> - if (name != NULL) {
> - if (map->name == NULL)
> - continue;
> - if (strcmp(map->name, name))
> - continue;
> - }
> + /* State name must be the one we're looking for */
> + if (strcmp(map->name, name))
> + continue;
>
> ret = pinmux_apply_muxmap(pctldev, p, dev, devname, map);
> if (ret) {
> diff --git a/drivers/tty/serial/sirfsoc_uart.c b/drivers/tty/serial/sirfsoc_uart.c
> index c1a871e..3cabb65 100644
> --- a/drivers/tty/serial/sirfsoc_uart.c
> +++ b/drivers/tty/serial/sirfsoc_uart.c
> @@ -673,7 +673,7 @@ int sirfsoc_uart_probe(struct platform_device *pdev)
> port->irq = res->start;
>
> if (sirfport->hw_flow_ctrl) {
> - sirfport->p = pinctrl_get(&pdev->dev, NULL);
> + sirfport->p = pinctrl_get(&pdev->dev, PINCTRL_STATE_DEFAULT);
> ret = IS_ERR(sirfport->p);
> if (ret)
> goto pin_err;
> --
> 1.7.0.4
>
>
Dong Aisheng wrote at Tuesday, February 28, 2012 2:30 AM:
> On Mon, Feb 27, 2012 at 04:55:08PM -0700, Stephen Warren wrote:
> > This provides a single centralized name for the default state.
> >
> > Update PIN_MAP_* macros to use this state name, instead of requiring the
> > user to pass a state name in.
...
> > diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> > static struct pinctrl_map __initdata mapping[] = {
> > - PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
> > + PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
>
> It seemed Linus already applied your "re-order struct pinctrl_map".
> This may need change a bit according to that patch.
Yes, that patch is applied.
However, note that I deliberately did not change PIN_MAP()'s parameter
order in that patch. I deferred as much rework of those macros to the
later patch "pinctrl: Enhance mapping table to support pin config
operations" to avoid repeatedly changing those macros where possible.
For reference, the definition of PIN_MAP() at this point in the series
is:
#define PIN_MAP(a, b, c, d) \
{ .name = a, .ctrl_dev_name = b, .function = c, .dev_name = d }
> > @@ -989,7 +989,7 @@ of the pin controller itself, like this:
> >
> > {
> > .dev_name = "pinctrl-foo",
> > - .name = "POWERMAP"
> > + .name = PINCTRL_STATE_DEFAULT,
> > .ctrl_dev_name = "pinctrl-foo",
> > .function = "power_func",
> > },
>
> ditto
That already matches the order of fields in the struct definition,
which for reference is as follows at this point in the series:
struct pinctrl_map {
const char *dev_name;
const char *name;
const char *ctrl_dev_name;
const char *function;
const char *group;
};
(.group is still allowed to be NULL)
> > @@ -1078,8 +944,6 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
> > device_root, pctldev, &pinctrl_groups_ops);
> > debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
> > device_root, pctldev, &pinctrl_gpioranges_ops);
> > - debugfs_create_file("pinmux-hogs", S_IFREG | S_IRUGO,
> > - device_root, pctldev, &pinmux_hogs_ops);
>
> I see you remove the pinnmux-hogs sysfs file here.
> I guess you may want to merge it into pinctrl-maps sysfs file since
> they're almost same(only difference is device name).
>
> Do you think if we can add a special flag for that type of map in sysfs
> (e.g. a "Hog" flag behind the regular map debug info)?
> Then users do not need to check device's name to see if it's a hogged function.
>
> (Anyway, it should be in another patch)
I'd rather not myself; I don't see why "hog" should be a special-case;
it's just that the pin controller driver's mapping table entries have
it set up certain pin mux/config settings.
That said, feel free to submit a patch for this. I'd prefer that any
text you add to the debugfs files to indicate this "hogging" be in
addition to anything that's already present rather than replacing it
(as has unfortunately happened already in "pinmux-pins"), otherwise
it obscures information.
> > diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
> > index 73fbb27..20e9735 100644
> > --- a/include/linux/pinctrl/machine.h
> > +++ b/include/linux/pinctrl/machine.h
> > @@ -12,6 +12,8 @@
> > #ifndef __LINUX_PINCTRL_MACHINE_H
> > #define __LINUX_PINCTRL_MACHINE_H
> >
> > +#include "pinctrl.h"
> > +
> > /**
> > * struct pinctrl_map - boards/machines shall provide this map for devices
> > * @dev_name: the name of the device using this specific mapping, the name
> > @@ -49,17 +51,18 @@ struct pinctrl_map {
> > * Convenience macro to map a system function onto a certain pinctrl device,
> > * to be hogged by the pin control core until the system shuts down.
> > */
> > -#define PIN_MAP_SYS_HOG(a, b, c) \
> > - { .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, }
> > +#define PIN_MAP_SYS_HOG(a, b) \
> > + { .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
> > + .function = b, }
> >
> > /*
> > * Convenience macro to map a system function onto a certain pinctrl device
> > * using a specified group, to be hogged by the pin control core until the
> > * system shuts down.
> > */
> > -#define PIN_MAP_SYS_HOG_GROUP(a, b, c, d) \
> > - { .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, \
> > - .group = d, }
> > +#define PIN_MAP_SYS_HOG_GROUP(a, b, c) \
> > + { .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
> > + .function = b, .group = c, }
> >
>
> In your v1 patch, we discussed about the possible requirement of different state
> support for hog functions(it seemed still no conclusion, right?).
I agreed to fully support that. However as I mentioned, the support will be
actually implemented in patch "pinctrl: Enhance mapping table to support
pin config operations" since I was already reworking all the mapping
table macros there, it felt better to do the rework once in that patch.
I'm waiting on resolution of the locking patch issue before I repost an
updated version of the mapping table rework patch that includes this
support.
> Acked-by: Dong Aisheng <[email protected]>
Thanks.
--
nvpublic
On Tue, Feb 28, 2012 at 09:26:22AM -0800, Stephen Warren wrote:
> Dong Aisheng wrote at Tuesday, February 28, 2012 2:30 AM:
> > On Mon, Feb 27, 2012 at 04:55:08PM -0700, Stephen Warren wrote:
> > > This provides a single centralized name for the default state.
> > >
> > > Update PIN_MAP_* macros to use this state name, instead of requiring the
> > > user to pass a state name in.
> ...
>
> > > diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
>
> > > static struct pinctrl_map __initdata mapping[] = {
> > > - PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
> > > + PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
> >
> > It seemed Linus already applied your "re-order struct pinctrl_map".
> > This may need change a bit according to that patch.
>
> Yes, that patch is applied.
>
> However, note that I deliberately did not change PIN_MAP()'s parameter
> order in that patch. I deferred as much rework of those macros to the
> later patch "pinctrl: Enhance mapping table to support pin config
> operations" to avoid repeatedly changing those macros where possible.
>
> For reference, the definition of PIN_MAP() at this point in the series
> is:
>
> #define PIN_MAP(a, b, c, d) \
> { .name = a, .ctrl_dev_name = b, .function = c, .dev_name = d }
>
Ok, i see.
> (.group is still allowed to be NULL)
>
> > > @@ -1078,8 +944,6 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
> > > device_root, pctldev, &pinctrl_groups_ops);
> > > debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
> > > device_root, pctldev, &pinctrl_gpioranges_ops);
> > > - debugfs_create_file("pinmux-hogs", S_IFREG | S_IRUGO,
> > > - device_root, pctldev, &pinmux_hogs_ops);
> >
> > I see you remove the pinnmux-hogs sysfs file here.
> > I guess you may want to merge it into pinctrl-maps sysfs file since
> > they're almost same(only difference is device name).
> >
> > Do you think if we can add a special flag for that type of map in sysfs
> > (e.g. a "Hog" flag behind the regular map debug info)?
> > Then users do not need to check device's name to see if it's a hogged function.
> >
> > (Anyway, it should be in another patch)
>
> I'd rather not myself; I don't see why "hog" should be a special-case;
> it's just that the pin controller driver's mapping table entries have
> it set up certain pin mux/config settings.
>
> That said, feel free to submit a patch for this. I'd prefer that any
> text you add to the debugfs files to indicate this "hogging" be in
> addition to anything that's already present rather than replacing it
> (as has unfortunately happened already in "pinmux-pins"), otherwise
> it obscures information.
>
Yes, correct.
> > > diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
> > > index 73fbb27..20e9735 100644
> > > --- a/include/linux/pinctrl/machine.h
> > > +++ b/include/linux/pinctrl/machine.h
> > > @@ -12,6 +12,8 @@
> > > #ifndef __LINUX_PINCTRL_MACHINE_H
> > > #define __LINUX_PINCTRL_MACHINE_H
> > >
> > > +#include "pinctrl.h"
> > > +
> > > /**
> > > * struct pinctrl_map - boards/machines shall provide this map for devices
> > > * @dev_name: the name of the device using this specific mapping, the name
> > > @@ -49,17 +51,18 @@ struct pinctrl_map {
> > > * Convenience macro to map a system function onto a certain pinctrl device,
> > > * to be hogged by the pin control core until the system shuts down.
> > > */
> > > -#define PIN_MAP_SYS_HOG(a, b, c) \
> > > - { .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, }
> > > +#define PIN_MAP_SYS_HOG(a, b) \
> > > + { .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
> > > + .function = b, }
> > >
> > > /*
> > > * Convenience macro to map a system function onto a certain pinctrl device
> > > * using a specified group, to be hogged by the pin control core until the
> > > * system shuts down.
> > > */
> > > -#define PIN_MAP_SYS_HOG_GROUP(a, b, c, d) \
> > > - { .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, \
> > > - .group = d, }
> > > +#define PIN_MAP_SYS_HOG_GROUP(a, b, c) \
> > > + { .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
> > > + .function = b, .group = c, }
> > >
> >
> > In your v1 patch, we discussed about the possible requirement of different state
> > support for hog functions(it seemed still no conclusion, right?).
>
> I agreed to fully support that. However as I mentioned, the support will be
Great.
> actually implemented in patch "pinctrl: Enhance mapping table to support
> pin config operations" since I was already reworking all the mapping
> table macros there, it felt better to do the rework once in that patch.
>
Reasonable to me.
> I'm waiting on resolution of the locking patch issue before I repost an
> updated version of the mapping table rework patch that includes this
> support.
>
Sounds good.
> > Acked-by: Dong Aisheng <[email protected]>
>
Regards
Dong Aisheng
On Tue, Feb 28, 2012 at 12:55 AM, Stephen Warren <[email protected]> wrote:
> This provides a single centralized name for the default state.
>
> Update PIN_MAP_* macros to use this state name, instead of requiring the
> user to pass a state name in.
>
> With this change, hog entries in the mapping table are defined as those
> with state name PINCTRL_STATE_DEFAULT, i.e. all entries have the same
> name. This interacts badly with the nested iteration over mapping table
> entries in pinctrl_hog_maps() and pinctrl_hog_map() which would now
> attempt to claim each hog mapping table entry multiple times. Replacing
> the custom hog code with a simple pinctrl_get()/pinctrl_enable().
>
> Update documentation and mapping tables to use this.
>
> Signed-off-by: Stephen Warren <[email protected]>
> ---
> v2: Added all the hogging rework now described above; hopefully will solve
> U300 runtime issues.
This is a bit better, I now have my devices but I lost my hogs :-(
Trying to see if I can fix it...
Yours,
Linus Walleij
Aha now I see the problem here...
On Tue, Feb 28, 2012 at 12:55 AM, Stephen Warren <[email protected]> wrote:
> This provides a single centralized name for the default state.
(...)
> -/* Hog a single map entry and add to the hoglist */
> -static int pinctrl_hog_map(struct pinctrl_dev *pctldev,
(...)
> -static int pinctrl_hog_maps(struct pinctrl_dev *pctldev)
(...)
> - ? ? ? pinctrl_hog_maps(pctldev);
> + ? ? ? pctldev->p = pinctrl_get(pctldev->dev, PINCTRL_STATE_DEFAULT);
> + ? ? ? if (!IS_ERR(pctldev->p))
> + ? ? ? ? ? ? ? pinctrl_enable(pctldev->p);
So what happens here is that my hogs will try to activate three different
functions in the same struct pinctrl *p.
This fails the sanity check in pinmux.c:
/*
* If the function selector is already set, it needs to be identical,
* we support several groups with one function but not several
* functions with one or several groups in the same pinmux.
*/
if (p->func_selector != UINT_MAX &&
p->func_selector != func_selector) {
dev_err(pctldev->dev,
"dual function defines in the map for device %s\n",
devname);
return -EINVAL;
}
p->func_selector = func_selector;
Since it is assumed that we can only activate one function at a time
for a given device handle (struct pinmux).
Which was sound, since we said that one single function can activate
several groups.
So you still need one struct pinmux *p for each hog or it won't work.
Allowing the struct pinmux to activate several functions at once is
another solution, but would defy the idea of having several groups
mapped to one function, then you could just as well use that instead,
and it would be a tad bit too many "many to many" relations in my
opinion.
So I would revert back to using one handle per hog.
(Sorry if I misunderstood the problem though we're getting pretty
complex here...)
Yours,
Linus Walleij
Linus Walleij wrote at Wednesday, February 29, 2012 9:41 AM:
> Aha now I see the problem here...
>
> On Tue, Feb 28, 2012 at 12:55 AM, Stephen Warren <[email protected]> wrote:
>
> > This provides a single centralized name for the default state.
> (...)
> > -/* Hog a single map entry and add to the hoglist */
> > -static int pinctrl_hog_map(struct pinctrl_dev *pctldev,
> (...)
> > -static int pinctrl_hog_maps(struct pinctrl_dev *pctldev)
> (...)
> > - ? ? ? pinctrl_hog_maps(pctldev);
> > + ? ? ? pctldev->p = pinctrl_get(pctldev->dev, PINCTRL_STATE_DEFAULT);
> > + ? ? ? if (!IS_ERR(pctldev->p))
> > + ? ? ? ? ? ? ? pinctrl_enable(pctldev->p);
>
> So what happens here is that my hogs will try to activate three different
> functions in the same struct pinctrl *p.
>
> This fails the sanity check in pinmux.c:
>
> /*
> * If the function selector is already set, it needs to be identical,
> * we support several groups with one function but not several
> * functions with one or several groups in the same pinmux.
> */
> if (p->func_selector != UINT_MAX &&
> p->func_selector != func_selector) {
> dev_err(pctldev->dev,
> "dual function defines in the map for device %s\n",
> devname);
> return -EINVAL;
> }
> p->func_selector = func_selector;
Oh right, I'd forgotten about that check at this stage in the patch
series.
Later in the series, this restriction is removed; each setting in a state
is completely independent, and could easily program a different function
onto each group, or the same function. At this stage in the series, a
struct pinctrl can't represent everything that a set of mapping table
entries can, but later struct pinctrl is fully capable.
I guess the solution here is to just make this patch introduce the
PINCTRL_STATE_DEFAULT define, but defer most usage of it until later
in the series when this restriction is removed.
Note that if you apply all my patches together, you should find that
U300 is unbroken, so you can at least test everything. Actually
committing that probably isn't a good idea since it'll break git bisect
for a little while though.
--
nvpublic
Stephen Warren wrote at Wednesday, February 29, 2012 10:42 AM:
> Linus Walleij wrote at Wednesday, February 29, 2012 9:41 AM:
> > Aha now I see the problem here...
> >
> > On Tue, Feb 28, 2012 at 12:55 AM, Stephen Warren <[email protected]> wrote:
> >
> > > This provides a single centralized name for the default state.
> > (...)
> > > -/* Hog a single map entry and add to the hoglist */
> > > -static int pinctrl_hog_map(struct pinctrl_dev *pctldev,
> > (...)
> > > -static int pinctrl_hog_maps(struct pinctrl_dev *pctldev)
> > (...)
> > > - ? ? ? pinctrl_hog_maps(pctldev);
> > > + ? ? ? pctldev->p = pinctrl_get(pctldev->dev, PINCTRL_STATE_DEFAULT);
> > > + ? ? ? if (!IS_ERR(pctldev->p))
> > > + ? ? ? ? ? ? ? pinctrl_enable(pctldev->p);
> >
> > So what happens here is that my hogs will try to activate three different
> > functions in the same struct pinctrl *p.
> >
> > This fails the sanity check in pinmux.c:
> >
> > /*
> > * If the function selector is already set, it needs to be identical,
> > * we support several groups with one function but not several
> > * functions with one or several groups in the same pinmux.
> > */
> > if (p->func_selector != UINT_MAX &&
> > p->func_selector != func_selector) {
> > dev_err(pctldev->dev,
> > "dual function defines in the map for device %s\n",
> > devname);
> > return -EINVAL;
> > }
> > p->func_selector = func_selector;
>
> Oh right, I'd forgotten about that check at this stage in the patch
> series.
>
> Later in the series, this restriction is removed; each setting in a state
> is completely independent, and could easily program a different function
> onto each group, or the same function. At this stage in the series, a
> struct pinctrl can't represent everything that a set of mapping table
> entries can, but later struct pinctrl is fully capable.
>
> I guess the solution here is to just make this patch introduce the
> PINCTRL_STATE_DEFAULT define, but defer most usage of it until later
> in the series when this restriction is removed.
Actually, it looks very easy to move the func_selector field out of
struct pinctrl into struct pinmux_group, which would solve this problem
in what I consider the correct way. I'll code up a patch to do that and
stack it before this current patch.
--
nvpublic
On Wed, Feb 29, 2012 at 8:21 PM, Stephen Warren <[email protected]> wrote:
> Actually, it looks very easy to move the func_selector field out of
> struct pinctrl into struct pinmux_group, which would solve this problem
> in what I consider the correct way. I'll code up a patch to do that and
> stack it before this current patch.
Hm that sounds really elegant, go for it!
Yours,
Linus Walleij