[PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Denis Kuzmenko]

Make s3c24xx LEDS driver use gpiolib.

Signed-off-by: Denis Kuzmenko <linux@solonet.org.ua>
---

Patch is against 3.0.9
Tested on Mini2440 board which implements most complex case where both
S3C24XX_LEDF_ACTLOW and S3C24XX_LEDF_TRISTATE are set

diff --git a/drivers/leds/leds-s3c24xx.c b/drivers/leds/leds-s3c24xx.c
index a77771d..b36f123 100644
--- a/drivers/leds/leds-s3c24xx.c
+++ b/drivers/leds/leds-s3c24xx.c
@@ -40,28 +40,40 @@ static inline struct s3c24xx_gpio_led
*to_gpio(struct led_classdev *led_cdev)
 }

 static void s3c24xx_led_set(struct led_classdev *led_cdev,
-			    enum led_brightness value)
+				enum led_brightness value)
 {
 	struct s3c24xx_gpio_led *led = to_gpio(led_cdev);
 	struct s3c24xx_led_platdata *pd = led->pdata;

-	/* there will be a short delay between setting the output and
-	 * going from output to input when using tristate. */
-
-	s3c2410_gpio_setpin(pd->gpio, (value ? 1 : 0) ^
-			    (pd->flags & S3C24XX_LEDF_ACTLOW));
-
-	if (pd->flags & S3C24XX_LEDF_TRISTATE)
-		s3c2410_gpio_cfgpin(pd->gpio,
-			value ? S3C2410_GPIO_OUTPUT : S3C2410_GPIO_INPUT);
+	/*
+	 * ensure value is 0 or 1 to use it with bitwise XOR (^)
+	 * (only 100% brightness is supported)
+	 */
+	value = value ? 1 : 0;
+
+	if (pd->flags & S3C24XX_LEDF_TRISTATE) {
+		if (value) {
+			/* invert value if S3C24XX_LEDF_ACTLOW is set */
+			value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
+			gpio_direction_output(pd->gpio, value);
+		} else {
+			gpio_direction_input(pd->gpio);
+		}
+	} else {
+		/* invert value if S3C24XX_LEDF_ACTLOW is set */
+		value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
+		gpio_set_value(pd->gpio, value);
+	}

 }

 static int s3c24xx_led_remove(struct platform_device *dev)
 {
+	struct s3c24xx_led_platdata *pdata = dev->dev.platform_data;
 	struct s3c24xx_gpio_led *led = pdev_to_gpio(dev);

 	led_classdev_unregister(&led->cdev);
+	gpio_free(pdata->gpio);
 	kfree(led);

 	return 0;
@@ -76,7 +88,8 @@ static int s3c24xx_led_probe(struct platform_device *dev)
 	led = kzalloc(sizeof(struct s3c24xx_gpio_led), GFP_KERNEL);
 	if (led == NULL) {
 		dev_err(&dev->dev, "No memory for device\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_kzalloc;
 	}

 	platform_set_drvdata(dev, led);
@@ -91,12 +104,15 @@ static int s3c24xx_led_probe(struct platform_device
*dev)
 	/* no point in having a pull-up if we are always driving */

 	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
-		s3c2410_gpio_setpin(pdata->gpio, 0);
-		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
+		ret = gpio_request_one(pdata->gpio, GPIOF_IN, pdata->name);
 	} else {
-		s3c2410_gpio_pullup(pdata->gpio, 0);
-		s3c2410_gpio_setpin(pdata->gpio, 0);
-		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_OUTPUT);
+		ret = gpio_request_one(pdata->gpio, GPIOF_OUT_INIT_LOW,
+												pdata->name);
+		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);
+	}
+	if (ret < 0) {
+		dev_err(&dev->dev, "gpio_request failed\n");
+		goto err_gpio_request;
 	}

 	/* register our new led device */
@@ -104,11 +120,17 @@ static int s3c24xx_led_probe(struct
platform_device *dev)
 	ret = led_classdev_register(&dev->dev, &led->cdev);
 	if (ret < 0) {
 		dev_err(&dev->dev, "led_classdev_register failed\n");
-		kfree(led);
-		return ret;
+		goto err_led_classdev_register;
 	}

 	return 0;
+
+err_led_classdev_register:
+		gpio_free(pdata->gpio);
+err_gpio_request:
+		kfree(led);
+err_kzalloc:
+		return ret;
 }

 static struct platform_driver s3c24xx_led_driver = {

RE: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Stephen Warren]

Denis Kuzmenko wrote at Thursday, November 17, 2011 1:47 PM:
> Make s3c24xx LEDS driver use gpiolib.

Structurally this looks fine to me, but I made some slightly nit-picky
comments below.

> diff --git a/drivers/leds/leds-s3c24xx.c b/drivers/leds/leds-s3c24xx.c

>  static void s3c24xx_led_set(struct led_classdev *led_cdev,
> -			    enum led_brightness value)
> +				enum led_brightness value)

Seems unnecessary, but is probably fine.

>  {
>  	struct s3c24xx_gpio_led *led = to_gpio(led_cdev);
>  	struct s3c24xx_led_platdata *pd = led->pdata;
> 
> -	/* there will be a short delay between setting the output and
> -	 * going from output to input when using tristate. */
> -
> -	s3c2410_gpio_setpin(pd->gpio, (value ? 1 : 0) ^
> -			    (pd->flags & S3C24XX_LEDF_ACTLOW));
> -
> -	if (pd->flags & S3C24XX_LEDF_TRISTATE)
> -		s3c2410_gpio_cfgpin(pd->gpio,
> -			value ? S3C2410_GPIO_OUTPUT : S3C2410_GPIO_INPUT);
> +	/*
> +	 * ensure value is 0 or 1 to use it with bitwise XOR (^)
> +	 * (only 100% brightness is supported)
> +	 */
> +	value = value ? 1 : 0;
> +
> +	if (pd->flags & S3C24XX_LEDF_TRISTATE) {
> +		if (value) {
> +			/* invert value if S3C24XX_LEDF_ACTLOW is set */
> +			value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
> +			gpio_direction_output(pd->gpio, value);
> +		} else {
> +			gpio_direction_input(pd->gpio);
> +		}
> +	} else {
> +		/* invert value if S3C24XX_LEDF_ACTLOW is set */
> +		value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
> +		gpio_set_value(pd->gpio, value);
> +	}

I'd be tempted to simplify the new code a little:

	/* invert value if S3C24XX_LEDF_ACTLOW is set */
	value = !!(pd->flags & S3C24XX_LEDF_ACTLOW) ^ !!value;

	if (pd->flags & S3C24XX_LEDF_TRISTATE) {
		if (value)
			gpio_direction_output(pd->gpio, value);
		else
			gpio_direction_input(pd->gpio);
	} else {
		gpio_set_value(pd->gpio, value);
	}

> @@ -76,7 +88,8 @@ static int s3c24xx_led_probe(struct platform_device *dev)
>  	led = kzalloc(sizeof(struct s3c24xx_gpio_led), GFP_KERNEL);
>  	if (led == NULL) {
>  		dev_err(&dev->dev, "No memory for device\n");
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_kzalloc;
>  	}

That works fine, but isn't strictly necessary; no previous allocations
have been made here that need to be undone.

> @@ -91,12 +104,15 @@ static int s3c24xx_led_probe(struct platform_device
> *dev)
>  	/* no point in having a pull-up if we are always driving */
> 
>  	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
> -		s3c2410_gpio_setpin(pdata->gpio, 0);
> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
> +		ret = gpio_request_one(pdata->gpio, GPIOF_IN, pdata->name);
>  	} else {
> -		s3c2410_gpio_pullup(pdata->gpio, 0);
> -		s3c2410_gpio_setpin(pdata->gpio, 0);
> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_OUTPUT);
> +		ret = gpio_request_one(pdata->gpio, GPIOF_OUT_INIT_LOW,
> +												pdata->name);
> +		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);
> +	}

