[PATCH V2 1/2] dt-bindings: leds: add Broadcom's BCM63138 controller

[PATCH V2 1/2] dt-bindings: leds: add Broadcom's BCM63138 controller

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Broadcom used 2 LEDs hardware blocks for their BCM63xx SoCs:
1. Older one (BCM6318, BCM6328, BCM6362, BCM63268, BCM6838)
2. Newer one (BCM6848, BCM6858, BCM63138, BCM63148, BCM63381, BCM68360)

The newer one was also later also used on BCM4908 SoC.

Old block is already documented in the leds-bcm6328.yaml. This binding
documents the new one which uses different registers & programming. It's
first used in BCM63138 thus the binding name.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Rename to bcm63138 & make "brcm,bcm63138-leds" the main compatible
---
 .../bindings/leds/leds-bcm63138.yaml          | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-bcm63138.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml b/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml
new file mode 100644
index 000000000000..99cd4ba9b0ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-bcm63138.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom's BCM63138 LEDs controller
+
+maintainers:
+  - Rafał Miłecki <rafal@milecki.pl>
+
+description: |
+  This LEDs controller was first used on BCM63138 and later reused on BCM4908,
+  BCM6848, BCM6858, BCM63138, BCM63148, BCM63381 and BCM68360 SoCs.
+
+  It supports up to 32 LEDs that can be connected parallelly or serially. It
+  also includes limited support for hardware blinking.
+
+  Binding serially connected LEDs isn't documented yet.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - brcm,bcm4908-leds
+              - brcm,bcm6848-leds
+              - brcm,bcm6858-leds
+              - brcm,bcm63148-leds
+              - brcm,bcm63381-leds
+              - brcm,bcm68360-leds
+          - const: brcm,bcm63138-leds
+      - const: brcm,bcm63138-leds
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[a-f0-9]+$":
+    type: object
+
+    $ref: common.yaml#
+
+    properties:
+      reg:
+        maxItems: 1
+        description: LED pin number
+
+      active-low:
+        type: boolean
+        description: Makes LED active low.
+
+    required:
+      - reg
+
+    unevaluatedProperties: false
+
+required:
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    leds@ff800800 {
+        compatible = "brcm,bcm4908-leds", "brcm,bcm63138-leds";
+        reg = <0xff800800 0xdc>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led@0 {
+            reg = <0x0>;
+            function = LED_FUNCTION_POWER;
+            color = <LED_COLOR_ID_GREEN>;
+            default-state = "on";
+        };
+
+        led@3 {
+            reg = <0x3>;
+            function = LED_FUNCTION_STATUS;
+            color = <LED_COLOR_ID_GREEN>;
+            active-low;
+        };
+    };
-- 
2.31.1

[PATCH V2 1/2] dt-bindings: leds: add Broadcom's BCM63138 controller

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Broadcom used 2 LEDs hardware blocks for their BCM63xx SoCs:
1. Older one (BCM6318, BCM6328, BCM6362, BCM63268, BCM6838)
2. Newer one (BCM6848, BCM6858, BCM63138, BCM63148, BCM63381, BCM68360)

The newer one was also later also used on BCM4908 SoC.

Old block is already documented in the leds-bcm6328.yaml. This binding
documents the new one which uses different registers & programming. It's
first used in BCM63138 thus the binding name.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Rename to bcm63138 & make "brcm,bcm63138-leds" the main compatible
---
 .../bindings/leds/leds-bcm63138.yaml          | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-bcm63138.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml b/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml
new file mode 100644
index 000000000000..99cd4ba9b0ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-bcm63138.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom's BCM63138 LEDs controller
+
+maintainers:
+  - Rafał Miłecki <rafal@milecki.pl>
+
+description: |
+  This LEDs controller was first used on BCM63138 and later reused on BCM4908,
+  BCM6848, BCM6858, BCM63138, BCM63148, BCM63381 and BCM68360 SoCs.
+
+  It supports up to 32 LEDs that can be connected parallelly or serially. It
+  also includes limited support for hardware blinking.
+
+  Binding serially connected LEDs isn't documented yet.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - brcm,bcm4908-leds
+              - brcm,bcm6848-leds
+              - brcm,bcm6858-leds
+              - brcm,bcm63148-leds
+              - brcm,bcm63381-leds
+              - brcm,bcm68360-leds
+          - const: brcm,bcm63138-leds
+      - const: brcm,bcm63138-leds
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[a-f0-9]+$":
+    type: object
+
+    $ref: common.yaml#
+
+    properties:
+      reg:
+        maxItems: 1
+        description: LED pin number
+
+      active-low:
+        type: boolean
+        description: Makes LED active low.
+
+    required:
+      - reg
+
+    unevaluatedProperties: false
+
+required:
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    leds@ff800800 {
+        compatible = "brcm,bcm4908-leds", "brcm,bcm63138-leds";
+        reg = <0xff800800 0xdc>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led@0 {
+            reg = <0x0>;
+            function = LED_FUNCTION_POWER;
+            color = <LED_COLOR_ID_GREEN>;
+            default-state = "on";
+        };
+
+        led@3 {
+            reg = <0x3>;
+            function = LED_FUNCTION_STATUS;
+            color = <LED_COLOR_ID_GREEN>;
+            active-low;
+        };
+    };
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V2 2/2] leds: bcm63xxx: add support for BCM63138 controller

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

