diff mbox

[2/4] gpio: davinci: Handle the return value of davinci_gpio_irq_setup function

Message ID 1500375436-9435-3-git-send-email-j-keerthy@ti.com
State New
Headers show

Commit Message

J, KEERTHY July 18, 2017, 10:57 a.m. UTC
Currently davinci_gpio_irq_setup return value is ignored. Handle the
return value appropriately.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Suman Anna July 18, 2017, 4:54 p.m. UTC | #1
Hi Keerthy,

On 07/18/2017 05:57 AM, Keerthy wrote:
> Currently davinci_gpio_irq_setup return value is ignored. Handle the
> return value appropriately.
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 2c88054..932f270 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>  		chips->regs[bank] = gpio_base + offset_array[bank];
>  
>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
> +	if (ret)
> +		goto err;
> +
> +	platform_set_drvdata(pdev, chips);
> +	ret = davinci_gpio_irq_setup(pdev);
>  	if (ret) {
> -		ctrl_num = 0;
> -		bank_base = 0;
> -		return ret;
> +		platform_set_drvdata(pdev, NULL);

This is not needed, driver core will set this automatically if probe fails.

> +		goto err;
>  	}
>  
> -	platform_set_drvdata(pdev, chips);
> -	davinci_gpio_irq_setup(pdev);
>  	return 0;
> +
> +err:
> +	ctrl_num = 0;
> +	bank_base = 0;

Same comments as on Patch 1.

regards
Suman
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J, KEERTHY July 18, 2017, 5:46 p.m. UTC | #2
On Tuesday 18 July 2017 10:24 PM, Suman Anna wrote:
> Hi Keerthy,
> 
> On 07/18/2017 05:57 AM, Keerthy wrote:
>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
>> return value appropriately.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>> index 2c88054..932f270 100644
>> --- a/drivers/gpio/gpio-davinci.c
>> +++ b/drivers/gpio/gpio-davinci.c
>> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>  		chips->regs[bank] = gpio_base + offset_array[bank];
>>  
>>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
>> +	if (ret)
>> +		goto err;
>> +
>> +	platform_set_drvdata(pdev, chips);
>> +	ret = davinci_gpio_irq_setup(pdev);
>>  	if (ret) {
>> -		ctrl_num = 0;
>> -		bank_base = 0;
>> -		return ret;
>> +		platform_set_drvdata(pdev, NULL);
> 
> This is not needed, driver core will set this automatically if probe fails.

okay. I will remove this.

> 
>> +		goto err;
>>  	}
>>  
>> -	platform_set_drvdata(pdev, chips);
>> -	davinci_gpio_irq_setup(pdev);
>>  	return 0;
>> +
>> +err:
>> +	ctrl_num = 0;
>> +	bank_base = 0;
> 
> Same comments as on Patch 1.

Yup will fix this as i have done with Patch 1.

> 
> regards
> Suman
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johan Hovold July 19, 2017, 11:10 a.m. UTC | #3
On Tue, Jul 18, 2017 at 04:27:14PM +0530, Keerthy wrote:
> Currently davinci_gpio_irq_setup return value is ignored. Handle the
> return value appropriately.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 2c88054..932f270 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>  		chips->regs[bank] = gpio_base + offset_array[bank];
>  
>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
> +	if (ret)
> +		goto err;
> +
> +	platform_set_drvdata(pdev, chips);
> +	ret = davinci_gpio_irq_setup(pdev);
>  	if (ret) {
> -		ctrl_num = 0;
> -		bank_base = 0;
> -		return ret;
> +		platform_set_drvdata(pdev, NULL);
> +		goto err;
>  	}
>  
> -	platform_set_drvdata(pdev, chips);
> -	davinci_gpio_irq_setup(pdev);
>  	return 0;