I always prefer not to duplicate function calls, but rather to calculate
the differing data (either directly in the call, or into a temporary
variable first), so:

	ret = gpio_request_one(pdata->gpio, 
				(pdata->flags & S3C24XX_LEDF_TRISTATE) ?
				GPIOF_IN : GPIOF_OUT_INIT_LOW,
				pdata->name);

	if (!(pdata->flags & S3C24XX_LEDF_TRISTATE))
		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);


> +	if (ret < 0) {
> +		dev_err(&dev->dev, "gpio_request failed\n");
> +		goto err_gpio_request;
>  	}

You should probably move that error check right after calling
gpio_request_one()

-- 
nvpublic

Re: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Denis Kuzmenko]

On 11/18/2011 07:08 PM, Stephen Warren wrote:
> Denis Kuzmenko wrote at Thursday, November 17, 2011 1:47 PM:
>> Make s3c24xx LEDS driver use gpiolib.
>
> I made some slightly nit-picky
> comments below.

Thanks for looking my patch.

>> diff --git a/drivers/leds/leds-s3c24xx.c b/drivers/leds/leds-s3c24xx.c
> 
>>  static void s3c24xx_led_set(struct led_classdev *led_cdev,
>> -			    enum led_brightness value)
>> +				enum led_brightness value)
> 
> Seems unnecessary, but is probably fine.

That was made unintentionally - will fix in next version.

>>  {
>>  	struct s3c24xx_gpio_led *led = to_gpio(led_cdev);
>>  	struct s3c24xx_led_platdata *pd = led->pdata;
>>
>> -	/* there will be a short delay between setting the output and
>> -	 * going from output to input when using tristate. */
>> -
>> -	s3c2410_gpio_setpin(pd->gpio, (value ? 1 : 0) ^
>> -			    (pd->flags & S3C24XX_LEDF_ACTLOW));
>> -
>> -	if (pd->flags & S3C24XX_LEDF_TRISTATE)
>> -		s3c2410_gpio_cfgpin(pd->gpio,
>> -			value ? S3C2410_GPIO_OUTPUT : S3C2410_GPIO_INPUT);
>> +	/*
>> +	 * ensure value is 0 or 1 to use it with bitwise XOR (^)
>> +	 * (only 100% brightness is supported)
>> +	 */
>> +	value = value ? 1 : 0;
>> +
>> +	if (pd->flags & S3C24XX_LEDF_TRISTATE) {
>> +		if (value) {
>> +			/* invert value if S3C24XX_LEDF_ACTLOW is set */
>> +			value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
>> +			gpio_direction_output(pd->gpio, value);
>> +		} else {
>> +			gpio_direction_input(pd->gpio);
>> +		}
>> +	} else {
>> +		/* invert value if S3C24XX_LEDF_ACTLOW is set */
>> +		value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
>> +		gpio_set_value(pd->gpio, value);
>> +	}
> 
> I'd be tempted to simplify the new code a little:
> 
> 	/* invert value if S3C24XX_LEDF_ACTLOW is set */
> 	value = !!(pd->flags & S3C24XX_LEDF_ACTLOW) ^ !!value;
> 
> 	if (pd->flags & S3C24XX_LEDF_TRISTATE) {
> 		if (value)
> 			gpio_direction_output(pd->gpio, value);
> 		else
> 			gpio_direction_input(pd->gpio);
> 	} else {
> 		gpio_set_value(pd->gpio, value);
> 	}

I've almost broke my mind writing this part and you've repeated my
mistake: in line where 'value' is checked ( if(value) ) the 'value'
shouldn't be inverted independently of S3C24XX_LEDF_ACTLOW flag.
This because S3C24XX_LEDF_TRISTATE means "tristate to turn off"
(arch/arm/mach-s3c2410/include/mach/leds-gpio.h:18) - that produces all
of complexity. Hope my description is understandable (if not, I'm sorry
- my English is too bad for this).

>> @@ -76,7 +88,8 @@ static int s3c24xx_led_probe(struct platform_device *dev)
>>  	led = kzalloc(sizeof(struct s3c24xx_gpio_led), GFP_KERNEL);
>>  	if (led == NULL) {
>>  		dev_err(&dev->dev, "No memory for device\n");
>> -		return -ENOMEM;
>> +		ret = -ENOMEM;
>> +		goto err_kzalloc;
>>  	}
>
> That works fine, but isn't strictly necessary; no previous allocations
> have been made here that need to be undone.
> 

I tried to use same error handling approach in all code, but you are
right - I've missed that in this place we can return safely and not
loosing much of code readability. _But_ this violates approach of not
having multiple returns unless you *really* need this. Still in doubt...

>> @@ -91,12 +104,15 @@ static int s3c24xx_led_probe(struct platform_device
>> *dev)
>>  	/* no point in having a pull-up if we are always driving */
>>
>>  	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
>> -		s3c2410_gpio_setpin(pdata->gpio, 0);
>> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
>> +		ret = gpio_request_one(pdata->gpio, GPIOF_IN, pdata->name);
>>  	} else {
>> -		s3c2410_gpio_pullup(pdata->gpio, 0);
>> -		s3c2410_gpio_setpin(pdata->gpio, 0);
>> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_OUTPUT);
>> +		ret = gpio_request_one(pdata->gpio, GPIOF_OUT_INIT_LOW,
>> +												pdata->name);
>> +		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);
>> +	}
> 
> I always prefer not to duplicate function calls, but rather to calculate
> the differing data (either directly in the call, or into a temporary
> variable first), so:
> 
> 	ret = gpio_request_one(pdata->gpio, 
> 				(pdata->flags & S3C24XX_LEDF_TRISTATE) ?
> 				GPIOF_IN : GPIOF_OUT_INIT_LOW,
> 				pdata->name);
> 
> 	if (!(pdata->flags & S3C24XX_LEDF_TRISTATE))
> 		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);
> 
> 
>> +	if (ret < 0) {
>> +		dev_err(&dev->dev, "gpio_request failed\n");
>> +		goto err_gpio_request;
>>  	}
> 
> You should probably move that error check right after calling
> gpio_request_one()
> 

I see no big difference between those two variants, but:
  1. my code looks for more readable
  2. your code allows not to call 's3c_gpio_setpull' in case of
gpio_request fail which looks like the _only_ usable variant.

Besides that, I've made a mistake changing

s3c2410_gpio_pullup(pdata->gpio, 0)
to
s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE)

because first variant actually means *enable* pull, trying first UP and,
if fail, DOWN direction. So pull-resistor is enabled in this case but in
some *random* direction.

The only use case for pull-resistor I see here is not to left pin
floating if someone configured _tristate_off_ LED on pin which actually
don't have it (LED or anything other connected). Considering this and a
fact that pullup is default enabled I think that it's safe to remove
it's configuration at all and left code in my variant. Or to add
pull-resistor enabling code for *opposite* case (S3C24XX_LEDF_TRISTATE).

