[PATCH 01/10] pinctrl: use postcore_initcall

[PATCH 01/10] pinctrl: use postcore_initcall

[Haojian Zhuang]

Since pins are configured in device driver, pinctrl driver should be
loaded by those device driver. module_platform_driver() only declares
pinctrl driver is in module_initcall privilege. Use postcore_initcall
privilege instead.

Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pinctrl/pinctrl-single.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 76a4260..64d109a 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -980,7 +980,17 @@ static struct platform_driver pcs_driver = {
 	},
 };
 
-module_platform_driver(pcs_driver);
+static int __init pcs_driver_init(void)
+{
+	return platform_driver_register(&pcs_driver);
+}
+postcore_initcall(pcs_driver_init);
+
+static void __exit pcs_driver_exit(void)
+{
+	platform_driver_unregister(&pcs_driver);
+}
+module_exit(pcs_driver_exit);
 
 MODULE_AUTHOR("Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>");
 MODULE_DESCRIPTION("One-register-per-pin type device tree based pinctrl driver");
-- 
1.7.0.4

[PATCH 01/10] pinctrl: use postcore_initcall

[Haojian Zhuang]

Since pins are configured in device driver, pinctrl driver should be
loaded by those device driver. module_platform_driver() only declares
pinctrl driver is in module_initcall privilege. Use postcore_initcall
privilege instead.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/pinctrl/pinctrl-single.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 76a4260..64d109a 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -980,7 +980,17 @@ static struct platform_driver pcs_driver = {
 	},
 };
 
-module_platform_driver(pcs_driver);
+static int __init pcs_driver_init(void)
+{
+	return platform_driver_register(&pcs_driver);
+}
+postcore_initcall(pcs_driver_init);
+
+static void __exit pcs_driver_exit(void)
+{
+	platform_driver_unregister(&pcs_driver);
+}
+module_exit(pcs_driver_exit);
 
 MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>");
 MODULE_DESCRIPTION("One-register-per-pin type device tree based pinctrl driver");
-- 
1.7.0.4

[PATCH 02/10] ARM: mmp: select pinctrl driver

[Haojian Zhuang]

Pinctrl driver is necessary for MMP DT & MMP2 DT platforms.

Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/mach-mmp/Kconfig |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-mmp/Kconfig b/arch/arm/mach-mmp/Kconfig
index 7fddd01..c70c787 100644
--- a/arch/arm/mach-mmp/Kconfig
+++ b/arch/arm/mach-mmp/Kconfig
@@ -89,6 +89,7 @@ config MACH_MMP_DT
 	select CPU_PXA168
 	select CPU_PXA910
 	select USE_OF
+	select PINCTRL
 	help
 	  Include support for Marvell MMP2 based platforms using
 	  the device tree. Needn't select any other machine while
@@ -99,6 +100,7 @@ config MACH_MMP2_DT
 	depends on !CPU_MOHAWK
 	select CPU_MMP2
 	select USE_OF
+	select PINCTRL
 	help
 	  Include support for Marvell MMP2 based platforms using
 	  the device tree.
-- 
1.7.0.4

[PATCH 02/10] ARM: mmp: select pinctrl driver

[Haojian Zhuang]

Pinctrl driver is necessary for MMP DT & MMP2 DT platforms.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 arch/arm/mach-mmp/Kconfig |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-mmp/Kconfig b/arch/arm/mach-mmp/Kconfig
index 7fddd01..c70c787 100644
--- a/arch/arm/mach-mmp/Kconfig
+++ b/arch/arm/mach-mmp/Kconfig
@@ -89,6 +89,7 @@ config MACH_MMP_DT
 	select CPU_PXA168
 	select CPU_PXA910
 	select USE_OF
+	select PINCTRL
 	help
 	  Include support for Marvell MMP2 based platforms using
 	  the device tree. Needn't select any other machine while
@@ -99,6 +100,7 @@ config MACH_MMP2_DT
 	depends on !CPU_MOHAWK
 	select CPU_MMP2
 	select USE_OF
+	select PINCTRL
 	help
 	  Include support for Marvell MMP2 based platforms using
 	  the device tree.
-- 
1.7.0.4

[PATCH 03/10] tty: pxa: configure pin

[Haojian Zhuang]

Configure pins by pinctrl driver.

Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/tty/serial/pxa.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index 5847a4b..ee6118a 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c
@@ -37,6 +37,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
@@ -795,6 +796,7 @@ static int serial_pxa_probe_dt(struct platform_device *pdev,
 			       struct uart_pxa_port *sport)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct pinctrl *pinctrl;
 	int ret;
 
 	if (!np)
@@ -805,6 +807,12 @@ static int serial_pxa_probe_dt(struct platform_device *pdev,
 		dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret);
 		return ret;
 	}
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl)) {
+		ret = PTR_ERR(pinctrl);
+		return ret;
+	}
+
 	sport->port.line = ret;
 	return 0;
 }
-- 
1.7.0.4

[PATCH 03/10] tty: pxa: configure pin

[Haojian Zhuang]

Configure pins by pinctrl driver.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/tty/serial/pxa.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index 5847a4b..ee6118a 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c
@@ -37,6 +37,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
@@ -795,6 +796,7 @@ static int serial_pxa_probe_dt(struct platform_device *pdev,
 			       struct uart_pxa_port *sport)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct pinctrl *pinctrl;
 	int ret;
 
 	if (!np)
@@ -805,6 +807,12 @@ static int serial_pxa_probe_dt(struct platform_device *pdev,
 		dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret);
 		return ret;
 	}
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl)) {
+		ret = PTR_ERR(pinctrl);
+		return ret;
+	}
+
 	sport->port.line = ret;
 	return 0;
 }
-- 
1.7.0.4

Re: [PATCH 03/10] tty: pxa: configure pin

[Linus Walleij]

On Thu, Oct 18, 2012 at 11:06 AM, Haojian Zhuang
<haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Configure pins by pinctrl driver.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij

[PATCH 03/10] tty: pxa: configure pin

[Linus Walleij]

On Thu, Oct 18, 2012 at 11:06 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:

> Configure pins by pinctrl driver.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 03/10] tty: pxa: configure pin

[Stephen Warren]

On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
> Configure pins by pinctrl driver.

In general, it seems better to use pinctrl "hogs" if the driver is only
going to statically set up one pinctrl state and never change it. The
alternative is make every single driver execute these pinctrl calls,
which could be tedious.

However, that does raise one question: If all the pinctrl setup is done
by hogs, then how do we ensure that the pinctrl driver is probed first,
and hence sets up the pins before any driver relies on them being set
up? If a driver explicitly enables a pinctrl state (as in this patch),
then deferred probe ensures that, but without any explicit pinctrl
action, it'll only work by luck.

LinusW, what are your thoughts on that?

[PATCH 03/10] tty: pxa: configure pin

[Stephen Warren]

On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
> Configure pins by pinctrl driver.

In general, it seems better to use pinctrl "hogs" if the driver is only
going to statically set up one pinctrl state and never change it. The
alternative is make every single driver execute these pinctrl calls,
which could be tedious.

However, that does raise one question: If all the pinctrl setup is done
by hogs, then how do we ensure that the pinctrl driver is probed first,
and hence sets up the pins before any driver relies on them being set
up? If a driver explicitly enables a pinctrl state (as in this patch),
then deferred probe ensures that, but without any explicit pinctrl
action, it'll only work by luck.

LinusW, what are your thoughts on that?

Re: [PATCH 03/10] tty: pxa: configure pin

[Linus Walleij]

On Fri, Oct 19, 2012 at 12:20 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
>> Configure pins by pinctrl driver.
>
> In general, it seems better to use pinctrl "hogs" if the driver is only
> going to statically set up one pinctrl state and never change it. The
> alternative is make every single driver execute these pinctrl calls,
> which could be tedious.

True. And each platform has to choose how to go ahead with this.

I always imagined that any system of the "power socket in wall"
type would just use hogs to configure its pins and be done with
it, and there appear to be some platforms like that. (e.g. MIPS and
various power-inaware references come to mind).

For the Ux500 drivers we have found that all of those that have pin
resources actually have to be handled by the driver. The reason is
that all of them have idle and/or sleep states that need to be
handled at runtime.

As an additional complication our drivers are used also by
the Versatile and SPEAr family of platforms, so the control
need to be tolerant (accept missing pinctrl handles and states)
as well. (I can see that other drivers take shortcuts by being less
elaborate since there is a 1:1 driver<->platform mapping.)

An alternative to embedding the pin handling code into
the drivers is to use bus notifiers as is done with the
shmobile clocking by drivers/base/power/clock_ops.c
I could easily imagine pinctrl_ops.c like that for some
platforms. This mandates still binding the pin table entries
to a device but avoids adding any code to the drivers.

However this latter approach does not work for us (Ux500) -
the three resources we have: clocks, pins and power domains
are dependent on state switch ordering (sometimes you need
to switch off the clock then set pin state, sometimes the
other way around) and it is not even
the same for all drivers - the notifier approach mandates
that all drivers do the clock, power domain and pin handling
uniformly.

> However, that does raise one question: If all the pinctrl setup is done
> by hogs, then how do we ensure that the pinctrl driver is probed first,
> and hence sets up the pins before any driver relies on them being set
> up? If a driver explicitly enables a pinctrl state (as in this patch),
> then deferred probe ensures that, but without any explicit pinctrl
> action, it'll only work by luck.

Yes, since there are no explicit dependencies with hogs
it is implicitly decoupled and you only know that the hogging will
happen whenever the pin controller driver is probed.

We have many such pieces of code in the kernel for sure,
but I agree it's not always very elegant :-/

If using the bus nofifier approach I described above the
listener can just listen to BUS_NOTIFY_BIND_DRIVER
and BUS_NOTIFY_UNBOUND_DRIVER and hog/unhog
the drivers' pins at this point, but as described this approach
has other drawbacks.

Yours,
Linus Walleij

[PATCH 03/10] tty: pxa: configure pin

[Linus Walleij]

On Fri, Oct 19, 2012 at 12:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
>> Configure pins by pinctrl driver.
>
> In general, it seems better to use pinctrl "hogs" if the driver is only
> going to statically set up one pinctrl state and never change it. The
> alternative is make every single driver execute these pinctrl calls,
> which could be tedious.

True. And each platform has to choose how to go ahead with this.

I always imagined that any system of the "power socket in wall"
type would just use hogs to configure its pins and be done with
it, and there appear to be some platforms like that. (e.g. MIPS and
various power-inaware references come to mind).

For the Ux500 drivers we have found that all of those that have pin
resources actually have to be handled by the driver. The reason is
that all of them have idle and/or sleep states that need to be
handled at runtime.

As an additional complication our drivers are used also by
the Versatile and SPEAr family of platforms, so the control
need to be tolerant (accept missing pinctrl handles and states)
as well. (I can see that other drivers take shortcuts by being less
elaborate since there is a 1:1 driver<->platform mapping.)

An alternative to embedding the pin handling code into
the drivers is to use bus notifiers as is done with the
shmobile clocking by drivers/base/power/clock_ops.c
I could easily imagine pinctrl_ops.c like that for some
platforms. This mandates still binding the pin table entries
to a device but avoids adding any code to the drivers.

However this latter approach does not work for us (Ux500) -
the three resources we have: clocks, pins and power domains
are dependent on state switch ordering (sometimes you need
to switch off the clock then set pin state, sometimes the
other way around) and it is not even
the same for all drivers - the notifier approach mandates
that all drivers do the clock, power domain and pin handling
uniformly.

> However, that does raise one question: If all the pinctrl setup is done
> by hogs, then how do we ensure that the pinctrl driver is probed first,
> and hence sets up the pins before any driver relies on them being set
> up? If a driver explicitly enables a pinctrl state (as in this patch),
> then deferred probe ensures that, but without any explicit pinctrl
> action, it'll only work by luck.

Yes, since there are no explicit dependencies with hogs
it is implicitly decoupled and you only know that the hogging will
happen whenever the pin controller driver is probed.

We have many such pieces of code in the kernel for sure,
but I agree it's not always very elegant :-/

If using the bus nofifier approach I described above the
listener can just listen to BUS_NOTIFY_BIND_DRIVER
and BUS_NOTIFY_UNBOUND_DRIVER and hog/unhog
the drivers' pins at this point, but as described this approach
has other drawbacks.

Yours,
Linus Walleij

Re: [PATCH 03/10] tty: pxa: configure pin

[Stephen Warren]

On 10/22/2012 02:45 AM, Linus Walleij wrote:
> On Fri, Oct 19, 2012 at 12:20 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
>> On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
>>> Configure pins by pinctrl driver.
>>
>> In general, it seems better to use pinctrl "hogs" if the driver is only
>> going to statically set up one pinctrl state and never change it. The
>> alternative is make every single driver execute these pinctrl calls,
>> which could be tedious.
> 
> True. And each platform has to choose how to go ahead with this.
> 
> I always imagined that any system of the "power socket in wall"
> type would just use hogs to configure its pins and be done with
> it, and there appear to be some platforms like that. (e.g. MIPS and
> various power-inaware references come to mind).
> 
> For the Ux500 drivers we have found that all of those that have pin
> resources actually have to be handled by the driver. The reason is
> that all of them have idle and/or sleep states that need to be
> handled at runtime.

Well, presumably the pinctrl driver itself could have both default/idle
states, and system sleep could engage the appropriate system-wide setting.

> As an additional complication our drivers are used also by
> the Versatile and SPEAr family of platforms, so the control
> need to be tolerant (accept missing pinctrl handles and states)
> as well. (I can see that other drivers take shortcuts by being less
> elaborate since there is a 1:1 driver<->platform mapping.)

Isn't the driver (or DT binding) supposed to define what pinctrl states
must exist, and those state must always exist? There's no requirement
for a pinctrl state definition to always actually contain content that
changes the state. That's exactly why PIN_MAP_TYPE_DUMMY_STATE exists.

> An alternative to embedding the pin handling code into
> the drivers is to use bus notifiers as is done with the
> shmobile clocking by drivers/base/power/clock_ops.c
> I could easily imagine pinctrl_ops.c like that for some
> platforms. This mandates still binding the pin table entries
> to a device but avoids adding any code to the drivers.
> 
> However this latter approach does not work for us (Ux500) -
> the three resources we have: clocks, pins and power domains
> are dependent on state switch ordering (sometimes you need
> to switch off the clock then set pin state, sometimes the
> other way around) and it is not even
> the same for all drivers - the notifier approach mandates
> that all drivers do the clock, power domain and pin handling
> uniformly.

That certainly does imply that individual drivers do need to handle this
explicitly. Although I still think that only specific drivers actually
affected by this should end up actively managing pinctrl; shouldn't
anything that can get away with relying on system hogs do so?

[PATCH 03/10] tty: pxa: configure pin

[Stephen Warren]

On 10/22/2012 02:45 AM, Linus Walleij wrote:
> On Fri, Oct 19, 2012 at 12:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
>>> Configure pins by pinctrl driver.
>>
>> In general, it seems better to use pinctrl "hogs" if the driver is only
>> going to statically set up one pinctrl state and never change it. The
>> alternative is make every single driver execute these pinctrl calls,
>> which could be tedious.
> 
> True. And each platform has to choose how to go ahead with this.
> 
> I always imagined that any system of the "power socket in wall"
> type would just use hogs to configure its pins and be done with
> it, and there appear to be some platforms like that. (e.g. MIPS and
> various power-inaware references come to mind).
> 
> For the Ux500 drivers we have found that all of those that have pin
> resources actually have to be handled by the driver. The reason is
> that all of them have idle and/or sleep states that need to be
> handled at runtime.

Well, presumably the pinctrl driver itself could have both default/idle
states, and system sleep could engage the appropriate system-wide setting.

> As an additional complication our drivers are used also by
> the Versatile and SPEAr family of platforms, so the control
> need to be tolerant (accept missing pinctrl handles and states)
> as well. (I can see that other drivers take shortcuts by being less
> elaborate since there is a 1:1 driver<->platform mapping.)

Isn't the driver (or DT binding) supposed to define what pinctrl states
must exist, and those state must always exist? There's no requirement
for a pinctrl state definition to always actually contain content that
changes the state. That's exactly why PIN_MAP_TYPE_DUMMY_STATE exists.

> An alternative to embedding the pin handling code into
> the drivers is to use bus notifiers as is done with the
> shmobile clocking by drivers/base/power/clock_ops.c
> I could easily imagine pinctrl_ops.c like that for some
> platforms. This mandates still binding the pin table entries
> to a device but avoids adding any code to the drivers.
> 
> However this latter approach does not work for us (Ux500) -
> the three resources we have: clocks, pins and power domains
> are dependent on state switch ordering (sometimes you need
> to switch off the clock then set pin state, sometimes the
> other way around) and it is not even
> the same for all drivers - the notifier approach mandates
> that all drivers do the clock, power domain and pin handling
> uniformly.

That certainly does imply that individual drivers do need to handle this
explicitly. Although I still think that only specific drivers actually
affected by this should end up actively managing pinctrl; shouldn't
anything that can get away with relying on system hogs do so?

Re: [PATCH 03/10] tty: pxa: configure pin

[Linus Walleij]

On Mon, Oct 22, 2012 at 10:26 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 10/22/2012 02:45 AM, Linus Walleij wrote:

>> For the Ux500 drivers we have found that all of those that have pin
>> resources actually have to be handled by the driver. The reason is
>> that all of them have idle and/or sleep states that need to be
>> handled at runtime.
>
> Well, presumably the pinctrl driver itself could have both default/idle
> states, and system sleep could engage the appropriate system-wide setting.

I thought about that, but it does not allow us to control the
resource activation/deactivation order.

In some drivers the suspend() path could be:

- pins to sleep
- clk_disable
- disable external regulator
- release power domain

In others:

- clk_disable()
- disable external regulator
- pins to sleep()
- release power domain

Ulf Hansson ran into this a while back.

Just like with the notifier approach, this approach assumes that either
the ordering above doesn't matter, or that it is the same for all drivers.

>> As an additional complication our drivers are used also by
>> the Versatile and SPEAr family of platforms, so the control
>> need to be tolerant (accept missing pinctrl handles and states)
>> as well. (I can see that other drivers take shortcuts by being less
>> elaborate since there is a 1:1 driver<->platform mapping.)
>
> Isn't the driver (or DT binding) supposed to define what pinctrl states
> must exist, and those state must always exist? There's no requirement
> for a pinctrl state definition to always actually contain content that
> changes the state. That's exactly why PIN_MAP_TYPE_DUMMY_STATE exists.

Well, it could also define that e.g. the "sleep" state is optional.

I'm sort of scared about imposing too strict usage patterns as
it may cause more problems than it solves even if the code
does look simpler.

>> An alternative to embedding the pin handling code into
>> the drivers is to use bus notifiers as is done with the
>> shmobile clocking by drivers/base/power/clock_ops.c
>> I could easily imagine pinctrl_ops.c like that for some
>> platforms. This mandates still binding the pin table entries
>> to a device but avoids adding any code to the drivers.
>>
>> However this latter approach does not work for us (Ux500) -
>> the three resources we have: clocks, pins and power domains
>> are dependent on state switch ordering (sometimes you need
>> to switch off the clock then set pin state, sometimes the
>> other way around) and it is not even
>> the same for all drivers - the notifier approach mandates
>> that all drivers do the clock, power domain and pin handling
>> uniformly.
>
> That certainly does imply that individual drivers do need to handle this
> explicitly. Although I still think that only specific drivers actually
> affected by this should end up actively managing pinctrl; shouldn't
> anything that can get away with relying on system hogs do so?

I don't know. Having say regulator, clock and pin handling in the
driver itself also gives a nice encapsulated view and sense of
control when just reading that one driver.

As one developer put it he does not really like that bus notifiers
are doing things behind his back. The debugging of the
driver can become very hairy, as you trace through bus
notifiers etc to find out what is wrong with your pins/clock/voltage.

We don't have hogs, bus notifiers and the like for regulators,
and IIRC Mark disliked the idea. Basically I think it's better if
all resources are atleast handled the same way.

Yours,
Linus Walleij

[PATCH 03/10] tty: pxa: configure pin

[Linus Walleij]

On Mon, Oct 22, 2012 at 10:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/22/2012 02:45 AM, Linus Walleij wrote:

>> For the Ux500 drivers we have found that all of those that have pin
>> resources actually have to be handled by the driver. The reason is
>> that all of them have idle and/or sleep states that need to be
>> handled at runtime.
>
> Well, presumably the pinctrl driver itself could have both default/idle
> states, and system sleep could engage the appropriate system-wide setting.

I thought about that, but it does not allow us to control the
resource activation/deactivation order.

In some drivers the suspend() path could be:

- pins to sleep
- clk_disable
- disable external regulator
- release power domain

In others:

- clk_disable()
- disable external regulator
- pins to sleep()
- release power domain

Ulf Hansson ran into this a while back.

Just like with the notifier approach, this approach assumes that either
the ordering above doesn't matter, or that it is the same for all drivers.

>> As an additional complication our drivers are used also by
>> the Versatile and SPEAr family of platforms, so the control
>> need to be tolerant (accept missing pinctrl handles and states)
>> as well. (I can see that other drivers take shortcuts by being less
>> elaborate since there is a 1:1 driver<->platform mapping.)
>
> Isn't the driver (or DT binding) supposed to define what pinctrl states
> must exist, and those state must always exist? There's no requirement
> for a pinctrl state definition to always actually contain content that
> changes the state. That's exactly why PIN_MAP_TYPE_DUMMY_STATE exists.

Well, it could also define that e.g. the "sleep" state is optional.

I'm sort of scared about imposing too strict usage patterns as
it may cause more problems than it solves even if the code
does look simpler.

>> An alternative to embedding the pin handling code into
>> the drivers is to use bus notifiers as is done with the
>> shmobile clocking by drivers/base/power/clock_ops.c
>> I could easily imagine pinctrl_ops.c like that for some
>> platforms. This mandates still binding the pin table entries
>> to a device but avoids adding any code to the drivers.
>>
>> However this latter approach does not work for us (Ux500) -
>> the three resources we have: clocks, pins and power domains
>> are dependent on state switch ordering (sometimes you need
>> to switch off the clock then set pin state, sometimes the
>> other way around) and it is not even
>> the same for all drivers - the notifier approach mandates
>> that all drivers do the clock, power domain and pin handling
>> uniformly.
>
> That certainly does imply that individual drivers do need to handle this
> explicitly. Although I still think that only specific drivers actually
> affected by this should end up actively managing pinctrl; shouldn't
> anything that can get away with relying on system hogs do so?

I don't know. Having say regulator, clock and pin handling in the
driver itself also gives a nice encapsulated view and sense of
control when just reading that one driver.

As one developer put it he does not really like that bus notifiers
are doing things behind his back. The debugging of the
driver can become very hairy, as you trace through bus
notifiers etc to find out what is wrong with your pins/clock/voltage.

We don't have hogs, bus notifiers and the like for regulators,
and IIRC Mark disliked the idea. Basically I think it's better if
all resources are atleast handled the same way.

Yours,
Linus Walleij

Re: [PATCH 03/10] tty: pxa: configure pin

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1194 bytes --]

On Tue, Oct 23, 2012 at 11:26:40AM +0200, Linus Walleij wrote:

> I thought about that, but it does not allow us to control the
> resource activation/deactivation order.

> In some drivers the suspend() path could be:

> - pins to sleep
> - clk_disable
> - disable external regulator
> - release power domain

> In others:

> - clk_disable()
> - disable external regulator
> - pins to sleep()
> - release power domain

> Ulf Hansson ran into this a while back.

> Just like with the notifier approach, this approach assumes that either
> the ordering above doesn't matter, or that it is the same for all drivers.

