i2c: s3c2410: allow pin pin configuration using pinctrl

PATCH New Thomas Abraham 4 participants 14 messages Sep 6, 2012 – Sep 17, 2012

Subsystems (3)

Device tree bindings Arm Samsung Exynos I2C subsystem host drivers

lore

Share & tools

lore

Thomas Abraham

author

2012-09-06 09:04 UTC #0 lore raw

cover i2c: s3c2410: allow pin pin configuration using pinctrl

This patch series adds support for pinctrl interface based i2c-bus configuration for the s3c2410 i2c controller driver.

The second patch would have conflict with Exynos4 DTS reorganization patches posted by Tomasz Figa. If those patches are applied prior to this patch, I will rebase this patch and resubmit.

Thomas Abraham (2):
i2c: s3c2410: add optional pin configuration using pinctrl interface
ARM: dts: exynos4: allow i2c0 bus to be configured using pinctrl interface

 arch/arm/boot/dts/exynos4210-smdkv310.dts |    2 --
 arch/arm/boot/dts/exynos4210.dtsi         |    2 ++
 drivers/i2c/busses/i2c-s3c2410.c          |   19 ++++++++++++++++---
 3 files changed, 18 insertions(+), 5 deletions(-)

Thomas Abraham

author

2012-09-06 09:05 UTC #1 lore raw

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

Add optional support for i2c bus pin configuration using pinctrl interface

Cc Ben Dooks <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

---
 drivers/i2c/busses/i2c-s3c2410.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-s3c2410.c +16 -3

index 5ae3b02..f4b2d6f 100644
@@ -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);

Signature · 1 line

1.6.6.rc2

Tomasz Figa

2012-09-06 09:34 UTC #2 lore raw

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

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,

Signature · 2 lines

Tomasz Figa
Samsung Poland R&D Center

Quoted 63 lines · 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 <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

---
 drivers/i2c/busses/i2c-s3c2410.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

(quoted diff) +0 -0

b/drivers/i2c/busses/i2c-s3c2410.c index 5ae3b02..f4b2d6f 100644

drivers/i2c/busses/i2c-s3c2410.c +16 -3

@@ -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);

Thomas Abraham

author

2012-09-06 11:06 UTC #3 lore raw

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

Quoted 8 lines · 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?

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.

Quoted 5 lines

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.

Quoted 71 lines

Best regards,
--
Tomasz Figa
Samsung Poland R&D Center

Quoted 63 lines · 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 <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

---
 drivers/i2c/busses/i2c-s3c2410.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

(quoted diff) +0 -0

b/drivers/i2c/busses/i2c-s3c2410.c index 5ae3b02..f4b2d6f 100644

drivers/i2c/busses/i2c-s3c2410.c +16 -3

@@ -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);

Tomasz Figa

2012-09-06 12:28 UTC #4 lore raw

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

Hi,

Thanks for your comments.

Quoted 9 lines · On Thursday 06 of September 2012 16:36:08 Thomas Abraham wrote:

Quoted 6 lines
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.

Quoted 8 lines

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

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 3 lines

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,

Signature · 2 lines

Tomasz Figa
Samsung Poland R&D Center

Linus Walleij

2012-09-10 19:55 UTC #5 lore raw

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

Quoted 10 lines · Tomasz Figa

Thanks for your comments.
Quoted 7 lines · On Thursday 06 of September 2012 16:36:08 Thomas Abraham wrote:
Quoted 6 lines
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?

Quoted 6 lines

Quoted 2 lines
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?

Quoted 3 lines

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.

Quoted 6 lines

Quoted 3 lines
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

Stephen Warren

2012-09-10 19:21 UTC #6 lore raw

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

In reply to Thomas Abraham

Quoted 16 lines · Thomas Abraham

Quoted 8 lines · 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?
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

Quoted 3 lines

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.

Quoted 1 line

2. Runtime muxing/config is possible.

The "client" driver would definitely have to be involved there, I agree.

Quoted 3 lines

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.

Quoted 10 lines

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.
Quoted 5 lines
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.

