[PATCH 0/5] Add generic pinctrl helpers for managing groups and functions

[PATCH 0/5] Add generic pinctrl helpers for managing groups and functions

[Tony Lindgren]

Hi all,

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

Regards,

Tony


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           | 233 ++++++++++++++++++++++++++++---
 drivers/pinctrl/core.h           |  67 +++++++++
 drivers/pinctrl/pinctrl-single.c | 289 ++++-----------------------------------
 drivers/pinctrl/pinmux.c         | 173 +++++++++++++++++++++++
 drivers/pinctrl/pinmux.h         |  42 ++++++
 6 files changed, 532 insertions(+), 283 deletions(-)

-- 
2.9.3

[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 hogs. This is
needed to be able to add pinctrl generic helper functions.

Note that the pinctrl functions already take care of the necessary
locking.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/core.c | 53 +++++++++++++++++++++++++++++++-------------------
 drivers/pinctrl/core.h |  2 ++
 2 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1738,6 +1738,35 @@ static int pinctrl_check_ops(struct pinctrl_dev *pctldev)
 }
 
 /**
+ * pinctrl_hog_work() - delayed work to set pincontroller self-claimed pins
+ * @work: work struct
+ */
+static void pinctrl_hog_work(struct work_struct *work)
+{
+	struct pinctrl_dev *pctldev;
+
+	pctldev = container_of(work, struct pinctrl_dev, hog_work.work);
+
+	pctldev->p = pinctrl_get(pctldev->dev);
+	if (IS_ERR(pctldev->p))
+		return;
+
+	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");
+}
+
+/**
  * pinctrl_register() - register a pin controller device
  * @pctldesc: descriptor for this pin controller
  * @dev: parent device for this pin controller
@@ -1766,6 +1795,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->hog_work, pinctrl_hog_work);
 	pctldev->dev = dev;
 	mutex_init(&pctldev->mutex);
 
@@ -1804,26 +1834,8 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	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");
-	}
+	schedule_delayed_work(&pctldev->hog_work,
+				      msecs_to_jiffies(100));
 
 	pinctrl_init_device_debugfs(pctldev);
 
@@ -1848,6 +1860,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
 	if (pctldev == NULL)
 		return;
 
+	cancel_delayed_work_sync(&pctldev->hog_work);
 	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
+ * @hog_work: delayed work for pin controller device claimed hog pins
  * @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 hog_work;
 	struct mutex mutex;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *device_root;
-- 
2.9.3

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

[Linus Walleij]

On Tue, Oct 25, 2016 at 11:02 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 hogs. This is
> needed to be able to add pinctrl generic helper functions.
>
> Note that the pinctrl functions already take care of the necessary
> locking.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

I don't see why this is necessary?

The hogging was placed inside pinctrl_register() so that any hogs
would be taken before it returns, so nothing else can take it
before the controller itself has the first chance. This semantic
needs to be preserved I think.

> +       schedule_delayed_work(&pctldev->hog_work,
> +                                     msecs_to_jiffies(100));

If we arbitrarily delay, something else can go in and take the
pins used by the hogs before the pinctrl core? That is what
we want to avoid.

Hm, 100ms seems arbitrarily chosen BTW. Can it be 1 ms?
1 ns?

I'm pretty sure that whatever it is that needs to happen before
the hog work runs can race with this delayed work under
some circumstances (such as slow external expanders
on i2c). It should be impossible for that to happen
and I don't think it is?

Yours,
Linus Walleij

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

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [161111 12:17]:
> On Tue, Oct 25, 2016 at 11:02 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 hogs. This is
> > needed to be able to add pinctrl generic helper functions.
> >
> > Note that the pinctrl functions already take care of the necessary
> > locking.
> >
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> I don't see why this is necessary?

It's needed because the pin controller driver has not yet
finished it's probe at this point. We end up calling functions
in the device driver where no struct pinctrl_dev is yet known
to the driver. Asking a device driver to do something before
it's probe is done does not quite follow the Linux driver model :)

> The hogging was placed inside pinctrl_register() so that any hogs
> would be taken before it returns, so nothing else can take it
> before the controller itself has the first chance. This semantic
> needs to be preserved I think.
> 
> > +       schedule_delayed_work(&pctldev->hog_work,
> > +                                     msecs_to_jiffies(100));
> 
> If we arbitrarily delay, something else can go in and take the
> pins used by the hogs before the pinctrl core? That is what
> we want to avoid.
> 
> Hm, 100ms seems arbitrarily chosen BTW. Can it be 1 ms?
> 1 ns?

Yeah well seems like it should not matter but the race we need
to remove somehow.

> I'm pretty sure that whatever it is that needs to happen before
> the hog work runs can race with this delayed work under
> some circumstances (such as slow external expanders
> on i2c). It should be impossible for that to happen
> and I don't think it is?

Yes it's totally possible even with delay set to 0.

Maybe we could add some trigger on the first consumer request
and if that does not happen use the timer?

Regards,

Tony

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [161111 12:27]:
> * Linus Walleij <linus.walleij@linaro.org> [161111 12:17]:
> > On Tue, Oct 25, 2016 at 11:02 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 hogs. This is
> > > needed to be able to add pinctrl generic helper functions.
> > >
> > > Note that the pinctrl functions already take care of the necessary
> > > locking.
> > >
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > 
> > I don't see why this is necessary?
> 
> It's needed because the pin controller driver has not yet
> finished it's probe at this point. We end up calling functions
> in the device driver where no struct pinctrl_dev is yet known
> to the driver. Asking a device driver to do something before
> it's probe is done does not quite follow the Linux driver model :)

To clarify, that's an issue with multiple instances of the same
driver probing as there's no static pointer to driver specific
data.

Regards,

Tony

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

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [161111 12:32]:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [161111 12:27]:
> > * Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> [161111 12:17]:
> > > On Tue, Oct 25, 2016 at 11:02 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> 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 hogs. This is
> > > > needed to be able to add pinctrl generic helper functions.
> > > >
> > > > Note that the pinctrl functions already take care of the necessary
> > > > locking.
> > > >
> > > > Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > > 
> > > I don't see why this is necessary?
> > 
> > It's needed because the pin controller driver has not yet
> > finished it's probe at this point. We end up calling functions
> > in the device driver where no struct pinctrl_dev is yet known
> > to the driver. Asking a device driver to do something before
> > it's probe is done does not quite follow the Linux driver model :)
> 
> To clarify, that's an issue with multiple instances of the same
> driver probing as there's no static pointer to driver specific
> data.

To clarify even more, the following patches in this series need
struct pinctrl_dev to pass to the generic functions :)

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [161111 12:27]:
> * Linus Walleij <linus.walleij@linaro.org> [161111 12:17]:
> > On Tue, Oct 25, 2016 at 11:02 PM, Tony Lindgren <tony@atomide.com> wrote:
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > 
> > I don't see why this is necessary?
> 
> It's needed because the pin controller driver has not yet
> finished it's probe at this point. We end up calling functions
> in the device driver where no struct pinctrl_dev is yet known
> to the driver. Asking a device driver to do something before
> it's probe is done does not quite follow the Linux driver model :)
> 
> > The hogging was placed inside pinctrl_register() so that any hogs
> > would be taken before it returns, so nothing else can take it
> > before the controller itself has the first chance. This semantic
> > needs to be preserved I think.
> > 
> > > +       schedule_delayed_work(&pctldev->hog_work,
> > > +                                     msecs_to_jiffies(100));
> > 
> > If we arbitrarily delay, something else can go in and take the
> > pins used by the hogs before the pinctrl core? That is what
> > we want to avoid.
> > 
> > Hm, 100ms seems arbitrarily chosen BTW. Can it be 1 ms?
> > 1 ns?
> 
> Yeah well seems like it should not matter but the race we need
> to remove somehow.
> 
> > I'm pretty sure that whatever it is that needs to happen before
> > the hog work runs can race with this delayed work under
> > some circumstances (such as slow external expanders
> > on i2c). It should be impossible for that to happen
> > and I don't think it is?
> 
> Yes it's totally possible even with delay set to 0.
> 
> Maybe we could add some trigger on the first consumer request
> and if that does not happen use the timer?

Below is what I came up with for removing the race for hogs. We
can do it by not registering the pctldev until in the deferred
work, does that seem OK to you?

Regards,

Tony

8<-----------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Tue, 25 Oct 2016 08:33:35 -0700
Subject: [PATCH] pinctrl: core: Use delayed work for hogs

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.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/pinctrl/core.c | 66 ++++++++++++++++++++++++++++++--------------------
 drivers/pinctrl/core.h |  2 ++
 2 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1911,6 +1911,43 @@ 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);
+	if (IS_ERR(pctldev->p))
+		return;
+
+	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
@@ -1941,6 +1978,7 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
 	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;
 	mutex_init(&pctldev->mutex);
 
@@ -1975,32 +2013,7 @@ 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);
+	schedule_delayed_work(&pctldev->late_init, 0);
 
 	return pctldev;
 
@@ -2023,6 +2036,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
@@ -37,6 +37,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
  */