Can we handle this with power domains - if different devices require
different orderings they can be placed in power domains which implement
the appropriate orderings for them?

> We don't have hogs, bus notifiers and the like for regulators,
> and IIRC Mark disliked the idea. Basically I think it's better if
> all resources are atleast handled the same way.

It'd be OK to have something that was manually activated for specific
regulators but it's not sane to try and do something generic as you'll
end up powering off all your wakeup sources which tends not to work so
well.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 03/10] tty: pxa: configure pin

[Mark Brown]

On Tue, Oct 23, 2012 at 11:26:40AM +0200, Linus Walleij wrote:

> I thought about that, but it does not allow us to control the
> resource activation/deactivation order.

> In some drivers the suspend() path could be:

> - pins to sleep
> - clk_disable
> - disable external regulator
> - release power domain

> In others:

> - clk_disable()
> - disable external regulator
> - pins to sleep()
> - release power domain

> Ulf Hansson ran into this a while back.

> Just like with the notifier approach, this approach assumes that either
> the ordering above doesn't matter, or that it is the same for all drivers.

Can we handle this with power domains - if different devices require
different orderings they can be placed in power domains which implement
the appropriate orderings for them?

> We don't have hogs, bus notifiers and the like for regulators,
> and IIRC Mark disliked the idea. Basically I think it's better if
> all resources are atleast handled the same way.

It'd be OK to have something that was manually activated for specific
regulators but it's not sane to try and do something generic as you'll
end up powering off all your wakeup sources which tends not to work so
well.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121023/18171929/attachment.sig>

Re: [PATCH 03/10] tty: pxa: configure pin

[Linus Walleij]

On Tue, Oct 23, 2012 at 11:37 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Tue, Oct 23, 2012 at 11:26:40AM +0200, Linus Walleij wrote:
>> I thought about that, but it does not allow us to control the
>> resource activation/deactivation order.
>
>> In some drivers the suspend() path could be:
>
>> - pins to sleep
>> - clk_disable
>> - disable external regulator
>> - release power domain
>
>> In others:
>
>> - clk_disable()
>> - disable external regulator
>> - pins to sleep()
>> - release power domain
>
>> Ulf Hansson ran into this a while back.
>
>> Just like with the notifier approach, this approach assumes that either
>> the ordering above doesn't matter, or that it is the same for all drivers.
>
> Can we handle this with power domains - if different devices require
> different orderings they can be placed in power domains which implement
> the appropriate orderings for them?

clocks, regulators, pins and all in power domains?

Then we should rename them "device resource domains" or
something...

I have a strong sense of system-wide all-or-nothing approaches
to this problem.

- Either we use "power" domains to handle every resource the
 device has.
- Or the device driver manages it's own resources.

I find it pretty hard to build consensus around either idea.

The problem is that the latter approach (devices drivers themselves
take clocks, domain power, etc) is already quite widespread
in the kernel so to get the entire world consistent to the former
approach would be pretty painful I think. Especially in the case
where a device driver is used on more than one SoC.

Yours,
Linus Walleij

[PATCH 03/10] tty: pxa: configure pin

[Linus Walleij]

On Tue, Oct 23, 2012 at 11:37 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Oct 23, 2012 at 11:26:40AM +0200, Linus Walleij wrote:
>> I thought about that, but it does not allow us to control the
>> resource activation/deactivation order.
>
>> In some drivers the suspend() path could be:
>
>> - pins to sleep
>> - clk_disable
>> - disable external regulator
>> - release power domain
>
>> In others:
>
>> - clk_disable()
>> - disable external regulator
>> - pins to sleep()
>> - release power domain
>
>> Ulf Hansson ran into this a while back.
>
>> Just like with the notifier approach, this approach assumes that either
>> the ordering above doesn't matter, or that it is the same for all drivers.
>
> Can we handle this with power domains - if different devices require
> different orderings they can be placed in power domains which implement
> the appropriate orderings for them?

clocks, regulators, pins and all in power domains?

Then we should rename them "device resource domains" or
something...

I have a strong sense of system-wide all-or-nothing approaches
to this problem.

- Either we use "power" domains to handle every resource the
 device has.
- Or the device driver manages it's own resources.

I find it pretty hard to build consensus around either idea.

The problem is that the latter approach (devices drivers themselves
take clocks, domain power, etc) is already quite widespread
in the kernel so to get the entire world consistent to the former
approach would be pretty painful I think. Especially in the case
where a device driver is used on more than one SoC.

Yours,
Linus Walleij

Re: [PATCH 03/10] tty: pxa: configure pin

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1989 bytes --]

On Tue, Oct 23, 2012 at 11:59:30AM +0200, Linus Walleij wrote:
> On Tue, Oct 23, 2012 at 11:37 AM, Mark Brown
> > On Tue, Oct 23, 2012 at 11:26:40AM +0200, Linus Walleij wrote:

> > Can we handle this with power domains - if different devices require
> > different orderings they can be placed in power domains which implement
> > the appropriate orderings for them?

> clocks, regulators, pins and all in power domains?

> Then we should rename them "device resource domains" or
> something...

We can call them Ichabod for all I care...

> I have a strong sense of system-wide all-or-nothing approaches
> to this problem.

> - Either we use "power" domains to handle every resource the
>  device has.
> - Or the device driver manages it's own resources.

> I find it pretty hard to build consensus around either idea.

Well, I don't think we want all or nothing approaches here.  One problem
is that we don't have a home for the SoC integration so we're trying to
shove it all into the drivers which just leads to a stack of pointless
boilerplate when the driver isn't actually doing anything beyond the
basic pattern of turning everything off when it goes idle and turning it
on again when it needs to do something.  Having to open code that stuff
in the drivers and then deal with the stubbing and error handling so the
error handling in the drivers is painful.  There's also another axis
where things aren't part of a SoC but are separate devices so you want
to carry things along with the driver rather than have a separate bit of
code which is required to glue things into the platform.

For example it seems fairly clear to me that things like the pinctrl
integration I see in something like sound/soc/fsl/imx-audmux.c shouldn't
really be there as we're just setting up a static configration on boot.
On the other hand where things are directly involved with the active
operation of a device like changing clock rates then clearly the driver
needs to know about and manage things.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 03/10] tty: pxa: configure pin

[Mark Brown]

On Tue, Oct 23, 2012 at 11:59:30AM +0200, Linus Walleij wrote:
> On Tue, Oct 23, 2012 at 11:37 AM, Mark Brown
> > On Tue, Oct 23, 2012 at 11:26:40AM +0200, Linus Walleij wrote:

> > Can we handle this with power domains - if different devices require
> > different orderings they can be placed in power domains which implement
> > the appropriate orderings for them?

> clocks, regulators, pins and all in power domains?

> Then we should rename them "device resource domains" or
> something...

We can call them Ichabod for all I care...

> I have a strong sense of system-wide all-or-nothing approaches
> to this problem.

> - Either we use "power" domains to handle every resource the
>  device has.
> - Or the device driver manages it's own resources.

> I find it pretty hard to build consensus around either idea.

Well, I don't think we want all or nothing approaches here.  One problem
is that we don't have a home for the SoC integration so we're trying to
shove it all into the drivers which just leads to a stack of pointless
boilerplate when the driver isn't actually doing anything beyond the
basic pattern of turning everything off when it goes idle and turning it
on again when it needs to do something.  Having to open code that stuff
in the drivers and then deal with the stubbing and error handling so the
error handling in the drivers is painful.  There's also another axis
where things aren't part of a SoC but are separate devices so you want
to carry things along with the driver rather than have a separate bit of
code which is required to glue things into the platform.

For example it seems fairly clear to me that things like the pinctrl
integration I see in something like sound/soc/fsl/imx-audmux.c shouldn't
really be there as we're just setting up a static configration on boot.
On the other hand where things are directly involved with the active
operation of a device like changing clock rates then clearly the driver
needs to know about and manage things.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121023/48f227e3/attachment.sig>

Re: [PATCH 03/10] tty: pxa: configure pin

[Linus Walleij]

On Tue, Oct 23, 2012 at 1:58 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:

> One problem
> is that we don't have a home for the SoC integration so we're trying to
> shove it all into the drivers which just leads to a stack of pointless
> boilerplate when the driver isn't actually doing anything beyond the
> basic pattern of turning everything off when it goes idle and turning it
> on again when it needs to do something.  Having to open code that stuff
> in the drivers and then deal with the stubbing and error handling so the
> error handling in the drivers is painful.  There's also another axis
> where things aren't part of a SoC but are separate devices so you want
> to carry things along with the driver rather than have a separate bit of
> code which is required to glue things into the platform.

I agree. I'm thinking about the approach of adding helpers into the
PM runtime layer so state handling is centralized, while only the
event trigger goes into the driver. Basically it's just the implicit
triggers from pm_runtime_[get|put][[_sync] that is causing
problems.

Yours,
Linus Walleij

[PATCH 03/10] tty: pxa: configure pin

[Linus Walleij]

On Tue, Oct 23, 2012 at 1:58 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> One problem
> is that we don't have a home for the SoC integration so we're trying to
> shove it all into the drivers which just leads to a stack of pointless
> boilerplate when the driver isn't actually doing anything beyond the
> basic pattern of turning everything off when it goes idle and turning it
> on again when it needs to do something.  Having to open code that stuff
> in the drivers and then deal with the stubbing and error handling so the
> error handling in the drivers is painful.  There's also another axis
> where things aren't part of a SoC but are separate devices so you want
> to carry things along with the driver rather than have a separate bit of
> code which is required to glue things into the platform.

I agree. I'm thinking about the approach of adding helpers into the
PM runtime layer so state handling is centralized, while only the
event trigger goes into the driver. Basically it's just the implicit
triggers from pm_runtime_[get|put][[_sync] that is causing
problems.

Yours,
Linus Walleij

[PATCH 04/10] i2c: pxa: configure pins

[Haojian Zhuang]

Configure pins by pinctrl driver.

Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-pxa.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 1034d93..cd66ec2 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -32,6 +32,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_i2c.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>
 #include <linux/clk.h>
@@ -1051,6 +1052,7 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 			    enum pxa_i2c_types *i2c_types)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct pinctrl *pinctrl;
 	const struct of_device_id *of_id =
 			of_match_device(i2c_pxa_dt_ids, &pdev->dev);
 	int ret;
@@ -1063,6 +1065,9 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 		return ret;
 	}
 	pdev->id = ret;
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		return 1;
 	if (of_get_property(np, "mrvl,i2c-polling", NULL))
 		i2c->use_pio = 1;
 	if (of_get_property(np, "mrvl,i2c-fast-mode", NULL))
-- 
1.7.0.4

[PATCH 04/10] i2c: pxa: configure pins

[Haojian Zhuang]

Configure pins by pinctrl driver.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/i2c/busses/i2c-pxa.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 1034d93..cd66ec2 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -32,6 +32,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_i2c.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>
 #include <linux/clk.h>
@@ -1051,6 +1052,7 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 			    enum pxa_i2c_types *i2c_types)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct pinctrl *pinctrl;
 	const struct of_device_id *of_id =
 			of_match_device(i2c_pxa_dt_ids, &pdev->dev);
 	int ret;
@@ -1063,6 +1065,9 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 		return ret;
 	}
 	pdev->id = ret;
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		return 1;
 	if (of_get_property(np, "mrvl,i2c-polling", NULL))
 		i2c->use_pio = 1;
 	if (of_get_property(np, "mrvl,i2c-fast-mode", NULL))
-- 
1.7.0.4

Re: [PATCH 04/10] i2c: pxa: configure pins

[Linus Walleij]

On Thu, Oct 18, 2012 at 11:06 AM, Haojian Zhuang
<haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Configure pins by pinctrl driver.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij

[PATCH 04/10] i2c: pxa: configure pins

[Linus Walleij]

On Thu, Oct 18, 2012 at 11:06 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:

> Configure pins by pinctrl driver.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 05/10] i2c: pxa: use devm_kzalloc

[Haojian Zhuang]

