Re: [PATCH 1/6] power: bq24190_charger: Call enable_irq() only at the end of probe()

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2860 bytes --]

Hi,

On Thu, Jan 12, 2017 at 12:58:11PM -0800, Tony Lindgren wrote:
> * Liam Breck <liam@networkimprov.net> [170112 12:25]:
> > On Thu, Jan 12, 2017 at 9:44 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > > On Wed, Jan 11, 2017 at 04:41:49PM -0800, Tony Lindgren wrote:
> > >> From: Liam Breck <kernel@networkimprov.net>
> > >>
> > >> 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@animalcreek.com>
> > >> Cc: Matt Ranostay <matt@ranostay.consulting>
> > >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> > >> [tony@atomide.com: cleaned up patch description a bit]
> > >> Signed-off-by: Tony Lindgren <tony@atomide.com>
> > >> ---
> > >>  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.

Then please send the variant, which moves the block in the v2
patchset, since it behaves correctly during driver removal and
results in less lines of code.

> > diff --git a/drivers/power/bq24190_charger.c b/drivers/power/bq24190_charger.c

Also make sure, that you base your patches on power-supply's
for-next branch. Your base is way too *old*, since the driver
lives in drivers/power/supply/bq24190_charger.c nowadays.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]