2012-11-01 17:59:24

by Pawel Moll

[permalink] [raw]
Subject: [PATCH 1/2] leds: Add generic support for memory mapped LEDs

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 <[email protected]>
---
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 <[email protected]>");
+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


2012-11-01 17:59:20

by Pawel Moll

[permalink] [raw]
Subject: [PATCH 2/2] mfd: vexpress-sysreg: Use MMIO driver for platform LEDs

This patch removes custom code for controlling LEDs on
Versatile Express platform and register MFD cell for
the SYS_LED register for the leds-mmio-simple driver.

Signed-off-by: Pawel Moll <[email protected]>
---
drivers/mfd/Kconfig | 1 +
drivers/mfd/vexpress-sysreg.c | 125 +++++++++++++++--------------------------
2 files changed, 46 insertions(+), 80 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 637bcdf..611d989 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1073,6 +1073,7 @@ endmenu

config VEXPRESS_CONFIG
bool
+ select MFD_CORE
help
Platform configuration infrastructure for the ARM Ltd.
Versatile Express.
diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
index 059d6b1..2241b98 100644
--- a/drivers/mfd/vexpress-sysreg.c
+++ b/drivers/mfd/vexpress-sysreg.c
@@ -15,6 +15,7 @@
#include <linux/gpio.h>
#include <linux/io.h>
#include <linux/leds.h>
+#include <linux/mfd/core.h>
#include <linux/of_address.h>
#include <linux/platform_device.h>
#include <linux/regulator/driver.h>
@@ -406,6 +407,35 @@ static struct gpio_chip vexpress_sysreg_gpio_chip = {
};


+static struct mfd_cell vexpress_sysreg_leds = {
+ .name = "leds-mmio-simple",
+ .platform_data = &(struct mmio_simple_leds_platform_data) {
+ .reg_size = 32,
+ .shift = 0,
+ .width = 8,
+ .names = (const char *[]) {
+ "v2m:green:user1", "v2m:green:user2",
+ "v2m:green:user3", "v2m:green:user4",
+ "v2m:green:user5", "v2m:green:user6",
+ "v2m:green:user7", "v2m:green:user8",
+ },
+ .default_triggers = (const char *[]) {
+ "heartbeat", "mmc0",
+ "cpu0", "cpu1",
+ "cpu2", "cpu3",
+ "cpu4", "cpu5",
+ },
+ .init_off = true,
+ },
+ .pdata_size = sizeof(struct mmio_simple_leds_platform_data),
+ .num_resources = 1,
+ .resources = (struct resource []) {
+ DEFINE_RES_MEM(SYS_LED, 4),
+ },
+ .ignore_resource_conflicts = true,
+};
+
+
static ssize_t vexpress_sysreg_sys_id_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -438,14 +468,19 @@ static int __devinit vexpress_sysreg_probe(struct platform_device *pdev)
setup_timer(&vexpress_sysreg_config_timer,
vexpress_sysreg_config_complete, 0);

+ err = mfd_add_devices(&pdev->dev, 0, &vexpress_sysreg_leds, 1,
+ res, 0, NULL);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to add LED device! (%d)\n", err);
+ goto error_mfd_add_devices;
+ }
+
vexpress_sysreg_gpio_chip.dev = &pdev->dev;
err = gpiochip_add(&vexpress_sysreg_gpio_chip);
if (err) {
- vexpress_config_bridge_unregister(
- vexpress_sysreg_config_bridge);
dev_err(&pdev->dev, "Failed to register GPIO chip! (%d)\n",
err);
- return err;
+ goto error_gpiochip_add;
}

vexpress_sysreg_dev = &pdev->dev;
@@ -453,6 +488,13 @@ static int __devinit vexpress_sysreg_probe(struct platform_device *pdev)
device_create_file(vexpress_sysreg_dev, &dev_attr_sys_id);

return 0;
+
+error_gpiochip_add:
+ mfd_remove_devices(&pdev->dev);
+error_mfd_add_devices:
+ vexpress_config_bridge_unregister(vexpress_sysreg_config_bridge);
+ return err;
+
}