It's a new controller first introduced in BCM63138 SoC. Later it was
also used in BCM4908, some BCM68xx and some BCM63xxx SoCs.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Rename to bcm63138
    Improve Kconfig help
    Add defines for magic values
    Support setting brightness
    Make bcm63xxx_leds_create_led() void function
---
 drivers/leds/Kconfig         |  12 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-bcm63138.c | 314 +++++++++++++++++++++++++++++++++++
 3 files changed, 327 insertions(+)
 create mode 100644 drivers/leds/leds-bcm63138.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ed800f5da7d8..3bde795f0951 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -122,6 +122,18 @@ config LEDS_BCM6358
 	  This option enables support for LEDs connected to the BCM6358
 	  LED HW controller accessed via MMIO registers.
 
+config LEDS_BCM63138
+	tristate "LED Support for Broadcom BCM63138 SoC"
+	depends on LEDS_CLASS
+	depends on ARCH_BCM4908 || BCM63XX || COMPILE_TEST
+	depends on HAS_IOMEM
+	depends on OF
+	default ARCH_BCM4908
+	help
+	  This option enables support for LED controller that is part of
+	  BCM63138 SoC. The same hardware block is known to be also used
+	  in BCM4908, BCM6848, BCM6858, BCM63148, BCM63381 and BCM68360.
+
 config LEDS_CPCAP
 	tristate "LED Support for Motorola CPCAP"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c636ec069612..c986630ce782 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_LEDS_ASIC3)		+= leds-asic3.o
 obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
+obj-$(CONFIG_LEDS_BCM63138)		+= leds-bcm63138.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
 obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