-- 
Best regards, Denis Kuzmenko.

Re: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Denis Kuzmenko]

On 11/18/2011 11:00 PM, Denis Kuzmenko wrote:
> On 11/18/2011 07:08 PM, Stephen Warren wrote:
>> Denis Kuzmenko wrote at Thursday, November 17, 2011 1:47 PM:
>>> Make s3c24xx LEDS driver use gpiolib.
>>
>> I made some slightly nit-picky
>> comments below.

How's that? (some comments are taken to account and problem with pull-resistor fixed)

---
Make s3c24xx LEDS driver use gpiolib. Disable using pull-resistor when not
using S3C24XX_LEDF_TRISTATE and enble it when in opposite case.

Signed-off-by: Denis Kuzmenko <linux@solonet.org.ua>
---

Patch is against 3.0.9
Tested on Mini2440 board which implements most complex case where both 
S3C24XX_LEDF_ACTLOW and S3C24XX_LEDF_TRISTATE are set

diff --git a/drivers/leds/leds-s3c24xx.c b/drivers/leds/leds-s3c24xx.c
index a77771d..02bd6a8 100644
--- a/drivers/leds/leds-s3c24xx.c
+++ b/drivers/leds/leds-s3c24xx.c
@@ -45,23 +45,35 @@ static void s3c24xx_led_set(struct led_classdev *led_cdev,
 	struct s3c24xx_gpio_led *led = to_gpio(led_cdev);
 	struct s3c24xx_led_platdata *pd = led->pdata;
 
-	/* there will be a short delay between setting the output and
-	 * going from output to input when using tristate. */
-
-	s3c2410_gpio_setpin(pd->gpio, (value ? 1 : 0) ^
-			    (pd->flags & S3C24XX_LEDF_ACTLOW));
-
-	if (pd->flags & S3C24XX_LEDF_TRISTATE)
-		s3c2410_gpio_cfgpin(pd->gpio,
-			value ? S3C2410_GPIO_OUTPUT : S3C2410_GPIO_INPUT);
+	/*
+	 * ensure value is 0 or 1 to use it with bitwise XOR (^)
+	 * (only 100% brightness is supported)
+	 */
+	value = value ? 1 : 0;
+
+	if (pd->flags & S3C24XX_LEDF_TRISTATE) {
+		if (value) {
+			/* invert value if S3C24XX_LEDF_ACTLOW is set */
+			value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
+			gpio_direction_output(pd->gpio, value);
+		} else {
+			gpio_direction_input(pd->gpio);
+		}
+	} else {
+		/* invert value if S3C24XX_LEDF_ACTLOW is set */
+		value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
+		gpio_set_value(pd->gpio, value);
+	}
 
 }
 
 static int s3c24xx_led_remove(struct platform_device *dev)
 {
+	struct s3c24xx_led_platdata *pdata = dev->dev.platform_data;
 	struct s3c24xx_gpio_led *led = pdev_to_gpio(dev);
 
 	led_classdev_unregister(&led->cdev);
+	gpio_free(pdata->gpio);
 	kfree(led);
 
 	return 0;
@@ -76,7 +88,8 @@ static int s3c24xx_led_probe(struct platform_device *dev)
 	led = kzalloc(sizeof(struct s3c24xx_gpio_led), GFP_KERNEL);
 	if (led == NULL) {
 		dev_err(&dev->dev, "No memory for device\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_kzalloc;
 	}
 
 	platform_set_drvdata(dev, led);
@@ -88,15 +101,34 @@ static int s3c24xx_led_probe(struct platform_device *dev)
 
 	led->pdata = pdata;
 
-	/* no point in having a pull-up if we are always driving */
+	ret = gpio_request(pdata->gpio, pdata->name);
+	if (ret < 0) {
+		dev_err(&dev->dev, "gpio_request failed\n");
+		goto err_gpio_request;
+	}
 
 	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
-		s3c2410_gpio_setpin(pdata->gpio, 0);
-		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
+		/*
+		 * pull is needed here to protect pin from being left
+		 * floating
+		 */
+		ret = s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_UP);
+		if (ret)
+			s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_DOWN);
+		/* initially turn off led */
+		ret = gpio_direction_input(pdata->gpio);
 	} else {
-		s3c2410_gpio_pullup(pdata->gpio, 0);
-		s3c2410_gpio_setpin(pdata->gpio, 0);
-		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_OUTPUT);
+		/* no point in having a pull-up as we are always driving */
+		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);
+		ret = gpio_request_one(pdata->gpio, GPIOF_OUT_INIT_LOW,
+								pdata->name);
+		/* initially turn off led */
+		ret = gpio_direction_output(pdata->gpio,
+				!!(pdata->flags & S3C24XX_LEDF_ACTLOW));
+	}
+	if (ret < 0) {
+		dev_err(&dev->dev, "can't set gpio direction\n");
+		goto err_gpio_set_direction;
 	}
 
 	/* register our new led device */
@@ -104,11 +136,18 @@ static int s3c24xx_led_probe(struct platform_device *dev)
 	ret = led_classdev_register(&dev->dev, &led->cdev);
 	if (ret < 0) {
 		dev_err(&dev->dev, "led_classdev_register failed\n");
-		kfree(led);
-		return ret;
+		goto err_led_classdev_register;
 	}
 
 	return 0;
+
+err_led_classdev_register:
+err_gpio_set_direction:
+		gpio_free(pdata->gpio);
+err_gpio_request:
+		kfree(led);
+err_kzalloc:
+		return ret;
 }
 
 static struct platform_driver s3c24xx_led_driver = {

RE: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Stephen Warren]