static const struct of_device_id vexpress_sysreg_match[] = {
@@ -473,80 +515,3 @@ static int __init vexpress_sysreg_init(void)
return platform_driver_register(&vexpress_sysreg_driver);
}
core_initcall(vexpress_sysreg_init);
-
-
-#if defined(CONFIG_LEDS_CLASS)
-
-struct vexpress_sysreg_led {
- u32 mask;
- struct led_classdev cdev;
-} vexpress_sysreg_leds[] = {
- { .mask = 1 << 0, .cdev.name = "v2m:green:user1",
- .cdev.default_trigger = "heartbeat", },
- { .mask = 1 << 1, .cdev.name = "v2m:green:user2",
- .cdev.default_trigger = "mmc0", },
- { .mask = 1 << 2, .cdev.name = "v2m:green:user3",
- .cdev.default_trigger = "cpu0", },
- { .mask = 1 << 3, .cdev.name = "v2m:green:user4",
- .cdev.default_trigger = "cpu1", },
- { .mask = 1 << 4, .cdev.name = "v2m:green:user5",
- .cdev.default_trigger = "cpu2", },
- { .mask = 1 << 5, .cdev.name = "v2m:green:user6",
- .cdev.default_trigger = "cpu3", },
- { .mask = 1 << 6, .cdev.name = "v2m:green:user7",
- .cdev.default_trigger = "cpu4", },
- { .mask = 1 << 7, .cdev.name = "v2m:green:user8",
- .cdev.default_trigger = "cpu5", },
-};
-
-static DEFINE_SPINLOCK(vexpress_sysreg_leds_lock);
-
-static void vexpress_sysreg_led_brightness_set(struct led_classdev *cdev,
- enum led_brightness brightness)
-{
- struct vexpress_sysreg_led *led = container_of(cdev,
- struct vexpress_sysreg_led, cdev);
- unsigned long flags;
- u32 val;
-
- spin_lock_irqsave(&vexpress_sysreg_leds_lock, flags);
-
- val = readl(vexpress_sysreg_base + SYS_LED);
- if (brightness == LED_OFF)
- val &= ~led->mask;
- else
- val |= led->mask;
- writel(val, vexpress_sysreg_base + SYS_LED);
-
- spin_unlock_irqrestore(&vexpress_sysreg_leds_lock, flags);
-}
-
-static int __init vexpress_sysreg_init_leds(void)
-{
- struct vexpress_sysreg_led *led;
- int i;
-
- /* Clear all user LEDs */
- writel(0, vexpress_sysreg_base + SYS_LED);
-
- for (i = 0, led = vexpress_sysreg_leds;
- i < ARRAY_SIZE(vexpress_sysreg_leds); i++, led++) {
- int err;
-
- led->cdev.brightness_set = vexpress_sysreg_led_brightness_set;
- err = led_classdev_register(vexpress_sysreg_dev, &led->cdev);
- if (err) {
- dev_err(vexpress_sysreg_dev,
- "Failed to register LED %d! (%d)\n",
- i, err);
- while (led--, i--)
- led_classdev_unregister(&led->cdev);
- return err;
- }
- }
-
- return 0;
-}
-device_initcall(vexpress_sysreg_init_leds);
-
-#endif
--
1.7.10.4

2012-11-05 09:41:05

by Jon Medhurst (Tixy)

[permalink] [raw]
Subject: Re: [PATCH 1/2] leds: Add generic support for memory mapped LEDs

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 <[email protected]>
> ---
> 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 <[email protected]>");
> +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 */


--
Tixy

2012-11-05 12:56:05

by Pawel Moll

[permalink] [raw]
Subject: Re: [PATCH 1/2] leds: Add generic support for memory mapped LEDs

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ł

2012-11-05 13:52:59

by Jon Medhurst (Tixy)

[permalink] [raw]
Subject: Re: [PATCH 1/2] leds: Add generic support for memory mapped LEDs

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.

--
Tixy

2012-11-08 15:35:58

by Pawel Moll

[permalink] [raw]
Subject: Re: [PATCH 1/2] leds: Add generic support for memory mapped LEDs

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 <[email protected]>

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

Cheers!

Paweł

2012-11-19 11:44:57

by Pawel Moll

[permalink] [raw]
Subject: Re: [PATCH 1/2] leds: Add generic support for memory mapped LEDs

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 <[email protected]>
>
> It's just a friendly and polite nag - any thoughts or feelings about
> this?

Ping again?

Paweł

2012-11-26 15:25:35

by Pawel Moll

[permalink] [raw]
Subject: Re: [PATCH 1/2] leds: Add generic support for memory mapped LEDs

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 <[email protected]>
> >
> > It's just a friendly and polite nag - any thoughts or feelings about
> > this?
>
> Ping again?

And another polite nag for this week...

Paweł

2012-11-26 15:37:39

by Vasily Khoruzhick

[permalink] [raw]
Subject: Re: [PATCH 1/2] leds: Add generic support for memory mapped LEDs

On Thu, Nov 1, 2012 at 8:58 PM, Pawel Moll <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>");
> +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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2012-11-26 16:10:42

by Pawel Moll

[permalink] [raw]
Subject: Re: [PATCH 1/2] leds: Add generic support for memory mapped LEDs

On Mon, 2012-11-26 at 15:37 +0000, Vasily Khoruzhick wrote:
> On Thu, Nov 1, 2012 at 8:58 PM, Pawel Moll <[email protected]> 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ł