pinctrl: use postcore_initcall
Subsystems (8)
PATCH 1/10 pinctrl: use postcore_initcall
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.
--- drivers/pinctrl/pinctrl-single.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
drivers/pinctrl/pinctrl-single.c +11 -1
@@ -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 <[email protected]>" );MODULE_DESCRIPTION ("One-register-per-pin type device tree based pinctrl driver" );
Signature · 1 line
1.7.0.4
PATCH 2/10 ARM: mmp: select pinctrl driver
Pinctrl driver is necessary for MMP DT & MMP2 DT platforms.
--- arch/arm/mach-mmp/Kconfig | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
arch/arm/mach-mmp/Kconfig +2 -0
@@ -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.
Signature · 1 line
1.7.0.4
PATCH 3/10 tty: pxa: configure pin
In reply to Haojian ZhuangConfigure pins by pinctrl driver.
--- drivers/tty/serial/pxa.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
drivers/tty/serial/pxa.c +8 -0
@@ -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 ; }
Signature · 1 line
1.7.0.4
PATCH 3/10 tty: pxa: configure pin
Quoted 3 lines · Haojian Zhuang
Configure pins by pinctrl driver.
Signed-off-by Haojian Zhuang <[email protected]>
Yours,
Linus Walleij
PATCH 3/10 tty: pxa: configure pin
In reply to Haojian ZhuangQuoted 1 line · Haojian Zhuang
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 3/10 tty: pxa: configure pin
Quoted 7 lines · Stephen Warren
Quoted 1 line · Haojian Zhuang
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.
Quoted 6 lines
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 3/10 tty: pxa: configure pin
Quoted 17 lines · Linus Walleij
Quoted 7 lines · Stephen Warren
Quoted 1 line · Haojian Zhuang
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).
Quoted 4 lines
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.
Quoted 5 lines
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.
Quoted 7 lines
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.cI could easily imaginepinctrl_ops.clike that for some platforms. This mandates still binding the pin table entries to a device but avoids adding any code to the drivers.
Quoted 8 lines
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 3/10 tty: pxa: configure pin
Quoted 9 lines · Stephen Warren
Quoted 4 lines · Linus Walleij
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.
Quoted 10 lines
Quoted 5 lines
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_STATEexists.
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.
Quoted 16 lines
Quoted 7 lines
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.cI could easily imaginepinctrl_ops.clike that for some platforms. This mandates still binding the pin table entries to a device but avoids adding any code to the drivers.Quoted 8 lines
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.
Quoted 4 lines
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 3/10 tty: pxa: configure pin
Quoted 17 lines · Linus Walleij
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 domainIn others:
-
clk_disable()
- disable external regulator
- pins tosleep()
- release power domain
Quoted 4 lines
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?
Quoted 3 lines
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>
PATCH 3/10 tty: pxa: configure pin
Quoted 18 lines · Mark Brown
Quoted 16 lines · Linus Walleij
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 domainIn others:
-
clk_disable()
- disable external regulator
- pins tosleep()
- release power domain
Quoted 8 lines
Quoted 4 lines
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 3/10 tty: pxa: configure pin
Quoted 11 lines · Linus Walleij
On Tue, Oct 23, 2012 at 11:37 AM, Mark Brown
Quoted 5 lines · Linus Walleij
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...
Quoted 8 lines
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>
PATCH 3/10 tty: pxa: configure pin
Quoted 11 lines · Mark Brown
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 4/10 i2c: pxa: configure pins
In reply to Haojian ZhuangConfigure pins by pinctrl driver.
--- drivers/i2c/busses/i2c-pxa.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/i2c/busses/i2c-pxa.c +5 -0
@@ -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 ))
Signature · 1 line
1.7.0.4
PATCH 4/10 i2c: pxa: configure pins
Quoted 3 lines · Haojian Zhuang
Configure pins by pinctrl driver.
Signed-off-by Haojian Zhuang <[email protected]>
Yours,
Linus Walleij
PATCH 5/10 i2c: pxa: use devm_kzalloc
In reply to Haojian ZhuangUse devm_kzalloc & add checking in probe() function.
--- drivers/i2c/busses/i2c-pxa.c | 26 ++++++++++---------------- 1 files changed, 10 insertions(+), 16 deletions(-)
drivers/i2c/busses/i2c-pxa.c +10 -16
@@ -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; }
Signature · 1 line
1.7.0.4
PATCH 5/10 i2c: pxa: use devm_kzalloc
Quoted 1 line · Haojian Zhuang
Use
devm_kzalloc& add checking inprobe()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 5/10 i2c: pxa: use devm_kzalloc
Quoted 6 lines · Stephen Warren
Quoted 1 line · Haojian Zhuang
Use
devm_kzalloc& add checking inprobe()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 6/10 pinctrl: single: support gpio request and free
In reply to Haojian ZhuangMarvell'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>
--- drivers/pinctrl/pinctrl-single.c | 140 +++++++++++++++++++++++++++++++++++++- 1 files changed, 138 insertions(+), 2 deletions(-)
drivers/pinctrl/pinctrl-single.c +138 -2
@@ -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);
Signature · 1 line
1.7.0.4
Tony Lindgren
maintainerPATCH 6/10 pinctrl: single: support gpio request and free
Hi,
Few minor comments below.
Quoted 4 lines · 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.
Quoted 8 lines
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?
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c +3
-0@@ -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
drivers/pinctrl/pinctrl-single.c struct pcs_function { · line 91
Quoted 10 lines
drivers/pinctrl/pinctrl-single.c +10
-0+* @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..
drivers/pinctrl/pinctrl-single.c static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector, · line 422
Quoted 12 lines
drivers/pinctrl/pinctrl-single.c +5
-2@@ -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?
drivers/pinctrl/pinctrl-single.c static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector, · line 435
Quoted 12 lines
drivers/pinctrl/pinctrl-single.c +12
-0+ 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.
Quoted 13 lines
drivers/pinctrl/pinctrl-single.c +13
-0+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)
drivers/pinctrl/pinctrl-single.c static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector, · line 458
Quoted 10 lines
drivers/pinctrl/pinctrl-single.c +10
-0+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.
Quoted 43 lines
drivers/pinctrl/pinctrl-single.c +43
-0+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 ];
drivers/pinctrl/pinctrl-single.c static void pcs_free_resources(struct pcs_device *pcs) · line 970
Quoted 10 lines
drivers/pinctrl/pinctrl-single.c +10
-0+ 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 7/10 pinctrl: remove mutex lock in groups show
In reply to Haojian ZhuangMutex 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().
--- drivers/pinctrl/pinconf.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
drivers/pinctrl/pinconf.c +0 -4
@@ -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 ; }
Signature · 1 line
1.7.0.4
Linus Walleij
AppliedPATCH 7/10 pinctrl: remove mutex lock in groups show
Quoted 5 lines · Haojian Zhuang
Mutex is locked duplicatly by
pinconf_groups_show()andpin_config_group_get(). It results dead lock. So avoid to lock mutex inpinconf_groups_show().Signed-off-by Haojian Zhuang <[email protected]>
Thanks! Applied to my fixes branch with a Cc: stable tag.
Yours,
Linus Walleij
PATCH 7/10 pinctrl: remove mutex lock in groups show
In reply to Haojian ZhuangQuoted 3 lines · Haojian Zhuang
Mutex is locked duplicatly by
pinconf_groups_show()andpin_config_group_get(). It results dead lock. So avoid to lock mutex inpinconf_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 7/10 pinctrl: remove mutex lock in groups show
Quoted 9 lines · Stephen Warren
Quoted 3 lines · Haojian Zhuang
Mutex is locked duplicatly by
pinconf_groups_show()andpin_config_group_get(). It results dead lock. So avoid to lock mutex inpinconf_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.
Quoted 4 lines
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)
Quoted 8 lines
{ 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->privateis 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 8/10 pinctrl: single: support pinconf generic
In reply to Haojian ZhuangAdd pinconf generic support with POWER SOURCE, BIAS PULL.
--- drivers/pinctrl/Kconfig | 2 +- drivers/pinctrl/pinctrl-single.c | 286 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 274 insertions(+), 14 deletions(-)
drivers/pinctrl/Kconfig +1 -1
@@ -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.
drivers/pinctrl/pinctrl-single.c +273 -13
405 lines not on this page · raw message
Signature · 1 line
1.7.0.4
PATCH 8/10 pinctrl: single: support pinconf generic
Quoted 3 lines · Haojian Zhuang
Add pinconf generic support with POWER SOURCE, BIAS PULL.
Signed-off-by Haojian Zhuang <[email protected]>
I really like the looks of this, good job Haojian!
Now we just need to hear what Tony says about it...
Yours,
Linus Walleij
Tony Lindgren
maintainerPATCH 8/10 pinctrl: single: support pinconf generic
Quoted 10 lines · Linus Walleij
Quoted 3 lines · Haojian Zhuang
Add pinconf generic support with POWER SOURCE, BIAS PULL.
Signed-off-by Haojian Zhuang <[email protected]>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 8/10 pinctrl: single: support pinconf generic
Quoted 10 lines · Tony Lindgren
Quoted 9 lines · Linus Walleij
Quoted 3 lines · Haojian Zhuang
Add pinconf generic support with POWER SOURCE, BIAS PULL.
Signed-off-by Haojian Zhuang <[email protected]>I really like the looks of this, good job Haojian!
Quoted 8 lines
Quoted 1 line
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
Tony Lindgren
maintainerPATCH 8/10 pinctrl: single: support pinconf generic
Quoted 6 lines · Haojian Zhuang
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.
Quoted 7 lines
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
Tony Lindgren
maintainerPATCH 8/10 pinctrl: single: support pinconf generic
Quoted 11 lines · Tony Lindgren
Quoted 10 lines · Haojian Zhuang
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".
Quoted 5 lines
Quoted 3 lines
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
Tony Lindgren
maintainerPATCH 8/10 pinctrl: single: support pinconf generic
In reply to Haojian ZhuangQuoted 5 lines · Haojian Zhuang
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
Tony Lindgren
maintainerPATCH 8/10 pinctrl: single: support pinconf generic
In reply to Haojian ZhuangQuoted 1 line · Haojian Zhuang
Add pinconf generic support with POWER SOURCE, BIAS PULL.
...
drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 520
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c +9
-0+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 8/10 pinctrl: single: support pinconf generic
Quoted 18 lines · Tony Lindgren
Quoted 1 line · Haojian Zhuang
Add pinconf generic support with POWER SOURCE, BIAS PULL.
...
drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 520
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c +9
-0+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?
Quoted 9 lines
I could use
PIN_CONFIG_POWER_SOURCEfor 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
Tony Lindgren
maintainerPATCH 8/10 pinctrl: single: support pinconf generic
Quoted 24 lines · Haojian Zhuang
Quoted 23 lines · Tony Lindgren
Quoted 1 line · Haojian Zhuang
Add pinconf generic support with POWER SOURCE, BIAS PULL.
...
drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 520
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c +9
-0+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_SOURCEfor 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.
Quoted 7 lines
Quoted 4 lines
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
Quoted 1 line
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
Tony Lindgren
maintainerPATCH 8/10 pinctrl: single: support pinconf generic
Quoted 45 lines · Tony Lindgren
Quoted 24 lines · Haojian Zhuang
Quoted 23 lines · Tony Lindgren
Quoted 1 line · Haojian Zhuang
Add pinconf generic support with POWER SOURCE, BIAS PULL.
...
drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 520
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c +9
-0+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_SOURCEfor 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.Quoted 7 lines
Quoted 4 lines
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
26MMC1_PWDNZ
25MMC1_PBIASLITE_HIZ_MODE
24MMC1_PBIASLITE_SUPPLY_HI_OUT
23MMC1_PBIASLITE_VMODE_ERRORthen this bit needs to clear..
22MMC1_PBIASLITE_PWRDNZ
21MMC1_PBIASLITE_VMODE..after VMODE bit is set to 3V
Quoted 8 lines
Quoted 1 line
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 8/10 pinctrl: single: support pinconf generic
Quoted 62 lines · Tony Lindgren
Quoted 45 lines · Tony Lindgren
Quoted 24 lines · Haojian Zhuang
Quoted 23 lines · Tony Lindgren
Quoted 1 line · Haojian Zhuang
Add pinconf generic support with POWER SOURCE, BIAS PULL.
...
drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 520
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c +9
-0+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_SOURCEfor 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.Quoted 7 lines
Quoted 4 lines
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 3VQuoted 8 lines
Quoted 1 line
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_ERRORbit is not just for VMODE, it's a comparator that can also triggers for the other invalid states forCONTROL_PBIASLITEpinconf register. So hidingVMODE_ERRORinto a regulator would be wrong. For now, VMODE best handled usingPIN_CONFIG_POWER_SOURCEand let the MMC driver do the checking using the pinconf API.
Quoted 3 lines
Regards,
Tony
Could you share the link of downloading the spec?
Regards
Haojian
Tony Lindgren
maintainerPATCH 8/10 pinctrl: single: support pinconf generic
Quoted 63 lines · Haojian Zhuang
Quoted 62 lines · Tony Lindgren
Quoted 45 lines · Tony Lindgren
Quoted 24 lines · Haojian Zhuang
Quoted 23 lines · Tony Lindgren
Quoted 1 line · Haojian Zhuang
Add pinconf generic support with POWER SOURCE, BIAS PULL.
...
drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 520
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c +9
-0+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_SOURCEfor 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.Quoted 7 lines
Quoted 4 lines
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 3VQuoted 8 lines
Quoted 1 line
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_ERRORbit is not just for VMODE, it's a comparator that can also triggers for the other invalid states forCONTROL_PBIASLITEpinconf register. So hidingVMODE_ERRORinto a regulator would be wrong. For now, VMODE best handled usingPIN_CONFIG_POWER_SOURCEand let the MMC driver do the checking using the pinconf API.
Quoted 5 lines
Quoted 3 lines
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 8/10 pinctrl: single: support pinconf generic
In reply to Tony LindgrenQuoted 55 lines · Tony Lindgren
Quoted 45 lines · Tony Lindgren
Quoted 24 lines · Haojian Zhuang
Quoted 23 lines · Tony Lindgren
Quoted 1 line · Haojian Zhuang
Add pinconf generic support with POWER SOURCE, BIAS PULL.
...
drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 520
Quoted 9 lines
drivers/pinctrl/pinctrl-single.c +9
-0+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_SOURCEfor 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.Quoted 7 lines
Quoted 4 lines
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 3VQuoted 8 lines
Quoted 1 line
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.
Quoted 7 lines
Well it turns out the
VMODE_ERRORbit is not just for VMODE, it's a comparator that can also triggers for the other invalid states forCONTROL_PBIASLITEpinconf register. So hidingVMODE_ERRORinto a regulator would be wrong. For now, VMODE best handled usingPIN_CONFIG_POWER_SOURCEand 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 9/10 ARM: dts: support pinctrl single in pxa910
In reply to Haojian ZhuangAdd pinctrl-single support with device tree in pxa910 dkb platform.
--- arch/arm/boot/dts/pxa910-dkb.dts | 187 +++++++++++++++++++++++++++++++++++++- arch/arm/boot/dts/pxa910.dtsi | 81 ++++++++++++++++ 2 files changed, 267 insertions(+), 1 deletions(-)
arch/arm/boot/dts/pxa910-dkb.dts +186 -1
@@ -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 {
arch/arm/boot/dts/pxa910.dtsi +81 -0
87 lines not on this page · raw message
Signature · 1 line
1.7.0.4
PATCH 10/10 document: devicetree: bind pinconf in pinctrl single
In reply to Haojian ZhuangAdd comments with pinconf & gpio range in the document of pinctrl-single.
--- .../devicetree/bindings/pinctrl/pinctrl-single.txt | 43 ++++++++++++++++++++ 1 files changed, 43 insertions(+), 0 deletions(-)
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +43 -0
@@ -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.
Signature · 1 line
1.7.0.4
Tony Lindgren
maintainerPATCH 10/10 document: devicetree: bind pinconf in pinctrl single
Quoted 22 lines · Haojian Zhuang
Add comments with pinconf & gpio range in the document of pinctrl-single.
Signed-off-by Haojian Zhuang <[email protected]>--- .../devicetree/bindings/pinctrl/pinctrl-single.txt | 43 ++++++++++++++++++++ 1 files changed, 43 insertions(+), 0 deletions(-)Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +6
-0@@ -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
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt Optional properties: · line 33
Quoted 10 lines
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +10
-0+ +- 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 1/10 pinctrl: use postcore_initcall
In reply to Haojian ZhuangThis 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 1/10 pinctrl: use postcore_initcall
In reply to Haojian ZhuangQuoted 4 lines · 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 inmodule_initcallprivilege. Usepostcore_initcallprivilege instead.
I'm not convinced this is needed; doesn't deferred probe sort out the dependencies correctly?
Tony Lindgren
maintainerPATCH 1/10 pinctrl: use postcore_initcall
Quoted 8 lines · Stephen Warren
Quoted 4 lines · 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 inmodule_initcallprivilege. Usepostcore_initcallprivilege 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 1/10 pinctrl: use postcore_initcall
Quoted 14 lines · Tony Lindgren
Quoted 8 lines · Stephen Warren
Quoted 4 lines · 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 inmodule_initcallprivilege. Usepostcore_initcallprivilege 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/Makefileorder and deferred probe should be already enough?
Quoted 7 lines
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
Tony Lindgren
maintainerPATCH 1/10 pinctrl: use postcore_initcall
Quoted 6 lines · Haojian Zhuang
Quoted 4 lines · Tony Lindgren
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.
Quoted 6 lines
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 1/10 pinctrl: use postcore_initcall
Quoted 19 lines · Tony Lindgren
Quoted 6 lines · Haojian Zhuang
Quoted 4 lines · Tony Lindgren
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.
Quoted 6 lines
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/Makefileis already:pinctrl/
i2c/
Quoted 7 lines
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
Tony Lindgren
maintainerPATCH 1/10 pinctrl: use postcore_initcall
Quoted 22 lines · Haojian Zhuang
Quoted 21 lines · Tony Lindgren
Quoted 6 lines · Haojian Zhuang
Quoted 4 lines · Tony Lindgren
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.
Quoted 6 lines
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/Makefileis already:pinctrl/
i2c/Maybe check that your i2c drivers don't have non-standard initcalls?
Quoted 8 lines
Quoted 5 lines
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 1/10 pinctrl: use postcore_initcall
In reply to Haojian ZhuangQuoted 4 lines · 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 inmodule_initcallprivilege. Usepostcore_initcallprivilege instead.
this should not be mandatory as you can use defer probe
Best Regards,
J.