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

[PATCH 1/2] dt-bindings: leds: add Broadcom's BCM63xxx 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
most commonly used in BCM63138 thus the binding name 63xxx.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../bindings/leds/leds-bcm63xxx.yaml          | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-bcm63xxx.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-bcm63xxx.yaml b/Documentation/devicetree/bindings/leds/leds-bcm63xxx.yaml
new file mode 100644
index 000000000000..3910dd607f3f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-bcm63xxx.yaml
@@ -0,0 +1,94 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-bcm63xxx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom's BCM63xxx LEDs controller
+
+maintainers:
+  - Rafał Miłecki <rafal@milecki.pl>
+
+description: |
+  This LEDs controller is used on BCM4908, BCM6848, BCM6858, BCM63138, BCM63148,
+  BCM63381 and BCM68360.
+
+  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:
+    items:
+      - enum:
+          - brcm,bcm4908-leds
+          - brcm,bcm6848-leds
+          - brcm,bcm6858-leds
+          - brcm,bcm63138-leds
+          - brcm,bcm63148-leds
+          - brcm,bcm63381-leds
+          - brcm,bcm68360-leds
+      - const: brcm,bcm63xxx-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,bcm63xxx-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 2/2] leds: bcm63xxx: add support for BCM63xxx controller

[Rafał Miłecki]

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

It's a new controller used on BCM4908, some BCM68xx and some BCM63xxx
SoCs.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/leds/Kconfig         |  11 ++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-bcm63xxx.c | 299 +++++++++++++++++++++++++++++++++++
 3 files changed, 311 insertions(+)
 create mode 100644 drivers/leds/leds-bcm63xxx.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ed800f5da7d8..b4a35b58eaae 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -122,6 +122,17 @@ config LEDS_BCM6358
 	  This option enables support for LEDs connected to the BCM6358
 	  LED HW controller accessed via MMIO registers.
 
+config LEDS_BCM63XXX
+	tristate "LED Support for Broadcom BCM63xxx SoCs"
+	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
+	  BCM63xxx family SoCs.
+
 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..b8f1b30e8398 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_BCM63XXX)		+= leds-bcm63xxx.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-bcm63xxx.c b/drivers/leds/leds-bcm63xxx.c