@@ -55,6 +56,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;
-- 
2.10.2

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

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [161114 12:54]:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [161111 12:27]:
> > * Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> [161111 12:17]:
> > > On Tue, Oct 25, 2016 at 11:02 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > > > Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > > 
> > > I don't see why this is necessary?
> > 
> > It's needed because the pin controller driver has not yet
> > finished it's probe at this point. We end up calling functions
> > in the device driver where no struct pinctrl_dev is yet known
> > to the driver. Asking a device driver to do something before
> > it's probe is done does not quite follow the Linux driver model :)
> > 
> > > The hogging was placed inside pinctrl_register() so that any hogs
> > > would be taken before it returns, so nothing else can take it
> > > before the controller itself has the first chance. This semantic
> > > needs to be preserved I think.
> > > 
> > > > +       schedule_delayed_work(&pctldev->hog_work,
> > > > +                                     msecs_to_jiffies(100));
> > > 
> > > If we arbitrarily delay, something else can go in and take the
> > > pins used by the hogs before the pinctrl core? That is what
> > > we want to avoid.
> > > 
> > > Hm, 100ms seems arbitrarily chosen BTW. Can it be 1 ms?
> > > 1 ns?
> > 
> > Yeah well seems like it should not matter but the race we need
> > to remove somehow.
> > 
> > > I'm pretty sure that whatever it is that needs to happen before
> > > the hog work runs can race with this delayed work under
> > > some circumstances (such as slow external expanders
> > > on i2c). It should be impossible for that to happen
> > > and I don't think it is?
> > 
> > Yes it's totally possible even with delay set to 0.
> > 
> > Maybe we could add some trigger on the first consumer request
> > and if that does not happen use the timer?
> 
> Below is what I came up with for removing the race for hogs. We
> can do it by not registering the pctldev until in the deferred
> work, does that seem OK to you?

