Re: [PATCH V2 2/2] leds: bcm63xxx: add support for BCM63138 controller
From: Pavel Machek <hidden>
Date: 2021-12-15 20:26:23
Also in:
linux-devicetree, linux-leds
Hi!
It's a new controller first introduced in BCM63138 SoC. Later it was also used in BCM4908, some BCM68xx and some BCM63xxx SoCs.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
--- /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