new file mode 100644
index 000000000000..5128b28b748d
--- /dev/null
+++ b/drivers/leds/leds-bcm63xxx.c
@@ -0,0 +1,299 @@
+// 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 BCM63XXX_MAX_LEDS			32
+
+#define BCM63XXX_GLB_CTRL			0x00
+#define BCM63XXX_MASK				0x04
+#define BCM63XXX_HW_LED_EN			0x08
+#define BCM63XXX_SERIAL_LED_SHIFT_SEL		0x0c
+#define BCM63XXX_FLASH_RATE_CTRL1		0x10
+#define BCM63XXX_FLASH_RATE_CTRL2		0x14
+#define BCM63XXX_FLASH_RATE_CTRL3		0x18
+#define BCM63XXX_FLASH_RATE_CTRL4		0x1c
+#define BCM63XXX_BRIGHT_CTRL1			0x20
+#define BCM63XXX_BRIGHT_CTRL2			0x24
+#define BCM63XXX_BRIGHT_CTRL3			0x28
+#define BCM63XXX_BRIGHT_CTRL4			0x2c
+#define BCM63XXX_POWER_LED_CFG			0x30
+#define BCM63XXX_HW_POLARITY			0xb4
+#define BCM63XXX_SW_DATA			0xb8
+#define BCM63XXX_SW_POLARITY			0xbc
+#define BCM63XXX_PARALLEL_LED_POLARITY		0xc0
+#define BCM63XXX_SERIAL_LED_POLARITY		0xc4
+#define BCM63XXX_HW_LED_STATUS			0xc8
+#define BCM63XXX_FLASH_CTRL_STATUS		0xcc
+#define BCM63XXX_FLASH_BRT_CTRL			0xd0
+#define BCM63XXX_FLASH_P_LED_OUT_STATUS		0xd4
+#define BCM63XXX_FLASH_S_LED_OUT_STATUS		0xd8
+
+struct bcm63xxx_leds {
+	struct device *dev;
+	void __iomem *base;
+	spinlock_t lock;
+};
+
+struct bcm63xxx_led {
+	struct bcm63xxx_leds *leds;
+	struct led_classdev cdev;
+	u32 pin;
+	bool active_low;
+};
+
+/*
+ * I/O access
+ */
+
+static void bcm63xxx_leds_write(struct bcm63xxx_leds *leds, unsigned int reg,
+				u32 data)
+{
+	writel(data, leds->base + reg);
+}
+
+static unsigned long bcm63xxx_leds_read(struct bcm63xxx_leds *leds,
+					unsigned int reg)
+{
+	return readl(leds->base + reg);
+}
+
+static void bcm63xxx_leds_update_bits(struct bcm63xxx_leds *leds,
+				      unsigned int reg, u32 mask, u32 val)
+{
+	WARN_ON(val & ~mask);
+
+	bcm63xxx_leds_write(leds, reg, (bcm63xxx_leds_read(leds, reg) & ~mask) | (val & mask));
+}
+
+/*
+ * Helpers
+ */
+
+static void bcm63xxx_leds_set_flash_rate(struct bcm63xxx_leds *leds,
+					 struct bcm63xxx_led *led,
+					 u8 value)
+{
+	int reg_offset = (led->pin >> 3) * 4;
+	int shift = (led->pin & 0xf) * 4;
+
+	bcm63xxx_leds_update_bits(leds, BCM63XXX_FLASH_RATE_CTRL1 + reg_offset,
+				  0xf << shift, value << shift);
+}
+
+static void bcm63xxx_leds_enable_led(struct bcm63xxx_leds *leds,
+				     struct bcm63xxx_led *led,
+				     enum led_brightness value)
+{
+	u32 bit = BIT(led->pin);
+
+	bcm63xxx_leds_update_bits(leds, BCM63XXX_SW_DATA, bit,
+				  value == LED_OFF ? 0 : bit);
+}
+
+/*
+ * API callbacks
+ */
+
+static void bcm63xxx_leds_brightness_set(struct led_classdev *led_cdev,
+					 enum led_brightness value)
+{
+	struct bcm63xxx_led *led = container_of(led_cdev, struct bcm63xxx_led, cdev);
+	struct bcm63xxx_leds *leds = led->leds;
+	unsigned long flags;
+
+	spin_lock_irqsave(&leds->lock, flags);
+
+	bcm63xxx_leds_enable_led(leds, led, value);
+	if (value == LED_OFF)
+		bcm63xxx_leds_set_flash_rate(leds, led, 0);
+
+	spin_unlock_irqrestore(&leds->lock, flags);
+}
+
+static int bcm63xxx_leds_blink_set(struct led_classdev *led_cdev,
+				   unsigned long *delay_on,
+				   unsigned long *delay_off)
+{
+	struct bcm63xxx_led *led = container_of(led_cdev, struct bcm63xxx_led, cdev);
+	struct bcm63xxx_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);
+
+	bcm63xxx_leds_enable_led(leds, led, LED_FULL);
+	bcm63xxx_leds_set_flash_rate(leds, led, value);
+
+	spin_unlock_irqrestore(&leds->lock, flags);
+
+	return 0;
+}
+
+/*
+ * LED driver
+ */
+
+static int bcm63xxx_leds_create_led(struct bcm63xxx_leds *leds, struct device_node *np)
+{
+	struct led_init_data init_data = {
+		.fwnode = of_fwnode_handle(np),
+	};
+	struct device *dev = leds->dev;
+	struct bcm63xxx_led *led;
+	struct pinctrl *pinctrl;
+	const char *state;
+	u32 bit;
+	int err;
+
+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->leds = leds;
+
+	if (of_property_read_u32(np, "reg", &led->pin)) {
+		dev_err(dev, "Missing \"reg\" property in %pOF\n", np);
+		err = -ENOENT;
+		goto err_out;
+	}
+
+	if (led->pin >= BCM63XXX_MAX_LEDS) {
+		dev_err(dev, "Invalid \"reg\" value %d\n", led->pin);
+		err = -EINVAL;
+		goto err_out;
+	}
+
+	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 = bcm63xxx_leds_brightness_set;
+	led->cdev.blink_set = bcm63xxx_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_out;
+	}
+
+	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);
+	bcm63xxx_leds_update_bits(leds, BCM63XXX_PARALLEL_LED_POLARITY, bit,
+				  led->active_low ? 0 : bit);
+	bcm63xxx_leds_update_bits(leds, BCM63XXX_HW_LED_EN, bit, 0);
+	bcm63xxx_leds_set_flash_rate(leds, led, 0);
+	bcm63xxx_leds_enable_led(leds, led, led->cdev.brightness);
+
+	return 0;
+
+err_out:
+	devm_kfree(dev, led);
+	return err;
+}
+
+static int bcm63xxx_leds_probe(struct platform_device *pdev)
+{
+	struct device_node *np = dev_of_node(&pdev->dev);
+	struct device *dev = &pdev->dev;
+	struct bcm63xxx_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);
+
+	bcm63xxx_leds_write(leds, BCM63XXX_GLB_CTRL, 0xa);
+	bcm63xxx_leds_write(leds, BCM63XXX_BRIGHT_CTRL1, 0x88888888);
+	bcm63xxx_leds_write(leds, BCM63XXX_BRIGHT_CTRL2, 0x88888888);
+	bcm63xxx_leds_write(leds, BCM63XXX_BRIGHT_CTRL3, 0x88888888);
+	bcm63xxx_leds_write(leds, BCM63XXX_BRIGHT_CTRL4, 0x88888888);
+	bcm63xxx_leds_write(leds, BCM63XXX_HW_LED_EN, 0);
+	bcm63xxx_leds_write(leds, BCM63XXX_SERIAL_LED_POLARITY, 0);
+	bcm63xxx_leds_write(leds, BCM63XXX_PARALLEL_LED_POLARITY, 0);
+
+	for_each_available_child_of_node(np, child) {
+		bcm63xxx_leds_create_led(leds, child);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id bcm63xxx_leds_of_match_table[] = {
+	{ .compatible = "brcm,bcm63xxx-leds", },
+	{ },
+};
+
+static struct platform_driver bcm63xxx_leds_driver = {
+	.probe = bcm63xxx_leds_probe,
+	.driver = {
+		.name = "leds-bcm63xxx",
+		.of_match_table = bcm63xxx_leds_of_match_table,
+	},
+};
+
+module_platform_driver(bcm63xxx_leds_driver);
+
+MODULE_AUTHOR("Rafał Miłecki");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, bcm63xxx_leds_of_match_table);
-- 
2.31.1

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

