Hi, On Mon, Mar 16, 2015 at 5:05 PM, Álvaro Fernández Rojas <[email protected]> wrote: > - Adds LEDs driver compatible with 6318/6328/6362/63268 > - Set serial LEDs pinmux for 63268 only (6328 and 6362 boards have LEDs on > these GPIOs) > - Refresh patches
I ignored most line break issues, but my comments about the code and bindings: > Signed-off-by: Álvaro Fernández Rojas <[email protected]> > --- > target/linux/brcm63xx/config-3.18 | 1 + > target/linux/brcm63xx/dts/bcm6318.dtsi | 10 + > target/linux/brcm63xx/dts/bcm63268.dtsi | 10 + > target/linux/brcm63xx/dts/bcm6328.dtsi | 10 + > target/linux/brcm63xx/dts/bcm6362.dtsi | 10 + > .../379-BCM63XX-pinmux-serial-LEDs.patch | 13 + > .../patches-3.18/380-BCM63XX-add-LEDs-driver.patch | 398 > +++++++++++++++++++++ > .../403-6358-enet1-external-mii-clk.patch | 2 +- > ...-allow-providing-fixup-data-in-board-data.patch | 4 +- > ...8-MIPS-BCM63XX-pass-caldata-info-to-flash.patch | 2 +- > .../420-BCM63XX-add-endian-check-for-ath9k.patch | 2 +- > .../421-BCM63XX-add-led-pin-for-ath9k.patch | 2 +- > ...22-BCM63XX-add-a-fixup-for-rt2x00-devices.patch | 2 +- > 13 files changed, 459 insertions(+), 7 deletions(-) > create mode 100644 > target/linux/brcm63xx/patches-3.18/379-BCM63XX-pinmux-serial-LEDs.patch > create mode 100644 > target/linux/brcm63xx/patches-3.18/380-BCM63XX-add-LEDs-driver.patch > > diff --git a/target/linux/brcm63xx/config-3.18 > b/target/linux/brcm63xx/config-3.18 > index 0d7a5c7..227157e 100644 > --- a/target/linux/brcm63xx/config-3.18 > +++ b/target/linux/brcm63xx/config-3.18 > @@ -133,6 +133,7 @@ CONFIG_IRQ_DOMAIN=y > CONFIG_IRQ_FORCED_THREADING=y > CONFIG_IRQ_WORK=y > CONFIG_KEXEC=y > +CONFIG_LEDS_BCM6328=y > CONFIG_LEDS_GPIO=y > CONFIG_LIBFDT=y > CONFIG_MDIO_BOARDINFO=y > diff --git a/target/linux/brcm63xx/dts/bcm6318.dtsi > b/target/linux/brcm63xx/dts/bcm6318.dtsi > index f851a9c..bcddcc5 100644 > --- a/target/linux/brcm63xx/dts/bcm6318.dtsi > +++ b/target/linux/brcm63xx/dts/bcm6318.dtsi > @@ -6,6 +6,7 @@ > aliases { > gpio0 = &gpio0; > gpio1 = &gpio1; > + leds0 = &leds0; > }; > > cpus { > @@ -74,5 +75,14 @@ > gpio-controller; > #gpio-cells = <2>; > }; > + > + leds0: led-controller@10000200 { > + compatible = "brcm,bcm6318-leds"; > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0x10000200 0x24>; > + > + status = "disabled"; > + }; > }; > }; > diff --git a/target/linux/brcm63xx/dts/bcm63268.dtsi > b/target/linux/brcm63xx/dts/bcm63268.dtsi > index 0a1f8b1..144cf05 100644 > --- a/target/linux/brcm63xx/dts/bcm63268.dtsi > +++ b/target/linux/brcm63xx/dts/bcm63268.dtsi > @@ -6,6 +6,7 @@ > aliases { > gpio0 = &gpio0; > gpio1 = &gpio1; > + leds0 = &leds0; > }; > > cpus { > @@ -81,5 +82,14 @@ > gpio-controller; > #gpio-cells = <2>; > }; > + > + leds0: led-controller@10001900 { > + compatible = "brcm,bcm63268-leds"; > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0x10001900 0x24>; > + > + status = "disabled"; > + }; > }; > }; > diff --git a/target/linux/brcm63xx/dts/bcm6328.dtsi > b/target/linux/brcm63xx/dts/bcm6328.dtsi > index a0b1316..d570e06 100644 > --- a/target/linux/brcm63xx/dts/bcm6328.dtsi > +++ b/target/linux/brcm63xx/dts/bcm6328.dtsi > @@ -5,6 +5,7 @@ > > aliases { > gpio0 = &gpio0; > + leds0 = &leds0; > }; > > cpus { > @@ -63,5 +64,14 @@ > gpio-controller; > #gpio-cells = <2>; > }; > + > + leds0: led-controller@10000800 { > + compatible = "brcm,bcm6328-leds"; > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0x10000800 0x24>; > + > + status = "disabled"; > + }; > }; > }; > diff --git a/target/linux/brcm63xx/dts/bcm6362.dtsi > b/target/linux/brcm63xx/dts/bcm6362.dtsi > index 6604f5c..dc3ea9e 100644 > --- a/target/linux/brcm63xx/dts/bcm6362.dtsi > +++ b/target/linux/brcm63xx/dts/bcm6362.dtsi > @@ -6,6 +6,7 @@ > aliases { > gpio0 = &gpio0; > gpio1 = &gpio1; > + leds0 = &leds0; > }; > > cpus { > @@ -81,5 +82,14 @@ > gpio-controller; > #gpio-cells = <2>; > }; > + > + leds0: led-controller@10001904 { > + compatible = "brcm,bcm6362-leds"; > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0x10001900 0x24>; > + > + status = "disabled"; > + }; > }; > }; > diff --git > a/target/linux/brcm63xx/patches-3.18/379-BCM63XX-pinmux-serial-LEDs.patch > b/target/linux/brcm63xx/patches-3.18/379-BCM63XX-pinmux-serial-LEDs.patch > new file mode 100644 > index 0000000..a59e862 > --- /dev/null > +++ b/target/linux/brcm63xx/patches-3.18/379-BCM63XX-pinmux-serial-LEDs.patch > @@ -0,0 +1,13 @@ > +--- a/arch/mips/bcm63xx/boards/board_common.c > ++++ b/arch/mips/bcm63xx/boards/board_common.c > +@@ -106,6 +106,10 @@ void __init board_early_setup(const stru > + GPIO_MODE_6348_G0_EXT_MII; > + } > + > ++ /* Serial LEDs pinmux */ > ++ if (BCMCPU_IS_63268()) > ++ val |= BIT(0) | BIT(1); Maybe also check if leds0 is present / enabled? > ++ > + bcm_gpio_writel(val, GPIO_MODE_REG); > + > + #if IS_ENABLED(CONFIG_USB) > diff --git > a/target/linux/brcm63xx/patches-3.18/380-BCM63XX-add-LEDs-driver.patch > b/target/linux/brcm63xx/patches-3.18/380-BCM63XX-add-LEDs-driver.patch > new file mode 100644 > index 0000000..a57036a > --- /dev/null > +++ b/target/linux/brcm63xx/patches-3.18/380-BCM63XX-add-LEDs-driver.patch > @@ -0,0 +1,398 @@ > +--- a/drivers/leds/Kconfig > ++++ b/drivers/leds/Kconfig > +@@ -32,6 +32,13 @@ config LEDS_88PM860X > + This option enables support for on-chip LED drivers found on Marvell > + Semiconductor 88PM8606 PMIC. > + > ++config LEDS_BCM6328 > ++ tristate "Serial LEDs support for BCM6328 chip" > ++ depends on LEDS_CLASS > ++ 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 > +--- /dev/null > ++++ b/drivers/leds/leds-bcm6328.c > +@@ -0,0 +1,369 @@ > ++/* > ++ * Driver for BCM6328 memory-mapped LEDs, based on leds-syscon.c > ++ * > ++ * Copyright 2015 Álvaro Fernández Rojas <[email protected]> > ++ * > ++ * 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/of_device.h> > ++#include <linux/of_address.h> > ++#include <linux/platform_device.h> > ++#include <linux/regmap.h> > ++#include <linux/spinlock.h> > ++#include <linux/slab.h> > ++#include <linux/stat.h> > ++ > ++#define REG_INIT 0x00 > ++#define REG_MODE_HI 0x04 > ++#define REG_MODE_LO 0x08 > ++#define REG_HWDIS 0x0c > ++#define REG_STROBE 0x10 > ++#define REG_LNKACTSEL_HI 0x14 > ++#define REG_LNKACTSEL_LO 0x18 > ++#define REG_RBACK 0x1c > ++#define REG_SERMUX 0x20 > ++ > ++#define LED_DEF_DELAY 500 > ++ > ++#define LED_INTERVAL_MS 20 > ++#define LED_INTV_MASK 0x3f > ++#define LED_FAST_INTV_SHIFT 6 > ++#define LED_FAST_INTV_MASK (LED_INTV_MASK << LED_FAST_INTV_SHIFT) > ++#define SERIAL_LED_EN BIT(12) > ++#define SERIAL_LED_MUX BIT(13) > ++#define SERIAL_LED_CLK_NPOL BIT(14) > ++#define SERIAL_LED_DATA_PPOL BIT(15) > ++#define SERIAL_LED_SHIFT_DIR BIT(16) > ++#define LED_SHIFT_TEST BIT(30) > ++#define LED_TEST BIT(31) > ++ > ++#define LED_MODE_MASK 3 > ++#define LED_MODE_OFF 0 > ++#define LED_MODE_FAST 1 > ++#define LED_MODE_BLINK 2 > ++#define LED_MODE_ON 3 > ++#define LED_SHIFT(X) ((X) << 1) Please use tabs for aligning the values, so they become more readable. > ++ > ++/** > ++ * 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 > ++ * @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_del; > ++ 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 / 2 bits per led -> 32 LEDs > ++ * bits 0-31: LEDs 8-23 > ++ * bits 32-47: LEDs 0-7 > ++ * bits 48-63: LEDs 24-32 The numbering is wrong, bits 0-31 are leds 0-15, bits 32-37 are leds 16 to 23. There are no leds 24 - 32, the controller only supports 24 leds. What you are thinking about are the corresponding gpios; leds 0-15 can be muxed to gpios 8 to 23, and leds 16 to 23 can be muxed to gpios 0 to 7. > ++ */ > ++static unsigned long bcm6328_pin2shift(unsigned long pin) > ++{ > ++ if(pin < 8) { > ++ /* LEDs 0-7 (bits 32-47) */ > ++ return pin + 16; > ++ } > ++ else if(pin < 23) { > ++ /* LEDs 8-23 (bits 0-31) */ > ++ return pin - 8; > ++ } > ++ else { > ++ /* LEDs 24-31 (bits 48-63) */ > ++ return pin; > ++ } > ++} Therefore this function is completely bogus. > ++ > ++static void bcm6328_led_mode(struct bcm6328_led *led, > ++ unsigned long value) Please align the > ++{ > ++ void __iomem *mode; > ++ unsigned long val, shift; > ++ > ++ shift = bcm6328_pin2shift(led->pin); with that the shift becomes just pin % 16. > ++ if(shift / 16) { > ++ mode = led->mem + REG_MODE_HI; > ++ } > ++ else { > ++ mode = led->mem + REG_MODE_LO; > ++ } Unnecessary braces. > ++ > ++ val = bcm6328_led_read(mode); > ++ val &= ~(LED_MODE_MASK << LED_SHIFT(shift % 16)); > ++ val |= (value << 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, LED_MODE_OFF); > ++ } > ++ else { > ++ bcm6328_led_mode(led, 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_ms, delay, flags; > ++ > ++ if(!*delay_on && !*delay_off) { Missing space before opening paranthesis. > ++ delay_ms = LED_DEF_DELAY; > ++ } > ++ else if(*delay_on != *delay_off) { No line break before else. > ++ if(!*delay_on) { > ++ *delay_on = LED_DEF_DELAY; > ++ } Unnecessary braces. > ++ if(!*delay_off) { > ++ *delay_off = LED_DEF_DELAY; > ++ } Unnecessary braces. > ++ dev_dbg(led_cdev->dev, "fallback to software blinking " > ++ "(delay_on != delay_off)\n"); Please don't split strings, it makes grepping for output annoying. > ++ return -EINVAL; > ++ } > ++ else { > ++ delay_ms = *delay_on; > ++ } > ++ > ++ delay = delay_ms / LED_INTERVAL_MS; > ++ if (delay == 0) { > ++ delay = 1; > ++ } > ++ else if(delay > LED_INTV_MASK) { No line break before else. > ++ dev_dbg(led_cdev->dev, "fallback to software blinking " > ++ "(delay > %ums)\n", LED_INTV_MASK * LED_INTERVAL_MS); Please don't split strings, it makes grepping for output annoying. > ++ return -EINVAL; > ++ } > ++ > ++ spin_lock_irqsave(led->lock, flags); > ++ if(*(led->blink_leds) == 0 || Missing space before opening paranthesis. > ++ *(led->blink_leds) == BIT(led->pin) || > ++ *(led->blink_del) == delay) { Please align them to the opening paranthesis. > ++ unsigned long val; > ++ > ++ *(led->blink_leds) = BIT(led->pin); > ++ *(led->blink_del) = delay; > ++ > ++ val = bcm6328_led_read(led->mem + REG_INIT); > ++ val &= ~LED_FAST_INTV_MASK; > ++ val |= (delay << LED_FAST_INTV_SHIFT); > ++ bcm6328_led_write(led->mem + REG_INIT, val); > ++ > ++ bcm6328_led_mode(led, LED_MODE_BLINK); > ++ > ++ spin_unlock_irqrestore(led->lock, flags); > ++ > ++ return 0; > ++ } > ++ else { > ++ spin_unlock_irqrestore(led->lock, flags); > ++ dev_dbg(led_cdev->dev, "fallback to software blinking " > ++ "(delay already configured)\n"); > ++ return -EINVAL; > ++ } > ++} > ++ > ++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 flags, val, *blink_leds, *blink_del; > ++ > ++ 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; Wrong indentation. > ++ > ++ blink_leds = devm_kzalloc(dev, sizeof(*blink_leds), GFP_KERNEL); > ++ if (!blink_leds) > ++ return -ENOMEM; Wrong indentation. > ++ > ++ blink_del = devm_kzalloc(dev, sizeof(*blink_del), GFP_KERNEL); > ++ if (!blink_del) > ++ return -ENOMEM; Wrong indentation. > ++ > ++ spin_lock_init(lock); > ++ > ++ bcm6328_led_write(mem + REG_HWDIS, ~0); > ++ bcm6328_led_write(mem + REG_LNKACTSEL_HI, 0); > ++ bcm6328_led_write(mem + REG_LNKACTSEL_LO, 0); > ++ > ++ val = bcm6328_led_read(mem + REG_INIT); > ++ val &= ~SERIAL_LED_EN; > ++ if (of_get_property(np, "brcm,serial-leds", NULL)) > ++ val |= SERIAL_LED_EN; > ++ bcm6328_led_write(mem + REG_INIT, val); > ++ > ++ for_each_available_child_of_node(np, child) { > ++ if (of_device_is_compatible(child, "bcm6328-led")) { You are missing the vendor-prefix for your compatible name. > ++ struct bcm6328_led *led; > ++ const char *state; > ++ u32 reg; > ++ > ++ led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL); > ++ if (!led) > ++ return -ENOMEM; > ++ > ++ led->mem = mem; > ++ led->lock = lock; > ++ led->blink_leds = blink_leds; > ++ led->blink_del = blink_del; > ++ > ++ if (of_property_read_u32(child, "reg", ®)) > ++ continue; > ++ led->pin = reg; > ++ > ++ if (of_get_property(child, "active-low", NULL)) > ++ led->active_low = 1; > ++ > ++ led->cdev.name = > ++ of_get_property(child, > ++ "label", NULL) ? : child->name; > ++ led->cdev.default_trigger = > ++ of_get_property(child, > ++ "linux,default-trigger", NULL); > ++ > ++ state = of_get_property(child, "default-state", NULL); > ++ if (state) { please use if (!of_property_read_string(child, "default-state", &state) this is also do a sort-of sanity check that this is a string property. > ++ spin_lock_irqsave(lock, flags); > ++ if (!strcmp(state, "on")) { > ++ led->cdev.brightness = LED_FULL; > ++ bcm6328_led_mode(led, LED_MODE_ON); > ++ } > ++ else if(!strcmp(state, "keep")) { Superfluous line break before else and missing space before opening paranthesis. > ++ void __iomem *mode; > ++ unsigned long val, shift; > ++ > ++ shift = bcm6328_pin2shift(led->pin); > ++ if(shift / 16) { > ++ mode = mem + REG_MODE_HI; > ++ } Superfluous braces. > ++ else { > ++ mode = mem + REG_MODE_LO; > ++ } Superfluous braces. > ++ > ++ val = bcm6328_led_read(mode) >> > ++ (shift % 16); > ++ val &= LED_MODE_MASK; > ++ if(val == LED_MODE_ON) { > ++ led->cdev.brightness = > LED_FULL; > ++ } > ++ else { Please no line break before else. > ++ led->cdev.brightness = > LED_OFF; > ++ bcm6328_led_mode(led, > ++ LED_MODE_OFF); > ++ } > ++ } > ++ else { Please no line break before else. > ++ led->cdev.brightness = LED_OFF; > ++ bcm6328_led_mode(led, LED_MODE_OFF); > ++ } > ++ spin_unlock_irqrestore(lock, flags); > ++ } > ++ > ++ led->cdev.brightness_set = bcm6328_led_set; > ++ led->cdev.blink_set = bcm6328_blink_set; > ++ > ++ if (led_classdev_register(dev, &led->cdev) < 0) > ++ continue; > ++ > ++ dev_info(dev, "registered LED %s\n", led->cdev.name); > ++ } > ++ else if(of_device_is_compatible(child, "bcm6328-hw-led")) { Being controlled by hardware is a software property, not a hardware property, so this should not be a different compatible name, just a simple property. > ++ u32 reg, act_shift; > ++ > ++ if (of_property_read_u32(child, "reg", ®)) > ++ continue; > ++ > ++ val = bcm6328_led_read(mem + REG_HWDIS); > ++ val &= ~BIT(reg); > ++ bcm6328_led_write(mem + REG_HWDIS, val); > ++ > ++ if (!of_property_read_u32(child, > ++ "act-high", &act_shift)) { > ++ val = bcm6328_led_read(mem + > REG_LNKACTSEL_HI); > ++ val |= (BIT(reg) << act_shift); > ++ bcm6328_led_write(mem + REG_LNKACTSEL_HI, > val); > ++ } > ++ > ++ if (!of_property_read_u32(child, > ++ "act-low", &act_shift)) { > ++ val = bcm6328_led_read(mem + > REG_LNKACTSEL_LO); > ++ val |= (BIT(reg) << act_shift); > ++ bcm6328_led_write(mem + REG_LNKACTSEL_LO, > val); > ++ } These two should be limited to leds 16-19, and a more sensible binding would be brcm,led-activity-signals = <0 3>; brcm,led-link-signals = <2 4 5>; which would then set the appropriate bits at the appropriate places. > ++ } > ++ else { > ++ continue; > ++ } Completely superfluous branch, as you are at the end of a loop ;) > ++ } > ++ > ++ return 0; > ++} > ++ > ++static const struct of_device_id bcm6328_leds_of_match[] = { > ++ { .compatible = "brcm,bcm6318-leds", }, > ++ { .compatible = "brcm,bcm6328-leds", }, > ++ { .compatible = "brcm,bcm6362-leds", }, > ++ { .compatible = "brcm,bcm63268-leds", }, > ++ {}, > ++}; > ++ > ++static struct platform_driver bcm6328_leds_driver = { > ++ .probe = bcm6328_leds_probe, > ++ .driver = { > ++ .name = "bcm6328-leds", > ++ .of_match_table = of_match_ptr(bcm6328_leds_of_match), No need for of_match_ptr() if you require OF. > ++ }, > ++}; > ++ > ++module_platform_driver(bcm6328_leds_driver); > +--- a/drivers/leds/Makefile > ++++ b/drivers/leds/Makefile > +@@ -6,6 +6,7 @@ obj-$(CONFIG_LEDS_TRIGGERS) += led-trig > + > + # 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 Jonas _______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