diff --git a/drivers/leds/leds-bcm63138.c b/drivers/leds/leds-bcm63138.c
new file mode 100644
index 000000000000..160c9282deda
--- /dev/null
+++ b/drivers/leds/leds-bcm63138.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
+ */
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define BCM63138_MAX_LEDS				32
+
+#define BCM63138_LED_BITS				4				/* how many bits control a single LED */
+#define BCM63138_LED_MASK				((1 << BCM63138_LED_BITS) - 1)	/* 0xf */
+#define BCM63138_LEDS_PER_REG				(32 / BCM63138_LED_BITS)	/* 8 */
+
+#define BCM63138_GLB_CTRL				0x00
+#define  BCM63138_GLB_CTRL_SERIAL_LED_DATA_PPOL		0x00000002
+#define  BCM63138_GLB_CTRL_SERIAL_LED_EN_POL		0x00000008
+#define BCM63138_MASK					0x04
+#define BCM63138_HW_LED_EN				0x08
+#define BCM63138_SERIAL_LED_SHIFT_SEL			0x0c
+#define BCM63138_FLASH_RATE_CTRL1			0x10
+#define BCM63138_FLASH_RATE_CTRL2			0x14
+#define BCM63138_FLASH_RATE_CTRL3			0x18
+#define BCM63138_FLASH_RATE_CTRL4			0x1c
+#define BCM63138_BRIGHT_CTRL1				0x20
+#define BCM63138_BRIGHT_CTRL2				0x24
+#define BCM63138_BRIGHT_CTRL3				0x28
+#define BCM63138_BRIGHT_CTRL4				0x2c
+#define BCM63138_POWER_LED_CFG				0x30
+#define BCM63138_HW_POLARITY				0xb4
+#define BCM63138_SW_DATA				0xb8
+#define BCM63138_SW_POLARITY				0xbc
+#define BCM63138_PARALLEL_LED_POLARITY			0xc0
+#define BCM63138_SERIAL_LED_POLARITY			0xc4
+#define BCM63138_HW_LED_STATUS				0xc8
+#define BCM63138_FLASH_CTRL_STATUS			0xcc
+#define BCM63138_FLASH_BRT_CTRL				0xd0
+#define BCM63138_FLASH_P_LED_OUT_STATUS			0xd4
+#define BCM63138_FLASH_S_LED_OUT_STATUS			0xd8
+
+struct bcm63138_leds {
+	struct device *dev;
+	void __iomem *base;
+	spinlock_t lock;
+};
+
+struct bcm63138_led {
+	struct bcm63138_leds *leds;
+	struct led_classdev cdev;
+	u32 pin;
+	bool active_low;
+};
+
+/*
+ * I/O access
+ */
+
+static void bcm63138_leds_write(struct bcm63138_leds *leds, unsigned int reg,
+				u32 data)
+{
+	writel(data, leds->base + reg);
+}
+
+static unsigned long bcm63138_leds_read(struct bcm63138_leds *leds,
+					unsigned int reg)
+{
+	return readl(leds->base + reg);
+}
+
+static void bcm63138_leds_update_bits(struct bcm63138_leds *leds,
+				      unsigned int reg, u32 mask, u32 val)
+{
+	WARN_ON(val & ~mask);
+
+	bcm63138_leds_write(leds, reg, (bcm63138_leds_read(leds, reg) & ~mask) | (val & mask));
+}
+
+/*
+ * Helpers
+ */
+
+static void bcm63138_leds_set_flash_rate(struct bcm63138_leds *leds,
+					 struct bcm63138_led *led,
+					 u8 value)
+{
+	int reg_offset = (led->pin >> fls((BCM63138_LEDS_PER_REG - 1))) * 4;
+	int shift = (led->pin & (BCM63138_LEDS_PER_REG - 1)) * BCM63138_LED_BITS;
+
+	bcm63138_leds_update_bits(leds, BCM63138_FLASH_RATE_CTRL1 + reg_offset,
+				  BCM63138_LED_MASK << shift, value << shift);
+}
+
+static void bcm63138_leds_set_bright(struct bcm63138_leds *leds,
+				     struct bcm63138_led *led,
+				     u8 value)
+{
+	int reg_offset = (led->pin >> fls((BCM63138_LEDS_PER_REG - 1))) * 4;
+	int shift = (led->pin & (BCM63138_LEDS_PER_REG - 1)) * BCM63138_LED_BITS;
+
+	bcm63138_leds_update_bits(leds, BCM63138_BRIGHT_CTRL1 + reg_offset,
+				  BCM63138_LED_MASK << shift, value << shift);
+}
+
+static void bcm63138_leds_enable_led(struct bcm63138_leds *leds,
+				     struct bcm63138_led *led,
+				     enum led_brightness value)
+{
+	u32 bit = BIT(led->pin);
+
+	bcm63138_leds_update_bits(leds, BCM63138_SW_DATA, bit,
+				  value == LED_OFF ? 0 : bit);
+}
+
+/*
+ * API callbacks
+ */
+
+static void bcm63138_leds_brightness_set(struct led_classdev *led_cdev,
+					 enum led_brightness value)
+{
+	struct bcm63138_led *led = container_of(led_cdev, struct bcm63138_led, cdev);
+	struct bcm63138_leds *leds = led->leds;
+	unsigned long flags;
+
+	spin_lock_irqsave(&leds->lock, flags);
+
+	bcm63138_leds_enable_led(leds, led, value);
+	if (value == LED_OFF)
+		bcm63138_leds_set_flash_rate(leds, led, 0);
+	else
+		bcm63138_leds_set_bright(leds, led, (value >> 5) + 1);
+
+	spin_unlock_irqrestore(&leds->lock, flags);
+}
+
+static int bcm63138_leds_blink_set(struct led_classdev *led_cdev,
+				   unsigned long *delay_on,
+				   unsigned long *delay_off)
+{
+	struct bcm63138_led *led = container_of(led_cdev, struct bcm63138_led, cdev);
+	struct bcm63138_leds *leds = led->leds;
+	unsigned long flags;
+	u8 value;
+
+	if (!*delay_on && !*delay_off) {
+		*delay_on = 640;
+		*delay_off = 640;
+	}
+
+	if (*delay_on != *delay_off) {
+		dev_dbg(led_cdev->dev, "Blinking at unequal delays is not supported\n");
+		return -EINVAL;
+	}
+
+	switch (*delay_on) {
+	case 1152 ... 1408: /* 1280 ms ± 10% */
+		value = 0x7;
+		break;
+	case 576 ... 704: /* 640 ms ± 10% */
+		value = 0x6;
+		break;
+	case 288 ... 352: /* 320 ms ± 10% */
+		value = 0x5;
+		break;
+	case 126 ... 154: /* 140 ms ± 10% */
+		value = 0x4;
+		break;
+	case 59 ... 72: /* 65 ms ± 10% */
+		value = 0x3;
+		break;
+	default:
+		dev_dbg(led_cdev->dev, "Blinking delay value %lu is unsupported\n",
+			*delay_on);
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&leds->lock, flags);
+
+	bcm63138_leds_enable_led(leds, led, LED_FULL);
+	bcm63138_leds_set_flash_rate(leds, led, value);
+
+	spin_unlock_irqrestore(&leds->lock, flags);
+
+	return 0;
+}
+
+/*
+ * LED driver
+ */
+
+static void bcm63138_leds_create_led(struct bcm63138_leds *leds,
+				     struct device_node *np)
+{
+	struct led_init_data init_data = {
+		.fwnode = of_fwnode_handle(np),
+	};
+	struct device *dev = leds->dev;
+	struct bcm63138_led *led;
+	struct pinctrl *pinctrl;
+	const char *state;
+	u32 bit;
+	int err;
+
+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return;
+
+	led->leds = leds;
+
+	if (of_property_read_u32(np, "reg", &led->pin)) {
+		dev_err(dev, "Missing \"reg\" property in %pOF\n", np);
+		goto err_free;
+	}
+
+	if (led->pin >= BCM63138_MAX_LEDS) {
+		dev_err(dev, "Invalid \"reg\" value %d\n", led->pin);
+		goto err_free;
+	}
+
+	led->active_low = of_property_read_bool(np, "active-low");
+
+	if (!of_property_read_string(np, "default-state", &state)) {
+		if (!strcmp(state, "on"))
+			led->cdev.brightness = LED_FULL;
+		else
+			led->cdev.brightness = LED_OFF;
+	} else {
+		led->cdev.brightness = LED_OFF;
+	}
+
+	led->cdev.brightness_set = bcm63138_leds_brightness_set;
+	led->cdev.blink_set = bcm63138_leds_blink_set;
+
+	err = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+	if (err) {
+		dev_err(dev, "Failed to register LED %pOF: %d\n", np, err);
+		goto err_free;
+	}
+
+	pinctrl = devm_pinctrl_get_select_default(led->cdev.dev);
+	if (IS_ERR(pinctrl) && PTR_ERR(pinctrl) != -ENODEV) {
+		dev_warn(led->cdev.dev, "Failed to select %pOF pinctrl: %ld\n",
+			 np, PTR_ERR(pinctrl));
+	}
+
+	bit = BIT(led->pin);
+	bcm63138_leds_update_bits(leds, BCM63138_PARALLEL_LED_POLARITY, bit,
+				  led->active_low ? 0 : bit);
+	bcm63138_leds_update_bits(leds, BCM63138_HW_LED_EN, bit, 0);
+	bcm63138_leds_set_flash_rate(leds, led, 0);
+	bcm63138_leds_enable_led(leds, led, led->cdev.brightness);
+
+	return;
+
+err_free:
+	devm_kfree(dev, led);
+}
+
+static int bcm63138_leds_probe(struct platform_device *pdev)
+{
+	struct device_node *np = dev_of_node(&pdev->dev);
+	struct device *dev = &pdev->dev;
+	struct bcm63138_leds *leds;
+	struct device_node *child;
+
+	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	leds->dev = dev;
+
+	leds->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(leds->base))
+		return PTR_ERR(leds->base);
+
+	spin_lock_init(&leds->lock);
+
+	bcm63138_leds_write(leds, BCM63138_GLB_CTRL,
+			    BCM63138_GLB_CTRL_SERIAL_LED_DATA_PPOL |
+			    BCM63138_GLB_CTRL_SERIAL_LED_EN_POL);
+	bcm63138_leds_write(leds, BCM63138_HW_LED_EN, 0);
+	bcm63138_leds_write(leds, BCM63138_SERIAL_LED_POLARITY, 0);
+	bcm63138_leds_write(leds, BCM63138_PARALLEL_LED_POLARITY, 0);
+
+	for_each_available_child_of_node(np, child) {
+		bcm63138_leds_create_led(leds, child);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id bcm63138_leds_of_match_table[] = {
+	{ .compatible = "brcm,bcm63138-leds", },
+	{ },
+};
+
+static struct platform_driver bcm63138_leds_driver = {
+	.probe = bcm63138_leds_probe,
+	.driver = {
+		.name = "leds-bcm63xxx",
+		.of_match_table = bcm63138_leds_of_match_table,
+	},
+};
+
+module_platform_driver(bcm63138_leds_driver);
+
+MODULE_AUTHOR("Rafał Miłecki");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, bcm63138_leds_of_match_table);
-- 
2.31.1