[Florian Fainelli]

On 11/15/21 1:11 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)

Just so the existing pattern/regexps continue to work, I would be naming
this "bcm63xx" to be consistent with the rest of existing code-base.
-- 
Florian

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

[Rafał Miłecki]

On 22.11.2021 22:51, Florian Fainelli wrote:
> On 11/15/21 1:11 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)
> 
> Just so the existing pattern/regexps continue to work, I would be naming
> this "bcm63xx" to be consistent with the rest of existing code-base.

The problem I saw with "bcm63xx" is that it seems to match all SoCs:
those with old block and those with new block. So I guess both groups
have the same right to use that "bcm63xx" based binding.

To avoid favouring old or new block I decided to avoid "bcm63xx".

Given above explanation: do you still prefer using "bcm63xx" based
binding for the new block? I'm OK with that, I just want to make sure
you're aware of that minor issue. Please let me know :)

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

[Florian Fainelli]

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

On 11/15/21 1:11 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> It's a new controller used on BCM4908, some BCM68xx and some BCM63xxx
> SoCs.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Same comment as the binding, please s/bcm63xxx/bcm63xx/ for matchign
existing drivers/patterns.

[snip]

> +
> +#define BCM63XXX_MAX_LEDS			32> +
> +#define BCM63XXX_GLB_CTRL			0x00
> +#define BCM63XXX_MASK				0x04

