[PATCH 0/2] pinctrl: imx: use generic pinctrl helpers

[PATCH 0/2] pinctrl: imx: use generic pinctrl helpers

[Gary Bisson]

Hi all,

This series is a follow-up to Linus suggestion to use the generic helpers
introduced by Tony [1].

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 special.

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 kept the
radix_insert functions. This could be modified in the future though.

Regards,
Gary

[1] http://www.spinics.net/lists/devicetree/msg155984.html

Gary Bisson (2):
  pinctrl: imx: use generic pinctrl helpers for managing groups
  pinctrl: imx: use generic pinmux helpers for managing functions

 drivers/pinctrl/freescale/Kconfig       |   3 +-
 drivers/pinctrl/freescale/pinctrl-imx.c | 286 ++++++++++----------------------
 drivers/pinctrl/freescale/pinctrl-imx.h |  33 +---
 3 files changed, 87 insertions(+), 235 deletions(-)

-- 
2.11.0

[PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Gary Bisson]

Now using group_desc structure instead of imx_pin_group.

Also leveraging generic functions to retrieve groups count/name/pins.

The imx_free_pingroups function can be removed since it is now handled by
the core driver during unregister.

Finally the device tree parsing is moved after the pinctrl driver registration
since this latter initializes the radix trees.

Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>
---
 drivers/pinctrl/freescale/Kconfig       |   1 +
 drivers/pinctrl/freescale/pinctrl-imx.c | 190 +++++++++++---------------------
 drivers/pinctrl/freescale/pinctrl-imx.h |  19 +---
 3 files changed, 69 insertions(+), 141 deletions(-)

diff --git a/drivers/pinctrl/freescale/Kconfig b/drivers/pinctrl/freescale/Kconfig
index fc8cbf611723..cb50e21615da 100644
--- a/drivers/pinctrl/freescale/Kconfig
+++ b/drivers/pinctrl/freescale/Kconfig
@@ -1,5 +1,6 @@
 config PINCTRL_IMX
 	bool
+	select GENERIC_PINCTRL_GROUPS
 	select PINMUX
 	select PINCONF
 	select REGMAP
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index e832a2c7af68..62c20f661fed 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -45,15 +45,15 @@ struct imx_pinctrl {
 	struct imx_pinctrl_soc_info *info;
 };
 