[PATCH V2 2/2] leds: bcm63xxx: add support for BCM63138 controller

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

It's a new controller first introduced in BCM63138 SoC. Later it was
also used in BCM4908, some BCM68xx and some BCM63xxx SoCs.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Rename to bcm63138
    Improve Kconfig help
    Add defines for magic values
    Support setting brightness
    Make bcm63xxx_leds_create_led() void function
---
 drivers/leds/Kconfig         |  12 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-bcm63138.c | 314 +++++++++++++++++++++++++++++++++++
 3 files changed, 327 insertions(+)
 create mode 100644 drivers/leds/leds-bcm63138.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ed800f5da7d8..3bde795f0951 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -122,6 +122,18 @@ config LEDS_BCM6358
 	  This option enables support for LEDs connected to the BCM6358
 	  LED HW controller accessed via MMIO registers.
 
+config LEDS_BCM63138
+	tristate "LED Support for Broadcom BCM63138 SoC"
+	depends on LEDS_CLASS
+	depends on ARCH_BCM4908 || BCM63XX || COMPILE_TEST
+	depends on HAS_IOMEM
+	depends on OF
+	default ARCH_BCM4908
+	help
+	  This option enables support for LED controller that is part of
+	  BCM63138 SoC. The same hardware block is known to be also used
+	  in BCM4908, BCM6848, BCM6858, BCM63148, BCM63381 and BCM68360.
+
 config LEDS_CPCAP
 	tristate "LED Support for Motorola CPCAP"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c636ec069612..c986630ce782 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_LEDS_ASIC3)		+= leds-asic3.o
 obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.o
 obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
 obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
+obj-$(CONFIG_LEDS_BCM63138)		+= leds-bcm63138.o
 obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
 obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
