| Message ID | 20200619141904.910889-4-codrin.ciubotariu@microchip.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: core: add generic GPIO bus recovery | expand |
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. > } > > 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. > } > > 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. > > /* 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 >
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
> > 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? I am fine if this is included in this change. > 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. My gut feeling says we need to keep it. I can't recall the reason now and want to send out this answer ASAP. Anyhow, this definately would be a seperate patch. If you really want to, send a patch, and then I have to think why we still need it ;) > Ok. Perhaps I should also move the debug print with the registered > adapter after calling i2c_init_recovery(). Yes, makes sense. > Do you want me to integrate this patch in the previous one? Nope, please keep it seperate.
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; } 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; } 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; /* 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);
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(-)