Denis Kuzmenko wrote at Friday, November 18, 2011 2:01 PM:
> On 11/18/2011 07:08 PM, Stephen Warren wrote:
> > Denis Kuzmenko wrote at Thursday, November 17, 2011 1:47 PM:
> >> Make s3c24xx LEDS driver use gpiolib.
> >
> > I made some slightly nit-picky
> > comments below.
> 
> Thanks for looking my patch.
> 
> >> diff --git a/drivers/leds/leds-s3c24xx.c b/drivers/leds/leds-s3c24xx.c
> >
> >>  static void s3c24xx_led_set(struct led_classdev *led_cdev,
> >> -			    enum led_brightness value)
> >> +				enum led_brightness value)
> >
> > Seems unnecessary, but is probably fine.
> 
> That was made unintentionally - will fix in next version.
> 
> >>  {
> >>  	struct s3c24xx_gpio_led *led = to_gpio(led_cdev);
> >>  	struct s3c24xx_led_platdata *pd = led->pdata;
> >>
> >> -	/* there will be a short delay between setting the output and
> >> -	 * going from output to input when using tristate. */
> >> -
> >> -	s3c2410_gpio_setpin(pd->gpio, (value ? 1 : 0) ^
> >> -			    (pd->flags & S3C24XX_LEDF_ACTLOW));
> >> -
> >> -	if (pd->flags & S3C24XX_LEDF_TRISTATE)
> >> -		s3c2410_gpio_cfgpin(pd->gpio,
> >> -			value ? S3C2410_GPIO_OUTPUT : S3C2410_GPIO_INPUT);
> >> +	/*
> >> +	 * ensure value is 0 or 1 to use it with bitwise XOR (^)
> >> +	 * (only 100% brightness is supported)
> >> +	 */
> >> +	value = value ? 1 : 0;
> >> +
> >> +	if (pd->flags & S3C24XX_LEDF_TRISTATE) {
> >> +		if (value) {
> >> +			/* invert value if S3C24XX_LEDF_ACTLOW is set */
> >> +			value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
> >> +			gpio_direction_output(pd->gpio, value);
> >> +		} else {
> >> +			gpio_direction_input(pd->gpio);
> >> +		}
> >> +	} else {
> >> +		/* invert value if S3C24XX_LEDF_ACTLOW is set */
> >> +		value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
> >> +		gpio_set_value(pd->gpio, value);
> >> +	}
> >
> > I'd be tempted to simplify the new code a little:
> >
> > 	/* invert value if S3C24XX_LEDF_ACTLOW is set */
> > 	value = !!(pd->flags & S3C24XX_LEDF_ACTLOW) ^ !!value;
> >
> > 	if (pd->flags & S3C24XX_LEDF_TRISTATE) {
> > 		if (value)
> > 			gpio_direction_output(pd->gpio, value);
> > 		else
> > 			gpio_direction_input(pd->gpio);
> > 	} else {
> > 		gpio_set_value(pd->gpio, value);
> > 	}
> 
> I've almost broke my mind writing this part and you've repeated my
> mistake: in line where 'value' is checked ( if(value) ) the 'value'
> shouldn't be inverted independently of S3C24XX_LEDF_ACTLOW flag.
> This because S3C24XX_LEDF_TRISTATE means "tristate to turn off"
> (arch/arm/mach-s3c2410/include/mach/leds-gpio.h:18) - that produces all
> of complexity. Hope my description is understandable (if not, I'm sorry
> - my English is too bad for this).

Oh right yes. I guess you'd need to make the assignment to a second
variable instead of over-writing value then. Given that, your original
code is probably fine.

> >> @@ -76,7 +88,8 @@ static int s3c24xx_led_probe(struct platform_device *dev)
> >>  	led = kzalloc(sizeof(struct s3c24xx_gpio_led), GFP_KERNEL);
> >>  	if (led == NULL) {
> >>  		dev_err(&dev->dev, "No memory for device\n");
> >> -		return -ENOMEM;
> >> +		ret = -ENOMEM;
> >> +		goto err_kzalloc;
> >>  	}
> >
> > That works fine, but isn't strictly necessary; no previous allocations
> > have been made here that need to be undone.
> 
> I tried to use same error handling approach in all code, but you are
> right - I've missed that in this place we can return safely and not
> loosing much of code readability. _But_ this violates approach of not
> having multiple returns unless you *really* need this. Still in doubt...
> 
> >> @@ -91,12 +104,15 @@ static int s3c24xx_led_probe(struct platform_device
> >> *dev)
> >>  	/* no point in having a pull-up if we are always driving */
> >>
> >>  	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
> >> -		s3c2410_gpio_setpin(pdata->gpio, 0);
> >> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
> >> +		ret = gpio_request_one(pdata->gpio, GPIOF_IN, pdata->name);
> >>  	} else {
> >> -		s3c2410_gpio_pullup(pdata->gpio, 0);
> >> -		s3c2410_gpio_setpin(pdata->gpio, 0);
> >> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_OUTPUT);
> >> +		ret = gpio_request_one(pdata->gpio, GPIOF_OUT_INIT_LOW,
> >> +												pdata->name);
> >> +		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);
> >> +	}
> >
> > I always prefer not to duplicate function calls, but rather to calculate
> > the differing data (either directly in the call, or into a temporary
> > variable first), so:
> >
> > 	ret = gpio_request_one(pdata->gpio,
> > 				(pdata->flags & S3C24XX_LEDF_TRISTATE) ?
> > 				GPIOF_IN : GPIOF_OUT_INIT_LOW,
> > 				pdata->name);
> >
> > 	if (!(pdata->flags & S3C24XX_LEDF_TRISTATE))
> > 		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);
> >
> >
> >> +	if (ret < 0) {
> >> +		dev_err(&dev->dev, "gpio_request failed\n");
> >> +		goto err_gpio_request;
> >>  	}
> >
> > You should probably move that error check right after calling
> > gpio_request_one()
> >
> 
> I see no big difference between those two variants, but:
>   1. my code looks for more readable

I actually wrote my comment because I found the function call duplication
less readable. But, this is probably personal preference. Using a temporary
variable rather than a ternary operator in the function call might help.

>   2. your code allows not to call 's3c_gpio_setpull' in case of
> gpio_request fail which looks like the _only_ usable variant.
> 
> Besides that, I've made a mistake changing
> 
> s3c2410_gpio_pullup(pdata->gpio, 0)
> to
> s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE)
> 
> because first variant actually means *enable* pull, trying first UP and,
> if fail, DOWN direction. So pull-resistor is enabled in this case but in
> some *random* direction.
> 
> The only use case for pull-resistor I see here is not to left pin
> floating if someone configured _tristate_off_ LED on pin which actually
> don't have it (LED or anything other connected). Considering this and a
> fact that pullup is default enabled I think that it's safe to remove
> it's configuration at all and left code in my variant. Or to add
> pull-resistor enabling code for *opposite* case (S3C24XX_LEDF_TRISTATE).

-- 
nvpublic

RE: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Stephen Warren]

Denis Kuzmenko wrote at Friday, November 18, 2011 2:45 PM:
> Make s3c24xx LEDS driver use gpiolib. Disable using pull-resistor when not
> using S3C24XX_LEDF_TRISTATE and enble it when in opposite case.
> 
> Signed-off-by: Denis Kuzmenko <linux@solonet.org.ua>

>  	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
> -		s3c2410_gpio_setpin(pdata->gpio, 0);
> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
> +		/*
> +		 * pull is needed here to protect pin from being left
> +		 * floating
> +		 */
> +		ret = s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_UP);
> +		if (ret)
> +			s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_DOWN);

Sorry, could you explain why it's appropriate to configure a pull here
at all, let alone why it's OK to have a random pull on the line?

> +		/* initially turn off led */
> +		ret = gpio_direction_input(pdata->gpio);
>  	} else {
> -		s3c2410_gpio_pullup(pdata->gpio, 0);
> -		s3c2410_gpio_setpin(pdata->gpio, 0);
> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_OUTPUT);
> +		/* no point in having a pull-up as we are always driving */
> +		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);
> +		ret = gpio_request_one(pdata->gpio, GPIOF_OUT_INIT_LOW,
> +								pdata->name);
> +		/* initially turn off led */
> +		ret = gpio_direction_output(pdata->gpio,
> +				!!(pdata->flags & S3C24XX_LEDF_ACTLOW));

Wouldn't it be better to pass GPIOF_OUT_INIT_HIGH or GPIOF_OUT_INIT_LOW
to gpio_request_one() based on S3C24XX_LEDF_ACTLOW, and avoid the call to
gpio_direction_output; that will avoid glitching the line for a short time.

I'm fine with the rest of the code.

-- 
nvpublic

Re: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Denis Kuzmenko]