Use devm_kzalloc & add checking in probe() function.

Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-pxa.c |   26 ++++++++++----------------
 1 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index cd66ec2..5f96310 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1083,6 +1083,8 @@ static int i2c_pxa_probe_pdata(struct platform_device *pdev,
 	struct i2c_pxa_platform_data *plat = pdev->dev.platform_data;
 	const struct platform_device_id *id = platform_get_device_id(pdev);
 
+	if (!id)
+		return -EINVAL;
 	*i2c_types = id->driver_data;
 	if (plat) {
 		i2c->use_pio = plat->use_pio;
@@ -1099,29 +1101,23 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	struct resource *res = NULL;
 	int ret, irq;
 
-	i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
-	if (!i2c) {
-		ret = -ENOMEM;
-		goto emalloc;
-	}
+	i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
 
 	ret = i2c_pxa_probe_dt(dev, i2c, &i2c_type);
 	if (ret > 0)
 		ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
 	if (ret < 0)
-		goto eclk;
+		return ret;
 
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(dev, 0);
-	if (res == NULL || irq < 0) {
-		ret = -ENODEV;
-		goto eclk;
-	}
+	if (res == NULL || irq < 0)
+		return -ENODEV;
 
-	if (!request_mem_region(res->start, resource_size(res), res->name)) {
-		ret = -ENOMEM;
-		goto eclk;
-	}
+	if (!request_mem_region(res->start, resource_size(res), res->name))
+		return -ENOMEM;
 
 	i2c->adap.owner   = THIS_MODULE;
 	i2c->adap.retries = 5;
@@ -1214,8 +1210,6 @@ ereqirq:
 eremap:
 	clk_put(i2c->clk);
 eclk:
-	kfree(i2c);
-emalloc:
 	release_mem_region(res->start, resource_size(res));
 	return ret;
 }
-- 
1.7.0.4

[PATCH 05/10] i2c: pxa: use devm_kzalloc

[Haojian Zhuang]

Use devm_kzalloc & add checking in probe() function.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/i2c/busses/i2c-pxa.c |   26 ++++++++++----------------
 1 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index cd66ec2..5f96310 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1083,6 +1083,8 @@ static int i2c_pxa_probe_pdata(struct platform_device *pdev,
 	struct i2c_pxa_platform_data *plat = pdev->dev.platform_data;
 	const struct platform_device_id *id = platform_get_device_id(pdev);
 
+	if (!id)
+		return -EINVAL;
 	*i2c_types = id->driver_data;
 	if (plat) {
 		i2c->use_pio = plat->use_pio;
@@ -1099,29 +1101,23 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	struct resource *res = NULL;
 	int ret, irq;
 
-	i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
-	if (!i2c) {
-		ret = -ENOMEM;
-		goto emalloc;
-	}
+	i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
 
 	ret = i2c_pxa_probe_dt(dev, i2c, &i2c_type);
 	if (ret > 0)
 		ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
 	if (ret < 0)
-		goto eclk;
+		return ret;
 
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(dev, 0);
-	if (res == NULL || irq < 0) {
-		ret = -ENODEV;
-		goto eclk;
-	}
+	if (res == NULL || irq < 0)
+		return -ENODEV;
 
-	if (!request_mem_region(res->start, resource_size(res), res->name)) {
-		ret = -ENOMEM;
-		goto eclk;
-	}
+	if (!request_mem_region(res->start, resource_size(res), res->name))
+		return -ENOMEM;
 
 	i2c->adap.owner   = THIS_MODULE;
 	i2c->adap.retries = 5;
@@ -1214,8 +1210,6 @@ ereqirq:
 eremap:
 	clk_put(i2c->clk);
 eclk:
-	kfree(i2c);
-emalloc:
 	release_mem_region(res->start, resource_size(res));
 	return ret;
 }
-- 
1.7.0.4

Re: [PATCH 05/10] i2c: pxa: use devm_kzalloc

[Stephen Warren]

On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
> Use devm_kzalloc & add checking in probe() function.

This patch seems unrelated to this series. In fact, the series touches a
bunch of different subsystems; would you expect all these patches to be
merged through a single tree? If so, which?

[PATCH 05/10] i2c: pxa: use devm_kzalloc

[Stephen Warren]

On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
> Use devm_kzalloc & add checking in probe() function.

This patch seems unrelated to this series. In fact, the series touches a
bunch of different subsystems; would you expect all these patches to be
merged through a single tree? If so, which?

Re: [PATCH 05/10] i2c: pxa: use devm_kzalloc

[Haojian Zhuang]

On Fri, Oct 19, 2012 at 6:27 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
>> Use devm_kzalloc & add checking in probe() function.
>
> This patch seems unrelated to this series. In fact, the series touches a
> bunch of different subsystems; would you expect all these patches to be
> merged through a single tree? If so, which?

Yes, I expect all these patches could be merged by pinctrl subtree.

Regards
Haojian

[PATCH 05/10] i2c: pxa: use devm_kzalloc

[Haojian Zhuang]

On Fri, Oct 19, 2012 at 6:27 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
>> Use devm_kzalloc & add checking in probe() function.
>
> This patch seems unrelated to this series. In fact, the series touches a
> bunch of different subsystems; would you expect all these patches to be
> merged through a single tree? If so, which?

Yes, I expect all these patches could be merged by pinctrl subtree.

Regards
Haojian

[PATCH 06/10] pinctrl: single: support gpio request and free

[Haojian Zhuang]

Marvell's PXA/MMP silicon also match the behavior of pinctrl-single.
Each pin binds to one register. A lot of pins could be configured
as gpio.

Now add three properties in below.
pinctrl-single,gpio-mask: mask of enable/disable value of gpio
pinctrl-single,gpio-ranges: gpio range array
pinctrl-single,gpio: <gpio base, npins in range, pin base>
pinctrl-single,gpio-enable: <gpio enable register offset, enable
value>
pinctrl-single,gpio-disable: <gpio disable register offset, disable
value>

Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pinctrl/pinctrl-single.c |  140 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 138 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 64d109a..02cd412 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -29,6 +29,7 @@
 #define PCS_MUX_NAME			"pinctrl-single,pins"
 #define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
 #define PCS_OFF_DISABLED		~0U
+#define PCS_MAX_GPIO_VALUES		3
 
 /**
  * struct pcs_pingroup - pingroups for a function
@@ -75,6 +76,26 @@ struct pcs_function {
 };
 
 /**
+ * struct pcs_gpio_range - pinctrl gpio range
+ * @range:	subrange of the GPIO number space
+ * @reg_en:	register of enabling gpio function
+ * @reg_dis:	register of disabling gpio function
+ * @val_en:	enable value on gpio function
+ * @val_dis:	disable value on gpio function
+ * @need_en:	need to handle enable value on gpio function
+ * @need_dis:	need to handle disable value on gpio function
+ */
+struct pcs_gpio_range {
+	struct pinctrl_gpio_range range;
+	u32 reg_en;
+	u32 reg_dis;
+	int val_en;
+	int val_dis;
+	unsigned need_en:1;
+	unsigned need_dis:1;
+};
+
+/**
  * struct pcs_data - wrapper for data needed by pinctrl framework
  * @pa:		pindesc array
  * @cur:	index to current element
@@ -115,14 +136,17 @@ struct pcs_name {
  * @fshift:	function register shift
  * @foff:	value to turn mux off
  * @fmax:	max number of functions in fmask
+ * @gmask:	gpio control mask
  * @names:	array of register names for pins
  * @pins:	physical pins on the SoC
  * @pgtree:	pingroup index radix tree
  * @ftree:	function index radix tree
  * @pingroups:	list of pingroups
  * @functions:	list of functions
+ * @ranges:	list of gpio ranges
  * @ngroups:	number of pingroups
  * @nfuncs:	number of functions
+ * @nranges:	number of gpio ranges
  * @desc:	pin controller descriptor
  * @read:	register read function to use
  * @write:	register write function to use
@@ -139,14 +163,17 @@ struct pcs_device {
 	unsigned fshift;
 	unsigned foff;
 	unsigned fmax;
+	unsigned gmask;
 	struct pcs_name *names;
 	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 ranges;
 	unsigned ngroups;
 	unsigned nfuncs;
+	unsigned nranges;
 	struct pinctrl_desc desc;
 	unsigned (*read)(void __iomem *reg);
 	void (*write)(unsigned val, void __iomem *reg);
@@ -387,9 +414,48 @@ static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector,
 }
 
 static int pcs_request_gpio(struct pinctrl_dev *pctldev,
-			struct pinctrl_gpio_range *range, unsigned offset)
+			    struct pinctrl_gpio_range *range, unsigned offset)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	struct pcs_gpio_range *gpio = NULL;
+	int end;
+	unsigned data;
+
+	gpio = container_of(range, struct pcs_gpio_range, range);
+	if (!gpio->need_en)
+		return 0;
+	end = range->pin_base + range->npins - 1;
+	if (offset < range->pin_base || offset > end) {
+		dev_err(pctldev->dev, "offset %d isn't in the range of "
+			"%d to %d\n", offset, range->pin_base, end);
+		return -EINVAL;
+	}
+	data = pcs_readl((void __iomem *)gpio->reg_en) & ~pcs->gmask;
+	data |= gpio->val_en;
+	pcs_writel(data, (void __iomem *)gpio->reg_en);
+	return 0;
+}
+
+static void pcs_disable_gpio(struct pinctrl_dev *pctldev,
+			     struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	struct pcs_gpio_range *gpio = NULL;
+	int end;
+	unsigned data;
+
+	gpio = container_of(range, struct pcs_gpio_range, range);
+	if (!gpio->need_dis)
+		return;
+	end = range->pin_base + range->npins - 1;
+	if (offset < range->pin_base || offset > end) {
+		dev_err(pctldev->dev, "offset %d isn't in the range of "
+			"%d to %d\n", offset, range->pin_base, end);
+		return;
+	}
+	data = pcs_readl((void __iomem *)gpio->reg_dis) & ~pcs->gmask;
+	data |= gpio->val_dis;
+	pcs_writel(data, (void __iomem *)gpio->reg_dis);
 }
 
 static struct pinmux_ops pcs_pinmux_ops = {
@@ -399,6 +465,7 @@ static struct pinmux_ops pcs_pinmux_ops = {
 	.enable = pcs_enable,
 	.disable = pcs_disable,
 	.gpio_request_enable = pcs_request_gpio,
+	.gpio_disable_free = pcs_disable_gpio,
 };
 
 static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
@@ -848,6 +915,70 @@ static void pcs_free_resources(struct pcs_device *pcs)
 
 static struct of_device_id pcs_of_match[];
 
+static int __devinit pcs_add_gpio_range(struct device_node *node,
+					struct pcs_device *pcs)
+{
+	struct pcs_gpio_range *gpio;
+	struct device_node *np;
+	const __be32 *list;
+	const char list_name[] = "pinctrl-single,gpio-ranges";
+	const char name[] = "pinctrl-single";
+	u32 gpiores[PCS_MAX_GPIO_VALUES];
+	int ret, size, i, mux_bytes = 0;
+
+	ret = of_property_read_u32(node, "pinctrl-single,gpio-mask",
+				&pcs->gmask);
+	if (ret < 0)
+		return 0;
+	list = of_get_property(node, list_name, &size);
+	if (!list)
+		return -ENOENT;
+	size = size / sizeof(*list);
+	for (i = 0; i < size; i++) {
+		np = of_parse_phandle(node, list_name, i);
+		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
+		ret = of_property_read_u32_array(np, "pinctrl-single,gpio",
+						 gpiores, PCS_MAX_GPIO_VALUES);
+		if (ret < 0)
+			return -ENOENT;
+		gpio = devm_kzalloc(pcs->dev, sizeof(*gpio), GFP_KERNEL);
+		if (!gpio) {
+			dev_err(pcs->dev, "failed to allocate pcs gpio\n");
+			return -ENOMEM;
+		}
+		gpio->range.id = i;
+		gpio->range.base = gpiores[0];
+		gpio->range.npins = gpiores[1];
+		gpio->range.name = kmemdup(name, sizeof(name), GFP_KERNEL);
+		mux_bytes = pcs->width / BITS_PER_BYTE;
+		gpio->range.pin_base = gpiores[2] / mux_bytes;
+		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
+		ret = of_property_read_u32_array(np,
+				"pinctrl-single,gpio-enable", gpiores, 2);
+		if (!ret) {
+			gpio->reg_en = (u32)pcs->base + gpiores[0];
+			gpio->val_en = gpiores[1];
+			gpio->need_en = 1;
+		}
+		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
+		ret = of_property_read_u32_array(np,
+				"pinctrl-single,gpio-disable", gpiores, 2);
+		if (!ret) {
+			gpio->reg_dis = (u32)pcs->base + gpiores[0];
+			gpio->val_dis = gpiores[1];
+			gpio->need_dis = 1;
+		}
+
+		mutex_lock(&pcs->mutex);
+		list_add_tail(&gpio->range.node, &pcs->ranges);
+		pcs->nranges++;
+		mutex_unlock(&pcs->mutex);
+
+		pinctrl_add_gpio_range(pcs->pctl, &gpio->range);
+	}
+	return 0;
+}
+
 static int __devinit pcs_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -869,6 +1000,7 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 	mutex_init(&pcs->mutex);
 	INIT_LIST_HEAD(&pcs->pingroups);
 	INIT_LIST_HEAD(&pcs->functions);
+	INIT_LIST_HEAD(&pcs->ranges);
 
 	PCS_GET_PROP_U32("pinctrl-single,register-width", &pcs->width,
 			 "register width not specified\n");
@@ -941,6 +1073,10 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 		goto free;
 	}
 
+	ret = pcs_add_gpio_range(np, pcs);
+	if (ret < 0)
+		return ret;
+
 	dev_info(pcs->dev, "%i pins at pa %p size %u\n",
 		 pcs->desc.npins, pcs->base, pcs->size);
 
-- 
1.7.0.4

[PATCH 06/10] pinctrl: single: support gpio request and free

[Haojian Zhuang]

Marvell's PXA/MMP silicon also match the behavior of pinctrl-single.
Each pin binds to one register. A lot of pins could be configured
as gpio.

Now add three properties in below.
pinctrl-single,gpio-mask: mask of enable/disable value of gpio
pinctrl-single,gpio-ranges: gpio range array
pinctrl-single,gpio: <gpio base, npins in range, pin base>
pinctrl-single,gpio-enable: <gpio enable register offset, enable
value>
pinctrl-single,gpio-disable: <gpio disable register offset, disable
value>

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/pinctrl/pinctrl-single.c |  140 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 138 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 64d109a..02cd412 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -29,6 +29,7 @@
 #define PCS_MUX_NAME			"pinctrl-single,pins"
 #define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
 #define PCS_OFF_DISABLED		~0U
+#define PCS_MAX_GPIO_VALUES		3
 
 /**
  * struct pcs_pingroup - pingroups for a function
@@ -75,6 +76,26 @@ struct pcs_function {
 };
 
 /**
+ * struct pcs_gpio_range - pinctrl gpio range
+ * @range:	subrange of the GPIO number space
+ * @reg_en:	register of enabling gpio function
+ * @reg_dis:	register of disabling gpio function
+ * @val_en:	enable value on gpio function
+ * @val_dis:	disable value on gpio function
+ * @need_en:	need to handle enable value on gpio function
+ * @need_dis:	need to handle disable value on gpio function
+ */
+struct pcs_gpio_range {
+	struct pinctrl_gpio_range range;
+	u32 reg_en;
+	u32 reg_dis;
+	int val_en;
+	int val_dis;
+	unsigned need_en:1;
+	unsigned need_dis:1;
+};
+
+/**
  * struct pcs_data - wrapper for data needed by pinctrl framework
  * @pa:		pindesc array
  * @cur:	index to current element
@@ -115,14 +136,17 @@ struct pcs_name {
  * @fshift:	function register shift
  * @foff:	value to turn mux off
  * @fmax:	max number of functions in fmask
+ * @gmask:	gpio control mask
  * @names:	array of register names for pins
  * @pins:	physical pins on the SoC
  * @pgtree:	pingroup index radix tree
  * @ftree:	function index radix tree
  * @pingroups:	list of pingroups
  * @functions:	list of functions
+ * @ranges:	list of gpio ranges
  * @ngroups:	number of pingroups
  * @nfuncs:	number of functions
+ * @nranges:	number of gpio ranges
  * @desc:	pin controller descriptor
  * @read:	register read function to use
  * @write:	register write function to use
@@ -139,14 +163,17 @@ struct pcs_device {
 	unsigned fshift;
 	unsigned foff;
 	unsigned fmax;
+	unsigned gmask;
 	struct pcs_name *names;
 	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 ranges;
 	unsigned ngroups;
 	unsigned nfuncs;
+	unsigned nranges;
 	struct pinctrl_desc desc;
 	unsigned (*read)(void __iomem *reg);
 	void (*write)(unsigned val, void __iomem *reg);
@@ -387,9 +414,48 @@ static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector,
 }
 
 static int pcs_request_gpio(struct pinctrl_dev *pctldev,
-			struct pinctrl_gpio_range *range, unsigned offset)
+			    struct pinctrl_gpio_range *range, unsigned offset)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	struct pcs_gpio_range *gpio = NULL;
+	int end;
+	unsigned data;
+
+	gpio = container_of(range, struct pcs_gpio_range, range);
+	if (!gpio->need_en)
+		return 0;
+	end = range->pin_base + range->npins - 1;
+	if (offset < range->pin_base || offset > end) {
+		dev_err(pctldev->dev, "offset %d isn't in the range of "
+			"%d to %d\n", offset, range->pin_base, end);
+		return -EINVAL;
+	}
+	data = pcs_readl((void __iomem *)gpio->reg_en) & ~pcs->gmask;
+	data |= gpio->val_en;
+	pcs_writel(data, (void __iomem *)gpio->reg_en);
+	return 0;
+}
+
+static void pcs_disable_gpio(struct pinctrl_dev *pctldev,
+			     struct pinctrl_gpio_range *range, unsigned offset)
+{
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	struct pcs_gpio_range *gpio = NULL;
+	int end;
+	unsigned data;
+
+	gpio = container_of(range, struct pcs_gpio_range, range);
+	if (!gpio->need_dis)
+		return;
+	end = range->pin_base + range->npins - 1;
+	if (offset < range->pin_base || offset > end) {
+		dev_err(pctldev->dev, "offset %d isn't in the range of "
+			"%d to %d\n", offset, range->pin_base, end);
+		return;
+	}
+	data = pcs_readl((void __iomem *)gpio->reg_dis) & ~pcs->gmask;
+	data |= gpio->val_dis;
+	pcs_writel(data, (void __iomem *)gpio->reg_dis);
 }
 
 static struct pinmux_ops pcs_pinmux_ops = {
@@ -399,6 +465,7 @@ static struct pinmux_ops pcs_pinmux_ops = {
 	.enable = pcs_enable,
 	.disable = pcs_disable,
 	.gpio_request_enable = pcs_request_gpio,
+	.gpio_disable_free = pcs_disable_gpio,
 };
 
 static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
@@ -848,6 +915,70 @@ static void pcs_free_resources(struct pcs_device *pcs)
 
 static struct of_device_id pcs_of_match[];
 
+static int __devinit pcs_add_gpio_range(struct device_node *node,
+					struct pcs_device *pcs)
+{
+	struct pcs_gpio_range *gpio;
+	struct device_node *np;
+	const __be32 *list;
+	const char list_name[] = "pinctrl-single,gpio-ranges";
+	const char name[] = "pinctrl-single";
+	u32 gpiores[PCS_MAX_GPIO_VALUES];
+	int ret, size, i, mux_bytes = 0;
+
+	ret = of_property_read_u32(node, "pinctrl-single,gpio-mask",
+				&pcs->gmask);
+	if (ret < 0)
+		return 0;
+	list = of_get_property(node, list_name, &size);
+	if (!list)
+		return -ENOENT;
+	size = size / sizeof(*list);
+	for (i = 0; i < size; i++) {
+		np = of_parse_phandle(node, list_name, i);
+		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
+		ret = of_property_read_u32_array(np, "pinctrl-single,gpio",
+						 gpiores, PCS_MAX_GPIO_VALUES);
+		if (ret < 0)
+			return -ENOENT;
+		gpio = devm_kzalloc(pcs->dev, sizeof(*gpio), GFP_KERNEL);
+		if (!gpio) {
+			dev_err(pcs->dev, "failed to allocate pcs gpio\n");
+			return -ENOMEM;
+		}
+		gpio->range.id = i;
+		gpio->range.base = gpiores[0];
+		gpio->range.npins = gpiores[1];
+		gpio->range.name = kmemdup(name, sizeof(name), GFP_KERNEL);
+		mux_bytes = pcs->width / BITS_PER_BYTE;
+		gpio->range.pin_base = gpiores[2] / mux_bytes;
+		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
+		ret = of_property_read_u32_array(np,
+				"pinctrl-single,gpio-enable", gpiores, 2);
+		if (!ret) {
+			gpio->reg_en = (u32)pcs->base + gpiores[0];
+			gpio->val_en = gpiores[1];
+			gpio->need_en = 1;
+		}
+		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
+		ret = of_property_read_u32_array(np,
+				"pinctrl-single,gpio-disable", gpiores, 2);
+		if (!ret) {
+			gpio->reg_dis = (u32)pcs->base + gpiores[0];
+			gpio->val_dis = gpiores[1];
+			gpio->need_dis = 1;
+		}
+
+		mutex_lock(&pcs->mutex);
+		list_add_tail(&gpio->range.node, &pcs->ranges);
+		pcs->nranges++;
+		mutex_unlock(&pcs->mutex);
+
+		pinctrl_add_gpio_range(pcs->pctl, &gpio->range);
+	}
+	return 0;
+}
+
 static int __devinit pcs_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -869,6 +1000,7 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 	mutex_init(&pcs->mutex);
 	INIT_LIST_HEAD(&pcs->pingroups);
 	INIT_LIST_HEAD(&pcs->functions);
+	INIT_LIST_HEAD(&pcs->ranges);
 
 	PCS_GET_PROP_U32("pinctrl-single,register-width", &pcs->width,
 			 "register width not specified\n");
@@ -941,6 +1073,10 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 		goto free;
 	}
 
+	ret = pcs_add_gpio_range(np, pcs);
+	if (ret < 0)
+		return ret;
+
 	dev_info(pcs->dev, "%i pins at pa %p size %u\n",
 		 pcs->desc.npins, pcs->base, pcs->size);
 
-- 
1.7.0.4

Re: [PATCH 06/10] pinctrl: single: support gpio request and free

[Tony Lindgren]

Hi,

Few minor comments below.

* Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 02:08]:
> Marvell's PXA/MMP silicon also match the behavior of pinctrl-single.
> Each pin binds to one register. A lot of pins could be configured
> as gpio.
> 
> Now add three properties in below.
> pinctrl-single,gpio-mask: mask of enable/disable value of gpio
> pinctrl-single,gpio-ranges: gpio range array
> pinctrl-single,gpio: <gpio base, npins in range, pin base>
> pinctrl-single,gpio-enable: <gpio enable register offset, enable
> value>
> pinctrl-single,gpio-disable: <gpio disable register offset, disable
> value>

Looks like this needs to be rebased also against v3.7-rc1 to apply
cleanly. Maybe also undo the wrapping in the description above while
at it?
 
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -75,6 +76,26 @@ struct pcs_function {
>  };
>  
>  /**
> + * struct pcs_gpio_range - pinctrl gpio range
> + * @range:	subrange of the GPIO number space
> + * @reg_en:	register of enabling gpio function
> + * @reg_dis:	register of disabling gpio function
> + * @val_en:	enable value on gpio function
> + * @val_dis:	disable value on gpio function
> + * @need_en:	need to handle enable value on gpio function
> + * @need_dis:	need to handle disable value on gpio function
> + */
> +struct pcs_gpio_range {
> +	struct pinctrl_gpio_range range;
> +	u32 reg_en;
> +	u32 reg_dis;

These should be void __iomem *reg_en and reg_dis to avoid casts?

You now introduce few "warning: cast removes address space of
expression" warnings when checking with sparse..

> @@ -387,9 +414,48 @@ static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector,
>  }
>  
>  static int pcs_request_gpio(struct pinctrl_dev *pctldev,
> -			struct pinctrl_gpio_range *range, unsigned offset)
> +			    struct pinctrl_gpio_range *range, unsigned offset)
>  {
> -	return -ENOTSUPP;
> +	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
> +	struct pcs_gpio_range *gpio = NULL;
> +	int end;
> +	unsigned data;

Should you return -ENOTSUPP if not configured for GPIO here?

> +	gpio = container_of(range, struct pcs_gpio_range, range);
> +	if (!gpio->need_en)
> +		return 0;
> +	end = range->pin_base + range->npins - 1;
> +	if (offset < range->pin_base || offset > end) {
> +		dev_err(pctldev->dev, "offset %d isn't in the range of "
> +			"%d to %d\n", offset, range->pin_base, end);
> +		return -EINVAL;
> +	}
> +	data = pcs_readl((void __iomem *)gpio->reg_en) & ~pcs->gmask;
> +	data |= gpio->val_en;
> +	pcs_writel(data, (void __iomem *)gpio->reg_en);

These casts should not be needed then.

> +	return 0;
> +}
> +
> +static void pcs_disable_gpio(struct pinctrl_dev *pctldev,
> +			     struct pinctrl_gpio_range *range, unsigned offset)
> +{
> +	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
> +	struct pcs_gpio_range *gpio = NULL;
> +	int end;
> +	unsigned data;
> +
> +	gpio = container_of(range, struct pcs_gpio_range, range);
> +	if (!gpio->need_dis)
> +		return;
> +	end = range->pin_base + range->npins - 1;
> +	if (offset < range->pin_base || offset > end) {
> +		dev_err(pctldev->dev, "offset %d isn't in the range of "
> +			"%d to %d\n", offset, range->pin_base, end);
> +		return;
> +	}
> +	data = pcs_readl((void __iomem *)gpio->reg_dis) & ~pcs->gmask;
> +	data |= gpio->val_dis;
> +	pcs_writel(data, (void __iomem *)gpio->reg_dis);

And these casts.

> +static int __devinit pcs_add_gpio_range(struct device_node *node,
> +					struct pcs_device *pcs)
> +{
> +	struct pcs_gpio_range *gpio;
> +	struct device_node *np;
> +	const __be32 *list;
> +	const char list_name[] = "pinctrl-single,gpio-ranges";
> +	const char name[] = "pinctrl-single";
> +	u32 gpiores[PCS_MAX_GPIO_VALUES];
> +	int ret, size, i, mux_bytes = 0;
> +
> +	ret = of_property_read_u32(node, "pinctrl-single,gpio-mask",
> +				&pcs->gmask);
> +	if (ret < 0)
> +		return 0;
> +	list = of_get_property(node, list_name, &size);
> +	if (!list)
> +		return -ENOENT;
> +	size = size / sizeof(*list);
> +	for (i = 0; i < size; i++) {
> +		np = of_parse_phandle(node, list_name, i);
> +		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
> +		ret = of_property_read_u32_array(np, "pinctrl-single,gpio",
> +						 gpiores, PCS_MAX_GPIO_VALUES);
> +		if (ret < 0)
> +			return -ENOENT;
> +		gpio = devm_kzalloc(pcs->dev, sizeof(*gpio), GFP_KERNEL);
> +		if (!gpio) {
> +			dev_err(pcs->dev, "failed to allocate pcs gpio\n");
> +			return -ENOMEM;
> +		}
> +		gpio->range.id = i;
> +		gpio->range.base = gpiores[0];
> +		gpio->range.npins = gpiores[1];
> +		gpio->range.name = kmemdup(name, sizeof(name), GFP_KERNEL);
> +		mux_bytes = pcs->width / BITS_PER_BYTE;
> +		gpio->range.pin_base = gpiores[2] / mux_bytes;
> +		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
> +		ret = of_property_read_u32_array(np,
> +				"pinctrl-single,gpio-enable", gpiores, 2);
> +		if (!ret) {
> +			gpio->reg_en = (u32)pcs->base + gpiores[0];
> +			gpio->val_en = gpiores[1];
> +			gpio->need_en = 1;
> +		}
> +		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
> +		ret = of_property_read_u32_array(np,
> +				"pinctrl-single,gpio-disable", gpiores, 2);
> +		if (!ret) {
> +			gpio->reg_dis = (u32)pcs->base + gpiores[0];
> +			gpio->val_dis = gpiores[1];
> +			gpio->need_dis = 1;
> +		}

I think it's the u32 casts here that introduce the sparse warnings.

Other than that looks OK to me.

Regards,

Tony

[PATCH 06/10] pinctrl: single: support gpio request and free

[Tony Lindgren]

Hi,

Few minor comments below.

* Haojian Zhuang <haojian.zhuang@gmail.com> [121018 02:08]:
> Marvell's PXA/MMP silicon also match the behavior of pinctrl-single.
> Each pin binds to one register. A lot of pins could be configured
> as gpio.
> 
> Now add three properties in below.
> pinctrl-single,gpio-mask: mask of enable/disable value of gpio
> pinctrl-single,gpio-ranges: gpio range array
> pinctrl-single,gpio: <gpio base, npins in range, pin base>
> pinctrl-single,gpio-enable: <gpio enable register offset, enable
> value>
> pinctrl-single,gpio-disable: <gpio disable register offset, disable
> value>

Looks like this needs to be rebased also against v3.7-rc1 to apply
cleanly. Maybe also undo the wrapping in the description above while
at it?
 
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -75,6 +76,26 @@ struct pcs_function {
>  };
>  
>  /**
> + * struct pcs_gpio_range - pinctrl gpio range
> + * @range:	subrange of the GPIO number space
> + * @reg_en:	register of enabling gpio function
> + * @reg_dis:	register of disabling gpio function
> + * @val_en:	enable value on gpio function
> + * @val_dis:	disable value on gpio function
> + * @need_en:	need to handle enable value on gpio function
> + * @need_dis:	need to handle disable value on gpio function
> + */
> +struct pcs_gpio_range {
> +	struct pinctrl_gpio_range range;
> +	u32 reg_en;
> +	u32 reg_dis;

These should be void __iomem *reg_en and reg_dis to avoid casts?

You now introduce few "warning: cast removes address space of
expression" warnings when checking with sparse..

> @@ -387,9 +414,48 @@ static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector,
>  }
>  
>  static int pcs_request_gpio(struct pinctrl_dev *pctldev,
> -			struct pinctrl_gpio_range *range, unsigned offset)
> +			    struct pinctrl_gpio_range *range, unsigned offset)
>  {
> -	return -ENOTSUPP;
> +	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
> +	struct pcs_gpio_range *gpio = NULL;
> +	int end;
> +	unsigned data;

Should you return -ENOTSUPP if not configured for GPIO here?

> +	gpio = container_of(range, struct pcs_gpio_range, range);
> +	if (!gpio->need_en)
> +		return 0;
> +	end = range->pin_base + range->npins - 1;
> +	if (offset < range->pin_base || offset > end) {
> +		dev_err(pctldev->dev, "offset %d isn't in the range of "
> +			"%d to %d\n", offset, range->pin_base, end);
> +		return -EINVAL;
> +	}
> +	data = pcs_readl((void __iomem *)gpio->reg_en) & ~pcs->gmask;
> +	data |= gpio->val_en;
> +	pcs_writel(data, (void __iomem *)gpio->reg_en);

These casts should not be needed then.

> +	return 0;
> +}
> +
> +static void pcs_disable_gpio(struct pinctrl_dev *pctldev,
> +			     struct pinctrl_gpio_range *range, unsigned offset)
> +{
> +	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
> +	struct pcs_gpio_range *gpio = NULL;
> +	int end;
> +	unsigned data;
> +
> +	gpio = container_of(range, struct pcs_gpio_range, range);
> +	if (!gpio->need_dis)
> +		return;
> +	end = range->pin_base + range->npins - 1;
> +	if (offset < range->pin_base || offset > end) {
> +		dev_err(pctldev->dev, "offset %d isn't in the range of "
> +			"%d to %d\n", offset, range->pin_base, end);
> +		return;
> +	}
> +	data = pcs_readl((void __iomem *)gpio->reg_dis) & ~pcs->gmask;
> +	data |= gpio->val_dis;
> +	pcs_writel(data, (void __iomem *)gpio->reg_dis);

And these casts.

> +static int __devinit pcs_add_gpio_range(struct device_node *node,
> +					struct pcs_device *pcs)
> +{
> +	struct pcs_gpio_range *gpio;
> +	struct device_node *np;
> +	const __be32 *list;
> +	const char list_name[] = "pinctrl-single,gpio-ranges";
> +	const char name[] = "pinctrl-single";
> +	u32 gpiores[PCS_MAX_GPIO_VALUES];
> +	int ret, size, i, mux_bytes = 0;
> +
> +	ret = of_property_read_u32(node, "pinctrl-single,gpio-mask",
> +				&pcs->gmask);
> +	if (ret < 0)
> +		return 0;
> +	list = of_get_property(node, list_name, &size);
> +	if (!list)
> +		return -ENOENT;
> +	size = size / sizeof(*list);
> +	for (i = 0; i < size; i++) {
> +		np = of_parse_phandle(node, list_name, i);
> +		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
> +		ret = of_property_read_u32_array(np, "pinctrl-single,gpio",
> +						 gpiores, PCS_MAX_GPIO_VALUES);
> +		if (ret < 0)
> +			return -ENOENT;
> +		gpio = devm_kzalloc(pcs->dev, sizeof(*gpio), GFP_KERNEL);
> +		if (!gpio) {
> +			dev_err(pcs->dev, "failed to allocate pcs gpio\n");
> +			return -ENOMEM;
> +		}
> +		gpio->range.id = i;
> +		gpio->range.base = gpiores[0];
> +		gpio->range.npins = gpiores[1];
> +		gpio->range.name = kmemdup(name, sizeof(name), GFP_KERNEL);
> +		mux_bytes = pcs->width / BITS_PER_BYTE;
> +		gpio->range.pin_base = gpiores[2] / mux_bytes;
> +		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
> +		ret = of_property_read_u32_array(np,
> +				"pinctrl-single,gpio-enable", gpiores, 2);
> +		if (!ret) {
> +			gpio->reg_en = (u32)pcs->base + gpiores[0];
> +			gpio->val_en = gpiores[1];
> +			gpio->need_en = 1;
> +		}
> +		memset(gpiores, 0, sizeof(u32) * PCS_MAX_GPIO_VALUES);
> +		ret = of_property_read_u32_array(np,
> +				"pinctrl-single,gpio-disable", gpiores, 2);
> +		if (!ret) {
> +			gpio->reg_dis = (u32)pcs->base + gpiores[0];
> +			gpio->val_dis = gpiores[1];
> +			gpio->need_dis = 1;
> +		}

I think it's the u32 casts here that introduce the sparse warnings.

Other than that looks OK to me.

Regards,

Tony

[PATCH 07/10] pinctrl: remove mutex lock in groups show

[Haojian Zhuang]

Mutex is locked duplicatly by pinconf_groups_show() and
pin_config_group_get(). It results dead lock. So avoid to lock mutex
in pinconf_groups_show().

Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pinctrl/pinconf.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 43f474c..baee2cc 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -537,8 +537,6 @@ static int pinconf_groups_show(struct seq_file *s, void *what)
 	seq_puts(s, "Pin config settings per pin group\n");
 	seq_puts(s, "Format: group (name): configs\n");
 
-	mutex_lock(&pinctrl_mutex);
-
 	while (selector < ngroups) {
 		const char *gname = pctlops->get_group_name(pctldev, selector);
 
@@ -549,8 +547,6 @@ static int pinconf_groups_show(struct seq_file *s, void *what)
 		selector++;
 	}
 
-	mutex_unlock(&pinctrl_mutex);
-
 	return 0;
 }
 
-- 
1.7.0.4

[PATCH 07/10] pinctrl: remove mutex lock in groups show

[Haojian Zhuang]

Mutex is locked duplicatly by pinconf_groups_show() and
pin_config_group_get(). It results dead lock. So avoid to lock mutex
in pinconf_groups_show().

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/pinctrl/pinconf.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 43f474c..baee2cc 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -537,8 +537,6 @@ static int pinconf_groups_show(struct seq_file *s, void *what)
 	seq_puts(s, "Pin config settings per pin group\n");
 	seq_puts(s, "Format: group (name): configs\n");
 
-	mutex_lock(&pinctrl_mutex);
-
 	while (selector < ngroups) {
 		const char *gname = pctlops->get_group_name(pctldev, selector);
 
@@ -549,8 +547,6 @@ static int pinconf_groups_show(struct seq_file *s, void *what)
 		selector++;
 	}
 
-	mutex_unlock(&pinctrl_mutex);
-
 	return 0;
 }
 
-- 
1.7.0.4

Re: [PATCH 07/10] pinctrl: remove mutex lock in groups show

[Linus Walleij]

On Thu, Oct 18, 2012 at 11:07 AM, Haojian Zhuang
<haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Mutex is locked duplicatly by pinconf_groups_show() and
> pin_config_group_get(). It results dead lock. So avoid to lock mutex
> in pinconf_groups_show().
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Thanks! Applied to my fixes branch with a Cc: stable tag.

Yours,
Linus Walleij

[PATCH 07/10] pinctrl: remove mutex lock in groups show

[Linus Walleij]

On Thu, Oct 18, 2012 at 11:07 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:

> Mutex is locked duplicatly by pinconf_groups_show() and
> pin_config_group_get(). It results dead lock. So avoid to lock mutex
> in pinconf_groups_show().
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>

Thanks! Applied to my fixes branch with a Cc: stable tag.

Yours,
Linus Walleij

Re: [PATCH 07/10] pinctrl: remove mutex lock in groups show

[Stephen Warren]

On 10/18/2012 03:07 AM, Haojian Zhuang wrote:
> Mutex is locked duplicatly by pinconf_groups_show() and
> pin_config_group_get(). It results dead lock. So avoid to lock mutex
> in pinconf_groups_show().

With this outer lock removed, how do we ensure that the pinctrl driver
that is being called into remains loaded? Does the existence of the
debugfs file ensure this, such that if it's open, the pinctrl driver
can't be removed?

Related, I wonder if much of the variable setup at the start of the
function shouldn't happen inside the lock instead of outside:

static int pinconf_groups_show(struct seq_file *s, void *what)
{
        struct pinctrl_dev *pctldev = s->private;
        const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
        const struct pinconf_ops *ops = pctldev->desc->confops;
        unsigned ngroups = pctlops->get_groups_count(pctldev);

since what if s->private is unregistered/destroyed while this function
is running?

[PATCH 07/10] pinctrl: remove mutex lock in groups show

[Stephen Warren]

On 10/18/2012 03:07 AM, Haojian Zhuang wrote:
> Mutex is locked duplicatly by pinconf_groups_show() and
> pin_config_group_get(). It results dead lock. So avoid to lock mutex
> in pinconf_groups_show().

With this outer lock removed, how do we ensure that the pinctrl driver
that is being called into remains loaded? Does the existence of the
debugfs file ensure this, such that if it's open, the pinctrl driver
can't be removed?

Related, I wonder if much of the variable setup at the start of the
function shouldn't happen inside the lock instead of outside:

static int pinconf_groups_show(struct seq_file *s, void *what)
{
        struct pinctrl_dev *pctldev = s->private;
        const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
        const struct pinconf_ops *ops = pctldev->desc->confops;
        unsigned ngroups = pctlops->get_groups_count(pctldev);

since what if s->private is unregistered/destroyed while this function
is running?

Re: [PATCH 07/10] pinctrl: remove mutex lock in groups show

[Linus Walleij]

On Fri, Oct 19, 2012 at 12:26 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 10/18/2012 03:07 AM, Haojian Zhuang wrote:
>> Mutex is locked duplicatly by pinconf_groups_show() and
>> pin_config_group_get(). It results dead lock. So avoid to lock mutex
>> in pinconf_groups_show().
>
> With this outer lock removed, how do we ensure that the pinctrl driver
> that is being called into remains loaded? Does the existence of the
> debugfs file ensure this, such that if it's open, the pinctrl driver
> can't be removed?

No, don't think so, dangling debugfs files is a common problem.

> Related, I wonder if much of the variable setup at the start of the
> function shouldn't happen inside the lock instead of outside:
>
> static int pinconf_groups_show(struct seq_file *s, void *what)
> {
>         struct pinctrl_dev *pctldev = s->private;
>         const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
>         const struct pinconf_ops *ops = pctldev->desc->confops;
>         unsigned ngroups = pctlops->get_groups_count(pctldev);
>
> since what if s->private is unregistered/destroyed while this function
> is running?

The debugfs code is fragile by nature I think, that's why we are
usually a bit relaxed here and it's also why it should be disabled
on production systems.

But any hardening patches are welcome, however we need to
get around the deadlock Haojian was seeing, maybe we should
introduce a separate debugfs mutex?

/me is slightly confused though...

Yours,
Linus Walleij

[PATCH 07/10] pinctrl: remove mutex lock in groups show

[Linus Walleij]

On Fri, Oct 19, 2012 at 12:26 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/18/2012 03:07 AM, Haojian Zhuang wrote:
>> Mutex is locked duplicatly by pinconf_groups_show() and
>> pin_config_group_get(). It results dead lock. So avoid to lock mutex
>> in pinconf_groups_show().
>
> With this outer lock removed, how do we ensure that the pinctrl driver
> that is being called into remains loaded? Does the existence of the
> debugfs file ensure this, such that if it's open, the pinctrl driver
> can't be removed?

No, don't think so, dangling debugfs files is a common problem.

> Related, I wonder if much of the variable setup at the start of the
> function shouldn't happen inside the lock instead of outside:
>
> static int pinconf_groups_show(struct seq_file *s, void *what)
> {
>         struct pinctrl_dev *pctldev = s->private;
>         const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
>         const struct pinconf_ops *ops = pctldev->desc->confops;
>         unsigned ngroups = pctlops->get_groups_count(pctldev);
>
> since what if s->private is unregistered/destroyed while this function
> is running?

The debugfs code is fragile by nature I think, that's why we are
usually a bit relaxed here and it's also why it should be disabled
on production systems.

But any hardening patches are welcome, however we need to
get around the deadlock Haojian was seeing, maybe we should
introduce a separate debugfs mutex?

/me is slightly confused though...

Yours,
Linus Walleij

[PATCH 08/10] pinctrl: single: support pinconf generic

[Haojian Zhuang]

Add pinconf generic support with POWER SOURCE, BIAS PULL.

Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/pinctrl/Kconfig          |    2 +-
 drivers/pinctrl/pinctrl-single.c |  286 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 274 insertions(+), 14 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 54e3588..cc2ef20 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -106,7 +106,7 @@ config PINCTRL_SINGLE
 	tristate "One-register-per-pin type device tree based pinctrl driver"
 	depends on OF
 	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
index 02cd412..a396944 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -20,6 +20,7 @@
 #include <linux/of_device.h>
 #include <linux/of_address.h>
 
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 
@@ -27,6 +28,9 @@
 
 #define DRIVER_NAME			"pinctrl-single"
 #define PCS_MUX_NAME			"pinctrl-single,pins"
+#define PCS_BIAS_NAME			"pinctrl-single,bias"
+#define PCS_POWER_SOURCE_NAME		"pinctrl-single,power-source"
+#define PCS_SCHMITT_NAME		"pinctrl-single,input-schmitt"
 #define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
 #define PCS_OFF_DISABLED		~0U
 #define PCS_MAX_GPIO_VALUES		3
@@ -137,6 +141,15 @@ struct pcs_name {
  * @foff:	value to turn mux off
  * @fmax:	max number of functions in fmask
  * @gmask:	gpio control mask
+ * @bmask:	mask of bias in pinconf
+ * @bshift:	offset of bias in pinconf
+ * @bdis:	bias disable value in pinconf
+ * @bpullup:	bias pull up value in pinconf
+ * @bpulldown:	bias pull down value in pinconf
+ * @ismask:	mask of input schmitt in pinconf
+ * @isshift:	offset of input schmitt in pinconf
+ * @psmask:	mask of power source in pinconf
+ * @psshift:	offset of power source in pinconf
  * @names:	array of register names for pins
  * @pins:	physical pins on the SoC
  * @pgtree:	pingroup index radix tree
@@ -164,6 +177,15 @@ struct pcs_device {
 	unsigned foff;
 	unsigned fmax;
 	unsigned gmask;
+	unsigned bmask;
+	unsigned bshift;
+	unsigned bdis;
+	unsigned bpullup;
+	unsigned bpulldown;
+	unsigned ismask;
+	unsigned isshift;
+	unsigned psmask;
+	unsigned psshift;
 	struct pcs_name *names;
 	struct pcs_data pins;
 	struct radix_tree_root pgtree;
@@ -268,9 +290,14 @@ static int pcs_get_group_pins(struct pinctrl_dev *pctldev,
 
 static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
 					struct seq_file *s,
-					unsigned offset)
+					unsigned pin)
 {
-	seq_printf(s, " " DRIVER_NAME);
+	struct pcs_device *pcs;
+	unsigned offset;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	offset = pin * (pcs->width / BITS_PER_BYTE);
+	seq_printf(s, " register value:0x%x", pcs_readl(pcs->base + offset));
 }
 
 static void pcs_dt_free_map(struct pinctrl_dev *pctldev,
@@ -468,28 +495,163 @@ static struct pinmux_ops pcs_pinmux_ops = {
 	.gpio_disable_free = pcs_disable_gpio,
 };
 
+static void pcs_free_pingroups(struct pcs_device *pcs);
+
 static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
 				unsigned pin, unsigned long *config)
 {
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	unsigned data;
+	u32 offset;
+
+	offset = pin * (pcs->width / BITS_PER_BYTE);
+	data = pcs_readl(pcs->base + offset);
+
+	switch (param) {
+	case PIN_CONFIG_POWER_SOURCE:
+		if (pcs->psmask == PCS_OFF_DISABLED
+			|| pcs->psshift == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->psmask;
+		data = data >> pcs->psshift;
+		*config = data;
+		return 0;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED
+			|| pcs->bdis == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->bmask;
+		*config = 0;
+		if (data == pcs->bdis)
+			return 0;
+		else
+			return -EINVAL;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED
+			|| pcs->bpullup == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->bmask;
+		*config = 0;
+		if (data == pcs->bpullup)
+			return 0;
+		else
+			return -EINVAL;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED
+			|| pcs->bpulldown == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->bmask;
+		*config = 0;
+		if (data == pcs->bpulldown)
+			return 0;
+		else
+			return -EINVAL;
+		break;
+	default:
+		break;
+	}
 	return -ENOTSUPP;
 }
 
 static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
 				unsigned pin, unsigned long config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_param = pinconf_to_config_param(config);
+	unsigned ret, mask = ~0UL;
+	u32 offset, data;
+
+	switch (config_param) {
+	case PIN_CONFIG_POWER_SOURCE:
+		if (pcs->psmask == PCS_OFF_DISABLED
+			|| pcs->psshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->psmask;
+		data = (pinconf_to_config_argument(config) << pcs->psshift)
+			& pcs->psmask;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->bmask;
+		data = (pinconf_to_config_argument(config) << pcs->bshift)
+			& pcs->bmask;
+		break;
+	default:
+		return 0;
+	}
+	offset = pin * (pcs->width / BITS_PER_BYTE);
+	ret = pcs_readl(pcs->base + offset) & ~mask;
+	pcs_writel(ret | data, pcs->base + offset);
+	return 0;
 }
 
 static int pcs_pinconf_group_get(struct pinctrl_dev *pctldev,
 				unsigned group, unsigned long *config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	struct pcs_pingroup *pins;
+
+	pins = radix_tree_lookup(&pcs->pgtree, group);
+	if (!pins) {
+		dev_err(pcs->dev, "%s could not find pingroup%i\n",
+			__func__, group);
+		return -EINVAL;
+	}
+	return pcs_pinconf_get(pctldev, pins->gpins[0], config);
 }
 
 static int pcs_pinconf_group_set(struct pinctrl_dev *pctldev,
 				unsigned group, unsigned long config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_param = pinconf_to_config_param(config);
+	struct pcs_pingroup *pins;
+	u32 offset, data;
+	unsigned ret, mask = ~0UL;
+	int i;
+
+	switch (config_param) {
+	case PIN_CONFIG_POWER_SOURCE:
+		if (pcs->psmask == PCS_OFF_DISABLED
+			|| pcs->psshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->psmask;
+		data = (pinconf_to_config_argument(config) << pcs->psshift)
+			& pcs->psmask;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->bmask;
+		data = (pinconf_to_config_argument(config) << pcs->bshift)
+			& pcs->bmask;
+		break;
+	default:
+		return 0;
+	}
+
+	pins = radix_tree_lookup(&pcs->pgtree, group);
+	if (!pins) {
+		dev_err(pcs->dev, "%s could not find pingroup%i\n",
+			__func__, group);
+		return -EINVAL;
+	}
+	for (i = 0; i < pins->ngpins; i++) {
+		offset = pins->gpins[i] * (pcs->width / BITS_PER_BYTE);
+		ret = pcs_readl(pcs->base + offset) & ~mask;
+		pcs_writel(ret | data, pcs->base + offset);
+	}
+	return 0;
 }
 
 static void pcs_pinconf_dbg_show(struct pinctrl_dev *pctldev,
@@ -503,6 +665,7 @@ static void pcs_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
 }
 
 static struct pinconf_ops pcs_pinconf_ops = {
+	.is_generic = true,
 	.pin_config_get = pcs_pinconf_get,
 	.pin_config_set = pcs_pinconf_set,
 	.pin_config_group_get = pcs_pinconf_group_get,
@@ -720,12 +883,16 @@ static int pcs_get_pin_by_offset(struct pcs_device *pcs, unsigned offset)
 static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 						struct device_node *np,
 						struct pinctrl_map **map,
+						unsigned num_configs,
 						const char **pgnames)
 {
 	struct pcs_func_vals *vals;
+	struct pinctrl_map *p = *map;
 	const __be32 *mux;
 	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
 	struct pcs_function *function;
+	unsigned long *config;
+	u32 value;
 
 	mux = of_get_property(np, PCS_MUX_NAME, &size);
 	if ((!mux) || (size < sizeof(*mux) * 2)) {
@@ -773,12 +940,42 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 	if (res < 0)
 		goto free_function;
 
-	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
-	(*map)->data.mux.group = np->name;
-	(*map)->data.mux.function = np->name;
+	p->type = PIN_MAP_TYPE_MUX_GROUP;
+	p->data.mux.group = np->name;
+	p->data.mux.function = np->name;
+
+	if (!num_configs)
+		return 0;
+	config = devm_kzalloc(pcs->dev, sizeof(*config) * num_configs,
+			      GFP_KERNEL);
+	if (!config) {
+		res = -ENOMEM;
+		goto free_pingroup;
+	}
+	index = 0;
+	if (!of_property_read_u32(np, PCS_SCHMITT_NAME, &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_INPUT_SCHMITT,
+						 value & 0xffff);
+	if (!of_property_read_u32(np, PCS_BIAS_NAME, &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_BIAS_DISABLE,
+						 value & 0xffff);
+	if (!of_property_read_u32(np, PCS_POWER_SOURCE_NAME, &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_POWER_SOURCE,
+						 value & 0xffff);
+	p++;
+	p->type = PIN_MAP_TYPE_CONFIGS_GROUP;
+	p->data.configs.group_or_pin = np->name;
+	p->data.configs.configs = config;
+	p->data.configs.num_configs = num_configs;
 
 	return 0;
 
+free_pingroup:
+	pcs_free_pingroups(pcs);
+
 free_function:
 	pcs_remove_function(pcs, function);
 
@@ -790,6 +987,29 @@ free_vals:
 
 	return res;
 }
+
+static int pcs_dt_check_maps(struct device_node *np, unsigned *num_maps,
+			     unsigned *num_configs)
+{
+	unsigned size;
+
+	*num_maps = 0;
+	*num_configs = 0;
+	if (of_get_property(np, PCS_MUX_NAME, &size))
+		(*num_maps)++;
+	if (of_get_property(np, PCS_SCHMITT_NAME, &size))
+		(*num_configs)++;
+	if (of_get_property(np, PCS_BIAS_NAME, &size))
+		(*num_configs)++;
+	if (of_get_property(np, PCS_POWER_SOURCE_NAME, &size))
+		(*num_configs)++;
+	if (*num_configs)
+		(*num_maps)++;
+	if (!(*num_maps))
+		return -EINVAL;
+	return 0;
+}
+
 /**
  * pcs_dt_node_to_map() - allocates and parses pinctrl maps
  * @pctldev: pinctrl instance
@@ -803,29 +1023,32 @@ static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
 {
 	struct pcs_device *pcs;
 	const char **pgnames;
+	unsigned num_configs;
 	int ret;
 
 	pcs = pinctrl_dev_get_drvdata(pctldev);
 
-	*map = devm_kzalloc(pcs->dev, sizeof(**map), GFP_KERNEL);
+	ret = pcs_dt_check_maps(np_config, num_maps, &num_configs);
+	if (ret)
+		return ret;
+
+	*map = devm_kzalloc(pcs->dev, sizeof(**map) * (*num_maps), GFP_KERNEL);
 	if (!map)
 		return -ENOMEM;
 
-	*num_maps = 0;
-
 	pgnames = devm_kzalloc(pcs->dev, sizeof(*pgnames), GFP_KERNEL);
 	if (!pgnames) {
 		ret = -ENOMEM;
 		goto free_map;
 	}
 
-	ret = pcs_parse_one_pinctrl_entry(pcs, np_config, map, pgnames);
+	ret = pcs_parse_one_pinctrl_entry(pcs, np_config, map,
+					  num_configs, pgnames);
 	if (ret < 0) {
 		dev_err(pcs->dev, "no pins entries for %s\n",
 			np_config->name);
 		goto free_pgnames;
 	}
-	*num_maps = 1;
 
 	return 0;
 
@@ -1015,6 +1238,43 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 	if (ret)
 		pcs->foff = PCS_OFF_DISABLED;
 
+	ret = of_property_read_u32(np, "pinctrl-single,power-source-mask",
+					&pcs->psmask);
+	if (ret)
+		pcs->psmask = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,power-source-shift",
+					&pcs->psshift);
+	if (ret)
+		pcs->psshift = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-mask",
+					&pcs->bmask);
+	if (ret)
+		pcs->bmask = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-shift",
+					&pcs->bshift);
+	if (ret)
+		pcs->bshift = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-disable",
+					&pcs->bdis);
+	if (ret)
+		pcs->bdis = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-pull-up",
+					&pcs->bpullup);
+	if (ret)
+		pcs->bpullup = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-pull-down",
+					&pcs->bpulldown);
+	if (ret)
+		pcs->bpulldown = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,input-schmitt-mask",
+					&pcs->ismask);
+	if (ret)
+		pcs->ismask = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,input-schmitt-shift",
+					&pcs->isshift);
+	if (ret)
+		pcs->isshift = PCS_OFF_DISABLED;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(pcs->dev, "could not get resource\n");
-- 
1.7.0.4

[PATCH 08/10] pinctrl: single: support pinconf generic

[Haojian Zhuang]

Add pinconf generic support with POWER SOURCE, BIAS PULL.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 drivers/pinctrl/Kconfig          |    2 +-
 drivers/pinctrl/pinctrl-single.c |  286 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 274 insertions(+), 14 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 54e3588..cc2ef20 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -106,7 +106,7 @@ config PINCTRL_SINGLE
 	tristate "One-register-per-pin type device tree based pinctrl driver"
 	depends on OF
 	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
index 02cd412..a396944 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -20,6 +20,7 @@
 #include <linux/of_device.h>
 #include <linux/of_address.h>
 
+#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 
@@ -27,6 +28,9 @@
 
 #define DRIVER_NAME			"pinctrl-single"
 #define PCS_MUX_NAME			"pinctrl-single,pins"
+#define PCS_BIAS_NAME			"pinctrl-single,bias"
+#define PCS_POWER_SOURCE_NAME		"pinctrl-single,power-source"
+#define PCS_SCHMITT_NAME		"pinctrl-single,input-schmitt"
 #define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
 #define PCS_OFF_DISABLED		~0U
 #define PCS_MAX_GPIO_VALUES		3
@@ -137,6 +141,15 @@ struct pcs_name {
  * @foff:	value to turn mux off
  * @fmax:	max number of functions in fmask
  * @gmask:	gpio control mask
+ * @bmask:	mask of bias in pinconf
+ * @bshift:	offset of bias in pinconf
+ * @bdis:	bias disable value in pinconf
+ * @bpullup:	bias pull up value in pinconf
+ * @bpulldown:	bias pull down value in pinconf
+ * @ismask:	mask of input schmitt in pinconf
+ * @isshift:	offset of input schmitt in pinconf
+ * @psmask:	mask of power source in pinconf
+ * @psshift:	offset of power source in pinconf
  * @names:	array of register names for pins
  * @pins:	physical pins on the SoC
  * @pgtree:	pingroup index radix tree
@@ -164,6 +177,15 @@ struct pcs_device {
 	unsigned foff;
 	unsigned fmax;
 	unsigned gmask;
+	unsigned bmask;
+	unsigned bshift;
+	unsigned bdis;
+	unsigned bpullup;
+	unsigned bpulldown;
+	unsigned ismask;
+	unsigned isshift;
+	unsigned psmask;
+	unsigned psshift;
 	struct pcs_name *names;
 	struct pcs_data pins;
 	struct radix_tree_root pgtree;
@@ -268,9 +290,14 @@ static int pcs_get_group_pins(struct pinctrl_dev *pctldev,
 
 static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
 					struct seq_file *s,
-					unsigned offset)
+					unsigned pin)
 {
-	seq_printf(s, " " DRIVER_NAME);
+	struct pcs_device *pcs;
+	unsigned offset;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	offset = pin * (pcs->width / BITS_PER_BYTE);
+	seq_printf(s, " register value:0x%x", pcs_readl(pcs->base + offset));
 }
 
 static void pcs_dt_free_map(struct pinctrl_dev *pctldev,
@@ -468,28 +495,163 @@ static struct pinmux_ops pcs_pinmux_ops = {
 	.gpio_disable_free = pcs_disable_gpio,
 };
 
+static void pcs_free_pingroups(struct pcs_device *pcs);
+
 static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
 				unsigned pin, unsigned long *config)
 {
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	unsigned data;
+	u32 offset;
+
+	offset = pin * (pcs->width / BITS_PER_BYTE);
+	data = pcs_readl(pcs->base + offset);
+
+	switch (param) {
+	case PIN_CONFIG_POWER_SOURCE:
+		if (pcs->psmask == PCS_OFF_DISABLED
+			|| pcs->psshift == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->psmask;
+		data = data >> pcs->psshift;
+		*config = data;
+		return 0;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED
+			|| pcs->bdis == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->bmask;
+		*config = 0;
+		if (data == pcs->bdis)
+			return 0;
+		else
+			return -EINVAL;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED
+			|| pcs->bpullup == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->bmask;
+		*config = 0;
+		if (data == pcs->bpullup)
+			return 0;
+		else
+			return -EINVAL;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED
+			|| pcs->bpulldown == PCS_OFF_DISABLED)
+			return -ENOTSUPP;
+		data &= pcs->bmask;
+		*config = 0;
+		if (data == pcs->bpulldown)
+			return 0;
+		else
+			return -EINVAL;
+		break;
+	default:
+		break;
+	}
 	return -ENOTSUPP;
 }
 
 static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
 				unsigned pin, unsigned long config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_param = pinconf_to_config_param(config);
+	unsigned ret, mask = ~0UL;
+	u32 offset, data;
+
+	switch (config_param) {
+	case PIN_CONFIG_POWER_SOURCE:
+		if (pcs->psmask == PCS_OFF_DISABLED
+			|| pcs->psshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->psmask;
+		data = (pinconf_to_config_argument(config) << pcs->psshift)
+			& pcs->psmask;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->bmask;
+		data = (pinconf_to_config_argument(config) << pcs->bshift)
+			& pcs->bmask;
+		break;
+	default:
+		return 0;
+	}
+	offset = pin * (pcs->width / BITS_PER_BYTE);
+	ret = pcs_readl(pcs->base + offset) & ~mask;
+	pcs_writel(ret | data, pcs->base + offset);
+	return 0;
 }
 
 static int pcs_pinconf_group_get(struct pinctrl_dev *pctldev,
 				unsigned group, unsigned long *config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	struct pcs_pingroup *pins;
+
+	pins = radix_tree_lookup(&pcs->pgtree, group);
+	if (!pins) {
+		dev_err(pcs->dev, "%s could not find pingroup%i\n",
+			__func__, group);
+		return -EINVAL;
+	}
+	return pcs_pinconf_get(pctldev, pins->gpins[0], config);
 }
 
 static int pcs_pinconf_group_set(struct pinctrl_dev *pctldev,
 				unsigned group, unsigned long config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_param = pinconf_to_config_param(config);
+	struct pcs_pingroup *pins;
+	u32 offset, data;
+	unsigned ret, mask = ~0UL;
+	int i;
+
+	switch (config_param) {
+	case PIN_CONFIG_POWER_SOURCE:
+		if (pcs->psmask == PCS_OFF_DISABLED
+			|| pcs->psshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->psmask;
+		data = (pinconf_to_config_argument(config) << pcs->psshift)
+			& pcs->psmask;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (pcs->bmask == PCS_OFF_DISABLED
+			|| pcs->bshift == PCS_OFF_DISABLED)
+			return 0;
+		mask = pcs->bmask;
+		data = (pinconf_to_config_argument(config) << pcs->bshift)
+			& pcs->bmask;
+		break;
+	default:
+		return 0;
+	}
+
+	pins = radix_tree_lookup(&pcs->pgtree, group);
+	if (!pins) {
+		dev_err(pcs->dev, "%s could not find pingroup%i\n",
+			__func__, group);
+		return -EINVAL;
+	}
+	for (i = 0; i < pins->ngpins; i++) {
+		offset = pins->gpins[i] * (pcs->width / BITS_PER_BYTE);
+		ret = pcs_readl(pcs->base + offset) & ~mask;
+		pcs_writel(ret | data, pcs->base + offset);
+	}
+	return 0;
 }
 
 static void pcs_pinconf_dbg_show(struct pinctrl_dev *pctldev,
@@ -503,6 +665,7 @@ static void pcs_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
 }
 
 static struct pinconf_ops pcs_pinconf_ops = {
+	.is_generic = true,
 	.pin_config_get = pcs_pinconf_get,
 	.pin_config_set = pcs_pinconf_set,
 	.pin_config_group_get = pcs_pinconf_group_get,
@@ -720,12 +883,16 @@ static int pcs_get_pin_by_offset(struct pcs_device *pcs, unsigned offset)
 static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 						struct device_node *np,
 						struct pinctrl_map **map,
+						unsigned num_configs,
 						const char **pgnames)
 {
 	struct pcs_func_vals *vals;
+	struct pinctrl_map *p = *map;
 	const __be32 *mux;
 	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
 	struct pcs_function *function;
+	unsigned long *config;
+	u32 value;
 
 	mux = of_get_property(np, PCS_MUX_NAME, &size);
 	if ((!mux) || (size < sizeof(*mux) * 2)) {
@@ -773,12 +940,42 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 	if (res < 0)
 		goto free_function;
 
-	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
-	(*map)->data.mux.group = np->name;
-	(*map)->data.mux.function = np->name;
+	p->type = PIN_MAP_TYPE_MUX_GROUP;
+	p->data.mux.group = np->name;
+	p->data.mux.function = np->name;
+
+	if (!num_configs)
+		return 0;
+	config = devm_kzalloc(pcs->dev, sizeof(*config) * num_configs,
+			      GFP_KERNEL);
+	if (!config) {
+		res = -ENOMEM;
+		goto free_pingroup;
+	}
+	index = 0;
+	if (!of_property_read_u32(np, PCS_SCHMITT_NAME, &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_INPUT_SCHMITT,
+						 value & 0xffff);
+	if (!of_property_read_u32(np, PCS_BIAS_NAME, &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_BIAS_DISABLE,
+						 value & 0xffff);
+	if (!of_property_read_u32(np, PCS_POWER_SOURCE_NAME, &value))
+		config[index++] =
+			pinconf_to_config_packed(PIN_CONFIG_POWER_SOURCE,
+						 value & 0xffff);
+	p++;
+	p->type = PIN_MAP_TYPE_CONFIGS_GROUP;
+	p->data.configs.group_or_pin = np->name;
+	p->data.configs.configs = config;
+	p->data.configs.num_configs = num_configs;
 
 	return 0;
 
+free_pingroup:
+	pcs_free_pingroups(pcs);
+
 free_function:
 	pcs_remove_function(pcs, function);
 
@@ -790,6 +987,29 @@ free_vals:
 
 	return res;
 }
+
+static int pcs_dt_check_maps(struct device_node *np, unsigned *num_maps,
+			     unsigned *num_configs)
+{
+	unsigned size;
+
+	*num_maps = 0;
+	*num_configs = 0;
+	if (of_get_property(np, PCS_MUX_NAME, &size))
+		(*num_maps)++;
+	if (of_get_property(np, PCS_SCHMITT_NAME, &size))
+		(*num_configs)++;
+	if (of_get_property(np, PCS_BIAS_NAME, &size))
+		(*num_configs)++;
+	if (of_get_property(np, PCS_POWER_SOURCE_NAME, &size))
+		(*num_configs)++;
+	if (*num_configs)
+		(*num_maps)++;
+	if (!(*num_maps))
+		return -EINVAL;
+	return 0;
+}
+
 /**
  * pcs_dt_node_to_map() - allocates and parses pinctrl maps
  * @pctldev: pinctrl instance
@@ -803,29 +1023,32 @@ static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
 {
 	struct pcs_device *pcs;
 	const char **pgnames;
+	unsigned num_configs;
 	int ret;
 
 	pcs = pinctrl_dev_get_drvdata(pctldev);
 
-	*map = devm_kzalloc(pcs->dev, sizeof(**map), GFP_KERNEL);
+	ret = pcs_dt_check_maps(np_config, num_maps, &num_configs);
+	if (ret)
+		return ret;
+
+	*map = devm_kzalloc(pcs->dev, sizeof(**map) * (*num_maps), GFP_KERNEL);
 	if (!map)
 		return -ENOMEM;
 
-	*num_maps = 0;
-
 	pgnames = devm_kzalloc(pcs->dev, sizeof(*pgnames), GFP_KERNEL);
 	if (!pgnames) {
 		ret = -ENOMEM;
 		goto free_map;
 	}
 
-	ret = pcs_parse_one_pinctrl_entry(pcs, np_config, map, pgnames);
+	ret = pcs_parse_one_pinctrl_entry(pcs, np_config, map,
+					  num_configs, pgnames);
 	if (ret < 0) {
 		dev_err(pcs->dev, "no pins entries for %s\n",
 			np_config->name);
 		goto free_pgnames;
 	}
-	*num_maps = 1;
 
 	return 0;
 
@@ -1015,6 +1238,43 @@ static int __devinit pcs_probe(struct platform_device *pdev)
 	if (ret)
 		pcs->foff = PCS_OFF_DISABLED;
 
+	ret = of_property_read_u32(np, "pinctrl-single,power-source-mask",
+					&pcs->psmask);
+	if (ret)
+		pcs->psmask = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,power-source-shift",
+					&pcs->psshift);
+	if (ret)
+		pcs->psshift = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-mask",
+					&pcs->bmask);
+	if (ret)
+		pcs->bmask = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-shift",
+					&pcs->bshift);
+	if (ret)
+		pcs->bshift = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-disable",
+					&pcs->bdis);
+	if (ret)
+		pcs->bdis = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-pull-up",
+					&pcs->bpullup);
+	if (ret)
+		pcs->bpullup = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,bias-pull-down",
+					&pcs->bpulldown);
+	if (ret)
+		pcs->bpulldown = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,input-schmitt-mask",
+					&pcs->ismask);
+	if (ret)
+		pcs->ismask = PCS_OFF_DISABLED;
+	ret = of_property_read_u32(np, "pinctrl-single,input-schmitt-shift",
+					&pcs->isshift);
+	if (ret)
+		pcs->isshift = PCS_OFF_DISABLED;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(pcs->dev, "could not get resource\n");
-- 
1.7.0.4

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Linus Walleij]

On Thu, Oct 18, 2012 at 11:07 AM, Haojian Zhuang
<haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Add pinconf generic support with POWER SOURCE, BIAS PULL.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

I really like the looks of this, good job Haojian!

Now we just need to hear what Tony says about it...

Yours,
Linus Walleij

[PATCH 08/10] pinctrl: single: support pinconf generic

[Linus Walleij]

On Thu, Oct 18, 2012 at 11:07 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:

> Add pinconf generic support with POWER SOURCE, BIAS PULL.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>

I really like the looks of this, good job Haojian!

Now we just need to hear what Tony says about it...

Yours,
Linus Walleij

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> [121018 11:32]:
> On Thu, Oct 18, 2012 at 11:07 AM, Haojian Zhuang
> <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > Add pinconf generic support with POWER SOURCE, BIAS PULL.
> >
> > Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> I really like the looks of this, good job Haojian!
> 
> Now we just need to hear what Tony says about it...

Hey that's cool, maybe I'll find some use for those too :)
I'll take a closer look tonigh or on Friday.

Regards,

Tony

[PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [121018 11:32]:
> On Thu, Oct 18, 2012 at 11:07 AM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
> 
> > Add pinconf generic support with POWER SOURCE, BIAS PULL.
> >
> > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> 
> I really like the looks of this, good job Haojian!
> 
> Now we just need to hear what Tony says about it...

Hey that's cool, maybe I'll find some use for those too :)
I'll take a closer look tonigh or on Friday.

Regards,

Tony

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Haojian Zhuang]

On Fri, Oct 19, 2012 at 6:29 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> [121018 11:32]:
>> On Thu, Oct 18, 2012 at 11:07 AM, Haojian Zhuang
>> <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> > Add pinconf generic support with POWER SOURCE, BIAS PULL.
>> >
>> > Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> I really like the looks of this, good job Haojian!
>>
>> Now we just need to hear what Tony says about it...
>
> Hey that's cool, maybe I'll find some use for those too :)
> I'll take a closer look tonigh or on Friday.
>
> Regards,
>
> Tony

I wonder whether gpio function in OMAP is also configured in the pinmux
register. For example, the function is 3bit field in pinmux register
of Marvell's
PXA/MMP silicon. GPIO function is the one of function.

If the usage case is same in OMAP silicon, I can merge
"pinctrl-single,gpio-mask"
with "pinctrl-single,function-mask".

Do we need "pinctrl-single,gpio-disable" at here? I want to remove it,
but I don't
know whether it's necessary in OMAP case.

Regards
Haojian

[PATCH 08/10] pinctrl: single: support pinconf generic

[Haojian Zhuang]

On Fri, Oct 19, 2012 at 6:29 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Linus Walleij <linus.walleij@linaro.org> [121018 11:32]:
>> On Thu, Oct 18, 2012 at 11:07 AM, Haojian Zhuang
>> <haojian.zhuang@gmail.com> wrote:
>>
>> > Add pinconf generic support with POWER SOURCE, BIAS PULL.
>> >
>> > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
>>
>> I really like the looks of this, good job Haojian!
>>
>> Now we just need to hear what Tony says about it...
>
> Hey that's cool, maybe I'll find some use for those too :)
> I'll take a closer look tonigh or on Friday.
>
> Regards,
>
> Tony

I wonder whether gpio function in OMAP is also configured in the pinmux
register. For example, the function is 3bit field in pinmux register
of Marvell's
PXA/MMP silicon. GPIO function is the one of function.

If the usage case is same in OMAP silicon, I can merge
"pinctrl-single,gpio-mask"
with "pinctrl-single,function-mask".

Do we need "pinctrl-single,gpio-disable" at here? I want to remove it,
but I don't
know whether it's necessary in OMAP case.

Regards
Haojian

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 19:25]:
> 
> I wonder whether gpio function in OMAP is also configured in the pinmux
> register. For example, the function is 3bit field in pinmux register
> of Marvell's
> PXA/MMP silicon. GPIO function is the one of function.
> 
> If the usage case is same in OMAP silicon, I can merge
> "pinctrl-single,gpio-mask"
> with "pinctrl-single,function-mask".
> 
> Do we need "pinctrl-single,gpio-disable" at here? I want to remove it,
> but I don't
> know whether it's necessary in OMAP case.

Maybe.. I'll take a look at at that tomorrow.

Regards,

Tony

[PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121018 19:25]:
> 
> I wonder whether gpio function in OMAP is also configured in the pinmux
> register. For example, the function is 3bit field in pinmux register
> of Marvell's
> PXA/MMP silicon. GPIO function is the one of function.
> 
> If the usage case is same in OMAP silicon, I can merge
> "pinctrl-single,gpio-mask"
> with "pinctrl-single,function-mask".
> 
> Do we need "pinctrl-single,gpio-disable" at here? I want to remove it,
> but I don't
> know whether it's necessary in OMAP case.

Maybe.. I'll take a look at at that tomorrow.

Regards,

Tony

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [121018 19:41]:
> * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 19:25]:
> > 
> > I wonder whether gpio function in OMAP is also configured in the pinmux
> > register. For example, the function is 3bit field in pinmux register
> > of Marvell's
> > PXA/MMP silicon. GPIO function is the one of function.
> > 
> > If the usage case is same in OMAP silicon, I can merge
> > "pinctrl-single,gpio-mask"
> > with "pinctrl-single,function-mask".
> > 
> > Do we need "pinctrl-single,gpio-disable" at here? I want to remove it,
> > but I don't
> > know whether it's necessary in OMAP case.
> 
> Maybe.. I'll take a look at at that tomorrow.

This one does not apply to v3.7-rc1 because of the pinctrl,bits
support merged from Peter Ujfalusi. Can you please update and
repost?

Thanks,

Tony

[PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [121018 19:41]:
> * Haojian Zhuang <haojian.zhuang@gmail.com> [121018 19:25]:
> > 
> > I wonder whether gpio function in OMAP is also configured in the pinmux
> > register. For example, the function is 3bit field in pinmux register
> > of Marvell's
> > PXA/MMP silicon. GPIO function is the one of function.
> > 
> > If the usage case is same in OMAP silicon, I can merge
> > "pinctrl-single,gpio-mask"
> > with "pinctrl-single,function-mask".
> > 
> > Do we need "pinctrl-single,gpio-disable" at here? I want to remove it,
> > but I don't
> > know whether it's necessary in OMAP case.
> 
> Maybe.. I'll take a look at at that tomorrow.

This one does not apply to v3.7-rc1 because of the pinctrl,bits
support merged from Peter Ujfalusi. Can you please update and
repost?

Thanks,

Tony

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 19:25]:
> 
> I wonder whether gpio function in OMAP is also configured in the pinmux
> register. For example, the function is 3bit field in pinmux register
> of Marvell's
> PXA/MMP silicon. GPIO function is the one of function.

For omaps, the mux registers in padconf area just control
the routing of the signal, pulls and wake-up events. So
gpio mode is a number in the mode bits [2:0]. It's mode 3
for some omaps and mode 4 for some omaps. All the other GPIO
related registers are elsewhere.

Sounds like this is different from what you are describing,
but if you want to compare it, the register bits in the
legacy mach-omap2/mux.h are:

/* 34xx mux mode options for each pin. See TRM for options */
#define OMAP_MUX_MODE0      0
#define OMAP_MUX_MODE1      1
#define OMAP_MUX_MODE2      2
#define OMAP_MUX_MODE3      3
#define OMAP_MUX_MODE4      4
#define OMAP_MUX_MODE5      5
#define OMAP_MUX_MODE6      6
#define OMAP_MUX_MODE7      7
               
/* 24xx/34xx mux bit defines */
#define OMAP_PULL_ENA                   (1 << 3)
#define OMAP_PULL_UP                    (1 << 4)
#define OMAP_ALTELECTRICALSEL           (1 << 5)
               
/* 34xx specific mux bit defines */
#define OMAP_INPUT_EN                   (1 << 8)
#define OMAP_OFF_EN                     (1 << 9)
#define OMAP_OFFOUT_EN                  (1 << 10)
#define OMAP_OFFOUT_VAL                 (1 << 11)
#define OMAP_OFF_PULL_EN                (1 << 12)
#define OMAP_OFF_PULL_UP                (1 << 13)
#define OMAP_WAKEUP_EN                  (1 << 14)
       
/* 44xx specific mux bit defines */
#define OMAP_WAKEUP_EVENT               (1 << 15)

Regards,

Tony

[PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121018 19:25]:
> 
> I wonder whether gpio function in OMAP is also configured in the pinmux
> register. For example, the function is 3bit field in pinmux register
> of Marvell's
> PXA/MMP silicon. GPIO function is the one of function.

For omaps, the mux registers in padconf area just control
the routing of the signal, pulls and wake-up events. So
gpio mode is a number in the mode bits [2:0]. It's mode 3
for some omaps and mode 4 for some omaps. All the other GPIO
related registers are elsewhere.

Sounds like this is different from what you are describing,
but if you want to compare it, the register bits in the
legacy mach-omap2/mux.h are:

/* 34xx mux mode options for each pin. See TRM for options */
#define OMAP_MUX_MODE0      0
#define OMAP_MUX_MODE1      1
#define OMAP_MUX_MODE2      2
#define OMAP_MUX_MODE3      3
#define OMAP_MUX_MODE4      4
#define OMAP_MUX_MODE5      5
#define OMAP_MUX_MODE6      6
#define OMAP_MUX_MODE7      7
               
/* 24xx/34xx mux bit defines */
#define OMAP_PULL_ENA                   (1 << 3)
#define OMAP_PULL_UP                    (1 << 4)
#define OMAP_ALTELECTRICALSEL           (1 << 5)
               
/* 34xx specific mux bit defines */
#define OMAP_INPUT_EN                   (1 << 8)
#define OMAP_OFF_EN                     (1 << 9)
#define OMAP_OFFOUT_EN                  (1 << 10)
#define OMAP_OFFOUT_VAL                 (1 << 11)
#define OMAP_OFF_PULL_EN                (1 << 12)
#define OMAP_OFF_PULL_UP                (1 << 13)
#define OMAP_WAKEUP_EN                  (1 << 14)
       
/* 44xx specific mux bit defines */
#define OMAP_WAKEUP_EVENT               (1 << 15)

Regards,

Tony

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 02:08]:
> Add pinconf generic support with POWER SOURCE, BIAS PULL.
...

> +	case PIN_CONFIG_POWER_SOURCE:
> +		if (pcs->psmask == PCS_OFF_DISABLED
> +			|| pcs->psshift == PCS_OFF_DISABLED)
> +			return -ENOTSUPP;
> +		data &= pcs->psmask;
> +		data = data >> pcs->psshift;
> +		*config = data;
> +		return 0;
> +		break;

Hmm, only slightly related to this patch, mostly a generic
question to others: Do others have any mux registers with
status bits for things like PIN_CONFIG_POWER_SOURCE?

I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
for omap MMC. But there's also a status bit that needs to be
checked for that. I think there was some other similar mux
register for USB PHY that has a status register.

So I'm wondering should the checking for status bit be handled
in the pinctrl consume driver? Or should we have some bindings
for that?

Regards,

Tony

[PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121018 02:08]:
> Add pinconf generic support with POWER SOURCE, BIAS PULL.
...

> +	case PIN_CONFIG_POWER_SOURCE:
> +		if (pcs->psmask == PCS_OFF_DISABLED
> +			|| pcs->psshift == PCS_OFF_DISABLED)
> +			return -ENOTSUPP;
> +		data &= pcs->psmask;
> +		data = data >> pcs->psshift;
> +		*config = data;
> +		return 0;
> +		break;

Hmm, only slightly related to this patch, mostly a generic
question to others: Do others have any mux registers with
status bits for things like PIN_CONFIG_POWER_SOURCE?

I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
for omap MMC. But there's also a status bit that needs to be
checked for that. I think there was some other similar mux
register for USB PHY that has a status register.

So I'm wondering should the checking for status bit be handled
in the pinctrl consume driver? Or should we have some bindings
for that?

Regards,

Tony

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Haojian Zhuang]

On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 02:08]:
>> Add pinconf generic support with POWER SOURCE, BIAS PULL.
> ...
>
>> +     case PIN_CONFIG_POWER_SOURCE:
>> +             if (pcs->psmask == PCS_OFF_DISABLED
>> +                     || pcs->psshift == PCS_OFF_DISABLED)
>> +                     return -ENOTSUPP;
>> +             data &= pcs->psmask;
>> +             data = data >> pcs->psshift;
>> +             *config = data;
>> +             return 0;
>> +             break;
>
> Hmm, only slightly related to this patch, mostly a generic
> question to others: Do others have any mux registers with
> status bits for things like PIN_CONFIG_POWER_SOURCE?
>
> I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
> for omap MMC. But there's also a status bit that needs to be
> checked for that. I think there was some other similar mux
> register for USB PHY that has a status register.
>
> So I'm wondering should the checking for status bit be handled
> in the pinctrl consume driver? Or should we have some bindings
> for that?
>

Do you mean that the status register only exists in USB PHY controller or
MMC controller?

If so, could we use regulator framework in USB PHY or MMC driver?

Regards
Haojian

[PATCH 08/10] pinctrl: single: support pinconf generic

[Haojian Zhuang]

On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Haojian Zhuang <haojian.zhuang@gmail.com> [121018 02:08]:
>> Add pinconf generic support with POWER SOURCE, BIAS PULL.
> ...
>
>> +     case PIN_CONFIG_POWER_SOURCE:
>> +             if (pcs->psmask == PCS_OFF_DISABLED
>> +                     || pcs->psshift == PCS_OFF_DISABLED)
>> +                     return -ENOTSUPP;
>> +             data &= pcs->psmask;
>> +             data = data >> pcs->psshift;
>> +             *config = data;
>> +             return 0;
>> +             break;
>
> Hmm, only slightly related to this patch, mostly a generic
> question to others: Do others have any mux registers with
> status bits for things like PIN_CONFIG_POWER_SOURCE?
>
> I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
> for omap MMC. But there's also a status bit that needs to be
> checked for that. I think there was some other similar mux
> register for USB PHY that has a status register.
>
> So I'm wondering should the checking for status bit be handled
> in the pinctrl consume driver? Or should we have some bindings
> for that?
>

Do you mean that the status register only exists in USB PHY controller or
MMC controller?

If so, could we use regulator framework in USB PHY or MMC driver?

Regards
Haojian

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121022 03:11]:
> On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 02:08]:
> >> Add pinconf generic support with POWER SOURCE, BIAS PULL.
> > ...
> >
> >> +     case PIN_CONFIG_POWER_SOURCE:
> >> +             if (pcs->psmask == PCS_OFF_DISABLED
> >> +                     || pcs->psshift == PCS_OFF_DISABLED)
> >> +                     return -ENOTSUPP;
> >> +             data &= pcs->psmask;
> >> +             data = data >> pcs->psshift;
> >> +             *config = data;
> >> +             return 0;
> >> +             break;
> >
> > Hmm, only slightly related to this patch, mostly a generic
> > question to others: Do others have any mux registers with
> > status bits for things like PIN_CONFIG_POWER_SOURCE?
> >
> > I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
> > for omap MMC. But there's also a status bit that needs to be
> > checked for that. I think there was some other similar mux
> > register for USB PHY that has a status register.
> >
> > So I'm wondering should the checking for status bit be handled
> > in the pinctrl consume driver? Or should we have some bindings
> > for that?
> >
> 
> Do you mean that the status register only exists in USB PHY controller or
> MMC controller?

The status register is in the MMC PBIAS register that is mux
related otherwise. From OMAP4470_ES1.0_PUBLIC_TRM_vE.pdf,
Table 19-599. CONTROL_PBIASLITE:

Bits
26	MMC1_PWDNZ
25	MMC1_PBIASLITE_HIZ_MODE
24	MMC1_PBIASLITE_SUPPLY_HI_OUT
23	MMC1_PBIASLITE_VMODE_ERROR	then this bit needs to clear..
22	MMC1_PBIASLITE_PWRDNZ
21	MMC1_PBIASLITE_VMODE		..after VMODE bit is set to 3V
 
> If so, could we use regulator framework in USB PHY or MMC driver?

Yes we could use regulator framework for that that. Or just read the
status in the MMC driver for that bit if nobody else has mixed
mux-regulator needs like this.

The sequence is MMC specific, so from that point of view it would
make sense to have the logic in the MMC driver.

Regards,

Tony

[PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121022 03:11]:
> On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Haojian Zhuang <haojian.zhuang@gmail.com> [121018 02:08]:
> >> Add pinconf generic support with POWER SOURCE, BIAS PULL.
> > ...
> >
> >> +     case PIN_CONFIG_POWER_SOURCE:
> >> +             if (pcs->psmask == PCS_OFF_DISABLED
> >> +                     || pcs->psshift == PCS_OFF_DISABLED)
> >> +                     return -ENOTSUPP;
> >> +             data &= pcs->psmask;
> >> +             data = data >> pcs->psshift;
> >> +             *config = data;
> >> +             return 0;
> >> +             break;
> >
> > Hmm, only slightly related to this patch, mostly a generic
> > question to others: Do others have any mux registers with
> > status bits for things like PIN_CONFIG_POWER_SOURCE?
> >
> > I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
> > for omap MMC. But there's also a status bit that needs to be
> > checked for that. I think there was some other similar mux
> > register for USB PHY that has a status register.
> >
> > So I'm wondering should the checking for status bit be handled
> > in the pinctrl consume driver? Or should we have some bindings
> > for that?
> >
> 
> Do you mean that the status register only exists in USB PHY controller or
> MMC controller?

The status register is in the MMC PBIAS register that is mux
related otherwise. From OMAP4470_ES1.0_PUBLIC_TRM_vE.pdf,
Table 19-599. CONTROL_PBIASLITE:

Bits
26	MMC1_PWDNZ
25	MMC1_PBIASLITE_HIZ_MODE
24	MMC1_PBIASLITE_SUPPLY_HI_OUT
23	MMC1_PBIASLITE_VMODE_ERROR	then this bit needs to clear..
22	MMC1_PBIASLITE_PWRDNZ
21	MMC1_PBIASLITE_VMODE		..after VMODE bit is set to 3V
 
> If so, could we use regulator framework in USB PHY or MMC driver?

Yes we could use regulator framework for that that. Or just read the
status in the MMC driver for that bit if nobody else has mixed
mux-regulator needs like this.

The sequence is MMC specific, so from that point of view it would
make sense to have the logic in the MMC driver.

Regards,

Tony

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [121022 10:11]:
> * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121022 03:11]:
> > On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > > * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 02:08]:
> > >> Add pinconf generic support with POWER SOURCE, BIAS PULL.
> > > ...
> > >
> > >> +     case PIN_CONFIG_POWER_SOURCE:
> > >> +             if (pcs->psmask == PCS_OFF_DISABLED
> > >> +                     || pcs->psshift == PCS_OFF_DISABLED)
> > >> +                     return -ENOTSUPP;
> > >> +             data &= pcs->psmask;
> > >> +             data = data >> pcs->psshift;
> > >> +             *config = data;
> > >> +             return 0;
> > >> +             break;
> > >
> > > Hmm, only slightly related to this patch, mostly a generic
> > > question to others: Do others have any mux registers with
> > > status bits for things like PIN_CONFIG_POWER_SOURCE?
> > >
> > > I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
> > > for omap MMC. But there's also a status bit that needs to be
> > > checked for that. I think there was some other similar mux
> > > register for USB PHY that has a status register.
> > >
> > > So I'm wondering should the checking for status bit be handled
> > > in the pinctrl consume driver? Or should we have some bindings
> > > for that?
> > >
> > 
> > Do you mean that the status register only exists in USB PHY controller or
> > MMC controller?
> 
> The status register is in the MMC PBIAS register that is mux
> related otherwise. From OMAP4470_ES1.0_PUBLIC_TRM_vE.pdf,
> Table 19-599. CONTROL_PBIASLITE:
> 
> Bits
> 26	MMC1_PWDNZ
> 25	MMC1_PBIASLITE_HIZ_MODE
> 24	MMC1_PBIASLITE_SUPPLY_HI_OUT
> 23	MMC1_PBIASLITE_VMODE_ERROR	then this bit needs to clear..
> 22	MMC1_PBIASLITE_PWRDNZ
> 21	MMC1_PBIASLITE_VMODE		..after VMODE bit is set to 3V
>  
> > If so, could we use regulator framework in USB PHY or MMC driver?
> 
> Yes we could use regulator framework for that that. Or just read the
> status in the MMC driver for that bit if nobody else has mixed
> mux-regulator needs like this.
> 
> The sequence is MMC specific, so from that point of view it would
> make sense to have the logic in the MMC driver.

Well it turns out the VMODE_ERROR bit is not just for VMODE, it's a
comparator that can also triggers for the other invalid states for
CONTROL_PBIASLITE pinconf register. So hiding VMODE_ERROR into a
regulator would be wrong. For now, VMODE best handled using
PIN_CONFIG_POWER_SOURCE and let the MMC driver do the checking
using the pinconf API.

Regards,

Tony

[PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [121022 10:11]:
> * Haojian Zhuang <haojian.zhuang@gmail.com> [121022 03:11]:
> > On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony@atomide.com> wrote:
> > > * Haojian Zhuang <haojian.zhuang@gmail.com> [121018 02:08]:
> > >> Add pinconf generic support with POWER SOURCE, BIAS PULL.
> > > ...
> > >
> > >> +     case PIN_CONFIG_POWER_SOURCE:
> > >> +             if (pcs->psmask == PCS_OFF_DISABLED
> > >> +                     || pcs->psshift == PCS_OFF_DISABLED)
> > >> +                     return -ENOTSUPP;
> > >> +             data &= pcs->psmask;
> > >> +             data = data >> pcs->psshift;
> > >> +             *config = data;
> > >> +             return 0;
> > >> +             break;
> > >
> > > Hmm, only slightly related to this patch, mostly a generic
> > > question to others: Do others have any mux registers with
> > > status bits for things like PIN_CONFIG_POWER_SOURCE?
> > >
> > > I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
> > > for omap MMC. But there's also a status bit that needs to be
> > > checked for that. I think there was some other similar mux
> > > register for USB PHY that has a status register.
> > >
> > > So I'm wondering should the checking for status bit be handled
> > > in the pinctrl consume driver? Or should we have some bindings
> > > for that?
> > >
> > 
> > Do you mean that the status register only exists in USB PHY controller or
> > MMC controller?
> 
> The status register is in the MMC PBIAS register that is mux
> related otherwise. From OMAP4470_ES1.0_PUBLIC_TRM_vE.pdf,
> Table 19-599. CONTROL_PBIASLITE:
> 
> Bits
> 26	MMC1_PWDNZ
> 25	MMC1_PBIASLITE_HIZ_MODE
> 24	MMC1_PBIASLITE_SUPPLY_HI_OUT
> 23	MMC1_PBIASLITE_VMODE_ERROR	then this bit needs to clear..
> 22	MMC1_PBIASLITE_PWRDNZ
> 21	MMC1_PBIASLITE_VMODE		..after VMODE bit is set to 3V
>  
> > If so, could we use regulator framework in USB PHY or MMC driver?
> 
> Yes we could use regulator framework for that that. Or just read the
> status in the MMC driver for that bit if nobody else has mixed
> mux-regulator needs like this.
> 
> The sequence is MMC specific, so from that point of view it would
> make sense to have the logic in the MMC driver.

Well it turns out the VMODE_ERROR bit is not just for VMODE, it's a
comparator that can also triggers for the other invalid states for
CONTROL_PBIASLITE pinconf register. So hiding VMODE_ERROR into a
regulator would be wrong. For now, VMODE best handled using
PIN_CONFIG_POWER_SOURCE and let the MMC driver do the checking
using the pinconf API.

Regards,

Tony

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Haojian Zhuang]

On Fri, Oct 26, 2012 at 7:43 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [121022 10:11]:
>> * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121022 03:11]:
>> > On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>> > > * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 02:08]:
>> > >> Add pinconf generic support with POWER SOURCE, BIAS PULL.
>> > > ...
>> > >
>> > >> +     case PIN_CONFIG_POWER_SOURCE:
>> > >> +             if (pcs->psmask == PCS_OFF_DISABLED
>> > >> +                     || pcs->psshift == PCS_OFF_DISABLED)
>> > >> +                     return -ENOTSUPP;
>> > >> +             data &= pcs->psmask;
>> > >> +             data = data >> pcs->psshift;
>> > >> +             *config = data;
>> > >> +             return 0;
>> > >> +             break;
>> > >
>> > > Hmm, only slightly related to this patch, mostly a generic
>> > > question to others: Do others have any mux registers with
>> > > status bits for things like PIN_CONFIG_POWER_SOURCE?
>> > >
>> > > I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
>> > > for omap MMC. But there's also a status bit that needs to be
>> > > checked for that. I think there was some other similar mux
>> > > register for USB PHY that has a status register.
>> > >
>> > > So I'm wondering should the checking for status bit be handled
>> > > in the pinctrl consume driver? Or should we have some bindings
>> > > for that?
>> > >
>> >
>> > Do you mean that the status register only exists in USB PHY controller or
>> > MMC controller?
>>
>> The status register is in the MMC PBIAS register that is mux
>> related otherwise. From OMAP4470_ES1.0_PUBLIC_TRM_vE.pdf,
>> Table 19-599. CONTROL_PBIASLITE:
>>
>> Bits
>> 26    MMC1_PWDNZ
>> 25    MMC1_PBIASLITE_HIZ_MODE
>> 24    MMC1_PBIASLITE_SUPPLY_HI_OUT
>> 23    MMC1_PBIASLITE_VMODE_ERROR      then this bit needs to clear..
>> 22    MMC1_PBIASLITE_PWRDNZ
>> 21    MMC1_PBIASLITE_VMODE            ..after VMODE bit is set to 3V
>>
>> > If so, could we use regulator framework in USB PHY or MMC driver?
>>
>> Yes we could use regulator framework for that that. Or just read the
>> status in the MMC driver for that bit if nobody else has mixed
>> mux-regulator needs like this.
>>
>> The sequence is MMC specific, so from that point of view it would
>> make sense to have the logic in the MMC driver.
>
> Well it turns out the VMODE_ERROR bit is not just for VMODE, it's a
> comparator that can also triggers for the other invalid states for
> CONTROL_PBIASLITE pinconf register. So hiding VMODE_ERROR into a
> regulator would be wrong. For now, VMODE best handled using
> PIN_CONFIG_POWER_SOURCE and let the MMC driver do the checking
> using the pinconf API.
>
> Regards,
>
> Tony

Could you share the link of downloading the spec?

Regards
Haojian

[PATCH 08/10] pinctrl: single: support pinconf generic

[Haojian Zhuang]

On Fri, Oct 26, 2012 at 7:43 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Tony Lindgren <tony@atomide.com> [121022 10:11]:
>> * Haojian Zhuang <haojian.zhuang@gmail.com> [121022 03:11]:
>> > On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony@atomide.com> wrote:
>> > > * Haojian Zhuang <haojian.zhuang@gmail.com> [121018 02:08]:
>> > >> Add pinconf generic support with POWER SOURCE, BIAS PULL.
>> > > ...
>> > >
>> > >> +     case PIN_CONFIG_POWER_SOURCE:
>> > >> +             if (pcs->psmask == PCS_OFF_DISABLED
>> > >> +                     || pcs->psshift == PCS_OFF_DISABLED)
>> > >> +                     return -ENOTSUPP;
>> > >> +             data &= pcs->psmask;
>> > >> +             data = data >> pcs->psshift;
>> > >> +             *config = data;
>> > >> +             return 0;
>> > >> +             break;
>> > >
>> > > Hmm, only slightly related to this patch, mostly a generic
>> > > question to others: Do others have any mux registers with
>> > > status bits for things like PIN_CONFIG_POWER_SOURCE?
>> > >
>> > > I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
>> > > for omap MMC. But there's also a status bit that needs to be
>> > > checked for that. I think there was some other similar mux
>> > > register for USB PHY that has a status register.
>> > >
>> > > So I'm wondering should the checking for status bit be handled
>> > > in the pinctrl consume driver? Or should we have some bindings
>> > > for that?
>> > >
>> >
>> > Do you mean that the status register only exists in USB PHY controller or
>> > MMC controller?
>>
>> The status register is in the MMC PBIAS register that is mux
>> related otherwise. From OMAP4470_ES1.0_PUBLIC_TRM_vE.pdf,
>> Table 19-599. CONTROL_PBIASLITE:
>>
>> Bits
>> 26    MMC1_PWDNZ
>> 25    MMC1_PBIASLITE_HIZ_MODE
>> 24    MMC1_PBIASLITE_SUPPLY_HI_OUT
>> 23    MMC1_PBIASLITE_VMODE_ERROR      then this bit needs to clear..
>> 22    MMC1_PBIASLITE_PWRDNZ
>> 21    MMC1_PBIASLITE_VMODE            ..after VMODE bit is set to 3V
>>
>> > If so, could we use regulator framework in USB PHY or MMC driver?
>>
>> Yes we could use regulator framework for that that. Or just read the
>> status in the MMC driver for that bit if nobody else has mixed
>> mux-regulator needs like this.
>>
>> The sequence is MMC specific, so from that point of view it would
>> make sense to have the logic in the MMC driver.
>
> Well it turns out the VMODE_ERROR bit is not just for VMODE, it's a
> comparator that can also triggers for the other invalid states for
> CONTROL_PBIASLITE pinconf register. So hiding VMODE_ERROR into a
> regulator would be wrong. For now, VMODE best handled using
> PIN_CONFIG_POWER_SOURCE and let the MMC driver do the checking
> using the pinconf API.
>
> Regards,
>
> Tony

Could you share the link of downloading the spec?

Regards
Haojian

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121025 18:49]:
> On Fri, Oct 26, 2012 at 7:43 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [121022 10:11]:
> >> * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121022 03:11]:
> >> > On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> >> > > * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 02:08]:
> >> > >> Add pinconf generic support with POWER SOURCE, BIAS PULL.
> >> > > ...
> >> > >
> >> > >> +     case PIN_CONFIG_POWER_SOURCE:
> >> > >> +             if (pcs->psmask == PCS_OFF_DISABLED
> >> > >> +                     || pcs->psshift == PCS_OFF_DISABLED)
> >> > >> +                     return -ENOTSUPP;
> >> > >> +             data &= pcs->psmask;
> >> > >> +             data = data >> pcs->psshift;
> >> > >> +             *config = data;
> >> > >> +             return 0;
> >> > >> +             break;
> >> > >
> >> > > Hmm, only slightly related to this patch, mostly a generic
> >> > > question to others: Do others have any mux registers with
> >> > > status bits for things like PIN_CONFIG_POWER_SOURCE?
> >> > >
> >> > > I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
> >> > > for omap MMC. But there's also a status bit that needs to be
> >> > > checked for that. I think there was some other similar mux
> >> > > register for USB PHY that has a status register.
> >> > >
> >> > > So I'm wondering should the checking for status bit be handled
> >> > > in the pinctrl consume driver? Or should we have some bindings
> >> > > for that?
> >> > >
> >> >
> >> > Do you mean that the status register only exists in USB PHY controller or
> >> > MMC controller?
> >>
> >> The status register is in the MMC PBIAS register that is mux
> >> related otherwise. From OMAP4470_ES1.0_PUBLIC_TRM_vE.pdf,
> >> Table 19-599. CONTROL_PBIASLITE:
> >>
> >> Bits
> >> 26    MMC1_PWDNZ
> >> 25    MMC1_PBIASLITE_HIZ_MODE
> >> 24    MMC1_PBIASLITE_SUPPLY_HI_OUT
> >> 23    MMC1_PBIASLITE_VMODE_ERROR      then this bit needs to clear..
> >> 22    MMC1_PBIASLITE_PWRDNZ
> >> 21    MMC1_PBIASLITE_VMODE            ..after VMODE bit is set to 3V
> >>
> >> > If so, could we use regulator framework in USB PHY or MMC driver?
> >>
> >> Yes we could use regulator framework for that that. Or just read the
> >> status in the MMC driver for that bit if nobody else has mixed
> >> mux-regulator needs like this.
> >>
> >> The sequence is MMC specific, so from that point of view it would
> >> make sense to have the logic in the MMC driver.
> >
> > Well it turns out the VMODE_ERROR bit is not just for VMODE, it's a
> > comparator that can also triggers for the other invalid states for
> > CONTROL_PBIASLITE pinconf register. So hiding VMODE_ERROR into a
> > regulator would be wrong. For now, VMODE best handled using
> > PIN_CONFIG_POWER_SOURCE and let the MMC driver do the checking
> > using the pinconf API.
> >
> > Regards,
> >
> > Tony
> 
> Could you share the link of downloading the spec?

Yes here's the omap4470 public TRM:

http://www.ti.com/litv/pdf/swpu270n

See CONTROL_PBIASLITE, and also "19.4.9.3 PBIAS Error Generation"
table on page 3520 for the combinations when the comparator can
generate errors.

Regards,

Tony

[PATCH 08/10] pinctrl: single: support pinconf generic

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121025 18:49]:
> On Fri, Oct 26, 2012 at 7:43 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Tony Lindgren <tony@atomide.com> [121022 10:11]:
> >> * Haojian Zhuang <haojian.zhuang@gmail.com> [121022 03:11]:
> >> > On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony@atomide.com> wrote:
> >> > > * Haojian Zhuang <haojian.zhuang@gmail.com> [121018 02:08]:
> >> > >> Add pinconf generic support with POWER SOURCE, BIAS PULL.
> >> > > ...
> >> > >
> >> > >> +     case PIN_CONFIG_POWER_SOURCE:
> >> > >> +             if (pcs->psmask == PCS_OFF_DISABLED
> >> > >> +                     || pcs->psshift == PCS_OFF_DISABLED)
> >> > >> +                     return -ENOTSUPP;
> >> > >> +             data &= pcs->psmask;
> >> > >> +             data = data >> pcs->psshift;
> >> > >> +             *config = data;
> >> > >> +             return 0;
> >> > >> +             break;
> >> > >
> >> > > Hmm, only slightly related to this patch, mostly a generic
> >> > > question to others: Do others have any mux registers with
> >> > > status bits for things like PIN_CONFIG_POWER_SOURCE?
> >> > >
> >> > > I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
> >> > > for omap MMC. But there's also a status bit that needs to be
> >> > > checked for that. I think there was some other similar mux
> >> > > register for USB PHY that has a status register.
> >> > >
> >> > > So I'm wondering should the checking for status bit be handled
> >> > > in the pinctrl consume driver? Or should we have some bindings
> >> > > for that?
> >> > >
> >> >
> >> > Do you mean that the status register only exists in USB PHY controller or
> >> > MMC controller?
> >>
> >> The status register is in the MMC PBIAS register that is mux
> >> related otherwise. From OMAP4470_ES1.0_PUBLIC_TRM_vE.pdf,
> >> Table 19-599. CONTROL_PBIASLITE:
> >>
> >> Bits
> >> 26    MMC1_PWDNZ
> >> 25    MMC1_PBIASLITE_HIZ_MODE
> >> 24    MMC1_PBIASLITE_SUPPLY_HI_OUT
> >> 23    MMC1_PBIASLITE_VMODE_ERROR      then this bit needs to clear..
> >> 22    MMC1_PBIASLITE_PWRDNZ
> >> 21    MMC1_PBIASLITE_VMODE            ..after VMODE bit is set to 3V
> >>
> >> > If so, could we use regulator framework in USB PHY or MMC driver?
> >>
> >> Yes we could use regulator framework for that that. Or just read the
> >> status in the MMC driver for that bit if nobody else has mixed
> >> mux-regulator needs like this.
> >>
> >> The sequence is MMC specific, so from that point of view it would
> >> make sense to have the logic in the MMC driver.
> >
> > Well it turns out the VMODE_ERROR bit is not just for VMODE, it's a
> > comparator that can also triggers for the other invalid states for
> > CONTROL_PBIASLITE pinconf register. So hiding VMODE_ERROR into a
> > regulator would be wrong. For now, VMODE best handled using
> > PIN_CONFIG_POWER_SOURCE and let the MMC driver do the checking
> > using the pinconf API.
> >
> > Regards,
> >
> > Tony
> 
> Could you share the link of downloading the spec?

Yes here's the omap4470 public TRM:

http://www.ti.com/litv/pdf/swpu270n

See CONTROL_PBIASLITE, and also "19.4.9.3 PBIAS Error Generation"
table on page 3520 for the combinations when the comparator can
generate errors.

Regards,

Tony

Re: [PATCH 08/10] pinctrl: single: support pinconf generic

[Haojian Zhuang]

On Fri, Oct 26, 2012 at 1:43 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [121022 10:11]:
>> * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121022 03:11]:
>> > On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>> > > * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 02:08]:
>> > >> Add pinconf generic support with POWER SOURCE, BIAS PULL.
>> > > ...
>> > >
>> > >> +     case PIN_CONFIG_POWER_SOURCE:
>> > >> +             if (pcs->psmask == PCS_OFF_DISABLED
>> > >> +                     || pcs->psshift == PCS_OFF_DISABLED)
>> > >> +                     return -ENOTSUPP;
>> > >> +             data &= pcs->psmask;
>> > >> +             data = data >> pcs->psshift;
>> > >> +             *config = data;
>> > >> +             return 0;
>> > >> +             break;
>> > >
>> > > Hmm, only slightly related to this patch, mostly a generic
>> > > question to others: Do others have any mux registers with
>> > > status bits for things like PIN_CONFIG_POWER_SOURCE?
>> > >
>> > > I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
>> > > for omap MMC. But there's also a status bit that needs to be
>> > > checked for that. I think there was some other similar mux
>> > > register for USB PHY that has a status register.
>> > >
>> > > So I'm wondering should the checking for status bit be handled
>> > > in the pinctrl consume driver? Or should we have some bindings
>> > > for that?
>> > >
>> >
>> > Do you mean that the status register only exists in USB PHY controller or
>> > MMC controller?
>>
>> The status register is in the MMC PBIAS register that is mux
>> related otherwise. From OMAP4470_ES1.0_PUBLIC_TRM_vE.pdf,
>> Table 19-599. CONTROL_PBIASLITE:
>>
>> Bits
>> 26    MMC1_PWDNZ
>> 25    MMC1_PBIASLITE_HIZ_MODE
>> 24    MMC1_PBIASLITE_SUPPLY_HI_OUT
>> 23    MMC1_PBIASLITE_VMODE_ERROR      then this bit needs to clear..
>> 22    MMC1_PBIASLITE_PWRDNZ
>> 21    MMC1_PBIASLITE_VMODE            ..after VMODE bit is set to 3V
>>
>> > If so, could we use regulator framework in USB PHY or MMC driver?
>>
>> Yes we could use regulator framework for that that. Or just read the
>> status in the MMC driver for that bit if nobody else has mixed
>> mux-regulator needs like this.
>>
>> The sequence is MMC specific, so from that point of view it would
>> make sense to have the logic in the MMC driver.
>
> Well it turns out the VMODE_ERROR bit is not just for VMODE, it's a
> comparator that can also triggers for the other invalid states for
> CONTROL_PBIASLITE pinconf register. So hiding VMODE_ERROR into a
> regulator would be wrong. For now, VMODE best handled using
> PIN_CONFIG_POWER_SOURCE and let the MMC driver do the checking
> using the pinconf API.
>
I'm seeking whether there's more flexible way to support your case.
The fix won't
be contained in v3 round.