Oops, that does not yet work, will have to look into it more.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [161114 14:09]:
> * Tony Lindgren <tony@atomide.com> [161114 12:54]:
> > * Tony Lindgren <tony@atomide.com> [161111 12:27]:
> > > * Linus Walleij <linus.walleij@linaro.org> [161111 12:17]:
> > > > On Tue, Oct 25, 2016 at 11:02 PM, Tony Lindgren <tony@atomide.com> wrote:
> > > > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > > 
> > > > I don't see why this is necessary?
> > > 
> > > It's needed because the pin controller driver has not yet
> > > finished it's probe at this point. We end up calling functions
> > > in the device driver where no struct pinctrl_dev is yet known
> > > to the driver. Asking a device driver to do something before
> > > it's probe is done does not quite follow the Linux driver model :)
> > > 
> > > > The hogging was placed inside pinctrl_register() so that any hogs
> > > > would be taken before it returns, so nothing else can take it
> > > > before the controller itself has the first chance. This semantic
> > > > needs to be preserved I think.
> > > > 
> > > > > +       schedule_delayed_work(&pctldev->hog_work,
> > > > > +                                     msecs_to_jiffies(100));
> > > > 
> > > > If we arbitrarily delay, something else can go in and take the
> > > > pins used by the hogs before the pinctrl core? That is what
> > > > we want to avoid.
> > > > 
> > > > Hm, 100ms seems arbitrarily chosen BTW. Can it be 1 ms?
> > > > 1 ns?
> > > 
> > > Yeah well seems like it should not matter but the race we need
> > > to remove somehow.
> > > 
> > > > I'm pretty sure that whatever it is that needs to happen before
> > > > the hog work runs can race with this delayed work under
> > > > some circumstances (such as slow external expanders
> > > > on i2c). It should be impossible for that to happen
> > > > and I don't think it is?
> > > 
> > > Yes it's totally possible even with delay set to 0.
> > > 
> > > Maybe we could add some trigger on the first consumer request
> > > and if that does not happen use the timer?
> > 
> > Below is what I came up with for removing the race for hogs. We
> > can do it by not registering the pctldev until in the deferred
> > work, does that seem OK to you?
> 
> Oops, that does not yet work, will have to look into it more.