Quoted 9 lines

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

Thomas Abraham

author

2012-09-17 07:28 UTC #7 lore raw

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

Quoted 17 lines · Stephen Warren

Quoted 16 lines · Thomas Abraham
Quoted 8 lines · 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?
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

Quoted 6 lines

Quoted 3 lines
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.

[...]

Linus Walleij

2012-09-10 20:02 UTC #8 lore raw

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

In reply to Thomas Abraham

Quoted 4 lines · Thomas Abraham

Add optional support for i2c bus pin configuration using pinctrl interface
Cc Ben Dooks <[email protected]>
Signed-off-by Thomas Abraham <[email protected]>

(...)

Quoted 1 line

drivers/i2c/busses/i2c-s3c2410.c +0 -0

(...)

drivers/i2c/busses/i2c-s3c2410.c static int s3c24xx_i2c_init(struct s3c24xx_i2c *i2c) · line 868

Quoted 4 lines

drivers/i2c/busses/i2c-s3c2410.c +2 -2

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

Quoted 11 lines

drivers/i2c/busses/i2c-s3c2410.c +6 -0

+               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 */

drivers/i2c/busses/i2c-s3c2410.c static int s3c24xx_i2c_probe(struct platform_device *pdev) · line 1028

Quoted 7 lines

drivers/i2c/busses/i2c-s3c2410.c +4 -0

@@ -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

Thomas Abraham

author

2012-09-17 07:52 UTC #9 lore raw

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

Quoted 15 lines · Linus Walleij