[PATCH 08/10] pinctrl: single: support pinconf generic

[Haojian Zhuang]

On Fri, Oct 26, 2012 at 1:43 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Tony Lindgren <tony@atomide.com> [121022 10:11]:
>> * Haojian Zhuang <haojian.zhuang@gmail.com> [121022 03:11]:
>> > On Sat, Oct 20, 2012 at 3:13 AM, Tony Lindgren <tony@atomide.com> wrote:
>> > > * Haojian Zhuang <haojian.zhuang@gmail.com> [121018 02:08]:
>> > >> Add pinconf generic support with POWER SOURCE, BIAS PULL.
>> > > ...
>> > >
>> > >> +     case PIN_CONFIG_POWER_SOURCE:
>> > >> +             if (pcs->psmask == PCS_OFF_DISABLED
>> > >> +                     || pcs->psshift == PCS_OFF_DISABLED)
>> > >> +                     return -ENOTSUPP;
>> > >> +             data &= pcs->psmask;
>> > >> +             data = data >> pcs->psshift;
>> > >> +             *config = data;
>> > >> +             return 0;
>> > >> +             break;
>> > >
>> > > Hmm, only slightly related to this patch, mostly a generic
>> > > question to others: Do others have any mux registers with
>> > > status bits for things like PIN_CONFIG_POWER_SOURCE?
>> > >
>> > > I could use PIN_CONFIG_POWER_SOURCE for controlling the PBIAS
>> > > for omap MMC. But there's also a status bit that needs to be
>> > > checked for that. I think there was some other similar mux
>> > > register for USB PHY that has a status register.
>> > >
>> > > So I'm wondering should the checking for status bit be handled
>> > > in the pinctrl consume driver? Or should we have some bindings
>> > > for that?
>> > >
>> >
>> > Do you mean that the status register only exists in USB PHY controller or
>> > MMC controller?
>>
>> The status register is in the MMC PBIAS register that is mux
>> related otherwise. From OMAP4470_ES1.0_PUBLIC_TRM_vE.pdf,
>> Table 19-599. CONTROL_PBIASLITE:
>>
>> Bits
>> 26    MMC1_PWDNZ
>> 25    MMC1_PBIASLITE_HIZ_MODE
>> 24    MMC1_PBIASLITE_SUPPLY_HI_OUT
>> 23    MMC1_PBIASLITE_VMODE_ERROR      then this bit needs to clear..
>> 22    MMC1_PBIASLITE_PWRDNZ
>> 21    MMC1_PBIASLITE_VMODE            ..after VMODE bit is set to 3V
>>
>> > If so, could we use regulator framework in USB PHY or MMC driver?
>>
>> Yes we could use regulator framework for that that. Or just read the
>> status in the MMC driver for that bit if nobody else has mixed
>> mux-regulator needs like this.
>>
>> The sequence is MMC specific, so from that point of view it would
>> make sense to have the logic in the MMC driver.
>
> Well it turns out the VMODE_ERROR bit is not just for VMODE, it's a
> comparator that can also triggers for the other invalid states for
> CONTROL_PBIASLITE pinconf register. So hiding VMODE_ERROR into a
> regulator would be wrong. For now, VMODE best handled using
> PIN_CONFIG_POWER_SOURCE and let the MMC driver do the checking
> using the pinconf API.
>
I'm seeking whether there's more flexible way to support your case.
The fix won't
be contained in v3 round.