diff --git a/drivers/leds/leds-bcm63138.c b/drivers/leds/leds-bcm63138.c
new file mode 100644
index 000000000000..160c9282deda
--- /dev/null
+++ b/drivers/leds/leds-bcm63138.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
+ */
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define BCM63138_MAX_LEDS				32
+
+#define BCM63138_LED_BITS				4				/* how many bits control a single LED */
+#define BCM63138_LED_MASK				((1 << BCM63138_LED_BITS) - 1)	/* 0xf */
+#define BCM63138_LEDS_PER_REG				(32 / BCM63138_LED_BITS)	/* 8 */
+
+#define BCM63138_GLB_CTRL				0x00
+#define  BCM63138_GLB_CTRL_SERIAL_LED_DATA_PPOL		0x00000002
+#define  BCM63138_GLB_CTRL_SERIAL_LED_EN_POL		0x00000008
+#define BCM63138_MASK					0x04
+#define BCM63138_HW_LED_EN				0x08
+#define BCM63138_SERIAL_LED_SHIFT_SEL			0x0c
+#define BCM63138_FLASH_RATE_CTRL1			0x10
+#define BCM63138_FLASH_RATE_CTRL2			0x14
+#define BCM63138_FLASH_RATE_CTRL3			0x18
+#define BCM63138_FLASH_RATE_CTRL4			0x1c
+#define BCM63138_BRIGHT_CTRL1				0x20
+#define BCM63138_BRIGHT_CTRL2				0x24
+#define BCM63138_BRIGHT_CTRL3				0x28
+#define BCM63138_BRIGHT_CTRL4				0x2c
+#define BCM63138_POWER_LED_CFG				0x30
+#define BCM63138_HW_POLARITY				0xb4
+#define BCM63138_SW_DATA				0xb8
+#define BCM63138_SW_POLARITY				0xbc
+#define BCM63138_PARALLEL_LED_POLARITY			0xc0
+#define BCM63138_SERIAL_LED_POLARITY			0xc4
+#define BCM63138_HW_LED_STATUS				0xc8
+#define BCM63138_FLASH_CTRL_STATUS			0xcc
+#define BCM63138_FLASH_BRT_CTRL				0xd0
+#define BCM63138_FLASH_P_LED_OUT_STATUS			0xd4
+#define BCM63138_FLASH_S_LED_OUT_STATUS			0xd8
+
+struct bcm63138_leds {
+	struct device *dev;
+	void __iomem *base;
+	spinlock_t lock;
+};
+
+struct bcm63138_led {
+	struct bcm63138_leds *leds;
+	struct led_classdev cdev;
+	u32 pin;
+	bool active_low;
+};
+
+/*
+ * I/O access
+ */
+
+static void bcm63138_leds_write(struct bcm63138_leds *leds, unsigned int reg,
+				u32 data)
+{
+	writel(data, leds->base + reg);
+}
+
+static unsigned long bcm63138_leds_read(struct bcm63138_leds *leds,
+					unsigned int reg)
+{
+	return readl(leds->base + reg);
+}
+
+static void bcm63138_leds_update_bits(struct bcm63138_leds *leds,
+				      unsigned int reg, u32 mask, u32 val)
+{
+	WARN_ON(val & ~mask);
+
+	bcm63138_leds_write(leds, reg, (bcm63138_leds_read(leds, reg) & ~mask) | (val & mask));
+}
+
+/*
+ * Helpers
+ */
+
+static void bcm63138_leds_set_flash_rate(struct bcm63138_leds *leds,
+					 struct bcm63138_led *led,
+					 u8 value)
+{
+	int reg_offset = (led->pin >> fls((BCM63138_LEDS_PER_REG - 1))) * 4;
+	int shift = (led->pin & (BCM63138_LEDS_PER_REG - 1)) * BCM63138_LED_BITS;
+
+	bcm63138_leds_update_bits(leds, BCM63138_FLASH_RATE_CTRL1 + reg_offset,
+				  BCM63138_LED_MASK << shift, value << shift);
+}
+
+static void bcm63138_leds_set_bright(struct bcm63138_leds *leds,
+				     struct bcm63138_led *led,
+				     u8 value)
+{
+	int reg_offset = (led->pin >> fls((BCM63138_LEDS_PER_REG - 1))) * 4;
+	int shift = (led->pin & (BCM63138_LEDS_PER_REG - 1)) * BCM63138_LED_BITS;
+
+	bcm63138_leds_update_bits(leds, BCM63138_BRIGHT_CTRL1 + reg_offset,
+				  BCM63138_LED_MASK << shift, value << shift);
+}
+
+static void bcm63138_leds_enable_led(struct bcm63138_leds *leds,
+				     struct bcm63138_led *led,
+				     enum led_brightness value)
+{
+	u32 bit = BIT(led->pin);
+
+	bcm63138_leds_update_bits(leds, BCM63138_SW_DATA, bit,
+				  value == LED_OFF ? 0 : bit);
+}
+
+/*
+ * API callbacks
+ */
+
+static void bcm63138_leds_brightness_set(struct led_classdev *led_cdev,
+					 enum led_brightness value)
+{
+	struct bcm63138_led *led = container_of(led_cdev, struct bcm63138_led, cdev);
+	struct bcm63138_leds *leds = led->leds;
+	unsigned long flags;
+
+	spin_lock_irqsave(&leds->lock, flags);
+
+	bcm63138_leds_enable_led(leds, led, value);
+	if (value == LED_OFF)
+		bcm63138_leds_set_flash_rate(leds, led, 0);
+	else
+		bcm63138_leds_set_bright(leds, led, (value >> 5) + 1);
+
+	spin_unlock_irqrestore(&leds->lock, flags);
+}
+
+static int bcm63138_leds_blink_set(struct led_classdev *led_cdev,
+				   unsigned long *delay_on,
+				   unsigned long *delay_off)
+{
+	struct bcm63138_led *led = container_of(led_cdev, struct bcm63138_led, cdev);
+	struct bcm63138_leds *leds = led->leds;
+	unsigned long flags;
+	u8 value;
+
+	if (!*delay_on && !*delay_off) {
+		*delay_on = 640;
+		*delay_off = 640;
+	}
+
+	if (*delay_on != *delay_off) {
+		dev_dbg(led_cdev->dev, "Blinking at unequal delays is not supported\n");
+		return -EINVAL;
+	}
+
+	switch (*delay_on) {
+	case 1152 ... 1408: /* 1280 ms ± 10% */
+		value = 0x7;
+		break;
+	case 576 ... 704: /* 640 ms ± 10% */
+		value = 0x6;
+		break;
+	case 288 ... 352: /* 320 ms ± 10% */
+		value = 0x5;
+		break;
+	case 126 ... 154: /* 140 ms ± 10% */
+		value = 0x4;
+		break;
+	case 59 ... 72: /* 65 ms ± 10% */
+		value = 0x3;
+		break;
+	default:
+		dev_dbg(led_cdev->dev, "Blinking delay value %lu is unsupported\n",
+			*delay_on);
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&leds->lock, flags);
+
+	bcm63138_leds_enable_led(leds, led, LED_FULL);
+	bcm63138_leds_set_flash_rate(leds, led, value);
+
+	spin_unlock_irqrestore(&leds->lock, flags);
+
+	return 0;
+}
+
+/*
+ * LED driver
+ */
+
+static void bcm63138_leds_create_led(struct bcm63138_leds *leds,
+				     struct device_node *np)
+{
+	struct led_init_data init_data = {
+		.fwnode = of_fwnode_handle(np),
+	};
+	struct device *dev = leds->dev;
+	struct bcm63138_led *led;
+	struct pinctrl *pinctrl;
+	const char *state;
+	u32 bit;
+	int err;
+
+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return;
+
+	led->leds = leds;
+
+	if (of_property_read_u32(np, "reg", &led->pin)) {
+		dev_err(dev, "Missing \"reg\" property in %pOF\n", np);
+		goto err_free;
+	}
+
+	if (led->pin >= BCM63138_MAX_LEDS) {
+		dev_err(dev, "Invalid \"reg\" value %d\n", led->pin);
+		goto err_free;
+	}
+
+	led->active_low = of_property_read_bool(np, "active-low");
+
+	if (!of_property_read_string(np, "default-state", &state)) {
+		if (!strcmp(state, "on"))
+			led->cdev.brightness = LED_FULL;
+		else
+			led->cdev.brightness = LED_OFF;
+	} else {
+		led->cdev.brightness = LED_OFF;
+	}
+
+	led->cdev.brightness_set = bcm63138_leds_brightness_set;
+	led->cdev.blink_set = bcm63138_leds_blink_set;
+
+	err = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+	if (err) {
+		dev_err(dev, "Failed to register LED %pOF: %d\n", np, err);
+		goto err_free;
+	}
+
+	pinctrl = devm_pinctrl_get_select_default(led->cdev.dev);
+	if (IS_ERR(pinctrl) && PTR_ERR(pinctrl) != -ENODEV) {
+		dev_warn(led->cdev.dev, "Failed to select %pOF pinctrl: %ld\n",
+			 np, PTR_ERR(pinctrl));
+	}
+
+	bit = BIT(led->pin);
+	bcm63138_leds_update_bits(leds, BCM63138_PARALLEL_LED_POLARITY, bit,
+				  led->active_low ? 0 : bit);
+	bcm63138_leds_update_bits(leds, BCM63138_HW_LED_EN, bit, 0);
+	bcm63138_leds_set_flash_rate(leds, led, 0);
+	bcm63138_leds_enable_led(leds, led, led->cdev.brightness);
+
+	return;
+
+err_free:
+	devm_kfree(dev, led);
+}
+
+static int bcm63138_leds_probe(struct platform_device *pdev)
+{
+	struct device_node *np = dev_of_node(&pdev->dev);
+	struct device *dev = &pdev->dev;
+	struct bcm63138_leds *leds;
+	struct device_node *child;
+
+	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	leds->dev = dev;
+
+	leds->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(leds->base))
+		return PTR_ERR(leds->base);
+
+	spin_lock_init(&leds->lock);
+
+	bcm63138_leds_write(leds, BCM63138_GLB_CTRL,
+			    BCM63138_GLB_CTRL_SERIAL_LED_DATA_PPOL |
+			    BCM63138_GLB_CTRL_SERIAL_LED_EN_POL);
+	bcm63138_leds_write(leds, BCM63138_HW_LED_EN, 0);
+	bcm63138_leds_write(leds, BCM63138_SERIAL_LED_POLARITY, 0);
+	bcm63138_leds_write(leds, BCM63138_PARALLEL_LED_POLARITY, 0);
+
+	for_each_available_child_of_node(np, child) {
+		bcm63138_leds_create_led(leds, child);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id bcm63138_leds_of_match_table[] = {
+	{ .compatible = "brcm,bcm63138-leds", },
+	{ },
+};
+
+static struct platform_driver bcm63138_leds_driver = {
+	.probe = bcm63138_leds_probe,
+	.driver = {
+		.name = "leds-bcm63xxx",
+		.of_match_table = bcm63138_leds_of_match_table,
+	},
+};
+
+module_platform_driver(bcm63138_leds_driver);
+
+MODULE_AUTHOR("Rafał Miłecki");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, bcm63138_leds_of_match_table);
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/2] dt-bindings: leds: add Broadcom's BCM63138 controller

