[PATCHv2 0/5] Add generic pinctrl helpers for managing groups and function

[PATCHv2 0/5] Add generic pinctrl helpers for managing groups and function

[Tony Lindgren]

Hi all,

Here are some changes to add generic helpers for managing pinctrl groups and
functions.

Regards,

Tony

Changes since v1:

- Updates to the first patch in the series to use delayed work for pinctrl
  hogs based on what was discussed on the mailing list, mostly to pass
  pctldev to the parsers and add pinctrl_dt_has_hogs() check

Tony Lindgren (5):
  pinctrl: core: Use delayed work for hogs
  pinctrl: core: Add generic pinctrl functions for managing groups
  pinctrl: core: Add generic pinctrl functions for managing groups
  pinctrl: single: Use generic pinctrl helpers for managing groups
  pinctrl: single: Use generic pinmux helpers for managing functions

 drivers/pinctrl/Kconfig          |  11 +-
 drivers/pinctrl/core.c           | 270 +++++++++++++++++++++++++++++++-----
 drivers/pinctrl/core.h           |  67 +++++++++
 drivers/pinctrl/devicetree.c     |  28 +++-
 drivers/pinctrl/devicetree.h     |  12 +-
 drivers/pinctrl/pinctrl-single.c | 290 ++++-----------------------------------
 drivers/pinctrl/pinmux.c         | 173 +++++++++++++++++++++++
 drivers/pinctrl/pinmux.h         |  42 ++++++
 8 files changed, 591 insertions(+), 302 deletions(-)

-- 
2.11.0

[PATCH 1/5] pinctrl: core: Use delayed work for hogs

[Tony Lindgren]

Having the pin control framework call pin controller functions
before it's probe has finished is not nice as the pin controller
device driver does not yet have struct pinctrl_dev handle.

Let's fix this issue by adding deferred work for late init. This is
needed to be able to add pinctrl generic helper functions that expect
to know struct pinctrl_dev handle. Note that we now need to call
create_pinctrl() directly as we don't want to add the pin controller
to the list of controllers until the hogs are claimed. We also need
to pass the pinctrl_dev to the device tree parser functions as they
otherwise won't find the right controller at this point.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/core.c       | 90 ++++++++++++++++++++++++++++----------------
 drivers/pinctrl/core.h       |  2 +
 drivers/pinctrl/devicetree.c | 28 +++++++++++---
 drivers/pinctrl/devicetree.h | 12 +++++-
 4 files changed, 93 insertions(+), 39 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -720,7 +720,8 @@ static struct pinctrl_state *create_state(struct pinctrl *p,
 	return state;
 }
 