-static inline const struct imx_pin_group *imx_pinctrl_find_group_by_name(
-				struct imx_pinctrl_soc_info *info,
+static inline const struct group_desc *imx_pinctrl_find_group_by_name(
+				struct pinctrl_dev *pctldev,
 				const char *name)
 {
-	const struct imx_pin_group *grp = NULL;
+	const struct group_desc *grp = NULL;
 	int i;
 
-	for (i = 0; i < info->ngroups; i++) {
-		grp = radix_tree_lookup(&info->pgtree, i);
+	for (i = 0; i < pctldev->num_groups; i++) {
+		grp = pinctrl_generic_get_group(pctldev, i);
 		if (grp && !strcmp(grp->name, name))
 			break;
 	}
@@ -61,49 +61,6 @@ static inline const struct imx_pin_group *imx_pinctrl_find_group_by_name(
 	return grp;
 }
 
-static int imx_get_groups_count(struct pinctrl_dev *pctldev)
-{
-	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
-	struct imx_pinctrl_soc_info *info = ipctl->info;
-
-	return info->ngroups;
-}
-
-static const char *imx_get_group_name(struct pinctrl_dev *pctldev,
-				       unsigned selector)
-{
-	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
-	struct imx_pinctrl_soc_info *info = ipctl->info;
-	struct imx_pin_group *grp = NULL;
-
-	grp = radix_tree_lookup(&info->pgtree, selector);
-	if (!grp)
-		return NULL;
-
-	return grp->name;
-}
-
-static int imx_get_group_pins(struct pinctrl_dev *pctldev, unsigned selector,
-			       const unsigned **pins,
-			       unsigned *npins)
-{
-	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
-	struct imx_pinctrl_soc_info *info = ipctl->info;
-	struct imx_pin_group *grp = NULL;
-
-	if (selector >= info->ngroups)
-		return -EINVAL;
-
-	grp = radix_tree_lookup(&info->pgtree, selector);
-	if (!grp)
-		return -EINVAL;
-
-	*pins = grp->pin_ids;
-	*npins = grp->npins;
-
-	return 0;
-}
-
 static void imx_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
 		   unsigned offset)
 {
@@ -116,7 +73,7 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 {
 	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
 	struct imx_pinctrl_soc_info *info = ipctl->info;
-	const struct imx_pin_group *grp;
+	const struct group_desc *grp;
 	struct pinctrl_map *new_map;
 	struct device_node *parent;
 	int map_num = 1;
@@ -126,15 +83,17 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 	 * first find the group of this node and check if we need create
 	 * config maps for pins
 	 */
-	grp = imx_pinctrl_find_group_by_name(info, np->name);
+	grp = imx_pinctrl_find_group_by_name(pctldev, np->name);
 	if (!grp) {
 		dev_err(info->dev, "unable to find group for node %s\n",
 			np->name);
 		return -EINVAL;
 	}
 
-	for (i = 0; i < grp->npins; i++) {
-		if (!(grp->pins[i].config & IMX_NO_PAD_CTL))
+	for (i = 0; i < grp->num_pins; i++) {
+		struct imx_pin *pin = &((struct imx_pin *)(grp->data))[i];
+
+		if (!(pin->config & IMX_NO_PAD_CTL))
 			map_num++;
 	}
 
@@ -158,12 +117,14 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
 
 	/* create config map */
 	new_map++;
-	for (i = j = 0; i < grp->npins; i++) {
-		if (!(grp->pins[i].config & IMX_NO_PAD_CTL)) {
+	for (i = j = 0; i < grp->num_pins; i++) {
+		struct imx_pin *pin = &((struct imx_pin *)(grp->data))[i];
+
+		if (!(pin->config & IMX_NO_PAD_CTL)) {
 			new_map[j].type = PIN_MAP_TYPE_CONFIGS_PIN;
 			new_map[j].data.configs.group_or_pin =
-					pin_get_name(pctldev, grp->pins[i].pin);
-			new_map[j].data.configs.configs = &grp->pins[i].config;
+					pin_get_name(pctldev, pin->pin);
+			new_map[j].data.configs.configs = &pin->config;
 			new_map[j].data.configs.num_configs = 1;
 			j++;
 		}
@@ -182,9 +143,9 @@ static void imx_dt_free_map(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinctrl_ops imx_pctrl_ops = {
-	.get_groups_count = imx_get_groups_count,
-	.get_group_name = imx_get_group_name,
-	.get_group_pins = imx_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 = imx_pin_dbg_show,
 	.dt_node_to_map = imx_dt_node_to_map,
 	.dt_free_map = imx_dt_free_map,
@@ -199,14 +160,14 @@ static int imx_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
 	const struct imx_pin_reg *pin_reg;
 	unsigned int npins, pin_id;
 	int i;
-	struct imx_pin_group *grp = NULL;
+	struct group_desc *grp = NULL;
 	struct imx_pmx_func *func = NULL;
 
 	/*
 	 * Configure the mux mode for each pin in the group for a specific
 	 * function.
 	 */
-	grp = radix_tree_lookup(&info->pgtree, group);
+	grp = pinctrl_generic_get_group(pctldev, group);
 	if (!grp)
 		return -EINVAL;
 
@@ -214,13 +175,14 @@ static int imx_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
 	if (!func)
 		return -EINVAL;
 
-	npins = grp->npins;
+	npins = grp->num_pins;
 
 	dev_dbg(ipctl->dev, "enable function %s group %s\n",
 		func->name, grp->name);
 
 	for (i = 0; i < npins; i++) {
-		struct imx_pin *pin = &grp->pins[i];
+		struct imx_pin *pin = &((struct imx_pin *)(grp->data))[i];
+
 		pin_id = pin->pin;
 		pin_reg = &info->pin_regs[pin_id];
 
@@ -335,7 +297,7 @@ static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
 	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
 	struct imx_pinctrl_soc_info *info = ipctl->info;
 	const struct imx_pin_reg *pin_reg;
-	struct imx_pin_group *grp;
+	struct group_desc *grp;
 	struct imx_pin *imx_pin;
 	unsigned int pin, group;
 	u32 reg;
@@ -349,12 +311,12 @@ static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
 		return -EINVAL;
 
 	/* Find the pinctrl config with GPIO mux mode for the requested pin */
-	for (group = 0; group < info->ngroups; group++) {
-		grp = radix_tree_lookup(&info->pgtree, group);
+	for (group = 0; group < pctldev->num_groups; group++) {
+		grp = pinctrl_generic_get_group(pctldev, group);
 		if (!grp)
 			continue;
-		for (pin = 0; pin < grp->npins; pin++) {
-			imx_pin = &grp->pins[pin];
+		for (pin = 0; pin < grp->num_pins; pin++) {
+			imx_pin = &((struct imx_pin *)(grp->data))[pin];
 			if (imx_pin->pin == offset && !imx_pin->mux_mode)
 				goto mux_pin;
 		}
@@ -512,23 +474,22 @@ static void imx_pinconf_dbg_show(struct pinctrl_dev *pctldev,
 static void imx_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
 					 struct seq_file *s, unsigned group)
 {
-	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
-	struct imx_pinctrl_soc_info *info = ipctl->info;
-	struct imx_pin_group *grp;
+	struct group_desc *grp;
 	unsigned long config;
 	const char *name;
 	int i, ret;
 
-	if (group > info->ngroups)
+	if (group > pctldev->num_groups)
 		return;
 
 	seq_printf(s, "\n");
-	grp = radix_tree_lookup(&info->pgtree, group);
+	grp = pinctrl_generic_get_group(pctldev, group);
 	if (!grp)
 		return;
 
-	for (i = 0; i < grp->npins; i++) {
-		struct imx_pin *pin = &grp->pins[i];
+	for (i = 0; i < grp->num_pins; i++) {
+		struct imx_pin *pin = &((struct imx_pin *)(grp->data))[i];
+
 		name = pin_get_name(pctldev, pin->pin);
 		ret = imx_pinconf_get(pctldev, pin->pin, &config);
 		if (ret)
@@ -552,7 +513,7 @@ static const struct pinconf_ops imx_pinconf_ops = {
 #define SHARE_FSL_PIN_SIZE 20
 
 static int imx_pinctrl_parse_groups(struct device_node *np,
-				    struct imx_pin_group *grp,
+				    struct group_desc *grp,
 				    struct imx_pinctrl_soc_info *info,
 				    u32 index)
 {
@@ -586,20 +547,20 @@ static int imx_pinctrl_parse_groups(struct device_node *np,
 		return -EINVAL;
 	}
 
-	grp->npins = size / pin_size;
-	grp->pins = devm_kzalloc(info->dev, grp->npins * sizeof(struct imx_pin),
-				GFP_KERNEL);
-	grp->pin_ids = devm_kzalloc(info->dev, grp->npins * sizeof(unsigned int),
-				GFP_KERNEL);
-	if (!grp->pins || ! grp->pin_ids)
+	grp->num_pins = size / pin_size;
+	grp->data = devm_kzalloc(info->dev, grp->num_pins *
+				 sizeof(struct imx_pin), GFP_KERNEL);
+	grp->pins = devm_kzalloc(info->dev, grp->num_pins *
+				 sizeof(unsigned int), GFP_KERNEL);
+	if (!grp->pins || !grp->data)
 		return -ENOMEM;
 
-	for (i = 0; i < grp->npins; i++) {
+	for (i = 0; i < grp->num_pins; i++) {
 		u32 mux_reg = be32_to_cpu(*list++);
 		u32 conf_reg;
 		unsigned int pin_id;
 		struct imx_pin_reg *pin_reg;
-		struct imx_pin *pin = &grp->pins[i];
+		struct imx_pin *pin = &((struct imx_pin *)(grp->data))[i];
 
 		if (!(info->flags & ZERO_OFFSET_VALID) && !mux_reg)
 			mux_reg = -1;
@@ -615,7 +576,7 @@ static int imx_pinctrl_parse_groups(struct device_node *np,
 		pin_id = (mux_reg != -1) ? mux_reg / 4 : conf_reg / 4;
 		pin_reg = &info->pin_regs[pin_id];
 		pin->pin = pin_id;
-		grp->pin_ids[i] = pin_id;
+		grp->pins[i] = pin_id;
 		pin_reg->mux_reg = mux_reg;
 		pin_reg->conf_reg = conf_reg;
 		pin->input_reg = be32_to_cpu(*list++);
@@ -636,12 +597,14 @@ static int imx_pinctrl_parse_groups(struct device_node *np,
 }
 
 static int imx_pinctrl_parse_functions(struct device_node *np,
-				       struct imx_pinctrl_soc_info *info,
+				       struct imx_pinctrl *ipctl,
 				       u32 index)
 {
+	struct pinctrl_dev *pctl = ipctl->pctl;
+	struct imx_pinctrl_soc_info *info = ipctl->info;
 	struct device_node *child;
 	struct imx_pmx_func *func;
-	struct imx_pin_group *grp;
+	struct group_desc *grp;
 	u32 i = 0;
 
 	dev_dbg(info->dev, "parse function(%d): %s\n", index, np->name);
@@ -663,13 +626,14 @@ static int imx_pinctrl_parse_functions(struct device_node *np,
 	for_each_child_of_node(np, child) {
 		func->groups[i] = child->name;
 
-		grp = devm_kzalloc(info->dev, sizeof(struct imx_pin_group),
+		grp = devm_kzalloc(info->dev, sizeof(struct group_desc),
 				   GFP_KERNEL);
 		if (!grp)
 			return -ENOMEM;
 
 		mutex_lock(&info->mutex);
-		radix_tree_insert(&info->pgtree, info->group_index++, grp);
+		radix_tree_insert(&pctl->pin_group_tree,
+				  info->group_index++, grp);
 		mutex_unlock(&info->mutex);
 
 		imx_pinctrl_parse_groups(child, grp, info, i++);
@@ -702,10 +666,12 @@ static bool imx_pinctrl_dt_is_flat_functions(struct device_node *np)
 }
 
 static int imx_pinctrl_probe_dt(struct platform_device *pdev,
-				struct imx_pinctrl_soc_info *info)
+				struct imx_pinctrl *ipctl)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *child;
+	struct pinctrl_dev *pctl = ipctl->pctl;
+	struct imx_pinctrl_soc_info *info = ipctl->info;
 	u32 nfuncs = 0;
 	u32 i = 0;
 	bool flat_funcs;
@@ -740,19 +706,19 @@ static int imx_pinctrl_probe_dt(struct platform_device *pdev,
 
 	info->group_index = 0;
 	if (flat_funcs) {
-		info->ngroups = of_get_child_count(np);
+		pctl->num_groups = of_get_child_count(np);
 	} else {
-		info->ngroups = 0;
+		pctl->num_groups = 0;
 		for_each_child_of_node(np, child)
-			info->ngroups += of_get_child_count(child);
+			pctl->num_groups += of_get_child_count(child);
 	}
 
 	if (flat_funcs) {
-		imx_pinctrl_parse_functions(np, info, 0);
+		imx_pinctrl_parse_functions(np, ipctl, 0);
 	} else {
 		i = 0;
 		for_each_child_of_node(np, child)
-			imx_pinctrl_parse_functions(child, info, i++);
+			imx_pinctrl_parse_functions(child, ipctl, i++);
 	}
 
 	return 0;
@@ -779,26 +745,6 @@ static void imx_free_funcs(struct imx_pinctrl_soc_info *info)
 }
 
 /*
- * imx_free_pingroups() - free memory used by pingroups
- * @info: info driver instance
- */
-static void imx_free_pingroups(struct imx_pinctrl_soc_info *info)
-{
-	int i;
-
-	mutex_lock(&info->mutex);
-	for (i = 0; i < info->ngroups; i++) {
-		struct imx_pin_group *pingroup;
-
-		pingroup = radix_tree_lookup(&info->pgtree, i);
-		if (!pingroup)
-			continue;
-		radix_tree_delete(&info->pgtree, i);
-	}
-	mutex_unlock(&info->mutex);
-}
-
-/*
  * imx_free_resources() - free memory used by this driver
  * @info: info driver instance
  */
@@ -808,7 +754,6 @@ static void imx_free_resources(struct imx_pinctrl *ipctl)
 		pinctrl_unregister(ipctl->pctl);
 
 	imx_free_funcs(ipctl->info);
-	imx_free_pingroups(ipctl->info);
 }
 
 int imx_pinctrl_probe(struct platform_device *pdev,
@@ -886,15 +831,8 @@ int imx_pinctrl_probe(struct platform_device *pdev,
 
 	mutex_init(&info->mutex);
 
-	INIT_RADIX_TREE(&info->pgtree, GFP_KERNEL);
 	INIT_RADIX_TREE(&info->ftree, GFP_KERNEL);
 
-	ret = imx_pinctrl_probe_dt(pdev, info);
-	if (ret) {
-		dev_err(&pdev->dev, "fail to probe dt properties\n");
-		goto free;
-	}
-
 	ipctl->info = info;
 	ipctl->dev = info->dev;
 	platform_set_drvdata(pdev, ipctl);
@@ -906,6 +844,12 @@ int imx_pinctrl_probe(struct platform_device *pdev,
 		goto free;
 	}
 
+	ret = imx_pinctrl_probe_dt(pdev, ipctl);
+	if (ret) {
+		dev_err(&pdev->dev, "fail to probe dt properties\n");
+		goto free;
+	}
+
 	dev_info(&pdev->dev, "initialized IMX pinctrl driver\n");
 
 	return 0;
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h
index 62aa8f8f57e9..3c51db15223b 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -18,7 +18,7 @@
 struct platform_device;
 
 /**
- * struct imx_pin_group - describes a single i.MX pin
+ * struct imx_pin - describes a single i.MX pin
  * @pin: the pin_id of this pin
  * @mux_mode: the mux mode for this pin.
  * @input_reg: the select input register offset for this pin if any
@@ -35,21 +35,6 @@ struct imx_pin {
 };
 
 /**
- * struct imx_pin_group - describes an IMX pin group
- * @name: the name of this specific pin group
- * @npins: the number of pins in this group array, i.e. the number of
- *	elements in .pins so we can iterate over that array
- * @pin_ids: array of pin_ids. pinctrl forces us to maintain such an array
- * @pins: array of pins
- */
-struct imx_pin_group {
-	const char *name;
-	unsigned npins;
-	unsigned int *pin_ids;
-	struct imx_pin *pins;
-};
-
-/**
  * struct imx_pmx_func - describes IMX pinmux functions
  * @name: the name of this specific function
  * @groups: corresponding pin groups
@@ -76,13 +61,11 @@ struct imx_pinctrl_soc_info {
 	const struct pinctrl_pin_desc *pins;
 	unsigned int npins;
 	struct imx_pin_reg *pin_regs;
-	unsigned int ngroups;
 	unsigned int group_index;
 	unsigned int nfunctions;
 	unsigned int flags;
 	const char *gpr_compatible;
 	struct radix_tree_root ftree;
-	struct radix_tree_root pgtree;
 	struct mutex mutex;
 };
 
-- 
2.11.0

Re: [PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Linus Walleij]

On Mon, Jan 2, 2017 at 7:20 PM, Gary Bisson
<gary.bisson@boundarydevices.com> wrote:

> Now using group_desc structure instead of imx_pin_group.
>
> Also leveraging generic functions to retrieve groups count/name/pins.
>
> The imx_free_pingroups function can be removed since it is now handled by
> the core driver during unregister.
>
> Finally the device tree parsing is moved after the pinctrl driver registration
> since this latter initializes the radix trees.
>
> Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Fabio Estevam]

Hi Gary,

On Mon, Jan 2, 2017 at 4:20 PM, Gary Bisson
<gary.bisson@boundarydevices.com> wrote:
> Now using group_desc structure instead of imx_pin_group.
>
> Also leveraging generic functions to retrieve groups count/name/pins.
>
> The imx_free_pingroups function can be removed since it is now handled by
> the core driver during unregister.
>
> Finally the device tree parsing is moved after the pinctrl driver registration
> since this latter initializes the radix trees.
>
> Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>

This patch is in 4.11-rc1 now and it causes a regression on the hog group:

[    0.061005] imx6q-pinctrl 20e0000.iomuxc: unable to find group for
node hoggrp

The hog pins can no longer be initialized.

Could you take a look, please?

Re: [PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Gary Bisson]

Hi Fabio,

On Tue, Mar 7, 2017 at 6:19 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Gary,
>
> On Mon, Jan 2, 2017 at 4:20 PM, Gary Bisson
> <gary.bisson@boundarydevices.com> wrote:
>> Now using group_desc structure instead of imx_pin_group.
>>
>> Also leveraging generic functions to retrieve groups count/name/pins.
>>
>> The imx_free_pingroups function can be removed since it is now handled by
>> the core driver during unregister.
>>
>> Finally the device tree parsing is moved after the pinctrl driver registration
>> since this latter initializes the radix trees.
>>
>> Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>
>
> This patch is in 4.11-rc1 now and it causes a regression on the hog group:
>
> [    0.061005] imx6q-pinctrl 20e0000.iomuxc: unable to find group for
> node hoggrp
>
> The hog pins can no longer be initialized.
>
> Could you take a look, please?

No it isn't because of the use of radix but instead of:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=950b0d91dc10

It has been discussed here:
https://lkml.org/lkml/2017/2/28/140

Tony offered a patch in that thread which works. Tony, have you
submitted it yet?

Regards,
Gary

Re: [PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Fabio Estevam]

Hi Gary,

On Tue, Mar 7, 2017 at 2:26 PM, Gary Bisson
<gary.bisson@boundarydevices.com> wrote:

> No it isn't because of the use of radix but instead of:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=950b0d91dc10
>
> It has been discussed here:
> https://lkml.org/lkml/2017/2/28/140

Thanks for the clarification.

> Tony offered a patch in that thread which works. Tony, have you
> submitted it yet?

Looking forward to this one :-)

Thanks

Re: [PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Tony Lindgren]

* Fabio Estevam <festevam@gmail.com> [170307 09:39]:
> Hi Gary,
> 
> On Tue, Mar 7, 2017 at 2:26 PM, Gary Bisson
> <gary.bisson@boundarydevices.com> wrote:
> 
> > No it isn't because of the use of radix but instead of:
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=950b0d91dc10
> >
> > It has been discussed here:
> > https://lkml.org/lkml/2017/2/28/140
> 
> Thanks for the clarification.
> 
> > Tony offered a patch in that thread which works. Tony, have you
> > submitted it yet?
> 
> Looking forward to this one :-)

Still waiting to hear back from Linus on what he prefers.

Regards,

Tony

Re: [PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Linus Walleij]

On Tue, Mar 7, 2017 at 6:54 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Fabio Estevam <festevam@gmail.com> [170307 09:39]:
>> Hi Gary,
>>
>> On Tue, Mar 7, 2017 at 2:26 PM, Gary Bisson
>> <gary.bisson@boundarydevices.com> wrote:
>>
>> > No it isn't because of the use of radix but instead of:
>> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=950b0d91dc10
>> >
>> > It has been discussed here:
>> > https://lkml.org/lkml/2017/2/28/140
>>
>> Thanks for the clarification.
>>
>> > Tony offered a patch in that thread which works. Tony, have you
>> > submitted it yet?
>>
>> Looking forward to this one :-)
>
> Still waiting to hear back from Linus on what he prefers.

I am snowed under by mail and might be missing stuff at the
moment...

If there is some especially urgent fix patch I need to look into
can you please point it out?

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Fabio Estevam]

Tony/Linus

On Wed, Mar 15, 2017 at 6:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> Still waiting to hear back from Linus on what he prefers.
>
> I am snowed under by mail and might be missing stuff at the
> moment...
>
> If there is some especially urgent fix patch I need to look into
> can you please point it out?

Has this issue been solved in linux-next? Still seeing this regression
in 4.11-rc4.

Thanks

Re: [PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Tony Lindgren]

* Fabio Estevam <festevam@gmail.com> [170329 06:36]:
> Tony/Linus
> 
> On Wed, Mar 15, 2017 at 6:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> >> Still waiting to hear back from Linus on what he prefers.
> >
> > I am snowed under by mail and might be missing stuff at the
> > moment...
> >
> > If there is some especially urgent fix patch I need to look into
> > can you please point it out?
> 
> Has this issue been solved in linux-next? Still seeing this regression
> in 4.11-rc4.

Yeah I was wondering about that too. We discussed using Mika's patch
[0] but with minor changes [1]. But we have not heard back from Mika,
I wonder if he is travelling or something?

Would be nice to get this regression fixed.

Regards,

Tony

[0] https://lkml.org/lkml/2017/2/28/24
[1] https://lkml.org/lkml/2017/3/16/285

Re: [PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Linus Walleij]

On Wed, Mar 29, 2017 at 3:33 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Tony/Linus
>
> On Wed, Mar 15, 2017 at 6:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> Still waiting to hear back from Linus on what he prefers.
>>
>> I am snowed under by mail and might be missing stuff at the
>> moment...
>>
>> If there is some especially urgent fix patch I need to look into
>> can you please point it out?
>
> Has this issue been solved in linux-next? Still seeing this regression
> in 4.11-rc4.

I'm a bit relying on others to test and supply patches here,
if Tony's patch works for you you can send it with your
own Signed-off-by and suggest that I merge it.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: imx: use generic pinctrl helpers for managing groups

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [170330 02:08]:
> On Wed, Mar 29, 2017 at 3:33 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > Tony/Linus
> >
> > On Wed, Mar 15, 2017 at 6:46 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> >>> Still waiting to hear back from Linus on what he prefers.
> >>
> >> I am snowed under by mail and might be missing stuff at the
> >> moment...
> >>
> >> If there is some especially urgent fix patch I need to look into
> >> can you please point it out?
> >
> > Has this issue been solved in linux-next? Still seeing this regression
> > in 4.11-rc4.
> 
> I'm a bit relying on others to test and supply patches here,
> if Tony's patch works for you you can send it with your
> own Signed-off-by and suggest that I merge it.

I'll send a patch for testing shortly.

Regards,

Tony

[PATCH 2/2] pinctrl: imx: use generic pinmux helpers for managing functions

[Gary Bisson]

Now using function_desc structure instead of imx_pmx_func.

Also leveraging generic functions to retrieve functions count/name/groups.

The imx_free_funcs function can be removed since it is now handled by
the core driver during unregister.

Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>
---
 drivers/pinctrl/freescale/Kconfig       |  2 +-
 drivers/pinctrl/freescale/pinctrl-imx.c | 96 ++++++---------------------------
 drivers/pinctrl/freescale/pinctrl-imx.h | 14 -----
 3 files changed, 18 insertions(+), 94 deletions(-)

diff --git a/drivers/pinctrl/freescale/Kconfig b/drivers/pinctrl/freescale/Kconfig
index cb50e21615da..cae05e76c111 100644
--- a/drivers/pinctrl/freescale/Kconfig
+++ b/drivers/pinctrl/freescale/Kconfig
@@ -1,7 +1,7 @@
 config PINCTRL_IMX
 	bool
 	select GENERIC_PINCTRL_GROUPS
-	select PINMUX
+	select GENERIC_PINMUX_FUNCTIONS
 	select PINCONF
 	select REGMAP
 
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 62c20f661fed..bccd9416d44f 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -27,6 +27,7 @@
 #include <linux/regmap.h>
 
 #include "../core.h"
+#include "../pinmux.h"
 #include "pinctrl-imx.h"
 
 /* The bits in CONFIG cell defined in binding doc*/
@@ -161,7 +162,7 @@ static int imx_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
 	unsigned int npins, pin_id;
 	int i;
 	struct group_desc *grp = NULL;
-	struct imx_pmx_func *func = NULL;
+	struct function_desc *func = NULL;
 
 	/*
 	 * Configure the mux mode for each pin in the group for a specific
@@ -171,7 +172,7 @@ static int imx_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
 	if (!grp)
 		return -EINVAL;
 
-	func = radix_tree_lookup(&info->ftree, selector);
+	func = pinmux_generic_get_function(pctldev, selector);
 	if (!func)
 		return -EINVAL;
 
@@ -251,46 +252,6 @@ static int imx_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
 	return 0;
 }
 
-static int imx_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
-{
-	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
-	struct imx_pinctrl_soc_info *info = ipctl->info;
-
-	return info->nfunctions;
-}
-
-static const char *imx_pmx_get_func_name(struct pinctrl_dev *pctldev,
-					  unsigned selector)
-{
-	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
-	struct imx_pinctrl_soc_info *info = ipctl->info;
-	struct imx_pmx_func *func = NULL;
-
-	func = radix_tree_lookup(&info->ftree, selector);
-	if (!func)
-		return NULL;
-
-	return func->name;
-}
-
-static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
-			       const char * const **groups,
-			       unsigned * const num_groups)
-{
-	struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
-	struct imx_pinctrl_soc_info *info = ipctl->info;
-	struct imx_pmx_func *func = NULL;
-
-	func = radix_tree_lookup(&info->ftree, selector);
-	if (!func)
-		return -EINVAL;
-
-	*groups = func->groups;
-	*num_groups = func->num_groups;
-
-	return 0;
-}
-
 static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
 			struct pinctrl_gpio_range *range, unsigned offset)
 {
@@ -389,9 +350,9 @@ static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinmux_ops imx_pmx_ops = {
-	.get_functions_count = imx_pmx_get_funcs_count,
-	.get_function_name = imx_pmx_get_func_name,
-	.get_function_groups = imx_pmx_get_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 = imx_pmx_set,
 	.gpio_request_enable = imx_pmx_gpio_request_enable,
 	.gpio_disable_free = imx_pmx_gpio_disable_free,
@@ -603,28 +564,29 @@ static int imx_pinctrl_parse_functions(struct device_node *np,
 	struct pinctrl_dev *pctl = ipctl->pctl;
 	struct imx_pinctrl_soc_info *info = ipctl->info;
 	struct device_node *child;
-	struct imx_pmx_func *func;
+	struct function_desc *func;
 	struct group_desc *grp;
 	u32 i = 0;
 
 	dev_dbg(info->dev, "parse function(%d): %s\n", index, np->name);
 
-	func = radix_tree_lookup(&info->ftree, index);
+	func = pinmux_generic_get_function(pctl, index);
 	if (!func)
 		return -EINVAL;
 
 	/* Initialise function */
 	func->name = np->name;
-	func->num_groups = of_get_child_count(np);
-	if (func->num_groups == 0) {
+	func->num_group_names = of_get_child_count(np);
+	if (func->num_group_names == 0) {
 		dev_err(info->dev, "no groups defined in %s\n", np->full_name);
 		return -EINVAL;
 	}
-	func->groups = devm_kzalloc(info->dev,
-			func->num_groups * sizeof(char *), GFP_KERNEL);
+	func->group_names = devm_kzalloc(info->dev,
+					 func->num_group_names *
+					 sizeof(char *), GFP_KERNEL);
 
 	for_each_child_of_node(np, child) {
-		func->groups[i] = child->name;
+		func->group_names[i] = child->name;
 
 		grp = devm_kzalloc(info->dev, sizeof(struct group_desc),
 				   GFP_KERNEL);
@@ -691,7 +653,7 @@ static int imx_pinctrl_probe_dt(struct platform_device *pdev,
 	}
 
 	for (i = 0; i < nfuncs; i++) {
-		struct imx_pmx_func *function;
+		struct function_desc *function;
 
 		function = devm_kzalloc(&pdev->dev, sizeof(*function),
 					GFP_KERNEL);
@@ -699,10 +661,10 @@ static int imx_pinctrl_probe_dt(struct platform_device *pdev,
 			return -ENOMEM;
 
 		mutex_lock(&info->mutex);
-		radix_tree_insert(&info->ftree, i, function);
+		radix_tree_insert(&pctl->pin_function_tree, i, function);
 		mutex_unlock(&info->mutex);
 	}
-	info->nfunctions = nfuncs;
+	pctl->num_functions = nfuncs;
 
 	info->group_index = 0;
 	if (flat_funcs) {
@@ -725,26 +687,6 @@ static int imx_pinctrl_probe_dt(struct platform_device *pdev,
 }
 
 /*
- * imx_free_funcs() - free memory used by functions
- * @info: info driver instance
- */
-static void imx_free_funcs(struct imx_pinctrl_soc_info *info)
-{
-	int i;
-
-	mutex_lock(&info->mutex);
-	for (i = 0; i < info->nfunctions; i++) {
-		struct imx_pmx_func *func;
-
-		func = radix_tree_lookup(&info->ftree, i);
-		if (!func)
-			continue;
-		radix_tree_delete(&info->ftree, i);
-	}
-	mutex_unlock(&info->mutex);
-}
-
-/*
  * imx_free_resources() - free memory used by this driver
  * @info: info driver instance
  */
@@ -752,8 +694,6 @@ static void imx_free_resources(struct imx_pinctrl *ipctl)
 {
 	if (ipctl->pctl)
 		pinctrl_unregister(ipctl->pctl);
-
-	imx_free_funcs(ipctl->info);
 }
 
 int imx_pinctrl_probe(struct platform_device *pdev,
@@ -831,8 +771,6 @@ int imx_pinctrl_probe(struct platform_device *pdev,
 
 	mutex_init(&info->mutex);
 
-	INIT_RADIX_TREE(&info->ftree, GFP_KERNEL);
-
 	ipctl->info = info;
 	ipctl->dev = info->dev;
 	platform_set_drvdata(pdev, ipctl);
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h
index 3c51db15223b..ff2d3e56b7c5 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -35,18 +35,6 @@ struct imx_pin {
 };
 
 /**
- * struct imx_pmx_func - describes IMX pinmux functions
- * @name: the name of this specific function
- * @groups: corresponding pin groups
- * @num_groups: the number of groups
- */
-struct imx_pmx_func {
-	const char *name;
-	const char **groups;
-	unsigned num_groups;
-};
-
-/**
  * struct imx_pin_reg - describe a pin reg map
  * @mux_reg: mux register offset
  * @conf_reg: config register offset
@@ -62,10 +50,8 @@ struct imx_pinctrl_soc_info {
 	unsigned int npins;
 	struct imx_pin_reg *pin_regs;
 	unsigned int group_index;
-	unsigned int nfunctions;
 	unsigned int flags;
 	const char *gpr_compatible;
-	struct radix_tree_root ftree;
 	struct mutex mutex;
 };
 
-- 
2.11.0

Re: [PATCH 2/2] pinctrl: imx: use generic pinmux helpers for managing functions

[Linus Walleij]

On Mon, Jan 2, 2017 at 7:20 PM, Gary Bisson
<gary.bisson@boundarydevices.com> wrote:

> Now using function_desc structure instead of imx_pmx_func.
>
> Also leveraging generic functions to retrieve functions count/name/groups.
>
> The imx_free_funcs function can be removed since it is now handled by
> the core driver during unregister.
>
> Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 0/2] pinctrl: imx: use generic pinctrl helpers

[Linus Walleij]

On Mon, Jan 2, 2017 at 7:20 PM, Gary Bisson
<gary.bisson@boundarydevices.com> wrote:

> This series is a follow-up to Linus suggestion to use the generic helpers
> introduced by Tony [1].
>
> 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 special.

Awesome job. Really nice!

> 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 kept the
> radix_insert functions. This could be modified in the future though.

Let's see if Tony has some ideas. We already save a bunch of
code by this which is very nice.

Yours,
Linus Walleij

Re: [PATCH 0/2] pinctrl: imx: use generic pinctrl helpers

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [170102 13:44]:
> On Mon, Jan 2, 2017 at 7:20 PM, Gary Bisson
> <gary.bisson@boundarydevices.com> wrote:
> 
> > This series is a follow-up to Linus suggestion to use the generic helpers
> > introduced by Tony [1].
> >
> > 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 special.
> 
> Awesome job. Really nice!
> 
> > 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 kept the
> > radix_insert functions. This could be modified in the future though.
> 
> Let's see if Tony has some ideas. We already save a bunch of
> code by this which is very nice.

I attempted a generic iterator in the ti-iodelay.c but so far
did not really manage to simplify it at all..

Tony