The first 3 patches were previously ack'd but weren't applied due to earlier
patches in the original series not having been applied. These rebased versions
can be applied directly on top of the pinctrl for-next branch.
The last patch is the conceptually ack'd portion of the larger locking rework
patch, extracted on its own for commit.
Stephen Warren (4):
pinctrl: Disallow map table entries with NULL dev_name field
pinctrl: Use dev_*() instead of pr_*(), add some msgs, minor cleanups
pinctrl: Allocate sizeof(*p) instead of sizeof(struct foo)
pinctrl: Remove pin and hogs locks from struct pinctrl_dev
Documentation/pinctrl.txt | 15 ++---
drivers/pinctrl/core.c | 131 +++++++++++++-------------------------
drivers/pinctrl/core.h | 11 ++--
drivers/pinctrl/pinmux.c | 2 +-
include/linux/pinctrl/machine.h | 7 --
5 files changed, 57 insertions(+), 109 deletions(-)
This hopefully makes it harder to take the sizeof the wrong type.
Signed-off-by: Stephen Warren <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Dong Aisheng <[email protected]>
---
v2: Rebased
drivers/pinctrl/core.c | 6 +++---
drivers/pinctrl/pinmux.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 1d59430..2cc8f72 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -502,7 +502,7 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
* mapping, this is what consumers will get when requesting
* a pin control handle with pinctrl_get()
*/
- p = kzalloc(sizeof(struct pinctrl), GFP_KERNEL);
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
if (p == NULL) {
dev_err(dev, "failed to alloc struct pinctrl\n");
return ERR_PTR(-ENOMEM);
@@ -726,7 +726,7 @@ static int pinctrl_hog_map(struct pinctrl_dev *pctldev,
struct pinctrl *p;
int ret;
- hog = kzalloc(sizeof(struct pinctrl_hog), GFP_KERNEL);
+ hog = kzalloc(sizeof(*hog), GFP_KERNEL);
if (!hog) {
dev_err(pctldev->dev, "failed to alloc struct pinctrl_hog\n");
return -ENOMEM;
@@ -1160,7 +1160,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
if (pctldesc->name == NULL)
return NULL;
- pctldev = kzalloc(sizeof(struct pinctrl_dev), GFP_KERNEL);
+ pctldev = kzalloc(sizeof(*pctldev), GFP_KERNEL);
if (pctldev == NULL) {
dev_err(dev, "failed to alloc struct pinctrl_dev\n");
return NULL;
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 2887897..98b89d6 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -491,7 +491,7 @@ static int pinmux_enable_muxmap(struct pinctrl_dev *pctldev,
p->func_selector = func_selector;
/* Now add this group selector, we may have many of them */
- grp = kmalloc(sizeof(struct pinmux_group), GFP_KERNEL);
+ grp = kmalloc(sizeof(*grp), GFP_KERNEL);
if (!grp)
return -ENOMEM;
grp->group_selector = group_selector;
--
1.7.0.4
e.g. dev_err instead of pr_err prints messages in a slightly more
standardized format.
Also, add a few more error messages to track down errors.
Also, some small cleanups of messages.
Signed-off-by: Stephen Warren <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Dong Aisheng <[email protected]>
---
v2: Rebased
drivers/pinctrl/core.c | 33 +++++++++++++++++++++------------
1 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 5411e32..1d59430 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -212,8 +212,10 @@ static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev,
}
pindesc = kzalloc(sizeof(*pindesc), GFP_KERNEL);
- if (pindesc == NULL)
+ if (pindesc == NULL) {
+ dev_err(pctldev->dev, "failed to alloc struct pin_desc\n");
return -ENOMEM;
+ }
spin_lock_init(&pindesc->lock);
@@ -493,7 +495,7 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
devname = dev_name(dev);
- pr_debug("get pin control handle device %s state %s\n", devname, name);
+ dev_dbg(dev, "pinctrl_get() for device %s state %s\n", devname, name);
/*
* create the state cookie holder struct pinctrl for each
@@ -501,8 +503,10 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
* a pin control handle with pinctrl_get()
*/
p = kzalloc(sizeof(struct pinctrl), GFP_KERNEL);
- if (p == NULL)
+ if (p == NULL) {
+ dev_err(dev, "failed to alloc struct pinctrl\n");
return ERR_PTR(-ENOMEM);
+ }
mutex_init(&p->mutex);
pinmux_init_pinctrl_handle(p);
@@ -521,8 +525,8 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
return ERR_PTR(-ENODEV);
}
- pr_debug("in map, found pctldev %s to handle function %s",
- dev_name(pctldev->dev), map->function);
+ dev_dbg(dev, "in map, found pctldev %s to handle function %s",
+ dev_name(pctldev->dev), map->function);
/* Map must be for this device */
if (strcmp(map->dev_name, devname))
@@ -558,8 +562,8 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
if (!num_maps)
dev_info(dev, "zero maps found for mapping %s\n", name);
- pr_debug("found %u mux maps for device %s, UD %s\n",
- num_maps, devname, name ? name : "(undefined)");
+ dev_dbg(dev, "found %u maps for device %s state %s\n",
+ num_maps, devname, name ? name : "(undefined)");
/* Add the pinmux to the global list */
mutex_lock(&pinctrl_list_mutex);
@@ -670,7 +674,7 @@ int pinctrl_register_mappings(struct pinctrl_map const *maps,
for (i = 0; i < num_maps; i++) {
if (!maps[i].name) {
pr_err("failed to register map %d: no map name given\n",
- i);
+ i);
return -EINVAL;
}
@@ -682,13 +686,13 @@ int pinctrl_register_mappings(struct pinctrl_map const *maps,
if (!maps[i].function) {
pr_err("failed to register map %s (%d): no function ID given\n",
- maps[i].name, i);
+ maps[i].name, i);
return -EINVAL;
}
if (!maps[i].dev_name) {
pr_err("failed to register map %s (%d): no device given\n",
- maps[i].name, i);
+ maps[i].name, i);
return -EINVAL;
}
}
@@ -702,6 +706,7 @@ int pinctrl_register_mappings(struct pinctrl_map const *maps,
maps_node->num_maps = num_maps;
maps_node->maps = kmemdup(maps, sizeof(*maps) * num_maps, GFP_KERNEL);
if (!maps_node->maps) {
+ pr_err("failed to duplicate mapping table\n");
kfree(maps_node);
return -ENOMEM;
}
@@ -722,8 +727,10 @@ static int pinctrl_hog_map(struct pinctrl_dev *pctldev,
int ret;
hog = kzalloc(sizeof(struct pinctrl_hog), GFP_KERNEL);
- if (!hog)
+ 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)) {
@@ -1154,8 +1161,10 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
return NULL;
pctldev = kzalloc(sizeof(struct pinctrl_dev), GFP_KERNEL);
- if (pctldev == NULL)
+ if (pctldev == NULL) {
+ dev_err(dev, "failed to alloc struct pinctrl_dev\n");
return NULL;
+ }
/* Initialize pin control device struct */
pctldev->owner = pctldesc->owner;
--
1.7.0.4
Hog entries are mapping table entries with .ctrl_dev_name == .dev_name.
All other mapping table entries need .dev_name set so that they will
match some pinctrl_get() call. All extant PIN_MAP*() macros set
.dev_name.
So, there is no reason to allow mapping table entries without .dev_name
set. Update the code and documentation to disallow this.
Signed-off-by: Stephen Warren <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Dong Aisheng <[email protected]>
---
v2: Rebased
Documentation/pinctrl.txt | 15 +++-----
drivers/pinctrl/core.c | 73 ++++++++++++---------------------------
include/linux/pinctrl/machine.h | 7 ----
3 files changed, 27 insertions(+), 68 deletions(-)
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index ee3266b..fa9163a 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -494,14 +494,10 @@ Definitions:
{"map-i2c0", i2c0, pinctrl0, fi2c0, gi2c0}
}
- Every map must be assigned a symbolic name, pin controller and function.
- The group is not compulsory - if it is omitted the first group presented by
- the driver as applicable for the function will be selected, which is
- useful for simple cases.
-
- The device name is present in map entries tied to specific devices. Maps
- without device names are referred to as SYSTEM pinmuxes, such as can be taken
- by the machine implementation on boot and not tied to any specific device.
+ Every map must be assigned a state name, pin controller, device and
+ function. The group is not compulsory - if it is omitted the first group
+ presented by the driver as applicable for the function will be selected,
+ which is useful for simple cases.
It is possible to map several groups to the same combination of device,
pin controller and function. This is for cases where a certain function on
@@ -983,8 +979,7 @@ after this you should be able to see this in the debugfs listing of all pins.
System pin control hogging
==========================
-A system pin control map entry, i.e. a pin control setting that does not have
-a device associated with it, can be hogged by the core when the pin controller
+Pin control map entries can be hogged by the core when the pin controller
is registered. This means that the core will attempt to call pinctrl_get() and
pinctrl_enable() on it immediately after the pin control device has been
registered.
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index fb3fbb7..5411e32 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -479,24 +479,21 @@ EXPORT_SYMBOL_GPL(pinctrl_gpio_direction_output);
static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
{
struct pinctrl_dev *pctldev = NULL;
- const char *devname = NULL;
+ const char *devname;
struct pinctrl *p;
- bool found_map;
unsigned num_maps = 0;
int ret = -ENODEV;
struct pinctrl_maps *maps_node;
int i;
struct pinctrl_map const *map;
- /* We must have dev or ID or both */
- if (!dev && !name)
+ /* We must have a dev name */
+ if (WARN_ON(!dev))
return ERR_PTR(-EINVAL);
- if (dev)
- devname = dev_name(dev);
+ devname = dev_name(dev);
- pr_debug("get pin control handle %s for device %s\n", name,
- devname ? devname : "(none)");
+ pr_debug("get pin control handle device %s state %s\n", devname, name);
/*
* create the state cookie holder struct pinctrl for each
@@ -511,8 +508,6 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
/* Iterate over the pin control maps to locate the right ones */
for_each_maps(maps_node, i, map) {
- found_map = false;
-
/*
* First, try to find the pctldev given in the map
*/
@@ -529,6 +524,10 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
pr_debug("in map, found pctldev %s to handle function %s",
dev_name(pctldev->dev), map->function);
+ /* Map must be for this device */
+ 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.
@@ -540,30 +539,12 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
continue;
}
- /*
- * This is for the case where no device name is given, we
- * already know that the function name matches from above
- * code.
- */
- if (!map->dev_name && (name != NULL))
- found_map = true;
-
- /* If the mapping has a device set up it must match */
- if (map->dev_name &&
- (!devname || !strcmp(map->dev_name, devname)))
- /* MATCH! */
- found_map = true;
-
- /* If this map is applicable, then apply it */
- if (found_map) {
- ret = pinmux_apply_muxmap(pctldev, p, dev,
- devname, map);
- if (ret) {
- kfree(p);
- return ERR_PTR(ret);
- }
- num_maps++;
+ ret = pinmux_apply_muxmap(pctldev, p, dev, devname, map);
+ if (ret) {
+ kfree(p);
+ return ERR_PTR(ret);
}
+ num_maps++;
}
/*
@@ -578,9 +559,7 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev, const char *name)
dev_info(dev, "zero maps found for mapping %s\n", name);
pr_debug("found %u mux maps for device %s, UD %s\n",
- num_maps,
- devname ? devname : "(anonymous)",
- name ? name : "(undefined)");
+ num_maps, devname, name ? name : "(undefined)");
/* Add the pinmux to the global list */
mutex_lock(&pinctrl_list_mutex);
@@ -707,14 +686,11 @@ int pinctrl_register_mappings(struct pinctrl_map const *maps,
return -EINVAL;
}
- if (!maps[i].dev_name)
- pr_debug("add system map %s function %s with no device\n",
- maps[i].name,
- maps[i].function);
- else
- pr_debug("register map %s, function %s\n",
- maps[i].name,
- maps[i].function);
+ if (!maps[i].dev_name) {
+ pr_err("failed to register map %s (%d): no device given\n",
+ maps[i].name, i);
+ return -EINVAL;
+ }
}
maps_node = kzalloc(sizeof(*maps_node), GFP_KERNEL);
@@ -938,13 +914,8 @@ static int pinctrl_maps_show(struct seq_file *s, void *what)
mutex_lock(&pinctrl_maps_mutex);
for_each_maps(maps_node, i, map) {
seq_printf(s, "%s:\n", map->name);
- if (map->dev_name)
- seq_printf(s, " device: %s\n",
- map->dev_name);
- else
- seq_printf(s, " SYSTEM MUX\n");
- seq_printf(s, " controlling device %s\n",
- map->ctrl_dev_name);
+ seq_printf(s, " device: %s\n", map->dev_name);
+ seq_printf(s, " controlling device %s\n", map->ctrl_dev_name);
seq_printf(s, " function: %s\n", map->function);
seq_printf(s, " group: %s\n", map->group ? map->group :
"(default)");
diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
index af145d5..400f192 100644
--- a/include/linux/pinctrl/machine.h
+++ b/include/linux/pinctrl/machine.h
@@ -46,13 +46,6 @@ struct pinctrl_map {
{ .name = a, .ctrl_dev_name = b, .function = c, .dev_name = d }
/*
- * Convenience macro to map a system function onto a certain pinctrl device.
- * System functions are not assigned to a particular device.
- */
-#define PIN_MAP_SYS(a, b, c) \
- { .name = a, .ctrl_dev_name = b, .function = c }
-
-/*
* 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.
*/
--
1.7.0.4
struct pinctrl_dev's pin_desc_tree_lock and pinctrl_hogs_lock aren't
useful; the data they protect is read-only except when registering or
unregistering a pinctrl_dev, and at those times, it doesn't make sense to
protect one part of the structure independently from the rest.
Move pinctrl_init_device_debugfs() to the end of pinctrl_register() so
that debugfs can't access the struct pinctrl_dev until it's fully
initialized, i.e. after the hogs are set up.
Signed-off-by: Stephen Warren <[email protected]>
---
v2: Extracted from previous lock rework patch.
drivers/pinctrl/core.c | 25 ++-----------------------
drivers/pinctrl/core.h | 11 ++++++-----
2 files changed, 8 insertions(+), 28 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2cc8f72..633b97e 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -115,18 +115,6 @@ struct pinctrl_dev *get_pinctrl_dev_from_devname(const char *devname)
return found ? pctldev : NULL;
}
-struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, unsigned int pin)
-{
- struct pin_desc *pindesc;
- unsigned long flags;
-
- spin_lock_irqsave(&pctldev->pin_desc_tree_lock, flags);
- pindesc = radix_tree_lookup(&pctldev->pin_desc_tree, pin);
- spin_unlock_irqrestore(&pctldev->pin_desc_tree_lock, flags);
-
- return pindesc;
-}
-
/**
* pin_get_from_name() - look up a pin number from a name
* @pctldev: the pin control device to lookup the pin on
@@ -182,7 +170,6 @@ static void pinctrl_free_pindescs(struct pinctrl_dev *pctldev,
{
int i;
- spin_lock(&pctldev->pin_desc_tree_lock);
for (i = 0; i < num_pins; i++) {
struct pin_desc *pindesc;
@@ -196,7 +183,6 @@ static void pinctrl_free_pindescs(struct pinctrl_dev *pctldev,
}
kfree(pindesc);
}
- spin_unlock(&pctldev->pin_desc_tree_lock);
}
static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev,
@@ -232,9 +218,7 @@ static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev,
pindesc->dynamic_name = true;
}
- spin_lock(&pctldev->pin_desc_tree_lock);
radix_tree_insert(&pctldev->pin_desc_tree, number, pindesc);
- spin_unlock(&pctldev->pin_desc_tree_lock);
pr_debug("registered pin %d (%s) on %s\n",
number, pindesc->name, pctldev->desc->name);
return 0;
@@ -756,9 +740,7 @@ static int pinctrl_hog_map(struct pinctrl_dev *pctldev,
dev_info(pctldev->dev, "hogged map %s, function %s\n", map->name,
map->function);
- mutex_lock(&pctldev->pinctrl_hogs_lock);
list_add_tail(&hog->node, &pctldev->pinctrl_hogs);
- mutex_unlock(&pctldev->pinctrl_hogs_lock);
return 0;
}
@@ -781,7 +763,6 @@ static int pinctrl_hog_maps(struct pinctrl_dev *pctldev)
struct pinctrl_map const *map;
INIT_LIST_HEAD(&pctldev->pinctrl_hogs);
- mutex_init(&pctldev->pinctrl_hogs_lock);
mutex_lock(&pinctrl_maps_mutex);
for_each_maps(maps_node, i, map) {
@@ -808,7 +789,6 @@ static void pinctrl_unhog_maps(struct pinctrl_dev *pctldev)
{
struct list_head *node, *tmp;
- mutex_lock(&pctldev->pinctrl_hogs_lock);
list_for_each_safe(node, tmp, &pctldev->pinctrl_hogs) {
struct pinctrl_hog *hog =
list_entry(node, struct pinctrl_hog, node);
@@ -817,7 +797,6 @@ static void pinctrl_unhog_maps(struct pinctrl_dev *pctldev)
list_del(node);
kfree(hog);
}
- mutex_unlock(&pctldev->pinctrl_hogs_lock);
}
#ifdef CONFIG_DEBUG_FS
@@ -1171,7 +1150,6 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
pctldev->desc = pctldesc;
pctldev->driver_data = driver_data;
INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL);
- spin_lock_init(&pctldev->pin_desc_tree_lock);
INIT_LIST_HEAD(&pctldev->gpio_ranges);
mutex_init(&pctldev->gpio_ranges_lock);
pctldev->dev = dev;
@@ -1207,11 +1185,12 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
goto out_err;
}
- pinctrl_init_device_debugfs(pctldev);
mutex_lock(&pinctrldev_list_mutex);
list_add_tail(&pctldev->node, &pinctrldev_list);
mutex_unlock(&pinctrldev_list_mutex);
pinctrl_hog_maps(pctldev);
+ pinctrl_init_device_debugfs(pctldev);
+
return pctldev;
out_err:
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 061c57d..7551611 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -20,7 +20,6 @@ struct pinctrl_gpio_range;
* controller
* @pin_desc_tree: each pin descriptor for this pin controller is stored in
* this radix tree
- * @pin_desc_tree_lock: lock for the descriptor tree
* @gpio_ranges: a list of GPIO ranges that is handled by this pin controller,
* ranges are added to this list at runtime
* @gpio_ranges_lock: lock for the GPIO ranges list
@@ -28,7 +27,6 @@ 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_lock: lock for the pin control hog list
* @pinctrl_hogs: list of pin control maps hogged by this device
* @device_root: debugfs root for this device
*/
@@ -36,13 +34,11 @@ struct pinctrl_dev {
struct list_head node;
struct pinctrl_desc *desc;
struct radix_tree_root pin_desc_tree;
- spinlock_t pin_desc_tree_lock;
struct list_head gpio_ranges;
struct mutex gpio_ranges_lock;
struct device *dev;
struct module *owner;
void *driver_data;
- struct mutex pinctrl_hogs_lock;
struct list_head pinctrl_hogs;
#ifdef CONFIG_DEBUG_FS
struct dentry *device_root;
@@ -99,7 +95,12 @@ struct pin_desc {
};
struct pinctrl_dev *get_pinctrl_dev_from_devname(const char *dev_name);
-struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev, unsigned int pin);
int pin_get_from_name(struct pinctrl_dev *pctldev, const char *name);
int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
const char *pin_group);
+
+static inline struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev,
+ unsigned int pin)
+{
+ return radix_tree_lookup(&pctldev->pin_desc_tree, pin);
+}
--
1.7.0.4
On Wed, Feb 22, 2012 at 10:25 PM, Stephen Warren <[email protected]> wrote:
> The first 3 patches were previously ack'd but weren't applied due to earlier
> patches in the original series not having been applied. These rebased versions
> can be applied directly on top of the pinctrl for-next branch.
>
> The last patch is the conceptually ack'd portion of the larger locking rework
> patch, extracted on its own for commit.
>
> Stephen Warren (4):
> ?pinctrl: Disallow map table entries with NULL dev_name field
> ?pinctrl: Use dev_*() instead of pr_*(), add some msgs, minor cleanups
> ?pinctrl: Allocate sizeof(*p) instead of sizeof(struct foo)
> ?pinctrl: Remove pin and hogs locks from struct pinctrl_dev
Thanks a ton Stephen, all patches applied and pushed to for-next.
Yours,
Linus Walleij