Re: [PATCH] LED: add driver for LT3593 controlled LEDs

[Richard Purdie <rpurdie@rpsys.net>]

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