There's a separate but related bug here too as the clk_prepare_enable()
in davinci_gpio_irq_setup() is never balanced on driver unbind.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J, KEERTHY July 20, 2017, 6:44 a.m. UTC | #4
On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
> On Tue, Jul 18, 2017 at 04:27:14PM +0530, Keerthy wrote:
>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
>> return value appropriately.
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>> index 2c88054..932f270 100644
>> --- a/drivers/gpio/gpio-davinci.c
>> +++ b/drivers/gpio/gpio-davinci.c
>> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>  		chips->regs[bank] = gpio_base + offset_array[bank];
>>  
>>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
>> +	if (ret)
>> +		goto err;
>> +
>> +	platform_set_drvdata(pdev, chips);
>> +	ret = davinci_gpio_irq_setup(pdev);
>>  	if (ret) {
>> -		ctrl_num = 0;
>> -		bank_base = 0;
>> -		return ret;
>> +		platform_set_drvdata(pdev, NULL);
>> +		goto err;
>>  	}
>>  
>> -	platform_set_drvdata(pdev, chips);
>> -	davinci_gpio_irq_setup(pdev);
>>  	return 0;
> 
> There's a separate but related bug here too as the clk_prepare_enable()
> in davinci_gpio_irq_setup() is never balanced on driver unbind.

Yes Johan. I will send that as a separate patch.

> 
> Johan
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J, KEERTHY July 20, 2017, 9:10 a.m. UTC | #5
On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
> 
> 
> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
>> On Tue, Jul 18, 2017 at 04:27:14PM +0530, Keerthy wrote:
>>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
>>> return value appropriately.
>>>
>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>> ---
>>>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>>> index 2c88054..932f270 100644
>>> --- a/drivers/gpio/gpio-davinci.c
>>> +++ b/drivers/gpio/gpio-davinci.c
>>> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>>  		chips->regs[bank] = gpio_base + offset_array[bank];
>>>  
>>>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
>>> +	if (ret)
>>> +		goto err;
>>> +
>>> +	platform_set_drvdata(pdev, chips);
>>> +	ret = davinci_gpio_irq_setup(pdev);
>>>  	if (ret) {
>>> -		ctrl_num = 0;
>>> -		bank_base = 0;
>>> -		return ret;
>>> +		platform_set_drvdata(pdev, NULL);
>>> +		goto err;
>>>  	}
>>>  
>>> -	platform_set_drvdata(pdev, chips);
>>> -	davinci_gpio_irq_setup(pdev);
>>>  	return 0;
>>
>> There's a separate but related bug here too as the clk_prepare_enable()
>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
> 
> Yes Johan. I will send that as a separate patch.

This is already fixed in the latest kernel:

commit 6dc0048cff988858254fcc26becfc1e9753efa79
Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
Date:   Tue May 23 14:48:57 2017 +0530

Regards,
Keerthy
> 
>>
>> Johan
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johan Hovold July 20, 2017, 9:50 a.m. UTC | #6
On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
> 
> 
> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
> > 
> > 
> > On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
> >> On Tue, Jul 18, 2017 at 04:27:14PM +0530, Keerthy wrote:
> >>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
> >>> return value appropriately.
> >>>
> >>> Signed-off-by: Keerthy <j-keerthy@ti.com>
> >>> ---
> >>>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
> >>>  1 file changed, 13 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> >>> index 2c88054..932f270 100644
> >>> --- a/drivers/gpio/gpio-davinci.c
> >>> +++ b/drivers/gpio/gpio-davinci.c
> >>> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> >>>  		chips->regs[bank] = gpio_base + offset_array[bank];
> >>>  
> >>>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
> >>> +	if (ret)
> >>> +		goto err;
> >>> +
> >>> +	platform_set_drvdata(pdev, chips);
> >>> +	ret = davinci_gpio_irq_setup(pdev);
> >>>  	if (ret) {
> >>> -		ctrl_num = 0;
> >>> -		bank_base = 0;
> >>> -		return ret;
> >>> +		platform_set_drvdata(pdev, NULL);
> >>> +		goto err;
> >>>  	}
> >>>  
> >>> -	platform_set_drvdata(pdev, chips);
> >>> -	davinci_gpio_irq_setup(pdev);
> >>>  	return 0;
> >>
> >> There's a separate but related bug here too as the clk_prepare_enable()
> >> in davinci_gpio_irq_setup() is never balanced on driver unbind.
> > 
> > Yes Johan. I will send that as a separate patch.
> 
> This is already fixed in the latest kernel:
> 
> commit 6dc0048cff988858254fcc26becfc1e9753efa79
> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
> Date:   Tue May 23 14:48:57 2017 +0530

