Re: [PATCH 1/5] iio: adc: exynos_adc: Use devm_* APIs

[Jonathan Cameron <jic23@kernel.org>]

On 18/06/14 15:53, Srinivas Pandruvada wrote:
> On 06/17/2014 11:35 PM, Sachin Kamat wrote:
>> Hi Beomho,
>>
>> On Wed, Jun 18, 2014 at 11:51 AM, Beomho Seo <beomho.seo@samsung.com> wrote:
>>> This patch changes APIs from request_irq() and iio_device_register()
>>> to devm_* APIs. Using them, make code simpler.
>>>
>>> Signed-off-by: Beomho Seo <beomho.seo@samsung.com>
>>> ---
>>>   drivers/iio/adc/exynos_adc.c |   11 +++--------
>>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>>> index d325aea..37b6eb5 100644
>>> --- a/drivers/iio/adc/exynos_adc.c
>>> +++ b/drivers/iio/adc/exynos_adc.c
>>> @@ -337,7 +337,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>>          else
>>>                  indio_dev->num_channels = MAX_ADC_V2_CHANNELS;
>>>
>>> -       ret = request_irq(info->irq, exynos_adc_isr,
>>> +       ret = devm_request_irq(&pdev->dev, info->irq, exynos_adc_isr,
>>>                                          0, dev_name(&pdev->dev), info);
>>>          if (ret < 0) {
>>>                  dev_err(&pdev->dev, "failed requesting irq, irq = %d\n",
>>> @@ -345,9 +345,9 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>>                  goto err_disable_clk;
>>>          }
>>>
>>> -       ret = iio_device_register(indio_dev);
>>> +       ret = devm_iio_device_register(&pdev->dev, indio_dev);
> Same here. The remove operation needs some clock / regulator disable.
> So devm_xx may be not be correct based on Jonathan's recent comments.
devm will be fine for anything before non devm elements that need to be
reversed.  After that the ordering gets messed up in the driver tear down
as described.
>
> Thanks,
> Srinivas
>>>          if (ret)
>>> -               goto err_irq;
>>> +               goto err_disable_clk;
>>>
>>>          exynos_adc_hw_init(info);
>>>
>>> @@ -362,9 +362,6 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>>   err_of_populate:
>>>          device_for_each_child(&pdev->dev, NULL,
>>>                                  exynos_adc_remove_devices);
>>> -       iio_device_unregister(indio_dev);
>>> -err_irq:
>>> -       free_irq(info->irq, info);
>>>   err_disable_clk:
>>>          writel(0, info->enable_reg);
>>>          clk_disable_unprepare(info->clk);
>>> @@ -380,8 +377,6 @@ static int exynos_adc_remove(struct platform_device *pdev)
>>>
>>>          device_for_each_child(&pdev->dev, NULL,
>>>                                  exynos_adc_remove_devices);
>>> -       iio_device_unregister(indio_dev);
>>> -       free_irq(info->irq, info);
>>>          writel(0, info->enable_reg);
>>>          clk_disable_unprepare(info->clk);
>>>          regulator_disable(info->vdd);
>>> --
>>> 1.7.9.5
>>>
>>
>> I am not sure if this change is safe as it changes the order of execution.
>> With devm_request_irq, irq is freed only after the function completes, i.e.,
>> the order will now be something like
>>
>> writel(0, info->enable_reg)
>> clk_disable_unprepare(info->clk);
>> regulator_disable(info->vdd);
>> iio_device_unregister
>> free_irq
>>
>> which is not what the original code did.
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html