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

[David Yang]

On Wed, Jul 22, 2026 at 1:32 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> > +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?

It's semantically consistent. The led will be turned on on LINK or
HALF_DUPLEX or FULL_DUPLEX.

> > +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?

It's the HW default, and we always perform a hard reset on device init.

> [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?

LED_CTRL_EN is actually kind of no-op, LEDs are always enabled even if
!LED_CTRL_EN.

> [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?

https://elixir.bootlin.com/linux/v7.1.4/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L206
, and such leds subnode can pass dt_binding_check.

> [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?

Pure slop, https://elixir.bootlin.com/linux/v7.1.4/source/Documentation/devicetree/bindings/leds/common.yaml#L213