That change only handles errors in davinci_gpio_irq_setup() (i.e. during
probe) and not the imbalance at driver unbind that I was referring to.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J, KEERTHY July 20, 2017, 10:02 a.m. UTC | #7
On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
> On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
>>
>>
>> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
>>>
>>>
>>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
>>>> On Tue, Jul 18, 2017 at 04:27:14PM +0530, Keerthy wrote:
>>>>> Currently davinci_gpio_irq_setup return value is ignored. Handle the
>>>>> return value appropriately.
>>>>>
>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>> ---
>>>>>  drivers/gpio/gpio-davinci.c | 18 +++++++++++++-----
>>>>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>>>>> index 2c88054..932f270 100644
>>>>> --- a/drivers/gpio/gpio-davinci.c
>>>>> +++ b/drivers/gpio/gpio-davinci.c
>>>>> @@ -233,15 +233,23 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>>>>  		chips->regs[bank] = gpio_base + offset_array[bank];
>>>>>  
>>>>>  	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
>>>>> +	if (ret)
>>>>> +		goto err;
>>>>> +
>>>>> +	platform_set_drvdata(pdev, chips);
>>>>> +	ret = davinci_gpio_irq_setup(pdev);
>>>>>  	if (ret) {
>>>>> -		ctrl_num = 0;
>>>>> -		bank_base = 0;
>>>>> -		return ret;
>>>>> +		platform_set_drvdata(pdev, NULL);
>>>>> +		goto err;
>>>>>  	}
>>>>>  
>>>>> -	platform_set_drvdata(pdev, chips);
>>>>> -	davinci_gpio_irq_setup(pdev);
>>>>>  	return 0;
>>>>
>>>> There's a separate but related bug here too as the clk_prepare_enable()
>>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
>>>
>>> Yes Johan. I will send that as a separate patch.
>>
>> This is already fixed in the latest kernel:
>>
>> commit 6dc0048cff988858254fcc26becfc1e9753efa79
>> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> Date:   Tue May 23 14:48:57 2017 +0530
> 
> That change only handles errors in davinci_gpio_irq_setup() (i.e. during
> probe) and not the imbalance at driver unbind that I was referring to.

Okay got it. One more clk_unprepare_disable() call needs to be there in
probe err path.

> 
> Johan
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johan Hovold July 20, 2017, 10:05 a.m. UTC | #8
On Thu, Jul 20, 2017 at 03:32:27PM +0530, Keerthy wrote:
> On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
> > On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
> >> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
> >>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:

> >>>> There's a separate but related bug here too as the clk_prepare_enable()
> >>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
> >>>
> >>> Yes Johan. I will send that as a separate patch.
> >>
> >> This is already fixed in the latest kernel:
> >>
> >> commit 6dc0048cff988858254fcc26becfc1e9753efa79
> >> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
> >> Date:   Tue May 23 14:48:57 2017 +0530
> > 
> > That change only handles errors in davinci_gpio_irq_setup() (i.e. during
> > probe) and not the imbalance at driver unbind that I was referring to.
> 
> Okay got it. One more clk_unprepare_disable() call needs to be there in
> probe err path.