[Rob Herring]

On Wed, 24 Nov 2021 12:19:51 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Broadcom used 2 LEDs hardware blocks for their BCM63xx SoCs:
> 1. Older one (BCM6318, BCM6328, BCM6362, BCM63268, BCM6838)
> 2. Newer one (BCM6848, BCM6858, BCM63138, BCM63148, BCM63381, BCM68360)
> 
> The newer one was also later also used on BCM4908 SoC.
> 
> Old block is already documented in the leds-bcm6328.yaml. This binding
> documents the new one which uses different registers & programming. It's
> first used in BCM63138 thus the binding name.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Rename to bcm63138 & make "brcm,bcm63138-leds" the main compatible
> ---
>  .../bindings/leds/leds-bcm63138.yaml          | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-bcm63138.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH V2 1/2] dt-bindings: leds: add Broadcom's BCM63138 controller

[Rob Herring]

On Wed, 24 Nov 2021 12:19:51 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Broadcom used 2 LEDs hardware blocks for their BCM63xx SoCs:
> 1. Older one (BCM6318, BCM6328, BCM6362, BCM63268, BCM6838)
> 2. Newer one (BCM6848, BCM6858, BCM63138, BCM63148, BCM63381, BCM68360)
> 
> The newer one was also later also used on BCM4908 SoC.
> 
> Old block is already documented in the leds-bcm6328.yaml. This binding
> documents the new one which uses different registers & programming. It's
> first used in BCM63138 thus the binding name.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Rename to bcm63138 & make "brcm,bcm63138-leds" the main compatible
> ---
>  .../bindings/leds/leds-bcm63138.yaml          | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-bcm63138.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/2] leds: bcm63xxx: add support for BCM63138 controller

