Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756045AbZKCKDf (ORCPT ); Tue, 3 Nov 2009 05:03:35 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751435AbZKCKDe (ORCPT ); Tue, 3 Nov 2009 05:03:34 -0500 Received: from buzzloop.caiaq.de ([212.112.241.133]:39588 "EHLO buzzloop.caiaq.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751416AbZKCKDd (ORCPT ); Tue, 3 Nov 2009 05:03:33 -0500 Date: Tue, 3 Nov 2009 11:03:31 +0100 From: Daniel Mack To: Richard Purdie Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH] LED: add driver for LT3593 controlled LEDs Message-ID: <20091103100331.GF29559@buzzloop.caiaq.de> References: <1254865260-13570-1-git-send-email-daniel@caiaq.de> <1255544001.14634.8.camel@ted> <20091015005935.GN28832@buzzloop.caiaq.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091015005935.GN28832@buzzloop.caiaq.de> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10872 Lines: 376 Hi Richard, Excuse the annoyance - I just want to make sure this is not getting lost ... Thanks, Daniel On Thu, Oct 15, 2009 at 02:59:35AM +0200, Daniel Mack wrote: > Date: Thu, 15 Oct 2009 02:59:35 +0200 > From: Daniel Mack > To: Richard Purdie > Cc: linux-kernel@vger.kernel.org > Subject: Re: [PATCH] LED: add driver for LT3593 controlled LEDs > > 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 > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#include > > > > Should this be ? > > 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 > > From c1024d34445d19abe53d80756b2439521ad03579 Mon Sep 17 00:00:00 2001 > From: Daniel Mack > Date: Wed, 7 Oct 2009 04:11:40 +0800 > Subject: [PATCH] LED: add driver for LT3593 controlled LEDs > > 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. > > Signed-off-by: Daniel Mack > Cc: Richard Purdie > --- > drivers/leds/Kconfig | 8 ++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-lt3593.c | 217 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 226 insertions(+), 0 deletions(-) > create mode 100644 drivers/leds/leds-lt3593.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 7c8e712..80a183f 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -229,6 +229,14 @@ config LEDS_BD2802 > This option enables support for BD2802GU RGB LED driver chips > accessed via the I2C bus. > > +config LEDS_LT3593 > + tristate "LED driver for LT3593 controllers" > + depends on LEDS_CLASS && GENERIC_GPIO > + help > + This option enables support for LEDs driven by a Linear Technology > + LT3593 controller. This controller uses a special one-wire pulse > + coding protocol to set the brightness. > + > comment "LED Triggers" > > config LEDS_TRIGGERS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index e8cdcf7..3d79287 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -28,6 +28,7 @@ obj-$(CONFIG_LEDS_PCA955X) += leds-pca955x.o > obj-$(CONFIG_LEDS_DA903X) += leds-da903x.o > obj-$(CONFIG_LEDS_WM8350) += leds-wm8350.o > obj-$(CONFIG_LEDS_PWM) += leds-pwm.o > +obj-$(CONFIG_LEDS_LT3593) += leds-lt3593.o > > # LED SPI Drivers > obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o > diff --git a/drivers/leds/leds-lt3593.c b/drivers/leds/leds-lt3593.c > new file mode 100644 > index 0000000..fee40a8 > --- /dev/null > +++ b/drivers/leds/leds-lt3593.c > @@ -0,0 +1,217 @@ > +/* > + * LEDs driver for LT3593 controllers > + * > + * See the datasheet at http://cds.linear.com/docs/Datasheet/3593f.pdf > + * > + * Copyright (c) 2009 Daniel Mack > + * > + * Based on leds-gpio.c, > + * > + * Copyright (C) 2007 8D Technologies inc. > + * Raphael Assenat > + * Copyright (C) 2008 Freescale Semiconductor, Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +struct lt3593_led_data { > + struct led_classdev cdev; > + unsigned gpio; > + struct work_struct work; > + u8 new_level; > +}; > + > +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); > + > + /* > + * The LT3593 resets its internal current level register to the maximum > + * level on the first falling edge on the control pin. Each following > + * falling edge decreases the current level by 625uA. Up to 32 pulses > + * can be sent, so the maximum power reduction is 20mA. > + * After a timeout of 128us, the value is taken from the register and > + * applied is to the output driver. > + */ > + > + if (led_dat->new_level == 0) { > + gpio_set_value_cansleep(led_dat->gpio, 0); > + return; > + } > + > + 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; > + } > + > + 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); > + } > +} > + > +static void lt3593_led_set(struct led_classdev *led_cdev, > + enum led_brightness value) > +{ > + struct lt3593_led_data *led_dat = > + container_of(led_cdev, struct lt3593_led_data, cdev); > + > + led_dat->new_level = value; > + schedule_work(&led_dat->work); > +} > + > +static int __devinit create_lt3593_led(const struct gpio_led *template, > + struct lt3593_led_data *led_dat, struct device *parent) > +{ > + int ret, state; > + > + /* skip leds on GPIOs that aren't available */ > + if (!gpio_is_valid(template->gpio)) { > + printk(KERN_INFO "%s: skipping unavailable LT3593 LED at gpio %d (%s)\n", > + KBUILD_MODNAME, template->gpio, template->name); > + return 0; > + } > + > + ret = gpio_request(template->gpio, template->name); > + if (ret < 0) > + return ret; > + > + led_dat->cdev.name = template->name; > + led_dat->cdev.default_trigger = template->default_trigger; > + led_dat->gpio = template->gpio; > + > + led_dat->cdev.brightness_set = lt3593_led_set; > + > + state = (template->default_state == LEDS_GPIO_DEFSTATE_ON); > + led_dat->cdev.brightness = state ? LED_FULL : LED_OFF; > + > + if (!template->retain_state_suspended) > + led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME; > + > + ret = gpio_direction_output(led_dat->gpio, state); > + if (ret < 0) > + goto err; > + > + INIT_WORK(&led_dat->work, lt3593_led_work); > + > + ret = led_classdev_register(parent, &led_dat->cdev); > + if (ret < 0) > + goto err; > + > + printk(KERN_INFO "%s: registered LT3593 LED '%s' at GPIO %d\n", > + KBUILD_MODNAME, template->name, template->gpio); > + > + return 0; > + > +err: > + gpio_free(led_dat->gpio); > + return ret; > +} > + > +static void delete_lt3593_led(struct lt3593_led_data *led) > +{ > + if (!gpio_is_valid(led->gpio)) > + return; > + > + led_classdev_unregister(&led->cdev); > + cancel_work_sync(&led->work); > + gpio_free(led->gpio); > +} > + > +static int __devinit lt3593_led_probe(struct platform_device *pdev) > +{ > + struct gpio_led_platform_data *pdata = pdev->dev.platform_data; > + struct lt3593_led_data *leds_data; > + int i, ret = 0; > + > + if (!pdata) > + return -EBUSY; > + > + leds_data = kzalloc(sizeof(struct lt3593_led_data) * pdata->num_leds, > + GFP_KERNEL); > + if (!leds_data) > + return -ENOMEM; > + > + for (i = 0; i < pdata->num_leds; i++) { > + ret = create_lt3593_led(&pdata->leds[i], &leds_data[i], > + &pdev->dev); > + if (ret < 0) > + goto err; > + } > + > + platform_set_drvdata(pdev, leds_data); > + > + return 0; > + > +err: > + for (i = i - 1; i >= 0; i--) > + delete_lt3593_led(&leds_data[i]); > + > + kfree(leds_data); > + > + return ret; > +} > + > +static int __devexit lt3593_led_remove(struct platform_device *pdev) > +{ > + int i; > + struct gpio_led_platform_data *pdata = pdev->dev.platform_data; > + struct lt3593_led_data *leds_data; > + > + leds_data = platform_get_drvdata(pdev); > + > + for (i = 0; i < pdata->num_leds; i++) > + delete_lt3593_led(&leds_data[i]); > + > + kfree(leds_data); > + > + return 0; > +} > + > +static struct platform_driver lt3593_led_driver = { > + .probe = lt3593_led_probe, > + .remove = __devexit_p(lt3593_led_remove), > + .driver = { > + .name = "leds-lt3593", > + .owner = THIS_MODULE, > + }, > +}; > + > +MODULE_ALIAS("platform:leds-lt3593"); > + > +static int __init lt3593_led_init(void) > +{ > + return platform_driver_register(<3593_led_driver); > +} > + > +static void __exit lt3593_led_exit(void) > +{ > + platform_driver_unregister(<3593_led_driver); > +} > + > +module_init(lt3593_led_init); > +module_exit(lt3593_led_exit); > + > +MODULE_AUTHOR("Daniel Mack "); > +MODULE_DESCRIPTION("LED driver for LT3593 controllers"); > +MODULE_LICENSE("GPL"); > -- > 1.6.0.4 > -- 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/