No, you need to balance it on driver unbind, that is, in a new remove()
callback.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J, KEERTHY July 20, 2017, 10:10 a.m. UTC | #9
On Thursday 20 July 2017 03:35 PM, Johan Hovold wrote:
> On Thu, Jul 20, 2017 at 03:32:27PM +0530, Keerthy wrote:
>> On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
>>> On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
>>>> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
>>>>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
> 
>>>>>> There's a separate but related bug here too as the clk_prepare_enable()
>>>>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
>>>>>
>>>>> Yes Johan. I will send that as a separate patch.
>>>>
>>>> This is already fixed in the latest kernel:
>>>>
>>>> commit 6dc0048cff988858254fcc26becfc1e9753efa79
>>>> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
>>>> Date:   Tue May 23 14:48:57 2017 +0530
>>>
>>> That change only handles errors in davinci_gpio_irq_setup() (i.e. during
>>> probe) and not the imbalance at driver unbind that I was referring to.
>>
>> Okay got it. One more clk_unprepare_disable() call needs to be there in
>> probe err path.
> 
> No, you need to balance it on driver unbind, that is, in a new remove()
> callback.

Okay yes davinci_gpio_irq_setup is the last call in probe so no need of
that in probe error path. I will add a new remove() to balance.

Thanks,
Keerthy

> 
> Johan
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grygorii Strashko July 20, 2017, 9:34 p.m. UTC | #10
On 07/20/2017 05:05 AM, Johan Hovold wrote:
> On Thu, Jul 20, 2017 at 03:32:27PM +0530, Keerthy wrote:
>> On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
>>> On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
>>>> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
>>>>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
> 
>>>>>> There's a separate but related bug here too as the clk_prepare_enable()
>>>>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
>>>>>
>>>>> Yes Johan. I will send that as a separate patch.
>>>>
>>>> This is already fixed in the latest kernel:
>>>>
>>>> commit 6dc0048cff988858254fcc26becfc1e9753efa79
>>>> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
>>>> Date:   Tue May 23 14:48:57 2017 +0530
>>>
>>> That change only handles errors in davinci_gpio_irq_setup() (i.e. during
>>> probe) and not the imbalance at driver unbind that I was referring to.
>>
>> Okay got it. One more clk_unprepare_disable() call needs to be there in
>> probe err path.
> 
> No, you need to balance it on driver unbind, that is, in a new remove()
> callback.
> 

Sry, but manual driver unbind for this driver is really smth unexpected ;(
So, I'm not sure if it need to be implemented and even yes - it should not be
a part of this patch. Probably, smth like "convert driver to be a module".

By the way, I've tried to unbind gpio-omap, result - failure (expected),
as unbind does not take into account module refcnt state.
J, KEERTHY July 21, 2017, 3:53 a.m. UTC | #11
On Friday 21 July 2017 03:04 AM, Grygorii Strashko wrote:
> 
> 
> On 07/20/2017 05:05 AM, Johan Hovold wrote:
>> On Thu, Jul 20, 2017 at 03:32:27PM +0530, Keerthy wrote:
>>> On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
>>>> On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
>>>>> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
>>>>>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
>>
>>>>>>> There's a separate but related bug here too as the clk_prepare_enable()
>>>>>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
>>>>>>
>>>>>> Yes Johan. I will send that as a separate patch.
>>>>>
>>>>> This is already fixed in the latest kernel:
>>>>>
>>>>> commit 6dc0048cff988858254fcc26becfc1e9753efa79
>>>>> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
>>>>> Date:   Tue May 23 14:48:57 2017 +0530
>>>>
>>>> That change only handles errors in davinci_gpio_irq_setup() (i.e. during
>>>> probe) and not the imbalance at driver unbind that I was referring to.
>>>
>>> Okay got it. One more clk_unprepare_disable() call needs to be there in
>>> probe err path.
>>
>> No, you need to balance it on driver unbind, that is, in a new remove()
>> callback.
>>
> 
> Sry, but manual driver unbind for this driver is really smth unexpected ;(
> So, I'm not sure if it need to be implemented and even yes - it should not be
> a part of this patch. Probably, smth like "convert driver to be a module".
> 

The GPIO_DAVINCI config is bool. Thanks for checking on that Grygorii.

> By the way, I've tried to unbind gpio-omap, result - failure (expected),
> as unbind does not take into account module refcnt state.

Okay.

> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johan Hovold July 21, 2017, 7:46 a.m. UTC | #12
On Thu, Jul 20, 2017 at 04:34:42PM -0500, Grygorii Strashko wrote:
> 
> 
> On 07/20/2017 05:05 AM, Johan Hovold wrote:
> > On Thu, Jul 20, 2017 at 03:32:27PM +0530, Keerthy wrote:
> >> On Thursday 20 July 2017 03:20 PM, Johan Hovold wrote:
> >>> On Thu, Jul 20, 2017 at 02:40:37PM +0530, Keerthy wrote:
> >>>> On Thursday 20 July 2017 12:14 PM, Keerthy wrote:
> >>>>> On Wednesday 19 July 2017 04:40 PM, Johan Hovold wrote:
> > 
> >>>>>> There's a separate but related bug here too as the clk_prepare_enable()
> >>>>>> in davinci_gpio_irq_setup() is never balanced on driver unbind.
> >>>>>
> >>>>> Yes Johan. I will send that as a separate patch.
> >>>>
> >>>> This is already fixed in the latest kernel:
> >>>>
> >>>> commit 6dc0048cff988858254fcc26becfc1e9753efa79
> >>>> Author: Arvind Yadav <arvind.yadav.cs@gmail.com>
> >>>> Date:   Tue May 23 14:48:57 2017 +0530
> >>>
> >>> That change only handles errors in davinci_gpio_irq_setup() (i.e. during
> >>> probe) and not the imbalance at driver unbind that I was referring to.
> >>
> >> Okay got it. One more clk_unprepare_disable() call needs to be there in
> >> probe err path.
> > 
> > No, you need to balance it on driver unbind, that is, in a new remove()
> > callback.
> > 
> 
> Sry, but manual driver unbind for this driver is really smth unexpected ;(

It certainly wouldn't be something often used (e.g. besides during
development) but that doesn't mean it should not be implemented.

> So, I'm not sure if it need to be implemented and even yes - it should not be
> a part of this patch.

That's why I said "separate, but related" above.

> Probably, smth like "convert driver to be a module".
>
> By the way, I've tried to unbind gpio-omap, result - failure (expected),
> as unbind does not take into account module refcnt state.

Indeed. We also have CONFIG_DEBUG_TEST_DRIVER_REMOVE which would try to
unbind this driver during probe if enabled.

Getting into the habit of properly cleaning up allocated and enabled
resources is only a good thing; it shows that the author has thought
this through and serves as documentation of what needs to be released in
both probe error paths and driver unbind callbacks.

Assumptions also change over time (e.g. deferred probe and
CONFIG_DEBUG_TEST_DRIVER_REMOVE), and by not taking such shortcuts we
are also preventing incomplete code from being copied and reproduced in
other drivers (e.g. on hotpluggable buses).

So, just add the remove callback (in a separate patch) and everything is
good.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 2c88054..932f270 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -233,15 +233,23 @@  static int davinci_gpio_probe(struct platform_device *pdev)
 		chips->regs[bank] = gpio_base + offset_array[bank];
 
 	ret = devm_gpiochip_add_data(dev, &chips->chip, chips);
+	if (ret)
+		goto err;
+
+	platform_set_drvdata(pdev, chips);
+	ret = davinci_gpio_irq_setup(pdev);
 	if (ret) {
-		ctrl_num = 0;
-		bank_base = 0;
-		return ret;
+		platform_set_drvdata(pdev, NULL);
+		goto err;
 	}
 
-	platform_set_drvdata(pdev, chips);
-	davinci_gpio_irq_setup(pdev);
 	return 0;
+
+err:
+	ctrl_num = 0;
+	bank_base = 0;
+
+	return ret;
 }
 
 /*--------------------------------------------------------------------------*/