[PATCH 0/4] leds: bcm63138: Add some new bindings and code

[PATCH 0/4] leds: bcm63138: Add some new bindings and code

[Linus Walleij]

This brings over the few know-how items and little things
I found about the BCMBCA LEDs in my previous duplicate driver.

This was tested on the BCM6846-based Genexis XG6846B.

The main addition is the ability to define the number shifting
bits in the serial shift register which is necessary if the
previous boot stages have not set up the hardware properly
before.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Linus Walleij (4):
      dt-bindings: leds: bcm63128: Add shift register bits
      leds: bcm63138: Use scopes and guards
      leds: bcm63128: Handle shift register config
      leds: bcm63128: Add some register defines

 .../devicetree/bindings/leds/leds-bcm63138.yaml    | 13 ++++++++++
 drivers/leds/blink/leds-bcm63138.c                 | 28 ++++++++++++----------
 2 files changed, 29 insertions(+), 12 deletions(-)
---
base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
change-id: 20240920-bcm63138-leds-1bc5f41c47cd

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH 1/4] dt-bindings: leds: bcm63128: Add shift register bits

[Linus Walleij]

The BCM63138 family of serial LED controllers has a register
where we can set up bits for the shift registers. These are
the number of rounds the bits need to be shifted before all
bits have been shifted through the external shift registers.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/devicetree/bindings/leds/leds-bcm63138.yaml | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml b/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml
index bb20394fca5c..d27148279ead 100644
--- a/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml
@@ -41,6 +41,18 @@ properties:
   "#size-cells":
     const: 0
 
+  brcm,serial-shift-bits:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 32
+    description:
+      This describes the number of 8-bit serial shifters
+      connected to the LED controller block. The hardware
+      is typically using 8-bit shift registers with 8 LEDs
+      per shift register, so 4 shifters results in 32 LEDs
+      or 2 shifters give 16 LEDs etc, but the hardware
+      supports any odd number of registers.
+
 patternProperties:
   "^led@[a-f0-9]+$":
     type: object
