Re: Re: [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery
> I agree will all your suggestions, except with the removal of if > (bri->recover_bus) . If you agree with this, I can send the next version > and remove the RFC. Yes, sure. That was a brown paper bag thingie from my side. Please go ahead! signature.asc Description: PGP signature
Re: Re: [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery
On 02.08.2020 19:54, Wolfram Sang wrote: > 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: >> >> { >> ... >> pinctrl-names = "default", "gpio"; >> // or pinctrl-names = "default", "recovery"; >> pinctrl-0 = <_i2c_default>; >> pinctrl-1 = <_i2c_gpio>; >> sda-gpios = < 0 GPIO_ACTIVE_HIGH>; >> scl-gpios = < 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 > > 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 >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -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. Agree > >> 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 = >dev; >> +struct pinctrl *p = bri->pinctrl; >> + >> +/* >> + * we can't change states without pinctrl, so remove the states if >> + * available > > s/available/populated/ ? OK > >> + */ >> +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. since default state is missing, the next check can be skipped, but I agree that removing the label makes things easier to read. > >> +} >> +} >> +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... right > >> +} >> +} >> + >> +cleanup_pinctrl: > > ... and this label can go then. Also nicer to read, I'd say. I will remove it. > >> +/* 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
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: > > { > ... > pinctrl-names = "default", "gpio"; > // or pinctrl-names = "default", "recovery"; > pinctrl-0 = <_i2c_default>; > pinctrl-1 = <_i2c_gpio>; > sda-gpios = < 0 GPIO_ACTIVE_HIGH>; > scl-gpios = < 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 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 > #include > #include > +#include > #include > #include > #include > @@ -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 = >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 = >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
[RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery
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: { ... pinctrl-names = "default", "gpio"; // or pinctrl-names = "default", "recovery"; pinctrl-0 = <_i2c_default>; pinctrl-1 = <_i2c_gpio>; sda-gpios = < 0 GPIO_ACTIVE_HIGH>; scl-gpios = < 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 --- 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 #include #include +#include #include #include #include @@ -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); 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); 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 = >dev; + struct pinctrl *p = bri->pinctrl; + + /* +* we can't change states without pinctrl, so remove the states if +* available +*/ + 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; + } + } + 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; + } + } + +cleanup_pinctrl: + /* 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 = >dev; + struct gpio_desc *gpiod; + int ret = 0; + + /* don't touch the recovery information if the driver is not using +* generic SCL recovery +*/ + if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery) + return 0; + + /* +* pins might be taken as GPIO, so we might as well inform pinctrl about +* 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; +