Re: [PATCH 3/4] Use devm_request_irq to avoid some checks

[Marcos Souza]

Hi Lars,

2012/10/19 Lars-Peter Clausen <lars@xxxxxxxxxx>:
> On 10/19/2012 01:41 AM, Marcos Paulo de Souza wrote:
>> With this we can remove a lot of checks.
>>
>> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@xxxxxxxxx>
>
> This one is a bit more tricky and the driver currently gets it (partially
> wrong). The issue is that since the power supply is referenced in the
> interrupt handler, the IRQs should be requested after the power supply has
> been registered and be freed before the power supply is unregistered.
>
> So devm can't really be used here. But as I said the current driver already
> gets it wrong.

Tanks for all the reviews! I will resend these patches will your comments.

But in this case, there is a way that we can fix this issue about
register/unregister IRQ?

Maybe another approach?

If you give some tips for me, my next patches will have this issue fixed too.

Thanks for all!

> - Lars
>
>> ---
>>  drivers/power/jz4740-battery.c |   25 ++++++++-----------------
>>  1 file changed, 8 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
>> index 9d9b2a8..b80f7ed 100644
>> --- a/drivers/power/jz4740-battery.c
>> +++ b/drivers/power/jz4740-battery.c
>> @@ -307,8 +307,8 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>
>>       INIT_DELAYED_WORK(&jz_battery->work, jz_battery_work);
>>
>> -     ret = request_irq(jz_battery->irq, jz_battery_irq_handler, 0, pdev->name,
>> -                     jz_battery);
>> +     ret = devm_request_irq(&pdev->dev, jz_battery->irq, jz_battery_irq_handler,
>> +                             0, pdev->name, jz_battery);
>>       if (ret) {
>>               dev_err(&pdev->dev, "Failed to request irq %d\n", ret);
>>               goto err;
>> @@ -319,7 +319,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>               ret = gpio_request(pdata->gpio_charge, dev_name(&pdev->dev));
>>               if (ret) {
>>                       dev_err(&pdev->dev, "charger state gpio request failed.\n");
>> -                     goto err_free_irq;
>> +                     goto err;
>>               }
>>               ret = gpio_direction_input(pdata->gpio_charge);
>>               if (ret) {
>> @@ -330,7 +330,8 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>               jz_battery->charge_irq = gpio_to_irq(pdata->gpio_charge);
>>
>>               if (jz_battery->charge_irq >= 0) {
>> -                     ret = request_irq(jz_battery->charge_irq,
>> +                     ret = devm_request_irq(&pdev->dev,
>> +                                 jz_battery->charge_irq,
>>                                   jz_battery_charge_irq,
>>                                   IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>                                   dev_name(&pdev->dev), jz_battery);
>> @@ -352,7 +353,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>       ret = power_supply_register(&pdev->dev, &jz_battery->battery);
>>       if (ret) {
>>               dev_err(&pdev->dev, "power supply battery register failed.\n");
>> -             goto err_free_charge_irq;
>> +             goto err_free_gpio;
>>       }
>>
>>       platform_set_drvdata(pdev, jz_battery);
>> @@ -360,16 +361,11 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>>
>>       return 0;
>>
>> -err_free_charge_irq:
>> -     if (jz_battery->charge_irq >= 0)
>> -             free_irq(jz_battery->charge_irq, jz_battery);
>>  err_free_gpio:
>>       if (gpio_is_valid(pdata->gpio_charge))
>>               gpio_free(jz_battery->pdata->gpio_charge);
>> -err_free_irq:
>> -     free_irq(jz_battery->irq, jz_battery);
>> -     platform_set_drvdata(pdev, NULL);
>>  err:
>> +     platform_set_drvdata(pdev, NULL);
>>       return ret;
>>  }
>>
>> @@ -379,16 +375,11 @@ static int __devexit jz_battery_remove(struct platform_device *pdev)
>>
>>       cancel_delayed_work_sync(&jz_battery->work);
>>
>> -     if (gpio_is_valid(jz_battery->pdata->gpio_charge)) {
>> -             if (jz_battery->charge_irq >= 0)
>> -                     free_irq(jz_battery->charge_irq, jz_battery);
>> +     if (gpio_is_valid(jz_battery->pdata->gpio_charge))
>>               gpio_free(jz_battery->pdata->gpio_charge);
>> -     }
>>
>>       power_supply_unregister(&jz_battery->battery);
>>
>> -     free_irq(jz_battery->irq, jz_battery);
>> -
>>       return 0;
>>  }
>>
>



-- 
Att,

Marcos Paulo de Souza
Acadêmico de Ciencia da Computação - FURB - SC
Github: https://github.com/marcosps/
"Uma vida sem desafios é uma vida sem razão"
"A life without challenges, is a non reason life"
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/