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

[Daniel Mack]

Hi,

thanks for the careful review!

On Wed, Oct 14, 2009 at 07:13:21PM +0100, Richard Purdie wrote:
> 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> ?

Yep, correct. As this is taken from leds-gpio, the original version
should need an update, too.

> > +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...

Fixed.

> > +	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.

Thanks! New patch below.

Daniel