Messages in this thread Patch in this message | | | From | Stephen Warren <> | Date | Fri, 18 Nov 2011 09:08:39 -0800 | Subject | RE: [PATCH] s3c/s3c24xx: arm: leds: Make s3c24xx LEDS driver use gpiolib |
| |
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
| |