Date: Mon, 29 Dec 2008 15:11:52 -0800
From: Andrew Morton <akpm@linux-foundation.org>
To: Guennadi Liakhovetski <lg@denx.de>
Cc: ben-linux@fluff.org, linux-kernel@vger.kernel.org, rpurdie@rpsys.net
Subject: Re: [PATCH v2] leds: add a dac124s085 SPI LED driver
Message-Id: <20081229151152.5699d59c.akpm@linux-foundation.org>
In-Reply-To: <Pine.LNX.4.64.0812181346440.5510@axis700.grange>
References: <Pine.LNX.4.64.0812181213540.5510@axis700.grange>
	<20081218120619.GJ12431@fluff.org.uk>
	<Pine.LNX.4.64.0812181346440.5510@axis700.grange>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6518
Lines: 223

On Thu, 18 Dec 2008 13:51:06 +0100 (CET)
Guennadi Liakhovetski <lg@denx.de> wrote:

> A simple LED driver using the DAC124S085 DAC from NatSemi
> 
> Signed-off-by: Guennadi Liakhovetski <lg@denx.de>
> ---
> 
> Changes since v1: implemented corrections by Ben Dooks: macro instead of a 
> magic constant, endianness conversion, ARRAY_SIZE instead of a hard-coded 
> constant.
> 
> Still needs a previous patch making maximum brightness configurable:
> 
> http://marc.info/?l=linux-kernel&m=122771446123369&w=2
> 
>  drivers/leds/Kconfig           |    7 ++
>  drivers/leds/Makefile          |    3 +
>  drivers/leds/leds-dac124s085.c |  147 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 157 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/leds/leds-dac124s085.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index a4a1ae2..a28635f 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -178,6 +178,13 @@ config LEDS_DA903X
>  	  This option enables support for on-chip LED drivers found
>  	  on Dialog Semiconductor DA9030/DA9034 PMICs.
>  
> +config LEDS_DAC124S085
> +	tristate "LED Support for DAC124S085 SPI DAC"
> +	depends on LEDS_CLASS && SPI
> +	help
> +	  This option enables support for DAC124S085 SPI DAC from NatSemi,
> +	  which can be used to control up to four LEDs.
> +
>  comment "LED Triggers"
>  
>  config LEDS_TRIGGERS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index bc247cb..8780152 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -26,6 +26,9 @@ obj-$(CONFIG_LEDS_DA903X)		+= leds-da903x.o
>  obj-$(CONFIG_LEDS_HP_DISK)		+= leds-hp-disk.o
>  obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
>  
> +# LED SPI Drivers
> +obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> +
>  # LED Triggers
>  obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
>  obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK)	+= ledtrig-ide-disk.o
> diff --git a/drivers/leds/leds-dac124s085.c b/drivers/leds/leds-dac124s085.c
> new file mode 100644
> index 0000000..9bd25c1
> --- /dev/null
> +++ b/drivers/leds/leds-dac124s085.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright 2008
> + * Guennadi Liakhovetski, DENX Software Engineering, <lg@denx.de>
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License.  See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * LED driver for the DAC124S085 SPI DAC
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +#include <linux/spi/spi.h>
> +
> +struct dac124s085_led {
> +	struct led_classdev	ldev;
> +	struct spi_device	*spi;
> +	int			id;
> +	int			brightness;
> +	char			name[sizeof("dac124s085-3")];
> +
> +	struct mutex		mutex;
> +	struct work_struct	work;
> +	spinlock_t		lock;
> +};
> +
> +struct dac124s085 {
> +	struct dac124s085_led leds[4];
> +};
> +
> +#define REG_WRITE		(0 << 12)
> +#define REG_WRITE_UPDATE	(1 << 12)
> +#define ALL_WRITE_UPDATE	(2 << 12)
> +#define POWER_DOWN_OUTPUT	(3 << 12)
> +
> +static void dac124s085_led_work(struct work_struct *work)
> +{
> +	struct dac124s085_led *led = container_of(work, struct dac124s085_led,
> +						  work);
> +	u16 word;
> +
> +	mutex_lock(&led->mutex);
> +	word = cpu_to_le16(((led->id) << 14) | REG_WRITE_UPDATE |
> +			   (led->brightness & 0xfff));
> +	spi_write(led->spi, (const u8 *)&word, sizeof(word));

This looks like it might have endianness issues?

> +	mutex_unlock(&led->mutex);
> +}
> +
> +static void dac124s085_set_brightness(struct led_classdev *ldev,
> +				      enum led_brightness brightness)
> +{
> +	struct dac124s085_led *led = container_of(ldev, struct dac124s085_led,
> +						  ldev);
> +
> +	spin_lock(&led->lock);
> +	led->brightness = brightness;
> +	schedule_work(&led->work);
> +	spin_unlock(&led->lock);
> +}

