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 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs Date: Mon, 3 Aug 2020 15:33:41 +0000 [thread overview] Message-ID: <15a449fa-d649-846a-e6f2-1540f9581846@microchip.com> (raw) In-Reply-To: <20200802170500.GB10193@kunai> On 02.08.2020 20:05, Wolfram Sang wrote: > On Fri, Jun 19, 2020 at 05:19:03PM +0300, Codrin Ciubotariu wrote: >> Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return >> -EPROBE_DEFER, so we should at least treat that. This ends up with >> i2c_register_adapter() to be able to return -EPROBE_DEFER. >> >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> --- >> drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++------ >> 1 file changed, 16 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c >> index 4ee29fec4e93..f8d9f2048ca8 100644 >> --- a/drivers/i2c/i2c-core-base.c >> +++ b/drivers/i2c/i2c-core-base.c >> @@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter *adap) >> return i2c_gpio_init_generic_recovery(adap); >> } >> >> -static void i2c_init_recovery(struct i2c_adapter *adap) >> +static int i2c_init_recovery(struct i2c_adapter *adap) >> { >> struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; >> char *err_str; >> >> if (!bri) >> - return; >> + return 0; >> >> - i2c_gpio_init_recovery(adap); >> + if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> >> if (!bri->recover_bus) { >> err_str = "no recover_bus() found"; >> @@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap) >> if (gpiod_get_direction(bri->sda_gpiod) == 0) >> bri->set_sda = set_sda_gpio_value; >> } >> - return; >> + return 0; > > This is correct but I think the code flow is/was confusing. Can you drop > this 'return' and use 'else if' for the next code block? I think this is > more readable. Ok, it makes sense. Should I make a separate patch for this only? One more question, should we keep: if (!bri->set_sda && !bri->get_sda) { err_str = "either get_sda() or set_sda() needed"; goto err; } ? Without {get/set}_sda we won't be able to generate stop commands and possibly check if the bus is free, but we can still generate the SCL clock pulses. > >> } >> >> if (bri->recover_bus == i2c_generic_scl_recovery) { >> @@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap) >> } >> } >> >> - return; >> + return 0; >> err: >> dev_err(&adap->dev, "Not using recovery: %s\n", err_str); >> adap->bus_recovery_info = NULL; >> + >> + return 0; > > 'return -EINVAL;' I'd suggest. OK > >> } >> >> static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client) >> @@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap) >> "Failed to create compatibility class link\n"); >> #endif >> >> - i2c_init_recovery(adap); >> + res = i2c_init_recovery(adap); >> + if (res == -EPROBE_DEFER) >> + goto out_link; > > Please move 'i2c_init_recovery' above the class-link creation. It > shouldn't make a difference but we can skip the extra label and the > ifdeffery. Ok. Perhaps I should also move the debug print with the registered adapter after calling i2c_init_recovery(). > >> >> /* create pre-declared device nodes */ >> of_i2c_register_devices(adap); >> @@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap) >> >> return 0; >> >> +out_link: >> +#ifdef CONFIG_I2C_COMPAT >> + class_compat_remove_link(i2c_adapter_compat_class, &adap->dev, >> + adap->dev.parent); >> +#endif >> out_reg: >> init_completion(&adap->dev_released); >> device_unregister(&adap->dev); >> -- >> 2.25.1 >> Do you want me to integrate this patch in the previous one? 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 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs Date: Mon, 3 Aug 2020 15:33:41 +0000 [thread overview] Message-ID: <15a449fa-d649-846a-e6f2-1540f9581846@microchip.com> (raw) In-Reply-To: <20200802170500.GB10193@kunai> On 02.08.2020 20:05, Wolfram Sang wrote: > On Fri, Jun 19, 2020 at 05:19:03PM +0300, Codrin Ciubotariu wrote: >> Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return >> -EPROBE_DEFER, so we should at least treat that. This ends up with >> i2c_register_adapter() to be able to return -EPROBE_DEFER. >> >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> >> --- >> drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++------ >> 1 file changed, 16 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c >> index 4ee29fec4e93..f8d9f2048ca8 100644 >> --- a/drivers/i2c/i2c-core-base.c >> +++ b/drivers/i2c/i2c-core-base.c >> @@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter *adap) >> return i2c_gpio_init_generic_recovery(adap); >> } >> >> -static void i2c_init_recovery(struct i2c_adapter *adap) >> +static int i2c_init_recovery(struct i2c_adapter *adap) >> { >> struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; >> char *err_str; >> >> if (!bri) >> - return; >> + return 0; >> >> - i2c_gpio_init_recovery(adap); >> + if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> >> if (!bri->recover_bus) { >> err_str = "no recover_bus() found"; >> @@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap) >> if (gpiod_get_direction(bri->sda_gpiod) == 0) >> bri->set_sda = set_sda_gpio_value; >> } >> - return; >> + return 0; > > This is correct but I think the code flow is/was confusing. Can you drop > this 'return' and use 'else if' for the next code block? I think this is > more readable. Ok, it makes sense. Should I make a separate patch for this only? One more question, should we keep: if (!bri->set_sda && !bri->get_sda) { err_str = "either get_sda() or set_sda() needed"; goto err; } ? Without {get/set}_sda we won't be able to generate stop commands and possibly check if the bus is free, but we can still generate the SCL clock pulses. > >> } >> >> if (bri->recover_bus == i2c_generic_scl_recovery) { >> @@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap) >> } >> } >> >> - return; >> + return 0; >> err: >> dev_err(&adap->dev, "Not using recovery: %s\n", err_str); >> adap->bus_recovery_info = NULL; >> + >> + return 0; > > 'return -EINVAL;' I'd suggest. OK > >> } >> >> static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client) >> @@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap) >> "Failed to create compatibility class link\n"); >> #endif >> >> - i2c_init_recovery(adap); >> + res = i2c_init_recovery(adap); >> + if (res == -EPROBE_DEFER) >> + goto out_link; > > Please move 'i2c_init_recovery' above the class-link creation. It > shouldn't make a difference but we can skip the extra label and the > ifdeffery. Ok. Perhaps I should also move the debug print with the registered adapter after calling i2c_init_recovery(). > >> >> /* create pre-declared device nodes */ >> of_i2c_register_devices(adap); >> @@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap) >> >> return 0; >> >> +out_link: >> +#ifdef CONFIG_I2C_COMPAT >> + class_compat_remove_link(i2c_adapter_compat_class, &adap->dev, >> + adap->dev.parent); >> +#endif >> out_reg: >> init_completion(&adap->dev_released); >> device_unregister(&adap->dev); >> -- >> 2.25.1 >> Do you want me to integrate this patch in the previous one? 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 15:33 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 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 [this message] 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=15a449fa-d649-846a-e6f2-1540f9581846@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.