This define appears unused.

> +#define BCM63XXX_HW_LED_EN			0x08
> +#define BCM63XXX_SERIAL_LED_SHIFT_SEL		0x0c
> +#define BCM63XXX_FLASH_RATE_CTRL1		0x10
> +#define BCM63XXX_FLASH_RATE_CTRL2		0x14
> +#define BCM63XXX_FLASH_RATE_CTRL3		0x18
> +#define BCM63XXX_FLASH_RATE_CTRL4		0x1c
> +#define BCM63XXX_BRIGHT_CTRL1			0x20
> +#define BCM63XXX_BRIGHT_CTRL2			0x24
> +#define BCM63XXX_BRIGHT_CTRL3			0x28
> +#define BCM63XXX_BRIGHT_CTRL4			0x2c
> +#define BCM63XXX_POWER_LED_CFG			0x30
> +#define BCM63XXX_HW_POLARITY			0xb4
> +#define BCM63XXX_SW_DATA			0xb8

This is called SW_LED_IP in the register but I guess this name is a bit
clearer.

> +#define BCM63XXX_SW_POLARITY			0xbc
> +#define BCM63XXX_PARALLEL_LED_POLARITY		0xc0
> +#define BCM63XXX_SERIAL_LED_POLARITY		0xc4
> +#define BCM63XXX_HW_LED_STATUS			0xc8
> +#define BCM63XXX_FLASH_CTRL_STATUS		0xcc
> +#define BCM63XXX_FLASH_BRT_CTRL			0xd0
> +#define BCM63XXX_FLASH_P_LED_OUT_STATUS		0xd4
> +#define BCM63XXX_FLASH_S_LED_OUT_STATUS		0xd8
> +
> +struct bcm63xxx_leds {
> +	struct device *dev;
> +	void __iomem *base;
> +	spinlock_t lock;
> +};
> +
> +struct bcm63xxx_led {
> +	struct bcm63xxx_leds *leds;
> +	struct led_classdev cdev;
> +	u32 pin;
> +	bool active_low;
> +};
> +
> +/*
> + * I/O access
> + */
> +
> +static void bcm63xxx_leds_write(struct bcm63xxx_leds *leds, unsigned int reg,
> +				u32 data)
> +{
> +	writel(data, leds->base + reg);
> +}
> +
> +static unsigned long bcm63xxx_leds_read(struct bcm63xxx_leds *leds,
> +					unsigned int reg)
> +{
> +	return readl(leds->base + reg);
> +}
> +
> +static void bcm63xxx_leds_update_bits(struct bcm63xxx_leds *leds,
> +				      unsigned int reg, u32 mask, u32 val)
> +{
> +	WARN_ON(val & ~mask);
> +
> +	bcm63xxx_leds_write(leds, reg, (bcm63xxx_leds_read(leds, reg) & ~mask) | (val & mask));
> +}
> +
> +/*
> + * Helpers
> + */
> +
> +static void bcm63xxx_leds_set_flash_rate(struct bcm63xxx_leds *leds,
> +					 struct bcm63xxx_led *led,
> +					 u8 value)
> +{
> +	int reg_offset = (led->pin >> 3) * 4;

Maybe add some definitions here, like LEDS_PER_WORD and LED_SHIFT and
LED_MASK?

[snip]

> +static int bcm63xxx_leds_create_led(struct bcm63xxx_leds *leds, struct device_node *np)
> +{

You are not checking the return value of this function, make it void?

[snip]

> +static int bcm63xxx_leds_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = dev_of_node(&pdev->dev);
> +	struct device *dev = &pdev->dev;
> +	struct bcm63xxx_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);
> +
> +	bcm63xxx_leds_write(leds, BCM63XXX_GLB_CTRL, 0xa);

We would need a define for that:

0x2 -> SERIAL_LED_DATA_PPOL
0x8 -> SERIAL_LED_EN_POL

> +	bcm63xxx_leds_write(leds, BCM63XXX_BRIGHT_CTRL1, 0x88888888);

Cannot we let the LED subsystem change the default brightness?
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

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

[Florian Fainelli]

On 11/22/21 2:00 PM, Rafał Miłecki wrote:
> On 22.11.2021 22:51, Florian Fainelli wrote:
>> On 11/15/21 1:11 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)
>>
>> Just so the existing pattern/regexps continue to work, I would be naming
>> this "bcm63xx" to be consistent with the rest of existing code-base.
> 
> The problem I saw with "bcm63xx" is that it seems to match all SoCs:
> those with old block and those with new block. So I guess both groups
> have the same right to use that "bcm63xx" based binding.
> 
> To avoid favouring old or new block I decided to avoid "bcm63xx".
> 
> Given above explanation: do you still prefer using "bcm63xx" based
> binding for the new block? I'm OK with that, I just want to make sure
> you're aware of that minor issue. Please let me know :)

