diff mbox

[1/2] leds: Add generic support for memory mapped LEDs

Message ID 1351792722-15250-1-git-send-email-pawel.moll@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pawel Moll Nov. 1, 2012, 5:58 p.m. UTC 
LEDs are often controlled by writing to memory mapped
register. This patch adds:

1. Generic functions for platform code and drivers to create
   class device for LEDs controlled by arbitrary bit masks.
   The control register value is read, modified by logic AND
   and OR operations with respective mask and written back.

2. A platform driver for simple use case when one or more LED
   are controlled by consecutive bits in a register pointed
   at by the platform device's memory resource. It can be
   particularly useful for MFD cells being part of an other
   device.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/leds/Kconfig     |    8 ++
 drivers/leds/Makefile    |    1 +
 drivers/leds/leds-mmio.c |  250 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h     |   38 +++++++
 4 files changed, 297 insertions(+)
 create mode 100644 drivers/leds/leds-mmio.c

Comments

Jon Medhurst (Tixy) Nov. 5, 2012, 9:39 a.m. UTC | #1 
On Thu, 2012-11-01 at 17:58 +0000, Pawel Moll wrote:
> LEDs are often controlled by writing to memory mapped
> register. This patch adds:
> 
> 1. Generic functions for platform code and drivers to create
>    class device for LEDs controlled by arbitrary bit masks.
>    The control register value is read, modified by logic AND
>    and OR operations with respective mask and written back.
> 
> 2. A platform driver for simple use case when one or more LED
>    are controlled by consecutive bits in a register pointed
>    at by the platform device's memory resource. It can be
>    particularly useful for MFD cells being part of an other
>    device.
> 
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  drivers/leds/Kconfig     |    8 ++
>  drivers/leds/Makefile    |    1 +
>  drivers/leds/leds-mmio.c |  250 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/leds.h     |   38 +++++++
>  4 files changed, 297 insertions(+)
>  create mode 100644 drivers/leds/leds-mmio.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f508def..93707e6 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -457,6 +457,14 @@ config LEDS_BLINKM
>  	  This option enables support for the BlinkM RGB LED connected
>  	  through I2C. Say Y to enable support for the BlinkM LED.
>  
> +config LEDS_MMIO
> +	tristate "Generic LED support for memory mapped peripherals"
> +	depends on LEDS_CLASS
> +	depends on HAS_IOMEM
> +	help
> +	  This option enables generic support for LEDs controlled via
> +	  memory mapped registers.
> +
>  config LEDS_TRIGGERS
>  	bool "LED Trigger support"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 3fb9641..8e5d0c8 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_LEDS_RENESAS_TPU)		+= leds-renesas-tpu.o
>  obj-$(CONFIG_LEDS_MAX8997)		+= leds-max8997.o
>  obj-$(CONFIG_LEDS_LM355x)		+= leds-lm355x.o
>  obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
> +obj-$(CONFIG_LEDS_MMIO)			+= leds-mmio.o
>  
>  # LED SPI Drivers
>  obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> diff --git a/drivers/leds/leds-mmio.c b/drivers/leds/leds-mmio.c
> new file mode 100644
> index 0000000..1ef0cda
> --- /dev/null
> +++ b/drivers/leds/leds-mmio.c
> @@ -0,0 +1,250 @@
> +/*
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Copyright (C) 2012 ARM Limited
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +
> +static u32 mmio_led_read(void __iomem *reg, unsigned reg_size)
> +{
> +	switch (reg_size) {
> +	case 32:
> +		return readl(reg);
> +	case 16:
> +		return readw(reg);
> +	case 8:
> +		return readb(reg);
> +	}
> +	return 0;
> +}
> +
> +static void mmio_led_write(void __iomem *reg, unsigned reg_size, u32 val)
> +{
> +	switch (reg_size) {
> +	case 32:
> +		writel(val, reg);
> +		return;
> +	case 16:
> +		writew(val, reg);
> +		return;
> +	case 8:
> +		writeb(val, reg);
> +		return;
> +	}
> +}
> +
> +
> +struct mmio_led {
> +	struct led_classdev cdev;
> +	spinlock_t *lock;
> +	void __iomem *reg;
> +	unsigned reg_size;
> +	u32 off_and_mask, off_or_mask;
> +	u32 on_and_mask, on_or_mask;
> +};
> +
> +static void mmio_led_brightness_set(struct led_classdev *cdev,
> +		enum led_brightness brightness)
> +{
> +	struct mmio_led *led = container_of(cdev, struct mmio_led, cdev);
> +	unsigned long uninitialized_var(flags);

uninitialized_var seems to be a bit contentious, Linus Torvalds had a
recent complaint about it which prompted Ingo to post a patch proposing
to removing it: https://patchwork.kernel.org/patch/1655621/ So perhaps
best to avoid using it ;-).

In this case, you could possibly keep gcc quite with something like:

        spinlock_t *lock = led->lock;

and then use the local variable 'lock' everywhere instead of led->lock.
Or just keep it simple an initialise flags to 0 instead.

> +	u32 val;
> +
> +	if (led->lock)
> +		spin_lock_irqsave(led->lock, flags);
> +
> +	val = mmio_led_read(led->reg, led->reg_size);
> +	if (brightness == LED_OFF) {
> +		val &= led->off_and_mask;
> +		val |= led->off_or_mask;
> +	} else {
> +		val &= led->on_and_mask;
> +		val |= led->on_or_mask;
> +	}
> +	mmio_led_write(led->reg, led->reg_size, val);
> +
> +	if (led->lock)
> +		spin_unlock_irqrestore(led->lock, flags);
> +};
> +
> +static enum led_brightness mmio_led_brightness_get(struct led_classdev *cdev)
> +{
> +	struct mmio_led *led = container_of(cdev, struct mmio_led, cdev);
> +	unsigned long uninitialized_var(flags);

Same comment as previous.

> +	u32 val;
> +
> +	if (led->lock)
> +		spin_lock_irqsave(led->lock, flags);
> +	val = mmio_led_read(led->reg, led->reg_size);
> +	if (led->lock)
> +		spin_unlock_irqrestore(led->lock, flags);
> +
> +	if (((val & led->on_and_mask) | led->on_or_mask) == val)
> +		return LED_FULL;
> +	else
> +		return LED_OFF;
> +}
> +
> +struct mmio_led *mmio_led_register(struct device *parent, spinlock_t *lock,
> +		const char *name, const char *default_trigger,
> +		void __iomem *reg, unsigned reg_size, u32 off_and_mask,
> +		u32 off_or_mask, u32 on_and_mask, u32 on_or_mask)
> +{
> +	struct mmio_led *led;
> +	int err;
> +
> +	if (WARN_ON(reg_size != 8 && reg_size != 16 && reg_size != 32))
> +		return ERR_PTR(-EINVAL);
> +
> +	led = kzalloc(sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return ERR_PTR(-ENOMEM);
> +
> +	led->cdev.brightness_set = mmio_led_brightness_set;
> +	led->cdev.brightness_get = mmio_led_brightness_get;
> +	led->cdev.name = name;
> +	led->cdev.default_trigger = default_trigger;
> +
> +	led->lock = lock;
> +	led->reg = reg;
> +	led->reg_size = reg_size;
> +	led->off_and_mask = off_and_mask;
> +	led->off_or_mask = off_or_mask;
> +	led->on_and_mask = on_and_mask;
> +	led->on_or_mask = on_or_mask;
> +
> +	err = led_classdev_register(parent, &led->cdev);
> +	if (err) {
> +		kfree(led);
> +		return ERR_PTR(err);
> +	}
> +
> +	return led;
> +}
> +EXPORT_SYMBOL_GPL(mmio_led_register);
> +
> +void mmio_led_unregister(struct mmio_led *led)
> +{
> +	led_classdev_unregister(&led->cdev);
> +	kfree(led);
> +}
> +EXPORT_SYMBOL_GPL(mmio_led_unregister);
> +
> +
> +struct mmio_simple_leds {
> +	spinlock_t lock;
> +	int num_leds;
> +	struct mmio_led *leds[0];
> +};
> +
> +static int __devinit mmio_simple_leds_probe(struct platform_device *pdev)
> +{
> +	struct mmio_simple_leds_platform_data *pdata = pdev->dev.platform_data;
> +	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	struct mmio_simple_leds *leds;
> +	void __iomem *reg;
> +	u32 val, mask;
> +	int i;
> +
> +	if (!pdata)
> +		return -EINVAL;
> +
> +	if (pdata->reg_size != 8 && pdata->reg_size != 16 &&
> +			pdata->reg_size != 32)
> +		return -EFAULT;

Is EFAULT appropriate here? Why not EINVAL?

> +	leds = devm_kzalloc(&pdev->dev, sizeof(*leds) +
> +			sizeof(leds->leds) * pdata->width, GFP_KERNEL);
> +	if (!leds)
> +		return -ENOMEM;
> +	spin_lock_init(&leds->lock);
> +
> +	if ((!pdev->mfd_cell || !pdev->mfd_cell->ignore_resource_conflicts) &&
> +			!devm_request_mem_region(&pdev->dev, res->start,
> +			resource_size(res), pdev->name))
> +		return -EBUSY;
> +
> +	reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	if (!reg)
> +		return -ENOMEM;
> +
> +	val = mmio_led_read(reg, pdata->reg_size);
> +	mask = ((1 << pdata->width) - 1) << pdata->shift;
> +	if ((pdata->init_full && pdata->active_low) ||
> +			(pdata->init_off && !pdata->active_low))
> +		val &= ~mask;
> +	else if ((pdata->init_off && pdata->active_low) ||
> +			(pdata->init_full && !pdata->active_low))
> +		val |= mask;
> +	mmio_led_write(reg, pdata->reg_size, val);
> +
> +	leds->num_leds = pdata->width;
> +	for (i = 0; i < leds->num_leds; i++) {
> +		unsigned shift = pdata->shift + i;
> +		u32 and_mask = ~BIT(shift);
> +		u32 off_or_mask = pdata->active_low ? BIT(shift) : 0;
> +		u32 on_or_mask = pdata->active_low ? 0 : BIT(shift);
> +		struct mmio_led *led = mmio_led_register(&pdev->dev,
> +				&leds->lock, pdata->names[i],
> +				pdata->default_triggers ?
> +				pdata->default_triggers[i] : NULL,
> +				reg, pdata->reg_size, and_mask, off_or_mask,
> +				and_mask, on_or_mask);
> +
> +		if (IS_ERR(led)) {
> +			while (--i >= 0)
> +				mmio_led_unregister(leds->leds[i]);
> +			return PTR_ERR(led);
> +		}
> +		leds->leds[i] = led;
> +	}
> +
> +	platform_set_drvdata(pdev, leds);
> +
> +	return 0;
> +}
> +
> +static int __devexit mmio_simple_leds_remove(struct platform_device *pdev)
> +{
> +	struct mmio_simple_leds *leds = platform_get_drvdata(pdev);
> +	int i;
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	for (i = 0; i < leds->num_leds; i++)
> +		mmio_led_unregister(leds->leds[i]);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mmio_simple_leds_driver = {
> +	.probe		= mmio_simple_leds_probe,
> +	.remove		= __devexit_p(mmio_simple_leds_remove),
> +	.driver		= {
> +		.name	= "leds-mmio-simple",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(mmio_simple_leds_driver);
> +
> +MODULE_AUTHOR("Pawel Moll <pawel.moll@arm.com>");
> +MODULE_DESCRIPTION("MMIO LED driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-mmio-simple");
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 6e53bb3..a6338b0 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -241,6 +241,44 @@ struct gpio_led_platform_data {
>  struct platform_device *gpio_led_register_device(
>  		int id, const struct gpio_led_platform_data *pdata);
>  
> +/* For the leds-mmio driver */
> +
> +struct mmio_led;
> +
> +#if defined(CONFIG_LEDS_MMIO)
> +/* Returns ERR_PTR in case of error */
> +struct mmio_led *mmio_led_register(struct device *parent, spinlock_t *lock,
> +		const char *name, const char *default_trigger,
> +		void __iomem *reg, unsigned reg_size, u32 off_and_mask,
> +		u32 off_or_mask, u32 on_and_mask, u32 on_or_mask);
> +void mmio_led_unregister(struct mmio_led *led);
> +#else
> +struct mmio_led *mmio_led_register(struct device *parent, spinlock_t *lock,
> +		const char *name, const char *default_trigger,
> +		void __iomem *reg, unsigned reg_size, u32 off_and_mask,
> +		u32 off_or_mask, u32 on_and_mask, u32 on_or_mask)
> +{
> +	return NULL;
> +}
> +
> +void mmio_led_unregister(struct mmio_led *led)
> +{
> +}
> +#endif
> +
> +struct mmio_simple_leds_platform_data {
> +	unsigned reg_size; /* Register size (8/16/32) */
> +	unsigned shift; /* First bit controlling LEDs */
> +	unsigned width; /* Number of consecutive bits */
> +	const char **names; /* Must define 'width' names */
> +	const char **default_triggers; /* NULL or 'width' strings */
> +	bool active_low;
> +	bool init_full;
> +	bool init_off;
> +};
> +
> +/* CPU led trigger */
> +
>  enum cpu_led_event {
>  	CPU_LED_IDLE_START,	/* CPU enters idle */
>  	CPU_LED_IDLE_END,	/* CPU idle ends */
Pawel Moll Nov. 5, 2012, 12:55 p.m. UTC | #2 
On Mon, 2012-11-05 at 09:39 +0000, Jon Medhurst (Tixy) wrote:
> > +static void mmio_led_brightness_set(struct led_classdev *cdev,
> > +		enum led_brightness brightness)
> > +{
> > +	struct mmio_led *led = container_of(cdev, struct mmio_led, cdev);
> > +	unsigned long uninitialized_var(flags);
> 
> uninitialized_var seems to be a bit contentious, Linus Torvalds had a
> recent complaint about it which prompted Ingo to post a patch proposing
> to removing it: https://patchwork.kernel.org/patch/1655621/ So perhaps
> best to avoid using it ;-).
> 
> In this case, you could possibly keep gcc quite with something like:
> 
>         spinlock_t *lock = led->lock;
> 
> and then use the local variable 'lock' everywhere instead of led->lock.
> Or just keep it simple an initialise flags to 0 instead.

Yeah, = 0 will do...

> > +	if (!pdata)
> > +		return -EINVAL;
> > +
> > +	if (pdata->reg_size != 8 && pdata->reg_size != 16 &&
> > +			pdata->reg_size != 32)
> > +		return -EFAULT;
> 
> Is EFAULT appropriate here? Why not EINVAL?

Hm. To distinguish it from !pdata case I guess (and a 13 bit wide
transaction sounds like a fault to me ;-), but I can be persuaded
otherwise without much effort...

Thanks!

Pawe?
Jon Medhurst (Tixy) Nov. 5, 2012, 1:52 p.m. UTC | #3 
On Mon, 2012-11-05 at 12:55 +0000, Pawel Moll wrote:
> On Mon, 2012-11-05 at 09:39 +0000, Jon Medhurst (Tixy) wrote:
> > > +static void mmio_led_brightness_set(struct led_classdev *cdev,
> > > +		enum led_brightness brightness)
> > > +{
> > > +	struct mmio_led *led = container_of(cdev, struct mmio_led, cdev);
> > > +	unsigned long uninitialized_var(flags);
> > 
> > uninitialized_var seems to be a bit contentious, Linus Torvalds had a
> > recent complaint about it which prompted Ingo to post a patch proposing
> > to removing it: https://patchwork.kernel.org/patch/1655621/ So perhaps
> > best to avoid using it ;-).
> > 
> > In this case, you could possibly keep gcc quite with something like:
> > 
> >         spinlock_t *lock = led->lock;
> > 
> > and then use the local variable 'lock' everywhere instead of led->lock.
> > Or just keep it simple an initialise flags to 0 instead.
> 
> Yeah, = 0 will do...
> 
> > > +	if (!pdata)
> > > +		return -EINVAL;
> > > +
> > > +	if (pdata->reg_size != 8 && pdata->reg_size != 16 &&
> > > +			pdata->reg_size != 32)
> > > +		return -EFAULT;
> > 
> > Is EFAULT appropriate here? Why not EINVAL?
> 
> Hm. To distinguish it from !pdata case I guess (and a 13 bit wide
> transaction sounds like a fault to me ;-), but I can be persuaded
> otherwise without much effort...

I was asking as much for my own education about use of error values as
anything else. The comments in errno-base.h are:

#define	EINVAL		22	/* Invalid argument */
#define	EFAULT		14	/* Bad address */

and from looking in the source tree it seems EFAULT is mostly used to
indicate a bad memory address passed from user-side to the kernel.

It's a trivial point so it's not worth wasting time on a long
discussion.
Pawel Moll Nov. 8, 2012, 3:35 p.m. UTC | #4 
Hi Bryan, Richard,

On Thu, 2012-11-01 at 17:58 +0000, Pawel Moll wrote:
> LEDs are often controlled by writing to memory mapped
> register. This patch adds:
> 
> 1. Generic functions for platform code and drivers to create
>    class device for LEDs controlled by arbitrary bit masks.
>    The control register value is read, modified by logic AND
>    and OR operations with respective mask and written back.
> 
> 2. A platform driver for simple use case when one or more LED
>    are controlled by consecutive bits in a register pointed
>    at by the platform device's memory resource. It can be
>    particularly useful for MFD cells being part of an other
>    device.
> 
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>

It's just a friendly and polite nag - any thoughts or feelings about
this?

Cheers!

Pawe?
Pawel Moll Nov. 19, 2012, 11:44 a.m. UTC | #5 
On Thu, 2012-11-08 at 15:35 +0000, Pawel Moll wrote:
> Hi Bryan, Richard,
> 
> On Thu, 2012-11-01 at 17:58 +0000, Pawel Moll wrote:
> > LEDs are often controlled by writing to memory mapped
> > register. This patch adds:
> > 
> > 1. Generic functions for platform code and drivers to create
> >    class device for LEDs controlled by arbitrary bit masks.
> >    The control register value is read, modified by logic AND
> >    and OR operations with respective mask and written back.
> > 
> > 2. A platform driver for simple use case when one or more LED
> >    are controlled by consecutive bits in a register pointed
> >    at by the platform device's memory resource. It can be
> >    particularly useful for MFD cells being part of an other
> >    device.
> > 
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> 
> It's just a friendly and polite nag - any thoughts or feelings about
> this?

Ping again?

Pawe?
Pawel Moll Nov. 26, 2012, 3:25 p.m. UTC | #6 
On Mon, 2012-11-19 at 11:44 +0000, Pawel Moll wrote:
> On Thu, 2012-11-08 at 15:35 +0000, Pawel Moll wrote:
> > Hi Bryan, Richard,
> > 
> > On Thu, 2012-11-01 at 17:58 +0000, Pawel Moll wrote:
> > > LEDs are often controlled by writing to memory mapped
> > > register. This patch adds:
> > > 
> > > 1. Generic functions for platform code and drivers to create
> > >    class device for LEDs controlled by arbitrary bit masks.
> > >    The control register value is read, modified by logic AND
> > >    and OR operations with respective mask and written back.
> > > 
> > > 2. A platform driver for simple use case when one or more LED
> > >    are controlled by consecutive bits in a register pointed
> > >    at by the platform device's memory resource. It can be
> > >    particularly useful for MFD cells being part of an other
> > >    device.
> > > 
> > > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > 
> > It's just a friendly and polite nag - any thoughts or feelings about
> > this?
> 
> Ping again?

And another polite nag for this week...

Pawe?
Vasily Khoruzhick Nov. 26, 2012, 3:37 p.m. UTC | #7 
On Thu, Nov 1, 2012 at 8:58 PM, Pawel Moll <pawel.moll@arm.com> wrote:
> LEDs are often controlled by writing to memory mapped
> register. This patch adds:
>
> 1. Generic functions for platform code and drivers to create
>    class device for LEDs controlled by arbitrary bit masks.
>    The control register value is read, modified by logic AND
>    and OR operations with respective mask and written back.
>
> 2. A platform driver for simple use case when one or more LED
>    are controlled by consecutive bits in a register pointed
>    at by the platform device's memory resource. It can be
>    particularly useful for MFD cells being part of an other
>    device.

Hi

This MMIO controls some latch (or whatever) which is actually GPIO.
So implementing generic GPIO MMIO driver and using leds-gpio on top of
it appears to be a better solution for me.

Regards
Vasily

> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  drivers/leds/Kconfig     |    8 ++
>  drivers/leds/Makefile    |    1 +
>  drivers/leds/leds-mmio.c |  250 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/leds.h     |   38 +++++++
>  4 files changed, 297 insertions(+)
>  create mode 100644 drivers/leds/leds-mmio.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f508def..93707e6 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -457,6 +457,14 @@ config LEDS_BLINKM
>           This option enables support for the BlinkM RGB LED connected
>           through I2C. Say Y to enable support for the BlinkM LED.
>
> +config LEDS_MMIO
> +       tristate "Generic LED support for memory mapped peripherals"
> +       depends on LEDS_CLASS
> +       depends on HAS_IOMEM
> +       help
> +         This option enables generic support for LEDs controlled via
> +         memory mapped registers.
> +
>  config LEDS_TRIGGERS
>         bool "LED Trigger support"
>         depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 3fb9641..8e5d0c8 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_LEDS_RENESAS_TPU)                += leds-renesas-tpu.o
>  obj-$(CONFIG_LEDS_MAX8997)             += leds-max8997.o
>  obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
>  obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
> +obj-$(CONFIG_LEDS_MMIO)                        += leds-mmio.o
>
>  # LED SPI Drivers
>  obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
> diff --git a/drivers/leds/leds-mmio.c b/drivers/leds/leds-mmio.c
> new file mode 100644
> index 0000000..1ef0cda
> --- /dev/null
> +++ b/drivers/leds/leds-mmio.c
> @@ -0,0 +1,250 @@
> +/*
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Copyright (C) 2012 ARM Limited
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +
> +static u32 mmio_led_read(void __iomem *reg, unsigned reg_size)
> +{
> +       switch (reg_size) {
> +       case 32:
> +               return readl(reg);
> +       case 16:
> +               return readw(reg);
> +       case 8:
> +               return readb(reg);
> +       }
> +       return 0;
> +}
> +
> +static void mmio_led_write(void __iomem *reg, unsigned reg_size, u32 val)
> +{
> +       switch (reg_size) {
> +       case 32:
> +               writel(val, reg);
> +               return;
> +       case 16:
> +               writew(val, reg);
> +               return;
> +       case 8:
> +               writeb(val, reg);
> +               return;
> +       }
> +}
> +
> +
> +struct mmio_led {
> +       struct led_classdev cdev;
> +       spinlock_t *lock;
> +       void __iomem *reg;
> +       unsigned reg_size;
> +       u32 off_and_mask, off_or_mask;
> +       u32 on_and_mask, on_or_mask;
> +};
> +
> +static void mmio_led_brightness_set(struct led_classdev *cdev,
> +               enum led_brightness brightness)
> +{
> +       struct mmio_led *led = container_of(cdev, struct mmio_led, cdev);
> +       unsigned long uninitialized_var(flags);
> +       u32 val;
> +
> +       if (led->lock)
> +               spin_lock_irqsave(led->lock, flags);
> +
> +       val = mmio_led_read(led->reg, led->reg_size);
> +       if (brightness == LED_OFF) {
> +               val &= led->off_and_mask;
> +               val |= led->off_or_mask;
> +       } else {
> +               val &= led->on_and_mask;
> +               val |= led->on_or_mask;
> +       }
> +       mmio_led_write(led->reg, led->reg_size, val);
> +
> +       if (led->lock)
> +               spin_unlock_irqrestore(led->lock, flags);
> +};
> +
> +static enum led_brightness mmio_led_brightness_get(struct led_classdev *cdev)
> +{
> +       struct mmio_led *led = container_of(cdev, struct mmio_led, cdev);
> +       unsigned long uninitialized_var(flags);
> +       u32 val;
> +
> +       if (led->lock)
> +               spin_lock_irqsave(led->lock, flags);
> +       val = mmio_led_read(led->reg, led->reg_size);
> +       if (led->lock)
> +               spin_unlock_irqrestore(led->lock, flags);
> +
> +       if (((val & led->on_and_mask) | led->on_or_mask) == val)
> +               return LED_FULL;
> +       else
> +               return LED_OFF;
> +}
> +
> +struct mmio_led *mmio_led_register(struct device *parent, spinlock_t *lock,
> +               const char *name, const char *default_trigger,
> +               void __iomem *reg, unsigned reg_size, u32 off_and_mask,
> +               u32 off_or_mask, u32 on_and_mask, u32 on_or_mask)
> +{
> +       struct mmio_led *led;
> +       int err;
> +
> +       if (WARN_ON(reg_size != 8 && reg_size != 16 && reg_size != 32))
> +               return ERR_PTR(-EINVAL);
> +
> +       led = kzalloc(sizeof(*led), GFP_KERNEL);
> +       if (!led)
> +               return ERR_PTR(-ENOMEM);
> +
> +       led->cdev.brightness_set = mmio_led_brightness_set;
> +       led->cdev.brightness_get = mmio_led_brightness_get;
> +       led->cdev.name = name;
> +       led->cdev.default_trigger = default_trigger;
> +
> +       led->lock = lock;
> +       led->reg = reg;
> +       led->reg_size = reg_size;
> +       led->off_and_mask = off_and_mask;
> +       led->off_or_mask = off_or_mask;
> +       led->on_and_mask = on_and_mask;
> +       led->on_or_mask = on_or_mask;
> +
> +       err = led_classdev_register(parent, &led->cdev);
> +       if (err) {
> +               kfree(led);
> +               return ERR_PTR(err);
> +       }
> +
> +       return led;
> +}
> +EXPORT_SYMBOL_GPL(mmio_led_register);
> +
> +void mmio_led_unregister(struct mmio_led *led)
> +{
> +       led_classdev_unregister(&led->cdev);
> +       kfree(led);
> +}
> +EXPORT_SYMBOL_GPL(mmio_led_unregister);
> +
> +
> +struct mmio_simple_leds {
> +       spinlock_t lock;
> +       int num_leds;
> +       struct mmio_led *leds[0];
> +};
> +
> +static int __devinit mmio_simple_leds_probe(struct platform_device *pdev)
> +{
> +       struct mmio_simple_leds_platform_data *pdata = pdev->dev.platform_data;
> +       struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       struct mmio_simple_leds *leds;
> +       void __iomem *reg;
> +       u32 val, mask;
> +       int i;
> +
> +       if (!pdata)
> +               return -EINVAL;
> +
> +       if (pdata->reg_size != 8 && pdata->reg_size != 16 &&
> +                       pdata->reg_size != 32)
> +               return -EFAULT;
> +
> +       leds = devm_kzalloc(&pdev->dev, sizeof(*leds) +
> +                       sizeof(leds->leds) * pdata->width, GFP_KERNEL);
> +       if (!leds)
> +               return -ENOMEM;
> +       spin_lock_init(&leds->lock);
> +
> +       if ((!pdev->mfd_cell || !pdev->mfd_cell->ignore_resource_conflicts) &&
> +                       !devm_request_mem_region(&pdev->dev, res->start,
> +                       resource_size(res), pdev->name))
> +               return -EBUSY;
> +
> +       reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +       if (!reg)
> +               return -ENOMEM;
> +
> +       val = mmio_led_read(reg, pdata->reg_size);
> +       mask = ((1 << pdata->width) - 1) << pdata->shift;
> +       if ((pdata->init_full && pdata->active_low) ||
> +                       (pdata->init_off && !pdata->active_low))
> +               val &= ~mask;
> +       else if ((pdata->init_off && pdata->active_low) ||
> +                       (pdata->init_full && !pdata->active_low))
> +               val |= mask;
> +       mmio_led_write(reg, pdata->reg_size, val);
> +
> +       leds->num_leds = pdata->width;
> +       for (i = 0; i < leds->num_leds; i++) {
> +               unsigned shift = pdata->shift + i;
> +               u32 and_mask = ~BIT(shift);
> +               u32 off_or_mask = pdata->active_low ? BIT(shift) : 0;
> +               u32 on_or_mask = pdata->active_low ? 0 : BIT(shift);
> +               struct mmio_led *led = mmio_led_register(&pdev->dev,
> +                               &leds->lock, pdata->names[i],
> +                               pdata->default_triggers ?
> +                               pdata->default_triggers[i] : NULL,
> +                               reg, pdata->reg_size, and_mask, off_or_mask,
> +                               and_mask, on_or_mask);
> +
> +               if (IS_ERR(led)) {
> +                       while (--i >= 0)
> +                               mmio_led_unregister(leds->leds[i]);
> +                       return PTR_ERR(led);
> +               }
> +               leds->leds[i] = led;
> +       }
> +
> +       platform_set_drvdata(pdev, leds);
> +
> +       return 0;
> +}
> +
> +static int __devexit mmio_simple_leds_remove(struct platform_device *pdev)
> +{
> +       struct mmio_simple_leds *leds = platform_get_drvdata(pdev);
> +       int i;
> +
> +       platform_set_drvdata(pdev, NULL);
> +
> +       for (i = 0; i < leds->num_leds; i++)
> +               mmio_led_unregister(leds->leds[i]);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver mmio_simple_leds_driver = {
> +       .probe          = mmio_simple_leds_probe,
> +       .remove         = __devexit_p(mmio_simple_leds_remove),
> +       .driver         = {
> +               .name   = "leds-mmio-simple",
> +               .owner  = THIS_MODULE,
> +       },
> +};
> +
> +module_platform_driver(mmio_simple_leds_driver);
> +
> +MODULE_AUTHOR("Pawel Moll <pawel.moll@arm.com>");
> +MODULE_DESCRIPTION("MMIO LED driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-mmio-simple");
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 6e53bb3..a6338b0 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -241,6 +241,44 @@ struct gpio_led_platform_data {
>  struct platform_device *gpio_led_register_device(
>                 int id, const struct gpio_led_platform_data *pdata);
>
> +/* For the leds-mmio driver */
> +
> +struct mmio_led;
> +
> +#if defined(CONFIG_LEDS_MMIO)
> +/* Returns ERR_PTR in case of error */
> +struct mmio_led *mmio_led_register(struct device *parent, spinlock_t *lock,
> +               const char *name, const char *default_trigger,
> +               void __iomem *reg, unsigned reg_size, u32 off_and_mask,
> +               u32 off_or_mask, u32 on_and_mask, u32 on_or_mask);
> +void mmio_led_unregister(struct mmio_led *led);
> +#else
> +struct mmio_led *mmio_led_register(struct device *parent, spinlock_t *lock,
> +               const char *name, const char *default_trigger,
> +               void __iomem *reg, unsigned reg_size, u32 off_and_mask,
> +               u32 off_or_mask, u32 on_and_mask, u32 on_or_mask)
> +{
> +       return NULL;
> +}
> +
> +void mmio_led_unregister(struct mmio_led *led)
> +{
> +}
> +#endif
> +
> +struct mmio_simple_leds_platform_data {
> +       unsigned reg_size; /* Register size (8/16/32) */
> +       unsigned shift; /* First bit controlling LEDs */
> +       unsigned width; /* Number of consecutive bits */
> +       const char **names; /* Must define 'width' names */
> +       const char **default_triggers; /* NULL or 'width' strings */
> +       bool active_low;
> +       bool init_full;
> +       bool init_off;
> +};
> +
> +/* CPU led trigger */
> +
>  enum cpu_led_event {
>         CPU_LED_IDLE_START,     /* CPU enters idle */
>         CPU_LED_IDLE_END,       /* CPU idle ends */
> --
> 1.7.10.4
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Pawel Moll Nov. 26, 2012, 4:10 p.m. UTC | #8 
On Mon, 2012-11-26 at 15:37 +0000, Vasily Khoruzhick wrote:
> On Thu, Nov 1, 2012 at 8:58 PM, Pawel Moll <pawel.moll@arm.com> wrote:
> > LEDs are often controlled by writing to memory mapped
> > register. This patch adds:
> >
> > 1. Generic functions for platform code and drivers to create
> >    class device for LEDs controlled by arbitrary bit masks.
> >    The control register value is read, modified by logic AND
> >    and OR operations with respective mask and written back.
> >
> > 2. A platform driver for simple use case when one or more LED
> >    are controlled by consecutive bits in a register pointed
> >    at by the platform device's memory resource. It can be
> >    particularly useful for MFD cells being part of an other
> >    device.
>
> This MMIO controls some latch (or whatever) which is actually GPIO.
> So implementing generic GPIO MMIO driver and using leds-gpio on top of
> it appears to be a better solution for me.

I won't argue that it is doable - I don't even have to implement the
MMIO GPIO driver, because my MFD already has a set of pseudo-GPIO lines
and extending those won't be a problem at all an I'll do exactly that if
there is no other choice. But at the same time I hope you would agree
that my_code->gpio_controller->gpio_led_description->register_device
chain is longer than my_code->mmio_led one if you want to add a single
"status" led for your little board ;-). I don't event mention that at
least couple of the existing drivers follow the same "MMIO pattern"
blindly.

