[PATCH 0/2] i2c: s3c2410: allow pin pin configuration using pinctrl

[PATCH 0/2] i2c: s3c2410: allow pin pin configuration using pinctrl

[Thomas Abraham]

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

[PATCH 0/2] i2c: s3c2410: allow pin pin configuration using pinctrl

[Thomas Abraham]

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

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

[Thomas Abraham]

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);
-- 
1.6.6.rc2

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

[Thomas Abraham]

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);
-- 
1.6.6.rc2

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

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

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

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

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

[Thomas Abraham]

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

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

[Thomas Abraham]

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

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

[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 R&D Center

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

[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 R&D Center

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

[Linus Walleij]

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

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

[Linus Walleij]

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

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

[Stephen Warren]

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

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

[Stephen Warren]

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

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

[Thomas Abraham]

On 11 September 2012 00:51, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 09/06/2012 05:06 AM, Thomas Abraham wrote:
>> On 6 September 2012 15:04, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 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.

[...]

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

[Thomas Abraham]

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.

[...]

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

[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

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

[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

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

[Thomas Abraham]

On 11 September 2012 01:32, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Sep 6, 2012 at 11:23 AM, Thomas Abraham
> <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> Add optional support for i2c bus pin configuration using pinctrl interface
>>
>> Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
>> Signed-off-by: Thomas Abraham <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.

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

[Thomas Abraham]

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.

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

[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 <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/exynos4210-smdkv310.dts |    2 --
 arch/arm/boot/dts/exynos4210.dtsi         |    2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-smdkv310.dts b/arch/arm/boot/dts/exynos4210-smdkv310.dts
index 1beccc8..ea76542 100644
--- a/arch/arm/boot/dts/exynos4210-smdkv310.dts
+++ b/arch/arm/boot/dts/exynos4210-smdkv310.dts
@@ -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@50 {
 			compatible = "samsung,24ad0xd1";
diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index a4bd0c9..e08387f 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -157,6 +157,8 @@
 		compatible = "samsung,s3c2440-i2c";
 		reg = <0x13860000 0x100>;
 		interrupts = <0 58 0>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&i2c0_bus>;
 	};
 
 	i2c@13870000 {
-- 
1.6.6.rc2

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

[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 <thomas.abraham@linaro.org>
---
 arch/arm/boot/dts/exynos4210-smdkv310.dts |    2 --
 arch/arm/boot/dts/exynos4210.dtsi         |    2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-smdkv310.dts b/arch/arm/boot/dts/exynos4210-smdkv310.dts
index 1beccc8..ea76542 100644
--- a/arch/arm/boot/dts/exynos4210-smdkv310.dts
+++ b/arch/arm/boot/dts/exynos4210-smdkv310.dts
@@ -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";
diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index a4bd0c9..e08387f 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -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 {
-- 
1.6.6.rc2

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

[Tomasz Figa]

Hi Thomas,

On Thursday 06 of September 2012 14:53:01 Thomas Abraham wrote:
>  		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,
-- 
Tomasz Figa
Samsung Poland R&D Center

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

[Tomasz Figa]

Hi Thomas,

On Thursday 06 of September 2012 14:53:01 Thomas Abraham wrote:
>  		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,
-- 
Tomasz Figa
Samsung Poland R&D Center

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

[Thomas Abraham]

On 6 September 2012 15:43, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Thomas,
>
> On Thursday 06 of September 2012 14:53:01 Thomas Abraham wrote:
>>               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?

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.

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

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

[Thomas Abraham]

On 6 September 2012 15:43, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Thomas,
>
> On Thursday 06 of September 2012 14:53:01 Thomas Abraham wrote:
>>               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?

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.

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

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

[Stephen Warren]

On 09/06/2012 05:14 AM, Thomas Abraham wrote:
> On 6 September 2012 15:43, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Thomas,
>>
>> On Thursday 06 of September 2012 14:53:01 Thomas Abraham wrote:
>>>               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?
> 
> 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.

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

[Stephen Warren]

On 09/06/2012 05:14 AM, Thomas Abraham wrote:
> On 6 September 2012 15:43, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Thomas,
>>
>> On Thursday 06 of September 2012 14:53:01 Thomas Abraham wrote:
>>>               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?
> 
> 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.