Re: [PATCH 07/11] leds: Add NVIDIA LSTP LED driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] leds: applying initial brightness before parsing DT max-brightness
- [Medium] leds: rejecting DT nodes with only color and allowing deprecated label
- [Low] leds: non-standard commit message prefix missing driver name
--

commit 231bdc9e871148317f849221691f708a54dbd399
Author: Adrian Ambrozewicz <aambrozewicz@nvidia.com>

leds: Add NVIDIA LSTP LED driver
    
This commit introduces a new driver for LEDs on NVIDIA LSTP USB adapters.
It uses the GPIO channel of the adapter, hands blink and pattern requests
directly to the hardware, and configures them via device tree child nodes.

> leds: Add NVIDIA LSTP LED driver

[Severity: Low]
This isn't a bug, but the commit message prefix seems to be missing the
driver name. Should this follow the standard format, such as
"leds: lstp: Add NVIDIA LSTP LED driver" to help with git history
categorization?

diff --git a/drivers/leds/leds-lstp.c b/drivers/leds/leds-lstp.c
new file mode 100644
index 0000000000000..352e484eb9194
--- /dev/null
+++ b/drivers/leds/leds-lstp.c
[ ... ]
@@ -275,20 +275,21 @@ static int lstp_led_register(struct device *dev, struct lstp_led *led, struct fw
+		if (!ret) {
+			bool pattern = fwnode_property_present(fwnode, "led-pattern");
+			bool trigger = fwnode_property_present(fwnode, "linux,default-trigger");
+
+			/* A trigger takes precedence; the LED core applies it from @fwnode. */
+			if (pattern && trigger)
+				dev_err(dev,
+					"%s: ch_%d: Ignoring led-pattern of LED '%s': %pfw also sets linux,default-trigger\n",
+					__func__, led->ch->ch_id, led->cdev.name, fwnode);
+			if (pattern && !trigger)
+				lstp_led_send_fwnode_pattern(led, fwnode);
+			else
+				lstp_led_apply_default_state(led, fwnode);

[Severity: High]
Does applying the initial hardware state here happen too early? At this
point, led_classdev_register_ext() has not yet been called, so the device
tree max-brightness property hasn't been parsed into led->cdev.max_brightness.

As a result, lstp_led_apply_default_state() will use the uninitialized
max_brightness value of 255 (U8_MAX) to drive the hardware. Could this risk
over-driving the hardware before the registration layer correctly enforces
the bound?

+			ret = lstp_led_classdev_register(dev, &led->cdev, &init_data);
+		}
+	}
+
+	if (ret)
+		dev_warn(dev, "%s: ch_%d: Could not register LED '%s', pin %u (%pe)\n", __func__,
+			 led->ch->ch_id, led->cdev.name, led->pin, ERR_PTR(ret));
+	return ret;
+}
[ ... ]
@@ -328,34 +328,41 @@ static int lstp_leds_probe(struct auxiliary_device *auxdev, const struct auxilia
[ ... ]
+		if (led->cdev.dev) {
+			dev_warn(dev, "%s: ch_%d: Ignoring %pfw: pin %u is already described\n",
+				 __func__, ch->ch_id, child, pin);
+			continue;
+		}
+		if (!fwnode_property_present(child, "label") &&
+		    !fwnode_property_present(child, "function")) {

[Severity: Medium]
Does this logic incorrectly reject modern device tree nodes? The LED
subsystem guidelines dictate that the label property is deprecated and
should be avoided, while color and function are preferred.

By warning and skipping nodes that lack both label and function, valid
modern DT nodes that rely solely on the color property (e.g.,
color = <LED_COLOR_ID_GREEN>) would be entirely rejected. Should this check
be updated to account for the color property and align with modern naming
requirements?

+			dev_warn(dev, "%s: ch_%d: Ignoring %pfw: neither label nor function\n",
+				 __func__, ch->ch_id, child);
+			continue;
+		}
+		lstp_led_register(dev, led, child);
+	}
+	fwnode_handle_put(node);
+
+	for (u16 i = 0; i < pins->npins; i++) {
+		if (!leds[i].cdev.dev)
+			lstp_led_register(dev, &leds[i], NULL);
+	}
+
+	return 0;
+}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-lstp-usb-adapter-v1-0-aambrozewicz@nvidia.com?part=7