Nevertheless I'd really like to hear from the maintainers...

Thanks!

Pawe?
diff mbox

Patch

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index f508def..93707e6 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -457,6 +457,14 @@  config LEDS_BLINKM
 	  This option enables support for the BlinkM RGB LED connected
 	  through I2C. Say Y to enable support for the BlinkM LED.
 
+config LEDS_MMIO
+	tristate "Generic LED support for memory mapped peripherals"
+	depends on LEDS_CLASS
+	depends on HAS_IOMEM
+	help
+	  This option enables generic support for LEDs controlled via
+	  memory mapped registers.
+
 config LEDS_TRIGGERS
 	bool "LED Trigger support"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 3fb9641..8e5d0c8 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -51,6 +51,7 @@  obj-$(CONFIG_LEDS_RENESAS_TPU)		+= leds-renesas-tpu.o
 obj-$(CONFIG_LEDS_MAX8997)		+= leds-max8997.o
 obj-$(CONFIG_LEDS_LM355x)		+= leds-lm355x.o
 obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
+obj-$(CONFIG_LEDS_MMIO)			+= leds-mmio.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-mmio.c b/drivers/leds/leds-mmio.c
new file mode 100644
index 0000000..1ef0cda
--- /dev/null
+++ b/drivers/leds/leds-mmio.c
@@ -0,0 +1,250 @@ 
+/*
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Copyright (C) 2012 ARM Limited
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+
+static u32 mmio_led_read(void __iomem *reg, unsigned reg_size)
+{
+	switch (reg_size) {
+	case 32:
+		return readl(reg);
+	case 16:
+		return readw(reg);
+	case 8:
+		return readb(reg);
+	}
+	return 0;
+}
+
+static void mmio_led_write(void __iomem *reg, unsigned reg_size, u32 val)
+{
+	switch (reg_size) {
+	case 32:
+		writel(val, reg);
+		return;
+	case 16:
+		writew(val, reg);
+		return;
+	case 8:
+		writeb(val, reg);
+		return;
+	}
+}
+
+
+struct mmio_led {
+	struct led_classdev cdev;
+	spinlock_t *lock;
+	void __iomem *reg;
+	unsigned reg_size;
+	u32 off_and_mask, off_or_mask;
+	u32 on_and_mask, on_or_mask;
+};
+
+static void mmio_led_brightness_set(struct led_classdev *cdev,
+		enum led_brightness brightness)
+{
+	struct mmio_led *led = container_of(cdev, struct mmio_led, cdev);
+	unsigned long uninitialized_var(flags);
+	u32 val;
+
+	if (led->lock)
+		spin_lock_irqsave(led->lock, flags);
+
+	val = mmio_led_read(led->reg, led->reg_size);
+	if (brightness == LED_OFF) {
+		val &= led->off_and_mask;
+		val |= led->off_or_mask;
+	} else {
+		val &= led->on_and_mask;
+		val |= led->on_or_mask;
+	}
+	mmio_led_write(led->reg, led->reg_size, val);
+
+	if (led->lock)
+		spin_unlock_irqrestore(led->lock, flags);
+};
+
+static enum led_brightness mmio_led_brightness_get(struct led_classdev *cdev)
+{
+	struct mmio_led *led = container_of(cdev, struct mmio_led, cdev);
+	unsigned long uninitialized_var(flags);
+	u32 val;
+
+	if (led->lock)
+		spin_lock_irqsave(led->lock, flags);
+	val = mmio_led_read(led->reg, led->reg_size);
+	if (led->lock)
+		spin_unlock_irqrestore(led->lock, flags);
+
+	if (((val & led->on_and_mask) | led->on_or_mask) == val)
+		return LED_FULL;
+	else
+		return LED_OFF;
+}
+
+struct mmio_led *mmio_led_register(struct device *parent, spinlock_t *lock,
+		const char *name, const char *default_trigger,
+		void __iomem *reg, unsigned reg_size, u32 off_and_mask,
+		u32 off_or_mask, u32 on_and_mask, u32 on_or_mask)
+{
+	struct mmio_led *led;
+	int err;
+
+	if (WARN_ON(reg_size != 8 && reg_size != 16 && reg_size != 32))
+		return ERR_PTR(-EINVAL);
+
+	led = kzalloc(sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return ERR_PTR(-ENOMEM);
+
+	led->cdev.brightness_set = mmio_led_brightness_set;
+	led->cdev.brightness_get = mmio_led_brightness_get;
+	led->cdev.name = name;
+	led->cdev.default_trigger = default_trigger;
+
+	led->lock = lock;
+	led->reg = reg;
+	led->reg_size = reg_size;
+	led->off_and_mask = off_and_mask;
+	led->off_or_mask = off_or_mask;
+	led->on_and_mask = on_and_mask;
+	led->on_or_mask = on_or_mask;
+
+	err = led_classdev_register(parent, &led->cdev);
+	if (err) {
+		kfree(led);
+		return ERR_PTR(err);
+	}
+
+	return led;
+}
+EXPORT_SYMBOL_GPL(mmio_led_register);
+
+void mmio_led_unregister(struct mmio_led *led)
+{
+	led_classdev_unregister(&led->cdev);
+	kfree(led);
+}
+EXPORT_SYMBOL_GPL(mmio_led_unregister);
+
+
+struct mmio_simple_leds {
+	spinlock_t lock;
+	int num_leds;
+	struct mmio_led *leds[0];
+};
+
+static int __devinit mmio_simple_leds_probe(struct platform_device *pdev)
+{
+	struct mmio_simple_leds_platform_data *pdata = pdev->dev.platform_data;
+	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct mmio_simple_leds *leds;
+	void __iomem *reg;
+	u32 val, mask;
+	int i;
+
+	if (!pdata)
+		return -EINVAL;
+
+	if (pdata->reg_size != 8 && pdata->reg_size != 16 &&
+			pdata->reg_size != 32)
+		return -EFAULT;
+
+	leds = devm_kzalloc(&pdev->dev, sizeof(*leds) +
+			sizeof(leds->leds) * pdata->width, GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+	spin_lock_init(&leds->lock);
+
+	if ((!pdev->mfd_cell || !pdev->mfd_cell->ignore_resource_conflicts) &&
+			!devm_request_mem_region(&pdev->dev, res->start,
+			resource_size(res), pdev->name))
+		return -EBUSY;
+
+	reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!reg)
+		return -ENOMEM;
+
+	val = mmio_led_read(reg, pdata->reg_size);
+	mask = ((1 << pdata->width) - 1) << pdata->shift;
+	if ((pdata->init_full && pdata->active_low) ||
+			(pdata->init_off && !pdata->active_low))
+		val &= ~mask;
+	else if ((pdata->init_off && pdata->active_low) ||
+			(pdata->init_full && !pdata->active_low))
+		val |= mask;
+	mmio_led_write(reg, pdata->reg_size, val);
+
+	leds->num_leds = pdata->width;
+	for (i = 0; i < leds->num_leds; i++) {
+		unsigned shift = pdata->shift + i;
+		u32 and_mask = ~BIT(shift);
+		u32 off_or_mask = pdata->active_low ? BIT(shift) : 0;
+		u32 on_or_mask = pdata->active_low ? 0 : BIT(shift);
+		struct mmio_led *led = mmio_led_register(&pdev->dev,
+				&leds->lock, pdata->names[i],
+				pdata->default_triggers ?
+				pdata->default_triggers[i] : NULL,
+				reg, pdata->reg_size, and_mask, off_or_mask,
+				and_mask, on_or_mask);
+
+		if (IS_ERR(led)) {
+			while (--i >= 0)
+				mmio_led_unregister(leds->leds[i]);
+			return PTR_ERR(led);
+		}
+		leds->leds[i] = led;
+	}
+
+	platform_set_drvdata(pdev, leds);
+
+	return 0;
+}
+
+static int __devexit mmio_simple_leds_remove(struct platform_device *pdev)
+{
+	struct mmio_simple_leds *leds = platform_get_drvdata(pdev);
+	int i;
+
+	platform_set_drvdata(pdev, NULL);
+
+	for (i = 0; i < leds->num_leds; i++)
+		mmio_led_unregister(leds->leds[i]);
+
+	return 0;
+}
+
+static struct platform_driver mmio_simple_leds_driver = {
+	.probe		= mmio_simple_leds_probe,
+	.remove		= __devexit_p(mmio_simple_leds_remove),
+	.driver		= {
+		.name	= "leds-mmio-simple",
+		.owner	= THIS_MODULE,
+	},
+};
+
+module_platform_driver(mmio_simple_leds_driver);
+
+MODULE_AUTHOR("Pawel Moll <pawel.moll@arm.com>");
+MODULE_DESCRIPTION("MMIO LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-mmio-simple");
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6e53bb3..a6338b0 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -241,6 +241,44 @@  struct gpio_led_platform_data {
 struct platform_device *gpio_led_register_device(
 		int id, const struct gpio_led_platform_data *pdata);
 
+/* For the leds-mmio driver */
+
+struct mmio_led;
+
+#if defined(CONFIG_LEDS_MMIO)
+/* Returns ERR_PTR in case of error */
+struct mmio_led *mmio_led_register(struct device *parent, spinlock_t *lock,
+		const char *name, const char *default_trigger,
+		void __iomem *reg, unsigned reg_size, u32 off_and_mask,
+		u32 off_or_mask, u32 on_and_mask, u32 on_or_mask);
+void mmio_led_unregister(struct mmio_led *led);
+#else
+struct mmio_led *mmio_led_register(struct device *parent, spinlock_t *lock,
+		const char *name, const char *default_trigger,
+		void __iomem *reg, unsigned reg_size, u32 off_and_mask,
+		u32 off_or_mask, u32 on_and_mask, u32 on_or_mask)
+{
+	return NULL;
+}
+
+void mmio_led_unregister(struct mmio_led *led)
+{
+}
+#endif
+
+struct mmio_simple_leds_platform_data {
+	unsigned reg_size; /* Register size (8/16/32) */
+	unsigned shift; /* First bit controlling LEDs */
+	unsigned width; /* Number of consecutive bits */
+	const char **names; /* Must define 'width' names */
+	const char **default_triggers; /* NULL or 'width' strings */
+	bool active_low;
+	bool init_full;
+	bool init_off;
+};
+
+/* CPU led trigger */
+
 enum cpu_led_event {
 	CPU_LED_IDLE_START,	/* CPU enters idle */
 	CPU_LED_IDLE_END,	/* CPU idle ends */