[RFC] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery
diff mbox series

Message ID 20200415070643.23663-1-codrin.ciubotariu@microchip.com
State In Next
Commit a53acc7ebf272d195bf23a79721d5e18cf1025dc
Headers show
Series
  • [RFC] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery
Related show

Commit Message

Codrin Ciubotariu April 15, 2020, 7:06 a.m. UTC
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(-)

Comments

Ludovic Desroches April 19, 2020, 12:50 p.m. UTC | #1
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
>
Wolfram Sang May 5, 2020, 3:12 p.m. UTC | #2
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.
Codrin Ciubotariu May 13, 2020, 11:07 a.m. UTC | #3
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
Wolfram Sang May 20, 2020, 4:27 p.m. UTC | #4
> > 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 :)
Codrin Ciubotariu June 9, 2020, 11:43 a.m. UTC | #5
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

Patch
diff mbox series

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;