On 11/18/2011 11:59 PM, Stephen Warren wrote:
> Denis Kuzmenko wrote at Friday, November 18, 2011 2:45 PM:
>> Make s3c24xx LEDS driver use gpiolib. Disable using pull-resistor when not
>> using S3C24XX_LEDF_TRISTATE and enble it when in opposite case.
>>
>> Signed-off-by: Denis Kuzmenko <linux@solonet.org.ua>
> 
>>  	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
>> -		s3c2410_gpio_setpin(pdata->gpio, 0);
>> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
>> +		/*
>> +		 * pull is needed here to protect pin from being left
>> +		 * floating
>> +		 */
>> +		ret = s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_UP);
>> +		if (ret)
>> +			s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_DOWN);
> 
> Sorry, could you explain why it's appropriate to configure a pull here
> at all, let alone why it's OK to have a random pull on the line?
> 

Of course I'll explain.
Imagine you are working with generic GPIO lines on your board connecting
and disconnecting LEDs and other stuff. In this case there can be
situation where GPIO line is configured as TRISTATE LED but have nothing
connected physically to pin. This configuration is dangerous because
input pin without _any_ pull-resistor is _much_ more sensitive to
statical electricity (ESD) so you can *burn* (unsure this is correct
word) your pin much easily (especially is you are using soldering iron
as much as I do). Most of GPIO modules I worked with have "input with
pull-up" as default and most safe initial state (and s3c2440's one is
not an exception).
Maybe, I need to write more wide exlanation in comment above?

>> +		/* initially turn off led */
>> +		ret = gpio_direction_input(pdata->gpio);
>>  	} else {
>> -		s3c2410_gpio_pullup(pdata->gpio, 0);
>> -		s3c2410_gpio_setpin(pdata->gpio, 0);
>> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_OUTPUT);
>> +		/* no point in having a pull-up as we are always driving */
>> +		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);
>> +		ret = gpio_request_one(pdata->gpio, GPIOF_OUT_INIT_LOW,
>> +								pdata->name);
>> +		/* initially turn off led */
>> +		ret = gpio_direction_output(pdata->gpio,
>> +				!!(pdata->flags & S3C24XX_LEDF_ACTLOW));
> 
> Wouldn't it be better to pass GPIOF_OUT_INIT_HIGH or GPIOF_OUT_INIT_LOW
> to gpio_request_one() based on S3C24XX_LEDF_ACTLOW, and avoid the call to
> gpio_direction_output; that will avoid glitching the line for a short time.
> 
> I'm fine with the rest of the code.
> 

Good finding! There shouldn't be a
	'gpio_request_one(...GPIOF_OUT_INIT_LOW...)'
at all. This is code from previous version since we have already
requested GPIO line and gpio_direction_output follows.
Sorry for that.

I'll send a new version.


-- 
Best regards, Denis Kuzmenko.

[PATCH v2] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Denis Kuzmenko]

Make s3c24xx LEDS driver use gpiolib. Disable using pull-resistor when not
using S3C24XX_LEDF_TRISTATE and enble it when in opposite case.

Signed-off-by: Denis Kuzmenko <linux@solonet.org.ua>
---

Patch is against 3.0.9
Tested on Mini2440 board which implements most complex case where both 
S3C24XX_LEDF_ACTLOW and S3C24XX_LEDF_TRISTATE are set

diff --git a/drivers/leds/leds-s3c24xx.c b/drivers/leds/leds-s3c24xx.c
index a77771d..63feda8 100644
--- a/drivers/leds/leds-s3c24xx.c
+++ b/drivers/leds/leds-s3c24xx.c
@@ -45,23 +45,35 @@ static void s3c24xx_led_set(struct led_classdev *led_cdev,
 	struct s3c24xx_gpio_led *led = to_gpio(led_cdev);
 	struct s3c24xx_led_platdata *pd = led->pdata;
 
-	/* there will be a short delay between setting the output and
-	 * going from output to input when using tristate. */
-
-	s3c2410_gpio_setpin(pd->gpio, (value ? 1 : 0) ^
-			    (pd->flags & S3C24XX_LEDF_ACTLOW));
-
-	if (pd->flags & S3C24XX_LEDF_TRISTATE)
-		s3c2410_gpio_cfgpin(pd->gpio,
-			value ? S3C2410_GPIO_OUTPUT : S3C2410_GPIO_INPUT);
+	/*
+	 * ensure value is 0 or 1 to use it with bitwise XOR (^)
+	 * (only 100% brightness is supported)
+	 */
+	value = value ? 1 : 0;
+
+	if (pd->flags & S3C24XX_LEDF_TRISTATE) {
+		if (value) {
+			/* invert value if S3C24XX_LEDF_ACTLOW is set */
+			value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
+			gpio_direction_output(pd->gpio, value);
+		} else {
+			gpio_direction_input(pd->gpio);
+		}
+	} else {
+		/* invert value if S3C24XX_LEDF_ACTLOW is set */
+		value = (pd->flags & S3C24XX_LEDF_ACTLOW) ^ value;
+		gpio_set_value(pd->gpio, value);
+	}
 
 }
 
 static int s3c24xx_led_remove(struct platform_device *dev)
 {
+	struct s3c24xx_led_platdata *pdata = dev->dev.platform_data;
 	struct s3c24xx_gpio_led *led = pdev_to_gpio(dev);
 
 	led_classdev_unregister(&led->cdev);
+	gpio_free(pdata->gpio);
 	kfree(led);
 
 	return 0;
@@ -76,7 +88,8 @@ static int s3c24xx_led_probe(struct platform_device *dev)
 	led = kzalloc(sizeof(struct s3c24xx_gpio_led), GFP_KERNEL);
 	if (led == NULL) {
 		dev_err(&dev->dev, "No memory for device\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_kzalloc;
 	}
 
 	platform_set_drvdata(dev, led);
@@ -88,15 +101,33 @@ static int s3c24xx_led_probe(struct platform_device *dev)
 
 	led->pdata = pdata;
 
-	/* no point in having a pull-up if we are always driving */
+	ret = gpio_request(pdata->gpio, pdata->name);
+	if (ret < 0) {
+		dev_err(&dev->dev, "gpio_request failed\n");
+		goto err_gpio_request;
+	}
 
 	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
-		s3c2410_gpio_setpin(pdata->gpio, 0);
-		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
+		/*
+		 * pull is needed here to protect pin from being left
+		 * floating (this is dangerous since pin becomes more
+		 * sensitive to ESD)
+		 */
+		ret = s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_UP);
+		if (ret)
+			s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_DOWN);
+		/* initially turn off led */
+		ret = gpio_direction_input(pdata->gpio);
 	} else {
-		s3c2410_gpio_pullup(pdata->gpio, 0);
-		s3c2410_gpio_setpin(pdata->gpio, 0);
-		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_OUTPUT);
+		/* no point in having a pull-up as we are always driving */
+		s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_NONE);
+		/* initially turn off led */
+		ret = gpio_direction_output(pdata->gpio,
+				!!(pdata->flags & S3C24XX_LEDF_ACTLOW));
+	}
+	if (ret < 0) {
+		dev_err(&dev->dev, "can't set gpio direction\n");
+		goto err_gpio_set_direction;
 	}
 
 	/* register our new led device */
@@ -104,11 +135,18 @@ static int s3c24xx_led_probe(struct platform_device *dev)
 	ret = led_classdev_register(&dev->dev, &led->cdev);
 	if (ret < 0) {
 		dev_err(&dev->dev, "led_classdev_register failed\n");
-		kfree(led);
-		return ret;
+		goto err_led_classdev_register;
 	}
 
 	return 0;