[PATCH 09/10] ARM: dts: support pinctrl single in pxa910

[Haojian Zhuang]

Add pinctrl-single support with device tree in pxa910 dkb platform.

Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/pxa910-dkb.dts |  187 +++++++++++++++++++++++++++++++++++++-
 arch/arm/boot/dts/pxa910.dtsi    |   81 ++++++++++++++++
 2 files changed, 267 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/pxa910-dkb.dts b/arch/arm/boot/dts/pxa910-dkb.dts
index e92be5a..2ad0a98 100644
--- a/arch/arm/boot/dts/pxa910-dkb.dts
+++ b/arch/arm/boot/dts/pxa910-dkb.dts
@@ -24,10 +24,195 @@
 
 	soc {
 		apb@d4000000 {
-			uart1: uart@d4017000 {
+			pmx: pinmux@d401e000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&board_pins>;
+
+				board_pins: pinmux_board_pins {
+					/* pins not owned by device driver */
+				};
+				uart1_pins: pinmux_uart1_pins {
+					pinctrl-single,pins = <
+						0x198 0x6	/* GPIO47_UART1_RXD */
+						0x19c 0x6	/* GPIO48_UART1_TXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0x6>;
+				};
+				uart2_pins: pinmux_uart2_pins {
+					pinctrl-single,pins = <
+						0x150 0x4	/* GPIO29_UART2_CTS */
+						0x154 0x4	/* GPIO30_UART2_RTS */
+						0x158 0x4	/* GPIO31_UART2_TXD */
+						0x15c 0x4	/* GPIO32_UART2_RXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				uart3_pins: pinmux_uart3_pins {
+					pinctrl-single,pins = <
+						0x188 0x7	/* GPIO43_UART3_RXD */
+						0x18c 0x7	/* GPIO44_UART3_TXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				twsi1_pins: pinmux_twsi1_pins {
+					pinctrl-single,pins = <
+						0x1b0 0x2	/* GPIO53_TWSI_SCL */
+						0x1b4 0x2	/* GPIO54_TWSI_SDA */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				nand_pins: pinmux_nand_pins {
+					pinctrl-single,pins = <
+						0x040 0x0	/* ND_IO0 */
+						0x03c 0x0	/* ND_IO1 */
+						0x038 0x0	/* ND_IO2 */
+						0x034 0x0	/* ND_IO3 */
+						0x030 0x0	/* ND_IO4 */
+						0x02c 0x0	/* ND_IO5 */
+						0x028 0x0	/* ND_IO6 */
+						0x024 0x0	/* ND_IO7 */
+						0x020 0x0	/* ND_IO8 */
+						0x01c 0x0	/* ND_IO9 */
+						0x018 0x0	/* ND_IO10 */
+						0x014 0x0	/* ND_IO11 */
+						0x010 0x0	/* ND_IO12 */
+						0x00c 0x0	/* ND_IO13 */
+						0x008 0x0	/* ND_IO14 */
+						0x004 0x0	/* ND_IO15 */
+						0x044 0x0	/* ND_nCS0 */
+						0x060 0x1	/* ND_ALE */
+						0x05c 0x0	/* ND_CLE */
+						0x054 0x1	/* ND_nWE */
+						0x058 0x1	/* ND_nRE */
+						0x068 0x0	/* ND_RDY0 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_ldata_pins: pinmux_mmc1_ldata_pins {
+					pinctrl-single,pins = <
+						0x0a0 0x0	/* MMC1_DATA0 */
+						0x09c 0x0	/* MMC1_DATA1 */
+						0x098 0x0	/* MMC1_DATA2 */
+						0x094 0x0	/* MMC1_DATA3 */
+					>;
+					pinctrl-single,power-source = <0x3>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_hdata_pins: pinmux_mmc1_hdata_pins {
+					pinctrl-single,pins = <
+						0x090 0x0	/* MMC1_DATA4 */
+						0x08c 0x0	/* MMC1_DATA5 */
+						0x088 0x0	/* MMC1_DATA6 */
+						0x084 0x0	/* MMC1_DATA7 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_clk_pins: pinmux_mmc1_clk_pins {
+					pinctrl-single,pins = <
+						0x0a4 0x0	/* MMC1_CMD */
+						0x0a8 0x0	/* MMC1_CLK */
+					>;
+					pinctrl-single,power-source = <0x3>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_cd_pins: pinmux_mmc1_cd_pins {
+					pinctrl-single,pins = <
+						0x0ac 0x0	/* MMC1_CD */
+						0x0b0 0x0	/* MMC1_WP */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc2_pins: pinmux_mmc2_pins {
+					pinctrl-single,pins = <
+						0x180 0x1	/* MMC2_CMD */
+						0x184 0x1	/* MMC2_CLK */
+						0x17c 0x1	/* MMC2_DATA0 */
+						0x178 0x1	/* MMC2_DATA1 */
+						0x174 0x1	/* MMC2_DATA2 */
+						0x170 0x1	/* MMC2_DATA3 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				w1_pins: pinmux_w1_pins {
+					pinctrl-single,pins = <
+						0x0cc 0x2	/* CLK_REQ_W1 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				ssp1_pins: pinmux_ssp1_pins {
+					pinctrl-single,pins = <
+						0x130 0x1	/* GPIO21_SSP1_SCLK */
+						0x134 0x1	/* GPIO22_SSP1_FRM */
+						0x138 0x1	/* GPIO23_SSP1_TXD */
+						0x13c 0x1	/* GPIO24_SSP1_RXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				keypad_pins: pinmux_keypad_pins {
+					pinctrl-single,pins = <
+						0x0dc 0x1	/* GPIO0_MKIN0 */
+						0x0e0 0x1	/* GPIO1_MKOUT0 */
+						0x0e4 0x1	/* GPIO2_MKIN1 */
+						0x0e8 0x1	/* GPIO3_MKOUT1 */
+						0x0ec 0x1	/* GPIO4_MKIN2 */
+						0x0f0 0x1	/* GPIO5_MKOUT2 */
+						0x0f4 0x1	/* GPIO6_MKIN3 */
+						0x0f8 0x1	/* GPIO7_MKOUT3 */
+						0x0fc 0x1	/* GPIO8_MKIN4 */
+						0x100 0x1	/* GPIO9_MKOUT4 */
+						0x10c 0x1	/* GPIO12_MKIN6 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				nfc_pins: pinmux_nfc_pins {
+					pinctrl-single,pins = <
+						0x120 0x0	/* GPIO17 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				wlan_pins: pinmux_wlan_pins {
+					pinctrl-single,pins = <
+						0x114 0x0	/* GPIO14 */
+						0x12c 0x0	/* GPIO20 */
+						0x160 0x0	/* GPIO33 */
+						0x164 0x0	/* GPIO34 */
+						0x168 0x0	/* GPIO35 */
+						0x16c 0x0	/* GPIO36 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+			};
+			uart1: uart@d4017000 {	/* FFUART */
+				pinctrl-names = "default";
+				pinctrl-0 = <&uart1_pins>;
+				status = "okay";
+			};
+			uart2: uart@d4018000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&uart2_pins>;
+				status = "okay";
+			};
+			uart3: uart@d4036000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&uart3_pins>;
 				status = "okay";
 			};
 			twsi1: i2c@d4011000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&twsi1_pins>;
 				status = "okay";
 			};
 			rtc: rtc@d4010000 {
diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi
index aebf32d..d3f5205 100644
--- a/arch/arm/boot/dts/pxa910.dtsi
+++ b/arch/arm/boot/dts/pxa910.dtsi
@@ -49,6 +49,87 @@
 			reg = <0xd4000000 0x00200000>;
 			ranges;
 
+			pmx: pinmux@d401e000 {
+				compatible = "pinctrl-single";
+				reg = <0xd401e000 0x0330>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				pinctrl-single,register-width = <32>;
+				pinctrl-single,function-mask = <7>;
+				pinctrl-single,gpio-mask = <7>;
+				pinctrl-single,gpio-ranges = <&gpiorange0 &gpiorange1
+								&gpiorange2 &gpiorange3
+								&gpiorange4 &gpiorange5
+								&gpiorange6 &gpiorange7
+								&gpiorange8 &gpiorange9
+								&gpiorange10>;
+				pinctrl-single,power-source-mask = <0x1800>;
+				pinctrl-single,power-source-shift = <11>;
+				pinctrl-single,bias-mask = <0xe000>;
+				pinctrl-single,bias-shift = <13>;
+				pinctrl-single,bias-disable = <0>;
+				pinctrl-single,bias-pull-down = <0xa000>;
+				pinctrl-single,bias-pull-up = <0xc000>;
+				pinctrl-single,input-schmitt-mask = <0x70>;
+				pinctrl-single,input-schmitt-shift = <4>;
+
+				gpiorange0: gpiorange@d401e0dc {
+					/* GPIO0 ~ GPIO54 */
+					pinctrl-single,gpio = <0 55 0x0dc>;
+					pinctrl-single,gpio-enable = <0x0dc 0>;
+				};
+				gpiorange1: gpiorange@d401e2f0 {
+					/* GPIO55 ~ GPIO59 */
+					pinctrl-single,gpio = <55 5 0x2f0>;
+					pinctrl-single,gpio-enable = <0x2f0 1>;
+				};
+				gpiorange2: gpiorange@d401e304 {
+					/* GPIO60 ~ GPIO66 */
+					pinctrl-single,gpio = <60 7 0x304>;
+					pinctrl-single,gpio-enable = <0x304 0>;
+				};
+				gpiorange3: gpiorange@d401e1b8 {
+					/* GPIO67 ~ GPIO109 */
+					pinctrl-single,gpio = <67 43 0x1b8>;
+					pinctrl-single,gpio-enable = <0x1b8 0>;
+				};
+				gpiorange4: gpiorange@d401e298 {
+					/* GPIO110 ~ GPIO116 */
+					pinctrl-single,gpio = <110 7 0x298>;
+					pinctrl-single,gpio-enable = <0x298 0>;
+				};
+				gpiorange5: gpiorange@d401e0b4 {
+					/* GPIO117 ~ GPIO120 */
+					pinctrl-single,gpio = <117 4 0x0b4>;
+					pinctrl-single,gpio-enable = <0x0b4 1>;
+				};
+				gpiorange6: gpiorange@d401e32c {
+					/* GPIO121 */
+					pinctrl-single,gpio = <121 1 0x32c>;
+					pinctrl-single,gpio-enable = <0x32c 0>;
+				};
+				gpiorange7: gpiorange@d401e0c8 {
+					/* GPIO122 ~ GPIO123 */
+					pinctrl-single,gpio = <122 2 0x0c8>;
+					pinctrl-single,gpio-enable = <0x0c8 1>;
+				};
+				gpiorange8: gpiorange@d401e0d0 {
+					/* GPIO124 */
+					pinctrl-single,gpio = <124 1 0x0d0>;
+					pinctrl-single,gpio-enable = <0x0d0 0>;
+				};
+				gpiorange9: gpiorange@d401e0d4 {
+					/* GPIO125 */
+					pinctrl-single,gpio = <125 1 0x0d4>;
+					pinctrl-single,gpio-enable = <0x0d4 1>;
+				};
+				gpiorange10: gpiorange@d401e06c {
+					/* GPIO126 ~ GPIO127 */
+					pinctrl-single,gpio = <126 2 0x06c>;
+					pinctrl-single,gpio-enable = <0x06c 0>;
+				};
+			};
+
 			timer0: timer@d4014000 {
 				compatible = "mrvl,mmp-timer";
 				reg = <0xd4014000 0x100>;
-- 
1.7.0.4

[PATCH 09/10] ARM: dts: support pinctrl single in pxa910

[Haojian Zhuang]

Add pinctrl-single support with device tree in pxa910 dkb platform.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 arch/arm/boot/dts/pxa910-dkb.dts |  187 +++++++++++++++++++++++++++++++++++++-
 arch/arm/boot/dts/pxa910.dtsi    |   81 ++++++++++++++++
 2 files changed, 267 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/pxa910-dkb.dts b/arch/arm/boot/dts/pxa910-dkb.dts
index e92be5a..2ad0a98 100644
--- a/arch/arm/boot/dts/pxa910-dkb.dts
+++ b/arch/arm/boot/dts/pxa910-dkb.dts
@@ -24,10 +24,195 @@
 
 	soc {
 		apb at d4000000 {
-			uart1: uart at d4017000 {
+			pmx: pinmux at d401e000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&board_pins>;
+
+				board_pins: pinmux_board_pins {
+					/* pins not owned by device driver */
+				};
+				uart1_pins: pinmux_uart1_pins {
+					pinctrl-single,pins = <
+						0x198 0x6	/* GPIO47_UART1_RXD */
+						0x19c 0x6	/* GPIO48_UART1_TXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0x6>;
+				};
+				uart2_pins: pinmux_uart2_pins {
+					pinctrl-single,pins = <
+						0x150 0x4	/* GPIO29_UART2_CTS */
+						0x154 0x4	/* GPIO30_UART2_RTS */
+						0x158 0x4	/* GPIO31_UART2_TXD */
+						0x15c 0x4	/* GPIO32_UART2_RXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				uart3_pins: pinmux_uart3_pins {
+					pinctrl-single,pins = <
+						0x188 0x7	/* GPIO43_UART3_RXD */
+						0x18c 0x7	/* GPIO44_UART3_TXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				twsi1_pins: pinmux_twsi1_pins {
+					pinctrl-single,pins = <
+						0x1b0 0x2	/* GPIO53_TWSI_SCL */
+						0x1b4 0x2	/* GPIO54_TWSI_SDA */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				nand_pins: pinmux_nand_pins {
+					pinctrl-single,pins = <
+						0x040 0x0	/* ND_IO0 */
+						0x03c 0x0	/* ND_IO1 */
+						0x038 0x0	/* ND_IO2 */
+						0x034 0x0	/* ND_IO3 */
+						0x030 0x0	/* ND_IO4 */
+						0x02c 0x0	/* ND_IO5 */
+						0x028 0x0	/* ND_IO6 */
+						0x024 0x0	/* ND_IO7 */
+						0x020 0x0	/* ND_IO8 */
+						0x01c 0x0	/* ND_IO9 */
+						0x018 0x0	/* ND_IO10 */
+						0x014 0x0	/* ND_IO11 */
+						0x010 0x0	/* ND_IO12 */
+						0x00c 0x0	/* ND_IO13 */
+						0x008 0x0	/* ND_IO14 */
+						0x004 0x0	/* ND_IO15 */
+						0x044 0x0	/* ND_nCS0 */
+						0x060 0x1	/* ND_ALE */
+						0x05c 0x0	/* ND_CLE */
+						0x054 0x1	/* ND_nWE */
+						0x058 0x1	/* ND_nRE */
+						0x068 0x0	/* ND_RDY0 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_ldata_pins: pinmux_mmc1_ldata_pins {
+					pinctrl-single,pins = <
+						0x0a0 0x0	/* MMC1_DATA0 */
+						0x09c 0x0	/* MMC1_DATA1 */
+						0x098 0x0	/* MMC1_DATA2 */
+						0x094 0x0	/* MMC1_DATA3 */
+					>;
+					pinctrl-single,power-source = <0x3>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_hdata_pins: pinmux_mmc1_hdata_pins {
+					pinctrl-single,pins = <
+						0x090 0x0	/* MMC1_DATA4 */
+						0x08c 0x0	/* MMC1_DATA5 */
+						0x088 0x0	/* MMC1_DATA6 */
+						0x084 0x0	/* MMC1_DATA7 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_clk_pins: pinmux_mmc1_clk_pins {
+					pinctrl-single,pins = <
+						0x0a4 0x0	/* MMC1_CMD */
+						0x0a8 0x0	/* MMC1_CLK */
+					>;
+					pinctrl-single,power-source = <0x3>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc1_cd_pins: pinmux_mmc1_cd_pins {
+					pinctrl-single,pins = <
+						0x0ac 0x0	/* MMC1_CD */
+						0x0b0 0x0	/* MMC1_WP */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				mmc2_pins: pinmux_mmc2_pins {
+					pinctrl-single,pins = <
+						0x180 0x1	/* MMC2_CMD */
+						0x184 0x1	/* MMC2_CLK */
+						0x17c 0x1	/* MMC2_DATA0 */
+						0x178 0x1	/* MMC2_DATA1 */
+						0x174 0x1	/* MMC2_DATA2 */
+						0x170 0x1	/* MMC2_DATA3 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				w1_pins: pinmux_w1_pins {
+					pinctrl-single,pins = <
+						0x0cc 0x2	/* CLK_REQ_W1 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				ssp1_pins: pinmux_ssp1_pins {
+					pinctrl-single,pins = <
+						0x130 0x1	/* GPIO21_SSP1_SCLK */
+						0x134 0x1	/* GPIO22_SSP1_FRM */
+						0x138 0x1	/* GPIO23_SSP1_TXD */
+						0x13c 0x1	/* GPIO24_SSP1_RXD */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				keypad_pins: pinmux_keypad_pins {
+					pinctrl-single,pins = <
+						0x0dc 0x1	/* GPIO0_MKIN0 */
+						0x0e0 0x1	/* GPIO1_MKOUT0 */
+						0x0e4 0x1	/* GPIO2_MKIN1 */
+						0x0e8 0x1	/* GPIO3_MKOUT1 */
+						0x0ec 0x1	/* GPIO4_MKIN2 */
+						0x0f0 0x1	/* GPIO5_MKOUT2 */
+						0x0f4 0x1	/* GPIO6_MKIN3 */
+						0x0f8 0x1	/* GPIO7_MKOUT3 */
+						0x0fc 0x1	/* GPIO8_MKIN4 */
+						0x100 0x1	/* GPIO9_MKOUT4 */
+						0x10c 0x1	/* GPIO12_MKIN6 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				nfc_pins: pinmux_nfc_pins {
+					pinctrl-single,pins = <
+						0x120 0x0	/* GPIO17 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+				wlan_pins: pinmux_wlan_pins {
+					pinctrl-single,pins = <
+						0x114 0x0	/* GPIO14 */
+						0x12c 0x0	/* GPIO20 */
+						0x160 0x0	/* GPIO33 */
+						0x164 0x0	/* GPIO34 */
+						0x168 0x0	/* GPIO35 */
+						0x16c 0x0	/* GPIO36 */
+					>;
+					pinctrl-single,power-source = <0x2>;
+					pinctrl-single,bias = <0>;
+				};
+			};
+			uart1: uart at d4017000 {	/* FFUART */
+				pinctrl-names = "default";
+				pinctrl-0 = <&uart1_pins>;
+				status = "okay";
+			};
+			uart2: uart at d4018000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&uart2_pins>;
+				status = "okay";
+			};
+			uart3: uart at d4036000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&uart3_pins>;
 				status = "okay";
 			};
 			twsi1: i2c at d4011000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&twsi1_pins>;
 				status = "okay";
 			};
 			rtc: rtc at d4010000 {
diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi
index aebf32d..d3f5205 100644
--- a/arch/arm/boot/dts/pxa910.dtsi
+++ b/arch/arm/boot/dts/pxa910.dtsi
@@ -49,6 +49,87 @@
 			reg = <0xd4000000 0x00200000>;
 			ranges;
 
+			pmx: pinmux at d401e000 {
+				compatible = "pinctrl-single";
+				reg = <0xd401e000 0x0330>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				pinctrl-single,register-width = <32>;
+				pinctrl-single,function-mask = <7>;
+				pinctrl-single,gpio-mask = <7>;
+				pinctrl-single,gpio-ranges = <&gpiorange0 &gpiorange1
+								&gpiorange2 &gpiorange3
+								&gpiorange4 &gpiorange5
+								&gpiorange6 &gpiorange7
+								&gpiorange8 &gpiorange9
+								&gpiorange10>;
+				pinctrl-single,power-source-mask = <0x1800>;
+				pinctrl-single,power-source-shift = <11>;
+				pinctrl-single,bias-mask = <0xe000>;
+				pinctrl-single,bias-shift = <13>;
+				pinctrl-single,bias-disable = <0>;
+				pinctrl-single,bias-pull-down = <0xa000>;
+				pinctrl-single,bias-pull-up = <0xc000>;
+				pinctrl-single,input-schmitt-mask = <0x70>;
+				pinctrl-single,input-schmitt-shift = <4>;
+
+				gpiorange0: gpiorange at d401e0dc {
+					/* GPIO0 ~ GPIO54 */
+					pinctrl-single,gpio = <0 55 0x0dc>;
+					pinctrl-single,gpio-enable = <0x0dc 0>;
+				};
+				gpiorange1: gpiorange at d401e2f0 {
+					/* GPIO55 ~ GPIO59 */
+					pinctrl-single,gpio = <55 5 0x2f0>;
+					pinctrl-single,gpio-enable = <0x2f0 1>;
+				};
+				gpiorange2: gpiorange at d401e304 {
+					/* GPIO60 ~ GPIO66 */
+					pinctrl-single,gpio = <60 7 0x304>;
+					pinctrl-single,gpio-enable = <0x304 0>;
+				};
+				gpiorange3: gpiorange at d401e1b8 {
+					/* GPIO67 ~ GPIO109 */
+					pinctrl-single,gpio = <67 43 0x1b8>;
+					pinctrl-single,gpio-enable = <0x1b8 0>;
+				};
+				gpiorange4: gpiorange at d401e298 {
+					/* GPIO110 ~ GPIO116 */
+					pinctrl-single,gpio = <110 7 0x298>;
+					pinctrl-single,gpio-enable = <0x298 0>;
+				};
+				gpiorange5: gpiorange at d401e0b4 {
+					/* GPIO117 ~ GPIO120 */
+					pinctrl-single,gpio = <117 4 0x0b4>;
+					pinctrl-single,gpio-enable = <0x0b4 1>;
+				};
+				gpiorange6: gpiorange at d401e32c {
+					/* GPIO121 */
+					pinctrl-single,gpio = <121 1 0x32c>;
+					pinctrl-single,gpio-enable = <0x32c 0>;
+				};
+				gpiorange7: gpiorange at d401e0c8 {
+					/* GPIO122 ~ GPIO123 */
+					pinctrl-single,gpio = <122 2 0x0c8>;
+					pinctrl-single,gpio-enable = <0x0c8 1>;
+				};
+				gpiorange8: gpiorange at d401e0d0 {
+					/* GPIO124 */
+					pinctrl-single,gpio = <124 1 0x0d0>;
+					pinctrl-single,gpio-enable = <0x0d0 0>;
+				};
+				gpiorange9: gpiorange at d401e0d4 {
+					/* GPIO125 */
+					pinctrl-single,gpio = <125 1 0x0d4>;
+					pinctrl-single,gpio-enable = <0x0d4 1>;
+				};
+				gpiorange10: gpiorange at d401e06c {
+					/* GPIO126 ~ GPIO127 */
+					pinctrl-single,gpio = <126 2 0x06c>;
+					pinctrl-single,gpio-enable = <0x06c 0>;
+				};
+			};
+
 			timer0: timer at d4014000 {
 				compatible = "mrvl,mmp-timer";
 				reg = <0xd4014000 0x100>;
-- 
1.7.0.4

[PATCH 10/10] document: devicetree: bind pinconf in pinctrl single

[Haojian Zhuang]

Add comments with pinconf & gpio range in the document of
pinctrl-single.

Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/pinctrl/pinctrl-single.txt |   43 ++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
index 5187f0d..b0e5059 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
@@ -15,6 +15,49 @@ Optional properties:
   available and same for all registers; if not specified, disabling of
   pin functions is ignored
 
+- pinctrl-single,gpio-mask : mask of enabling gpio function register
+
+- pinctrl-single,gpio-ranges : gpio range list
+
+- pinctrl-single,gpio : array with gpio range start, size & register
+  offset
+
+- pinctrl-single,gpio-enable : value of enabling gpio function
+
+- pinctrl-single,gpio-disable : value of disabling gpio function
+
+- pinctrl-single,power-source-mask : mask of setting power source in
+  the pinmux register
+
+- pinctrl-single,power-source-shift : shift of power source field in
+  the pinmux register
+
+- pinctrl-single,power-source : value of setting power source field
+  in the pinmux register
+
+- pinctrl-single,bias-mask : mask of setting bias value in the pinmux
+  register
+
+- pinctrl-single,bias-shift : shift of setting bias value in the
+  pinmux register
+
+- pinctrl-single,bias-disable : value of disabling bias in the pinmux
+  register
+
+- pinctrl-single,bias-pull-down : value of setting bias pull down in
+  the pinmux register
+
+- pinctrl-single,bias-pull-up : value of setting bias pull up in the
+  pinmux register
+
+- pinctrl-single,bias : value of setting bias in the pinmux register
+
+- pinctrl-single,input-schmitt-mask : mask of setting input schmitt
+  in the pinmux register
+
+- pinctrl-single,input-schmitt-shift : shift of setting input schmitt
+  in the pinmux register
+
 This driver assumes that there is only one register for each pin,
 and uses the common pinctrl bindings as specified in the pinctrl-bindings.txt
 document in this directory.
-- 
1.7.0.4

[PATCH 10/10] document: devicetree: bind pinconf in pinctrl single

[Haojian Zhuang]

Add comments with pinconf & gpio range in the document of
pinctrl-single.

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 .../devicetree/bindings/pinctrl/pinctrl-single.txt |   43 ++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
index 5187f0d..b0e5059 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
@@ -15,6 +15,49 @@ Optional properties:
   available and same for all registers; if not specified, disabling of
   pin functions is ignored
 
+- pinctrl-single,gpio-mask : mask of enabling gpio function register
+
+- pinctrl-single,gpio-ranges : gpio range list
+
+- pinctrl-single,gpio : array with gpio range start, size & register
+  offset
+
+- pinctrl-single,gpio-enable : value of enabling gpio function
+
+- pinctrl-single,gpio-disable : value of disabling gpio function
+
+- pinctrl-single,power-source-mask : mask of setting power source in
+  the pinmux register
+
+- pinctrl-single,power-source-shift : shift of power source field in
+  the pinmux register
+
+- pinctrl-single,power-source : value of setting power source field
+  in the pinmux register
+
+- pinctrl-single,bias-mask : mask of setting bias value in the pinmux
+  register
+
+- pinctrl-single,bias-shift : shift of setting bias value in the
+  pinmux register
+
+- pinctrl-single,bias-disable : value of disabling bias in the pinmux
+  register
+
+- pinctrl-single,bias-pull-down : value of setting bias pull down in
+  the pinmux register
+
+- pinctrl-single,bias-pull-up : value of setting bias pull up in the
+  pinmux register
+
+- pinctrl-single,bias : value of setting bias in the pinmux register
+
+- pinctrl-single,input-schmitt-mask : mask of setting input schmitt
+  in the pinmux register
+
+- pinctrl-single,input-schmitt-shift : shift of setting input schmitt
+  in the pinmux register
+
 This driver assumes that there is only one register for each pin,
 and uses the common pinctrl bindings as specified in the pinctrl-bindings.txt
 document in this directory.
-- 
1.7.0.4

Re: [PATCH 10/10] document: devicetree: bind pinconf in pinctrl single

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 02:09]:
> Add comments with pinconf & gpio range in the document of
> pinctrl-single.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/pinctrl/pinctrl-single.txt |   43 ++++++++++++++++++++
>  1 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> index 5187f0d..b0e5059 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> @@ -15,6 +15,49 @@ Optional properties:
>    available and same for all registers; if not specified, disabling of
>    pin functions is ignored
>  
> +- pinctrl-single,gpio-mask : mask of enabling gpio function register
> +
> +- pinctrl-single,gpio-ranges : gpio range list
> +
> +- pinctrl-single,gpio : array with gpio range start, size & register
> +  offset
> +
> +- pinctrl-single,gpio-enable : value of enabling gpio function
> +
> +- pinctrl-single,gpio-disable : value of disabling gpio function
> +
> +- pinctrl-single,power-source-mask : mask of setting power source in
> +  the pinmux register
> +
> +- pinctrl-single,power-source-shift : shift of power source field in
> +  the pinmux register

Maybe use "value of", "mask for", "shift for" here?
Can somebody native english speaker maybe comment on that?

Regards,

Tony

[PATCH 10/10] document: devicetree: bind pinconf in pinctrl single

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121018 02:09]:
> Add comments with pinconf & gpio range in the document of
> pinctrl-single.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> ---
>  .../devicetree/bindings/pinctrl/pinctrl-single.txt |   43 ++++++++++++++++++++
>  1 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> index 5187f0d..b0e5059 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
> @@ -15,6 +15,49 @@ Optional properties:
>    available and same for all registers; if not specified, disabling of
>    pin functions is ignored
>  
> +- pinctrl-single,gpio-mask : mask of enabling gpio function register
> +
> +- pinctrl-single,gpio-ranges : gpio range list
> +
> +- pinctrl-single,gpio : array with gpio range start, size & register
> +  offset
> +
> +- pinctrl-single,gpio-enable : value of enabling gpio function
> +
> +- pinctrl-single,gpio-disable : value of disabling gpio function
> +
> +- pinctrl-single,power-source-mask : mask of setting power source in
> +  the pinmux register
> +
> +- pinctrl-single,power-source-shift : shift of power source field in
> +  the pinmux register

Maybe use "value of", "mask for", "shift for" here?
Can somebody native english speaker maybe comment on that?

Regards,

Tony

Re: [PATCH 01/10] pinctrl: use postcore_initcall

[Linus Walleij]

This patch has the wrong subject, should be pinctrl: single: or
pinctrl/single: ...

Anyway, I need Tony to ACK them before merging.

Yours,
Linus Walleij

[PATCH 01/10] pinctrl: use postcore_initcall

[Linus Walleij]

This patch has the wrong subject, should be pinctrl: single: or
pinctrl/single: ...

Anyway, I need Tony to ACK them before merging.

Yours,
Linus Walleij

Re: [PATCH 01/10] pinctrl: use postcore_initcall

[Stephen Warren]

On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
> Since pins are configured in device driver, pinctrl driver should be
> loaded by those device driver. module_platform_driver() only declares
> pinctrl driver is in module_initcall privilege. Use postcore_initcall
> privilege instead.

I'm not convinced this is needed; doesn't deferred probe sort out the
dependencies correctly?

[PATCH 01/10] pinctrl: use postcore_initcall

[Stephen Warren]

On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
> Since pins are configured in device driver, pinctrl driver should be
> loaded by those device driver. module_platform_driver() only declares
> pinctrl driver is in module_initcall privilege. Use postcore_initcall
> privilege instead.

I'm not convinced this is needed; doesn't deferred probe sort out the
dependencies correctly?

Re: [PATCH 01/10] pinctrl: use postcore_initcall

[Tony Lindgren]

* Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> [121018 15:20]:
> On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
> > Since pins are configured in device driver, pinctrl driver should be
> > loaded by those device driver. module_platform_driver() only declares
> > pinctrl driver is in module_initcall privilege. Use postcore_initcall
> > privilege instead.
> 
> I'm not convinced this is needed; doesn't deferred probe sort out the
> dependencies correctly?

I'm a bit concerned about this need too as the trend is towards
initializing things later than earlier. The drivers/Makefile order
and deferred probe should be already enough?

Specifically could you decribe the cases where this issue happens?
Also check if one of your client drivers has some early initcall
that's no longer needed.

Regards,

Tony

[PATCH 01/10] pinctrl: use postcore_initcall

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [121018 15:20]:
> On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
> > Since pins are configured in device driver, pinctrl driver should be
> > loaded by those device driver. module_platform_driver() only declares
> > pinctrl driver is in module_initcall privilege. Use postcore_initcall
> > privilege instead.
> 
> I'm not convinced this is needed; doesn't deferred probe sort out the
> dependencies correctly?

I'm a bit concerned about this need too as the trend is towards
initializing things later than earlier. The drivers/Makefile order
and deferred probe should be already enough?

Specifically could you decribe the cases where this issue happens?
Also check if one of your client drivers has some early initcall
that's no longer needed.

Regards,

Tony

Re: [PATCH 01/10] pinctrl: use postcore_initcall

[Haojian Zhuang]

On Fri, Oct 19, 2012 at 6:28 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> [121018 15:20]:
>> On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
>> > Since pins are configured in device driver, pinctrl driver should be
>> > loaded by those device driver. module_platform_driver() only declares
>> > pinctrl driver is in module_initcall privilege. Use postcore_initcall
>> > privilege instead.
>>
>> I'm not convinced this is needed; doesn't deferred probe sort out the
>> dependencies correctly?
>
> I'm a bit concerned about this need too as the trend is towards
> initializing things later than earlier. The drivers/Makefile order
> and deferred probe should be already enough?
>
> Specifically could you decribe the cases where this issue happens?
> Also check if one of your client drivers has some early initcall
> that's no longer needed.
>
> Regards,
>
> Tony

Yes, the special case is PMIC. Most of PMIC are based on I2C/SPI bus.
It means that I2C/SPI bus driver should be initialized firstly. For example,
we could find that PMIC mfd driver are initialized in subsys init call level.
It means that pinctrl should be initialized earlier than I2C/SPI bus driver.
Otherwise, pins of I2C bus may not be configured as I2C function since
pinctrl driver is module init call level.

Regards
Haojian

[PATCH 01/10] pinctrl: use postcore_initcall

[Haojian Zhuang]

On Fri, Oct 19, 2012 at 6:28 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [121018 15:20]:
>> On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
>> > Since pins are configured in device driver, pinctrl driver should be
>> > loaded by those device driver. module_platform_driver() only declares
>> > pinctrl driver is in module_initcall privilege. Use postcore_initcall
>> > privilege instead.
>>
>> I'm not convinced this is needed; doesn't deferred probe sort out the
>> dependencies correctly?
>
> I'm a bit concerned about this need too as the trend is towards
> initializing things later than earlier. The drivers/Makefile order
> and deferred probe should be already enough?
>
> Specifically could you decribe the cases where this issue happens?
> Also check if one of your client drivers has some early initcall
> that's no longer needed.
>
> Regards,
>
> Tony

Yes, the special case is PMIC. Most of PMIC are based on I2C/SPI bus.
It means that I2C/SPI bus driver should be initialized firstly. For example,
we could find that PMIC mfd driver are initialized in subsys init call level.
It means that pinctrl should be initialized earlier than I2C/SPI bus driver.
Otherwise, pins of I2C bus may not be configured as I2C function since
pinctrl driver is module init call level.

Regards
Haojian

Re: [PATCH 01/10] pinctrl: use postcore_initcall

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 19:17]:
> On Fri, Oct 19, 2012 at 6:28 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> >
> > Specifically could you decribe the cases where this issue happens?
> > Also check if one of your client drivers has some early initcall
> > that's no longer needed.
> 
> Yes, the special case is PMIC. Most of PMIC are based on I2C/SPI bus.
> It means that I2C/SPI bus driver should be initialized firstly. For example,
> we could find that PMIC mfd driver are initialized in subsys init call level.
> It means that pinctrl should be initialized earlier than I2C/SPI bus driver.
> Otherwise, pins of I2C bus may not be configured as I2C function since
> pinctrl driver is module init call level.

Hmm, the order in drivers/Makefile is already:

pinctrl/
i2c/

Maybe check that your i2c drivers don't have non-standard initcalls?

Also the i2c drivers may need to return -EPROBE_DEFER?

Regards,

Tony

[PATCH 01/10] pinctrl: use postcore_initcall

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121018 19:17]:
> On Fri, Oct 19, 2012 at 6:28 AM, Tony Lindgren <tony@atomide.com> wrote:
> >
> > Specifically could you decribe the cases where this issue happens?
> > Also check if one of your client drivers has some early initcall
> > that's no longer needed.
> 
> Yes, the special case is PMIC. Most of PMIC are based on I2C/SPI bus.
> It means that I2C/SPI bus driver should be initialized firstly. For example,
> we could find that PMIC mfd driver are initialized in subsys init call level.
> It means that pinctrl should be initialized earlier than I2C/SPI bus driver.
> Otherwise, pins of I2C bus may not be configured as I2C function since
> pinctrl driver is module init call level.

Hmm, the order in drivers/Makefile is already:

pinctrl/
i2c/

Maybe check that your i2c drivers don't have non-standard initcalls?

Also the i2c drivers may need to return -EPROBE_DEFER?

Regards,

Tony

Re: [PATCH 01/10] pinctrl: use postcore_initcall

[Haojian Zhuang]

On Fri, Oct 19, 2012 at 10:38 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 19:17]:
>> On Fri, Oct 19, 2012 at 6:28 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>> >
>> > Specifically could you decribe the cases where this issue happens?
>> > Also check if one of your client drivers has some early initcall
>> > that's no longer needed.
>>
>> Yes, the special case is PMIC. Most of PMIC are based on I2C/SPI bus.
>> It means that I2C/SPI bus driver should be initialized firstly. For example,
>> we could find that PMIC mfd driver are initialized in subsys init call level.
>> It means that pinctrl should be initialized earlier than I2C/SPI bus driver.
>> Otherwise, pins of I2C bus may not be configured as I2C function since
>> pinctrl driver is module init call level.
>
> Hmm, the order in drivers/Makefile is already:
>
> pinctrl/
> i2c/
>
> Maybe check that your i2c drivers don't have non-standard initcalls?
>
> Also the i2c drivers may need to return -EPROBE_DEFER?
>
> Regards,
>
> Tony

OK. I'll support -EPROBE_DEFER if failed to get pin from pinctrl system.
This solution could also resolve the issue.

Regards
Haojian

[PATCH 01/10] pinctrl: use postcore_initcall

[Haojian Zhuang]

On Fri, Oct 19, 2012 at 10:38 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Haojian Zhuang <haojian.zhuang@gmail.com> [121018 19:17]:
>> On Fri, Oct 19, 2012 at 6:28 AM, Tony Lindgren <tony@atomide.com> wrote:
>> >
>> > Specifically could you decribe the cases where this issue happens?
>> > Also check if one of your client drivers has some early initcall
>> > that's no longer needed.
>>
>> Yes, the special case is PMIC. Most of PMIC are based on I2C/SPI bus.
>> It means that I2C/SPI bus driver should be initialized firstly. For example,
>> we could find that PMIC mfd driver are initialized in subsys init call level.
>> It means that pinctrl should be initialized earlier than I2C/SPI bus driver.
>> Otherwise, pins of I2C bus may not be configured as I2C function since
>> pinctrl driver is module init call level.
>
> Hmm, the order in drivers/Makefile is already:
>
> pinctrl/
> i2c/
>
> Maybe check that your i2c drivers don't have non-standard initcalls?
>
> Also the i2c drivers may need to return -EPROBE_DEFER?
>
> Regards,
>
> Tony

OK. I'll support -EPROBE_DEFER if failed to get pin from pinctrl system.
This solution could also resolve the issue.

Regards
Haojian

Re: [PATCH 01/10] pinctrl: use postcore_initcall

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 19:54]:
> On Fri, Oct 19, 2012 at 10:38 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [121018 19:17]:
> >> On Fri, Oct 19, 2012 at 6:28 AM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> >> >
> >> > Specifically could you decribe the cases where this issue happens?
> >> > Also check if one of your client drivers has some early initcall
> >> > that's no longer needed.
> >>
> >> Yes, the special case is PMIC. Most of PMIC are based on I2C/SPI bus.
> >> It means that I2C/SPI bus driver should be initialized firstly. For example,
> >> we could find that PMIC mfd driver are initialized in subsys init call level.
> >> It means that pinctrl should be initialized earlier than I2C/SPI bus driver.
> >> Otherwise, pins of I2C bus may not be configured as I2C function since
> >> pinctrl driver is module init call level.
> >
> > Hmm, the order in drivers/Makefile is already:
> >
> > pinctrl/
> > i2c/
> >
> > Maybe check that your i2c drivers don't have non-standard initcalls?
> >
> > Also the i2c drivers may need to return -EPROBE_DEFER?
> >
> > Regards,
> >
> > Tony
> 
> OK. I'll support -EPROBE_DEFER if failed to get pin from pinctrl system.
> This solution could also resolve the issue.

OK good to hear.

Regards,

Tony

[PATCH 01/10] pinctrl: use postcore_initcall

[Tony Lindgren]

* Haojian Zhuang <haojian.zhuang@gmail.com> [121018 19:54]:
> On Fri, Oct 19, 2012 at 10:38 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Haojian Zhuang <haojian.zhuang@gmail.com> [121018 19:17]:
> >> On Fri, Oct 19, 2012 at 6:28 AM, Tony Lindgren <tony@atomide.com> wrote:
> >> >
> >> > Specifically could you decribe the cases where this issue happens?
> >> > Also check if one of your client drivers has some early initcall
> >> > that's no longer needed.
> >>
> >> Yes, the special case is PMIC. Most of PMIC are based on I2C/SPI bus.
> >> It means that I2C/SPI bus driver should be initialized firstly. For example,
> >> we could find that PMIC mfd driver are initialized in subsys init call level.
> >> It means that pinctrl should be initialized earlier than I2C/SPI bus driver.
> >> Otherwise, pins of I2C bus may not be configured as I2C function since
> >> pinctrl driver is module init call level.
> >
> > Hmm, the order in drivers/Makefile is already:
> >
> > pinctrl/
> > i2c/
> >
> > Maybe check that your i2c drivers don't have non-standard initcalls?
> >
> > Also the i2c drivers may need to return -EPROBE_DEFER?
> >
> > Regards,
> >
> > Tony
> 
> OK. I'll support -EPROBE_DEFER if failed to get pin from pinctrl system.
> This solution could also resolve the issue.

OK good to hear.

Regards,

Tony

Re: [PATCH 01/10] pinctrl: use postcore_initcall

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:06 Thu 18 Oct     , Haojian Zhuang wrote:
> Since pins are configured in device driver, pinctrl driver should be
> loaded by those device driver. module_platform_driver() only declares
> pinctrl driver is in module_initcall privilege. Use postcore_initcall
> privilege instead.
this  should not be mandatory as you can use defer probe

Best Regards,
J.

[PATCH 01/10] pinctrl: use postcore_initcall

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:06 Thu 18 Oct     , Haojian Zhuang wrote:
> Since pins are configured in device driver, pinctrl driver should be
> loaded by those device driver. module_platform_driver() only declares
> pinctrl driver is in module_initcall privilege. Use postcore_initcall
> privilege instead.
this  should not be mandatory as you can use defer probe

Best Regards,
J.