We need to pass pctldev to the parsers if we want to not add the
the controller to the list until hogs are claimed. Otherwise the
device tree parsers won't find the controller. The patch below has
that added.

Regards,

Tony

8< --------------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Tue, 25 Oct 2016 08:33:35 -0700
Subject: [PATCH] pinctrl: core: Use delayed work for hogs

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       | 87 ++++++++++++++++++++++++++++----------------
 drivers/pinctrl/core.h       |  2 +
 drivers/pinctrl/devicetree.c | 13 ++++---
 drivers/pinctrl/devicetree.h |  5 ++-
 4 files changed, 68 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,7 @@ 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);
+	schedule_delayed_work(&pctldev->late_init, 0);
 
 	return pctldev;
 
@@ -1848,6 +1870,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,7 @@ 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)
+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 +235,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
@@ -21,7 +21,7 @@ struct of_phandle_args;
 #ifdef CONFIG_OF
 
 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 +32,8 @@ int pinctrl_parse_index_with_args(const struct device_node *np,
 
 #else
 
-static inline int pinctrl_dt_to_map(struct pinctrl *p)
+static inline int pinctrl_dt_to_map(struct pinctrl *p,
+				    struct pinctrl_dev *pctldev)
 {
 	return 0;
 }
-- 
2.10.2

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

[Linus Walleij]

On Tue, Nov 15, 2016 at 1:47 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:

> 8< --------------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Date: Tue, 25 Oct 2016 08:33:35 -0700
> Subject: [PATCH] pinctrl: core: Use delayed work for hogs
>
> 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-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

This looks a lot better!

So if I understand correctly, we can guarantee that the delayed
work will not execute until the device driver probe() has finished,
and it *will* execute immediately after that?

So:
- Device driver probes
- Delayed work is called
- Next initcall

I'm not 100% familiar with how delayed work works... :/

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tony Lindgren]

* Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> [161114 22:53]:
> On Tue, Nov 15, 2016 at 1:47 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > 8< --------------------------------
> > From tony Mon Sep 17 00:00:00 2001
> > From: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> > Date: Tue, 25 Oct 2016 08:33:35 -0700
> > Subject: [PATCH] pinctrl: core: Use delayed work for hogs
> >
> > 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-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> 
> This looks a lot better!
> 
> So if I understand correctly, we can guarantee that the delayed
> work will not execute until the device driver probe() has finished,
> and it *will* execute immediately after that?
> 
> So:
> - Device driver probes
> - Delayed work is called
> - Next initcall
> 
> I'm not 100% familiar with how delayed work works... :/

Yeah well the delayed work gets scheduled for next jiffy but may
be pre-empted as it runs in process context.

