| Message ID | 1346923381-14144-2-git-send-email-thomas.abraham@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
This patch shows the problem of the need to explicitly migrate all drivers
to pinctrl.
Maybe we should consider extending the pinctrl subsystem to set the default
state automatically before binding a driver to a device, at least in case
of DT-based platforms?
This would be similar to what is done currently with samsung-gpio bindings
- the pin is being configured by custom xlate callback based on additional
cells in GPIO specifier, when the driver retrieves the pin using
of_get{_named,}_gpio without the need of setting it up in the driver.
Best regards,
On 6 September 2012 15:04, Tomasz Figa <t.figa@samsung.com> wrote: > Hi, > > This patch shows the problem of the need to explicitly migrate all drivers > to pinctrl. > > Maybe we should consider extending the pinctrl subsystem to set the default > state automatically before binding a driver to a device, at least in case > of DT-based platforms? The pinctrl driver allows for activating default pin configuration when the pinctrl driver loads. This is referred to as "hogging". But should hog be used or not is something that needs to be decided. Some of the factors which favor the driver explicitly setting up the pin configuration are 1. After a suspend and resume cycle, the pin configuration registers may be reset to default values. Hence, during resume, the pin configuration has be redone. 2. Runtime muxing/config is possible. 3. Setting some of the config options such as pull-up by default might start consuming power from boot time itself, which could be avoided if such setup is done only when needed. Adding pinctrl driver support in device drivers seems to be simple task. And it is just one time effort which can be reused on multiple SoC's. > > This would be similar to what is done currently with samsung-gpio bindings > - the pin is being configured by custom xlate callback based on additional > cells in GPIO specifier, when the driver retrieves the pin using > of_get{_named,}_gpio without the need of setting it up in the driver. The Samsung gpio dt bindings was just a bootstrap method to get device tree support going for Samsung platforms. The gpio xlate callback was used as a back door to setup the pinmux/pinconfig due to lack of generic driver interface to setup the pinmux/pinconfig for Samsung platforms. From a linux perspective, gpio and pinmux/pinconfig are separate entities. So using gpio xlate to setup pinmux/pinconfig was not correct but helped getting device tree enabled for Samsung platforms. With the pinctrl framework available now, there are generic interfaces to setup gpio and pinmux /pinconfig. Thanks, Thomas. > > Best regards, > -- > Tomasz Figa > Samsung Poland R&D Center > > On Thursday 06 of September 2012 14:53:00 Thomas Abraham wrote: >> Add optional support for i2c bus pin configuration using pinctrl >> interface >> >> Cc: Ben Dooks <ben-linux@fluff.org> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> >> --- >> drivers/i2c/busses/i2c-s3c2410.c | 19 ++++++++++++++++--- >> 1 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-s3c2410.c >> b/drivers/i2c/busses/i2c-s3c2410.c index 5ae3b02..f4b2d6f 100644 >> --- a/drivers/i2c/busses/i2c-s3c2410.c >> +++ b/drivers/i2c/busses/i2c-s3c2410.c >> @@ -38,6 +38,7 @@ >> #include <linux/io.h> >> #include <linux/of_i2c.h> >> #include <linux/of_gpio.h> >> +#include <linux/pinctrl/consumer.h> >> >> #include <asm/irq.h> >> >> @@ -83,6 +84,8 @@ struct s3c24xx_i2c { >> >> struct s3c2410_platform_i2c *pdata; >> int gpios[2]; >> + struct pinctrl *pctrl; >> + struct pinctrl_state *pctrl_state; >> #ifdef CONFIG_CPU_FREQ >> struct notifier_block freq_transition; >> #endif >> @@ -859,11 +862,17 @@ static int s3c24xx_i2c_init(struct s3c24xx_i2c >> *i2c) >> >> /* inititalise the gpio */ >> >> - if (pdata->cfg_gpio) >> + if (pdata->cfg_gpio) { >> pdata->cfg_gpio(to_platform_device(i2c->dev)); >> - else >> - if (s3c24xx_i2c_parse_dt_gpio(i2c)) >> + } else if (!IS_ERR_OR_NULL(i2c->pctrl) && >> + !IS_ERR_OR_NULL(i2c->pctrl_state)) { >> + if (pinctrl_select_state(i2c->pctrl, i2c->pctrl_state)) { >> + dev_err(i2c->dev, "failed to configure io-pins\n"); >> + return -ENXIO; >> + } >> + } else if (s3c24xx_i2c_parse_dt_gpio(i2c)) { >> return -EINVAL; >> + } >> >> /* write slave address */ >> >> @@ -1013,6 +1022,10 @@ static int s3c24xx_i2c_probe(struct >> platform_device *pdev) i2c->adap.algo_data = i2c; >> i2c->adap.dev.parent = &pdev->dev; >> >> + i2c->pctrl = devm_pinctrl_get(i2c->dev); >> + if (!IS_ERR_OR_NULL(i2c->pctrl)) >> + i2c->pctrl_state = pinctrl_lookup_state(i2c->pctrl, "default"); >> + >> /* initialise the i2c controller */ >> >> ret = s3c24xx_i2c_init(i2c); >
Hi, Thanks for your comments. On Thursday 06 of September 2012 16:36:08 Thomas Abraham wrote: > > This patch shows the problem of the need to explicitly migrate all > > drivers to pinctrl. > > > > Maybe we should consider extending the pinctrl subsystem to set the > > default state automatically before binding a driver to a device, at > > least in case of DT-based platforms? > > The pinctrl driver allows for activating default pin configuration > when the pinctrl driver loads. This is referred to as "hogging". What I suggested is that such default configuration would be applied just before binding a driver, i.e. when it's almost sure that the device will be actually used and the configuration will be needed. Of course such functionality would not have to be obligatory. For example, we could add new property, like pinctrl-set-default, to point in device tree that this device should have its pinctrl state set up automatically. > But > should hog be used or not is something that needs to be decided. Some > of the factors which favor the driver explicitly setting up the pin > configuration are > > 1. After a suspend and resume cycle, the pin configuration registers > may be reset to default values. Hence, during resume, the pin > configuration has be redone. In my opinion it should be saved and restored by pinctrl driver (as it was done in case of GPIO subsystem previously). This is because the driver can usually quickly restore all pins at once and some hardware even provide facilities for restoring pin configuration after suspend. (e.g. s3c6410, probably not going to use pinctrl, but possibly there are more systems using this kind of solution, which allows to switch all pins at once from sleep mode settings to normal mode). > 2. Runtime muxing/config is possible. Of course, for cases where runtime remuxing is required this would be no option, but often there is just a single pin configuration used all the time. > 3. Setting some of the config options such as pull-up by default might > start consuming power from boot time itself, which could be avoided if > such setup is done only when needed. Making it optional would solve this issue, because in such cases the driver already has to be aware of setting pull-ups and similar. > Adding pinctrl driver support in device drivers seems to be simple > task. And it is just one time effort which can be reused on multiple > SoC's. I agree. Although not modifying the drivers at all would be nicer. Best regards,
On 09/06/2012 05:06 AM, Thomas Abraham wrote: > On 6 September 2012 15:04, Tomasz Figa <t.figa@samsung.com> wrote: >> Hi, >> >> This patch shows the problem of the need to explicitly migrate all drivers >> to pinctrl. >> >> Maybe we should consider extending the pinctrl subsystem to set the default >> state automatically before binding a driver to a device, at least in case >> of DT-based platforms? > > The pinctrl driver allows for activating default pin configuration > when the pinctrl driver loads. This is referred to as "hogging". But > should hog be used or not is something that needs to be decided. Some > of the factors which favor the driver explicitly setting up the pin > configuration are > > 1. After a suspend and resume cycle, the pin configuration registers > may be reset to default values. Hence, during resume, the pin > configuration has be redone. I'd think it's the pinctrl driver's responsibility to make hogging work correctly across suspend/resume. > 2. Runtime muxing/config is possible. The "client" driver would definitely have to be involved there, I agree. > 3. Setting some of the config options such as pull-up by default might > start consuming power from boot time itself, which could be avoided if > such setup is done only when needed. Well, the difference in time between "just before driver binding" and "during probe" is minimal. If the driver/HW really needs to explicitly differentiate between those states to save power, I'd assert that it's covered by case (2) above. > Adding pinctrl driver support in device drivers seems to be simple > task. And it is just one time effort which can be reused on multiple > SoC's. > >> >> This would be similar to what is done currently with samsung-gpio bindings >> - the pin is being configured by custom xlate callback based on additional >> cells in GPIO specifier, when the driver retrieves the pin using >> of_get{_named,}_gpio without the need of setting it up in the driver. > > The Samsung gpio dt bindings was just a bootstrap method to get device > tree support going for Samsung platforms. The gpio xlate callback was > used as a back door to setup the pinmux/pinconfig due to lack of > generic driver interface to setup the pinmux/pinconfig for Samsung > platforms. From a linux perspective, gpio and pinmux/pinconfig are > separate entities. So using gpio xlate to setup pinmux/pinconfig was > not correct but helped getting device tree enabled for Samsung > platforms. With the pinctrl framework available now, there are generic > interfaces to setup gpio and pinmux /pinconfig. I agree; the Samsung GPIO bindings were surprising to me when I first realized what was in the GPIO specifiers...
On Thu, Sep 6, 2012 at 2:28 PM, Tomasz Figa <t.figa@samsung.com> wrote: > Thanks for your comments. > > On Thursday 06 of September 2012 16:36:08 Thomas Abraham wrote: >> > This patch shows the problem of the need to explicitly migrate all >> > drivers to pinctrl. >> > >> > Maybe we should consider extending the pinctrl subsystem to set the >> > default state automatically before binding a driver to a device, at >> > least in case of DT-based platforms? >> >> The pinctrl driver allows for activating default pin configuration >> when the pinctrl driver loads. This is referred to as "hogging". > > What I suggested is that such default configuration would be applied just > before binding a driver, i.e. when it's almost sure that the device will be > actually used and the configuration will be needed. In that case the pinctrl driver can poke down a default register configuration, but just like Stephen remarks I do not see the point either. Hogs should work just fine? > Of course such functionality would not have to be obligatory. For example, > we could add new property, like pinctrl-set-default, to point in device > tree that this device should have its pinctrl state set up automatically. This doesn't have a semantic difference to a default hog AFAICT. >> 1. After a suspend and resume cycle, the pin configuration registers >> may be reset to default values. Hence, during resume, the pin >> configuration has be redone. > > In my opinion it should be saved and restored by pinctrl driver (as it was > done in case of GPIO subsystem previously). This is one way to do it. I see Stephen has the same idea. Another way is actually to go to the default state from the default state. Or from any state to itself rather. Currently pinctrl_select_state_locked() contains this: if (p->state == state) return 0; If we just add pinctrl_select_state_force() excluding that clause we can force re-poking of the state at any time. Compare regulator_force_disable() and similar. Maybe this is actually more helpful? Yours, Linus Walleij
On Thu, Sep 6, 2012 at 11:23 AM, Thomas Abraham <thomas.abraham@linaro.org> wrote: > Add optional support for i2c bus pin configuration using pinctrl interface > > Cc: Ben Dooks <ben-linux@fluff.org> > Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> (...) > diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c (...) > - else > - if (s3c24xx_i2c_parse_dt_gpio(i2c)) > + } else if (!IS_ERR_OR_NULL(i2c->pctrl) && > + !IS_ERR_OR_NULL(i2c->pctrl_state)) { You don't need to check i2c->pctrl here, just check i2c->pctrl_state. If i2c->pctrl failed the other one will be null too, anyway. (Or the struct isn't kzalloc:ed properly... which I assume.) > + if (pinctrl_select_state(i2c->pctrl, i2c->pctrl_state)) { > + dev_err(i2c->dev, "failed to configure io-pins\n"); > + return -ENXIO; > + } > + } else if (s3c24xx_i2c_parse_dt_gpio(i2c)) { > return -EINVAL; > + } > > /* write slave address */ > > @@ -1013,6 +1022,10 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev) > i2c->adap.algo_data = i2c; > i2c->adap.dev.parent = &pdev->dev; > > + i2c->pctrl = devm_pinctrl_get(i2c->dev); > + if (!IS_ERR_OR_NULL(i2c->pctrl)) > + i2c->pctrl_state = pinctrl_lookup_state(i2c->pctrl, "default"); > + If all you do is select the default state (later, in the init function) the use devm_pinctrl_get_select_default() and be done with it. In any case do not open code the string "default", use PINCTRL_STATE_DEFAULT which defines to that string. Yours, Linus Walleij
On 11 September 2012 00:51, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 09/06/2012 05:06 AM, Thomas Abraham wrote: >> On 6 September 2012 15:04, Tomasz Figa <t.figa@samsung.com> wrote: >>> Hi, >>> >>> This patch shows the problem of the need to explicitly migrate all drivers >>> to pinctrl. >>> >>> Maybe we should consider extending the pinctrl subsystem to set the default >>> state automatically before binding a driver to a device, at least in case >>> of DT-based platforms? >> >> The pinctrl driver allows for activating default pin configuration >> when the pinctrl driver loads. This is referred to as "hogging". But >> should hog be used or not is something that needs to be decided. Some >> of the factors which favor the driver explicitly setting up the pin >> configuration are >> >> 1. After a suspend and resume cycle, the pin configuration registers >> may be reset to default values. Hence, during resume, the pin >> configuration has be redone. > > I'd think it's the pinctrl driver's responsibility to make hogging work > correctly across suspend/resume. Ok. I will add this functionality in the Samsung pinctrl driver. Thanks, Thomas. [...]
On 11 September 2012 01:32, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Sep 6, 2012 at 11:23 AM, Thomas Abraham > <thomas.abraham@linaro.org> wrote: > >> Add optional support for i2c bus pin configuration using pinctrl interface >> >> Cc: Ben Dooks <ben-linux@fluff.org> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> > (...) >> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c > (...) >> - else >> - if (s3c24xx_i2c_parse_dt_gpio(i2c)) >> + } else if (!IS_ERR_OR_NULL(i2c->pctrl) && >> + !IS_ERR_OR_NULL(i2c->pctrl_state)) { > > You don't need to check i2c->pctrl here, just check i2c->pctrl_state. > > If i2c->pctrl failed the other one will be null too, anyway. > (Or the struct isn't kzalloc:ed properly... which I assume.) Yes, that's right. I will modify this check. > >> + if (pinctrl_select_state(i2c->pctrl, i2c->pctrl_state)) { >> + dev_err(i2c->dev, "failed to configure io-pins\n"); >> + return -ENXIO; >> + } >> + } else if (s3c24xx_i2c_parse_dt_gpio(i2c)) { >> return -EINVAL; >> + } >> >> /* write slave address */ >> >> @@ -1013,6 +1022,10 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev) >> i2c->adap.algo_data = i2c; >> i2c->adap.dev.parent = &pdev->dev; >> >> + i2c->pctrl = devm_pinctrl_get(i2c->dev); >> + if (!IS_ERR_OR_NULL(i2c->pctrl)) >> + i2c->pctrl_state = pinctrl_lookup_state(i2c->pctrl, "default"); >> + > > If all you do is select the default state (later, in the init function) > the use devm_pinctrl_get_select_default() and be done > with it. The driver has a separate init which is invoked during probe. This init function sets up the i/o pads for the i2c controller. This function has to support three different types of Samsung platforms - (a) non-dt (b) dt without pinctrl and (c) dt with pinctrl. And the decision to use one of the types of pin setup has to be taken at runtime (to support multi-platform images) So the order that is used is 1. Check if platform data has a callback for pin setup. If yes, invoke that callback. 2. If not, check if a vaild pinctrl state available, (implies pinctrl driver support is available), then use the pinctrl_select_state() call to setup the pins. 3. If both the above options are not available, then try setting up the pins with information available in device tree node. When all the Samsung platforms switch to using device tree and pinctrl driver, this code would be simplified to use the devm_pinctrl_get_select_default() function. > > In any case do not open code the string "default", use > PINCTRL_STATE_DEFAULT which defines to that string. Okay, that was a mistake. I will fix this. > > Yours, > Linus Walleij Thanks, Thomas.
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c index 5ae3b02..f4b2d6f 100644 --- a/drivers/i2c/busses/i2c-s3c2410.c +++ b/drivers/i2c/busses/i2c-s3c2410.c @@ -38,6 +38,7 @@ #include <linux/io.h> #include <linux/of_i2c.h> #include <linux/of_gpio.h> +#include <linux/pinctrl/consumer.h> #include <asm/irq.h> @@ -83,6 +84,8 @@ struct s3c24xx_i2c { struct s3c2410_platform_i2c *pdata; int gpios[2]; + struct pinctrl *pctrl; + struct pinctrl_state *pctrl_state; #ifdef CONFIG_CPU_FREQ struct notifier_block freq_transition; #endif @@ -859,11 +862,17 @@ static int s3c24xx_i2c_init(struct s3c24xx_i2c *i2c) /* inititalise the gpio */ - if (pdata->cfg_gpio) + if (pdata->cfg_gpio) { pdata->cfg_gpio(to_platform_device(i2c->dev)); - else - if (s3c24xx_i2c_parse_dt_gpio(i2c)) + } else if (!IS_ERR_OR_NULL(i2c->pctrl) && + !IS_ERR_OR_NULL(i2c->pctrl_state)) { + if (pinctrl_select_state(i2c->pctrl, i2c->pctrl_state)) { + dev_err(i2c->dev, "failed to configure io-pins\n"); + return -ENXIO; + } + } else if (s3c24xx_i2c_parse_dt_gpio(i2c)) { return -EINVAL; + } /* write slave address */ @@ -1013,6 +1022,10 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev) i2c->adap.algo_data = i2c; i2c->adap.dev.parent = &pdev->dev; + i2c->pctrl = devm_pinctrl_get(i2c->dev); + if (!IS_ERR_OR_NULL(i2c->pctrl)) + i2c->pctrl_state = pinctrl_lookup_state(i2c->pctrl, "default"); + /* initialise the i2c controller */ ret = s3c24xx_i2c_init(i2c);
Add optional support for i2c bus pin configuration using pinctrl interface Cc: Ben Dooks <ben-linux@fluff.org> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> --- drivers/i2c/busses/i2c-s3c2410.c | 19 ++++++++++++++++--- 1 files changed, 16 insertions(+), 3 deletions(-)