Quoted 4 lines · Thomas Abraham
Add optional support for i2c bus pin configuration using pinctrl interface
Cc Ben Dooks <[email protected]>
Signed-off-by Thomas Abraham <[email protected]>
(...)
Quoted 1 line
drivers/i2c/busses/i2c-s3c2410.c +0 -0
(...)
drivers/i2c/busses/i2c-s3c2410.c static int s3c24xx_i2c_init(struct s3c24xx_i2c *i2c) · line 868
Quoted 4 lines
drivers/i2c/busses/i2c-s3c2410.c +2 -2
-       else
-               if (s3c24xx_i2c_parse_dt_gpio(i2c))
+       } else if (!IS_ERR_OR_NULL(i2c->pctrl) &&
+                       !IS_ERR_OR_NULL(i2c->pctrl_state)) {

Quoted 4 lines

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.

Quoted 15 lines

drivers/i2c/busses/i2c-s3c2410.c +6 -0

+               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;

Quoted 8 lines

drivers/i2c/busses/i2c-s3c2410.c static int s3c24xx_i2c_probe(struct platform_device *pdev) · line 1028
Quoted 4 lines
drivers/i2c/busses/i2c-s3c2410.c +4 -0
+       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.

Quoted 3 lines

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.

Quoted 3 lines

Yours,
Linus Walleij

Thanks,
Thomas.

Thomas Abraham

author

2012-09-06 09:23 UTC #10 lore raw

PATCH 2/2 ARM: dts: exynos4: allow i2c0 bus to be configured using pinctrl interface

In reply to Thomas Abraham

Add a default pin state for i2c0 controller which the pinctrl interface can use to setup the i2c0 bus.

Signed-off-by Thomas Abraham <[email protected]>

---
 arch/arm/boot/dts/exynos4210-smdkv310.dts |    2 --
 arch/arm/boot/dts/exynos4210.dtsi         |    2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/boot/dts/exynos4210-smdkv310.dts +0 -2

index 1beccc8..ea76542 100644
@@ -126,8 +126,6 @@
 		#size-cells = <0>;
 		samsung,i2c-sda-delay = <100>;
 		samsung,i2c-max-bus-freq = <20000>;
-		gpios = <&gpd1 0 2 3 0>,
-			<&gpd1 1 2 3 0>;
 
 		eeprom at 50 {
 			compatible = "samsung,24ad0xd1";

arch/arm/boot/dts/exynos4210.dtsi +2 -0

index a4bd0c9..e08387f 100644
@@ -157,6 +157,8 @@
 		compatible = "samsung,s3c2440-i2c";
 		reg = <0x13860000 0x100>;
 		interrupts = <0 58 0>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&i2c0_bus>;
 	};
 
 	i2c at 13870000 {

Signature · 1 line

1.6.6.rc2

Tomasz Figa

2012-09-06 10:13 UTC #11 lore raw

PATCH 2/2 ARM: dts: exynos4: allow i2c0 bus to be configured using pinctrl interface

Hi Thomas,

arch/arm/boot/dts/exynos4210.dtsi line 161

Quoted 5 lines · On Thursday 06 of September 2012 14:53:01 Thomas Abraham wrote:

arch/arm/boot/dts/exynos4210.dtsi +2 -0

 		compatible = "samsung,s3c2440-i2c";
 		reg = <0x13860000 0x100>;
 		interrupts = <0 58 0>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&i2c0_bus>;

If pinctrl-names property is omitted then the state index is used as a name (e.g. pinctrl-0 would be named "0"). Maybe it would be better to use this approach (with respective adjustment in first patch)? What do you think?

Best regards,

Signature · 2 lines

Tomasz Figa
Samsung Poland R&D Center

Thomas Abraham

author

2012-09-06 11:14 UTC #12 lore raw

PATCH 2/2 ARM: dts: exynos4: allow i2c0 bus to be configured using pinctrl interface

Quoted 9 lines · Tomasz Figa

Hi Thomas,
arch/arm/boot/dts/exynos4210.dtsi line 161
Quoted 5 lines · On Thursday 06 of September 2012 14:53:01 Thomas Abraham wrote:
              compatible = "samsung,s3c2440-i2c";
              reg = <0x13860000 0x100>;
              interrupts = <0 58 0>;
arch/arm/boot/dts/exynos4210.dtsi +2 -0
+             pinctrl-names = "default";
+             pinctrl-0 = <&i2c0_bus>;

Quoted 3 lines

If pinctrl-names property is omitted then the state index is used as a name (e.g. pinctrl-0 would be named "0"). Maybe it would be better to use this approach (with respective adjustment in first patch)? What do you think?

I tend to prefer to name the states because it is easier to cross-reference code and dts files. i2c was a simple one, but for mmc controllers, there will 1-bit state, 4-bit state and 8-bit state, and it will be nicer to name then accordingly. So I prefer to use names but if there is wider consensus on not using names, we can drop names.

Thanks,
Thomas.

Quoted 6 lines

Best regards,
--
Tomasz Figa
Samsung Poland R&D Center

Stephen Warren

2012-09-10 19:23 UTC #13 lore raw

PATCH 2/2 ARM: dts: exynos4: allow i2c0 bus to be configured using pinctrl interface

Quoted 14 lines · Thomas Abraham

Quoted 9 lines · Tomasz Figa
Hi Thomas,
arch/arm/boot/dts/exynos4210.dtsi line 161
Quoted 5 lines · On Thursday 06 of September 2012 14:53:01 Thomas Abraham wrote:
              compatible = "samsung,s3c2440-i2c";
              reg = <0x13860000 0x100>;
              interrupts = <0 58 0>;
arch/arm/boot/dts/exynos4210.dtsi +2 -0
+             pinctrl-names = "default";
+             pinctrl-0 = <&i2c0_bus>;
Quoted 3 lines
If pinctrl-names property is omitted then the state index is used as a name (e.g. pinctrl-0 would be named "0"). Maybe it would be better to use this approach (with respective adjustment in first patch)? What do you think?

Quoted 5 lines

I tend to prefer to name the states because it is easier to cross-reference code and dts files. i2c was a simple one, but for mmc controllers, there will 1-bit state, 4-bit state and 8-bit state, and it will be nicer to name then accordingly. So I prefer to use names but if there is wider consensus on not using names, we can drop names.

I would only expect to see multiple states defined in a single board .dts file /if/ runtime muxing is required. Given MMC doesn't runtime mux, I wouldn't expect there to be multiple states for different bus widths; it's just that the "default" state would point at whatever single configuration is appropriate for the board.