* [PATCH 2/2] mfd: vexpress-sysreg: Use MMIO driver for platform LEDs
2012-11-01 17:58 [PATCH 1/2] leds: Add generic support for memory mapped LEDs Pawel Moll
@ 2012-11-01 17:58 ` Pawel Moll
2012-11-05 9:39 ` [PATCH 1/2] leds: Add generic support for memory mapped LEDs Jon Medhurst (Tixy)
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Pawel Moll @ 2012-11-01 17:58 UTC (permalink / raw)
To: linux-arm-kernel
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 <pawel.moll@arm.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 1/2] leds: Add generic support for memory mapped LEDs
2012-11-01 17:58 [PATCH 1/2] leds: Add generic support for memory mapped LEDs Pawel Moll
2012-11-01 17:58 ` [PATCH 2/2] mfd: vexpress-sysreg: Use MMIO driver for platform LEDs Pawel Moll
@ 2012-11-05 9:39 ` Jon Medhurst (Tixy)
2012-11-05 12:55 ` Pawel Moll
2012-11-08 15:35 ` Pawel Moll
2012-11-26 15:37 ` Vasily Khoruzhick
3 siblings, 1 reply; 10+ messages in thread
From: Jon Medhurst (Tixy) @ 2012-11-05 9:39 UTC (permalink / raw)
To: linux-arm-kernel
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 */
--
Tixy
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] leds: Add generic support for memory mapped LEDs
2012-11-05 9:39 ` [PATCH 1/2] leds: Add generic support for memory mapped LEDs Jon Medhurst (Tixy)
@ 2012-11-05 12:55 ` Pawel Moll
2012-11-05 13:52 ` Jon Medhurst (Tixy)
0 siblings, 1 reply; 10+ messages in thread
From: Pawel Moll @ 2012-11-05 12:55 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] leds: Add generic support for memory mapped LEDs
2012-11-05 12:55 ` Pawel Moll
@ 2012-11-05 13:52 ` Jon Medhurst (Tixy)
0 siblings, 0 replies; 10+ messages in thread
From: Jon Medhurst (Tixy) @ 2012-11-05 13:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] leds: Add generic support for memory mapped LEDs
2012-11-01 17:58 [PATCH 1/2] leds: Add generic support for memory mapped LEDs Pawel Moll
2012-11-01 17:58 ` [PATCH 2/2] mfd: vexpress-sysreg: Use MMIO driver for platform LEDs Pawel Moll
2012-11-05 9:39 ` [PATCH 1/2] leds: Add generic support for memory mapped LEDs Jon Medhurst (Tixy)
@ 2012-11-08 15:35 ` Pawel Moll
2012-11-19 11:44 ` Pawel Moll
2012-11-26 15:37 ` Vasily Khoruzhick
3 siblings, 1 reply; 10+ messages in thread
From: Pawel Moll @ 2012-11-08 15:35 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] leds: Add generic support for memory mapped LEDs
2012-11-08 15:35 ` Pawel Moll
@ 2012-11-19 11:44 ` Pawel Moll
2012-11-26 15:25 ` Pawel Moll
0 siblings, 1 reply; 10+ messages in thread
From: Pawel Moll @ 2012-11-19 11:44 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] leds: Add generic support for memory mapped LEDs
2012-11-19 11:44 ` Pawel Moll
@ 2012-11-26 15:25 ` Pawel Moll
0 siblings, 0 replies; 10+ messages in thread
From: Pawel Moll @ 2012-11-26 15:25 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] leds: Add generic support for memory mapped LEDs
2012-11-01 17:58 [PATCH 1/2] leds: Add generic support for memory mapped LEDs Pawel Moll
` (2 preceding siblings ...)
2012-11-08 15:35 ` Pawel Moll
@ 2012-11-26 15:37 ` Vasily Khoruzhick
2012-11-26 16:10 ` Pawel Moll
3 siblings, 1 reply; 10+ messages in thread
From: Vasily Khoruzhick @ 2012-11-26 15:37 UTC (permalink / raw)
To: linux-arm-kernel
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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] leds: Add generic support for memory mapped LEDs
2012-11-26 15:37 ` Vasily Khoruzhick
@ 2012-11-26 16:10 ` Pawel Moll
0 siblings, 0 replies; 10+ messages in thread
From: Pawel Moll @ 2012-11-26 16:10 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 10+ messages in thread