| Message ID | 20200415070643.23663-1-codrin.ciubotariu@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery | expand |
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 >
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.
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
> > 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 :)
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
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;
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(-)