[Florian Fainelli]

On 11/24/2021 3:19 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> It's a new controller first introduced in BCM63138 SoC. Later it was
> also used in BCM4908, some BCM68xx and some BCM63xxx SoCs.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH V2 2/2] leds: bcm63xxx: add support for BCM63138 controller

[Florian Fainelli]

On 11/24/2021 3:19 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> It's a new controller first introduced in BCM63138 SoC. Later it was
> also used in BCM4908, some BCM68xx and some BCM63xxx SoCs.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 1/2] dt-bindings: leds: add Broadcom's BCM63138 controller

[Florian Fainelli]

On 11/24/2021 3:19 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Broadcom used 2 LEDs hardware blocks for their BCM63xx SoCs:
> 1. Older one (BCM6318, BCM6328, BCM6362, BCM63268, BCM6838)
> 2. Newer one (BCM6848, BCM6858, BCM63138, BCM63148, BCM63381, BCM68360)
> 
> The newer one was also later also used on BCM4908 SoC.
> 
> Old block is already documented in the leds-bcm6328.yaml. This binding
> documents the new one which uses different registers & programming. It's
> first used in BCM63138 thus the binding name.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH V2 1/2] dt-bindings: leds: add Broadcom's BCM63138 controller

[Florian Fainelli]

On 11/24/2021 3:19 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Broadcom used 2 LEDs hardware blocks for their BCM63xx SoCs:
> 1. Older one (BCM6318, BCM6328, BCM6362, BCM63268, BCM6838)
> 2. Newer one (BCM6848, BCM6858, BCM63138, BCM63148, BCM63381, BCM68360)
> 
> The newer one was also later also used on BCM4908 SoC.
> 
> Old block is already documented in the leds-bcm6328.yaml. This binding
> documents the new one which uses different registers & programming. It's
> first used in BCM63138 thus the binding name.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/2] leds: bcm63xxx: add support for BCM63138 controller

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]

Hi!

> It's a new controller first introduced in BCM63138 SoC. Later it was
> also used in BCM4908, some BCM68xx and some BCM63xxx SoCs.
> 

> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index ed800f5da7d8..3bde795f0951 100644
> --- a/drivers/leds/Kconfig

Lets put it into drivers/leds/blink/, please.

> --- /dev/null
> +++ b/drivers/leds/leds-bcm63138.c
> @@ -0,0 +1,314 @@

> +#define BCM63138_LED_BITS				4				/* how many bits control a single LED */
> +#define BCM63138_LED_MASK				((1 << BCM63138_LED_BITS) - 1)	/* 0xf */
> +#define BCM63138_LEDS_PER_REG				(32 / BCM63138_LED_BITS)	/* 8 */

I'm not sure these kinds of defines are useful.

> +static void bcm63138_leds_create_led(struct bcm63138_leds *leds,
> +				     struct device_node *np)
> +{
> +	struct led_init_data init_data = {
> +		.fwnode = of_fwnode_handle(np),
> +	};
> +	struct device *dev = leds->dev;
> +	struct bcm63138_led *led;
> +	struct pinctrl *pinctrl;
> +	const char *state;
> +	u32 bit;
> +	int err;
> +
> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return;

At least warn. User wants to know why his leds don't work.

> +	if (!of_property_read_string(np, "default-state", &state)) {
> +		if (!strcmp(state, "on"))
> +			led->cdev.brightness = LED_FULL;
> +		else
> +			led->cdev.brightness = LED_OFF;
> +	} else {
> +		led->cdev.brightness = LED_OFF;
> +	}

Do you actually need default-state support? Just remove it if not.

You support 4 bit brightness. You should set max_brightness to
15. LED_FULL is mistake (or very old API) in your case.

Otherwise looks quite sane.

Thank you,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH V2 2/2] leds: bcm63xxx: add support for BCM63138 controller

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1733 bytes --]

Hi!

> It's a new controller first introduced in BCM63138 SoC. Later it was
> also used in BCM4908, some BCM68xx and some BCM63xxx SoCs.
> 

> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index ed800f5da7d8..3bde795f0951 100644
> --- a/drivers/leds/Kconfig

Lets put it into drivers/leds/blink/, please.

> --- /dev/null
> +++ b/drivers/leds/leds-bcm63138.c
> @@ -0,0 +1,314 @@

> +#define BCM63138_LED_BITS				4				/* how many bits control a single LED */
> +#define BCM63138_LED_MASK				((1 << BCM63138_LED_BITS) - 1)	/* 0xf */
> +#define BCM63138_LEDS_PER_REG				(32 / BCM63138_LED_BITS)	/* 8 */

I'm not sure these kinds of defines are useful.

