Message ID | 20230216013230.22978-1-ansuelsmth@gmail.com |
---|---|
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp41793wrn; Wed, 15 Feb 2023 17:37:54 -0800 (PST) X-Google-Smtp-Source: AK7set9MbJKPFjljRFWhdV2nK31ucbCCVPIguJW5paTCB8aOwgkEfqhmUKYnma0idCaIB2y8sux/ X-Received: by 2002:a17:906:9e08:b0:8b1:2dd5:d868 with SMTP id fp8-20020a1709069e0800b008b12dd5d868mr3189442ejc.55.1676511474410; Wed, 15 Feb 2023 17:37:54 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1676511474; cv=none; d=google.com; s=arc-20160816; b=P8T8rzdTj23YO6K52s1KXF+sBKg0uVJuXMppFNyzbJNuhnZWupwJccBfF4k1zraTcB W8F7c3RsOSTO4p9Itjl/4AXSrD3PaEXykZWPrjHe2nHDUEQcD/sac7c8lVfzvyl+sAZl 2iJavZYu2N0ZAUdiknvqsXpo05Y1nExi1Jnyhrb5HHoFHKQI7iKTJ0U1EFREe1ovLQ/F i+ynRDvhl44eTPpdYpwu2GHisHXhgWiogTdVafdZnYNAchtRsqoe/R2NWCprSZxwXp2E IklhAww0Ejm2o4HTt2nBrRBahItZvb9RMX9gTIjbD4n1TKMxVfD8XFg+EPw+Kgg3rXAC ifaQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:to:from:dkim-signature; bh=/1hkZg8A4EmBuVis3IHFGticAZdxNJl1p3OSUfGtWaA=; b=zTnj9raKdgZEjh5/HwzLD2rXsO7ON5kkhDAnMHViu4huK+jZtZwQCxsymHsEDNhztv OkygEzi6TkZlv65R6GkbzvvLvxCp3VuVyK/1W6b9/92gYH8CJXi2VIfaNxi8dpJ4Njx4 +ygLzbdHKBFnc5mX/tDvISyClZJ7htYDFpod+iXPruABTchQtUTUKQ+aSwd6/F0eYe3T qbNveVGok0RgShNcfDG2gRIRQCi5BJV9hppQOm9yNhzbUB1g+B7rx1r4yZflrpLQTR6o +oyKERfsuesIC8G3ghFwTTRVWJPEHlPTsUZ/6LG9SbuGiB4orjwB13v3+mPwz/Ftak2C oCGQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=dnCfzecb; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id kb7-20020a170907924700b008b14b443aaesi311549ejb.158.2023.02.15.17.37.31; Wed, 15 Feb 2023 17:37:54 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=dnCfzecb; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229731AbjBPBgW (ORCPT <rfc822;hadasmailinglist@gmail.com> + 99 others); Wed, 15 Feb 2023 20:36:22 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39338 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229539AbjBPBgS (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 15 Feb 2023 20:36:18 -0500 Received: from mail-wm1-x334.google.com (mail-wm1-x334.google.com [IPv6:2a00:1450:4864:20::334]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 86A0442DF0; Wed, 15 Feb 2023 17:36:17 -0800 (PST) Received: by mail-wm1-x334.google.com with SMTP id m16-20020a05600c3b1000b003dc4050c94aso432996wms.4; Wed, 15 Feb 2023 17:36:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:to :from:from:to:cc:subject:date:message-id:reply-to; bh=/1hkZg8A4EmBuVis3IHFGticAZdxNJl1p3OSUfGtWaA=; b=dnCfzecbg9zzd6qLie831efng+VlSP8B4sOVH29D/TCpNwuLhm/6LZHbYc1GqaQMcF rNs8XqIdqLxG1AoLljRHqWN0eIb5R/0Jjm4wqyBSzgn40eVw4xbVS9SLwBFgg7XWcgo4 EOgtwJuEKXhAWAugQEtA2zK6oJhh+AkLtXyz3ZwSEsPBilZaM0HiT6dJXoG9hwIymYN/ WKxPlSHnGIeWdBrjj6GEO5wEZ2epqnkh7J57oOMy+mLOIGEQjzEbYSD69snTFv0ZtzOY rxxRzmYfEobPDmxhjQICBIXvtCRIHhm/LouxIKcTUz0R60MTmrzkQRq0bOPF7KVum6hL /Pjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=/1hkZg8A4EmBuVis3IHFGticAZdxNJl1p3OSUfGtWaA=; b=eKc0V0JESdTkIZti8xcCk2cMc4+yVfTZGK8MzFcGhzK2QfFIKjRL1xr3r99NwzIcJj 59Z08oTWkSbAdJyeA1CH/85Wh7xVc1r29c3/plH0u4nnJou+FD+9fCHfAtywtalhXAYI MRTnPqDUDnMAWV/ETNdLsHi3BJn68UztptmfUnClWhyu3hcBzpTXVaCEuuxc8P3O8S6z Rjmg2Zw6riP//fvqKGJsr2KZvtYYaJhrBrOWKm+iwr1bJn15SUZdgxBJ/qIRzoAPyVMp PmkQj3sjPJVcv0vc3thQzH/nMdgQ5fX9Ft0f5n5c72qtYo+XvVOa0KVkvhn+Dd/hIZYy NwVA== X-Gm-Message-State: AO0yUKVypkNEOaYVIKmFdqMGa2UDkpsdflT7j+RJVvlpwV1YQLCeOOHc y7T5d/suQjdRKRuWf6KKKHY= X-Received: by 2002:a05:600c:2a08:b0:3da:fb3c:c1ab with SMTP id w8-20020a05600c2a0800b003dafb3cc1abmr3239734wme.0.1676511375775; Wed, 15 Feb 2023 17:36:15 -0800 (PST) Received: from localhost.localdomain (93-34-91-73.ip49.fastwebnet.it. [93.34.91.73]) by smtp.googlemail.com with ESMTPSA id v15-20020a05600c214f00b003e1fb31fc2bsm64189wml.37.2023.02.15.17.36.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Feb 2023 17:36:15 -0800 (PST) From: Christian Marangi <ansuelsmth@gmail.com> To: Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Andrew Lunn <andrew@lunn.ch>, 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>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King <linux@armlinux.org.uk>, Jonathan Corbet <corbet@lwn.net>, Christian Marangi <ansuelsmth@gmail.com>, "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>, Jacek Anaszewski <jacek.anaszewski@gmail.com>, John Crispin <john@phrozen.org>, linux-leds@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, linux-doc@vger.kernel.org, Tim Harvey <tharvey@gateworks.com>, Alexander Stein <alexander.stein@ew.tq-group.com>, Rasmus Villemoes <rasmus.villemoes@prevas.dk>, Bagas Sanjaya <bagasdotme@gmail.com>, Arun.Ramadoss@microchip.com Subject: [PATCH v8 00/13] Adds support for PHY LEDs with offload triggers Date: Thu, 16 Feb 2023 02:32:17 +0100 Message-Id: <20230216013230.22978-1-ansuelsmth@gmail.com> X-Mailer: git-send-email 2.38.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1757949695961448266?= X-GMAIL-MSGID: =?utf-8?q?1757949695961448266?= |
Series |
Adds support for PHY LEDs with offload triggers
|
|
Message
Christian Marangi
Feb. 16, 2023, 1:32 a.m. UTC
This is another attempt on adding this feature on LEDs, hoping this is the right time and someone finally notice this. Most of the times Switch/PHY have connected multiple LEDs that are controlled by HW based on some rules/event. Currently we lack any support for a generic way to control the HW part and normally we either never implement the feature or only add control for brightness or hw blink. This is based on Marek idea of providing some API to cled but use a different implementation that in theory should be more generilized. The current idea is: - LED driver implement 3 API (hw_control_status/start/stop). They are used to put the LED in hardware mode and to configure the various trigger. - We have hardware triggers that are used to expose to userspace the supported hardware mode and set the hardware mode on trigger activation. - We can also have triggers that both support hardware and software mode. - The LED driver will declare each supported hardware blink mode and communicate with the trigger all the supported blink modes that will be available by sysfs. - A trigger will use blink_set to configure the blink mode to active in hardware mode. - On hardware trigger activation, only the hardware mode is enabled but the blink modes are not configured. The LED driver should reset any link mode active by default. Each LED driver will have to declare explicit support for the offload trigger (or return not supported error code) as we the trigger_data that the LED driver will elaborate and understand what is referring to (based on the current active trigger). I posted a user for this new implementation that will benefit from this and will add a big feature to it. Currently qca8k can have up to 3 LEDs connected to each PHY port and we have some device that have only one of them connected and the default configuration won't work for that. The netdev trigger is expanded and it does now support hardware only triggers. The idea is to use hardware mode when a device_name is not defined. An additional sysfs entry is added to give some info about the available trigger modes supported in the current configuration. It was reported that at least 3 other switch family would benefits by this as they all lack support for a generic way to setup their leds and netdev team NACK each try to add special code to support LEDs present on switch in favor of a generic solution. v8: - Improve the documentation of the new feature - Rename to a more symbolic name - Fix some bug in netdev trigger (not using BIT()) - Add more define for qca8k-leds driver - Add activity led mode - Drop interval support - Move qca8k brightness set to blocking variant (can sleep while setting the mode) - More some function out of config define and provide variant if not selected - Fix many bugs in the validate option in the netdev trigger - Add phy generic schema for leds support - Add additional required Documentation changes v7: - Rebase on top of net-next (for qca8k changes) - Fix some typo in commit description - Fix qca8k leds documentation warning - Remove RFC tag v6: - Back to RFC. - Drop additional trigger - Rework netdev trigger to support common modes used by switch and hardware only triggers - Refresh qca8k leds logic and driver v5: - Move out of RFC. (no comments from Andrew this is the right path?) - Fix more spelling mistake (thx Randy) - Fix error reported by kernel test bot - Drop the additional HW_CONTROL flag. It does simplify CONFIG handling and hw control should be available anyway to support triggers as module. v4: - Rework implementation and drop hw_configure logic. We now expand blink_set. - Address even more spelling mistake. (thx a lot Randy) - Drop blink option and use blink_set delay. - Rework phy-activity trigger to actually make the groups dynamic. v3: - Rework start/stop as Andrew asked. - Introduce more logic to permit a trigger to run in hardware mode. - Add additional patch with netdev hardware support. - Use test_bit API to check flag passed to hw_control_configure. - Added a new cmd to hw_control_configure to reset any active blink_mode. - Refactor all the patches to follow this new implementation. v2: - Fix spelling mistake (sorry) - Drop patch 02 "permit to declare supported offload triggers". Change the logic, now the LED driver declare support for them using the configure_offload with the cmd TRIGGER_SUPPORTED. - Rework code to follow this new implementation. - Update Documentation to better describe how this offload implementation work. Christian Marangi (13): leds: add support for hardware driven LEDs leds: add function to configure hardware controlled LED leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode leds: trigger: netdev: rename and expose NETDEV trigger enum modes leds: trigger: netdev: convert device attr to macro leds: trigger: netdev: add hardware control support leds: trigger: netdev: use mutex instead of spinlocks leds: trigger: netdev: add available mode sysfs attr leds: trigger: netdev: add additional hardware only triggers net: dsa: qca8k: add LEDs support dt-bindings: leds: Document netdev trigger dt-bindings: net: phy: Document support for leds node dt-bindings: net: dsa: qca8k: add LEDs definition example .../devicetree/bindings/leds/common.yaml | 2 + .../devicetree/bindings/net/dsa/qca8k.yaml | 24 + .../devicetree/bindings/net/ethernet-phy.yaml | 22 + Documentation/leds/leds-class.rst | 94 ++++ drivers/leds/Kconfig | 11 + drivers/leds/led-class.c | 27 ++ drivers/leds/led-triggers.c | 38 ++ drivers/leds/trigger/ledtrig-netdev.c | 414 ++++++++++++----- drivers/net/dsa/qca/Kconfig | 9 + drivers/net/dsa/qca/Makefile | 1 + drivers/net/dsa/qca/qca8k-8xxx.c | 4 + drivers/net/dsa/qca/qca8k-leds.c | 419 ++++++++++++++++++ drivers/net/dsa/qca/qca8k.h | 69 +++ include/linux/leds.h | 95 +++- 14 files changed, 1126 insertions(+), 103 deletions(-) create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
Comments
On Fri, Feb 17, 2023 at 03:30:13PM +0100, Andrew Lunn wrote: > On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote: > > This is another attempt on adding this feature on LEDs, hoping this is > > the right time and someone finally notice this. > > Hi Christian > > Thanks for keeping working on this. > > I want to review it, and maybe implement LED support in a PHY > driver. But i'm busy with reworking EEE at the moment. > > The merge window is about to open, so patches are not going to be > accepted for the next two weeks. So i will take a look within that > time and give you feedback. > Sure take your time happy to discuss any improvement to this.
On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote: > This is another attempt on adding this feature on LEDs, hoping this is > the right time and someone finally notice this. Hi Christian Thanks for keeping working on this. I want to review it, and maybe implement LED support in a PHY driver. But i'm busy with reworking EEE at the moment. The merge window is about to open, so patches are not going to be accepted for the next two weeks. So i will take a look within that time and give you feedback. Andrew
On Fri, 17 Feb 2023, Andrew Lunn wrote: > On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote: > > This is another attempt on adding this feature on LEDs, hoping this is > > the right time and someone finally notice this. > > Hi Christian > > Thanks for keeping working on this. > > I want to review it, and maybe implement LED support in a PHY > driver. But i'm busy with reworking EEE at the moment. > > The merge window is about to open, so patches are not going to be > accepted for the next two weeks. So i will take a look within that > time and give you feedback. Thanks Andrew. If Pavel is still unavailable to conduct reviews, I'm going to need all the help I can get with complex submissions such as these.
On Wed, Feb 22, 2023 at 03:02:04PM +0000, Lee Jones wrote: > On Fri, 17 Feb 2023, Andrew Lunn wrote: > > > On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote: > > > This is another attempt on adding this feature on LEDs, hoping this is > > > the right time and someone finally notice this. > > > > Hi Christian > > > > Thanks for keeping working on this. > > > > I want to review it, and maybe implement LED support in a PHY > > driver. But i'm busy with reworking EEE at the moment. > > > > The merge window is about to open, so patches are not going to be > > accepted for the next two weeks. So i will take a look within that > > time and give you feedback. > > Thanks Andrew. If Pavel is still unavailable to conduct reviews, I'm > going to need all the help I can get with complex submissions such as > these. > Hi Lee, thanks for stepping in. Just wanted to tell you I got some message with Andrew to make this thing less problematic and to dry/make it more review friendly. We decided on pushing this in 3 step: 1. Propose most basic things for some switch and some PHY. (brightness and blink_set support only, already supported by LED core) 2. A small series that should be just a cleanup for the netdev trigger 3. Support for hw_control in the most possible clean and way with small patch to they are not hard to track and understand the concept of this feature. I'm starting with the step 1 and sending some of my patch and Andrew patch to add basic support and I will add you and LED mailing list in Cc. Again thanks for starting checking this and feel free to ask any question about this to me also privately, I'm very open to any help.
On Mon, 06 Mar 2023, Christian Marangi wrote: > On Wed, Feb 22, 2023 at 03:02:04PM +0000, Lee Jones wrote: > > On Fri, 17 Feb 2023, Andrew Lunn wrote: > > > > > On Thu, Feb 16, 2023 at 02:32:17AM +0100, Christian Marangi wrote: > > > > This is another attempt on adding this feature on LEDs, hoping this is > > > > the right time and someone finally notice this. > > > > > > Hi Christian > > > > > > Thanks for keeping working on this. > > > > > > I want to review it, and maybe implement LED support in a PHY > > > driver. But i'm busy with reworking EEE at the moment. > > > > > > The merge window is about to open, so patches are not going to be > > > accepted for the next two weeks. So i will take a look within that > > > time and give you feedback. > > > > Thanks Andrew. If Pavel is still unavailable to conduct reviews, I'm > > going to need all the help I can get with complex submissions such as > > these. > > > > Hi Lee, > thanks for stepping in. Just wanted to tell you I got some message with > Andrew to make this thing less problematic and to dry/make it more > review friendly. > > We decided on pushing this in 3 step: > 1. Propose most basic things for some switch and some PHY. (brightness > and blink_set support only, already supported by LED core) > 2. A small series that should be just a cleanup for the netdev trigger > 3. Support for hw_control in the most possible clean and way with small > patch to they are not hard to track and understand the concept of this > feature. > > I'm starting with the step 1 and sending some of my patch and Andrew > patch to add basic support and I will add you and LED mailing list in > Cc. Sounds like a plan. Thank you both. > Again thanks for starting checking this and feel free to ask any > question about this to me also privately, I'm very open to any help. -- Lee Jones [李琼斯]
Hi Christian, thanks for your patch! On Thu, Feb 16, 2023 at 2:36 AM Christian Marangi <ansuelsmth@gmail.com> wrote: > The current idea is: > - LED driver implement 3 API (hw_control_status/start/stop). > They are used to put the LED in hardware mode and to configure the > various trigger. > - We have hardware triggers that are used to expose to userspace the > supported hardware mode and set the hardware mode on trigger > activation. > - We can also have triggers that both support hardware and software mode. > - The LED driver will declare each supported hardware blink mode and > communicate with the trigger all the supported blink modes that will > be available by sysfs. > - A trigger will use blink_set to configure the blink mode to active > in hardware mode. > - On hardware trigger activation, only the hardware mode is enabled but > the blink modes are not configured. The LED driver should reset any > link mode active by default. The series looks good as a start. There are some drivers and HW definitions etc for switch-controlled LEDs, which is great. I am a bit reluctant on the ambition to rely on configuration from sysfs for the triggers, and I am also puzzled to how a certain trigger on a certain LED is going to associate itself with, say, a certain port. I want to draw your attention to this recently merged patch series from Hans de Goede: https://lore.kernel.org/linux-leds/20230120114524.408368-1-hdegoede@redhat.com/ This adds the devm_led_get() API which works similar to getting regulators, clocks, GPIOs or any other resources. It is not yet (I think) hooked into the device tree framework, but it supports software nodes so adding DT handling should be sort of trivial. I think the ambition should be something like this (conjured example) for a DSA switch: platform { switch { compatible = "foo"; leds { #address-cells = <1>; #size-cells = <0>; led0: led@0 { reg = <0>; color =... function = ... function-enumerator = ... default-state = ... }; led1: led@1 { reg = <1>; color =... function = ... function-enumerator = ... default-state = ... }; }; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; label = "lan0"; phy-handle = <&phy0>; leds = <&led0>; }; port@1 { reg = <1>; label = "lan1"; phy-handle = <&phy1>; leds = <&led0>; }; }; mdio { compatible = "foo-mdio"; #address-cells = <1>; #size-cells = <0>; phy0: ethernet-phy@0 { reg = <0>; }; phy1: ethernet-phy@1 { reg = <1>; }; }; }; }; I am not the man to tell whether the leds = <&led0>; phandle should be on the port or actually on the phy, it may even vary. You guys know the answer to this. But certainly something like this resource phandle will be necessary to assign the right LED to the right port or phy, I hope you were not going to rely on strings and naming conventions? Yours, Linus Walleij
Hi, On 3/9/23 10:09, Linus Walleij wrote: > Hi Christian, > > thanks for your patch! > > On Thu, Feb 16, 2023 at 2:36 AM Christian Marangi <ansuelsmth@gmail.com> wrote: > >> The current idea is: >> - LED driver implement 3 API (hw_control_status/start/stop). >> They are used to put the LED in hardware mode and to configure the >> various trigger. >> - We have hardware triggers that are used to expose to userspace the >> supported hardware mode and set the hardware mode on trigger >> activation. >> - We can also have triggers that both support hardware and software mode. >> - The LED driver will declare each supported hardware blink mode and >> communicate with the trigger all the supported blink modes that will >> be available by sysfs. >> - A trigger will use blink_set to configure the blink mode to active >> in hardware mode. >> - On hardware trigger activation, only the hardware mode is enabled but >> the blink modes are not configured. The LED driver should reset any >> link mode active by default. > > The series looks good as a start. > There are some drivers and HW definitions etc for switch-controlled > LEDs, which is great. > > I am a bit reluctant on the ambition to rely on configuration from sysfs > for the triggers, and I am also puzzled to how a certain trigger on a > certain LED is going to associate itself with, say, a certain port. > > I want to draw your attention to this recently merged patch series > from Hans de Goede: > https://lore.kernel.org/linux-leds/20230120114524.408368-1-hdegoede@redhat.com/ > > This adds the devm_led_get() API which works similar to getting > regulators, clocks, GPIOs or any other resources. > > It is not yet (I think) hooked into the device tree framework, but it > supports software nodes so adding DT handling should be sort of > trivial. That series contains this (unmerged) patch to hookup DT handling: https://lore.kernel.org/linux-leds/20230120114524.408368-6-hdegoede@redhat.com/ this was not merged because there are no current users, but adding support is as easy as picking up that patch :) Note there also already is a devicetree *only*: struct led_classdev *of_led_get(struct device_node *np, int index); Since I was working on a x86/ACPI platform I needed something more generic though and ideally new code would use the generic approach. Regards, Hans > > I think the ambition should be something like this (conjured example) > for a DSA switch: > > platform { > switch { > compatible = "foo"; > > leds { > #address-cells = <1>; > #size-cells = <0>; > led0: led@0 { > reg = <0>; > color =... > function = ... > function-enumerator = ... > default-state = ... > }; > led1: led@1 { > reg = <1>; > color =... > function = ... > function-enumerator = ... > default-state = ... > }; > }; > > ports { > #address-cells = <1>; > #size-cells = <0>; > port@0 { > reg = <0>; > label = "lan0"; > phy-handle = <&phy0>; > leds = <&led0>; > }; > port@1 { > reg = <1>; > label = "lan1"; > phy-handle = <&phy1>; > leds = <&led0>; > }; > }; > > mdio { > compatible = "foo-mdio"; > #address-cells = <1>; > #size-cells = <0>; > > phy0: ethernet-phy@0 { > reg = <0>; > }; > phy1: ethernet-phy@1 { > reg = <1>; > }; > }; > }; > }; > > I am not the man to tell whether the leds = <&led0>; phandle should be on > the port or actually on the phy, it may even vary. You guys know the answer > to this. > > But certainly something like this resource phandle will be necessary to > assign the right LED to the right port or phy, I hope you were not going > to rely on strings and naming conventions? > > Yours, > Linus Walleij >
> I am a bit reluctant on the ambition to rely on configuration from sysfs > for the triggers, and I am also puzzled to how a certain trigger on a > certain LED is going to associate itself with, say, a certain port. Hi Linus There will need to be a device tree binding for the default trigger. That is what nearly all the rejected hacks so far have been about, write registers in the PHYs LEDs registers to put it into a specific mode. I don't see that part of the overall problem too problematic, apart from the old issue, is it describing configuration, not hardware. As to 'how a certain trigger on a certain LED is going to associate itself with, say, a certain port' is clearly a property of the hardware, when offloading is supported. I've not seen a switch you can arbitrarily assign LEDs to ports. The Marvell switches have the LED registers within the port registers, for example, two LEDs per port. > > I want to draw your attention to this recently merged patch series > from Hans de Goede: > https://lore.kernel.org/linux-leds/20230120114524.408368-1-hdegoede@redhat.com/ > > This adds the devm_led_get() API which works similar to getting > regulators, clocks, GPIOs or any other resources. Interesting. Thanks for pointing this out. But i don't think it is of interest in our use case, which is hardware offload. For a purely software controlled LED, where the LED could be anywhere, devm_led_get() makes sense. But in our case, the LED is in a well defined place, either the MAC or the PHY, where it has access to the RX and TX packets, link status etc. So we don't have the problem of finding it in an arbitrary location. There is also one additional problem we have, both for MAC and PHY LEDs. The trigger is ledtrig-netdev. All trigger state revolves around a netdev. A DSA port is not a netdev. A PHY is not a netdev. Each of these three things have there own life cycle. Often, a PHY does not know what netdev it is associated to until the interface is opened, ie. ip link set eth0 up. But once it is associated, phylib knows this information, so can return it, without any additional configuration data in DT. A DSA switch port can be created before the netdev associated to it is created. But again, the DSA core does know the mapping between a netdev and a port. Using devm_led_get() does not gain us anything. Andrew
On Thu, Mar 9, 2023 at 4:22 PM Andrew Lunn <andrew@lunn.ch> wrote: > As to 'how a certain trigger on a certain LED is going to associate > itself with, say, a certain port' is clearly a property of the > hardware, when offloading is supported. I've not seen a switch you can > arbitrarily assign LEDs to ports. The Marvell switches have the LED > registers within the port registers, for example, two LEDs per port. Aha so there is an implicit HW dependency between the port and the LED, that we just cannot see in the device tree. Okay, it makes sense. I think there will be a day when a switch without LED controller appears, but the system has a few LEDs for the ports connected to an arbitrary GPIO controller, and then we will need this. But we have not seen that yet :) Yours, Linus Walleij
On Fri, Mar 10, 2023 at 10:37:25AM +0100, Linus Walleij wrote: > On Thu, Mar 9, 2023 at 4:22 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > As to 'how a certain trigger on a certain LED is going to associate > > itself with, say, a certain port' is clearly a property of the > > hardware, when offloading is supported. I've not seen a switch you can > > arbitrarily assign LEDs to ports. The Marvell switches have the LED > > registers within the port registers, for example, two LEDs per port. > > Aha so there is an implicit HW dependency between the port and the > LED, that we just cannot see in the device tree. Okay, it makes sense. Well, i would say the dependency is in the device tree, in that the LEDs are described in the ports, not as a block of their own at a higher level within the switch. And in some switches, they might actually be a block of registers in there own space, rather than in the port registers. But i still expect there is a fixed mapping between LED and port. > I think there will be a day when a switch without LED controller appears, > but the system has a few LEDs for the ports connected to an > arbitrary GPIO controller, and then we will need this. But we have > not seen that yet :) The microchip sparx5 might be going in that direction. It has what looks like a reasonably generic sgpio controller: drivers/pinctrl/pinctrl-microchip-sgpio.c But this not just about switches. It is also plain NICs. And using ledtrig-netdev, you could make your keyboard LED blink based on network traffic etc. So yes, using a phandle to an LED could very well be useful in the future. Andrew
> > I think there will be a day when a switch without LED controller appears, > > but the system has a few LEDs for the ports connected to an > > arbitrary GPIO controller, and then we will need this. But we have > > not seen that yet :) > > The microchip sparx5 might be going in that direction. It has what > looks like a reasonably generic sgpio controller: > drivers/pinctrl/pinctrl-microchip-sgpio.c That gpio controller supports both, some kind of hardware controlled and pure software controlled mode. AFAIK the driver only supports software controlled mode (yet?). In any case, our board (arch/arm/boot/dts/lan966x-kontron-kswitch-d10-mmt.dtsi) is broken in a way that we are forced to use the software controlled mode anyway. Therefore, there is already such a board ;) -michael