| Message ID | 20200329161552.215075-4-david@ixit.cz (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Summit SMB3xx driver & device-tree | expand |
Hi, On Sun, Mar 29, 2020 at 06:15:46PM +0200, David Heidelberg wrote: > Simplify code, more convenient to use with Device Tree. > > Reviewed-by: Dmitry Osipenko <digetx@gmail.com> > Signed-off-by: David Heidelberg <david@ixit.cz> > --- Generally I like this change a lot, but it changes the removal order, so that the IRQ is only free'd after power-supply has already been unregistered. While it is disabled in the sm347 I think it's better to keep the previous order to be on the safe side. This can easily achieved by having another patch moving from power_supply_register to devm_power_supply_register, which will further cleanup the code as a nice side-effect. -- Sebastian > drivers/power/supply/smb347-charger.c | 45 +++++++++++++-------------- > 1 file changed, 21 insertions(+), 24 deletions(-) > > diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c > index f99026d81f2a..4add9f64ba90 100644 > --- a/drivers/power/supply/smb347-charger.c > +++ b/drivers/power/supply/smb347-charger.c > @@ -836,21 +836,31 @@ static int smb347_irq_init(struct smb347_charger *smb, > struct i2c_client *client) > { > const struct smb347_charger_platform_data *pdata = smb->pdata; > - int ret, irq = gpio_to_irq(pdata->irq_gpio); > + unsigned long irqflags = IRQF_ONESHOT; > + int ret; > > - ret = gpio_request_one(pdata->irq_gpio, GPIOF_IN, client->name); > - if (ret < 0) > - goto fail; > + /* Requesting GPIO for IRQ is only needed in non-DT way */ > + if (!client->irq) { > + int irq = gpio_to_irq(pdata->irq_gpio); > + > + ret = devm_gpio_request_one(smb->dev, pdata->irq_gpio, > + GPIOF_IN, client->name); > + if (ret < 0) > + return ret; > + > + irqflags |= IRQF_TRIGGER_FALLING; > + client->irq = irq; > + } > > - ret = request_threaded_irq(irq, NULL, smb347_interrupt, > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > - client->name, smb); > + ret = devm_request_threaded_irq(smb->dev, client->irq, NULL, > + smb347_interrupt, irqflags, > + client->name, smb); > if (ret < 0) > - goto fail_gpio; > + return ret; > > ret = smb347_set_writable(smb, true); > if (ret < 0) > - goto fail_irq; > + return ret; > > /* > * Configure the STAT output to be suitable for interrupts: disable > @@ -860,20 +870,10 @@ static int smb347_irq_init(struct smb347_charger *smb, > CFG_STAT_ACTIVE_HIGH | CFG_STAT_DISABLED, > CFG_STAT_DISABLED); > if (ret < 0) > - goto fail_readonly; > + client->irq = 0; > > smb347_set_writable(smb, false); > - client->irq = irq; > - return 0; > > -fail_readonly: > - smb347_set_writable(smb, false); > -fail_irq: > - free_irq(irq, smb); > -fail_gpio: > - gpio_free(pdata->irq_gpio); > -fail: > - client->irq = 0; > return ret; > } > > @@ -1299,11 +1299,8 @@ static int smb347_remove(struct i2c_client *client) > { > struct smb347_charger *smb = i2c_get_clientdata(client); > > - if (client->irq) { > + if (client->irq) > smb347_irq_disable(smb); > - free_irq(client->irq, smb); > - gpio_free(smb->pdata->irq_gpio); > - } > > power_supply_unregister(smb->battery); > if (smb->pdata->use_usb) > -- > 2.25.0 >
diff --git a/drivers/power/supply/smb347-charger.c b/drivers/power/supply/smb347-charger.c index f99026d81f2a..4add9f64ba90 100644 --- a/drivers/power/supply/smb347-charger.c +++ b/drivers/power/supply/smb347-charger.c @@ -836,21 +836,31 @@ static int smb347_irq_init(struct smb347_charger *smb, struct i2c_client *client) { const struct smb347_charger_platform_data *pdata = smb->pdata; - int ret, irq = gpio_to_irq(pdata->irq_gpio); + unsigned long irqflags = IRQF_ONESHOT; + int ret; - ret = gpio_request_one(pdata->irq_gpio, GPIOF_IN, client->name); - if (ret < 0) - goto fail; + /* Requesting GPIO for IRQ is only needed in non-DT way */ + if (!client->irq) { + int irq = gpio_to_irq(pdata->irq_gpio); + + ret = devm_gpio_request_one(smb->dev, pdata->irq_gpio, + GPIOF_IN, client->name); + if (ret < 0) + return ret; + + irqflags |= IRQF_TRIGGER_FALLING; + client->irq = irq; + } - ret = request_threaded_irq(irq, NULL, smb347_interrupt, - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, - client->name, smb); + ret = devm_request_threaded_irq(smb->dev, client->irq, NULL, + smb347_interrupt, irqflags, + client->name, smb); if (ret < 0) - goto fail_gpio; + return ret; ret = smb347_set_writable(smb, true); if (ret < 0) - goto fail_irq; + return ret; /* * Configure the STAT output to be suitable for interrupts: disable @@ -860,20 +870,10 @@ static int smb347_irq_init(struct smb347_charger *smb, CFG_STAT_ACTIVE_HIGH | CFG_STAT_DISABLED, CFG_STAT_DISABLED); if (ret < 0) - goto fail_readonly; + client->irq = 0; smb347_set_writable(smb, false); - client->irq = irq; - return 0; -fail_readonly: - smb347_set_writable(smb, false); -fail_irq: - free_irq(irq, smb); -fail_gpio: - gpio_free(pdata->irq_gpio); -fail: - client->irq = 0; return ret; } @@ -1299,11 +1299,8 @@ static int smb347_remove(struct i2c_client *client) { struct smb347_charger *smb = i2c_get_clientdata(client); - if (client->irq) { + if (client->irq) smb347_irq_disable(smb); - free_irq(client->irq, smb); - gpio_free(smb->pdata->irq_gpio); - } power_supply_unregister(smb->battery); if (smb->pdata->use_usb)