lkml.org 
[lkml]   [2009]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] LED: add driver for LT3593 controlled LEDs
    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 <daniel@caiaq.de>
    > To: Richard Purdie <rpurdie@rpsys.net>
    > 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 <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
    >
    > From c1024d34445d19abe53d80756b2439521ad03579 Mon Sep 17 00:00:00 2001
    > From: Daniel Mack <daniel@caiaq.de>
    > 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 <daniel@caiaq.de>
    > Cc: Richard Purdie <rpurdie@rpsys.net>
    > ---
    > 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 <daniel@caiaq.de>
    > + *
    > + * Based on leds-gpio.c,
    > + *
    > + * Copyright (C) 2007 8D Technologies inc.
    > + * Raphael Assenat <raph@8d.com>
    > + * 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 <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 <linux/gpio.h>
    > +
    > +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(&lt3593_led_driver);
    > +}
    > +
    > +static void __exit lt3593_led_exit(void)
    > +{
    > + platform_driver_unregister(&lt3593_led_driver);
    > +}
    > +
    > +module_init(lt3593_led_init);
    > +module_exit(lt3593_led_exit);
    > +
    > +MODULE_AUTHOR("Daniel Mack <daniel@caiaq.de>");
    > +MODULE_DESCRIPTION("LED driver for LT3593 controllers");
    > +MODULE_LICENSE("GPL");
    > --
    > 1.6.0.4
    >


    \
     
     \ /
      Last update: 2009-11-03 11:05    [W:0.047 / U:22.744 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site