Re: [PATCH v4 2/2] leds: add BCM6328 LED driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Alvaro,

Thanks for the update. I see one minor issue below.

On 04/26/2015 02:15 PM, Álvaro Fernández Rojas wrote:
This adds support for the LED controller on Broadcom's BCM6328.

Signed-off-by: Álvaro Fernández Rojas <noltari@xxxxxxxxx>
Signed-off-by: Jonas Gorski <jogo@xxxxxxxxxxx>
---
  v4: resend, no changes.
  v3: introduce changes suggested by Jacek
  - Revert compatible string to "brcm,bcm6328-leds".
  - Add BCM6328_ prefix to macros.
  - Rename blink_del to blink_delay.
  v2: introduce changes suggested by Florian and Jonas.
  - Improve Kconfig description.
  - Remove unused imports.
  - Fix bug in blink_set so it works for multiple LEDs.
  - "brcm,link-selection" -> "brcm,link-signal-sources".
  - "brcm,activity-selection" -> "brcm,activity-signal-sources".
  - Use of_property_read_bool instead of of_get_property for booleans.
  - Add MODULE_AUTHOR strings.

  drivers/leds/Kconfig        |   8 +
  drivers/leds/Makefile       |   1 +
  drivers/leds/leds-bcm6328.c | 413 ++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 422 insertions(+)
  create mode 100644 drivers/leds/leds-bcm6328.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 966b960..caa8ee6 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -42,6 +42,14 @@ config LEDS_88PM860X
  	  This option enables support for on-chip LED drivers found on Marvell
  	  Semiconductor 88PM8606 PMIC.

+config LEDS_BCM6328
+	tristate "LED Support for Broadcom BCM6328"
+	depends on LEDS_CLASS
+	depends on OF
+	help
+	  This option enables support for LEDs connected to the BCM6328
+	  LED HW controller accessed via MMIO registers.
+
  config LEDS_LM3530
  	tristate "LCD Backlight driver for LM3530"
  	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index bf46093..309a46b 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o

  # LED Platform Drivers
  obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
+obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
  obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
  obj-$(CONFIG_LEDS_LOCOMO)		+= leds-locomo.o
  obj-$(CONFIG_LEDS_LM3530)		+= leds-lm3530.o
diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c
new file mode 100644
index 0000000..d2bcbc3
--- /dev/null
+++ b/drivers/leds/leds-bcm6328.c
@@ -0,0 +1,413 @@
+/*
+ * Driver for BCM6328 memory-mapped LEDs, based on leds-syscon.c
+ *
+ * Copyright 2015 Álvaro Fernández Rojas <noltari@xxxxxxxxx>
+ * Copyright 2015 Jonas Gorski <jogo@xxxxxxxxxxx>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+#include <linux/io.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define BCM6328_REG_INIT		0x00
+#define BCM6328_REG_MODE_HI		0x04
+#define BCM6328_REG_MODE_LO		0x08
+#define BCM6328_REG_HWDIS		0x0c
+#define BCM6328_REG_STROBE		0x10
+#define BCM6328_REG_LNKACTSEL_HI	0x14
+#define BCM6328_REG_LNKACTSEL_LO	0x18
+#define BCM6328_REG_RBACK		0x1c
+#define BCM6328_REG_SERMUX		0x20
+
+#define BCM6328_LED_MAX_COUNT		24
+#define BCM6328_LED_DEF_DELAY		500
+#define BCM6328_LED_INTERVAL_MS		20
+
+#define BCM6328_LED_INTV_MASK		0x3f
+#define BCM6328_LED_FAST_INTV_SHIFT	6
+#define BCM6328_LED_FAST_INTV_MASK	(BCM6328_LED_INTV_MASK << \
+					 BCM6328_LED_FAST_INTV_SHIFT)
+#define BCM6328_SERIAL_LED_EN		BIT(12)
+#define BCM6328_SERIAL_LED_MUX		BIT(13)
+#define BCM6328_SERIAL_LED_CLK_NPOL	BIT(14)
+#define BCM6328_SERIAL_LED_DATA_PPOL	BIT(15)
+#define BCM6328_SERIAL_LED_SHIFT_DIR	BIT(16)
+#define BCM6328_LED_SHIFT_TEST		BIT(30)
+#define BCM6328_LED_TEST		BIT(31)
+
+#define BCM6328_LED_MODE_MASK		3
+#define BCM6328_LED_MODE_OFF		0
+#define BCM6328_LED_MODE_FAST		1
+#define BCM6328_LED_MODE_BLINK		2
+#define BCM6328_LED_MODE_ON		3
+#define BCM6328_LED_SHIFT(X)		((X) << 1)
+
+/**
+ * struct bcm6328_led - state container for bcm6328 based LEDs
+ * @cdev: LED class device for this LED
+ * @mem: memory resource
+ * @lock: memory lock
+ * @pin: LED pin number
+ * @blink_leds: blinking LEDs
+ * @blink_delay: blinking delay
+ * @active_low: LED is active low
+ */
+struct bcm6328_led {
+	struct led_classdev cdev;
+	void __iomem *mem;
+	spinlock_t *lock;
+	unsigned long pin;
+	unsigned long *blink_leds;
+	unsigned long *blink_delay;
+	bool active_low;
+};
+
+static void bcm6328_led_write(void __iomem *reg, unsigned long data)
+{
+	iowrite32be(data, reg);
+}
+
+static unsigned long bcm6328_led_read(void __iomem *reg)
+{
+	return ioread32be(reg);
+}
+
+/**
+ * LEDMode 64 bits / 24 LEDs
+ * bits [31:0] -> LEDs 8-23
+ * bits [47:32] -> LEDs 0-7
+ * bits [63:48] -> unused
+ */
+static unsigned long bcm6328_pin2shift(unsigned long pin)
+{
+	if (pin < 8)
+		return pin + 16; /* LEDs 0-7 (bits 47:32) */
+	else
+		return pin - 8; /* LEDs 8-23 (bits 31:0) */
+}
+
+static void bcm6328_led_mode(struct bcm6328_led *led, unsigned long value)
+{
+	void __iomem *mode;
+	unsigned long val, shift;
+
+	shift = bcm6328_pin2shift(led->pin);
+	if (shift / 16)
+		mode = led->mem + BCM6328_REG_MODE_HI;
+	else
+		mode = led->mem + BCM6328_REG_MODE_LO;
+
+	val = bcm6328_led_read(mode);
+	val &= ~(BCM6328_LED_MODE_MASK << BCM6328_LED_SHIFT(shift % 16));
+	val |= (value << BCM6328_LED_SHIFT(shift % 16));
+	bcm6328_led_write(mode, val);
+}
+
+static void bcm6328_led_set(struct led_classdev *led_cdev,
+			    enum led_brightness value)
+{
+	struct bcm6328_led *led =
+		container_of(led_cdev, struct bcm6328_led, cdev);
+	unsigned long flags;
+
+	spin_lock_irqsave(led->lock, flags);
+	*(led->blink_leds) &= ~BIT(led->pin);
+	if ((led->active_low && value == LED_OFF) ||
+	    (!led->active_low && value != LED_OFF))
+		bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
+	else
+		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
+	spin_unlock_irqrestore(led->lock, flags);
+}
+
+static int bcm6328_blink_set(struct led_classdev *led_cdev,
+			     unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct bcm6328_led *led =
+		container_of(led_cdev, struct bcm6328_led, cdev);
+	unsigned long delay, flags;
+
+	if (!*delay_on)
+		*delay_on = BCM6328_LED_DEF_DELAY;
+	if (!*delay_off)
+		*delay_off = BCM6328_LED_DEF_DELAY;
+
+	if (*delay_on != *delay_off) {
+		dev_dbg(led_cdev->dev,
+			"fallback to soft blinking (delay_on != delay_off)\n");
+		return -EINVAL;
+	}
+
+	delay = *delay_on / BCM6328_LED_INTERVAL_MS;
+	if (delay == 0)
+		delay = 1;
+	else if (delay > BCM6328_LED_INTV_MASK) {
+		dev_dbg(led_cdev->dev,
+			"fallback to soft blinking (delay > %ums)\n",
+			BCM6328_LED_INTV_MASK * BCM6328_LED_INTERVAL_MS);
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(led->lock, flags);
+	if (*(led->blink_leds) == 0 ||
+	    *(led->blink_leds) == BIT(led->pin) ||
+	    *(led->blink_delay) == delay) {
+		unsigned long val;
+
+		*(led->blink_leds) |= BIT(led->pin);
+		*(led->blink_delay) = delay;
+
+		val = bcm6328_led_read(led->mem + BCM6328_REG_INIT);
+		val &= ~BCM6328_LED_FAST_INTV_MASK;
+		val |= (delay << BCM6328_LED_FAST_INTV_SHIFT);
+		bcm6328_led_write(led->mem + BCM6328_REG_INIT, val);
+
+		bcm6328_led_mode(led, BCM6328_LED_MODE_BLINK);
+
+		spin_unlock_irqrestore(led->lock, flags);
+	} else {
+		spin_unlock_irqrestore(led->lock, flags);
+		dev_dbg(led_cdev->dev,
+			"fallback to soft blinking (delay already set)\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int bcm6328_hwled(struct device *dev, struct device_node *nc, u32 reg,
+			 void __iomem *mem, spinlock_t *lock)
+{
+	int i, cnt;
+	unsigned long flags, val;
+
+	spin_lock_irqsave(lock, flags);
+	val = bcm6328_led_read(mem + BCM6328_REG_HWDIS);
+	val &= ~BIT(reg);
+	bcm6328_led_write(mem + BCM6328_REG_HWDIS, val);
+	spin_unlock_irqrestore(lock, flags);
+
+	/* Only LEDs 0-7 can be activity/link controlled */
+	if (reg >= 8)
+		return 0;
+
+	cnt = of_property_count_elems_of_size(nc, "brcm,link-signal-sources",
+					      sizeof(u32));
+	for (i = 0; i < cnt; i++) {
+		u32 sel;
+		void __iomem *addr;
+
+		if (reg < 4)
+			addr = mem + BCM6328_REG_LNKACTSEL_LO;
+		else
+			addr = mem + BCM6328_REG_LNKACTSEL_HI;
+
+		of_property_read_u32_index(nc, "brcm,link-signal-sources", i,
+					   &sel);
+
+		if (reg / 4 != sel / 4) {
+			dev_warn(dev, "invalid link signal source\n");
+			continue;
+		}
+
+		spin_lock_irqsave(lock, flags);
+		val = bcm6328_led_read(addr);
+		val |= (BIT(reg) << (((sel % 4) * 4) + 16));
+		bcm6328_led_write(addr, val);
+		spin_unlock_irqrestore(lock, flags);
+	}
+
+	cnt = of_property_count_elems_of_size(nc,
+					      "brcm,activity-signal-sources",
+					      sizeof(u32));
+	for (i = 0; i < cnt; i++) {
+		u32 sel;
+		void __iomem *addr;
+
+		if (reg < 4)
+			addr = mem + BCM6328_REG_LNKACTSEL_LO;
+		else
+			addr = mem + BCM6328_REG_LNKACTSEL_HI;
+
+		of_property_read_u32_index(nc, "brcm,activity-signal-sources",
+					   i, &sel);
+
+		if (reg / 4 != sel / 4) {
+			dev_warn(dev, "invalid activity signal source\n");
+			continue;
+		}
+
+		spin_lock_irqsave(lock, flags);
+		val = bcm6328_led_read(addr);
+		val |= (BIT(reg) << ((sel % 4) * 4));
+		bcm6328_led_write(addr, val);
+		spin_unlock_irqrestore(lock, flags);
+	}
+
+	return 0;
+}
+
+static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
+		       void __iomem *mem, spinlock_t *lock,
+		       unsigned long *blink_leds, unsigned long *blink_delay)
+{
+	struct bcm6328_led *led;
+	unsigned long flags;
+	const char *state;
+	int rc;
+
+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->pin = reg;
+	led->mem = mem;
+	led->lock = lock;
+	led->blink_leds = blink_leds;
+	led->blink_delay = blink_delay;
+
+	if (of_property_read_bool(nc, "active-low"))
+		led->active_low = 1;

active_low is of bool type, so let's assign 'true' here instead of 1.

Having this fixed:

Acked-by: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>

+
+	led->cdev.name = of_get_property(nc, "label", NULL) ? : nc->name;
+	led->cdev.default_trigger = of_get_property(nc,
+						    "linux,default-trigger",
+						    NULL);
+
+	if (!of_property_read_string(nc, "default-state", &state)) {
+		spin_lock_irqsave(lock, flags);
+		if (!strcmp(state, "on")) {
+			led->cdev.brightness = LED_FULL;
+			bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
+		} else if (!strcmp(state, "keep")) {
+			void __iomem *mode;
+			unsigned long val, shift;
+
+			shift = bcm6328_pin2shift(led->pin);
+			if (shift / 16)
+				mode = mem + BCM6328_REG_MODE_HI;
+			else
+				mode = mem + BCM6328_REG_MODE_LO;
+
+			val = bcm6328_led_read(mode) >> (shift % 16);
+			val &= BCM6328_LED_MODE_MASK;
+			if (val == BCM6328_LED_MODE_ON)
+				led->cdev.brightness = LED_FULL;
+			else {
+				led->cdev.brightness = LED_OFF;
+				bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
+			}
+		} else {
+			led->cdev.brightness = LED_OFF;
+			bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
+		}
+		spin_unlock_irqrestore(lock, flags);
+	}
+
+	led->cdev.brightness_set = bcm6328_led_set;
+	led->cdev.blink_set = bcm6328_blink_set;
+
+	rc = led_classdev_register(dev, &led->cdev);
+	if (rc < 0)
+		return rc;
+
+	dev_dbg(dev, "registered LED %s\n", led->cdev.name);
+
+	return 0;
+}
+
+static int bcm6328_leds_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *child;
+	struct resource *mem_r;
+	void __iomem *mem;
+	spinlock_t *lock;
+	unsigned long val, *blink_leds, *blink_delay;
+
+	mem_r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem_r)
+		return -EINVAL;
+
+	mem = devm_ioremap_resource(dev, mem_r);
+	if (IS_ERR(mem))
+		return PTR_ERR(mem);
+
+	lock = devm_kzalloc(dev, sizeof(*lock), GFP_KERNEL);
+	if (!lock)
+		return -ENOMEM;
+
+	blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL);
+	if (!blink_leds)
+		return -ENOMEM;
+
+	blink_delay = devm_kzalloc(dev, sizeof(*blink_delay), GFP_KERNEL);
+	if (!blink_delay)
+		return -ENOMEM;
+
+	spin_lock_init(lock);
+
+	bcm6328_led_write(mem + BCM6328_REG_HWDIS, ~0);
+	bcm6328_led_write(mem + BCM6328_REG_LNKACTSEL_HI, 0);
+	bcm6328_led_write(mem + BCM6328_REG_LNKACTSEL_LO, 0);
+
+	val = bcm6328_led_read(mem + BCM6328_REG_INIT);
+	val &= ~BCM6328_SERIAL_LED_EN;
+	if (of_property_read_bool(np, "brcm,serial-leds"))
+		val |= BCM6328_SERIAL_LED_EN;
+	bcm6328_led_write(mem + BCM6328_REG_INIT, val);
+
+	for_each_available_child_of_node(np, child) {
+		int rc;
+		u32 reg;
+
+		if (of_property_read_u32(child, "reg", &reg))
+			continue;
+
+		if (reg >= BCM6328_LED_MAX_COUNT) {
+			dev_err(dev, "invalid LED (>= %d)\n",
+				BCM6328_LED_MAX_COUNT);
+			continue;
+		}
+
+		if (of_property_read_bool(child, "brcm,hardware-controlled"))
+			rc = bcm6328_hwled(dev, child, reg, mem, lock);
+		else
+			rc = bcm6328_led(dev, child, reg, mem, lock,
+					 blink_leds, blink_delay);
+
+		if (rc < 0)
+			return rc;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id bcm6328_leds_of_match[] = {
+	{ .compatible = "brcm,bcm6328-leds", },
+	{ },
+};
+
+static struct platform_driver bcm6328_leds_driver = {
+	.probe = bcm6328_leds_probe,
+	.driver = {
+		.name = "leds-bcm6328",
+		.of_match_table = bcm6328_leds_of_match,
+	},
+};
+
+module_platform_driver(bcm6328_leds_driver);
+
+MODULE_AUTHOR("Álvaro Fernández Rojas <noltari@xxxxxxxxx>");
+MODULE_AUTHOR("Jonas Gorski <jogo@xxxxxxxxxxx>");
+MODULE_DESCRIPTION("LED driver for BCM6328 controllers");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:leds-bcm6328");



--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux