[PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready

[PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready

[Stefan Agner]

Some SoC might load the GPIO driver after the I2C driver and
using the I2C bus recovery mechanism via GPIOs. In this case
it is crucial to defer probing if the GPIO request functions
do so, otherwise the I2C driver gets loaded without recovery
mechanisms enabled.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
This is an actual issue on NXP Vybrid devices on which the GPIO
driver gets loaded rather later.

--
Stefan

 drivers/i2c/busses/i2c-imx.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 592a8f2..47fc1f1 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1009,10 +1009,13 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
 	rinfo->sda_gpio = of_get_named_gpio(pdev->dev.of_node, "sda-gpios", 0);
 	rinfo->scl_gpio = of_get_named_gpio(pdev->dev.of_node, "scl-gpios", 0);
 
-	if (!gpio_is_valid(rinfo->sda_gpio) ||
-	    !gpio_is_valid(rinfo->scl_gpio) ||
-	    IS_ERR(i2c_imx->pinctrl_pins_default) ||
-	    IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
+	if (rinfo->sda_gpio == -EPROBE_DEFER ||
+	    rinfo->scl_gpio == -EPROBE_DEFER) {
+		return -EPROBE_DEFER;
+	} else if (!gpio_is_valid(rinfo->sda_gpio) ||
+		   !gpio_is_valid(rinfo->scl_gpio) ||
+		   IS_ERR(i2c_imx->pinctrl_pins_default) ||
+		   IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
 		dev_dbg(&pdev->dev, "recovery information incomplete\n");
 		return 0;
 	}
-- 
2.10.0

Re: [PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready

[Uwe Kleine-König]

Hello Stefan,

On Mon, Sep 26, 2016 at 05:18:58PM -0700, Stefan Agner wrote:
> Some SoC might load the GPIO driver after the I2C driver and
> using the I2C bus recovery mechanism via GPIOs. In this case
> it is crucial to defer probing if the GPIO request functions
> do so, otherwise the I2C driver gets loaded without recovery
> mechanisms enabled.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready

[Leo Li]

> -----Original Message-----
> From: Stefan Agner [mailto:stefan@agner.ch]
> Sent: Monday, September 26, 2016 7:19 PM
> To: wsa@the-dreams.de
> Cc: Leo Li <leoyang.li@nxp.com>; linux-i2c@vger.kernel.org; u.kleine-
> koenig@pengutronix.de; linux-kernel@vger.kernel.org; Stefan Agner
> <stefan@agner.ch>
> Subject: [PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready
> 
> Some SoC might load the GPIO driver after the I2C driver and
> using the I2C bus recovery mechanism via GPIOs. In this case
> it is crucial to defer probing if the GPIO request functions
> do so, otherwise the I2C driver gets loaded without recovery
> mechanisms enabled.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>

Acked-by: Li Yang <leoyang.li@nxp.com>

> ---
> This is an actual issue on NXP Vybrid devices on which the GPIO
> driver gets loaded rather later.
> 
> --
> Stefan
> 
>  drivers/i2c/busses/i2c-imx.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 592a8f2..47fc1f1 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1009,10 +1009,13 @@ static int i2c_imx_init_recovery_info(struct
> imx_i2c_struct *i2c_imx,
>  	rinfo->sda_gpio = of_get_named_gpio(pdev->dev.of_node, "sda-gpios",
> 0);
>  	rinfo->scl_gpio = of_get_named_gpio(pdev->dev.of_node, "scl-gpios",
> 0);
> 
> -	if (!gpio_is_valid(rinfo->sda_gpio) ||
> -	    !gpio_is_valid(rinfo->scl_gpio) ||
> -	    IS_ERR(i2c_imx->pinctrl_pins_default) ||
> -	    IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
> +	if (rinfo->sda_gpio == -EPROBE_DEFER ||
> +	    rinfo->scl_gpio == -EPROBE_DEFER) {
> +		return -EPROBE_DEFER;
> +	} else if (!gpio_is_valid(rinfo->sda_gpio) ||
> +		   !gpio_is_valid(rinfo->scl_gpio) ||
> +		   IS_ERR(i2c_imx->pinctrl_pins_default) ||
> +		   IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
>  		dev_dbg(&pdev->dev, "recovery information incomplete\n");
>  		return 0;
>  	}
> --
> 2.10.0

Re: [PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

On Mon, Sep 26, 2016 at 05:18:58PM -0700, Stefan Agner wrote:
> Some SoC might load the GPIO driver after the I2C driver and
> using the I2C bus recovery mechanism via GPIOs. In this case
> it is crucial to defer probing if the GPIO request functions
> do so, otherwise the I2C driver gets loaded without recovery
> mechanisms enabled.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>

Added stable and applied to for-current, thanks! Please consider adding
a "Fixes: <sha-id>" next time. This would be helpful.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready

[Stefan Agner]

commit 533169d164c6b4c8571d0d48779f6ff6be593d72 upstream.

Some SoC might load the GPIO driver after the I2C driver and
using the I2C bus recovery mechanism via GPIOs. In this case
it is crucial to defer probing if the GPIO request functions
do so, otherwise the I2C driver gets loaded without recovery
mechanisms enabled.

Signed-off-by: Stefan Agner <stefan@agner.ch>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Li Yang <leoyang.li@nxp.com>
Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
Cc: stable@kernel.org
---
 drivers/i2c/busses/i2c-imx.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index d4d8536..32fae2c 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1002,10 +1002,13 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
 	rinfo->scl_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
 			"scl-gpios", 0, NULL);
 
-	if (!gpio_is_valid(rinfo->sda_gpio) ||
-	    !gpio_is_valid(rinfo->scl_gpio) ||
-	    IS_ERR(i2c_imx->pinctrl_pins_default) ||
-	    IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
+	if (rinfo->sda_gpio == -EPROBE_DEFER ||
+	    rinfo->scl_gpio == -EPROBE_DEFER) {
+		return -EPROBE_DEFER;
+	} else if (!gpio_is_valid(rinfo->sda_gpio) ||
+		   !gpio_is_valid(rinfo->scl_gpio) ||
+		   IS_ERR(i2c_imx->pinctrl_pins_default) ||
+		   IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
 		dev_dbg(&pdev->dev, "recovery information incomplete\n");
 		return;
 	}
-- 
2.10.2

Re: [PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready

[Stefan Agner]

On 2016-11-08 16:32, Stefan Agner wrote:
> commit 533169d164c6b4c8571d0d48779f6ff6be593d72 upstream.
> 
> Some SoC might load the GPIO driver after the I2C driver and
> using the I2C bus recovery mechanism via GPIOs. In this case
> it is crucial to defer probing if the GPIO request functions
> do so, otherwise the I2C driver gets loaded without recovery
> mechanisms enabled.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Acked-by: Li Yang <leoyang.li@nxp.com>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> Cc: stable@kernel.org

I forgot to mention, this should be applied to 4.4 only.

Later versions made the return value void, and it would require a larger
rework. I am mainly concerned about LTS...

--
Stefan

> ---
>  drivers/i2c/busses/i2c-imx.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index d4d8536..32fae2c 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1002,10 +1002,13 @@ static void i2c_imx_init_recovery_info(struct
> imx_i2c_struct *i2c_imx,
>  	rinfo->scl_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
>  			"scl-gpios", 0, NULL);
>  
> -	if (!gpio_is_valid(rinfo->sda_gpio) ||
> -	    !gpio_is_valid(rinfo->scl_gpio) ||
> -	    IS_ERR(i2c_imx->pinctrl_pins_default) ||
> -	    IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
> +	if (rinfo->sda_gpio == -EPROBE_DEFER ||
> +	    rinfo->scl_gpio == -EPROBE_DEFER) {
> +		return -EPROBE_DEFER;
> +	} else if (!gpio_is_valid(rinfo->sda_gpio) ||
> +		   !gpio_is_valid(rinfo->scl_gpio) ||
> +		   IS_ERR(i2c_imx->pinctrl_pins_default) ||
> +		   IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
>  		dev_dbg(&pdev->dev, "recovery information incomplete\n");
>  		return;
>  	}

Re: [PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready

[Greg KH]

On Tue, Nov 08, 2016 at 04:34:50PM -0800, Stefan Agner wrote:
> On 2016-11-08 16:32, Stefan Agner wrote:
> > commit 533169d164c6b4c8571d0d48779f6ff6be593d72 upstream.
> > 
> > Some SoC might load the GPIO driver after the I2C driver and
> > using the I2C bus recovery mechanism via GPIOs. In this case
> > it is crucial to defer probing if the GPIO request functions
> > do so, otherwise the I2C driver gets loaded without recovery
> > mechanisms enabled.
> > 
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > Acked-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>
> > Acked-by: Li Yang <leoyang.li@nxp.com>
> > Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> > Cc: stable@kernel.org
> 
> I forgot to mention, this should be applied to 4.4 only.

Did you build the code there?

> Later versions made the return value void, and it would require a larger
> rework. I am mainly concerned about LTS...

I don't see this working for either 4.4 or 4.8-stable right now, do you?

confused,

greg k-h

Re: [PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready

[Stefan Agner]

On 2016-11-08 22:11, Greg KH wrote:
> On Tue, Nov 08, 2016 at 04:34:50PM -0800, Stefan Agner wrote:
>> On 2016-11-08 16:32, Stefan Agner wrote:
>> > commit 533169d164c6b4c8571d0d48779f6ff6be593d72 upstream.
>> >
>> > Some SoC might load the GPIO driver after the I2C driver and
>> > using the I2C bus recovery mechanism via GPIOs. In this case
>> > it is crucial to defer probing if the GPIO request functions
>> > do so, otherwise the I2C driver gets loaded without recovery
>> > mechanisms enabled.
>> >
>> > Signed-off-by: Stefan Agner <stefan@agner.ch>
>> > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> > Acked-by: Li Yang <leoyang.li@nxp.com>
>> > Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
>> > Cc: stable@kernel.org
>>
>> I forgot to mention, this should be applied to 4.4 only.
> 
> Did you build the code there?

Oh, I built it on a branch where I had some other patches applied, sorry
about that.

> 
>> Later versions made the return value void, and it would require a larger
>> rework. I am mainly concerned about LTS...
> 
> I don't see this working for either 4.4 or 4.8-stable right now, do you?

No, the patch really doesn't work for 4.4 through 4.8. The patch relies
on commit fd8961c5ba9e6c ("i2c: imx: make bus recovery through pinctrl
optional"), which is not marked for stable (and probably also not
suitable actually).

The issue can only be observed on Vybrid SoC's with I2C bus recovery
enabled. The upstream device trees don't use I2C bus recovery so far. So
the patch is actually just enabling a feature we did not have so far...
Given all this circumstances, I think we should not further waste our
time and try to backport this patch for stable.

Sorry for the noise.

--
Stefan

Re: [PATCH] i2c: imx: defer probe if bus recovery GPIOs are not ready

[Greg KH]

On Tue, Nov 08, 2016 at 04:32:39PM -0800, Stefan Agner wrote:
> commit 533169d164c6b4c8571d0d48779f6ff6be593d72 upstream.
> 
> Some SoC might load the GPIO driver after the I2C driver and
> using the I2C bus recovery mechanism via GPIOs. In this case
> it is crucial to defer probing if the GPIO request functions
> do so, otherwise the I2C driver gets loaded without recovery
> mechanisms enabled.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> Acked-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>
> Acked-by: Li Yang <leoyang.li@nxp.com>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> Cc: stable@kernel.org
> ---
>  drivers/i2c/busses/i2c-imx.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index d4d8536..32fae2c 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1002,10 +1002,13 @@ static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>  	rinfo->scl_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
>  			"scl-gpios", 0, NULL);
>  
> -	if (!gpio_is_valid(rinfo->sda_gpio) ||
> -	    !gpio_is_valid(rinfo->scl_gpio) ||
> -	    IS_ERR(i2c_imx->pinctrl_pins_default) ||
> -	    IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
> +	if (rinfo->sda_gpio == -EPROBE_DEFER ||
> +	    rinfo->scl_gpio == -EPROBE_DEFER) {
> +		return -EPROBE_DEFER;

Ok, but...

> +	} else if (!gpio_is_valid(rinfo->sda_gpio) ||
> +		   !gpio_is_valid(rinfo->scl_gpio) ||
> +		   IS_ERR(i2c_imx->pinctrl_pins_default) ||
> +		   IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
>  		dev_dbg(&pdev->dev, "recovery information incomplete\n");
>  		return;

Hm.

I don't think you built this patch :(

Same goes for 4.8-stable...

greg k-h