From: <Codrin.Ciubotariu@microchip.com> To: <wsa@kernel.org> Cc: <linux-i2c@vger.kernel.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <robh+dt@kernel.org>, <Ludovic.Desroches@microchip.com>, <Nicolas.Ferre@microchip.com>, <alexandre.belloni@bootlin.com>, <linux@armlinux.org.uk>, <kamel.bouhara@bootlin.com> Subject: Re: Re: [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery Date: Mon, 3 Aug 2020 13:27:28 +0000 [thread overview] Message-ID: <084249c3-dd2b-a975-6c8d-8045def0293e@microchip.com> (raw) In-Reply-To: <20200802165446.GA10193@kunai> 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: >> >> &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. 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 = &adap->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 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. Right, sorry about this. Will fix. > >> + 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 :) It is not the same thing. Without the first check, it will return 0 if bri->recover_bus is NULL, which is not what we want. If bri->recover_bus is NULL (and the pintrl states and gpios are in place) we can set recover_bus to i2c_generic_scl_recovery and use the generic recovery. > >> + >> + /* >> + * pins might be taken as GPIO, so we might as well inform pinctrl about > > s/might as well/should/ OK > >> + * 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); I agree. I will add it. > >> + >> + /* 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. > 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. Thanks and best regards, Codrin
WARNING: multiple messages have this Message-ID (diff)
From: <Codrin.Ciubotariu@microchip.com> To: <wsa@kernel.org> Cc: devicetree@vger.kernel.org, alexandre.belloni@bootlin.com, kamel.bouhara@bootlin.com, linux-kernel@vger.kernel.org, Ludovic.Desroches@microchip.com, robh+dt@kernel.org, linux-i2c@vger.kernel.org, linux@armlinux.org.uk, linux-arm-kernel@lists.infradead.org Subject: Re: Re: [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery Date: Mon, 3 Aug 2020 13:27:28 +0000 [thread overview] Message-ID: <084249c3-dd2b-a975-6c8d-8045def0293e@microchip.com> (raw) In-Reply-To: <20200802165446.GA10193@kunai> 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: >> >> &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. 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 = &adap->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 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. Right, sorry about this. Will fix. > >> + 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 :) It is not the same thing. Without the first check, it will return 0 if bri->recover_bus is NULL, which is not what we want. If bri->recover_bus is NULL (and the pintrl states and gpios are in place) we can set recover_bus to i2c_generic_scl_recovery and use the generic recovery. > >> + >> + /* >> + * pins might be taken as GPIO, so we might as well inform pinctrl about > > s/might as well/should/ OK > >> + * 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); I agree. I will add it. > >> + >> + /* 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. > 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. Thanks and best regards, Codrin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-08-03 13:27 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-19 14:19 [RFC PATCH 0/4] i2c: core: add generic GPIO bus recovery Codrin Ciubotariu 2020-06-19 14:19 ` Codrin Ciubotariu 2020-06-19 14:19 ` [RFC PATCH 1/4] dt-binding: i2c: add generic properties for " Codrin Ciubotariu 2020-06-19 14:19 ` Codrin Ciubotariu 2020-07-05 21:19 ` Wolfram Sang 2020-07-05 21:19 ` Wolfram Sang 2020-07-24 19:39 ` Wolfram Sang 2020-07-24 19:39 ` Wolfram Sang 2020-07-24 20:52 ` Russell King - ARM Linux admin 2020-07-24 20:52 ` Russell King - ARM Linux admin 2020-07-27 10:44 ` Codrin.Ciubotariu 2020-07-27 10:44 ` Codrin.Ciubotariu 2020-07-27 10:50 ` Russell King - ARM Linux admin 2020-07-27 10:50 ` Russell King - ARM Linux admin 2020-07-30 9:00 ` Codrin.Ciubotariu 2020-07-30 9:00 ` Codrin.Ciubotariu 2020-08-03 14:16 ` Russell King - ARM Linux admin 2020-08-03 14:16 ` Russell King - ARM Linux admin 2020-08-03 16:42 ` Codrin.Ciubotariu 2020-08-03 16:42 ` Codrin.Ciubotariu 2020-07-15 19:21 ` Rob Herring 2020-07-15 19:21 ` Rob Herring 2020-06-19 14:19 ` [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery Codrin Ciubotariu 2020-06-19 14:19 ` Codrin Ciubotariu 2020-08-02 16:54 ` Wolfram Sang 2020-08-02 16:54 ` Wolfram Sang 2020-08-03 13:27 ` Codrin.Ciubotariu [this message] 2020-08-03 13:27 ` Codrin.Ciubotariu 2020-08-03 16:49 ` wsa 2020-08-03 16:49 ` wsa 2020-06-19 14:19 ` [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs Codrin Ciubotariu 2020-06-19 14:19 ` Codrin Ciubotariu 2020-08-02 17:05 ` Wolfram Sang 2020-08-02 17:05 ` Wolfram Sang 2020-08-03 15:33 ` Codrin.Ciubotariu 2020-08-03 15:33 ` Codrin.Ciubotariu 2020-08-03 16:59 ` wsa 2020-08-03 16:59 ` wsa 2020-06-19 14:19 ` [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery Codrin Ciubotariu 2020-06-19 14:19 ` Codrin Ciubotariu 2020-08-02 17:08 ` Wolfram Sang 2020-08-02 17:08 ` Wolfram Sang 2020-08-03 15:42 ` Codrin.Ciubotariu 2020-08-03 15:42 ` Codrin.Ciubotariu 2020-08-03 16:59 ` wsa 2020-08-03 16:59 ` wsa 2020-08-26 6:14 ` Wolfram Sang 2020-08-26 6:14 ` Wolfram Sang 2020-09-04 8:55 ` Codrin.Ciubotariu 2020-09-04 8:55 ` Codrin.Ciubotariu 2020-09-04 9:20 ` Wolfram Sang 2020-09-04 9:20 ` Wolfram Sang 2020-07-05 21:09 ` [RFC PATCH 0/4] i2c: core: add " Wolfram Sang 2020-07-05 21:09 ` Wolfram Sang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=084249c3-dd2b-a975-6c8d-8045def0293e@microchip.com \ --to=codrin.ciubotariu@microchip.com \ --cc=Ludovic.Desroches@microchip.com \ --cc=Nicolas.Ferre@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=devicetree@vger.kernel.org \ --cc=kamel.bouhara@bootlin.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=robh+dt@kernel.org \ --cc=wsa@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.