[RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery

[RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery

[Codrin Ciubotariu]

devm_gpiod_get() usually calls gpio_request_enable() for non-strict pinmux
drivers. These puts the pins in GPIO mode, whithout notifying the pinctrl
driver. At this point, the I2C bus no longer owns the pins. To mux the
pins back to the I2C bus, we use the pinctrl driver to change the state
of the pins to GPIO, before using devm_gpiod_get(). After the pins are
received as GPIOs, we switch theer pinctrl state back to the default
one,

Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery")
Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 drivers/i2c/busses/i2c-at91-master.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index 0aba51a7df32..43d85845c897 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -845,6 +845,18 @@ static int at91_init_twi_recovery_info(struct platform_device *pdev,
 							 PINCTRL_STATE_DEFAULT);
 	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
 						      "gpio");
+	if (IS_ERR(dev->pinctrl_pins_default) ||
+	    IS_ERR(dev->pinctrl_pins_gpio)) {
+		dev_info(&pdev->dev, "pinctrl states incomplete for recovery\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * pins will be taken as GPIO, so we might as well inform pinctrl about
+	 * this and move the state to GPIO
+	 */
+	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
+
 	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
 	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
@@ -855,9 +867,7 @@ static int at91_init_twi_recovery_info(struct platform_device *pdev,
 		return -EPROBE_DEFER;
 
 	if (IS_ERR(rinfo->sda_gpiod) ||
-	    IS_ERR(rinfo->scl_gpiod) ||
-	    IS_ERR(dev->pinctrl_pins_default) ||
-	    IS_ERR(dev->pinctrl_pins_gpio)) {
+	    IS_ERR(rinfo->scl_gpiod)) {
 		dev_info(&pdev->dev, "recovery information incomplete\n");
 		if (!IS_ERR(rinfo->sda_gpiod)) {
 			gpiod_put(rinfo->sda_gpiod);
@@ -870,6 +880,9 @@ static int at91_init_twi_recovery_info(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
+	/* change the state of the pins back to their default state */
+	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
+
 	dev_info(&pdev->dev, "using scl, sda for recovery\n");
 
 	rinfo->prepare_recovery = at91_prepare_twi_recovery;
-- 
2.20.1

Re: [RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery

[ludovic.desroches]

On Wed, Apr 15, 2020 at 10:06:43AM +0300, Codrin Ciubotariu wrote:
> devm_gpiod_get() usually calls gpio_request_enable() for non-strict pinmux
> drivers. These puts the pins in GPIO mode, whithout notifying the pinctrl
> driver. At this point, the I2C bus no longer owns the pins. To mux the
> pins back to the I2C bus, we use the pinctrl driver to change the state
> of the pins to GPIO, before using devm_gpiod_get(). After the pins are
> received as GPIOs, we switch theer pinctrl state back to the default
> one,
> 
> Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery")
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

At the moment, I don't see another way to deal with this issue.

Link to the discussion:
https://lore.kernel.org/linux-arm-kernel/20191206173343.GX25745@shell.armlinux.org.uk/

Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

> ---
>  drivers/i2c/busses/i2c-at91-master.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index 0aba51a7df32..43d85845c897 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -845,6 +845,18 @@ static int at91_init_twi_recovery_info(struct platform_device *pdev,
>  							 PINCTRL_STATE_DEFAULT);
>  	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
>  						      "gpio");
> +	if (IS_ERR(dev->pinctrl_pins_default) ||
> +	    IS_ERR(dev->pinctrl_pins_gpio)) {
> +		dev_info(&pdev->dev, "pinctrl states incomplete for recovery\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * pins will be taken as GPIO, so we might as well inform pinctrl about
> +	 * this and move the state to GPIO
> +	 */
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> +
>  	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
>  	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
>  		return -EPROBE_DEFER;
> @@ -855,9 +867,7 @@ static int at91_init_twi_recovery_info(struct platform_device *pdev,
>  		return -EPROBE_DEFER;
>  
>  	if (IS_ERR(rinfo->sda_gpiod) ||
> -	    IS_ERR(rinfo->scl_gpiod) ||
> -	    IS_ERR(dev->pinctrl_pins_default) ||
> -	    IS_ERR(dev->pinctrl_pins_gpio)) {
> +	    IS_ERR(rinfo->scl_gpiod)) {
>  		dev_info(&pdev->dev, "recovery information incomplete\n");
>  		if (!IS_ERR(rinfo->sda_gpiod)) {
>  			gpiod_put(rinfo->sda_gpiod);
> @@ -870,6 +880,9 @@ static int at91_init_twi_recovery_info(struct platform_device *pdev,
>  		return -EINVAL;
>  	}
>  
> +	/* change the state of the pins back to their default state */
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> +
>  	dev_info(&pdev->dev, "using scl, sda for recovery\n");
>  
>  	rinfo->prepare_recovery = at91_prepare_twi_recovery;
> -- 
> 2.20.1
> 

Re: [RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery

[Wolfram Sang]

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

On Wed, Apr 15, 2020 at 10:06:43AM +0300, Codrin Ciubotariu wrote:
> devm_gpiod_get() usually calls gpio_request_enable() for non-strict pinmux
> drivers. These puts the pins in GPIO mode, whithout notifying the pinctrl
> driver. At this point, the I2C bus no longer owns the pins. To mux the
> pins back to the I2C bus, we use the pinctrl driver to change the state
> of the pins to GPIO, before using devm_gpiod_get(). After the pins are
> received as GPIOs, we switch theer pinctrl state back to the default
> one,
> 
> Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery")
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Applied to for-current, thanks!

This will do for 5.7. For 5.8 or 5.9, I can imagine to take the two
pinctrl_state pointers into bus_recovery_info and handle all this in the
core. I will try this later this week if noone is super-eager to try it
out before.


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

Re: Re: [RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery

[Codrin.Ciubotariu]

On 05.05.2020 18:12, Wolfram Sang wrote:
> On Wed, Apr 15, 2020 at 10:06:43AM +0300, Codrin Ciubotariu wrote:
>> devm_gpiod_get() usually calls gpio_request_enable() for non-strict pinmux
>> drivers. These puts the pins in GPIO mode, whithout notifying the pinctrl
>> driver. At this point, the I2C bus no longer owns the pins. To mux the
>> pins back to the I2C bus, we use the pinctrl driver to change the state
>> of the pins to GPIO, before using devm_gpiod_get(). After the pins are
>> received as GPIOs, we switch theer pinctrl state back to the default
>> one,
>>
>> Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery")
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> 
> Applied to for-current, thanks!

Just looked at this patch and noticed that I should change the pinctrl 
state back to default if we can't get the gpio pins. I will submit a 
patch to fix this.

> 
> This will do for 5.7. For 5.8 or 5.9, I can imagine to take the two
> pinctrl_state pointers into bus_recovery_info and handle all this in the
> core. I will try this later this week if noone is super-eager to try it
> out before.
> 

By 'all this' you mean to move the entire function in the core, right? 
Having just these two pointers bus_recinovery_info won't help much. I 
can try it, if you haven't already started...

Best regards,
Codrin

Re: Re: [RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery

[Wolfram Sang]

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


> > This will do for 5.7. For 5.8 or 5.9, I can imagine to take the two
> > pinctrl_state pointers into bus_recovery_info and handle all this in the
> > core. I will try this later this week if noone is super-eager to try it
> > out before.
> > 
> 
> By 'all this' you mean to move the entire function in the core, right? 
> Having just these two pointers bus_recinovery_info won't help much. I 
> can try it, if you haven't already started...

I mean to add those two pointers to bus_recinovery_info and if they are
populated, then the I2C core is doing the necessary magic (or maybe just
the pinctrl handle and assume the states have fixed names?). Russell
just sent patches to add it to the PXA driver, so we could now double
check how much could be factored out.

I haven't started yet, let's keep in touch who started first :)


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

Re: Re: Re: [RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery

[Codrin.Ciubotariu]

On 20.05.2020 19:27, Wolfram Sang wrote:
> 
>>> This will do for 5.7. For 5.8 or 5.9, I can imagine to take the two
>>> pinctrl_state pointers into bus_recovery_info and handle all this in the
>>> core. I will try this later this week if noone is super-eager to try it
>>> out before.
>>>
>>
>> By 'all this' you mean to move the entire function in the core, right?
>> Having just these two pointers bus_recinovery_info won't help much. I
>> can try it, if you haven't already started...
> 
> I mean to add those two pointers to bus_recinovery_info and if they are
> populated, then the I2C core is doing the necessary magic (or maybe just
> the pinctrl handle and assume the states have fixed names?). Russell
> just sent patches to add it to the PXA driver, so we could now double
> check how much could be factored out.
> 
> I haven't started yet, let's keep in touch who started first :)
> 

I started working at this. I added the pinctrl state initialization at 
the beginning of the i2c_init_recovery(). Due to the pinmux state issue 
with the GPIOs, the GPIO part needs to be also moved. The problem I ran 
in to now is the fact that, even if we can ignore if the GPIOs are not 
available, we should at least treat EPROBE_DEFER error. To do this, the 
I2C bus drivers should take into account the fact that 
i2c_register_adapter() can return -EPROBE_DEFER. Is this something to 
consider?

Thanks and best regards,
Codrin