Re: [PATCH net-next v6 3/3] net: dsa: motorcomm: Add LED support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: motorcomm: Add LED support

This patch adds LED support for the Motorcomm YT9215 DSA switch, allowing
up to 3 LEDs per port to be described in the device tree using a format
similar to qca8k. Only parallel mode and strict 1:1 mapping are supported.

> diff --git a/drivers/net/dsa/motorcomm/Kconfig b/drivers/net/dsa/motorcomm/Kconfig
> index 1fddd386f866..3438e0b14361 100644
> --- a/drivers/net/dsa/motorcomm/Kconfig
> +++ b/drivers/net/dsa/motorcomm/Kconfig
> @@ -6,3 +6,12 @@ config NET_DSA_YT921X
>  	help
>  	  This enables support for the Motorcomm YT9215 ethernet switch
>  	  chip.
> +
> +config NET_DSA_YT921X_LEDS
> +	bool "LED support for Motorcomm YT9215"
> +	default y
> +	depends on NET_DSA_YT921X
> +	depends on LEDS_CLASS=y || LEDS_CLASS=NET_DSA_YT921X

[ ... ]

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 6dee25b6754a..553efacd4f5e 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -4581,6 +4580,12 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
>  	if (res)
>  		return res;
>  
> +#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
> +	res = yt921x_leds_setup(priv);
> +	if (res)
> +		dev_warn(dev, "Failed to setup LEDs: %d\n", res);
> +#endif

[Medium]
When yt921x_leds_setup() fails, the current handling is a dev_warn plus
continue. Looking at the error path inside yt921x_leds_setup():

    for_each_child_of_node_scoped(leds_np, led_np) {
            res = yt921x_led_setup(priv, port,
                                   of_fwnode_handle(led_np),
                                   &inv_mask, &inv_ctrl);
            if (res) {
                    of_node_put(leds_np);
                    goto err;
            }
    }
    ...
err:
    yt921x_leds_remove(priv);

A single malformed subnode on any port (for example reg >= 3, both
active-high and active-low set, or an LED entry attached to the MCU port)
makes yt921x_led_setup() return non-zero, and yt921x_leds_setup() then
tears down every LED that was previously registered on every port. From
the user's perspective this appears as a single dev_warn followed by a
switch that has no working LEDs at all, even on correctly-configured
ports.

Would it be preferable to log the failing LED and continue registering
the rest, so a single DT typo does not disable the whole LED subsystem?

> +#endif
>  
>  	return 0;
>  }
>  
> @@ -4681,6 +4686,10 @@ static void yt921x_mdio_remove(struct mdio_device *mdiodev)

[ ... ]

> diff --git a/drivers/net/dsa/motorcomm/leds.c b/drivers/net/dsa/motorcomm/leds.c
> new file mode 100644
> index 000000000000..46749b57fee3
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/leds.c
> @@ -0,0 +1,630 @@

[ ... ]

