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

[Denis Kuzmenko <linux@solonet.org.ua>]

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.