* Liam Breck <liam@xxxxxxxxxxxxxxxxx> [170112 12:25]:
> On Thu, Jan 12, 2017 at 9:44 AM, Sebastian Reichel <sre@xxxxxxxxxx> wrote:
> > Hi Liam & Tony,
> >
> > On Wed, Jan 11, 2017 at 04:41:49PM -0800, Tony Lindgren wrote:
> >> From: Liam Breck <kernel@xxxxxxxxxxxxxxxxx>
> >>
> >> The device specific data is not fully initialized after
> >> request_threaded_irq().
> >>
> >> This causes problems when the IRQ handler tries to reference them.
> >> Fix the issue by enabling IRQ only at the end of the probe.
> >>
> >> Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190
> >> Battery Charger")
> >> Cc: Mark A. Greer <mgreer@xxxxxxxxxxxxxxx>
> >> Cc: Matt Ranostay <matt@ranostay.consulting>
> >> Signed-off-by: Liam Breck <kernel@xxxxxxxxxxxxxxxxx>
> >> [tony@xxxxxxxxxxx: cleaned up patch description a bit]
> >> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> >> ---
> >> drivers/power/supply/bq24190_charger.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> >> --- a/drivers/power/supply/bq24190_charger.c
> >> +++ b/drivers/power/supply/bq24190_charger.c
> >> @@ -1392,6 +1392,7 @@ static int bq24190_probe(struct i2c_client *client,
> >> return -EINVAL;
> >> }
> >>
> >> + irq_set_status_flags(bdi->irq, IRQ_NOAUTOEN);
> >> ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
> >> bq24190_irq_handler_thread,
> >> IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> >> @@ -1436,6 +1437,8 @@ static int bq24190_probe(struct i2c_client *client,
> >> goto out4;
> >> }
> >>
> >> + enable_irq(bdi->irq);
> >> +
> >> return 0;
> >>
> >> out4:
> >
> > Can't you just move the irq request towards the end of the probe?
> > That way it will also be released before the power-supply structure
> > is released.
>
> I did that in a first draft I showed Tony. He suggested this way.
> Tony, rationale?
Both will work for me just fine as long as done in a single patch
with no other changes.
The first option is less changes if needed as a fix, up to Sebastian
depending on what he prefers.
Regards,
Tony
> It looks like this:
> diff --git a/drivers/power/bq24190_charger.c b/drivers/power/bq24190_charger.c
> index b51eac1..54c8952 100644
> --- a/drivers/power/bq24190_charger.c
> +++ b/drivers/power/bq24190_charger.c
> @@ -1366,22 +1366,13 @@ static int bq24190_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> - ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
> - bq24190_irq_handler_thread,
> - IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> - "bq24190-charger", bdi);
> - if (ret < 0) {
> - dev_err(dev, "Can't set up irq handler\n");
> - goto out1;
> - }
> -
> pm_runtime_enable(dev);
> pm_runtime_resume(dev);
>
> ret = bq24190_hw_init(bdi);
> if (ret < 0) {
> dev_err(dev, "Hardware init failed\n");
> - goto out2;
> + goto out1;
> }
>
> charger_cfg.drv_data = bdi;
> @@ -1392,7 +1383,7 @@ static int bq24190_probe(struct i2c_client *client,
> if (IS_ERR(bdi->charger)) {
> dev_err(dev, "Can't register charger\n");
> ret = PTR_ERR(bdi->charger);
> - goto out2;
> + goto out1;
> }
>
> battery_cfg.drv_data = bdi;
> @@ -1401,24 +1392,34 @@ static int bq24190_probe(struct i2c_client *client,
> if (IS_ERR(bdi->battery)) {
> dev_err(dev, "Can't register battery\n");
> ret = PTR_ERR(bdi->battery);
> - goto out3;
> + goto out2;
> }
>
> ret = bq24190_sysfs_create_group(bdi);
> if (ret) {
> dev_err(dev, "Can't create sysfs entries\n");
> + goto out3;
> + }
> +
> + ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
> + bq24190_irq_handler_thread,
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> + "bq24190-charger", bdi);
> + if (ret < 0) {
> + dev_err(dev, "Can't set up irq handler\n");
> goto out4;
> }
>
> return 0;
>
> out4:
> - power_supply_unregister(bdi->battery);
> + bq24190_sysfs_remove_group(bdi);
> out3:
> - power_supply_unregister(bdi->charger);
> + power_supply_unregister(bdi->battery);
> out2:
> - pm_runtime_disable(dev);
> + power_supply_unregister(bdi->charger);
> out1:
> + pm_runtime_disable(dev);
> if (bdi->gpio_int)
> gpio_free(bdi->gpio_int);
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
