Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Thu, 23 Nov 2023 17:13:50 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Christian Marangi <ansuelsmth@gmail.com>,
        Tim Harvey <tharvey@gateworks.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH RFC] net: dsa: mv88e6xxx: Support LED control
Message-ID: <c8c821f8-e170-44b3-a3f9-207cf7cb70e2@lunn.ch>
References: <20231123-mv88e6xxx-leds-v1-1-3c379b3d23fb@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231123-mv88e6xxx-leds-v1-1-3c379b3d23fb@linaro.org>
Precedence: bulk

> This DT config is not yet configuring everything: the netdev
> default trigger is assigned by the hw acceleration callbacks are
> not called, and there is no way to set the netdev sub-trigger
> type from the device tree, such as if you want a gigabit link
> indicator. This has to be done from userspace at this point.

Yes, part of this is a known problem, and somewhere i have some code i
was working on to fix some of these issues.

What i would really like to see happen is that the DSA core handles
the registration of the LEDs, similar to how phylib does. The DT
binding should be identical for all DSA devices, so there is no need
for each driver to do its own parsing.

There are some WIP patches at

https://github.com/lunn/linux.git leds-offload-support-reduced-auto-netdev

which implement this. Feel free to make use of them.

> +/* The following is a lookup table to check what rules we can support on a
> + * certain LED given restrictions such as that some rules only work with fiber
> + * (SFP) connections and some blink on activity by default.
> + */
> +#define MV88E6XXX_PORTS_0_3 (BIT(0)|BIT(1)|BIT(2)|BIT(3))
> +#define MV88E6XXX_PORTS_4_5 (BIT(4)|BIT(5))
> +#define MV88E6XXX_PORT_4 BIT(4)
> +#define MV88E6XXX_PORT_5 BIT(5)
> +
> +/* Entries are listed in selector order */
> +static const struct mv88e6xxx_led_hwconfig mv88e6xxx_led_hwconfigs[] = {

You need to be careful with naming. These are probably specific to the
6352. Different switches probably have different capabilities. So it
would be good to have the names reflect the switch family they are
valid for.

When we come to add support for other switch families, i wounder how
tables like this scale. Is there some things which can be shared, if
we break the table up? I need to check the data sheets.

	Andrew