+
+err_led_classdev_register:
+err_gpio_set_direction:
+		gpio_free(pdata->gpio);
+err_gpio_request:
+		kfree(led);
+err_kzalloc:
+		return ret;
 }
 
 static struct platform_driver s3c24xx_led_driver = {

RE: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Stephen Warren]

Denis Kuzmenko wrote at Friday, November 18, 2011 3:35 PM:
> On 11/18/2011 11:59 PM, Stephen Warren wrote:
> > Denis Kuzmenko wrote at Friday, November 18, 2011 2:45 PM:
> >> Make s3c24xx LEDS driver use gpiolib. Disable using pull-resistor when not
> >> using S3C24XX_LEDF_TRISTATE and enble it when in opposite case.
> >>
> >> Signed-off-by: Denis Kuzmenko <linux@solonet.org.ua>
> >
> >>  	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
> >> -		s3c2410_gpio_setpin(pdata->gpio, 0);
> >> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
> >> +		/*
> >> +		 * pull is needed here to protect pin from being left
> >> +		 * floating
> >> +		 */
> >> +		ret = s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_UP);
> >> +		if (ret)
> >> +			s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_DOWN);
> >
> > Sorry, could you explain why it's appropriate to configure a pull here
> > at all, let alone why it's OK to have a random pull on the line?
> >
> 
> Of course I'll explain.
> Imagine you are working with generic GPIO lines on your board connecting
> and disconnecting LEDs and other stuff. In this case there can be
> situation where GPIO line is configured as TRISTATE LED but have nothing
> connected physically to pin. This configuration is dangerous because
> input pin without _any_ pull-resistor is _much_ more sensitive to
> statical electricity (ESD) so you can *burn* (unsure this is correct
> word) your pin much easily (especially is you are using soldering iron
> as much as I do). Most of GPIO modules I worked with have "input with
> pull-up" as default and most safe initial state (and s3c2440's one is
> not an exception).
> Maybe, I need to write more wide exlanation in comment above?

OK, I see the need for a pull of some kind (although aren't there meant
to be ESD protection diodes for this purpose; relying on what are probably
pretty weak pullup/down resistors doesn't seem like it will provide much
protection at all).

I have a slight feeling this detail should be hidden inside the gpiolib
driver.

Presumably the pull is pretty weak, so that if/when the pin is actively
driven later, the drive completely overrides this pull?

Is this pull strong enough to light the LED? Judging by what
s3c24xx_led_set() does, when the LED is off, the pin is tri-stated, and
when the LED is on, it's driven (high/low depending on the active high/low
flag). When tri-stated, if the pull happens to pull the same direction as
the on state would drive it, won't the LED light? It seems like you need
to pick a suitable pull direction based on flags & S3C24XX_LEDF_ACTLOW,
and if that can't be set, it's an error.

(BTW, when posting a new version, starting a new thread with just the
patch, rather than pasting it into a reply and prefixing it with other
text will make is easiest for people to take the patch and apply it)

-- 
nvpublic

Re: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Denis Kuzmenko]

On 11/19/2011 12:44 AM, Stephen Warren wrote:
> Denis Kuzmenko wrote at Friday, November 18, 2011 3:35 PM:
>> On 11/18/2011 11:59 PM, Stephen Warren wrote:
>>> Denis Kuzmenko wrote at Friday, November 18, 2011 2:45 PM:
>>>> Make s3c24xx LEDS driver use gpiolib. Disable using pull-resistor when not
>>>> using S3C24XX_LEDF_TRISTATE and enble it when in opposite case.
>>>>
>>>> Signed-off-by: Denis Kuzmenko <linux@solonet.org.ua>
>>>
>>>>  	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
>>>> -		s3c2410_gpio_setpin(pdata->gpio, 0);
>>>> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
>>>> +		/*
>>>> +		 * pull is needed here to protect pin from being left
>>>> +		 * floating
>>>> +		 */
>>>> +		ret = s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_UP);
>>>> +		if (ret)
>>>> +			s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_DOWN);
>>>
>>> Sorry, could you explain why it's appropriate to configure a pull here
>>> at all, let alone why it's OK to have a random pull on the line?
>>>
>>
>> Of course I'll explain.
>> Imagine you are working with generic GPIO lines on your board connecting
>> and disconnecting LEDs and other stuff. In this case there can be
>> situation where GPIO line is configured as TRISTATE LED but have nothing
>> connected physically to pin. This configuration is dangerous because
>> input pin without _any_ pull-resistor is _much_ more sensitive to
>> statical electricity (ESD) so you can *burn* (unsure this is correct
>> word) your pin much easily (especially is you are using soldering iron
>> as much as I do). Most of GPIO modules I worked with have "input with
>> pull-up" as default and most safe initial state (and s3c2440's one is
>> not an exception).
>> Maybe, I need to write more wide exlanation in comment above?
> 
> OK, I see the need for a pull of some kind (although aren't there meant
> to be ESD protection diodes for this purpose; relying on what are probably
> pretty weak pullup/down resistors doesn't seem like it will provide much
> protection at all).
> 

I don't mean pull as any kind of good protection. But it's much better
to have it than not.

> I have a slight feeling this detail should be hidden inside the gpiolib
> driver.
> 

Do you mean to add to a function that makes pin act as input some kind
of logic like:
	if(!(flags & FLAG_PULL_NONE))
		try_to_enable_pull(PULL_ANY);
?

> Presumably the pull is pretty weak, so that if/when the pin is actively
> driven later, the drive completely overrides this pull?
>

As far as know pull-resistor is _designed_ to be weak enough so driver
can always override it.

> Is this pull strong enough to light the LED? Judging by what
> s3c24xx_led_set() does, when the LED is off, the pin is tri-stated, and
> when the LED is on, it's driven (high/low depending on the active high/low
> flag). When tri-stated, if the pull happens to pull the same direction as
> the on state would drive it, won't the LED light? It seems like you need
> to pick a suitable pull direction based on flags & S3C24XX_LEDF_ACTLOW,
> and if that can't be set, it's an error.

I couldn't find any information about pull strength in s3c2440's
datasheet but usually pull is about 10-100k. So even in worst case
maximum current through it on 3.3V VCC is 0.33 mA (not taking into
account own LED's resistance and voltage-fall) which is definitely not
enough to light a LED.

> (BTW, when posting a new version, starting a new thread with just the
> patch, rather than pasting it into a reply and prefixing it with other
> text will make is easiest for people to take the patch and apply it)

Thank you, I'll do so in future.


-- 
Best regards, Denis Kuzmenko.

RE: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Stephen Warren]

