Messages in this thread Patch in this message | | | Date | Fri, 18 Nov 2011 23:00:53 +0200 | From | Denis Kuzmenko <> | Subject | Re: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib |
| |
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.
| |