lkml.org 
[lkml]   [2009]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] LED: add driver for LT3593 controlled LEDs
From
Date
On Wed, 2009-10-07 at 05:41 +0800, Daniel Mack wrote:
> The LT3593 is a step-up DC/DC converter designed to drive up to ten
> white LEDs in series. The current flow can be set with a control pin.
>
> This driver controls any number of such devices connected on generic
> GPIOs and exports the function as as platform_driver.
>
> The gpio_led platform data struct definition is reused for this purpose.
>
> Successfully tested on a PXA embedded board.

Looks good to me, just some minor comments:

> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/leds.h>
> +#include <linux/workqueue.h>
> +#include <linux/delay.h>
> +
> +#include <asm/gpio.h>

Should this be <linux/gpio.h> ?

> +static void lt3593_led_work(struct work_struct *work)
> +{
> + int pulses;
> + struct lt3593_led_data *led_dat =
> + container_of(work, struct lt3593_led_data, work);

There's a tab above which caught my eye. I'd have ignored it if I wasn't
mentioning the above...

> + pulses = 32 - (led_dat->new_level * 32) / 255;
> +
> + if (pulses == 0) {
> + gpio_set_value_cansleep(led_dat->gpio, 0);
> + mdelay(1);
> + gpio_set_value_cansleep(led_dat->gpio, 1);
> + return;
> + }

mdelay is frowned upon

> + gpio_set_value_cansleep(led_dat->gpio, 1);
> +
> + while (pulses--) {
> + gpio_set_value_cansleep(led_dat->gpio, 0);
> + udelay(1);
> + gpio_set_value_cansleep(led_dat->gpio, 1);
> + udelay(1);
> + }

and likewise udelay but I guess we can't do much else with this
hardware...

Otherwise it looks good, just check the include please and then I'll
apply it.

Cheers,

Richard



\
 
 \ /
  Last update: 2009-10-15 00:41    [W:0.034 / U:0.724 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site