Messages in this thread | | | Date | Sun, 2 Aug 2020 18:54:46 +0200 | From | Wolfram Sang <> | Subject | Re: [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery |
| |
On Fri, Jun 19, 2020 at 05:19:02PM +0300, Codrin Ciubotariu wrote: > Multiple I2C bus drivers use similar bindings to obtain information needed > for I2C recovery. For example, for platforms using device-tree, the > properties look something like this: > > &i2c { > ... > pinctrl-names = "default", "gpio"; > // or pinctrl-names = "default", "recovery"; > pinctrl-0 = <&pinctrl_i2c_default>; > pinctrl-1 = <&pinctrl_i2c_gpio>; > sda-gpios = <&pio 0 GPIO_ACTIVE_HIGH>; > scl-gpios = <&pio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; > ... > } > > For this reason, we can add this common initialization in the core. This > way, other I2C bus drivers will be able to support GPIO recovery just by > providing a pointer to platform's pinctrl and calling i2c_recover_bus() > when SDA is stuck low. > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
Thanks, it looks a lot like what I had in mind!
> --- > drivers/i2c/i2c-core-base.c | 119 ++++++++++++++++++++++++++++++++++++ > include/linux/i2c.h | 11 ++++ > 2 files changed, 130 insertions(+) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index d1f278f73011..4ee29fec4e93 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -32,6 +32,7 @@ > #include <linux/of_device.h> > #include <linux/of.h> > #include <linux/of_irq.h> > +#include <linux/pinctrl/consumer.h> > #include <linux/pm_domain.h> > #include <linux/pm_runtime.h> > #include <linux/pm_wakeirq.h> > @@ -179,6 +180,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) > struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; > int i = 0, scl = 1, ret = 0; > > + if (bri->pinctrl) > + pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
I think this should come after 'prepare_recovery'. It may be that 'prepare_recovery' already needs to select the pinctrl state to avoid a glitch. In this version, there would be a glitch then. If we move it down, the doubled 'pinctrl_select_state' would be a noop then.
> if (bri->prepare_recovery) > bri->prepare_recovery(adap); > > @@ -236,6 +239,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) > > if (bri->unprepare_recovery) > bri->unprepare_recovery(adap); > + if (bri->pinctrl) > + pinctrl_select_state(bri->pinctrl, bri->pins_default);
Here it is OK and will still work with the PXA version which needs to select the state on its own.
> > return ret; > } > @@ -251,6 +256,118 @@ int i2c_recover_bus(struct i2c_adapter *adap) > } > EXPORT_SYMBOL_GPL(i2c_recover_bus); > > +static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap) > +{ > + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; > + struct device *dev = &adap->dev; > + struct pinctrl *p = bri->pinctrl; > + > + /* > + * we can't change states without pinctrl, so remove the states if > + * available
s/available/populated/ ?
> + */ > + if (!p) { > + bri->pins_default = NULL; > + bri->pins_gpio = NULL; > + return; > + } > + > + if (!bri->pins_default) { > + bri->pins_default = pinctrl_lookup_state(p, > + PINCTRL_STATE_DEFAULT); > + if (IS_ERR(bri->pins_default)) { > + dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found for GPIO recovery\n"); > + bri->pins_default = NULL; > + > + goto cleanup_pinctrl;
I'd leave out the goto here. It is OK to check both parameters, I think.
> + } > + } > + if (!bri->pins_gpio) { > + bri->pins_gpio = pinctrl_lookup_state(p, "gpio"); > + if (IS_ERR(bri->pins_gpio)) > + bri->pins_gpio = pinctrl_lookup_state(p, "recovery"); > + > + if (IS_ERR(bri->pins_gpio)) { > + dev_dbg(dev, "no gpio or recovery state found for GPIO recovery\n"); > + bri->pins_gpio = NULL; > + > + goto cleanup_pinctrl;
This goto is not needed...
> + } > + } > + > +cleanup_pinctrl:
... and this label can go then. Also nicer to read, I'd say.
> + /* for pinctrl state changes, we need all the information */ > + if (!bri->pins_default || !bri->pins_gpio) { > + bri->pinctrl = NULL; > + bri->pins_default = NULL; > + bri->pins_gpio = NULL; > + } else { > + dev_info(dev, "using pinctrl states for GPIO recovery"); > + } > +} > + > +static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap) > +{ > + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; > + struct device *dev = &adap->dev; > + struct gpio_desc *gpiod; > + int ret = 0; > + > + /* don't touch the recovery information if the driver is not using > + * generic SCL recovery > + */
Not kernel comment style.
> + if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery) > + return 0;
No need for the first condition. 'i2c_generic_scl_recovery' is definately not NULL :)
> + > + /* > + * pins might be taken as GPIO, so we might as well inform pinctrl about
s/might as well/should/
> + * this and move the state to GPIO > + */ > + if (bri->pinctrl) > + pinctrl_select_state(bri->pinctrl, bri->pins_gpio); > + > + /* > + * if there is incomplete or no recovery information, see if generic > + * GPIO recovery is available > + */ > + if (!bri->scl_gpiod) { > + gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN); > + if (PTR_ERR(gpiod) == -EPROBE_DEFER) { > + ret = -EPROBE_DEFER; > + goto cleanup_pinctrl_state; > + } > + if (!IS_ERR(gpiod)) { > + bri->scl_gpiod = gpiod; > + bri->recover_bus = i2c_generic_scl_recovery; > + dev_info(dev, "using generic GPIOs for recovery\n"); > + } > + }
I think this extra code from the PXA driver makes sense in case SDA was released while we were executing this code:
1383 /* 1384 * We have SCL. Pull SCL low and wait a bit so that SDA glitches 1385 * have no effect. 1386 */ 1387 gpiod_direction_output(bri->scl_gpiod, 0); 1388 udelay(10); 1389 bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN); 1390 1391 /* Wait a bit in case of a SDA glitch, and then release SCL. */ 1392 udelay(10); 1393 gpiod_direction_output(bri->scl_gpiod, 1);
> + > + /* SDA GPIOD line is optional, so we care about DEFER only */ > + if (!bri->sda_gpiod) { > + gpiod = devm_gpiod_get(dev, "sda", GPIOD_IN); > + if (PTR_ERR(gpiod) == -EPROBE_DEFER) { > + ret = -EPROBE_DEFER; > + goto cleanup_pinctrl_state; > + } > + if (!IS_ERR(gpiod)) > + bri->sda_gpiod = gpiod; > + } > + > +cleanup_pinctrl_state: > + /* change the state of the pins back to their default state */ > + if (bri->pinctrl) > + pinctrl_select_state(bri->pinctrl, bri->pins_default); > + > + return ret; > +} > +
Rest looks good! If you have some time for this now, I will make sure to get it into 5.9. With these minor things fixed, this is good to go, me thinks.
[unhandled content-type:application/pgp-signature] | |