@@ -71,6 +83,7 @@ examples:
     leds@ff800800 {
         compatible = "brcm,bcm4908-leds", "brcm,bcm63138-leds";
         reg = <0xff800800 0xdc>;
+        brcm,serial-shift-bits = <16>;
 
         #address-cells = <1>;
         #size-cells = <0>;

-- 
2.46.0

Re: [PATCH 1/4] dt-bindings: leds: bcm63128: Add shift register bits

[Rob Herring (Arm)]

On Fri, 20 Sep 2024 23:59:03 +0200, Linus Walleij wrote:
> The BCM63138 family of serial LED controllers has a register
> where we can set up bits for the shift registers. These are
> the number of rounds the bits need to be shifted before all
> bits have been shifted through the external shift registers.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/devicetree/bindings/leds/leds-bcm63138.yaml | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/leds/leds-bcm63138.yaml: properties:brcm,serial-shift-bits: '$ref' should not be valid under {'const': '$ref'}
	hint: Standard unit suffix properties don't need a type $ref
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/leds/leds-bcm63138.example.dtb: leds@ff800800: brcm,serial-shift-bits: 16 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/leds/leds-bcm63138.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/leds/leds-bcm63138.example.dtb: leds@ff800800: brcm,serial-shift-bits: 16 is not of type 'array'
	from schema $id: http://devicetree.org/schemas/property-units.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240920-bcm63138-leds-v1-1-c150871324a0@linaro.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH 2/4] leds: bcm63138: Use scopes and guards

[Linus Walleij]

Use scoped helpers and guards to handle DT node iterations
and spinlocks. This cuts some lines of code and eliminates
common mistakes (such as the missing of_node_put()).

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/leds/blink/leds-bcm63138.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/blink/leds-bcm63138.c b/drivers/leds/blink/leds-bcm63138.c
index 3a5e0b98bfbc..374f68f4f277 100644
--- a/drivers/leds/blink/leds-bcm63138.c
+++ b/drivers/leds/blink/leds-bcm63138.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
  */
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/leds.h>
@@ -125,17 +126,14 @@ static void bcm63138_leds_brightness_set(struct led_classdev *led_cdev,
 {
 	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);
+	guard(spinlock_irqsave)(&leds->lock);
 
 	bcm63138_leds_enable_led(leds, led, value);
 	if (!value)
 		bcm63138_leds_set_flash_rate(leds, led, 0);
 	else
 		bcm63138_leds_set_bright(leds, led, value);
-
-	spin_unlock_irqrestore(&leds->lock, flags);
 }
 
 static int bcm63138_leds_blink_set(struct led_classdev *led_cdev,
@@ -144,7 +142,6 @@ static int bcm63138_leds_blink_set(struct led_classdev *led_cdev,
 {
 	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) {
@@ -179,13 +176,11 @@ static int bcm63138_leds_blink_set(struct led_classdev *led_cdev,
 		return -EINVAL;
 	}
 
-	spin_lock_irqsave(&leds->lock, flags);
+	guard(spinlock_irqsave)(&leds->lock);
 
 	bcm63138_leds_enable_led(leds, led, BCM63138_MAX_BRIGHTNESS);
 	bcm63138_leds_set_flash_rate(leds, led, value);
 
-	spin_unlock_irqrestore(&leds->lock, flags);
-
 	return 0;
 }
 
@@ -259,7 +254,6 @@ 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)
@@ -280,7 +274,7 @@ static int bcm63138_leds_probe(struct platform_device *pdev)
 	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) {
+	for_each_available_child_of_node_scoped(np, child) {
 		bcm63138_leds_create_led(leds, child);
 	}
 

-- 
2.46.0

Re: [PATCH 2/4] leds: bcm63138: Use scopes and guards

[Florian Fainelli]

On 9/20/24 14:59, Linus Walleij wrote:
> Use scoped helpers and guards to handle DT node iterations
> and spinlocks. This cuts some lines of code and eliminates
> common mistakes (such as the missing of_node_put()).
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH 3/4] leds: bcm63128: Handle shift register config

[Linus Walleij]

This adds code to optionally read the width of the shift register
chain from the device tree and use it to set up the register
controlling the shifter hardware.

If the property is not present, the boot-time default is used so
existing device trees keep working as this is what they assume.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/leds/blink/leds-bcm63138.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/leds/blink/leds-bcm63138.c b/drivers/leds/blink/leds-bcm63138.c
index 374f68f4f277..bf170a5bb12a 100644
--- a/drivers/leds/blink/leds-bcm63138.c
+++ b/drivers/leds/blink/leds-bcm63138.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
  */
+#include <linux/bits.h>
 #include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/io.h>
@@ -254,6 +255,7 @@ 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;
+	u32 shift_bits;
 
 	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
 	if (!leds)
@@ -267,6 +269,12 @@ static int bcm63138_leds_probe(struct platform_device *pdev)
 
 	spin_lock_init(&leds->lock);
 
+	/* If this property is not present, we use boot defaults */
+	if (!of_property_read_u32(np, "brcm,serial-shift-bits", &shift_bits)) {
+		bcm63138_leds_write(leds, BCM63138_SERIAL_LED_SHIFT_SEL,
+				    GENMASK(32 - shift_bits - 1, 0));
+	}
+
 	bcm63138_leds_write(leds, BCM63138_GLB_CTRL,
 			    BCM63138_GLB_CTRL_SERIAL_LED_DATA_PPOL |
 			    BCM63138_GLB_CTRL_SERIAL_LED_EN_POL);

-- 
2.46.0

Re: [PATCH 3/4] leds: bcm63128: Handle shift register config

[Florian Fainelli]

On 9/20/24 14:59, Linus Walleij wrote:
> This adds code to optionally read the width of the shift register
> chain from the device tree and use it to set up the register
> controlling the shifter hardware.
> 
> If the property is not present, the boot-time default is used so
> existing device trees keep working as this is what they assume.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>   drivers/leds/blink/leds-bcm63138.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/leds/blink/leds-bcm63138.c b/drivers/leds/blink/leds-bcm63138.c
> index 374f68f4f277..bf170a5bb12a 100644
> --- a/drivers/leds/blink/leds-bcm63138.c
> +++ b/drivers/leds/blink/leds-bcm63138.c
> @@ -2,6 +2,7 @@
>   /*
>    * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
>    */
> +#include <linux/bits.h>
>   #include <linux/cleanup.h>
>   #include <linux/delay.h>
>   #include <linux/io.h>
> @@ -254,6 +255,7 @@ 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;
> +	u32 shift_bits;
>   
>   	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
>   	if (!leds)
> @@ -267,6 +269,12 @@ static int bcm63138_leds_probe(struct platform_device *pdev)
>   
>   	spin_lock_init(&leds->lock);
>   
> +	/* If this property is not present, we use boot defaults */
> +	if (!of_property_read_u32(np, "brcm,serial-shift-bits", &shift_bits)) {
> +		bcm63138_leds_write(leds, BCM63138_SERIAL_LED_SHIFT_SEL,
> +				    GENMASK(32 - shift_bits - 1, 0));

In the binding you allow a value of up to 32 for that property, but that 
would lead to 32 - 32 - 1, which would be undefined?
-- 
Florian

RE: [PATCH 3/4] leds: bcm63128: Handle shift register config

[William Zhang]

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

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Friday, September 20, 2024 2:59 PM
> To: Pavel Machek <pavel@ucw.cz>; Lee Jones <lee@kernel.org>; Rob
> Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> Conor Dooley <conor+dt@kernel.org>; William Zhang
> <william.zhang@broadcom.com>; Anand Gore
> <anand.gore@broadcom.com>; Kursad Oney
> <kursad.oney@broadcom.com>; Florian Fainelli
> <florian.fainelli@broadcom.com>; Rafał Miłecki <rafal@milecki.pl>;
> Broadcom internal kernel review list <bcm-kernel-feedback-
> list@broadcom.com>
> Cc: linux-leds@vger.kernel.org; devicetree@vger.kernel.org; Linus Walleij
> <linus.walleij@linaro.org>
> Subject: [PATCH 3/4] leds: bcm63128: Handle shift register config
typo:  bcm63128 -> bcm63138

>
> This adds code to optionally read the width of the shift register
> chain from the device tree and use it to set up the register
> controlling the shifter hardware.
>
> If the property is not present, the boot-time default is used so
> existing device trees keep working as this is what they assume.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/leds/blink/leds-bcm63138.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/leds/blink/leds-bcm63138.c b/drivers/leds/blink/leds-
> bcm63138.c
> index 374f68f4f277..bf170a5bb12a 100644
> --- a/drivers/leds/blink/leds-bcm63138.c
> +++ b/drivers/leds/blink/leds-bcm63138.c
> @@ -2,6 +2,7 @@
>  /*
>   * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
>   */
> +#include <linux/bits.h>
>  #include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
> @@ -254,6 +255,7 @@ 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;
> +	u32 shift_bits;
>
>  	leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL);
>  	if (!leds)
> @@ -267,6 +269,12 @@ static int bcm63138_leds_probe(struct
> platform_device *pdev)
>
>  	spin_lock_init(&leds->lock);
>
> +	/* If this property is not present, we use boot defaults */
> +	if (!of_property_read_u32(np, "brcm,serial-shift-bits", &shift_bits)) {
> +		bcm63138_leds_write(leds,
> BCM63138_SERIAL_LED_SHIFT_SEL,
> +				    GENMASK(32 - shift_bits - 1, 0));
> +	}
> +
>  	bcm63138_leds_write(leds, BCM63138_GLB_CTRL,
>  			    BCM63138_GLB_CTRL_SERIAL_LED_DATA_PPOL |
>  			    BCM63138_GLB_CTRL_SERIAL_LED_EN_POL);
>
> --
> 2.46.0

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

[PATCH 4/4] leds: bcm63128: Add some register defines

[Linus Walleij]

The Powert LUT (Look-up Table) register base was missing, also
add the bit define for sending serial LED data in reverse order,
and use the BIT() macro to define the bits in the control
register.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/leds/blink/leds-bcm63138.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/blink/leds-bcm63138.c b/drivers/leds/blink/leds-bcm63138.c
index bf170a5bb12a..7d9d970b2082 100644
--- a/drivers/leds/blink/leds-bcm63138.c
+++ b/drivers/leds/blink/leds-bcm63138.c
@@ -21,8 +21,9 @@
 #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_GLB_CTRL_SERIAL_LED_DATA_PPOL		BIT(1)
+#define  BCM63138_GLB_CTRL_SERIAL_LED_EN_POL		BIT(3)
+#define  BCM63138_GLB_CTRL_SERIAL_LED_MSB_FIRST		BIT(4)
 #define BCM63138_MASK					0x04
 #define BCM63138_HW_LED_EN				0x08
 #define BCM63138_SERIAL_LED_SHIFT_SEL			0x0c
@@ -35,6 +36,7 @@
 #define BCM63138_BRIGHT_CTRL3				0x28
 #define BCM63138_BRIGHT_CTRL4				0x2c
 #define BCM63138_POWER_LED_CFG				0x30
+#define BCM63138_POWER_LUT				0x34 /* -> b0 */
 #define BCM63138_HW_POLARITY				0xb4
 #define BCM63138_SW_DATA				0xb8
 #define BCM63138_SW_POLARITY				0xbc

-- 
2.46.0