So in the worst case it could that we still may need to fix few
drivers to support -EPROBE_DEFER. I wonder if we should check for
hogs in probe already and only defer if hogs are defined?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [161115 07:42]:
> * Linus Walleij <linus.walleij@linaro.org> [161114 22:53]:
> > On Tue, Nov 15, 2016 at 1:47 AM, Tony Lindgren <tony@atomide.com> wrote:
> > 
> > > 8< --------------------------------
> > > From tony Mon Sep 17 00:00:00 2001
> > > From: Tony Lindgren <tony@atomide.com>
> > > Date: Tue, 25 Oct 2016 08:33:35 -0700
> > > Subject: [PATCH] pinctrl: core: Use delayed work for hogs
> > >
> > > 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>
> > 
> > This looks a lot better!
> > 
> > So if I understand correctly, we can guarantee that the delayed
> > work will not execute until the device driver probe() has finished,
> > and it *will* execute immediately after that?
> > 
> > So:
> > - Device driver probes
> > - Delayed work is called
> > - Next initcall
> > 
> > I'm not 100% familiar with how delayed work works... :/
> 
> Yeah well the delayed work gets scheduled for next jiffy but may
> be pre-empted as it runs in process context.
> 
> So in the worst case it could that we still may need to fix few
> drivers to support -EPROBE_DEFER. I wonder if we should check for
> hogs in probe already and only defer if hogs are defined?

Below is a version using delayed_work only if pinctrl_dt_has_hogs().

Not sure if testing only for pinctrl-0 is enough there though?

Regards,

Tony

8< --------------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Tue, 25 Oct 2016 08:33:35 -0700
Subject: [PATCH] pinctrl: core: Use delayed work for hogs

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

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

[Linus Walleij]

On Tue, Nov 15, 2016 at 6:08 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Tony Lindgren <tony@atomide.com> [161115 07:42]:
>> * Linus Walleij <linus.walleij@linaro.org> [161114 22:53]:
>> > On Tue, Nov 15, 2016 at 1:47 AM, Tony Lindgren <tony@atomide.com> wrote:
>> >
>> > > 8< --------------------------------
>> > > From tony Mon Sep 17 00:00:00 2001
>> > > From: Tony Lindgren <tony@atomide.com>
>> > > Date: Tue, 25 Oct 2016 08:33:35 -0700
>> > > Subject: [PATCH] pinctrl: core: Use delayed work for hogs
>> > >
>> > > 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>
>> >
>> > This looks a lot better!
>> >
>> > So if I understand correctly, we can guarantee that the delayed
>> > work will not execute until the device driver probe() has finished,
>> > and it *will* execute immediately after that?
>> >
>> > So:
>> > - Device driver probes
>> > - Delayed work is called
>> > - Next initcall
>> >
>> > I'm not 100% familiar with how delayed work works... :/
>>
>> Yeah well the delayed work gets scheduled for next jiffy but may
>> be pre-empted as it runs in process context.
>>
>> So in the worst case it could that we still may need to fix few
>> drivers to support -EPROBE_DEFER. I wonder if we should check for
>> hogs in probe already and only defer if hogs are defined?
>
> Below is a version using delayed_work only if pinctrl_dt_has_hogs().
>
> Not sure if testing only for pinctrl-0 is enough there though?

