| Message ID | 20211124111952.22419-1-zajec5@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [V2,1/2] dt-bindings: leds: add Broadcom's BCM63138 controller | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success | |
| robh/dtbs-check | success |
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>
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>
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>
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
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.
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; + }; + };