> +static void bcm63138_leds_create_led(struct bcm63138_leds *leds,
> +				     struct device_node *np)
> +{
> +	struct led_init_data init_data = {
> +		.fwnode = of_fwnode_handle(np),
> +	};
> +	struct device *dev = leds->dev;
> +	struct bcm63138_led *led;
> +	struct pinctrl *pinctrl;
> +	const char *state;
> +	u32 bit;
> +	int err;
> +
> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return;

At least warn. User wants to know why his leds don't work.

> +	if (!of_property_read_string(np, "default-state", &state)) {
> +		if (!strcmp(state, "on"))
> +			led->cdev.brightness = LED_FULL;
> +		else
> +			led->cdev.brightness = LED_OFF;
> +	} else {
> +		led->cdev.brightness = LED_OFF;
> +	}

Do you actually need default-state support? Just remove it if not.

You support 4 bit brightness. You should set max_brightness to
15. LED_FULL is mistake (or very old API) in your case.

Otherwise looks quite sane.

Thank you,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2 2/2] leds: bcm63xxx: add support for BCM63138 controller

[Rafał Miłecki]

On 15.12.2021 21:26, Pavel Machek wrote:
>> It's a new controller first introduced in BCM63138 SoC. Later it was
>> also used in BCM4908, some BCM68xx and some BCM63xxx SoCs.
>>
> 
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index ed800f5da7d8..3bde795f0951 100644
>> --- a/drivers/leds/Kconfig
> 
> Lets put it into drivers/leds/blink/, please.
> 
>> --- /dev/null
>> +++ b/drivers/leds/leds-bcm63138.c
>> @@ -0,0 +1,314 @@
> 
>> +#define BCM63138_LED_BITS				4				/* how many bits control a single LED */
>> +#define BCM63138_LED_MASK				((1 << BCM63138_LED_BITS) - 1)	/* 0xf */
>> +#define BCM63138_LEDS_PER_REG				(32 / BCM63138_LED_BITS)	/* 8 */
> 
> I'm not sure these kinds of defines are useful.

What do you suggest? I think defines are prefered in Linux kernel
compared to magic values in code.


>> +static void bcm63138_leds_create_led(struct bcm63138_leds *leds,
>> +				     struct device_node *np)
>> +{
>> +	struct led_init_data init_data = {
>> +		.fwnode = of_fwnode_handle(np),
>> +	};
>> +	struct device *dev = leds->dev;
>> +	struct bcm63138_led *led;
>> +	struct pinctrl *pinctrl;
>> +	const char *state;
>> +	u32 bit;
>> +	int err;
>> +
>> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>> +	if (!led)
>> +		return;
> 
> At least warn. User wants to know why his leds don't work.

That would be against Linux's rule. I'm not sure if/where it's explained
in Documentation/ but scripts/checkpatch.pl will complain about that for
sure. OOM errors are loud enough not to require an extra error at driver
level.


>> +	if (!of_property_read_string(np, "default-state", &state)) {
>> +		if (!strcmp(state, "on"))
>> +			led->cdev.brightness = LED_FULL;
>> +		else
>> +			led->cdev.brightness = LED_OFF;
>> +	} else {
>> +		led->cdev.brightness = LED_OFF;
>> +	}
> 
> Do you actually need default-state support? Just remove it if not.
> 
> You support 4 bit brightness. You should set max_brightness to
> 15. LED_FULL is mistake (or very old API) in your case.

Re: [PATCH V2 2/2] leds: bcm63xxx: add support for BCM63138 controller

[Rafał Miłecki]

On 15.12.2021 21:26, Pavel Machek wrote:
>> It's a new controller first introduced in BCM63138 SoC. Later it was
>> also used in BCM4908, some BCM68xx and some BCM63xxx SoCs.
>>
> 
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index ed800f5da7d8..3bde795f0951 100644
>> --- a/drivers/leds/Kconfig
> 
> Lets put it into drivers/leds/blink/, please.
> 
>> --- /dev/null
>> +++ b/drivers/leds/leds-bcm63138.c
>> @@ -0,0 +1,314 @@
> 
>> +#define BCM63138_LED_BITS				4				/* how many bits control a single LED */
>> +#define BCM63138_LED_MASK				((1 << BCM63138_LED_BITS) - 1)	/* 0xf */
>> +#define BCM63138_LEDS_PER_REG				(32 / BCM63138_LED_BITS)	/* 8 */
> 
> I'm not sure these kinds of defines are useful.

What do you suggest? I think defines are prefered in Linux kernel
compared to magic values in code.


>> +static void bcm63138_leds_create_led(struct bcm63138_leds *leds,
>> +				     struct device_node *np)
>> +{
>> +	struct led_init_data init_data = {
>> +		.fwnode = of_fwnode_handle(np),
>> +	};
>> +	struct device *dev = leds->dev;
>> +	struct bcm63138_led *led;
>> +	struct pinctrl *pinctrl;
>> +	const char *state;
>> +	u32 bit;
>> +	int err;
>> +
>> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>> +	if (!led)
>> +		return;
> 
> At least warn. User wants to know why his leds don't work.

That would be against Linux's rule. I'm not sure if/where it's explained
in Documentation/ but scripts/checkpatch.pl will complain about that for
sure. OOM errors are loud enough not to require an extra error at driver
level.


>> +	if (!of_property_read_string(np, "default-state", &state)) {
>> +		if (!strcmp(state, "on"))
>> +			led->cdev.brightness = LED_FULL;
>> +		else
>> +			led->cdev.brightness = LED_OFF;
>> +	} else {
>> +		led->cdev.brightness = LED_OFF;
>> +	}
> 
> Do you actually need default-state support? Just remove it if not.
> 
> You support 4 bit brightness. You should set max_brightness to
> 15. LED_FULL is mistake (or very old API) in your case.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel