Patchwork [v2,2/3] ep93xx: IDE driver platform support code

login
register
mail settings
Submitter Rafal Prylowski
Date April 4, 2012, 8:41 a.m.
Message ID <4F7C0922.1070200@metasoft.pl>
Download mbox | patch
Permalink /patch/150654/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Rafal Prylowski - April 4, 2012, 8:41 a.m.
On 2012-04-03 19:41, H Hartley Sweeten wrote:
> On Tuesday, April 03, 2012 7:47 AM, Rafal Prylowski wrote:
>>
>> +int ep93xx_ide_acquire_gpio(struct platform_device *pdev)
>> +{
>> +	int err;
>> +	int i;
>> +
>> +	for (i = 2; i < 8; i++) {
>> +		err = gpio_request(EP93XX_GPIO_LINE_E(i), dev_name(&pdev->dev));
>> +		if (err)
>> +			goto fail_gpio_e;
>> +	}
>> +	for (i = 4; i < 8; i++) {
>> +		err = gpio_request(EP93XX_GPIO_LINE_G(i), dev_name(&pdev->dev));
>> +		if (err)
>> +			goto fail_gpio_g;
>> +	}
>> +	for (i = 0; i < 8; i++) {
>> +		err = gpio_request(EP93XX_GPIO_LINE_H(i), dev_name(&pdev->dev));
>> +		if (err)
>> +			goto fail_gpio_h;
>> +	}
>> +
>> +	/* GPIO ports E[7:2], G[7:4] and H used by IDE */
>> +	ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_EONIDE |
>> +				 EP93XX_SYSCON_DEVCFG_GONIDE |
>> +				 EP93XX_SYSCON_DEVCFG_HONIDE);
>> +	return 0;
>> +
>> +fail_gpio_h:
>> +	for (--i; i >= 0; --i)
>> +		gpio_free(EP93XX_GPIO_LINE_H(i));
>> +	i = 8;
>> +fail_gpio_g:
>> +	for (--i; i >= 4; --i)
>> +		gpio_free(EP93XX_GPIO_LINE_G(i));
>> +	i = 8;
>> +fail_gpio_e:
>> +	for (--i; i >= 2; --i)
>> +		gpio_free(EP93XX_GPIO_LINE_E(i));
>> +	return err;
>> +}
>> +EXPORT_SYMBOL(ep93xx_ide_acquire_gpio);

[not related to my patch, but ep93xx keypad]:
Isn't ep93xx_keypad_acquire_gpio be more correct if we apply the following patch:


This way we don't double free EP93XX_GPIO_LINE_C(i), and don't free lines which were not
successfully acquired (I noticed this when writing my patch, which is based on
ep93xx_keypad_acquire/release_gpio).