-static int add_setting(struct pinctrl *p, struct pinctrl_map const *map)
+static int add_setting(struct pinctrl *p, struct pinctrl_dev *pctldev,
+		       struct pinctrl_map const *map)
 {
 	struct pinctrl_state *state;
 	struct pinctrl_setting *setting;
@@ -744,7 +745,11 @@ static int add_setting(struct pinctrl *p, struct pinctrl_map const *map)
 
 	setting->type = map->type;
 
-	setting->pctldev = get_pinctrl_dev_from_devname(map->ctrl_dev_name);
+	if (pctldev)
+		setting->pctldev = pctldev;
+	else
+		setting->pctldev =
+			get_pinctrl_dev_from_devname(map->ctrl_dev_name);
 	if (setting->pctldev == NULL) {
 		kfree(setting);
 		/* Do not defer probing of hogs (circular loop) */
@@ -800,7 +805,8 @@ static struct pinctrl *find_pinctrl(struct device *dev)
 
 static void pinctrl_free(struct pinctrl *p, bool inlist);
 
-static struct pinctrl *create_pinctrl(struct device *dev)
+static struct pinctrl *create_pinctrl(struct device *dev,
+				      struct pinctrl_dev *pctldev)
 {
 	struct pinctrl *p;
 	const char *devname;
@@ -823,7 +829,7 @@ static struct pinctrl *create_pinctrl(struct device *dev)
 	INIT_LIST_HEAD(&p->states);
 	INIT_LIST_HEAD(&p->dt_maps);
 
-	ret = pinctrl_dt_to_map(p);
+	ret = pinctrl_dt_to_map(p, pctldev);
 	if (ret < 0) {
 		kfree(p);
 		return ERR_PTR(ret);
@@ -838,7 +844,7 @@ static struct pinctrl *create_pinctrl(struct device *dev)
 		if (strcmp(map->dev_name, devname))
 			continue;
 
-		ret = add_setting(p, map);
+		ret = add_setting(p, pctldev, map);
 		/*
 		 * At this point the adding of a setting may:
 		 *
@@ -899,7 +905,7 @@ struct pinctrl *pinctrl_get(struct device *dev)
 		return p;
 	}
 
-	return create_pinctrl(dev);
+	return create_pinctrl(dev, NULL);
 }
 EXPORT_SYMBOL_GPL(pinctrl_get);
 
@@ -1738,6 +1744,46 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev)
 }
 
 /**
+ * pinctrl_late_init() - finish pin controller device registration
+ * @work: work struct
+ */
+static void pinctrl_late_init(struct work_struct *work)
+{
+	struct pinctrl_dev *pctldev;
+
+	pctldev = container_of(work, struct pinctrl_dev, late_init.work);
+
+	pctldev->p = create_pinctrl(pctldev->dev, pctldev);
+	if (!IS_ERR(pctldev->p)) {
+		kref_get(&pctldev->p->users);
+		pctldev->hog_default =
+			pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(pctldev->hog_default)) {
+			dev_dbg(pctldev->dev,
+				"failed to lookup the default state\n");
+		} else {
+			if (pinctrl_select_state(pctldev->p,
+						 pctldev->hog_default))
+				dev_err(pctldev->dev,
+					"failed to select default state\n");
+		}
+
+		pctldev->hog_sleep =
+			pinctrl_lookup_state(pctldev->p,
+					     PINCTRL_STATE_SLEEP);
+		if (IS_ERR(pctldev->hog_sleep))
+			dev_dbg(pctldev->dev,
+				"failed to lookup the sleep state\n");
+	}
+
+	mutex_lock(&pinctrldev_list_mutex);
+	list_add_tail(&pctldev->node, &pinctrldev_list);
+	mutex_unlock(&pinctrldev_list_mutex);
+
+	pinctrl_init_device_debugfs(pctldev);
+}
+
+/**
  * pinctrl_register() - register a pin controller device
  * @pctldesc: descriptor for this pin controller
  * @dev: parent device for this pin controller
@@ -1766,6 +1812,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	pctldev->driver_data = driver_data;
 	INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL);
 	INIT_LIST_HEAD(&pctldev->gpio_ranges);
+	INIT_DELAYED_WORK(&pctldev->late_init, pinctrl_late_init);
 	pctldev->dev = dev;
 	mutex_init(&pctldev->mutex);
 
@@ -1800,32 +1847,10 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 		goto out_err;
 	}
 
-	mutex_lock(&pinctrldev_list_mutex);
-	list_add_tail(&pctldev->node, &pinctrldev_list);
-	mutex_unlock(&pinctrldev_list_mutex);
-
-	pctldev->p = pinctrl_get(pctldev->dev);
-
-	if (!IS_ERR(pctldev->p)) {
-		pctldev->hog_default =
-			pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
-		if (IS_ERR(pctldev->hog_default)) {
-			dev_dbg(dev, "failed to lookup the default state\n");
-		} else {
-			if (pinctrl_select_state(pctldev->p,
-						pctldev->hog_default))
-				dev_err(dev,
-					"failed to select default state\n");
-		}
-
-		pctldev->hog_sleep =
-			pinctrl_lookup_state(pctldev->p,
-						    PINCTRL_STATE_SLEEP);
-		if (IS_ERR(pctldev->hog_sleep))
-			dev_dbg(dev, "failed to lookup the sleep state\n");
-	}
-
-	pinctrl_init_device_debugfs(pctldev);
+	if (pinctrl_dt_has_hogs(pctldev))
+		schedule_delayed_work(&pctldev->late_init, 0);
+	else
+		pinctrl_late_init(&pctldev->late_init.work);
 
 	return pctldev;
 
@@ -1848,6 +1873,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
 	if (pctldev == NULL)
 		return;
 
+	cancel_delayed_work_sync(&pctldev->late_init);
 	mutex_lock(&pctldev->mutex);
 	pinctrl_remove_device_debugfs(pctldev);
 	mutex_unlock(&pctldev->mutex);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -33,6 +33,7 @@ struct pinctrl_gpio_range;
  * @p: result of pinctrl_get() for this device
  * @hog_default: default state for pins hogged by this device
  * @hog_sleep: sleep state for pins hogged by this device
+ * @late_init: delayed work for pin controller to finish registration
  * @mutex: mutex taken on each pin controller specific action
  * @device_root: debugfs root for this device
  */
@@ -47,6 +48,7 @@ struct pinctrl_dev {
 	struct pinctrl *p;
 	struct pinctrl_state *hog_default;
 	struct pinctrl_state *hog_sleep;
+	struct delayed_work late_init;
 	struct mutex mutex;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *device_root;
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -100,11 +100,12 @@ struct pinctrl_dev *of_pinctrl_get(struct device_node *np)
 	return get_pinctrl_dev_from_of_node(np);
 }
 
-static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
+static int dt_to_map_one_config(struct pinctrl *p,
+				struct pinctrl_dev *pctldev,
+				const char *statename,
 				struct device_node *np_config)
 {
 	struct device_node *np_pctldev;
-	struct pinctrl_dev *pctldev;
 	const struct pinctrl_ops *ops;
 	int ret;
 	struct pinctrl_map *map;
@@ -121,7 +122,8 @@ static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
 			/* OK let's just assume this will appear later then */
 			return -EPROBE_DEFER;
 		}
-		pctldev = get_pinctrl_dev_from_of_node(np_pctldev);
+		if (!pctldev)
+			pctldev = get_pinctrl_dev_from_of_node(np_pctldev);
 		if (pctldev)
 			break;
 		/* Do not defer probing of hogs (circular loop) */
@@ -166,7 +168,22 @@ static int dt_remember_dummy_state(struct pinctrl *p, const char *statename)
 	return dt_remember_or_free_map(p, statename, NULL, map, 1);
 }
 
-int pinctrl_dt_to_map(struct pinctrl *p)
+bool pinctrl_dt_has_hogs(struct pinctrl_dev *pctldev)
+{
+	struct device_node *np;
+	struct property *prop;
+	int size;
+
+	np = pctldev->dev->of_node;
+	if (!np)
+		return false;
+
+	prop = of_find_property(np, "pinctrl-0", &size);
+
+	return prop ? true : false;
+}
+
+int pinctrl_dt_to_map(struct pinctrl *p, struct pinctrl_dev *pctldev)
 {
 	struct device_node *np = p->dev->of_node;
 	int state, ret;
@@ -233,7 +250,8 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 			}
 
 			/* Parse the node */
-			ret = dt_to_map_one_config(p, statename, np_config);
+			ret = dt_to_map_one_config(p, pctldev, statename,
+						   np_config);
 			of_node_put(np_config);
 			if (ret < 0)
 				goto err;
diff --git a/drivers/pinctrl/devicetree.h b/drivers/pinctrl/devicetree.h
--- a/drivers/pinctrl/devicetree.h
+++ b/drivers/pinctrl/devicetree.h
@@ -20,8 +20,10 @@ struct of_phandle_args;
 
 #ifdef CONFIG_OF
 
+bool pinctrl_dt_has_hogs(struct pinctrl_dev *pctldev);
+
 void pinctrl_dt_free_maps(struct pinctrl *p);
-int pinctrl_dt_to_map(struct pinctrl *p);
+int pinctrl_dt_to_map(struct pinctrl *p, struct pinctrl_dev *pctldev);
 
 int pinctrl_count_index_with_args(const struct device_node *np,
 				  const char *list_name);
@@ -32,7 +34,13 @@ int pinctrl_parse_index_with_args(const struct device_node *np,
 
 #else
 
-static inline int pinctrl_dt_to_map(struct pinctrl *p)
+static inline bool pinctrl_dt_has_hogs(struct pinctrl_dev *pctldev)
+{
+	return false;
+}
+
+static inline int pinctrl_dt_to_map(struct pinctrl *p,
+				    struct pinctrl_dev *pctldev)
 {
 	return 0;
 }
-- 
2.11.0

[PATCH 2/5] pinctrl: core: Add generic pinctrl functions for managing groups

[Tony Lindgren]

We can add generic helpers for pin group handling for cases where the pin
controller driver does not need to use static arrays.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/Kconfig |   3 +
 drivers/pinctrl/core.c  | 178 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/core.h  |  47 +++++++++++++
 3 files changed, 228 insertions(+)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -8,6 +8,9 @@ config PINCTRL
 menu "Pin controllers"
 	depends on PINCTRL
 
+config GENERIC_PINCTRL
+	bool
+
 config PINMUX
 	bool "Support pin multiplexing controllers" if COMPILE_TEST
 
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -540,6 +540,182 @@ void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 }
 EXPORT_SYMBOL_GPL(pinctrl_remove_gpio_range);
 
+#ifdef CONFIG_GENERIC_PINCTRL
+
+/**
+ * pinctrl_generic_get_group_count() - returns the number of pin groups
+ * @pctldev: pin controller device
+ */
+int pinctrl_generic_get_group_count(struct pinctrl_dev *pctldev)
+{
+	return pctldev->num_groups;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_get_group_count);
+
+/**
+ * pinctrl_generic_get_group_name() - returns the name of a pin group
+ * @pctldev: pin controller device
+ * @selector: group number
+ */
+const char *pinctrl_generic_get_group_name(struct pinctrl_dev *pctldev,
+					   unsigned int selector)
+{
+	struct group_desc *group;
+
+	group = radix_tree_lookup(&pctldev->pin_group_tree,
+				  selector);
+	if (!group)
+		return NULL;
+
+	return group->name;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_get_group_name);
+
+/**
+ * pinctrl_generic_get_group_pins() - gets the pin group pins
+ * @pctldev: pin controller device
+ * @selector: group number
+ * @pins: pins in the group
+ * @num_pins: number of pins in the group
+ */
+int pinctrl_generic_get_group_pins(struct pinctrl_dev *pctldev,
+				   unsigned int selector,
+				   const unsigned int **pins,
+				   unsigned int *num_pins)
+{
+	struct group_desc *group;
+
+	group = radix_tree_lookup(&pctldev->pin_group_tree,
+				  selector);
+	if (!group) {
+		dev_err(pctldev->dev, "%s could not find pingroup%i\n",
+			__func__, selector);
+		return -EINVAL;
+	}
+
+	*pins = group->pins;
+	*num_pins = group->num_pins;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_get_group_pins);
+
+/**
+ * pinctrl_generic_get_group() - returns a pin group based on the number
+ * @pctldev: pin controller device
+ * @gselector: group number
+ */
+struct group_desc *pinctrl_generic_get_group(struct pinctrl_dev *pctldev,
+					     unsigned int selector)
+{
+	struct group_desc *group;
+
+	group = radix_tree_lookup(&pctldev->pin_group_tree,
+				  selector);
+	if (!group)
+		return NULL;
+
+	return group;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_get_group);
+
+/**
+ * pinctrl_generic_add_group() - adds a new pin group
+ * @pctldev: pin controller device
+ * @name: name of the pin group
+ * @pins: pins in the pin group
+ * @num_pins: number of pins in the pin group
+ * @data: pin controller driver specific data
+ *
+ * Note that the caller must take care of locking.
+ */
+int pinctrl_generic_add_group(struct pinctrl_dev *pctldev, const char *name,
+			      int *pins, int num_pins, void *data)
+{
+	struct group_desc *group;
+
+	group = devm_kzalloc(pctldev->dev, sizeof(*group), GFP_KERNEL);
+	if (!group)
+		return -ENOMEM;
+
+	group->name = name;
+	group->pins = pins;
+	group->num_pins = num_pins;
+	group->data = data;
+
+	radix_tree_insert(&pctldev->pin_group_tree, pctldev->num_groups,
+			  group);
+
+	pctldev->num_groups++;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_add_group);
+
+/**
+ * pinctrl_generic_remove_group() - removes a numbered pin group
+ * @pctldev: pin controller device
+ * @selector: group number
+ *
+ * Note that the caller must take care of locking.
+ */
+int pinctrl_generic_remove_group(struct pinctrl_dev *pctldev,
+				 unsigned int selector)
+{
+	struct group_desc *group;
+
+	group = radix_tree_lookup(&pctldev->pin_group_tree,
+				  selector);
+	if (!group)
+		return -ENOENT;
+
+	radix_tree_delete(&pctldev->pin_group_tree, selector);
+	devm_kfree(pctldev->dev, group);
+
+	pctldev->num_groups--;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_remove_group);
+
+/**
+ * pinctrl_generic_free_groups() - removes all pin groups
+ * @pctldev: pin controller device
+ *
+ * Note that the caller must take care of locking.
+ */
+static void pinctrl_generic_free_groups(struct pinctrl_dev *pctldev)
+{
+	struct radix_tree_iter iter;
+	struct group_desc *group;
+	unsigned long *indices;
+	void **slot;
+	int i = 0;
+
+	indices = devm_kzalloc(pctldev->dev, sizeof(*indices) *
+			       pctldev->num_groups, GFP_KERNEL);
+	if (!indices)
+		return;
+
+	radix_tree_for_each_slot(slot, &pctldev->pin_group_tree, &iter, 0)
+		indices[i++] = iter.index;
+
+	for (i = 0; i < pctldev->num_groups; i++) {
+		group = radix_tree_lookup(&pctldev->pin_group_tree,
+					  indices[i]);
+		radix_tree_delete(&pctldev->pin_group_tree, indices[i]);
+		devm_kfree(pctldev->dev, group);
+	}
+
+	pctldev->num_groups = 0;
+}
+
+#else
+static inline void pinctrl_generic_free_groups(struct pinctrl_dev *pctldev)
+{
+}
+#endif /* CONFIG_GENERIC_PINCTRL */
+
 /**
  * pinctrl_get_group_selector() - returns the group selector for a group
  * @pctldev: the pin controller handling the group
@@ -1811,6 +1987,7 @@ 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);
+	INIT_RADIX_TREE(&pctldev->pin_group_tree, GFP_KERNEL);
 	INIT_LIST_HEAD(&pctldev->gpio_ranges);
 	INIT_DELAYED_WORK(&pctldev->late_init, pinctrl_late_init);
 	pctldev->dev = dev;
@@ -1885,6 +2062,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
 	mutex_lock(&pctldev->mutex);
 	/* TODO: check that no pinmuxes are still active? */
 	list_del(&pctldev->node);
+	pinctrl_generic_free_groups(pctldev);
 	/* Destroy descriptor tree */
 	pinctrl_free_pindescs(pctldev, pctldev->desc->pins,
 			      pctldev->desc->npins);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -24,6 +24,8 @@ struct pinctrl_gpio_range;
  *	controller
  * @pin_desc_tree: each pin descriptor for this pin controller is stored in
  *	this radix tree
+ * @pin_group_tree: optionally each pin group can be stored in this radix tree
+ * @num_groups: optionally number of groups can be kept here
  * @gpio_ranges: a list of GPIO ranges that is handled by this pin controller,
  *	ranges are added to this list at runtime
  * @dev: the device entry for this pin controller
@@ -41,6 +43,8 @@ struct pinctrl_dev {
 	struct list_head node;
 	struct pinctrl_desc *desc;
 	struct radix_tree_root pin_desc_tree;
+	struct radix_tree_root pin_group_tree;
+	unsigned int num_groups;
 	struct list_head gpio_ranges;
 	struct device *dev;
 	struct module *owner;
@@ -162,6 +166,20 @@ struct pin_desc {
 };
 
 /**
+ * struct group_desc - generic pin group descriptor
+ * @name: name of the pin group
+ * @pins: array of pins that belong to the group
+ * @num_pins: number of pins in the group
+ * @data: pin controller driver specific data
+ */
+struct group_desc {
+	const char *name;
+	int *pins;
+	int num_pins;
+	void *data;
+};
+
+/**
  * struct pinctrl_maps - a list item containing part of the mapping table
  * @node: mapping table list node
  * @maps: array of mapping table entries
@@ -173,6 +191,35 @@ struct pinctrl_maps {
 	unsigned num_maps;
 };
 
+#ifdef CONFIG_GENERIC_PINCTRL
+
+int pinctrl_generic_get_group_count(struct pinctrl_dev *pctldev);
+
+const char *pinctrl_generic_get_group_name(struct pinctrl_dev *pctldev,
+					   unsigned int group_selector);
+
+int pinctrl_generic_get_group_pins(struct pinctrl_dev *pctldev,
+				   unsigned int group_selector,
+				   const unsigned int **pins,
+				   unsigned int *npins);
+
+struct group_desc *pinctrl_generic_get_group(struct pinctrl_dev *pctldev,
+					     unsigned int group_selector);
+
+int pinctrl_generic_add_group(struct pinctrl_dev *pctldev, const char *name,
+			      int *gpins, int ngpins, void *data);
+
+int pinctrl_generic_remove_group(struct pinctrl_dev *pctldev,
+				 unsigned int group_selector);
+
+static inline int
+pinctrl_generic_remove_last_group(struct pinctrl_dev *pctldev)
+{
+	return pinctrl_generic_remove_group(pctldev, pctldev->num_groups - 1);
+}
+
+#endif	/* CONFIG_GENERIC_PINCTRL */
+
 struct pinctrl_dev *get_pinctrl_dev_from_devname(const char *dev_name);
 struct pinctrl_dev *get_pinctrl_dev_from_of_node(struct device_node *np);
 int pin_get_from_name(struct pinctrl_dev *pctldev, const char *name);
-- 
2.11.0

[PATCH 3/5] pinctrl: core: Add generic pinctrl functions for managing groups

[Tony Lindgren]

We can add generic helpers for function handling for cases where the pin
controller driver does not need to use static arrays.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/Kconfig  |   4 ++
 drivers/pinctrl/core.c   |   2 +
 drivers/pinctrl/core.h   |  18 +++++
 drivers/pinctrl/pinmux.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinmux.h |  42 ++++++++++++
 5 files changed, 239 insertions(+)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -14,6 +14,10 @@ config GENERIC_PINCTRL
 config PINMUX
 	bool "Support pin multiplexing controllers" if COMPILE_TEST
 
+config GENERIC_PINMUX
+	bool
+	select PINMUX
+
 config PINCONF
 	bool "Support pin configuration controllers" if COMPILE_TEST
 
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1988,6 +1988,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	pctldev->driver_data = driver_data;
 	INIT_RADIX_TREE(&pctldev->pin_desc_tree, GFP_KERNEL);
 	INIT_RADIX_TREE(&pctldev->pin_group_tree, GFP_KERNEL);
+	INIT_RADIX_TREE(&pctldev->pin_function_tree, GFP_KERNEL);
 	INIT_LIST_HEAD(&pctldev->gpio_ranges);
 	INIT_DELAYED_WORK(&pctldev->late_init, pinctrl_late_init);
 	pctldev->dev = dev;
@@ -2062,6 +2063,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
 	mutex_lock(&pctldev->mutex);
 	/* TODO: check that no pinmuxes are still active? */
 	list_del(&pctldev->node);
+	pinmux_generic_free_functions(pctldev);
 	pinctrl_generic_free_groups(pctldev);
 	/* Destroy descriptor tree */
 	pinctrl_free_pindescs(pctldev, pctldev->desc->pins,
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -25,7 +25,9 @@ struct pinctrl_gpio_range;
  * @pin_desc_tree: each pin descriptor for this pin controller is stored in
  *	this radix tree
  * @pin_group_tree: optionally each pin group can be stored in this radix tree
+ * @pin_function_tree: optionally each function can be stored in this radix tree
  * @num_groups: optionally number of groups can be kept here
+ * @num_functions: optionally number of functions can be kept here
  * @gpio_ranges: a list of GPIO ranges that is handled by this pin controller,
  *	ranges are added to this list at runtime
  * @dev: the device entry for this pin controller
@@ -44,7 +46,9 @@ struct pinctrl_dev {
 	struct pinctrl_desc *desc;
 	struct radix_tree_root pin_desc_tree;
 	struct radix_tree_root pin_group_tree;
+	struct radix_tree_root pin_function_tree;
 	unsigned int num_groups;
+	unsigned int num_functions;
 	struct list_head gpio_ranges;
 	struct device *dev;
 	struct module *owner;
@@ -180,6 +184,20 @@ struct group_desc {
 };
 
 /**
+ * struct function_desc - generic function descriptor
+ * @name: name of the function
+ * @group_names: array of pin group names
+ * @num_group_names: number of pin group names
+ * @data: pin controller driver specific data
+ */
+struct function_desc {
+	const char *name;
+	const char **group_names;
+	int num_group_names;
+	void *data;
+};
+
+/**
  * struct pinctrl_maps - a list item containing part of the mapping table
  * @node: mapping table list node
  * @maps: array of mapping table entries
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -695,3 +695,176 @@ void pinmux_init_device_debugfs(struct dentry *devroot,
 }
 
 #endif /* CONFIG_DEBUG_FS */
+
+#ifdef CONFIG_GENERIC_PINMUX
+
+/**
+ * pinmux_generic_get_function_count() - returns number of functions
+ * @pctldev: pin controller device
+ */
+int pinmux_generic_get_function_count(struct pinctrl_dev *pctldev)
+{
+	return pctldev->num_functions;
+}
+EXPORT_SYMBOL_GPL(pinmux_generic_get_function_count);
+
+/**
+ * pinmux_generic_get_function_name() - returns the function name
+ * @pctldev: pin controller device
+ * @selector: function number
+ */
+const char *
+pinmux_generic_get_function_name(struct pinctrl_dev *pctldev,
+				 unsigned int selector)
+{
+	struct function_desc *function;
+
+	function = radix_tree_lookup(&pctldev->pin_function_tree,
+				     selector);
+	if (!function)
+		return NULL;
+
+	return function->name;
+}
+EXPORT_SYMBOL_GPL(pinmux_generic_get_function_name);
+
+/**
+ * pinmux_generic_get_function_groups() - gets the function groups
+ * @pctldev: pin controller device
+ * @selector: function number
+ * @groups: array of pin groups
+ * @num_groups: number of pin groups
+ */
+int pinmux_generic_get_function_groups(struct pinctrl_dev *pctldev,
+				       unsigned int selector,
+				       const char * const **groups,
+				       unsigned * const num_groups)
+{
+	struct function_desc *function;
+
+	function = radix_tree_lookup(&pctldev->pin_function_tree,
+				     selector);
+	if (!function) {
+		dev_err(pctldev->dev, "%s could not find function%i\n",
+			__func__, selector);
+		return -EINVAL;
+	}
+	*groups = function->group_names;
+	*num_groups = function->num_group_names;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinmux_generic_get_function_groups);
+
+/**
+ * pinmux_generic_get_function() - returns a function based on the number
+ * @pctldev: pin controller device
+ * @group_selector: function number
+ */
+struct function_desc *pinmux_generic_get_function(struct pinctrl_dev *pctldev,
+						  unsigned int selector)
+{
+	struct function_desc *function;
+
+	function = radix_tree_lookup(&pctldev->pin_function_tree,
+				     selector);
+	if (!function)
+		return NULL;
+
+	return function;
+}
+EXPORT_SYMBOL_GPL(pinmux_generic_get_function);
+
+/**
+ * pinmux_generic_get_function_groups() - gets the function groups
+ * @pctldev: pin controller device
+ * @name: name of the function
+ * @groups: array of pin groups
+ * @num_groups: number of pin groups
+ * @data: pin controller driver specific data
+ */
+int pinmux_generic_add_function(struct pinctrl_dev *pctldev,
+				const char *name,
+				const char **groups,
+				const unsigned int num_groups,
+				void *data)
+{
+	struct function_desc *function;
+
+	function = devm_kzalloc(pctldev->dev, sizeof(*function), GFP_KERNEL);
+	if (!function)
+		return -ENOMEM;
+
+	function->name = name;
+	function->group_names = groups;
+	function->num_group_names = num_groups;
+	function->data = data;
+
+	radix_tree_insert(&pctldev->pin_function_tree, pctldev->num_functions,
+			  function);
+
+	pctldev->num_functions++;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinmux_generic_add_function);
+
+/**
+ * pinmux_generic_remove_function() - removes a numbered function
+ * @pctldev: pin controller device
+ * @selector: function number
+ *
+ * Note that the caller must take care of locking.
+ */
+int pinmux_generic_remove_function(struct pinctrl_dev *pctldev,
+				   unsigned int selector)
+{
+	struct function_desc *function;
+
+	function = radix_tree_lookup(&pctldev->pin_function_tree,
+				     selector);
+	if (!function)
+		return -ENOENT;
+
+	radix_tree_delete(&pctldev->pin_function_tree, selector);
+	devm_kfree(pctldev->dev, function);
+
+	pctldev->num_functions--;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pinmux_generic_remove_function);
+
+/**
+ * pinmux_generic_free_functions() - removes all functions
+ * @pctldev: pin controller device
+ *
+ * Note that the caller must take care of locking.
+ */
+void pinmux_generic_free_functions(struct pinctrl_dev *pctldev)
+{
+	struct radix_tree_iter iter;
+	struct function_desc *function;
+	unsigned long *indices;
+	void **slot;
+	int i = 0;
+
+	indices = devm_kzalloc(pctldev->dev, sizeof(*indices) *
+			       pctldev->num_functions, GFP_KERNEL);
+	if (!indices)
+		return;
+
+	radix_tree_for_each_slot(slot, &pctldev->pin_function_tree, &iter, 0)
+		indices[i++] = iter.index;
+
+	for (i = 0; i < pctldev->num_functions; i++) {
+		function = radix_tree_lookup(&pctldev->pin_function_tree,
+					     indices[i]);
+		radix_tree_delete(&pctldev->pin_function_tree, indices[i]);
+		devm_kfree(pctldev->dev, function);
+	}
+
+	pctldev->num_functions = 0;
+}
+
+#endif /* CONFIG_GENERIC_PINMUX */
diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -111,3 +111,45 @@ static inline void pinmux_init_device_debugfs(struct dentry *devroot,
 }
 
 #endif
+
+#ifdef CONFIG_GENERIC_PINMUX
+
+int pinmux_generic_get_function_count(struct pinctrl_dev *pctldev);
+
+const char *
+pinmux_generic_get_function_name(struct pinctrl_dev *pctldev,
+				 unsigned int selector);
+
+int pinmux_generic_get_function_groups(struct pinctrl_dev *pctldev,
+				       unsigned int selector,
+				       const char * const **groups,
+				       unsigned * const num_groups);
+
+struct function_desc *pinmux_generic_get_function(struct pinctrl_dev *pctldev,
+						  unsigned int selector);
+
+int pinmux_generic_add_function(struct pinctrl_dev *pctldev,
+				const char *name,
+				const char **groups,
+				unsigned const num_groups,
+				void *data);
+
+int pinmux_generic_remove_function(struct pinctrl_dev *pctldev,
+				   unsigned int selector);
+
+static inline int
+pinmux_generic_remove_last_function(struct pinctrl_dev *pctldev)
+{
+	return pinmux_generic_remove_function(pctldev,
+					      pctldev->num_functions - 1);
+}
+
+void pinmux_generic_free_functions(struct pinctrl_dev *pctldev);
+
+#else
+
+static inline void pinmux_generic_free_functions(struct pinctrl_dev *pctldev)
+{
+}
+
+#endif
-- 
2.11.0

[PATCH 4/5] pinctrl: single: Use generic pinctrl helpers for managing groups

[Tony Lindgren]

We can now drop the driver specific code for managing groups.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/Kconfig          |   2 +-
 drivers/pinctrl/pinctrl-single.c | 156 +++------------------------------------
 2 files changed, 12 insertions(+), 146 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -166,8 +166,8 @@ config PINCTRL_ROCKCHIP
 config PINCTRL_SINGLE
 	tristate "One-register-per-pin type device tree based pinctrl driver"
 	depends on OF
+	select GENERIC_PINCTRL
 	select PINMUX
-	select PINCONF
 	select GENERIC_PINCONF
 	help
 	  This selects the device tree based generic pinctrl driver.
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -38,22 +38,6 @@
 #define PCS_OFF_DISABLED		~0U
 
 /**
- * struct pcs_pingroup - pingroups for a function
- * @np:		pingroup device node pointer
- * @name:	pingroup name
- * @gpins:	array of the pins in the group
- * @ngpins:	number of pins in the group
- * @node:	list node
- */
-struct pcs_pingroup {
-	struct device_node *np;
-	const char *name;
-	int *gpins;
-	int ngpins;
-	struct list_head node;
-};
-
-/**
  * struct pcs_func_vals - mux function register offset and value pair
  * @reg:	register virtual address
  * @val:	register value
@@ -176,15 +160,12 @@ struct pcs_soc_data {
  * @bits_per_mux: number of bits per mux
  * @bits_per_pin: number of bits per pin
  * @pins:	physical pins on the SoC
- * @pgtree:	pingroup index radix tree
  * @ftree:	function index radix tree
- * @pingroups:	list of pingroups
  * @functions:	list of functions
  * @gpiofuncs:	list of gpio functions
  * @irqs:	list of interrupt registers
  * @chip:	chip container for this instance
  * @domain:	IRQ domain for this instance
- * @ngroups:	number of pingroups
  * @nfuncs:	number of functions
  * @desc:	pin controller descriptor
  * @read:	register read function to use
@@ -213,15 +194,12 @@ struct pcs_device {
 	bool bits_per_mux;
 	unsigned bits_per_pin;
 	struct pcs_data pins;
-	struct radix_tree_root pgtree;
 	struct radix_tree_root ftree;
-	struct list_head pingroups;
 	struct list_head functions;
 	struct list_head gpiofuncs;
 	struct list_head irqs;
 	struct irq_chip chip;
 	struct irq_domain *domain;
-	unsigned ngroups;
 	unsigned nfuncs;
 	struct pinctrl_desc desc;
 	unsigned (*read)(void __iomem *reg);
@@ -288,54 +266,6 @@ static void __maybe_unused pcs_writel(unsigned val, void __iomem *reg)
 	writel(val, reg);
 }
 
-static int pcs_get_groups_count(struct pinctrl_dev *pctldev)
-{
-	struct pcs_device *pcs;
-
-	pcs = pinctrl_dev_get_drvdata(pctldev);
-
-	return pcs->ngroups;
-}
-
-static const char *pcs_get_group_name(struct pinctrl_dev *pctldev,
-					unsigned gselector)
-{
-	struct pcs_device *pcs;
-	struct pcs_pingroup *group;
-
-	pcs = pinctrl_dev_get_drvdata(pctldev);
-	group = radix_tree_lookup(&pcs->pgtree, gselector);
-	if (!group) {
-		dev_err(pcs->dev, "%s could not find pingroup%i\n",
-			__func__, gselector);
-		return NULL;
-	}
-
-	return group->name;
-}
-
-static int pcs_get_group_pins(struct pinctrl_dev *pctldev,
-					unsigned gselector,
-					const unsigned **pins,
-					unsigned *npins)
-{
-	struct pcs_device *pcs;
-	struct pcs_pingroup *group;
-
-	pcs = pinctrl_dev_get_drvdata(pctldev);
-	group = radix_tree_lookup(&pcs->pgtree, gselector);
-	if (!group) {
-		dev_err(pcs->dev, "%s could not find pingroup%i\n",
-			__func__, gselector);
-		return -EINVAL;
-	}
-
-	*pins = group->gpins;
-	*npins = group->ngpins;
-
-	return 0;
-}
-
 static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
 					struct seq_file *s,
 					unsigned pin)
@@ -369,9 +299,9 @@ static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
 				struct pinctrl_map **map, unsigned *num_maps);
 
 static const struct pinctrl_ops pcs_pinctrl_ops = {
-	.get_groups_count = pcs_get_groups_count,
-	.get_group_name = pcs_get_group_name,
-	.get_group_pins = pcs_get_group_pins,
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name = pinctrl_generic_get_group_name,
+	.get_group_pins = pinctrl_generic_get_group_pins,
 	.pin_dbg_show = pcs_pin_dbg_show,
 	.dt_node_to_map = pcs_dt_node_to_map,
 	.dt_free_map = pcs_dt_free_map,
@@ -685,7 +615,7 @@ static int pcs_pinconf_group_get(struct pinctrl_dev *pctldev,
 	unsigned npins, old = 0;
 	int i, ret;
 
-	ret = pcs_get_group_pins(pctldev, group, &pins, &npins);
+	ret = pinctrl_generic_get_group_pins(pctldev, group, &pins, &npins);
 	if (ret)
 		return ret;
 	for (i = 0; i < npins; i++) {
@@ -707,7 +637,7 @@ static int pcs_pinconf_group_set(struct pinctrl_dev *pctldev,
 	unsigned npins;
 	int i, ret;
 
-	ret = pcs_get_group_pins(pctldev, group, &pins, &npins);
+	ret = pinctrl_generic_get_group_pins(pctldev, group, &pins, &npins);
 	if (ret)
 		return ret;
 	for (i = 0; i < npins; i++) {
@@ -897,40 +827,6 @@ static void pcs_remove_function(struct pcs_device *pcs,
 }
 
 /**
- * pcs_add_pingroup() - add a pingroup to the pingroup list
- * @pcs: pcs driver instance
- * @np: device node of the mux entry
- * @name: name of the pingroup
- * @gpins: array of the pins that belong to the group
- * @ngpins: number of pins in the group
- */
-static int pcs_add_pingroup(struct pcs_device *pcs,
-					struct device_node *np,
-					const char *name,
-					int *gpins,
-					int ngpins)
-{
-	struct pcs_pingroup *pingroup;
-
-	pingroup = devm_kzalloc(pcs->dev, sizeof(*pingroup), GFP_KERNEL);
-	if (!pingroup)
-		return -ENOMEM;
-
-	pingroup->name = name;
-	pingroup->np = np;
-	pingroup->gpins = gpins;
-	pingroup->ngpins = ngpins;
-
-	mutex_lock(&pcs->mutex);
-	list_add_tail(&pingroup->node, &pcs->pingroups);
-	radix_tree_insert(&pcs->pgtree, pcs->ngroups, pingroup);
-	pcs->ngroups++;
-	mutex_unlock(&pcs->mutex);
-
-	return 0;
-}
-
-/**
  * pcs_get_pin_by_offset() - get a pin index based on the register offset
  * @pcs: pcs driver instance
  * @offset: register offset from the base
@@ -1100,10 +996,9 @@ static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
 	return 0;
 }
 
-static void pcs_free_pingroups(struct pcs_device *pcs);
-
 /**
  * smux_parse_one_pinctrl_entry() - parses a device tree mux entry
+ * @pctldev: pin controller device
  * @pcs: pinctrl driver instance
  * @np: device node of the mux entry
  * @map: map entry
@@ -1186,7 +1081,7 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 		goto free_pins;
 	}
 
-	res = pcs_add_pingroup(pcs, np, np->name, pins, found);
+	res = pinctrl_generic_add_group(pcs->pctl, np->name, pins, found, pcs);
 	if (res < 0)
 		goto free_function;
 
@@ -1205,7 +1100,7 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 	return 0;
 
 free_pingroups:
-	pcs_free_pingroups(pcs);
+	pinctrl_generic_remove_last_group(pcs->pctl);
 	*num_maps = 1;
 free_function:
 	pcs_remove_function(pcs, function);
@@ -1320,7 +1215,7 @@ static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs,
 		goto free_pins;
 	}
 
-	res = pcs_add_pingroup(pcs, np, np->name, pins, found);
+	res = pinctrl_generic_add_group(pcs->pctl, np->name, pins, found, pcs);
 	if (res < 0)
 		goto free_function;
 
@@ -1337,7 +1232,7 @@ static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs,
 	return 0;
 
 free_pingroups:
-	pcs_free_pingroups(pcs);
+	pinctrl_generic_remove_last_group(pcs->pctl);
 	*num_maps = 1;
 free_function:
 	pcs_remove_function(pcs, function);
@@ -1436,33 +1331,6 @@ static void pcs_free_funcs(struct pcs_device *pcs)
 }
 
 /**
- * pcs_free_pingroups() - free memory used by pingroups
- * @pcs: pcs driver instance
- */
-static void pcs_free_pingroups(struct pcs_device *pcs)
-{
-	struct list_head *pos, *tmp;
-	int i;
-
-	mutex_lock(&pcs->mutex);
-	for (i = 0; i < pcs->ngroups; i++) {
-		struct pcs_pingroup *pingroup;
-
-		pingroup = radix_tree_lookup(&pcs->pgtree, i);
-		if (!pingroup)
-			continue;
-		radix_tree_delete(&pcs->pgtree, i);
-	}
-	list_for_each_safe(pos, tmp, &pcs->pingroups) {
-		struct pcs_pingroup *pingroup;
-
-		pingroup = list_entry(pos, struct pcs_pingroup, node);
-		list_del(&pingroup->node);
-	}
-	mutex_unlock(&pcs->mutex);
-}
-
-/**
  * pcs_irq_free() - free interrupt
  * @pcs: pcs driver instance
  */
@@ -1491,7 +1359,7 @@ static void pcs_free_resources(struct pcs_device *pcs)
 	pcs_irq_free(pcs);
 	pinctrl_unregister(pcs->pctl);
 	pcs_free_funcs(pcs);
-	pcs_free_pingroups(pcs);
+
 #if IS_BUILTIN(CONFIG_PINCTRL_SINGLE)
 	if (pcs->missing_nr_pinctrl_cells)
 		of_remove_property(pcs->np, pcs->missing_nr_pinctrl_cells);
@@ -1885,7 +1753,6 @@ static int pcs_probe(struct platform_device *pdev)
 	pcs->np = np;
 	raw_spin_lock_init(&pcs->lock);
 	mutex_init(&pcs->mutex);
-	INIT_LIST_HEAD(&pcs->pingroups);
 	INIT_LIST_HEAD(&pcs->functions);
 	INIT_LIST_HEAD(&pcs->gpiofuncs);
 	soc = match->data;
@@ -1947,7 +1814,6 @@ static int pcs_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	INIT_RADIX_TREE(&pcs->pgtree, GFP_KERNEL);
 	INIT_RADIX_TREE(&pcs->ftree, GFP_KERNEL);
 	platform_set_drvdata(pdev, pcs);
 
-- 
2.11.0

[PATCH 5/5] pinctrl: single: Use generic pinmux helpers for managing functions

[Tony Lindgren]

We can now drop the driver specific code for managing functions.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/Kconfig          |   2 +-
 drivers/pinctrl/pinctrl-single.c | 134 ++++++---------------------------------
 2 files changed, 19 insertions(+), 117 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -167,7 +167,7 @@ config PINCTRL_SINGLE
 	tristate "One-register-per-pin type device tree based pinctrl driver"
 	depends on OF
 	select GENERIC_PINCTRL
-	select PINMUX
+	select GENERIC_PINMUX
 	select GENERIC_PINCONF
 	help
 	  This selects the device tree based generic pinctrl driver.
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -33,6 +33,7 @@
 #include "core.h"
 #include "devicetree.h"
 #include "pinconf.h"
+#include "pinmux.h"
 
 #define DRIVER_NAME			"pinctrl-single"
 #define PCS_OFF_DISABLED		~0U
@@ -160,13 +161,10 @@ struct pcs_soc_data {
  * @bits_per_mux: number of bits per mux
  * @bits_per_pin: number of bits per pin
  * @pins:	physical pins on the SoC
- * @ftree:	function index radix tree
- * @functions:	list of functions
  * @gpiofuncs:	list of gpio functions
  * @irqs:	list of interrupt registers
  * @chip:	chip container for this instance
  * @domain:	IRQ domain for this instance
- * @nfuncs:	number of functions
  * @desc:	pin controller descriptor
  * @read:	register read function to use
  * @write:	register write function to use
@@ -194,13 +192,10 @@ struct pcs_device {
 	bool bits_per_mux;
 	unsigned bits_per_pin;
 	struct pcs_data pins;
-	struct radix_tree_root ftree;
-	struct list_head functions;
 	struct list_head gpiofuncs;
 	struct list_head irqs;
 	struct irq_chip chip;
 	struct irq_domain *domain;
-	unsigned nfuncs;
 	struct pinctrl_desc desc;
 	unsigned (*read)(void __iomem *reg);
 	void (*write)(unsigned val, void __iomem *reg);
@@ -307,59 +302,13 @@ static const struct pinctrl_ops pcs_pinctrl_ops = {
 	.dt_free_map = pcs_dt_free_map,
 };
 
-static int pcs_get_functions_count(struct pinctrl_dev *pctldev)
-{
-	struct pcs_device *pcs;
-
-	pcs = pinctrl_dev_get_drvdata(pctldev);
-
-	return pcs->nfuncs;
-}
-
-static const char *pcs_get_function_name(struct pinctrl_dev *pctldev,
-						unsigned fselector)
-{
-	struct pcs_device *pcs;
-	struct pcs_function *func;
-
-	pcs = pinctrl_dev_get_drvdata(pctldev);
-	func = radix_tree_lookup(&pcs->ftree, fselector);
-	if (!func) {
-		dev_err(pcs->dev, "%s could not find function%i\n",
-			__func__, fselector);
-		return NULL;
-	}
-
-	return func->name;
-}
-
-static int pcs_get_function_groups(struct pinctrl_dev *pctldev,
-					unsigned fselector,
-					const char * const **groups,
-					unsigned * const ngroups)
-{
-	struct pcs_device *pcs;
-	struct pcs_function *func;
-
-	pcs = pinctrl_dev_get_drvdata(pctldev);
-	func = radix_tree_lookup(&pcs->ftree, fselector);
-	if (!func) {
-		dev_err(pcs->dev, "%s could not find function%i\n",
-			__func__, fselector);
-		return -EINVAL;
-	}
-	*groups = func->pgnames;
-	*ngroups = func->npgnames;
-
-	return 0;
-}
-
 static int pcs_get_function(struct pinctrl_dev *pctldev, unsigned pin,
 			    struct pcs_function **func)
 {
 	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
 	struct pin_desc *pdesc = pin_desc_get(pctldev, pin);
 	const struct pinctrl_setting_mux *setting;
+	struct function_desc *function;
 	unsigned fselector;
 
 	/* If pin is not described in DTS & enabled, mux_setting is NULL. */
@@ -367,7 +316,8 @@ static int pcs_get_function(struct pinctrl_dev *pctldev, unsigned pin,
 	if (!setting)
 		return -ENOTSUPP;
 	fselector = setting->func;
-	*func = radix_tree_lookup(&pcs->ftree, fselector);
+	function = pinmux_generic_get_function(pctldev, fselector);
+	*func = function->data;
 	if (!(*func)) {
 		dev_err(pcs->dev, "%s could not find function%i\n",
 			__func__, fselector);
@@ -380,6 +330,7 @@ static int pcs_set_mux(struct pinctrl_dev *pctldev, unsigned fselector,
 	unsigned group)
 {
 	struct pcs_device *pcs;
+	struct function_desc *function;
 	struct pcs_function *func;
 	int i;
 
@@ -387,7 +338,8 @@ static int pcs_set_mux(struct pinctrl_dev *pctldev, unsigned fselector,
 	/* If function mask is null, needn't enable it. */
 	if (!pcs->fmask)
 		return 0;
-	func = radix_tree_lookup(&pcs->ftree, fselector);
+	function = pinmux_generic_get_function(pctldev, fselector);
+	func = function->data;
 	if (!func)
 		return -EINVAL;
 
@@ -445,9 +397,9 @@ static int pcs_request_gpio(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinmux_ops pcs_pinmux_ops = {
-	.get_functions_count = pcs_get_functions_count,
-	.get_function_name = pcs_get_function_name,
-	.get_function_groups = pcs_get_function_groups,
+	.get_functions_count = pinmux_generic_get_function_count,
+	.get_function_name = pinmux_generic_get_function_name,
+	.get_function_groups = pinmux_generic_get_function_groups,
 	.set_mux = pcs_set_mux,
 	.gpio_request_enable = pcs_request_gpio,
 };
@@ -789,43 +741,24 @@ static struct pcs_function *pcs_add_function(struct pcs_device *pcs,
 					unsigned npgnames)
 {
 	struct pcs_function *function;
+	int res;
 
 	function = devm_kzalloc(pcs->dev, sizeof(*function), GFP_KERNEL);
 	if (!function)
 		return NULL;
 
-	function->name = name;
 	function->vals = vals;
 	function->nvals = nvals;
-	function->pgnames = pgnames;
-	function->npgnames = npgnames;
 
-	mutex_lock(&pcs->mutex);
-	list_add_tail(&function->node, &pcs->functions);
-	radix_tree_insert(&pcs->ftree, pcs->nfuncs, function);
-	pcs->nfuncs++;
-	mutex_unlock(&pcs->mutex);
+	res = pinmux_generic_add_function(pcs->pctl, name,
+					  pgnames, npgnames,
+					  function);
+	if (res)
+		return NULL;
 
 	return function;
 }
 
-static void pcs_remove_function(struct pcs_device *pcs,
-				struct pcs_function *function)
-{
-	int i;
-
-	mutex_lock(&pcs->mutex);
-	for (i = 0; i < pcs->nfuncs; i++) {
-		struct pcs_function *found;
-
-		found = radix_tree_lookup(&pcs->ftree, i);
-		if (found == function)
-			radix_tree_delete(&pcs->ftree, i);
-	}
-	list_del(&function->node);
-	mutex_unlock(&pcs->mutex);
-}
-
 /**
  * pcs_get_pin_by_offset() - get a pin index based on the register offset
  * @pcs: pcs driver instance
@@ -1103,7 +1036,7 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 	pinctrl_generic_remove_last_group(pcs->pctl);
 	*num_maps = 1;
 free_function:
-	pcs_remove_function(pcs, function);
+	pinmux_generic_remove_last_function(pcs->pctl);
 
 free_pins:
 	devm_kfree(pcs->dev, pins);
@@ -1235,8 +1168,7 @@ static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs,
 	pinctrl_generic_remove_last_group(pcs->pctl);
 	*num_maps = 1;
 free_function:
-	pcs_remove_function(pcs, function);
-
+	pinmux_generic_remove_last_function(pcs->pctl);
 free_pins:
 	devm_kfree(pcs->dev, pins);
 
@@ -1304,33 +1236,6 @@ static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
 }
 
 /**
- * pcs_free_funcs() - free memory used by functions
- * @pcs: pcs driver instance
- */
-static void pcs_free_funcs(struct pcs_device *pcs)
-{
-	struct list_head *pos, *tmp;
-	int i;
-
-	mutex_lock(&pcs->mutex);
-	for (i = 0; i < pcs->nfuncs; i++) {
-		struct pcs_function *func;
-
-		func = radix_tree_lookup(&pcs->ftree, i);
-		if (!func)
-			continue;
-		radix_tree_delete(&pcs->ftree, i);
-	}
-	list_for_each_safe(pos, tmp, &pcs->functions) {
-		struct pcs_function *function;
-
-		function = list_entry(pos, struct pcs_function, node);
-		list_del(&function->node);
-	}
-	mutex_unlock(&pcs->mutex);
-}
-
-/**
  * pcs_irq_free() - free interrupt
  * @pcs: pcs driver instance
  */
@@ -1358,7 +1263,6 @@ static void pcs_free_resources(struct pcs_device *pcs)
 {
 	pcs_irq_free(pcs);
 	pinctrl_unregister(pcs->pctl);
-	pcs_free_funcs(pcs);
 
 #if IS_BUILTIN(CONFIG_PINCTRL_SINGLE)
 	if (pcs->missing_nr_pinctrl_cells)
@@ -1753,7 +1657,6 @@ static int pcs_probe(struct platform_device *pdev)
 	pcs->np = np;
 	raw_spin_lock_init(&pcs->lock);
 	mutex_init(&pcs->mutex);
-	INIT_LIST_HEAD(&pcs->functions);
 	INIT_LIST_HEAD(&pcs->gpiofuncs);
 	soc = match->data;
 	pcs->flags = soc->flags;
@@ -1814,7 +1717,6 @@ static int pcs_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	INIT_RADIX_TREE(&pcs->ftree, GFP_KERNEL);
 	platform_set_drvdata(pdev, pcs);
 
 	switch (pcs->width) {
-- 
2.11.0

Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs

[Linus Walleij]

On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> wrote:

> Having the pin control framework call pin controller functions
> before it's probe has finished is not nice as the pin controller
> device driver does not yet have struct pinctrl_dev handle.
> Let's fix this issue by adding deferred work for late init. This is
> needed to be able to add pinctrl generic helper functions that expect
> to know struct pinctrl_dev handle. Note that we now need to call
> create_pinctrl() directly as we don't want to add the pin controller
> to the list of controllers until the hogs are claimed. We also need
> to pass the pinctrl_dev to the device tree parser functions as they
> otherwise won't find the right controller at this point.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Patch applied.

I felt the code path needed some extra comments so I sent those
as a separate patch.

Yours,
Linus Walleij

Re: [PATCH 3/5] pinctrl: core: Add generic pinctrl functions for managing groups

[Linus Walleij]

On Tue, Dec 27, 2016 at 6:20 PM, Tony Lindgren <tony@atomide.com> wrote:

> We can add generic helpers for function handling for cases where the pin
> controller driver does not need to use static arrays.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Patch applied!

> +config GENERIC_PINMUX
> +       bool
> +       select PINMUX

I applied the first that had GENERIC_PINCTRL instead,

Then I went in and renamed this GENERIC_PINCTRL_GROUPS
since that is what it's all about.

Sent out as separate patch!

Yours,
Linus Walleij

Re: [PATCH 2/5] pinctrl: core: Add generic pinctrl functions for managing groups

[Linus Walleij]

On Tue, Dec 27, 2016 at 6:20 PM, Tony Lindgren <tony@atomide.com> wrote:

> We can add generic helpers for pin group handling for cases where the pin
> controller driver does not need to use static arrays.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Patch applied.

> +config GENERIC_PINCTRL
> +       bool

Then I renamed *this* to GENERIC_PINCTRL_GROUPS.
(Not the other patch, I got confused because gmail threads the
second and third patch together, sorry.)

Let's see if I manage to rebase the next patch on this.

Yours,
Linus Walleij

Re: [PATCH 3/5] pinctrl: core: Add generic pinctrl functions for managing groups

[Linus Walleij]

On Tue, Dec 27, 2016 at 6:20 PM, Tony Lindgren <tony@atomide.com> wrote:

> We can add generic helpers for function handling for cases where the pin
> controller driver does not need to use static arrays.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Patch applied.

> +config GENERIC_PINMUX
> +       bool
> +       select PINMUX

I renamed this GENERIC_PINMUX_FUNCTIONS

> +       INIT_RADIX_TREE(&pctldev->pin_function_tree, GFP_KERNEL);

#ifdefed this

> +       struct radix_tree_root pin_function_tree;
>         unsigned int num_groups;
> +       unsigned int num_functions;

#ifdefed these

>  /**
> + * struct function_desc - generic function descriptor
> + * @name: name of the function
> + * @group_names: array of pin group names
> + * @num_group_names: number of pin group names
> + * @data: pin controller driver specific data
> + */
> +struct function_desc {
> +       const char *name;
> +       const char **group_names;
> +       int num_group_names;
> +       void *data;
> +};

And moved this into pinmux.h

Yours,
Linus Walleij

Re: [PATCH 4/5] pinctrl: single: Use generic pinctrl helpers for managing groups

[Linus Walleij]

On Tue, Dec 27, 2016 at 6:20 PM, Tony Lindgren <tony@atomide.com> wrote:

> We can now drop the driver specific code for managing groups.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Patch applied!

> +       select GENERIC_PINCTRL

Replaced this with GENERIC_PINCTRL_GROUPS

Yours,
Linus Walleij

Re: [PATCH 5/5] pinctrl: single: Use generic pinmux helpers for managing functions

[Linus Walleij]

On Tue, Dec 27, 2016 at 6:20 PM, Tony Lindgren <tony@atomide.com> wrote:

> We can now drop the driver specific code for managing functions.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Patch applied.

> -       select PINMUX
> +       select GENERIC_PINMUX

Replaced this with GENERIC_PINMUX_FUNCTIONS

Yours,
Linus Walleij

Re: [PATCHv2 0/5] Add generic pinctrl helpers for managing groups and function

[Linus Walleij]

On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> wrote:

> Here are some changes to add generic helpers for managing pinctrl groups and
> functions.

I applied it, screwed around with it and pushed to the build servers to see
if it survived.

I really like the look of this and I hope lots of driver start to use it.

Gary, I just applied your radix patches for i.MX, can you look if you can
use the GENERIC_PINCTRL_GROUPS and GENERIC_PINMUX_FUCNTIONS
that Tony invented and that I just merged to my devel branch in the
pinctrl tree?

It seems the i.MX could just reuse this right off and slim down quite a bit
but the devil is in the details.

Yours,
Linus Walleij

Re: [PATCHv2 0/5] Add generic pinctrl helpers for managing groups and function

[Gary Bisson]

Hi Linus,

On Fri, Dec 30, 2016 at 3:39 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> wrote:
>
>> Here are some changes to add generic helpers for managing pinctrl groups and
>> functions.
>
> I applied it, screwed around with it and pushed to the build servers to see
> if it survived.
>
> I really like the look of this and I hope lots of driver start to use it.
>
> Gary, I just applied your radix patches for i.MX, can you look if you can
> use the GENERIC_PINCTRL_GROUPS and GENERIC_PINMUX_FUCNTIONS
> that Tony invented and that I just merged to my devel branch in the
> pinctrl tree?

Yes I will have a look. It does sound like a good idea. I'll share my
findings beginning of next week.

Regards,
Gary

Re: [PATCH 2/5] pinctrl: core: Add generic pinctrl functions for managing groups

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [161230 06:12]:
> On Tue, Dec 27, 2016 at 6:20 PM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > We can add generic helpers for pin group handling for cases where the pin
> > controller driver does not need to use static arrays.
> >
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Patch applied.
> 
> > +config GENERIC_PINCTRL
> > +       bool
> 
> Then I renamed *this* to GENERIC_PINCTRL_GROUPS.
> (Not the other patch, I got confused because gmail threads the
> second and third patch together, sorry.)

Yeah OK nice, the renames make sense to me.

> Let's see if I manage to rebase the next patch on this.

I'll do some testing with your devel branch today to make
sure things still work for me.

Regards,

Tony

Re: [PATCHv2 0/5] Add generic pinctrl helpers for managing groups and function

[Tony Lindgren]

* Gary Bisson <gary.bisson@boundarydevices.com> [161230 07:43]:
> Hi Linus,
> 
> On Fri, Dec 30, 2016 at 3:39 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> wrote:
> >
> >> Here are some changes to add generic helpers for managing pinctrl groups and
> >> functions.
> >
> > I applied it, screwed around with it and pushed to the build servers to see
> > if it survived.
> >
> > I really like the look of this and I hope lots of driver start to use it.
> >
> > Gary, I just applied your radix patches for i.MX, can you look if you can
> > use the GENERIC_PINCTRL_GROUPS and GENERIC_PINMUX_FUCNTIONS
> > that Tony invented and that I just merged to my devel branch in the
> > pinctrl tree?
> 
> Yes I will have a look. It does sound like a good idea. I'll share my
> findings beginning of next week.

OK great. Note that we may be able to come up also with a generic
iterator function for the node_to_map functions so maybe see if
you come up with some ideas for that while experimenting :)

Tony

Re: [PATCHv2 0/5] Add generic pinctrl helpers for managing groups and function

[Gary Bisson]

Hi Linus, Tony,

On Fri, Dec 30, 2016 at 07:59:57AM -0800, Tony Lindgren wrote:
> * Gary Bisson <gary.bisson@boundarydevices.com> [161230 07:43]:
> > Hi Linus,
> > 
> > On Fri, Dec 30, 2016 at 3:39 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> wrote:
> > >
> > >> Here are some changes to add generic helpers for managing pinctrl groups and
> > >> functions.
> > >
> > > I applied it, screwed around with it and pushed to the build servers to see
> > > if it survived.
> > >
> > > I really like the look of this and I hope lots of driver start to use it.
> > >
> > > Gary, I just applied your radix patches for i.MX, can you look if you can
> > > use the GENERIC_PINCTRL_GROUPS and GENERIC_PINMUX_FUCNTIONS
> > > that Tony invented and that I just merged to my devel branch in the
> > > pinctrl tree?
> > 
> > Yes I will have a look. It does sound like a good idea. I'll share my
> > findings beginning of next week.

So I've had a look and indeed I can use some of it. Here is what my
series looks like:
 drivers/pinctrl/freescale/Kconfig       |   3 +-
 drivers/pinctrl/freescale/pinctrl-imx.c | 273
+++++++++-----------------------
 drivers/pinctrl/freescale/pinctrl-imx.h |  33 +---
 3 files changed, 80 insertions(+), 229 deletions(-)

Mainly I've used the generic functions in pinctrl_ops and pinmux_ops and
switched to the generic group/function_desc structures instead of the
imx-specific one that didn't bring anything else.

However I couldn't use the 'add' functions to add elements since the
parsing is done at probe time in its own way. So I've keep the
radix_insert functions. This could be modified though, it just requires
to modify the driver even more which I didn't feel like doing right now.

> OK great. Note that we may be able to come up also with a generic
> iterator function for the node_to_map functions so maybe see if
> you come up with some ideas for that while experimenting :)

I haven't had time to think about a generic node_to_map yet, but I
hopefully will have some time in a near future to think about it. Right
now it doesn't look obvious.

Also, I have some comments about your patches (already applied) which I
will send as a reply to the original patches.

Regards,
Gary

Re: [3/5] pinctrl: core: Add generic pinctrl functions for managing groups

[Gary Bisson]

Hi Tony,

On Tue, Dec 27, 2016 at 09:20:01AM -0800, Tony Lindgren wrote:
> We can add generic helpers for function handling for cases where the pin
> controller driver does not need to use static arrays.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Shouldn't the patch title be:
pinctrl: core: Add generic pinmux functions for managing functions

It looks like a copy/paste issue since both patches have the same title:
824bef17d16c pinctrl: core: Add generic pinctrl functions for managing
groups
d70a0fb14682 pinctrl: core: Add generic pinctrl functions for managing
groups

That's actually my only remark, I had another comment about freeing the
trees but it is actually done in the unregister so everything is good.

Regards,
Gary

Re: [3/5] pinctrl: core: Add generic pinctrl functions for managing groups

[Tony Lindgren]

* Gary Bisson <gary.bisson@boundarydevices.com> [170102 08:21]:
> Hi Tony,
> 
> On Tue, Dec 27, 2016 at 09:20:01AM -0800, Tony Lindgren wrote:
> > We can add generic helpers for function handling for cases where the pin
> > controller driver does not need to use static arrays.
> > 
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Shouldn't the patch title be:
> pinctrl: core: Add generic pinmux functions for managing functions
> 
> It looks like a copy/paste issue since both patches have the same title:
> 824bef17d16c pinctrl: core: Add generic pinctrl functions for managing
> groups
> d70a0fb14682 pinctrl: core: Add generic pinctrl functions for managing
> groups

Oops, oh well.. At least the description is correct.

> That's actually my only remark, I had another comment about freeing the
> trees but it is actually done in the unregister so everything is good.

OK cool.

Tony

Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs

[Geert Uytterhoeven]

Hi Tony,

On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> wrote:
> Having the pin control framework call pin controller functions
> before it's probe has finished is not nice as the pin controller
> device driver does not yet have struct pinctrl_dev handle.
>
> Let's fix this issue by adding deferred work for late init. This is
> needed to be able to add pinctrl generic helper functions that expect
> to know struct pinctrl_dev handle. Note that we now need to call
> create_pinctrl() directly as we don't want to add the pin controller
> to the list of controllers until the hogs are claimed. We also need
> to pass the pinctrl_dev to the device tree parser functions as they
> otherwise won't find the right controller at this point.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

I believe this patch causes a regression on r8a7740/armadillo, where the
pin controller is also a GPIO controller, and lcd0 needs a hog
(cfr. arch/arm/boot/dts/r8a7740-armadillo800eva.dts):

-GPIO line 176 (lcd0) hogged as output/high
-sh-pfc e6050000.pfc: r8a7740_pfc handling gpio 0 -> 211
+gpiochip_add_data: GPIOs 0..211 (r8a7740_pfc) failed to register
+sh-pfc e6050000.pfc: failed to init GPIO chip, ignoring...
 sh-pfc e6050000.pfc: r8a7740_pfc support registered

Hence all drivers using GPIOs fail to initialize because their GPIOs never
become available.

Adding debug prints to the failure paths shows that the call to
of_pinctrl_get() in of_gpiochip_add_pin_range() fails with -EPROBE_DEFER.
Adding a debug print to the top of gpiochip_add_data() makes the problem go
away, presumably because it introduces a delay that allows the delayed work
to kick in...

Jon's fix ("pinctrl: core: Fix panic when pinctrl devices with hogs are
unregistered") doesn't help, as it affects unregistration only.

> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c

> @@ -1800,32 +1847,10 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
>                 goto out_err;
>         }
>
> -       mutex_lock(&pinctrldev_list_mutex);
> -       list_add_tail(&pctldev->node, &pinctrldev_list);
> -       mutex_unlock(&pinctrldev_list_mutex);
> -
> -       pctldev->p = pinctrl_get(pctldev->dev);
> -
> -       if (!IS_ERR(pctldev->p)) {
> -               pctldev->hog_default =
> -                       pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
> -               if (IS_ERR(pctldev->hog_default)) {
> -                       dev_dbg(dev, "failed to lookup the default state\n");
> -               } else {
> -                       if (pinctrl_select_state(pctldev->p,
> -                                               pctldev->hog_default))
> -                               dev_err(dev,
> -                                       "failed to select default state\n");
> -               }
> -
> -               pctldev->hog_sleep =
> -                       pinctrl_lookup_state(pctldev->p,
> -                                                   PINCTRL_STATE_SLEEP);
> -               if (IS_ERR(pctldev->hog_sleep))
> -                       dev_dbg(dev, "failed to lookup the sleep state\n");
> -       }
> -
> -       pinctrl_init_device_debugfs(pctldev);
> +       if (pinctrl_dt_has_hogs(pctldev))

Changing the above line to "if (0 && pinctrl_dt_has_hogs(pctldev))"
fixes the issue for me.

> +               schedule_delayed_work(&pctldev->late_init, 0);
> +       else
> +               pinctrl_late_init(&pctldev->late_init.work);
>
>         return pctldev;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs

[Tony Lindgren]

* Geert Uytterhoeven <geert@linux-m68k.org> [170110 06:09]:
> Hi Tony,
> 
> On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> wrote:
> > Having the pin control framework call pin controller functions
> > before it's probe has finished is not nice as the pin controller
> > device driver does not yet have struct pinctrl_dev handle.
> >
> > Let's fix this issue by adding deferred work for late init. This is
> > needed to be able to add pinctrl generic helper functions that expect
> > to know struct pinctrl_dev handle. Note that we now need to call
> > create_pinctrl() directly as we don't want to add the pin controller
> > to the list of controllers until the hogs are claimed. We also need
> > to pass the pinctrl_dev to the device tree parser functions as they
> > otherwise won't find the right controller at this point.
> >
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> I believe this patch causes a regression on r8a7740/armadillo, where the
> pin controller is also a GPIO controller, and lcd0 needs a hog
> (cfr. arch/arm/boot/dts/r8a7740-armadillo800eva.dts):
> 
> -GPIO line 176 (lcd0) hogged as output/high
> -sh-pfc e6050000.pfc: r8a7740_pfc handling gpio 0 -> 211
> +gpiochip_add_data: GPIOs 0..211 (r8a7740_pfc) failed to register
> +sh-pfc e6050000.pfc: failed to init GPIO chip, ignoring...
>  sh-pfc e6050000.pfc: r8a7740_pfc support registered
> 
> Hence all drivers using GPIOs fail to initialize because their GPIOs never
> become available.
> 
> Adding debug prints to the failure paths shows that the call to
> of_pinctrl_get() in of_gpiochip_add_pin_range() fails with -EPROBE_DEFER.
> Adding a debug print to the top of gpiochip_add_data() makes the problem go
> away, presumably because it introduces a delay that allows the delayed work
> to kick in...

OK. What if we added also an optional pinctrl function that the pin
controller driver could call to initialize hogs? Then the pin controller
driver could call it during or after probe as needed. That is after
there's a valid struct pinctrl_dev handle.

> Jon's fix ("pinctrl: core: Fix panic when pinctrl devices with hogs are
> unregistered") doesn't help, as it affects unregistration only.
> 
> > --- a/drivers/pinctrl/core.c
> > +++ b/drivers/pinctrl/core.c
> 
> > @@ -1800,32 +1847,10 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
> >                 goto out_err;
> >         }
> >
> > -       mutex_lock(&pinctrldev_list_mutex);
> > -       list_add_tail(&pctldev->node, &pinctrldev_list);
> > -       mutex_unlock(&pinctrldev_list_mutex);
> > -
> > -       pctldev->p = pinctrl_get(pctldev->dev);
> > -
> > -       if (!IS_ERR(pctldev->p)) {
> > -               pctldev->hog_default =
> > -                       pinctrl_lookup_state(pctldev->p, PINCTRL_STATE_DEFAULT);
> > -               if (IS_ERR(pctldev->hog_default)) {
> > -                       dev_dbg(dev, "failed to lookup the default state\n");
> > -               } else {
> > -                       if (pinctrl_select_state(pctldev->p,
> > -                                               pctldev->hog_default))
> > -                               dev_err(dev,
> > -                                       "failed to select default state\n");
> > -               }
> > -
> > -               pctldev->hog_sleep =
> > -                       pinctrl_lookup_state(pctldev->p,
> > -                                                   PINCTRL_STATE_SLEEP);
> > -               if (IS_ERR(pctldev->hog_sleep))
> > -                       dev_dbg(dev, "failed to lookup the sleep state\n");
> > -       }
> > -
> > -       pinctrl_init_device_debugfs(pctldev);
> > +       if (pinctrl_dt_has_hogs(pctldev))
> 
> Changing the above line to "if (0 && pinctrl_dt_has_hogs(pctldev))"
> fixes the issue for me.
> 
> > +               schedule_delayed_work(&pctldev->late_init, 0);
> > +       else
> > +               pinctrl_late_init(&pctldev->late_init.work);
> >
> >         return pctldev;

We could also pass some flag if should always call pinctrl_late_init()
directly. But that does not remove the problem of struct pinctrl_dev handle
being uninitialized when the pin controller driver functionas are called.

Regards,

Tony

Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170110 07:32]:
> * Geert Uytterhoeven <geert@linux-m68k.org> [170110 06:09]:
> > Hi Tony,
> > 
> > On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@atomide.com> wrote:
> > > Having the pin control framework call pin controller functions
> > > before it's probe has finished is not nice as the pin controller
> > > device driver does not yet have struct pinctrl_dev handle.
> > >
> > > Let's fix this issue by adding deferred work for late init. This is
> > > needed to be able to add pinctrl generic helper functions that expect
> > > to know struct pinctrl_dev handle. Note that we now need to call
> > > create_pinctrl() directly as we don't want to add the pin controller
> > > to the list of controllers until the hogs are claimed. We also need
> > > to pass the pinctrl_dev to the device tree parser functions as they
> > > otherwise won't find the right controller at this point.
> > >
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > 
> > I believe this patch causes a regression on r8a7740/armadillo, where the
> > pin controller is also a GPIO controller, and lcd0 needs a hog
> > (cfr. arch/arm/boot/dts/r8a7740-armadillo800eva.dts):
> > 
> > -GPIO line 176 (lcd0) hogged as output/high
> > -sh-pfc e6050000.pfc: r8a7740_pfc handling gpio 0 -> 211
> > +gpiochip_add_data: GPIOs 0..211 (r8a7740_pfc) failed to register
> > +sh-pfc e6050000.pfc: failed to init GPIO chip, ignoring...
> >  sh-pfc e6050000.pfc: r8a7740_pfc support registered
> > 
> > Hence all drivers using GPIOs fail to initialize because their GPIOs never
> > become available.
> > 
> > Adding debug prints to the failure paths shows that the call to
> > of_pinctrl_get() in of_gpiochip_add_pin_range() fails with -EPROBE_DEFER.
> > Adding a debug print to the top of gpiochip_add_data() makes the problem go
> > away, presumably because it introduces a delay that allows the delayed work
> > to kick in...
> 
> OK. What if we added also an optional pinctrl function that the pin
> controller driver could call to initialize hogs? Then the pin controller
> driver could call it during or after probe as needed. That is after
> there's a valid struct pinctrl_dev handle.
...
> We could also pass some flag if should always call pinctrl_late_init()
> directly. But that does not remove the problem of struct pinctrl_dev handle
> being uninitialized when the pin controller driver functionas are called.

Looks like we need both a flag and a way for the pin controller driver
to start things.

Below is an experimental fix to intorduce pinctrl_start() that I've
tested with pinctrl-single. Then we should probably make all pin controller
drivers call pinctrl_start() to properly fix the issue of struct pinctrl_dev
handle not being initialized before driver functions are called.

Or do you guys have any better ideas?

Regards,

Tony

8< --------------------------------
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1962,6 +1962,17 @@ static void pinctrl_late_init(struct work_struct *work)
 	pinctrl_init_device_debugfs(pctldev);
 }
 
+int pinctrl_start(struct pinctrl_dev *pctldev)
+{
+       if (!IS_ERR(pctldev->p))
+               return -EEXIST;
+
+       pinctrl_late_init(&pctldev->late_init.work);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(pinctrl_start);
+
 /**
  * pinctrl_register() - register a pin controller device
  * @pctldesc: descriptor for this pin controller
@@ -2035,9 +2046,14 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	/*
 	 * If the device has hogs we want the probe() function of the driver
 	 * to complete before we go in and hog them and add the pin controller
-	 * to the list of controllers. If it has no hogs, we can just complete
-	 * the registration immediately.
+	 * to the list of controllers. If the pin controller driver initializes
+	 * hogs, or the pin controller instance has no hogs, we can just
+	 * complete the registration immediately.
 	 */
+
+	if (pctldesc->flags & PINCTRL_DRIVER_START)
+		return pctldev;
+
 	if (pinctrl_dt_has_hogs(pctldev))
 		schedule_delayed_work(&pctldev->late_init, 0);
 	else
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1741,6 +1741,7 @@ static int pcs_probe(struct platform_device *pdev)
 	pcs->desc.pmxops = &pcs_pinmux_ops;
 	if (PCS_HAS_PINCONF)
 		pcs->desc.confops = &pcs_pinconf_ops;
+	pcs->desc.flags = PINCTRL_DRIVER_START;
 	pcs->desc.owner = THIS_MODULE;
 
 	ret = pcs_allocate_pin_table(pcs);
@@ -1754,6 +1755,10 @@ static int pcs_probe(struct platform_device *pdev)
 		goto free;
 	}
 
+	ret = pinctrl_start(pcs->pctl);
+	if (ret)
+		goto free;
+
 	ret = pcs_add_gpio_func(np, pcs);
 	if (ret < 0)
 		goto free;
diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c
--- a/drivers/pinctrl/sh-pfc/pinctrl.c
+++ b/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -815,7 +815,15 @@ int sh_pfc_register_pinctrl(struct sh_pfc *pfc)
 	pmx->pctl_desc.confops = &sh_pfc_pinconf_ops;
 	pmx->pctl_desc.pins = pmx->pins;
 	pmx->pctl_desc.npins = pfc->info->nr_pins;
+	pmx->pctl_desc.flags = PINCTRL_DRIVER_START;
 
 	pmx->pctl = devm_pinctrl_register(pfc->dev, &pmx->pctl_desc, pmx);
-	return PTR_ERR_OR_ZERO(pmx->pctl);
+	if (IS_ERR(pmx->pctl))
+		return PTR_ERR(pmx->pctl);
+
+	ret = pinctrl_start(pmx->pctl);
+	if (ret)
+		return ret;
+
+	return 0;
 }
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -104,6 +104,8 @@ struct pinctrl_ops {
 			     struct pinctrl_map *map, unsigned num_maps);
 };
 
+#define PINCTRL_DRIVER_START		BIT(0)
+
 /**
  * struct pinctrl_desc - pin controller descriptor, register this to pin
  * control subsystem
@@ -112,6 +114,8 @@ struct pinctrl_ops {
  *	this pin controller
  * @npins: number of descriptors in the array, usually just ARRAY_SIZE()
  *	of the pins field above
+ * @flags: Optional pin controller feature flags
+ *	handling is needed in the pin controller driver.
  * @pctlops: pin control operation vtable, to support global concepts like
  *	grouping of pins, this is optional.
  * @pmxops: pinmux operations vtable, if you support pinmuxing in your driver
@@ -129,6 +133,7 @@ struct pinctrl_desc {
 	const char *name;
 	const struct pinctrl_pin_desc *pins;
 	unsigned int npins;
+	unsigned int flags;
 	const struct pinctrl_ops *pctlops;
 	const struct pinmux_ops *pmxops;
 	const struct pinconf_ops *confops;
@@ -149,6 +154,7 @@ extern struct pinctrl_dev *devm_pinctrl_register(struct device *dev,
 				void *driver_data);
 extern void devm_pinctrl_unregister(struct device *dev,
 				struct pinctrl_dev *pctldev);
+extern int pinctrl_start(struct pinctrl_dev *pctldev);
 
 extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin);
 extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
-- 
2.11.0

Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs

[Linus Walleij]

On Tue, Jan 10, 2017 at 8:19 PM, Tony Lindgren <tony@atomide.com> wrote:

> Below is an experimental fix to intorduce pinctrl_start() that I've
> tested with pinctrl-single. Then we should probably make all pin controller
> drivers call pinctrl_start() to properly fix the issue of struct pinctrl_dev
> handle not being initialized before driver functions are called.

Hm I guess that could work, but can we keep pinctrl_register() with the old
semantics and add a separate pinctrl_register_and_defer()
for those who just wanna start it later by a separate call?

Then we don't need any special flags.

> Or do you guys have any better ideas?

Not really. So you mean revert the previous patch and apply something
like this instead?

Yours,
Linus Walleij

Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [170111 07:34]:
> On Tue, Jan 10, 2017 at 8:19 PM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > Below is an experimental fix to intorduce pinctrl_start() that I've
> > tested with pinctrl-single. Then we should probably make all pin controller
> > drivers call pinctrl_start() to properly fix the issue of struct pinctrl_dev
> > handle not being initialized before driver functions are called.
> 
> Hm I guess that could work, but can we keep pinctrl_register() with the old
> semantics and add a separate pinctrl_register_and_defer()
> for those who just wanna start it later by a separate call?
> 
> Then we don't need any special flags.

OK I'll take a look.

> > Or do you guys have any better ideas?
> 
> Not really. So you mean revert the previous patch and apply something
> like this instead?

Let me first take a look to see if we can fix it by making drivers using
GENERIC_PINCTRL_GROUPS or GENERIC_PINMUX_FUNCTIONS register with
pinctrl_register_and_defer(). I'll post a patch for that today.

Then maybe for v4.12 we can attempt to move all pin controller drivers
to using it so we can fix the problem for good.

Regards,

Tony

Re: [PATCH 1/5] pinctrl: core: Use delayed work for hogs

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170111 08:29]:
> * Linus Walleij <linus.walleij@linaro.org> [170111 07:34]:
> > On Tue, Jan 10, 2017 at 8:19 PM, Tony Lindgren <tony@atomide.com> wrote:
> > 
> > > Below is an experimental fix to intorduce pinctrl_start() that I've
> > > tested with pinctrl-single. Then we should probably make all pin controller
> > > drivers call pinctrl_start() to properly fix the issue of struct pinctrl_dev
> > > handle not being initialized before driver functions are called.
> > 
> > Hm I guess that could work, but can we keep pinctrl_register() with the old
> > semantics and add a separate pinctrl_register_and_defer()
> > for those who just wanna start it later by a separate call?
> > 
> > Then we don't need any special flags.
> 
> OK I'll take a look.
> 
> > > Or do you guys have any better ideas?
> > 
> > Not really. So you mean revert the previous patch and apply something
> > like this instead?
> 
> Let me first take a look to see if we can fix it by making drivers using
> GENERIC_PINCTRL_GROUPS or GENERIC_PINMUX_FUNCTIONS register with
> pinctrl_register_and_defer(). I'll post a patch for that today.

Yeah we can fix this by reverting the late_init parts of the earlier
attempt and introducing a new pinctrl_register_and_init() for controllers
to use:

extern int pinctrl_register_and_init(struct pinctrl_desc *pctldesc,
				     struct device *dev, void *driver_data,
				     struct pinctrl_dev **pctldev);

> Then maybe for v4.12 we can attempt to move all pin controller drivers
> to using it so we can fix the problem for good.

And that will also make converting existing drivers to use it later on
trivial.

Will post a patch shortly after some more testing.

Regards,

Tony