Subject: Re: [PATCH] LED: add driver for LT3593 controlled LEDs
From: Richard Purdie <rpurdie@rpsys.net>
To: Daniel Mack <daniel@caiaq.de>
Cc: linux-kernel@vger.kernel.org
In-Reply-To: <1254865260-13570-1-git-send-email-daniel@caiaq.de>
References: <1254865260-13570-1-git-send-email-daniel@caiaq.de>
Content-Type: text/plain
Date: Wed, 14 Oct 2009 19:13:21 +0100
Message-Id: <1255544001.14634.8.camel@ted>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1881
Lines: 69

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/