From: Marek Vasut <marex@denx.de>
To: Florian Fainelli <florian.fainelli@broadcom.com>,
netdev@vger.kernel.org, Christian Marangi <ansuelsmth@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
"Andrew Lunn" <andrew@lunn.ch>,
"Broadcom internal kernel review list"
<bcm-kernel-feedback-list@broadcom.com>,
"Conor Dooley" <conor+dt@kernel.org>,
"Eric Dumazet" <edumazet@google.com>,
"Heiner Kallweit" <hkallweit1@gmail.com>,
"Jakub Kicinski" <kuba@kernel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Lee Jones" <lee@kernel.org>, "Paolo Abeni" <pabeni@redhat.com>,
"Pavel Machek" <pavel@ucw.cz>, "Rafał Miłecki" <rafal@milecki.pl>,
"Rob Herring" <robh+dt@kernel.org>,
"Russell King" <linux@armlinux.org.uk>,
devicetree@vger.kernel.org, linux-leds@vger.kernel.org
Subject: Re: [PATCH] [RFC] net: phy: broadcom: Add DT LED configuration support
Date: Tue, 23 Jan 2024 18:34:28 +0100 [thread overview]
Message-ID: <493e9362-bed1-4257-92d5-d1f87e047376@denx.de> (raw)
In-Reply-To: <1c57c364-dbe8-42f8-836c-52fad76a3f48@broadcom.com>
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.
prev parent reply other threads:[~2024-01-23 17:34 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-22 20:45 [PATCH] [RFC] net: phy: broadcom: Add DT LED configuration support Marek Vasut
2024-01-22 22:34 ` Florian Fainelli
2024-01-23 17:34 ` Marek Vasut [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=493e9362-bed1-4257-92d5-d1f87e047376@denx.de \
--to=marex@denx.de \
--cc=andrew@lunn.ch \
--cc=ansuelsmth@gmail.com \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=conor+dt@kernel.org \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=edumazet@google.com \
--cc=florian.fainelli@broadcom.com \
--cc=hkallweit1@gmail.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=kuba@kernel.org \
--cc=lee@kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=pavel@ucw.cz \
--cc=rafal@milecki.pl \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).