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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1255544001.14634.8.camel@ted>
Date:	Wed, 14 Oct 2009 19:13:21 +0100
From:	Richard Purdie <rpurdie@...ys.net>
To:	Daniel Mack <daniel@...aq.de>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] LED: add driver for LT3593 controlled LEDs

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/