Denis Kuzmenko wrote at Friday, November 18, 2011 4:17 PM:
> On 11/19/2011 12:44 AM, Stephen Warren wrote:
> > Denis Kuzmenko wrote at Friday, November 18, 2011 3:35 PM:
> >> On 11/18/2011 11:59 PM, Stephen Warren wrote:
> >>> Denis Kuzmenko wrote at Friday, November 18, 2011 2:45 PM:
> >>>> Make s3c24xx LEDS driver use gpiolib. Disable using pull-resistor when not
> >>>> using S3C24XX_LEDF_TRISTATE and enble it when in opposite case.
> >>>>
> >>>> Signed-off-by: Denis Kuzmenko <linux@solonet.org.ua>
> >>>
> >>>>  	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
> >>>> -		s3c2410_gpio_setpin(pdata->gpio, 0);
> >>>> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
> >>>> +		/*
> >>>> +		 * pull is needed here to protect pin from being left
> >>>> +		 * floating
> >>>> +		 */
> >>>> +		ret = s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_UP);
> >>>> +		if (ret)
> >>>> +			s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_DOWN);
> >>>
> >>> Sorry, could you explain why it's appropriate to configure a pull here
> >>> at all, let alone why it's OK to have a random pull on the line?
> >>
> >> Of course I'll explain.
> >> Imagine you are working with generic GPIO lines on your board connecting
> >> and disconnecting LEDs and other stuff. In this case there can be
> >> situation where GPIO line is configured as TRISTATE LED but have nothing
> >> connected physically to pin. This configuration is dangerous because
> >> input pin without _any_ pull-resistor is _much_ more sensitive to
> >> statical electricity (ESD) so you can *burn* (unsure this is correct
> >> word) your pin much easily (especially is you are using soldering iron
> >> as much as I do). Most of GPIO modules I worked with have "input with
> >> pull-up" as default and most safe initial state (and s3c2440's one is
> >> not an exception).
> >> Maybe, I need to write more wide exlanation in comment above?
> >
> > OK, I see the need for a pull of some kind (although aren't there meant
> > to be ESD protection diodes for this purpose; relying on what are probably
> > pretty weak pullup/down resistors doesn't seem like it will provide much
> > protection at all).
> 
> I don't mean pull as any kind of good protection. But it's much better
> to have it than not.

Hmm. I'm not entirely convinced. If the board already has a pull-up/down,
it seems like it won't really make much difference to ESD, and you can't
make any assumptions in the core driver about whether such an external
resistor is already present. In fact, adding another pull resistor inside
the SoC in parallel will reduce the overall resistance, and increase wasted
power.

> > I have a slight feeling this detail should be hidden inside the gpiolib
> > driver.
> 
> Do you mean to add to a function that makes pin act as input some kind
> of logic like:
> 	if(!(flags & FLAG_PULL_NONE))
> 		try_to_enable_pull(PULL_ANY);

I meant that /if/ the GPIO HW or SoC really requires this for safety, then
the implementation behind gpio_direction_input() should be doing this.
That said, it seems pretty magic to do this.

Can you get the SoC vendor and gpiolib implementor for this SoC to weigh in
on this and answer if "magically" enabling a tri-state is a good thing to
do?

-- 
nvpublic

Re: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Denis Kuzmenko]

On 11/21/2011 08:07 PM, Stephen Warren wrote:
> Denis Kuzmenko wrote at Friday, November 18, 2011 4:17 PM:
>> On 11/19/2011 12:44 AM, Stephen Warren wrote:
>>> Denis Kuzmenko wrote at Friday, November 18, 2011 3:35 PM:
>>>> On 11/18/2011 11:59 PM, Stephen Warren wrote:
>>>>> Denis Kuzmenko wrote at Friday, November 18, 2011 2:45 PM:
>>>>>> Make s3c24xx LEDS driver use gpiolib. Disable using pull-resistor when not
>>>>>> using S3C24XX_LEDF_TRISTATE and enble it when in opposite case.
>>>>>>
>>>>>> Signed-off-by: Denis Kuzmenko <linux@solonet.org.ua>
>>>>>
>>>>>>  	if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
>>>>>> -		s3c2410_gpio_setpin(pdata->gpio, 0);
>>>>>> -		s3c2410_gpio_cfgpin(pdata->gpio, S3C2410_GPIO_INPUT);
>>>>>> +		/*
>>>>>> +		 * pull is needed here to protect pin from being left
>>>>>> +		 * floating
>>>>>> +		 */
>>>>>> +		ret = s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_UP);
>>>>>> +		if (ret)
>>>>>> +			s3c_gpio_setpull(pdata->gpio, S3C_GPIO_PULL_DOWN);
>>>>>
>>>>> Sorry, could you explain why it's appropriate to configure a pull here
>>>>> at all, let alone why it's OK to have a random pull on the line?
>>>>
>>>> Of course I'll explain.
>>>> Imagine you are working with generic GPIO lines on your board connecting
>>>> and disconnecting LEDs and other stuff. In this case there can be
>>>> situation where GPIO line is configured as TRISTATE LED but have nothing
>>>> connected physically to pin. This configuration is dangerous because
>>>> input pin without _any_ pull-resistor is _much_ more sensitive to
>>>> statical electricity (ESD) so you can *burn* (unsure this is correct
>>>> word) your pin much easily (especially is you are using soldering iron
>>>> as much as I do). Most of GPIO modules I worked with have "input with
>>>> pull-up" as default and most safe initial state (and s3c2440's one is
>>>> not an exception).
>>>> Maybe, I need to write more wide exlanation in comment above?
>>>
>>> OK, I see the need for a pull of some kind (although aren't there meant
>>> to be ESD protection diodes for this purpose; relying on what are probably
>>> pretty weak pullup/down resistors doesn't seem like it will provide much
>>> protection at all).
>>
>> I don't mean pull as any kind of good protection. But it's much better
>> to have it than not.
> 
> Hmm. I'm not entirely convinced. If the board already has a pull-up/down,
> it seems like it won't really make much difference to ESD, and you can't
> make any assumptions in the core driver about whether such an external
> resistor is already present. In fact, adding another pull resistor inside
> the SoC in parallel will reduce the overall resistance, and increase wasted
> power.
> 

	I don't think it's a real protection. It's rather "mistake-proofing"
(Poka-Yoke).
	You are right, I didn't considered additional pulls (however I can't
imagine tristate LED usage with additional external pull) and power
consumptions.
	I was just wondering, why was pull needed in previous implementation.
Additional ESD protection was the only thing I could imagine. I don't
think it's needed there and I'm OK to remove pull-related code.
	So I'll remove it, test and send patch V3?


> I meant that /if/ the GPIO HW or SoC really requires this for safety, then
> the implementation behind gpio_direction_input() should be doing this.
> That said, it seems pretty magic to do this.
> 
> Can you get the SoC vendor and gpiolib implementor for this SoC to weigh in
> on this and answer if "magically" enabling a tri-state is a good thing to
> do?

I don't like magic things neither believe that it's possible to get
those people to this conversation. So lets just remove that code?

-- 
Best regards, Denis Kuzmenko.

RE: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Stephen Warren]

Denis Kuzmenko wrote at Monday, November 21, 2011 12:38 PM:
> On 11/21/2011 08:07 PM, Stephen Warren wrote:
> > Denis Kuzmenko wrote at Friday, November 18, 2011 4:17 PM:
> >> On 11/19/2011 12:44 AM, Stephen Warren wrote:
...
> >>> OK, I see the need for a pull of some kind (although aren't there meant
> >>> to be ESD protection diodes for this purpose; relying on what are probably
> >>> pretty weak pullup/down resistors doesn't seem like it will provide much
> >>> protection at all).
> >>
> >> I don't mean pull as any kind of good protection. But it's much better
> >> to have it than not.
> >
> > Hmm. I'm not entirely convinced. If the board already has a pull-up/down,
> > it seems like it won't really make much difference to ESD, and you can't
> > make any assumptions in the core driver about whether such an external
> > resistor is already present. In fact, adding another pull resistor inside
> > the SoC in parallel will reduce the overall resistance, and increase wasted
> > power.
> >
> 
> 	I don't think it's a real protection. It's rather "mistake-proofing"
> (Poka-Yoke).
> 	You are right, I didn't considered additional pulls (however I can't
> imagine tristate LED usage with additional external pull) and power
> consumptions.
> 	I was just wondering, why was pull needed in previous implementation.
> Additional ESD protection was the only thing I could imagine. I don't
> think it's needed there and I'm OK to remove pull-related code.
> 	So I'll remove it, test and send patch V3?

