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:
>
> On Thu,  9 Jul 2026 09:47:47 +0800 David Yang wrote:
> > LEDs can be described in the device tree using the same format as qca8k.
> > Each port can configure up to 3 LEDs.
> >
> > Currently, only parallel mode and strict 1:1 mapping are supported.
>
> > +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
>
> is it really useful to prompt the user with this option?
> Shouldn't this be a hidden option which gets auto-enabled
> when dependencies are met?

NET_DSA_MV88E6XXX_LEDS, NET_DSA_QCA8K_LEDS_SUPPORT, and
NET_DSA_REALTEK_RTL8366RB_LEDS. Better to align with others.

> > +static bool
> > +yt921x_led_trigger_is_supported(const struct yt921x_priv *priv, int port,
> > +                             int group, unsigned long flags)
> > +{
> > +     if (!flags)
> > +             return true;
> > +
> > +     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 ((flags & map->flags) == map->flags) {
> > +                     flags &= ~map->flags;
> > +                     if (!flags)
> > +                             return true;
> > +             }
>
> Isn't this conditions simply:
>
>                 if (flags == map->flags)
>                         return true;
>
> ?

LED_PORT_ACT are bit-masks, so we can have any additive combinations
of trigger_map.flags.

> > +     }
> > +
> > +     return false;
>
>
> > +#if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS)
> > +
> > +void yt921x_leds_remove(struct yt921x_priv *priv);
> > +int yt921x_leds_setup(struct yt921x_priv *priv);
> > +
> > +#else
> > +
> > +static inline void yt921x_leds_remove(struct yt921x_priv *priv) {}
> > +
> > +static inline int yt921x_leds_setup(struct yt921x_priv *priv)
> > +{
> > +     return 0;
> > +}
>
> There are static inlines but I think you still protect the calls to
> these functions with itdef?
>
> +#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

Sure, but I want to play it safe since conditional availability of
functions is kind of error-prone.

> > +#endif
> > +
> > +#endif
>