Sorry for the lack of attention to this patch set on my part. :(

Do you think you could resend these last 5 patches after the
release of v4.10-rc1 so we merge it early for the next cycle
and people get a chance to test and see if it works well for
everyone?

I'm worried about adding it to the tree this late in the kernel
cycle...

However I like the look of the series overall a lot.

Yours,
Linus Walleij

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

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [161202 05:08]:
> On Tue, Nov 15, 2016 at 6:08 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Tony Lindgren <tony@atomide.com> [161115 07:42]:
> >> * Linus Walleij <linus.walleij@linaro.org> [161114 22:53]:
> >> > On Tue, Nov 15, 2016 at 1:47 AM, Tony Lindgren <tony@atomide.com> wrote:
> >> >
> >> > > 8< --------------------------------
> >> > > From tony Mon Sep 17 00:00:00 2001
> >> > > From: Tony Lindgren <tony@atomide.com>
> >> > > Date: Tue, 25 Oct 2016 08:33:35 -0700
> >> > > Subject: [PATCH] pinctrl: core: Use delayed work for hogs
> >> > >
> >> > > 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>
> >> >
> >> > This looks a lot better!
> >> >
> >> > So if I understand correctly, we can guarantee that the delayed
> >> > work will not execute until the device driver probe() has finished,
> >> > and it *will* execute immediately after that?
> >> >
> >> > So:
> >> > - Device driver probes
> >> > - Delayed work is called
> >> > - Next initcall
> >> >
> >> > I'm not 100% familiar with how delayed work works... :/
> >>
> >> Yeah well the delayed work gets scheduled for next jiffy but may
> >> be pre-empted as it runs in process context.
> >>
> >> So in the worst case it could that we still may need to fix few
> >> drivers to support -EPROBE_DEFER. I wonder if we should check for
> >> hogs in probe already and only defer if hogs are defined?
> >
> > Below is a version using delayed_work only if pinctrl_dt_has_hogs().
> >
> > Not sure if testing only for pinctrl-0 is enough there though?
> 
> Sorry for the lack of attention to this patch set on my part. :(
> 
> Do you think you could resend these last 5 patches after the
> release of v4.10-rc1 so we merge it early for the next cycle
> and people get a chance to test and see if it works well for
> everyone?

Yeah no problem, too late to do anything with them right now :)

> I'm worried about adding it to the tree this late in the kernel
> cycle...

Yup me too.

> However I like the look of the series overall a lot.

OK good to hear.

Tony

[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
@@ -1794,6 +1970,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->hog_work, pinctrl_hog_work);
 	pctldev->dev = dev;
@@ -1872,6 +2049,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.9.3

[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
@@ -1971,6 +1971,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->hog_work, pinctrl_hog_work);
 	pctldev->dev = dev;
@@ -2049,6 +2050,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.9.3

[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 | 155 +++------------------------------------
 2 files changed, 11 insertions(+), 146 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -157,8 +157,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)
@@ -368,9 +298,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,
@@ -684,7 +614,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++) {
@@ -706,7 +636,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++) {
@@ -896,40 +826,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
@@ -1099,10 +995,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
@@ -1183,7 +1078,7 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 	if (!function)
 		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;
 
@@ -1202,7 +1097,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);
@@ -1315,7 +1210,7 @@ static int pcs_parse_bits_in_pinctrl_entry(struct pcs_device *pcs,
 	if (!function)
 		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;
 
@@ -1332,7 +1227,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);
@@ -1431,33 +1326,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
  */
@@ -1486,7 +1354,6 @@ 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 (pcs->missing_nr_pinctrl_cells)
 		of_remove_property(pcs->np, pcs->missing_nr_pinctrl_cells);
 }
@@ -1874,7 +1741,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;
@@ -1936,7 +1802,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.9.3

[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
@@ -158,7 +158,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);
@@ -306,59 +301,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. */
@@ -366,7 +315,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);
@@ -379,6 +329,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;
 
@@ -386,7 +337,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;
 
@@ -444,9 +396,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,
 };
@@ -788,43 +740,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
@@ -1100,7 +1033,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);
@@ -1230,8 +1163,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);
 
@@ -1299,33 +1231,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
  */
@@ -1353,7 +1258,6 @@ static void pcs_free_resources(struct pcs_device *pcs)
 {
 	pcs_irq_free(pcs);
 	pinctrl_unregister(pcs->pctl);
-	pcs_free_funcs(pcs);
 	if (pcs->missing_nr_pinctrl_cells)
 		of_remove_property(pcs->np, pcs->missing_nr_pinctrl_cells);
 }
@@ -1741,7 +1645,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;
@@ -1802,7 +1705,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.9.3

[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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> 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-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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