Date: Tue, 23 Jan 2024 18:34:28 +0100
From: Marek Vasut <marex@...x.de>
To: Florian Fainelli <florian.fainelli@...adcom.com>, netdev@...r.kernel.org,
 Christian Marangi <ansuelsmth@...il.com>
Cc: "David S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>, Conor Dooley <conor+dt@...nel.org>,
 Eric Dumazet <edumazet@...gle.com>, Heiner Kallweit <hkallweit1@...il.com>,
 Jakub Kicinski <kuba@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Lee Jones <lee@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Pavel Machek <pavel@....cz>, Rafał Miłecki
 <rafal@...ecki.pl>, Rob Herring <robh+dt@...nel.org>,
 Russell King <linux@...linux.org.uk>, devicetree@...r.kernel.org,
 linux-leds@...r.kernel.org
Subject: Re: [PATCH] [RFC] net: phy: broadcom: Add DT LED configuration
 support

On 1/22/24 23:34, Florian Fainelli wrote:
> On 1/22/24 12:45, Marek Vasut wrote:
>> The BCM54213E and similar PHYs have extensive LED configuration
>> capabilities -- the PHY has two LEDs, either of the two LEDs can
>> be configured to 1 of 16 functions (speed, TX, RX, activity, on,
>> off, quality, ... multi-color) used to drive single-color LED.
>> The multi-color mode is special, it provides 16 more sub-modes
>> used to drive multi-color LED.
>>
>> The current configuration -- both LEDs configured as multi-color,
>> with both LEDs multi-color sub-mode set to link activity indicator,
>> is not suitable for all systems in which this PHY is used.
>>
>> Attempt to implement a way to describe the LED configuration in DT.
>>
>> Use Documentation/devicetree/bindings/net/ethernet-phy.yaml leds {}
>> subnode of the PHY DT node, describe both LEDs present on this PHY
>> as single LEDs within the leds {} subnode. Each described LED is a
>> subnode of its own, the description uses standard LED subsystem
>> bindings from Documentation/devicetree/bindings/leds/common.yaml .
>>
>> The DT description of the LED configuration can look for example
>> like this:
>>
>> "
>> ethernet-phy@1 {
>> ...
>>     leds {
>>         #address-cells = <1>;
>>         #size-cells = <0>;
>>
>>         led@0 {
>>             reg = <0>;
>>             function = LED_FUNCTION_ACTIVITY;
>>         };
>>
>>         led@1 {
>>             reg = <1>;
>>             function = LED_FUNCTION_SPEED_2;
>>         };
>>     };
>> };
>> "
>>
>> Implement parsing code in the broadcom PHY driver to detemine desired
>> LED configuration from DT. In case the leds {} subnode is present, the
>> parser code iterates over its subnodes and for each led@N subnode it
>> parses the following properties:
>>
>> - reg - LED ID, either 0 or 1, used to identify the LED on the PHY
>> - function - LED single-color function (speed, TX, RX, multi-color...),
>>               uses LED subsystem LED_FUNCTION_* string. The parser in
>>          the driver maps this to register setting.
>> - function-enumerator - In case function is set to "multi-color",
>>                          the multi-color function number. The parser
>>             in the driver uses this value directly for
>>             the multi-color configuration register.
>>
>> Once the properties are parsed, the LED configuration registers of the
>> PHY are programmed.
>>
>> The current list of LED subsystem LED_FUNCTION_* does not cover the
>> entire list of possible single-color LED functions of this PHY, add
>> example extension for "link speed 1" and "link speed 2" setting into
>> the leds/common.h header file.
>>
>> The function-enumerator should probably not be a number, but maybe
>> some sort of macro specific to this PHY ? I would like to avoid new
>> broadcom PHY specific DT properties.
> 
> The parsing should definitively not be in the driver code, the driver 
> should only be providing a mapping between the function and enumerator 
> and a method to set those. Christian has been working on Ethernet PHY 
> LEDs for a while now, so he would be in a better position to comment 
> about how to about that.
> 
> The LED functions and register interface is actually quite stable across 
> Ethernet PHYs from Broadcom so this code, however it looks like in the 
> future should be moved to bcm-phy-lib.[ch]. If and where they are 
> differences we can account for them in the library or by having each PHY 
> driver entry provide a bcm54xx_* wrapper function that provides a table 
> with the appropriate mapping.

I very much agree. I also hope Rafal can chime in, I saw some openwrt 
LED patches floating around recently.

Powered by blists - more mailing lists