Re: [PATCH 1/2] i2c: s3c2410: add optional pin configuration using pinctrl interface

2012-09-17 Thread Thomas Abraham
On 11 September 2012 00:51, Stephen Warren swar...@wwwdotorg.org wrote:
 On 09/06/2012 05:06 AM, Thomas Abraham wrote:
 On 6 September 2012 15:04, Tomasz Figa t.f...@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.

[...]
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] i2c: s3c2410: add optional pin configuration using pinctrl interface

2012-09-10 Thread Stephen Warren
On 09/06/2012 05:06 AM, Thomas Abraham wrote:
 On 6 September 2012 15:04, Tomasz Figa t.f...@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...

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] i2c: s3c2410: add optional pin configuration using pinctrl interface

2012-09-10 Thread Linus Walleij
On Thu, Sep 6, 2012 at 2:28 PM, Tomasz Figa t.f...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] i2c: s3c2410: add optional pin configuration using pinctrl interface

2012-09-10 Thread Linus Walleij
On Thu, Sep 6, 2012 at 11:23 AM, Thomas Abraham
thomas.abra...@linaro.org wrote:

 Add optional support for i2c bus pin configuration using pinctrl interface

 Cc: Ben Dooks ben-li...@fluff.org
 Signed-off-by: Thomas Abraham thomas.abra...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] i2c: s3c2410: add optional pin configuration using pinctrl interface

2012-09-06 Thread Tomasz Figa
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,
-- 
Tomasz Figa
Samsung Poland RD 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-li...@fluff.org
 Signed-off-by: Thomas Abraham thomas.abra...@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);

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] i2c: s3c2410: add optional pin configuration using pinctrl interface

2012-09-06 Thread Thomas Abraham
On 6 September 2012 15:04, Tomasz Figa t.f...@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 RD 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-li...@fluff.org
 Signed-off-by: Thomas Abraham thomas.abra...@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);

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] i2c: s3c2410: add optional pin configuration using pinctrl interface

2012-09-06 Thread Tomasz Figa
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,
-- 
Tomasz Figa
Samsung Poland RD Center

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html