So..  why do we need to defer this to a workqueue rather than just
performing the operation synchronously?

What happens if there are two calls to set_brightness() in quick
succession, and the second occurs before the first has completed?  I
think the schedule_work() fails and the second write gets lost?

> +static int dac124s085_probe(struct spi_device *spi)
> +{
> +	struct dac124s085	*dac;
> +	struct dac124s085_led	*led;
> +	int i, ret;
> +
> +	dac = kzalloc(sizeof(*dac), GFP_KERNEL);
> +	if (!dac)
> +		return -ENOMEM;
> +
> +	spi->bits_per_word = 16;
> +
> +	for (i = 0; i < ARRAY_SIZE(dac->leds); i++) {
> +		led		= dac->leds + i;
> +		led->id		= i;
> +		led->brightness	= LED_OFF;
> +		led->spi	= spi;
> +		snprintf(led->name, sizeof(led->name), "dac124s085-%d", i);
> +		spin_lock_init(&led->lock);
> +		INIT_WORK(&led->work, dac124s085_led_work);
> +		mutex_init(&led->mutex);
> +		led->ldev.name = led->name;
> +		led->ldev.brightness = LED_OFF;
> +		led->ldev.max_brightness = 0xfff;
> +		led->ldev.brightness_set = dac124s085_set_brightness;
> +		ret = led_classdev_register(&spi->dev, &led->ldev);
> +		if (ret < 0)
> +			goto eledcr;
> +	}
> +
> +	spi_set_drvdata(spi, dac);
> +
> +	return 0;
> +
> +eledcr:
> +	while (i--)
> +		led_classdev_unregister(&dac->leds[i].ldev);
> +
> +	spi_set_drvdata(spi, NULL);
> +	kfree(dac);
> +	return ret;
> +}
> +
> +static int dac124s085_remove(struct spi_device *spi)
> +{
> +	struct dac124s085	*dac = spi_get_drvdata(spi);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(dac->leds); i++) {
> +		led_classdev_unregister(&dac->leds[i].ldev);
> +		cancel_work_sync(&dac->leds[i].work);
> +	}
> +
> +	spi_set_drvdata(spi, NULL);
> +	kfree(dac);
> +
> +	return 0;
> +}
> +
> +static struct spi_driver dac124s085_driver = {
> +	.probe		= dac124s085_probe,
> +	.remove		= dac124s085_remove,
> +	.driver = {
> +		.name	= "dac124s085",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init dac124s085_leds_init(void)
> +{
> +	return spi_register_driver(&dac124s085_driver);
> +}
> +
> +static void __exit dac124s085_leds_exit(void)
> +{
> +	spi_unregister_driver(&dac124s085_driver);
> +}
> +
> +module_init(dac124s085_leds_init);
> +module_exit(dac124s085_leds_exit);
> +
> +MODULE_AUTHOR("Guennadi Liakhovetski <lg@denx.de>");
> +MODULE_DESCRIPTION("DAC124S085 LED driver");
> +MODULE_LICENSE("GPL v2");

--
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/