> +static const struct yt921x_led_trigger_map yt921x_led_trigger_maps[] = {
> +	{BIT(TRIGGER_NETDEV_LINK),
> +	 YT921X_LEDx_PORT_ACT_DUPLEX_HALF | YT921X_LEDx_PORT_ACT_DUPLEX_FULL},
> +	{BIT(TRIGGER_NETDEV_LINK_10), YT921X_LEDx_PORT_ACT_10M},
> +	{BIT(TRIGGER_NETDEV_LINK_100), YT921X_LEDx_PORT_ACT_100M},
> +	{BIT(TRIGGER_NETDEV_LINK_1000), YT921X_LEDx_PORT_ACT_1000M},
> +	{BIT(TRIGGER_NETDEV_HALF_DUPLEX), YT921X_LEDx_PORT_ACT_DUPLEX_HALF},
> +	{BIT(TRIGGER_NETDEV_FULL_DUPLEX), YT921X_LEDx_PORT_ACT_DUPLEX_FULL},

[ ... ]

> +static int
> +yt921x_led_trigger_get(struct yt921x_priv *priv, int port, int group,
> +		       unsigned long *flagsp)
> +{
[ ... ]
> +	*flagsp = 0;
> +	for (unsigned int i = 0; i < ARRAY_SIZE(yt921x_led_trigger_maps); i++) {
> +		const struct yt921x_led_trigger_map *map = &yt921x_led_trigger_maps[i];
> +
> +		if ((val & map->mask) == map->mask)
> +			*flagsp |= map->flags;
> +	}
> +
> +	return 0;
> +}

[Medium]
Does this iteration over-report the flags for TRIGGER_NETDEV_LINK?

The LINK entry in yt921x_led_trigger_maps sets both DUPLEX_HALF and
DUPLEX_FULL bits. After yt921x_led_trigger_set(LINK) programs the
register with those two bits, yt921x_led_trigger_get() scans every map
entry without breaking:

    /* LINK entry: (val & (HALF|FULL)) == (HALF|FULL)  -> match */
    /* HALF_DUPLEX entry: (val & HALF) == HALF         -> match */
    /* FULL_DUPLEX entry: (val & FULL) == FULL         -> match */

So *flagsp ends up as LINK | HALF_DUPLEX | FULL_DUPLEX rather than just
LINK. Should the map entries with overlapping masks be handled specially,
or should the loop break/clear after a compound match like yt921x_led_
trigger_set() does with flags &= ~map->flags?

[ ... ]

> +int yt921x_leds_setup(struct yt921x_priv *priv)
> +{
[ ... ]
> +	dsa_switch_for_each_port(dp, ds) {
[ ... ]
> +		for_each_child_of_node_scoped(leds_np, led_np) {
> +			res = yt921x_led_setup(priv, port,
> +					       of_fwnode_handle(led_np),
> +					       &inv_mask, &inv_ctrl);
> +			if (res) {
> +				of_node_put(leds_np);
> +				goto err;
> +			}
> +		}
> +
> +		of_node_put(leds_np);
> +	}
> +
> +	mutex_lock(&priv->reg_lock);
> +
> +	mask = YT921X_LED_CTRL_MODE_M | YT921X_LED_CTRL_PORT_NUM_M |
> +	       YT921X_LED_CTRL_EN;
> +	ctrl = YT921X_LED_CTRL_MODE_PARALLEL |
> +	       YT921X_LED_CTRL_PORT_NUM(YT921X_PORT_NUM - 1) |
> +	       YT921X_LED_CTRL_EN;
> +	res = yt921x_reg_update_bits(priv, YT921X_LED_CTRL, mask, ctrl);
> +	if (res)
> +		goto end;
> +
> +	/* Inversion is internal - force on will give low logic.
> +	 * In the rest of the file, treat LEDs as if active-low.
> +	 */
> +	if (inv_mask)
> +		res = yt921x_reg_update_bits(priv, YT921X_LED_PAR_INV, inv_mask,
> +					     inv_ctrl);
> +
> +end:
> +	mutex_unlock(&priv->reg_lock);

[Medium]
The commit message says "strict 1:1 mapping". Where is the port-to-LED
mapping actually being programmed to identity?

The header defines the field macros for these registers:

    #define YT921X_LED_PAR_MAPn(port)	(0xd01d0 + 4 * (port))
    #define  YT921X_LED_PAR_MAP_DSTn_PORT_M(grp)  ...
    #define  YT921X_LED_PAR_MAP_DSTn_PORT(grp, x) ...
    #define  YT921X_LED_PAR_MAP_DSTn_LED_M(grp)   ...
    #define  YT921X_LED_PAR_MAP_DSTn_LED(grp, x)  ...

but there is no writer for YT921X_LED_PAR_MAPn anywhere in the driver.
The 1:1 mapping seems to rely on the reset / bootloader default. If those
values are not identity for some reason, LED operations for port X would
end up steering to a different pin.

Should the driver write an identity mapping into YT921X_LED_PAR_MAPn, or
should the commit message drop the "strict 1:1 mapping" claim?

[Low]
The order in which the LED controller is enabled versus the inversion
register is programmed looks reversed:

    /* enables LED controller first */
    res = yt921x_reg_update_bits(priv, YT921X_LED_CTRL, mask, ctrl);
    ...
    /* programs inversion after */
    if (inv_mask)
            res = yt921x_reg_update_bits(priv, YT921X_LED_PAR_INV, ...);

Per the comment "force on will give low logic", between LED_CTRL_EN
being written and YT921X_LED_PAR_INV being programmed, every
active-high LED with default-state=on would be driven low (LED off)
and only flip to on once the inversion register is written.

If the second write fails, yt921x_leds_remove() is called, which
unregisters classdevs but leaves LED_CTRL_EN set with the wrong
polarity on the physical pins.

Would it be cleaner to program per-LED state and YT921X_LED_PAR_INV
first, and set YT921X_LED_CTRL_EN last?

[High]
Are Documentation/devicetree/bindings updates missing for this change?

The existing binding at
Documentation/devicetree/bindings/net/dsa/motorcomm,yt921x.yaml uses
unevaluatedProperties: false and only references
dsa.yaml#/$defs/ethernet-ports for the port schema, which does not
describe a leds subnode. A device tree that uses the new LED feature
would fail make dtbs_check with unevaluated property errors.

Should the YAML be extended to describe the leds subnode and the
per-LED properties reg, active-high, active-low, and default-state that
the driver consumes here?

[Medium]
The driver reads active-high and active-low as boolean properties on
each LED subnode:

    force_high = fwnode_property_read_bool(fwnode, "active-high");
    force_low = fwnode_property_read_bool(fwnode, "active-low");

active-low is a well-established convention used elsewhere (for
example as a GPIO flag), and the common LED subnode binding in
leds/common.yaml does not define active-high/active-low booleans on an
LED node. Since DT properties are ABI once shipped, would it be better
to reuse an already-standard property, or to invent a driver-namespaced
one, so the naming does not collide with existing conventions?
-- 
pw-bot: cr