Thanks,
RP
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
hartleys - April 4, 2012, 4:20 p.m.
On Wednesday, April 04, 2012 1:41 AM, Rafal Prylowski wrote:
> On 2012-04-03 19:41, H Hartley Sweeten wrote:
> [not related to my patch, but ep93xx keypad]:
> Isn't ep93xx_keypad_acquire_gpio be more correct if we apply the following patch:
>
> Index: linux-2.6/arch/arm/mach-ep93xx/core.c
> ===================================================================
> --- linux-2.6.orig/arch/arm/mach-ep93xx/core.c
> +++ linux-2.6/arch/arm/mach-ep93xx/core.c
> @@ -734,7 +734,7 @@ int ep93xx_keypad_acquire_gpio(struct pl
>  fail_gpio_d:
>  	gpio_free(EP93XX_GPIO_LINE_C(i));
>  fail_gpio_c:
> -	for ( ; i >= 0; --i) {
> +	for (--i; i >= 0; --i) {
>  		gpio_free(EP93XX_GPIO_LINE_C(i));
>  		gpio_free(EP93XX_GPIO_LINE_D(i));
>  	}
>
> This way we don't double free EP93XX_GPIO_LINE_C(i), and don't free lines which were not
> successfully acquired (I noticed this when writing my patch, which is based on
> ep93xx_keypad_acquire/release_gpio).

You are correct... ;-)

Care to submit an actual patch?

Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
hartleys - April 4, 2012, 4:43 p.m.
On Wednesday, April 04, 2012 9:20 AM, H Hartley Sweeten wrote:
> On Wednesday, April 04, 2012 1:41 AM, Rafal Prylowski wrote:
>> On 2012-04-03 19:41, H Hartley Sweeten wrote:
>> [not related to my patch, but ep93xx keypad]:
>> Isn't ep93xx_keypad_acquire_gpio be more correct if we apply the following patch:
>>
>> Index: linux-2.6/arch/arm/mach-ep93xx/core.c
>> ===================================================================
>> --- linux-2.6.orig/arch/arm/mach-ep93xx/core.c
>> +++ linux-2.6/arch/arm/mach-ep93xx/core.c
>> @@ -734,7 +734,7 @@ int ep93xx_keypad_acquire_gpio(struct pl
>>  fail_gpio_d:
>>  	gpio_free(EP93XX_GPIO_LINE_C(i));
>>  fail_gpio_c:
>> -	for ( ; i >= 0; --i) {
>> +	for (--i; i >= 0; --i) {
>>  		gpio_free(EP93XX_GPIO_LINE_C(i));
>>  		gpio_free(EP93XX_GPIO_LINE_D(i));
>>  	}
>>
>> This way we don't double free EP93XX_GPIO_LINE_C(i), and don't free lines which were not
>> successfully acquired (I noticed this when writing my patch, which is based on
>> ep93xx_keypad_acquire/release_gpio).
>
> You are correct... ;-)
>
> Care to submit an actual patch?

Note, if/when the devm_gpio_request patch gets into the kernel tree we can use
that and get rid of the gpio_free's completely.

Sorry, I don't have a link to that patch...

Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
hartleys - April 4, 2012, 5:11 p.m.
On Wednesday, April 04, 2012 9:43 AM, H Hartley Sweeten wrote:
> On Wednesday, April 04, 2012 9:20 AM, H Hartley Sweeten wrote:
>> On Wednesday, April 04, 2012 1:41 AM, Rafal Prylowski wrote:
>>> On 2012-04-03 19:41, H Hartley Sweeten wrote:
>>> [not related to my patch, but ep93xx keypad]:
>>> Isn't ep93xx_keypad_acquire_gpio be more correct if we apply the following patch:
>>>
>>> Index: linux-2.6/arch/arm/mach-ep93xx/core.c
>>> ===================================================================
>>> --- linux-2.6.orig/arch/arm/mach-ep93xx/core.c
>>> +++ linux-2.6/arch/arm/mach-ep93xx/core.c
>>> @@ -734,7 +734,7 @@ int ep93xx_keypad_acquire_gpio(struct pl
>>>  fail_gpio_d:
>>>  	gpio_free(EP93XX_GPIO_LINE_C(i));
>>>  fail_gpio_c:
>>> -	for ( ; i >= 0; --i) {
>>> +	for (--i; i >= 0; --i) {
>>>  		gpio_free(EP93XX_GPIO_LINE_C(i));
>>>  		gpio_free(EP93XX_GPIO_LINE_D(i));
>>>  	}
>>>
>>> This way we don't double free EP93XX_GPIO_LINE_C(i), and don't free lines which were not
>>> successfully acquired (I noticed this when writing my patch, which is based on
>>> ep93xx_keypad_acquire/release_gpio).
>>
>> You are correct... ;-)
>>
>> Care to submit an actual patch?
>
> Note, if/when the devm_gpio_request patch gets into the kernel tree we can use
> that and get rid of the gpio_free's completely.
>
> Sorry, I don't have a link to that patch...

I just pulled linux-next. It looks like the devm_gpio_* stuff is now in.

commit 1a0703ede4493bd74f9c6b53f782b749e175a88e
Author: John Crispin <blogic@openwrt.org>
Date:   Tue Dec 20 21:40:21 2011 +0100

    GPIO: add bindings for managed devices
    
    This patch adds 2 functions that allow managed devices to request GPIOs.
    These GPIOs will then be managed by drivers/base/devres.c.
    
    Signed-off-by: John Crispin <blogic@openwrt.org>
    Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Regards,
Hartley

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

Patch

Index: linux-2.6/arch/arm/mach-ep93xx/core.c
===================================================================
--- linux-2.6.orig/arch/arm/mach-ep93xx/core.c
+++ linux-2.6/arch/arm/mach-ep93xx/core.c
@@ -734,7 +734,7 @@  int ep93xx_keypad_acquire_gpio(struct pl
 fail_gpio_d:
 	gpio_free(EP93XX_GPIO_LINE_C(i));
 fail_gpio_c:
-	for ( ; i >= 0; --i) {
+	for (--i; i >= 0; --i) {
 		gpio_free(EP93XX_GPIO_LINE_C(i));
 		gpio_free(EP93XX_GPIO_LINE_D(i));
 	}