Maybe we use leds-bcm63138.c then since this is the first chip in the
list that featured that block, similar to how leds-bcm6328.c was
created? Then my second choice would be leds-bcm63xx.c just so the
existing patterns match, really and because it's easy to visually not be
able to tell the difference between two x versus three x.

Thanks
-- 
Florian

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

[Rafał Miłecki]

On 23.11.2021 23:17, Florian Fainelli wrote:
> On 11/22/21 2:00 PM, Rafał Miłecki wrote:
>> On 22.11.2021 22:51, Florian Fainelli wrote:
>>> On 11/15/21 1:11 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)
>>>
>>> Just so the existing pattern/regexps continue to work, I would be naming
>>> this "bcm63xx" to be consistent with the rest of existing code-base.
>>
>> The problem I saw with "bcm63xx" is that it seems to match all SoCs:
>> those with old block and those with new block. So I guess both groups
>> have the same right to use that "bcm63xx" based binding.
>>
>> To avoid favouring old or new block I decided to avoid "bcm63xx".
>>
>> Given above explanation: do you still prefer using "bcm63xx" based
>> binding for the new block? I'm OK with that, I just want to make sure
>> you're aware of that minor issue. Please let me know :)
> 
> Maybe we use leds-bcm63138.c then since this is the first chip in the
> list that featured that block, similar to how leds-bcm6328.c was
> created? Then my second choice would be leds-bcm63xx.c just so the
> existing patterns match, really and because it's easy to visually not be
> able to tell the difference between two x versus three x.

Sounds good to me, thanks for your review!

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

[Rafał Miłecki]

On 22.11.2021 23:04, Florian Fainelli wrote:
> On 11/15/21 1:11 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> It's a new controller used on BCM4908, some BCM68xx and some BCM63xxx
>> SoCs.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> 
> Same comment as the binding, please s/bcm63xxx/bcm63xx/ for matchign
> existing drivers/patterns.
> 
> [snip]
> 
>> +
>> +#define BCM63XXX_MAX_LEDS			32> +
>> +#define BCM63XXX_GLB_CTRL			0x00
>> +#define BCM63XXX_MASK				0x04
> 
> This define appears unused.

Just like few other registers. I think it's still worth to define all hw
registers.