I don't see any pulls being configured in the original code at all,
unless some of the s3c2410_* function have unexpected side-effect. The
only related thing is in probe:

        /* no point in having a pull-up if we are always driving */

        if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
..
        } else {
                s3c2410_gpio_pullup(pdata->gpio, 0);

which I assume disables an pull in the case where the pin is always driven.

So, yes, I'd say submit v3 without any pull manipulation at all.

-- 
nvpublic

Re: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Denis Kuzmenko]

On 11/22/2011 12:03 AM, Stephen Warren wrote:
> Denis Kuzmenko wrote at Monday, November 21, 2011 12:38 PM:
>> On 11/21/2011 08:07 PM, Stephen Warren wrote:
>>> Denis Kuzmenko wrote at Friday, November 18, 2011 4:17 PM:
>>>> On 11/19/2011 12:44 AM, Stephen Warren wrote:
> ...
>>>>> OK, I see the need for a pull of some kind (although aren't there meant
>>>>> to be ESD protection diodes for this purpose; relying on what are probably
>>>>> pretty weak pullup/down resistors doesn't seem like it will provide much
>>>>> protection at all).
>>>>
>>>> I don't mean pull as any kind of good protection. But it's much better
>>>> to have it than not.
>>>
>>> Hmm. I'm not entirely convinced. If the board already has a pull-up/down,
>>> it seems like it won't really make much difference to ESD, and you can't
>>> make any assumptions in the core driver about whether such an external
>>> resistor is already present. In fact, adding another pull resistor inside
>>> the SoC in parallel will reduce the overall resistance, and increase wasted
>>> power.
>>>
>>
>> 	I don't think it's a real protection. It's rather "mistake-proofing"
>> (Poka-Yoke).
>> 	You are right, I didn't considered additional pulls (however I can't
>> imagine tristate LED usage with additional external pull) and power
>> consumptions.
>> 	I was just wondering, why was pull needed in previous implementation.
>> Additional ESD protection was the only thing I could imagine. I don't
>> think it's needed there and I'm OK to remove pull-related code.
>> 	So I'll remove it, test and send patch V3?
> 
> I don't see any pulls being configured in the original code at all,
> unless some of the s3c2410_* function have unexpected side-effect. The
> only related thing is in probe:
> 
>         /* no point in having a pull-up if we are always driving */
> 
>         if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
> ..
>         } else {
>                 s3c2410_gpio_pullup(pdata->gpio, 0);
> 
> which I assume disables an pull in the case where the pin is always driven.
> 
> So, yes, I'd say submit v3 without any pull manipulation at all.
> 

Actually, "s3c2410_gpio_pullup(pdata->gpio, 0);" enables pull in the
same way I've done that. Here is it's code:

/* gpiolib wrappers until these are totally eliminated */

void s3c2410_gpio_pullup(unsigned int pin, unsigned int to)
{
	int ret;

	WARN_ON(to);	/* should be none of these left */

	if (!to) {
		/* if pull is enabled, try first with up, and if that
		 * fails, try using down */

		ret = s3c_gpio_setpull(pin, S3C_GPIO_PULL_UP);
		if (ret)
			s3c_gpio_setpull(pin, S3C_GPIO_PULL_DOWN);
	} else {
		s3c_gpio_setpull(pin, S3C_GPIO_PULL_NONE);
	}
}

So pull is enabled in same "random" way as I did but for *opposite*
state of S3C24XX_LEDF_TRISTATE flag.
And again:

>> 	I was just wondering, why was pull needed in previous implementation.
>> Additional ESD protection was the only thing I could imagine. I don't
>> think it's needed there and I'm OK to remove pull-related code.
>> 	So I'll remove it, test and send patch V3?

-- 
Best regards, Denis Kuzmenko.

RE: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Stephen Warren]

Denis Kuzmenko wrote at Monday, November 21, 2011 3:52 PM:
...
> > I don't see any pulls being configured in the original code at all,
> > unless some of the s3c2410_* function have unexpected side-effect. The
> > only related thing is in probe:
> >
> >         /* no point in having a pull-up if we are always driving */
> >
> >         if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
> > ..
> >         } else {
> >                 s3c2410_gpio_pullup(pdata->gpio, 0);
> >
> > which I assume disables an pull in the case where the pin is always driven.
> >
> > So, yes, I'd say submit v3 without any pull manipulation at all.
> >
> 
> Actually, "s3c2410_gpio_pullup(pdata->gpio, 0);" enables pull in the
> same way I've done that. Here is it's code:

So it does. That's extremely non-obvious if not broken.

Anyway, I guess that means that your patch V1 is at least a pure conversion
of the code from custom functions to gpiolib, even if what it was and still
is doing doesn't make much sense to me.

-- 
nvpublic

Re: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Denis Kuzmenko]

On 11/22/2011 01:39 AM, Stephen Warren wrote:
> Denis Kuzmenko wrote at Monday, November 21, 2011 3:52 PM:
> ...
>>> I don't see any pulls being configured in the original code at all,
>>> unless some of the s3c2410_* function have unexpected side-effect. The
>>> only related thing is in probe:
>>>
>>>         /* no point in having a pull-up if we are always driving */
>>>
>>>         if (pdata->flags & S3C24XX_LEDF_TRISTATE) {
>>> ..
>>>         } else {
>>>                 s3c2410_gpio_pullup(pdata->gpio, 0);
>>>
>>> which I assume disables an pull in the case where the pin is always driven.
>>>
>>> So, yes, I'd say submit v3 without any pull manipulation at all.
>>>
>>
>> Actually, "s3c2410_gpio_pullup(pdata->gpio, 0);" enables pull in the
>> same way I've done that. Here is it's code:
> 
> So it does. That's extremely non-obvious if not broken.
> 
> Anyway, I guess that means that your patch V1 is at least a pure conversion
> of the code from custom functions to gpiolib, even if what it was and still
> is doing doesn't make much sense to me.
> 

So what do you suggest? Leave original behavior?
I'd like to remove pull-related actions at all considering:
 - power consumption
 - we are both not able to find any sense in that code
 - s3c2410_gpio_pullup looks like deprecated
 - can't imagine situation where this change will break functionality
(of course when this code used to drive actually LED and not something else)

-- 
Best regards, Denis Kuzmenko.

RE: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib

[Stephen Warren]

Denis Kuzmenko wrote at Monday, November 21, 2011 5:28 PM:
> On 11/22/2011 01:39 AM, Stephen Warren wrote:
...
> > Anyway, I guess that means that your patch V1 is at least a pure conversion
> > of the code from custom functions to gpiolib, even if what it was and still
> > is doing doesn't make much sense to me.
> >
> 
> So what do you suggest? Leave original behavior?
> I'd like to remove pull-related actions at all considering:
>  - power consumption
>  - we are both not able to find any sense in that code
>  - s3c2410_gpio_pullup looks like deprecated
>  - can't imagine situation where this change will break functionality
> (of course when this code used to drive actually LED and not something else)

Well, if that works for you, that makes sense to me too.

-- 
nvpublic