[PATCH net-next + leds v2 0/7] PLEASE REVIEW: Add support for LEDs on Marvell PHYs

[PATCH net-next + leds v2 0/7] PLEASE REVIEW: Add support for LEDs on Marvell PHYs

[Marek Behún]

Hello Andrew and Pavel,

please review these patches adding support for HW controlled LEDs.
The main difference from previous version is that the API is now generalized
and lives in drivers/leds, so that part needs to be reviewed (and maybe even
applied) by Pavel.

As discussed previously between you two, I made it so that the devicename
part of the LED is now in the form `ethernet-phy%i` when the LED is probed
for an ethernet PHY. Userspace utility wanting to control LEDs for a specific
network interface can access them via /sys/class/net/eth0/phydev/leds:

  mox ~ # ls /sys/class/net/eth0/phydev/leds
  ethernet-phy0:green:status  ethernet-phy0:yellow:activity

  mox ~ # ls /sys/class/net/eth0/phydev/leds/ethernet-phy0:green:status
  brightness  device  hw_mode  max_brightness  power  subsystem  trigger  uevent

  mox ~ # cat /sys/class/net/eth0/phydev/leds/ethernet-phy0:green:status/trigger
  none [dev-hw-mode] timer oneshot heartbeat default-on mmc0 mmc1

  mox ~ # cat /sys/class/net/eth0/phydev/leds/ethernet-phy0:green:status/hw_mode
  link link/act [1Gbps/100Mbps/10Mbps] act blink-act tx copper 1Gbps blink

Thank you.

Marek

PS: After this series is applied, we can update some device trees of various
devices which use the `marvell,reg-init` property to initialize LEDs into
specific modes so that instead of using `marvell,reg-init` they can register
LEDs via this subsystem. The `marvell,reg-init` property does not comply with
the idea that the device tree should only describe how devices are connected
to each other on the board. Maybe this property could then be proclaimed as
legacy and a warning could be printed if it is used.

Changes since v1:
- the HW controlled LEDs API is now generalized (so not only for ethernet
  PHYs), and lives in drivers/leds/leds-hw-controlled.c.
  I did this because I am thinking forward for when I'll be adding support
  for LEDs connected to Marvell ethernet switches (mv88e6xxx driver).
  The LEDs there should be described as descendants of the `port` nodes, not
  `phy` nodes, because:
    - some ports don't have PHYs and yet can have LEDs
    - some ports have SERDES PHYs which are currently not described in any
      way in device-tree
    - some LEDs can be made to blink on activity/other event on multiple
      ports at once
- hence the private LED trigger was renamed from `phydev-hw-mode` to
  `dev-hw-mode`
- the `led-open-drain` DT property was renamed to `led-tristate` property,
  because I learned that the two things mean something different and in
  Marvell PHYs the polarity on off state can be put into tristate mode, not
  open drain mode
- the devicename part of PHY LEDs is now in the format `ethernet-phy%i`,
  instead of `phy%i`
- the code adding `phyindex` member to struct phy_device is now in separate
  patch
- YAML device-tree binding schema for HW controlled LEDs now lives in it's
  own file and the ethernet-phy.yaml file now contains a reference to the
  this schema
- added a patch adding nodes for PHY controlled LEDs for Turris MOX' device
  tree

Changes since RFC v4:
- added device-tree binding documentation.
- the OF code now checks for linux,default-hw-mode property so that
  default HW mode can be set in device tree (like linux,default-trigger)
  (this was suggested by Andrew)
- the OF code also checks for enable-active-high and led-open-drain
  properties, and the marvell PHY driver now understands uses these
  settings when initializing the LEDs
- the LED operations were moved to their own struct phy_device_led_ops
- a new member was added into struct phy_device: phyindex. This is an
  incrementing integer, new for each registered phy_device. This is used
  for a simple naming scheme for the devicename part of a LED, as was
  suggested in a discussion by Andrew and Pavel. A PHY controlled LED
  now has a name in form:
    phy%i:color:function
  When a PHY is attached to a netdevice, userspace can control available
  PHY controlled LEDs via /sys/class/net/<ifname>/phydev/leds/
- legacy LED configuration in Marvell PHY driver (in function
  marvell_config_led) now writes only to registers which do not
  correspond to any registered LED

Changes since RFC v3:
- addressed some of Andrew's suggestions
- phy_hw_led_mode.c renamed to phy_led.c
- the DT reading code is now also generic, moved to phy_led.c and called
  from phy_probe
- the function registering the phydev-hw-mode trigger is now called from
  phy_device.c function phy_init before registering genphy drivers
- PHY LED functionality now depends on CONFIG_LEDS_TRIGGERS

Changes since RFC v2:
- to share code with other drivers which may want to also offer PHY HW
  control of LEDs some of the code was refactored and now resides in
  phy_hw_led_mode.c. This code is compiled in when config option
  LED_TRIGGER_PHY_HW is enabled. Drivers wanting to offer PHY HW control
  of LEDs should depend on this option.
- the "hw-control" trigger is renamed to "phydev-hw-mode" and is
  registered by the code in phy_hw_led_mode.c
- the "hw_control" sysfs file is renamed to "hw_mode"
- struct phy_driver is extended by three methods to support PHY HW LED
  control
- I renamed the various HW control modes offeret by Marvell PHYs to
  conform to other Linux mode names, for example the "1000/100/10/else"
  mode was renamed to "1Gbps/100Mbps/10Mbps", or "recv/else" was renamed
  to "rx" (this is the name of the mode in netdev trigger).

Marek Behún (7):
  dt-bindings: leds: document binding for HW controlled LEDs
  leds: add generic API for LEDs that can be controlled by hardware
  net: phy: add simple incrementing phyindex member to phy_device struct
  dt-bindings: net: ethernet-phy: add description for PHY LEDs
  net: phy: add support for LEDs controlled by ethernet PHY chips
  net: phy: marvell: add support for LEDs controlled by Marvell PHYs
  arm64: dts: armada-3720-turris-mox: add nodes for ethernet PHY LEDs

 .../sysfs-class-led-trigger-dev-hw-mode       |   8 +
 .../leds/linux,hw-controlled-leds.yaml        |  99 ++++++
 .../devicetree/bindings/net/ethernet-phy.yaml |   8 +
 .../dts/marvell/armada-3720-turris-mox.dts    |  23 ++
 drivers/leds/Kconfig                          |  10 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-hw-controlled.c             | 227 +++++++++++++
 drivers/net/phy/marvell.c                     | 314 +++++++++++++++++-
 drivers/net/phy/phy_device.c                  | 106 ++++++
 include/linux/leds-hw-controlled.h            |  74 +++++
 include/linux/phy.h                           |   7 +
 11 files changed, 875 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
 create mode 100644 Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
 create mode 100644 drivers/leds/leds-hw-controlled.c
 create mode 100644 include/linux/leds-hw-controlled.h

-- 
2.26.2

[PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs

[Marek Behún]

Document binding for LEDs connected to and controlled by various chips
(such as ethernet PHY chips).

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 .../leds/linux,hw-controlled-leds.yaml        | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml

diff --git a/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
new file mode 100644
index 0000000000000..eaf6e5d80c5f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/linux,hw-controlled-leds.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
+
+maintainers:
+  - Marek Behún <marek.behun@nic.cz>
+
+description:
+  Many an ethernet PHY (and other chips) supports various HW control modes
+  for LEDs connected directly to them. With this binding such LEDs can be
+  described.
+
+properties:
+  compatible:
+    const: linux,hw-controlled-leds
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[0-9a-f]+$":
+    type: object
+    allOf:
+      - $ref: common.yaml#
+    description:
+      This node represents a LED device connected to a chip that can control
+      the LED in various HW controlled modes.
+
+    properties:
+      reg:
+        maxItems: 1
+        description:
+          This property identifies the LED to the chip the LED is connected to
+          (eg. an ethernet PHY chip can have multiple LEDs connected to it).
+
+      enable-active-high:
+        description:
+          Polarity of LED is active high. If missing, assumed default is active
+          low.
+        type: boolean
+
+      led-tristate:
+        description:
+          LED pin is tristate type. If missing, assumed false.
+        type: boolean
+
+      linux,default-hw-mode:
+        description:
+          This parameter, if present, specifies the default HW triggering mode
+          of the LED when LED trigger is set to `dev-hw-mode`.
+          Available values are specific per device the LED is connected to and
+          per LED itself.
+        $ref: /schemas/types.yaml#definitions/string
+
+    required:
+      - reg
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/leds/common.h>
+
+    ethernet-phy@0 {
+        compatible = "ethernet-phy-ieee802.3-c45";
+        reg = <0>;
+
+        leds {
+            compatible = "linux,hw-controlled-leds";
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            led@0 {
+                reg = <0>;
+                color = <LED_COLOR_ID_GREEN>;
+                function = <LED_FUNCTION_STATUS>;
+                linux,default-trigger = "dev-hw-mode";
+                linux,default-hw-mode = "1Gbps";
+            };
+
+            led@1 {
+                reg = <1>;
+                color = <LED_COLOR_ID_YELLOW>;
+                function = <LED_FUNCTION_ACTIVITY>;
+                linux,default-trigger = "dev-hw-mode";
+                linux,default-hw-mode = "activity";
+            };
+        };
+    };
+
+...
-- 
2.26.2

Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs

[Andrew Lunn]

On Wed, Sep 09, 2020 at 06:25:46PM +0200, Marek Behún wrote:
> Document binding for LEDs connected to and controlled by various chips
> (such as ethernet PHY chips).
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  .../leds/linux,hw-controlled-leds.yaml        | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> new file mode 100644
> index 0000000000000..eaf6e5d80c5f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/linux,hw-controlled-leds.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
> +
> +maintainers:
> +  - Marek Behún <marek.behun@nic.cz>
> +
> +description:
> +  Many an ethernet PHY (and other chips) supports various HW control modes
> +  for LEDs connected directly to them. With this binding such LEDs can be
> +  described.
> +
> +properties:
> +  compatible:
> +    const: linux,hw-controlled-leds
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^led@[0-9a-f]+$":
> +    type: object
> +    allOf:
> +      - $ref: common.yaml#
> +    description:
> +      This node represents a LED device connected to a chip that can control
> +      the LED in various HW controlled modes.
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +        description:
> +          This property identifies the LED to the chip the LED is connected to
> +          (eg. an ethernet PHY chip can have multiple LEDs connected to it).
> +
> +      enable-active-high:
> +        description:
> +          Polarity of LED is active high. If missing, assumed default is active
> +          low.
> +        type: boolean
> +
> +      led-tristate:
> +        description:
> +          LED pin is tristate type. If missing, assumed false.
> +        type: boolean
> +
> +      linux,default-hw-mode:
> +        description:
> +          This parameter, if present, specifies the default HW triggering mode
> +          of the LED when LED trigger is set to `dev-hw-mode`.
> +          Available values are specific per device the LED is connected to and
> +          per LED itself.
> +        $ref: /schemas/types.yaml#definitions/string
> +
> +    required:
> +      - reg

My Yaml foo is not very good. Do you need to list colour, function and
linux,default-trigger, or do they automagically get included from the
generic LED binding?

	Andrew

Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs

[Marek Behún]

On Wed, 9 Sep 2020 20:27:30 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, Sep 09, 2020 at 06:25:46PM +0200, Marek Behún wrote:
> > Document binding for LEDs connected to and controlled by various
> > chips (such as ethernet PHY chips).
> > 
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > ---
> >  .../leds/linux,hw-controlled-leds.yaml        | 99
> > +++++++++++++++++++ 1 file changed, 99 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > new file mode 100644 index 0000000000000..eaf6e5d80c5f5 ---
> > /dev/null +++
> > b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > @@ -0,0 +1,99 @@ +# SPDX-License-Identifier: GPL-2.0-only OR
> > BSD-2-Clause +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/leds/linux,hw-controlled-leds.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml# +
> > +title: LEDs that can be controlled by hardware (eg. by an ethernet
> > PHY chip) +
> > +maintainers:
> > +  - Marek Behún <marek.behun@nic.cz>
> > +
> > +description:
> > +  Many an ethernet PHY (and other chips) supports various HW
> > control modes
> > +  for LEDs connected directly to them. With this binding such LEDs
> > can be
> > +  described.
> > +
> > +properties:
> > +  compatible:
> > +    const: linux,hw-controlled-leds
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^led@[0-9a-f]+$":
> > +    type: object
> > +    allOf:
> > +      - $ref: common.yaml#
> > +    description:
> > +      This node represents a LED device connected to a chip that
> > can control
> > +      the LED in various HW controlled modes.
> > +
> > +    properties:
> > +      reg:
> > +        maxItems: 1
> > +        description:
> > +          This property identifies the LED to the chip the LED is
> > connected to
> > +          (eg. an ethernet PHY chip can have multiple LEDs
> > connected to it). +
> > +      enable-active-high:
> > +        description:
> > +          Polarity of LED is active high. If missing, assumed
> > default is active
> > +          low.
> > +        type: boolean
> > +
> > +      led-tristate:
> > +        description:
> > +          LED pin is tristate type. If missing, assumed false.
> > +        type: boolean
> > +
> > +      linux,default-hw-mode:
> > +        description:
> > +          This parameter, if present, specifies the default HW
> > triggering mode
> > +          of the LED when LED trigger is set to `dev-hw-mode`.
> > +          Available values are specific per device the LED is
> > connected to and
> > +          per LED itself.
> > +        $ref: /schemas/types.yaml#definitions/string
> > +
> > +    required:
> > +      - reg  
> 
> My Yaml foo is not very good. Do you need to list colour, function and
> linux,default-trigger, or do they automagically get included from the
> generic LED binding?
> 
> 	Andrew

I don't know :) I copied this from other drivers, I once tried setting
up environment for doing checking of device trees with YAML schemas,
and it was a little painful :)

Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs

[Rob Herring]

On Wed, Sep 09, 2020 at 08:33:10PM +0200, Marek Behún wrote:
> On Wed, 9 Sep 2020 20:27:30 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Wed, Sep 09, 2020 at 06:25:46PM +0200, Marek Behún wrote:
> > > Document binding for LEDs connected to and controlled by various
> > > chips (such as ethernet PHY chips).
> > > 
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: devicetree@vger.kernel.org
> > > ---
> > >  .../leds/linux,hw-controlled-leds.yaml        | 99
> > > +++++++++++++++++++ 1 file changed, 99 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > > b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > > new file mode 100644 index 0000000000000..eaf6e5d80c5f5 ---
> > > /dev/null +++
> > > b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > > @@ -0,0 +1,99 @@ +# SPDX-License-Identifier: GPL-2.0-only OR
> > > BSD-2-Clause +%YAML 1.2
> > > +---
> > > +$id:
> > > http://devicetree.org/schemas/leds/linux,hw-controlled-leds.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml# +
> > > +title: LEDs that can be controlled by hardware (eg. by an ethernet
> > > PHY chip) +
> > > +maintainers:
> > > +  - Marek Behún <marek.behun@nic.cz>
> > > +
> > > +description:
> > > +  Many an ethernet PHY (and other chips) supports various HW
> > > control modes
> > > +  for LEDs connected directly to them. With this binding such LEDs
> > > can be
> > > +  described.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: linux,hw-controlled-leds
> > > +
> > > +  "#address-cells":
> > > +    const: 1
> > > +
> > > +  "#size-cells":
> > > +    const: 0
> > > +
> > > +patternProperties:
> > > +  "^led@[0-9a-f]+$":
> > > +    type: object
> > > +    allOf:
> > > +      - $ref: common.yaml#
> > > +    description:
> > > +      This node represents a LED device connected to a chip that
> > > can control
> > > +      the LED in various HW controlled modes.
> > > +
> > > +    properties:
> > > +      reg:
> > > +        maxItems: 1
> > > +        description:
> > > +          This property identifies the LED to the chip the LED is
> > > connected to
> > > +          (eg. an ethernet PHY chip can have multiple LEDs
> > > connected to it). +
> > > +      enable-active-high:
> > > +        description:
> > > +          Polarity of LED is active high. If missing, assumed
> > > default is active
> > > +          low.
> > > +        type: boolean
> > > +
> > > +      led-tristate:
> > > +        description:
> > > +          LED pin is tristate type. If missing, assumed false.
> > > +        type: boolean
> > > +
> > > +      linux,default-hw-mode:
> > > +        description:
> > > +          This parameter, if present, specifies the default HW
> > > triggering mode
> > > +          of the LED when LED trigger is set to `dev-hw-mode`.
> > > +          Available values are specific per device the LED is
> > > connected to and
> > > +          per LED itself.
> > > +        $ref: /schemas/types.yaml#definitions/string
> > > +
> > > +    required:
> > > +      - reg  
> > 
> > My Yaml foo is not very good. Do you need to list colour, function and
> > linux,default-trigger, or do they automagically get included from the
> > generic LED binding?

They do with the '$ref: common.yaml#'. You need to list them if you want 
to say which properties of a common schema you are using (IMO, you 
should, but that's a secondary concern) and/or you have additional 
constraints on them.

> > 
> > 	Andrew
> 
> I don't know :) I copied this from other drivers, I once tried setting
> up environment for doing checking of device trees with YAML schemas,
> and it was a little painful :)

pip3 install dtschema ?

Can you elaborate on the issue.

Rob

Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs

[Marek Behun]

On Wed, 9 Sep 2020 14:59:23 -0600
Rob Herring <robh@kernel.org> wrote:

> > 
> > I don't know :) I copied this from other drivers, I once tried setting
> > up environment for doing checking of device trees with YAML schemas,
> > and it was a little painful :)  
> 
> pip3 install dtschema ?
> 
> Can you elaborate on the issue.
> 
> Rob
> 

I am using Gentoo and didn't want to bloat system with non-portage
packages, nor try to start a virtual environment. In the end I did it
in a chroot Ubuntu :)

The other thing is that the make dt_binding_check executed for
quite a long time, and I didn't find a way to just do the binding check
some of the schemas.

But I am not criticizing anything, I think that it is a good thing to
have this system.

Marek

Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs

[Rob Herring]

On Wed, Sep 09, 2020 at 11:07:26PM +0200, Marek Behun wrote:
> On Wed, 9 Sep 2020 14:59:23 -0600
> Rob Herring <robh@kernel.org> wrote:
> 
> > > 
> > > I don't know :) I copied this from other drivers, I once tried setting
> > > up environment for doing checking of device trees with YAML schemas,
> > > and it was a little painful :)  
> > 
> > pip3 install dtschema ?
> > 
> > Can you elaborate on the issue.
> > 
> > Rob
> > 
> 
> I am using Gentoo and didn't want to bloat system with non-portage
> packages, nor try to start a virtual environment. In the end I did it
> in a chroot Ubuntu :)

A user install doesn't work?

I don't really care for virtual env either.

> The other thing is that the make dt_binding_check executed for
> quite a long time, and I didn't find a way to just do the binding check
> some of the schemas.

It's a bit faster now with what's queued for 5.10. The schema 
validation is under 10sec now on my laptop. For the examples, any 
new schema could apply to any example, so we have to check them all. 
It's faster too, but still minutes to run.

> But I am not criticizing anything, I think that it is a good thing to
> have this system.

Good to hear. Just want to improve any pain points if possible.

Rob

Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs

[Marek Behun]

On Wed, 9 Sep 2020 15:31:22 -0600
Rob Herring <robh@kernel.org> wrote:

> On Wed, Sep 09, 2020 at 11:07:26PM +0200, Marek Behun wrote:
> > On Wed, 9 Sep 2020 14:59:23 -0600
> > Rob Herring <robh@kernel.org> wrote:
> >   
> > > > 
> > > > I don't know :) I copied this from other drivers, I once tried setting
> > > > up environment for doing checking of device trees with YAML schemas,
> > > > and it was a little painful :)    
> > > 
> > > pip3 install dtschema ?
> > > 
> > > Can you elaborate on the issue.
> > > 
> > > Rob
> > >   
> > 
> > I am using Gentoo and didn't want to bloat system with non-portage
> > packages, nor try to start a virtual environment. In the end I did it
> > in a chroot Ubuntu :)  
> 
> A user install doesn't work?
> 
> I don't really care for virtual env either.
> 
> > The other thing is that the make dt_binding_check executed for
> > quite a long time, and I didn't find a way to just do the binding check
> > some of the schemas.  
> 
> It's a bit faster now with what's queued for 5.10. The schema 
> validation is under 10sec now on my laptop. For the examples, any 
> new schema could apply to any example, so we have to check them all. 
> It's faster too, but still minutes to run.
> 
> > But I am not criticizing anything, I think that it is a good thing to
> > have this system.  
> 
> Good to hear. Just want to improve any pain points if possible.
> 
> Rob
> 

OK I am running it now in my chroot environment :)

Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs

[Rob Herring]

On Wed, 09 Sep 2020 18:25:46 +0200, Marek Behún wrote:
> Document binding for LEDs connected to and controlled by various chips
> (such as ethernet PHY chips).
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  .../leds/linux,hw-controlled-leds.yaml        | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

Error: Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.example.dts:34.33-41 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:342: Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1366: dt_binding_check] Error 2


See https://patchwork.ozlabs.org/patch/1360778

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs

[Rob Herring]

On Wed, Sep 09, 2020 at 06:25:46PM +0200, Marek Behún wrote:
> Document binding for LEDs connected to and controlled by various chips
> (such as ethernet PHY chips).

If they are h/w controlled, then why are they in DT?

> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  .../leds/linux,hw-controlled-leds.yaml        | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> new file mode 100644
> index 0000000000000..eaf6e5d80c5f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/linux,hw-controlled-leds.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
> +
> +maintainers:
> +  - Marek Behún <marek.behun@nic.cz>
> +
> +description:
> +  Many an ethernet PHY (and other chips) supports various HW control modes
> +  for LEDs connected directly to them. With this binding such LEDs can be
> +  described.
> +
> +properties:
> +  compatible:
> +    const: linux,hw-controlled-leds

What makes this linux specific?

Unless you're going to make this h/w specific, then it probably should 
just be dropped. 

The phy schema will need:

leds:
  type: object
  $ref: /schemas/leds/hw-controlled-leds.yaml#

> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^led@[0-9a-f]+$":
> +    type: object
> +    allOf:
> +      - $ref: common.yaml#
> +    description:
> +      This node represents a LED device connected to a chip that can control
> +      the LED in various HW controlled modes.
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +        description:
> +          This property identifies the LED to the chip the LED is connected to
> +          (eg. an ethernet PHY chip can have multiple LEDs connected to it).
> +
> +      enable-active-high:
> +        description:
> +          Polarity of LED is active high. If missing, assumed default is active
> +          low.
> +        type: boolean
> +
> +      led-tristate:
> +        description:
> +          LED pin is tristate type. If missing, assumed false.
> +        type: boolean
> +
> +      linux,default-hw-mode:

How is this linux specific? It sounds device specific. Your choice is 
make this a device specific property with device specific values or come 
up with generic modes.

Perhaps 'function' should be expanded.

> +        description:
> +          This parameter, if present, specifies the default HW triggering mode
> +          of the LED when LED trigger is set to `dev-hw-mode`.
> +          Available values are specific per device the LED is connected to and
> +          per LED itself.
> +        $ref: /schemas/types.yaml#definitions/string
> +
> +    required:
> +      - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    #include <dt-bindings/leds/common.h>
> +
> +    ethernet-phy@0 {
> +        compatible = "ethernet-phy-ieee802.3-c45";
> +        reg = <0>;
> +
> +        leds {
> +            compatible = "linux,hw-controlled-leds";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            led@0 {
> +                reg = <0>;
> +                color = <LED_COLOR_ID_GREEN>;
> +                function = <LED_FUNCTION_STATUS>;

Reading the description of LED_FUNCTION_STATUS doesn't align with how 
you are using it. Think of it as user alert/notification.

> +                linux,default-trigger = "dev-hw-mode";

This is deprecated in favor of 'function'.

> +                linux,default-hw-mode = "1Gbps";
> +            };
> +
> +            led@1 {
> +                reg = <1>;
> +                color = <LED_COLOR_ID_YELLOW>;
> +                function = <LED_FUNCTION_ACTIVITY>;
> +                linux,default-trigger = "dev-hw-mode";
> +                linux,default-hw-mode = "activity";
> +            };
> +        };
> +    };
> +
> +...
> -- 
> 2.26.2
> 

Re: [PATCH net-next + leds v2 1/7] dt-bindings: leds: document binding for HW controlled LEDs

[Marek Behun]

On Wed, 9 Sep 2020 15:15:52 -0600
Rob Herring <robh@kernel.org> wrote:

> On Wed, Sep 09, 2020 at 06:25:46PM +0200, Marek Behún wrote:
> > Document binding for LEDs connected to and controlled by various chips
> > (such as ethernet PHY chips).  
> 
> If they are h/w controlled, then why are they in DT?

The idea is that by default these LEDs are in some specific HW control
mode (the chip default), but the chip supports various HW control
modes, and also supports SW control.
For example on Marvell PHYs there is a 4-bit register for each LED, so
16 values, and some of them are:
  0000: On - Receive
        Off - No receive
  0001: On - Link
        Blink - Activity
        Off - No Link
  ...
  0101: On - 100Mbps or Fiber Link
        Off - Else
  ...
  1000: Force Off
  1001: Force On
  ...
  1011: Force Blink

So writing 0x8 disables the LED, 0x9 enabled it (SW control via
/sys/class/leds/<LED>/brightness), other values change the HW control
mode (in this proposal /sys/class/leds/<LED>/hw_mode when trigger is
set to dev-hw-mode).

> > 
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > ---
> >  .../leds/linux,hw-controlled-leds.yaml        | 99 +++++++++++++++++++
> >  1 file changed, 99 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > new file mode 100644
> > index 0000000000000..eaf6e5d80c5f5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/linux,hw-controlled-leds.yaml
> > @@ -0,0 +1,99 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/linux,hw-controlled-leds.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
> > +
> > +maintainers:
> > +  - Marek Behún <marek.behun@nic.cz>
> > +
> > +description:
> > +  Many an ethernet PHY (and other chips) supports various HW control modes
> > +  for LEDs connected directly to them. With this binding such LEDs can be
> > +  described.
> > +
> > +properties:
> > +  compatible:
> > +    const: linux,hw-controlled-leds  
> 
> What makes this linux specific?
> 
> Unless you're going to make this h/w specific, then it probably should 
> just be dropped. 
> 

Will do, thanks.

> The phy schema will need:
> 
> leds:
>   type: object
>   $ref: /schemas/leds/hw-controlled-leds.yaml#
> 
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^led@[0-9a-f]+$":
> > +    type: object
> > +    allOf:
> > +      - $ref: common.yaml#
> > +    description:
> > +      This node represents a LED device connected to a chip that can control
> > +      the LED in various HW controlled modes.
> > +
> > +    properties:
> > +      reg:
> > +        maxItems: 1
> > +        description:
> > +          This property identifies the LED to the chip the LED is connected to
> > +          (eg. an ethernet PHY chip can have multiple LEDs connected to it).
> > +
> > +      enable-active-high:
> > +        description:
> > +          Polarity of LED is active high. If missing, assumed default is active
> > +          low.
> > +        type: boolean
> > +
> > +      led-tristate:
> > +        description:
> > +          LED pin is tristate type. If missing, assumed false.
> > +        type: boolean
> > +
> > +      linux,default-hw-mode:  
> 
> How is this linux specific? It sounds device specific. Your choice is 
> make this a device specific property with device specific values or come 
> up with generic modes.

I was inspired by `linux,default-trigger` and `linux,keycode`
properties...
 
> Perhaps 'function' should be expanded.

The thing is that `function` is now used for creating LED device name.
I was not aware that `linux,default-trigger` is deprecated
Perhaps this should be discussed with Pavel as well.
But of course from the perspective that DT should be independent from
Linux, you are right.
I fear this will be quite a pain to resolve...

> > +        description:
> > +          This parameter, if present, specifies the default HW triggering mode
> > +          of the LED when LED trigger is set to `dev-hw-mode`.
> > +          Available values are specific per device the LED is connected to and
> > +          per LED itself.
> > +        $ref: /schemas/types.yaml#definitions/string
> > +
> > +    required:
> > +      - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    ethernet-phy@0 {
> > +        compatible = "ethernet-phy-ieee802.3-c45";
> > +        reg = <0>;
> > +
> > +        leds {
> > +            compatible = "linux,hw-controlled-leds";
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            led@0 {
> > +                reg = <0>;
> > +                color = <LED_COLOR_ID_GREEN>;
> > +                function = <LED_FUNCTION_STATUS>;  
> 
> Reading the description of LED_FUNCTION_STATUS doesn't align with how 
> you are using it. Think of it as user alert/notification.
> 
> > +                linux,default-trigger = "dev-hw-mode";  
> 
> This is deprecated in favor of 'function'.

As written above, this is not how the LED subsystem currently works.
Nor is the deprecation documented.

Currently `function` is used to create LED device name in the form
`device:color:function` or `device:color:function-N` if
`function-enumerator` is also set.

> 
> > +                linux,default-hw-mode = "1Gbps";
> > +            };
> > +
> > +            led@1 {
> > +                reg = <1>;
> > +                color = <LED_COLOR_ID_YELLOW>;
> > +                function = <LED_FUNCTION_ACTIVITY>;
> > +                linux,default-trigger = "dev-hw-mode";
> > +                linux,default-hw-mode = "activity";
> > +            };
> > +        };
> > +    };
> > +
> > +...
> > -- 
> > 2.26.2
> >   

[PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware

[Marek Behún]

Many an ethernet PHY (and other chips) supports various HW control modes
for LEDs connected directly to them.

This patch adds a generic API for registering such LEDs when described
in device tree. This API also exposes generic way to select between
these hardware control modes.

This API registers a new private LED trigger called dev-hw-mode. When
this trigger is enabled for a LED, the various HW control modes which
are supported by the device for given LED can be get/set via hw_mode
sysfs file.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 .../sysfs-class-led-trigger-dev-hw-mode       |   8 +
 drivers/leds/Kconfig                          |  10 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-hw-controlled.c             | 227 ++++++++++++++++++
 include/linux/leds-hw-controlled.h            |  74 ++++++
 5 files changed, 320 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
 create mode 100644 drivers/leds/leds-hw-controlled.c
 create mode 100644 include/linux/leds-hw-controlled.h

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode b/Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
new file mode 100644
index 0000000000000..7bca112e7ff93
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
@@ -0,0 +1,8 @@
+What:		/sys/class/leds/<led>/hw_mode
+Date:		September 2020
+KernelVersion:	5.10
+Contact:	Marek Behún <marek.behun@nic.cz>
+		linux-leds@vger.kernel.org
+Description:	(W) Set the HW control mode of this LED. The various available HW control modes
+		    are specific per device to which the LED is connected to and per LED itself.
+		(R) Show the available HW control modes and the currently selected one.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c181df24eae4..5e47ab21aafb4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED
 
 	  See Documentation/ABI/testing/sysfs-class-led for details.
 
+config LEDS_HW_CONTROLLED
+	bool "API for LEDs that can be controlled by hardware"
+	depends on LEDS_CLASS
+	select LEDS_TRIGGERS
+	help
+	  This option enables support for a generic API via which other drivers
+	  can register LEDs that can be put into hardware controlled mode, eg.
+	  a LED connected to an ethernet PHY can be configured to blink on
+	  network activity.
+
 comment "LED drivers"
 
 config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c2c7d7ade0d06..858e468e40df0 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
 obj-$(CONFIG_LEDS_CLASS_FLASH)		+= led-class-flash.o
 obj-$(CONFIG_LEDS_CLASS_MULTICOLOR)	+= led-class-multicolor.o
 obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
+obj-$(CONFIG_LEDS_HW_CONTROLLED)	+= leds-hw-controlled.o
 
 # LED Platform Drivers (keep this sorted, M-| sort)
 obj-$(CONFIG_LEDS_88PM860X)		+= leds-88pm860x.o
diff --git a/drivers/leds/leds-hw-controlled.c b/drivers/leds/leds-hw-controlled.c
new file mode 100644
index 0000000000000..9ef58bf275efd
--- /dev/null
+++ b/drivers/leds/leds-hw-controlled.c
@@ -0,0 +1,227 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Generic API for LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
+ *
+ * Copyright (C) 2020 Marek Behun <marek.behun@nic.cz>
+ */
+#include <linux/leds-hw-controlled.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
+{
+	struct hw_controlled_led *led = led_cdev_to_hw_controlled_led(cdev);
+	int ret;
+
+	mutex_lock(&led->lock);
+	ret = led->ops->led_brightness_set(cdev->dev->parent, led, brightness);
+	mutex_unlock(&led->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(hw_controlled_led_brightness_set);
+
+static int of_register_hw_controlled_led(struct device *dev, struct device_node *np,
+					 const char *devicename,
+					 const struct hw_controlled_led_ops *ops)
+{
+	struct led_init_data init_data = {};
+	struct hw_controlled_led *led;
+	u32 reg;
+	int ret;
+
+	ret = of_property_read_u32(np, "reg", &reg);
+	if (ret < 0)
+		return ret;
+
+	led = devm_kzalloc(dev, sizeof(struct hw_controlled_led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->ops = ops;
+
+	led->cdev.max_brightness = 1;
+	led->cdev.brightness_set_blocking = hw_controlled_led_brightness_set;
+	led->cdev.trigger_type = &hw_control_led_trig_type;
+	led->addr = reg;
+
+	of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger);
+	of_property_read_string(np, "linux,default-hw-mode", &led->hw_mode);
+
+	led->active_low = !of_property_read_bool(np, "enable-active-high");
+	led->tristate = of_property_read_bool(np, "led-tristate");
+
+	init_data.fwnode = &np->fwnode;
+	init_data.devname_mandatory = true;
+	init_data.devicename = devicename;
+
+	ret = led->ops->led_init(dev, led);
+	if (ret < 0)
+		goto err_free;
+
+	ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
+	if (ret < 0)
+		goto err_free;
+
+	return 0;
+err_free:
+	devm_kfree(dev, led);
+	return ret;
+}
+
+int of_register_hw_controlled_leds(struct device *dev, const char *devicename,
+				   const struct hw_controlled_led_ops *ops)
+{
+	struct device_node *node = dev->of_node;
+	struct device_node *leds, *led;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return 0;
+
+	if (!ops)
+		return -EINVAL;
+
+	/* maybe we should have of_get_compatible_available_child as well */
+	leds = of_get_compatible_child(node, "linux,hw-controlled-leds");
+	if (!leds)
+		return 0;
+
+	if (!devicename)
+		devicename = dev_name(dev);
+
+	for_each_available_child_of_node(leds, led) {
+		ret = of_register_hw_controlled_led(dev, led, devicename, ops);
+		if (ret < 0)
+			dev_err(dev, "Nonfatal error: cannot register LED from node %pOFn: %i\n",
+				led, ret);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_register_hw_controlled_leds);
+
+static int hw_control_led_trig_activate(struct led_classdev *cdev)
+{
+	struct hw_controlled_led *led;
+	int ret;
+
+	led = led_cdev_to_hw_controlled_led(cdev);
+
+	if (!led->hw_mode)
+		return 0;
+
+	mutex_lock(&led->lock);
+	ret = led->ops->led_set_hw_mode(cdev->dev->parent, led, led->hw_mode);
+	mutex_unlock(&led->lock);
+
+	if (ret < 0)
+		dev_warn(cdev->dev->parent, "Could not set HW mode %s on LED %s: %i\n",
+			 led->hw_mode, cdev->name, ret);
+
+	/* don't fail to activate this trigger so that user can write hw_mode file */
+	return 0;
+}
+
+static void hw_control_led_trig_deactivate(struct led_classdev *cdev)
+{
+	struct hw_controlled_led *led;
+	int ret;
+
+	led = led_cdev_to_hw_controlled_led(cdev);
+
+	mutex_lock(&led->lock);
+	/* store HW mode before deactivation */
+	led->hw_mode = led->ops->led_get_hw_mode(cdev->dev->parent, led);
+	ret = led->ops->led_set_hw_mode(cdev->dev->parent, led, NULL);
+	mutex_unlock(&led->lock);
+
+	if (ret < 0)
+		dev_err(cdev->dev->parent, "Failed deactivating HW mode on LED %s\n", cdev->name);
+}
+
+static ssize_t hw_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct hw_controlled_led *led;
+	const char *mode, *cur_mode;
+	void *iter = NULL;
+	int len = 0;
+
+	led = led_cdev_to_hw_controlled_led(led_trigger_get_led(dev));
+
+	mutex_lock(&led->lock);
+
+	cur_mode = led->ops->led_get_hw_mode(dev->parent, led);
+
+	for (mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter);
+	     mode;
+	     mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter)) {
+		bool sel;
+
+		sel = cur_mode && !strcmp(mode, cur_mode);
+
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode,
+				 sel ? "]" : "");
+	}
+
+	if (buf[len - 1] == ' ')
+		buf[len - 1] = '\n';
+
+	mutex_unlock(&led->lock);
+
+	return len;
+}
+
+static ssize_t hw_mode_store(struct device *dev, struct device_attribute *attr, const char *buf,
+			     size_t count)
+{
+	struct hw_controlled_led *led;
+	int ret;
+
+	led = led_cdev_to_hw_controlled_led(led_trigger_get_led(dev));
+
+	mutex_lock(&led->lock);
+	ret = led->ops->led_set_hw_mode(dev->parent, led, buf);
+	if (ret < 0)
+		return ret;
+	mutex_unlock(&led->lock);
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(hw_mode);
+
+static struct attribute *hw_control_led_trig_attrs[] = {
+	&dev_attr_hw_mode.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(hw_control_led_trig);
+
+struct led_hw_trigger_type hw_control_led_trig_type;
+EXPORT_SYMBOL_GPL(hw_control_led_trig_type);
+
+struct led_trigger hw_control_led_trig = {
+	.name		= "dev-hw-mode",
+	.activate	= hw_control_led_trig_activate,
+	.deactivate	= hw_control_led_trig_deactivate,
+	.trigger_type	= &hw_control_led_trig_type,
+	.groups		= hw_control_led_trig_groups,
+};
+EXPORT_SYMBOL_GPL(hw_control_led_trig);
+
+static int __init hw_controlled_leds_init(void)
+{
+	return led_trigger_register(&hw_control_led_trig);
+}
+
+static void __exit hw_controlled_leds_exit(void)
+{
+	led_trigger_unregister(&hw_control_led_trig);
+}
+
+subsys_initcall(hw_controlled_leds_init);
+module_exit(hw_controlled_leds_exit);
+
+MODULE_AUTHOR("Marek Behun <marek.behun@nic.cz>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("API for HW controlled LEDs");
diff --git a/include/linux/leds-hw-controlled.h b/include/linux/leds-hw-controlled.h
new file mode 100644
index 0000000000000..2c9b8a06def18
--- /dev/null
+++ b/include/linux/leds-hw-controlled.h
@@ -0,0 +1,74 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Generic API for LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
+ *
+ * Copyright (C) 2020 Marek Behun <marek.behun@nic.cz>
+ */
+#ifndef _LINUX_LEDS_HW_CONTROLLED_H_
+#define _LINUX_LEDS_HW_CONTROLLED_H_
+
+#include <linux/kernel.h>
+#include <linux/leds.h>
+
+struct hw_controlled_led {
+	struct led_classdev cdev;
+	const struct hw_controlled_led_ops *ops;
+	struct mutex lock;
+
+	/* these members are filled in by OF if OF is enabled */
+	int addr;
+	bool active_low;
+	bool tristate;
+
+	/* also filled in by OF, but changed by led_set_hw_mode operation */
+	const char *hw_mode;
+
+	void *priv;
+};
+#define led_cdev_to_hw_controlled_led(l) container_of(l, struct hw_controlled_led, cdev)
+
+/* struct hw_controlled_led_ops: Operations on LEDs that can be controlled by HW
+ *
+ * All the following operations must be implemented:
+ * @led_init: Should initialize the LED from OF data (and sanity check whether they are correct).
+ *            This should also change led->cdev.max_brightness, if the value differs from default,
+ *            which is 1.
+ * @led_brightness_set: Sets brightness.
+ * @led_iter_hw_mode: Iterates available HW control mode names for this LED.
+ * @led_set_hw_mode: Sets HW control mode to value specified by given name.
+ * @led_get_hw_mode: Returns current HW control mode name.
+ */
+struct hw_controlled_led_ops {
+	int (*led_init)(struct device *dev, struct hw_controlled_led *led);
+	int (*led_brightness_set)(struct device *dev, struct hw_controlled_led *led,
+				  enum led_brightness brightness);
+	const char *(*led_iter_hw_mode)(struct device *dev, struct hw_controlled_led *led,
+					void **iter);
+	int (*led_set_hw_mode)(struct device *dev, struct hw_controlled_led *led,
+			       const char *mode);
+	const char *(*led_get_hw_mode)(struct device *dev, struct hw_controlled_led *led);
+};
+
+#if IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED)
+
+#define hw_controlled_led_ops_ptr(s) (s)
+
+int of_register_hw_controlled_leds(struct device *dev, const char *devicename,
+				   const struct hw_controlled_led_ops *ops);
+int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness);
+
+extern struct led_hw_trigger_type hw_control_led_trig_type;
+extern struct led_trigger hw_control_led_trig;
+
+#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
+
+#define hw_controlled_led_ops_ptr(s) NULL
+static inline int of_register_hw_controlled_leds(struct device *dev, const char *devicename,
+						 const struct hw_controlled_led_ops *ops)
+{
+	return 0;
+}
+
+#endif /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
+
+#endif /* _LINUX_LEDS_HW_CONTROLLED_H_ */
-- 
2.26.2

Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware

[Randy Dunlap]

Hi,

On 9/9/20 9:25 AM, Marek Behún wrote:
> Many an ethernet PHY (and other chips) supports various HW control modes
> for LEDs connected directly to them.
> 
> This patch adds a generic API for registering such LEDs when described
> in device tree. This API also exposes generic way to select between
> these hardware control modes.
> 
> This API registers a new private LED trigger called dev-hw-mode. When
> this trigger is enabled for a LED, the various HW control modes which
> are supported by the device for given LED can be get/set via hw_mode
> sysfs file.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  .../sysfs-class-led-trigger-dev-hw-mode       |   8 +
>  drivers/leds/Kconfig                          |  10 +
>  drivers/leds/Makefile                         |   1 +
>  drivers/leds/leds-hw-controlled.c             | 227 ++++++++++++++++++
>  include/linux/leds-hw-controlled.h            |  74 ++++++
>  5 files changed, 320 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
>  create mode 100644 drivers/leds/leds-hw-controlled.c
>  create mode 100644 include/linux/leds-hw-controlled.h
> 

> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 1c181df24eae4..5e47ab21aafb4 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED
>  
>  	  See Documentation/ABI/testing/sysfs-class-led for details.
>  
> +config LEDS_HW_CONTROLLED
> +	bool "API for LEDs that can be controlled by hardware"
> +	depends on LEDS_CLASS

	depends on OF || COMPILE_TEST
?

> +	select LEDS_TRIGGERS
> +	help
> +	  This option enables support for a generic API via which other drivers
> +	  can register LEDs that can be put into hardware controlled mode, eg.

	                                                                   e.g.

> +	  a LED connected to an ethernet PHY can be configured to blink on

	  an LED

> +	  network activity.
> +
>  comment "LED drivers"
>  
>  config LEDS_88PM860X


> diff --git a/include/linux/leds-hw-controlled.h b/include/linux/leds-hw-controlled.h
> new file mode 100644
> index 0000000000000..2c9b8a06def18
> --- /dev/null
> +++ b/include/linux/leds-hw-controlled.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Generic API for LEDs that can be controlled by hardware (eg. by an ethernet PHY chip)
> + *
> + * Copyright (C) 2020 Marek Behun <marek.behun@nic.cz>
> + */
> +#ifndef _LINUX_LEDS_HW_CONTROLLED_H_
> +#define _LINUX_LEDS_HW_CONTROLLED_H_
> +
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +
> +struct hw_controlled_led {
> +	struct led_classdev cdev;
> +	const struct hw_controlled_led_ops *ops;
> +	struct mutex lock;
> +
> +	/* these members are filled in by OF if OF is enabled */
> +	int addr;
> +	bool active_low;
> +	bool tristate;
> +
> +	/* also filled in by OF, but changed by led_set_hw_mode operation */
> +	const char *hw_mode;
> +
> +	void *priv;
> +};
> +#define led_cdev_to_hw_controlled_led(l) container_of(l, struct hw_controlled_led, cdev)
> +
> +/* struct hw_controlled_led_ops: Operations on LEDs that can be controlled by HW
> + *
> + * All the following operations must be implemented:
> + * @led_init: Should initialize the LED from OF data (and sanity check whether they are correct).
> + *            This should also change led->cdev.max_brightness, if the value differs from default,
> + *            which is 1.
> + * @led_brightness_set: Sets brightness.
> + * @led_iter_hw_mode: Iterates available HW control mode names for this LED.
> + * @led_set_hw_mode: Sets HW control mode to value specified by given name.
> + * @led_get_hw_mode: Returns current HW control mode name.
> + */

Convert that struct info to kernel-doc?

> +struct hw_controlled_led_ops {
> +	int (*led_init)(struct device *dev, struct hw_controlled_led *led);
> +	int (*led_brightness_set)(struct device *dev, struct hw_controlled_led *led,
> +				  enum led_brightness brightness);
> +	const char *(*led_iter_hw_mode)(struct device *dev, struct hw_controlled_led *led,
> +					void **iter);
> +	int (*led_set_hw_mode)(struct device *dev, struct hw_controlled_led *led,
> +			       const char *mode);
> +	const char *(*led_get_hw_mode)(struct device *dev, struct hw_controlled_led *led);
> +};
> +

> +
> +#endif /* _LINUX_LEDS_HW_CONTROLLED_H_ */

thanks.
-- 
~Randy

Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware

[Marek Behún]

On Wed, 9 Sep 2020 11:20:00 -0700
Randy Dunlap <rdunlap@infradead.org> wrote:

> Hi,
> 
> On 9/9/20 9:25 AM, Marek Behún wrote:
> > Many an ethernet PHY (and other chips) supports various HW control
> > modes for LEDs connected directly to them.
> > 
> > This patch adds a generic API for registering such LEDs when
> > described in device tree. This API also exposes generic way to
> > select between these hardware control modes.
> > 
> > This API registers a new private LED trigger called dev-hw-mode.
> > When this trigger is enabled for a LED, the various HW control
> > modes which are supported by the device for given LED can be
> > get/set via hw_mode sysfs file.
> > 
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  .../sysfs-class-led-trigger-dev-hw-mode       |   8 +
> >  drivers/leds/Kconfig                          |  10 +
> >  drivers/leds/Makefile                         |   1 +
> >  drivers/leds/leds-hw-controlled.c             | 227
> > ++++++++++++++++++ include/linux/leds-hw-controlled.h            |
> > 74 ++++++ 5 files changed, 320 insertions(+)
> >  create mode 100644
> > Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
> > create mode 100644 drivers/leds/leds-hw-controlled.c create mode
> > 100644 include/linux/leds-hw-controlled.h 
> 
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 1c181df24eae4..5e47ab21aafb4 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED
> >  
> >  	  See Documentation/ABI/testing/sysfs-class-led for
> > details. 
> > +config LEDS_HW_CONTROLLED
> > +	bool "API for LEDs that can be controlled by hardware"
> > +	depends on LEDS_CLASS  
> 
> 	depends on OF || COMPILE_TEST
> ?
> 

I specifically did not add OF dependency so that this can be also used
on non-OF systems. A device driver may register such LED itself...
That's why hw_controlled_led_brightness_set symbol is exported.

Do you think I shouldn't do it?

> > +	select LEDS_TRIGGERS
> > +	help
> > +	  This option enables support for a generic API via which
> > other drivers
> > +	  can register LEDs that can be put into hardware
> > controlled mode, eg.  
> 
> 	                                                                   e.g.
> 

This will need to be changed on multiple places, thanks.

> > +	  a LED connected to an ethernet PHY can be configured to
> > blink on  
> 
> 	  an LED
> 

This as well

> > +	  network activity.
> > +
> >  comment "LED drivers"
> >  
> >  config LEDS_88PM860X  
> 
> 
> > diff --git a/include/linux/leds-hw-controlled.h
> > b/include/linux/leds-hw-controlled.h new file mode 100644
> > index 0000000000000..2c9b8a06def18
> > --- /dev/null
> > +++ b/include/linux/leds-hw-controlled.h
> > @@ -0,0 +1,74 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Generic API for LEDs that can be controlled by hardware (eg. by
> > an ethernet PHY chip)
> > + *
> > + * Copyright (C) 2020 Marek Behun <marek.behun@nic.cz>
> > + */
> > +#ifndef _LINUX_LEDS_HW_CONTROLLED_H_
> > +#define _LINUX_LEDS_HW_CONTROLLED_H_
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/leds.h>
> > +
> > +struct hw_controlled_led {
> > +	struct led_classdev cdev;
> > +	const struct hw_controlled_led_ops *ops;
> > +	struct mutex lock;
> > +
> > +	/* these members are filled in by OF if OF is enabled */
> > +	int addr;
> > +	bool active_low;
> > +	bool tristate;
> > +
> > +	/* also filled in by OF, but changed by led_set_hw_mode
> > operation */
> > +	const char *hw_mode;
> > +
> > +	void *priv;
> > +};
> > +#define led_cdev_to_hw_controlled_led(l) container_of(l, struct
> > hw_controlled_led, cdev) +
> > +/* struct hw_controlled_led_ops: Operations on LEDs that can be
> > controlled by HW
> > + *
> > + * All the following operations must be implemented:
> > + * @led_init: Should initialize the LED from OF data (and sanity
> > check whether they are correct).
> > + *            This should also change led->cdev.max_brightness, if
> > the value differs from default,
> > + *            which is 1.
> > + * @led_brightness_set: Sets brightness.
> > + * @led_iter_hw_mode: Iterates available HW control mode names for
> > this LED.
> > + * @led_set_hw_mode: Sets HW control mode to value specified by
> > given name.
> > + * @led_get_hw_mode: Returns current HW control mode name.
> > + */  
> 
> Convert that struct info to kernel-doc?
> 

Will look into this.
Thanks.

> > +struct hw_controlled_led_ops {
> > +	int (*led_init)(struct device *dev, struct
> > hw_controlled_led *led);
> > +	int (*led_brightness_set)(struct device *dev, struct
> > hw_controlled_led *led,
> > +				  enum led_brightness brightness);
> > +	const char *(*led_iter_hw_mode)(struct device *dev, struct
> > hw_controlled_led *led,
> > +					void **iter);
> > +	int (*led_set_hw_mode)(struct device *dev, struct
> > hw_controlled_led *led,
> > +			       const char *mode);
> > +	const char *(*led_get_hw_mode)(struct device *dev, struct
> > hw_controlled_led *led); +};
> > +  
> 
> > +
> > +#endif /* _LINUX_LEDS_HW_CONTROLLED_H_ */  
> 
> thanks.

Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware

[Randy Dunlap]

On 9/9/20 11:31 AM, Marek Behún wrote:
> On Wed, 9 Sep 2020 11:20:00 -0700
> Randy Dunlap <rdunlap@infradead.org> wrote:
> 
>> Hi,
>>
>> On 9/9/20 9:25 AM, Marek Behún wrote:
>>> Many an ethernet PHY (and other chips) supports various HW control
>>> modes for LEDs connected directly to them.
>>>
>>> This patch adds a generic API for registering such LEDs when
>>> described in device tree. This API also exposes generic way to
>>> select between these hardware control modes.
>>>
>>> This API registers a new private LED trigger called dev-hw-mode.
>>> When this trigger is enabled for a LED, the various HW control
>>> modes which are supported by the device for given LED can be
>>> get/set via hw_mode sysfs file.
>>>
>>> Signed-off-by: Marek Behún <marek.behun@nic.cz>
>>> ---
>>>  .../sysfs-class-led-trigger-dev-hw-mode       |   8 +
>>>  drivers/leds/Kconfig                          |  10 +
>>>  drivers/leds/Makefile                         |   1 +
>>>  drivers/leds/leds-hw-controlled.c             | 227
>>> ++++++++++++++++++ include/linux/leds-hw-controlled.h            |
>>> 74 ++++++ 5 files changed, 320 insertions(+)
>>>  create mode 100644
>>> Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode
>>> create mode 100644 drivers/leds/leds-hw-controlled.c create mode
>>> 100644 include/linux/leds-hw-controlled.h 
>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 1c181df24eae4..5e47ab21aafb4 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED
>>>  
>>>  	  See Documentation/ABI/testing/sysfs-class-led for
>>> details. 
>>> +config LEDS_HW_CONTROLLED
>>> +	bool "API for LEDs that can be controlled by hardware"
>>> +	depends on LEDS_CLASS  
>>
>> 	depends on OF || COMPILE_TEST
>> ?
>>
> 
> I specifically did not add OF dependency so that this can be also used
> on non-OF systems. A device driver may register such LED itself...
> That's why hw_controlled_led_brightness_set symbol is exported.
> 
> Do you think I shouldn't do it?

I have no problem with it as it is.

>>> +	select LEDS_TRIGGERS
>>> +	help
>>> +	  This option enables support for a generic API via which
>>> other drivers
>>> +	  can register LEDs that can be put into hardware
>>> controlled mode, eg.  

thanks.
-- 
~Randy

Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2673 bytes --]

Hi!

> Many an ethernet PHY (and other chips) supports various HW control modes
> for LEDs connected directly to them.

I guess this should be

"Many ethernet PHYs (and other chips) support various HW control modes
for LEDs connected directly to them."

> This API registers a new private LED trigger called dev-hw-mode. When
> this trigger is enabled for a LED, the various HW control modes which
> are supported by the device for given LED can be get/set via hw_mode
> sysfs file.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  .../sysfs-class-led-trigger-dev-hw-mode       |   8 +
>  drivers/leds/Kconfig                          |  10 +

I guess this should live in drivers/leds/trigger/ledtrig-hw.c . I'd
call the trigger just "hw"...

> +Contact:	Marek Behún <marek.behun@nic.cz>
> +		linux-leds@vger.kernel.org
> +Description:	(W) Set the HW control mode of this LED. The various available HW control modes
> +		    are specific per device to which the LED is connected to and per LED itself.
> +		(R) Show the available HW control modes and the currently selected one.

80 columns :-) (and please fix that globally, at least at places where
it is easy, like comments).

> +	return 0;
> +err_free:
> +	devm_kfree(dev, led);
> +	return ret;
> +}

No need for explicit free with devm_ infrastructure?

> +	cur_mode = led->ops->led_get_hw_mode(dev->parent, led);
> +
> +	for (mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter);
> +	     mode;
> +	     mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter)) {
> +		bool sel;
> +
> +		sel = cur_mode && !strcmp(mode, cur_mode);
> +
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode,
> +				 sel ? "]" : "");
> +	}
> +
> +	if (buf[len - 1] == ' ')
> +		buf[len - 1] = '\n';

Can this ever be false? Are you accessing buf[-1] in such case?

> +int of_register_hw_controlled_leds(struct device *dev, const char *devicename,
> +				   const struct hw_controlled_led_ops *ops);
> +int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness);
> +

Could we do something like hw_controlled_led -> hw_led to keep
verbosity down and line lengths reasonable? Or hwc_led?

> +extern struct led_hw_trigger_type hw_control_led_trig_type;
> +extern struct led_trigger hw_control_led_trig;
> +
> +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */

CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW?

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware

[Marek Behun]

On Wed, 9 Sep 2020 22:48:15 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > Many an ethernet PHY (and other chips) supports various HW control modes
> > for LEDs connected directly to them.  
> 
> I guess this should be
> 
> "Many ethernet PHYs (and other chips) support various HW control modes
> for LEDs connected directly to them."
> 

I guess it is older English, used mainly in poetry, but I read it in
works of contemporary fiction as well. As far as I could find, it is still
actually gramatically correct.
https://idioms.thefreedictionary.com/many+an
https://en.wiktionary.org/wiki/many_a
But I will change it if you insist on it.

> > This API registers a new private LED trigger called dev-hw-mode. When
> > this trigger is enabled for a LED, the various HW control modes which
> > are supported by the device for given LED can be get/set via hw_mode
> > sysfs file.
> > 
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  .../sysfs-class-led-trigger-dev-hw-mode       |   8 +
> >  drivers/leds/Kconfig                          |  10 +  
> 
> I guess this should live in drivers/leds/trigger/ledtrig-hw.c . I'd
> call the trigger just "hw"...
> 

Will move in next version. Lets see what others think about the trigger
name.

> > +Contact:	Marek Behún <marek.behun@nic.cz>
> > +		linux-leds@vger.kernel.org
> > +Description:	(W) Set the HW control mode of this LED. The various available HW control modes
> > +		    are specific per device to which the LED is connected to and per LED itself.
> > +		(R) Show the available HW control modes and the currently selected one.  
> 
> 80 columns :-) (and please fix that globally, at least at places where
> it is easy, like comments).
> 

Linux is at 100 columns now since commit bdc48fa11e46, commited by
Linus. See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
There was actually an article about this on Phoronix, I think.

> > +	return 0;
> > +err_free:
> > +	devm_kfree(dev, led);
> > +	return ret;
> > +}  
> 
> No need for explicit free with devm_ infrastructure?


No, but if the registration failed, I don't see any reason why the
memory should be freed only when the PHY device is destroyed, if the
memory is not used... On failures other code in Linux frees even devm_
allocated resources.

> > +	cur_mode = led->ops->led_get_hw_mode(dev->parent, led);
> > +
> > +	for (mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter);
> > +	     mode;
> > +	     mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter)) {
> > +		bool sel;
> > +
> > +		sel = cur_mode && !strcmp(mode, cur_mode);
> > +
> > +		len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode,
> > +				 sel ? "]" : "");
> > +	}
> > +
> > +	if (buf[len - 1] == ' ')
> > +		buf[len - 1] = '\n';  
> 
> Can this ever be false? Are you accessing buf[-1] in such case?
> 

It can be false if whole PAGE_SIZE is written. The code above
using scnprintf only writes the first PAGE_SIZE bytes.
You are right about the buf[-1] case though. len has to be nonzero.
Thanks.

> > +int of_register_hw_controlled_leds(struct device *dev, const char *devicename,
> > +				   const struct hw_controlled_led_ops *ops);
> > +int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness);
> > +  
> 
> Could we do something like hw_controlled_led -> hw_led to keep
> verbosity down and line lengths reasonable? Or hwc_led?
>

I am not opposed, lets see what Andrew / others think.

> > +extern struct led_hw_trigger_type hw_control_led_trig_type;
> > +extern struct led_trigger hw_control_led_trig;
> > +
> > +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */  
> 
> CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW?

The second option looks more reasonable to me, if we move to
drivers/leds/trigger.

Marek

Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware

[Pavel Machek]

Hi!

> > > Many an ethernet PHY (and other chips) supports various HW control modes
> > > for LEDs connected directly to them.  
> > 
> > I guess this should be
> > 
> > "Many ethernet PHYs (and other chips) support various HW control modes
> > for LEDs connected directly to them."
> > 
> 
> I guess it is older English, used mainly in poetry, but I read it in
> works of contemporary fiction as well. As far as I could find, it is still
> actually gramatically correct.
> https://idioms.thefreedictionary.com/many+an
> https://en.wiktionary.org/wiki/many_a
> But I will change it if you insist on it.

Okay, you got me.

> > > +Contact:	Marek Behún <marek.behun@nic.cz>
> > > +		linux-leds@vger.kernel.org
> > > +Description:	(W) Set the HW control mode of this LED. The various available HW control modes
> > > +		    are specific per device to which the LED is connected to and per LED itself.
> > > +		(R) Show the available HW control modes and the currently selected one.  
> > 
> > 80 columns :-) (and please fix that globally, at least at places where
> > it is easy, like comments).
> > 
> 
> Linux is at 100 columns now since commit bdc48fa11e46, commited by
> Linus. See
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> There was actually an article about this on Phoronix, I think.

It is not. Checkpatch no longer warns about it, but 80 columns is
still preffered, see Documentation/process/coding-style.rst . Plus,
you want me to take the patch, not Linus.

> > > +extern struct led_hw_trigger_type hw_control_led_trig_type;
> > > +extern struct led_trigger hw_control_led_trig;
> > > +
> > > +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */  
> > 
> > CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW?
> 
> The second option looks more reasonable to me, if we move to
> drivers/leds/trigger.

Ok :-).

Best regards,
							Pavel

Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware

[Marek Behun]

On Wed, 9 Sep 2020 23:40:09 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> > > 
> > > 80 columns :-) (and please fix that globally, at least at places where
> > > it is easy, like comments).
> > >   
> > 
> > Linux is at 100 columns now since commit bdc48fa11e46, commited by
> > Linus. See
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> > There was actually an article about this on Phoronix, I think.  
> 
> It is not. Checkpatch no longer warns about it, but 80 columns is
> still preffered, see Documentation/process/coding-style.rst . Plus,
> you want me to take the patch, not Linus.

Very well, I shall rewrap it to 80 columns :)

Marek

Re: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Thu 2020-09-10 00:15:26, Marek Behun wrote:
> On Wed, 9 Sep 2020 23:40:09 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > > > 
> > > > 80 columns :-) (and please fix that globally, at least at places where
> > > > it is easy, like comments).
> > > >   
> > > 
> > > Linux is at 100 columns now since commit bdc48fa11e46, commited by
> > > Linus. See
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> > > There was actually an article about this on Phoronix, I think.  
> > 
> > It is not. Checkpatch no longer warns about it, but 80 columns is
> > still preffered, see Documentation/process/coding-style.rst . Plus,
> > you want me to take the patch, not Linus.
> 
> Very well, I shall rewrap it to 80 columns :)

And thou shalt wrap to 80 columns ever after!
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH net-next + leds v2 3/7] net: phy: add simple incrementing phyindex member to phy_device struct

[Marek Behún]

Add a new integer member phyindex to struct phy_device. This member is
unique for every phy_device. Atomic incrementation occurs in
phy_device_register.

This can be used for example in LED sysfs API. The LED subsystem names
each LED in format `device:color:function`, but currently the PHY device
names are not suited for this, since in some situations a PHY device
name can look like this
  d0032004.mdio-mii:01
or even like this
  /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
Clearly this cannot be used as the `device` part of a LED name.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/phy_device.c | 3 +++
 include/linux/phy.h          | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8adfbad0a1e8f..38f56d39f1229 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,6 +9,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/atomic.h>
 #include <linux/bitmap.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
@@ -892,6 +893,7 @@ EXPORT_SYMBOL(get_phy_device);
  */
 int phy_device_register(struct phy_device *phydev)
 {
+	static atomic_t phyindex;
 	int err;
 
 	err = mdiobus_register_device(&phydev->mdio);
@@ -908,6 +910,7 @@ int phy_device_register(struct phy_device *phydev)
 		goto out;
 	}
 
+	phydev->phyindex = atomic_inc_return(&phyindex) - 1;
 	err = device_add(&phydev->mdio.dev);
 	if (err) {
 		phydev_err(phydev, "failed to add\n");
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3a09d2bf69ea4..52881e21ad951 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -406,6 +406,7 @@ struct macsec_ops;
 /* phy_device: An instance of a PHY
  *
  * drv: Pointer to the driver for this PHY instance
+ * phyindex: a simple incrementing PHY index
  * phy_id: UID for this device found during discovery
  * c45_ids: 802.3-c45 Device Identifers if is_c45.
  * is_c45:  Set to true if this phy uses clause 45 addressing.
@@ -446,6 +447,8 @@ struct phy_device {
 	/* And management functions */
 	struct phy_driver *drv;
 
+	int phyindex;
+
 	u32 phy_id;
 
 	struct phy_c45_device_ids c45_ids;
-- 
2.26.2

Re: [PATCH net-next + leds v2 3/7] net: phy: add simple incrementing phyindex member to phy_device struct

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

Hi!

> names are not suited for this, since in some situations a PHY device
> name can look like this
>   d0032004.mdio-mii:01
> or even like this
>   /soc/internal-regs@d0000000/mdio@32004/switch0@10/mdio:08
> Clearly this cannot be used as the `device` part of a LED name.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/net/phy/phy_device.c | 3 +++
>  include/linux/phy.h          | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8adfbad0a1e8f..38f56d39f1229 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -9,6 +9,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/atomic.h>
>  #include <linux/bitmap.h>
>  #include <linux/delay.h>
>  #include <linux/errno.h>
> @@ -892,6 +893,7 @@ EXPORT_SYMBOL(get_phy_device);
>   */
>  int phy_device_register(struct phy_device *phydev)
>  {
> +	static atomic_t phyindex;
>  	int err;
>  
>  	err = mdiobus_register_device(&phydev->mdio);

I'd put the static out of the function... for greater visibility.

Otherwise: Reviewed-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH net-next + leds v2 4/7] dt-bindings: net: ethernet-phy: add description for PHY LEDs

[Marek Behún]

Document binding for LEDs connected to an ethernet PHY chip.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/net/ethernet-phy.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index a9e547ac79051..f593e8709dd0d 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -174,6 +174,14 @@ properties:
       PHY's that have configurable TX internal delays. If this property is
       present then the PHY applies the TX delay.
 
+  leds:
+    type: object
+    description: |
+      This is used to described LEDs that are connected to the PHY chip and
+      their blinking can be controlled by the PHY.
+    allOf:
+      - $ref: /schemas/leds/linux,hw-controlled-leds.yaml#
+
 required:
   - reg
 
-- 
2.26.2

[PATCH net-next + leds v2 5/7] net: phy: add support for LEDs controlled by ethernet PHY chips

[Marek Behún]

This patch uses the new API for HW controlled LEDs to add support for
probing and control of LEDs connected to an ethernet PHY chip.

A PHY driver wishing to utilize this API needs to implement the methods
in struct hw_controlled_led_ops and set the member led_ops in struct
phy_driver to point to that structure.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/phy_device.c | 103 +++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |   4 ++
 2 files changed, 107 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 38f56d39f1229..54d5c88e4d4b2 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2820,6 +2820,103 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->ack_interrupt;
 }
 
+#if IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED)
+
+/* PHY mutex lock wrappers for operations on PHY HW controlled LEDs, so that PHY drivers
+ * implementing these operations don't have to lock phydev->lock themselves.
+ */
+static int phy_led_init(struct device *dev, struct hw_controlled_led *led)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->led_ops->led_init(dev, led);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+static int phy_led_brightness_set(struct device *dev, struct hw_controlled_led *led,
+				  enum led_brightness brightness)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->led_ops->led_brightness_set(dev, led, brightness);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+static const char *phy_led_iter_hw_mode(struct device *dev, struct hw_controlled_led *led,
+					void **iter)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	const char *ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->led_ops->led_iter_hw_mode(dev, led, iter);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+static int phy_led_set_hw_mode(struct device *dev, struct hw_controlled_led *led, const char *mode)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	int ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->led_ops->led_set_hw_mode(dev, led, mode);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+static const char *phy_led_get_hw_mode(struct device *dev, struct hw_controlled_led *led)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	const char *ret;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->led_ops->led_get_hw_mode(dev, led);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+static const struct hw_controlled_led_ops phy_hw_controlled_led_ops = {
+	.led_init		= phy_led_init,
+	.led_brightness_set	= phy_led_brightness_set,
+	.led_iter_hw_mode	= phy_led_iter_hw_mode,
+	.led_set_hw_mode	= phy_led_set_hw_mode,
+	.led_get_hw_mode	= phy_led_get_hw_mode,
+};
+
+static int of_phy_probe_leds(struct phy_device *phydev)
+{
+	char devicename[32];
+
+	if (!phydev->drv->led_ops)
+		return 0;
+
+	snprintf(devicename, sizeof(devicename), "ethernet-phy%i", phydev->phyindex);
+
+	return of_register_hw_controlled_leds(&phydev->mdio.dev, devicename,
+					      &phy_hw_controlled_led_ops);
+}
+
+#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
+
+static inline int of_phy_probe_leds(struct phy_device *phydev)
+{
+	return 0;
+}
+
+#endif /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
+
 /**
  * phy_probe - probe and init a PHY device
  * @dev: device to probe and init
@@ -2922,6 +3019,12 @@ static int phy_probe(struct device *dev)
 
 	mutex_unlock(&phydev->lock);
 
+	/* LEDs have to be registered with phydev mutex unlocked, because some operations can be
+	 * called during registration that lock the mutex themselves
+	 */
+	if (!err)
+		of_phy_probe_leds(phydev);
+
 	return err;
 }
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 52881e21ad951..8a4ab72c74dd4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -14,6 +14,7 @@
 #include <linux/compiler.h>
 #include <linux/spinlock.h>
 #include <linux/ethtool.h>
+#include <linux/leds-hw-controlled.h>
 #include <linux/linkmode.h>
 #include <linux/netlink.h>
 #include <linux/mdio.h>
@@ -741,6 +742,9 @@ struct phy_driver {
 	int (*set_loopback)(struct phy_device *dev, bool enable);
 	int (*get_sqi)(struct phy_device *dev);
 	int (*get_sqi_max)(struct phy_device *dev);
+
+	/* PHY connected and controlled LEDs */
+	const struct hw_controlled_led_ops *led_ops;
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
-- 
2.26.2

Re: [PATCH net-next + leds v2 5/7] net: phy: add support for LEDs controlled by ethernet PHY chips

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Wed 2020-09-09 18:25:50, Marek Behún wrote:
> This patch uses the new API for HW controlled LEDs to add support for
> probing and control of LEDs connected to an ethernet PHY chip.
> 
> A PHY driver wishing to utilize this API needs to implement the methods
> in struct hw_controlled_led_ops and set the member led_ops in struct
> phy_driver to point to that structure.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Marek Behún]

This patch adds support for controlling the LEDs connected to several
families of Marvell PHYs via the PHY HW LED trigger API. These families
are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
be added.

This patch does not yet add support for compound LED modes. This could
be achieved via the LED multicolor framework.

Settings such as HW blink rate or pulse stretch duration are not yet
supported.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/phy/marvell.c | 314 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 312 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bb86ac0bd0920..7aedb529e1540 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -148,6 +148,13 @@
 #define MII_88E1510_PHY_LED_DEF		0x1177
 #define MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE	0x1040
 
+#define MII_PHY_LED_POLARITY_CTRL	17
+#define MII_PHY_LED_TIMER_CTRL		18
+#define MII_PHY_LED45_CTRL		19
+
+#define MII_PHY_LED_CTRL_FORCE_ON	0x9
+#define MII_PHY_LED_CTRL_FORCE_OFF	0x8
+
 #define MII_M1011_PHY_STATUS		0x11
 #define MII_M1011_PHY_STATUS_1000	0x8000
 #define MII_M1011_PHY_STATUS_100	0x4000
@@ -252,6 +259,8 @@
 #define LPA_PAUSE_FIBER		0x180
 #define LPA_PAUSE_ASYM_FIBER	0x100
 
+#define MARVELL_PHY_MAX_LEDS	6
+
 #define NB_FIBER_STATS	1
 
 MODULE_DESCRIPTION("Marvell PHY driver");
@@ -280,6 +289,7 @@ struct marvell_priv {
 	u32 last;
 	u32 step;
 	s8 pair;
+	u16 legacy_led_config_mask;
 };
 
 static int marvell_read_page(struct phy_device *phydev)
@@ -662,8 +672,300 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
 	return err;
 }
 
+#if IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED)
+
+enum {
+	COMMON			= BIT(0),
+	L1V0_RECV		= BIT(1),
+	L1V0_COPPER		= BIT(2),
+	L1V5_100_FIBER		= BIT(3),
+	L1V5_100_10		= BIT(4),
+	L2V2_INIT		= BIT(5),
+	L2V2_PTP		= BIT(6),
+	L2V2_DUPLEX		= BIT(7),
+	L3V0_FIBER		= BIT(8),
+	L3V0_LOS		= BIT(9),
+	L3V5_TRANS		= BIT(10),
+	L3V7_FIBER		= BIT(11),
+	L3V7_DUPLEX		= BIT(12),
+};
+
+struct marvell_led_mode_info {
+	const char *name;
+	s8 regval[MARVELL_PHY_MAX_LEDS];
+	u32 flags;
+};
+
+static const struct marvell_led_mode_info marvell_led_mode_info[] = {
+	{ "link",			{ 0x0,  -1, 0x0,  -1,  -1,  -1, }, COMMON },
+	{ "link/act",			{ 0x1, 0x1, 0x1, 0x1, 0x1, 0x1, }, COMMON },
+	{ "1Gbps/100Mbps/10Mbps",	{ 0x2,  -1,  -1,  -1,  -1,  -1, }, COMMON },
+	{ "act",			{ 0x3, 0x3, 0x3, 0x3, 0x3, 0x3, }, COMMON },
+	{ "blink-act",			{ 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, }, COMMON },
+	{ "tx",				{ 0x5,  -1, 0x5,  -1, 0x5, 0x5, }, COMMON },
+	{ "tx",				{  -1,  -1,  -1, 0x5,  -1,  -1, }, L3V5_TRANS },
+	{ "rx",				{  -1,  -1,  -1,  -1, 0x0, 0x0, }, COMMON },
+	{ "rx",				{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_RECV },
+	{ "copper",			{ 0x6,  -1,  -1,  -1,  -1,  -1, }, COMMON },
+	{ "copper",			{  -1, 0x0,  -1,  -1,  -1,  -1, }, L1V0_COPPER },
+	{ "1Gbps",			{ 0x7,  -1,  -1,  -1,  -1,  -1, }, COMMON },
+	{ "link/rx",			{  -1, 0x2,  -1, 0x2, 0x2, 0x2, }, COMMON },
+	{ "100Mbps-fiber",		{  -1, 0x5,  -1,  -1,  -1,  -1, }, L1V5_100_FIBER },
+	{ "100Mbps-10Mbps",		{  -1, 0x5,  -1,  -1,  -1,  -1, }, L1V5_100_10 },
+	{ "1Gbps-100Mbps",		{  -1, 0x6,  -1,  -1,  -1,  -1, }, COMMON },
+	{ "1Gbps-10Mbps",		{  -1,  -1, 0x6, 0x6,  -1,  -1, }, COMMON },
+	{ "100Mbps",			{  -1, 0x7,  -1,  -1,  -1,  -1, }, COMMON },
+	{ "10Mbps",			{  -1,  -1, 0x7,  -1,  -1,  -1, }, COMMON },
+	{ "fiber",			{  -1,  -1,  -1, 0x0,  -1,  -1, }, L3V0_FIBER },
+	{ "fiber",			{  -1,  -1,  -1, 0x7,  -1,  -1, }, L3V7_FIBER },
+	{ "FullDuplex",			{  -1,  -1,  -1, 0x7,  -1,  -1, }, L3V7_DUPLEX },
+	{ "FullDuplex",			{  -1,  -1,  -1,  -1, 0x6, 0x6, }, COMMON },
+	{ "FullDuplex/collision",	{  -1,  -1,  -1,  -1, 0x7, 0x7, }, COMMON },
+	{ "FullDuplex/collision",	{  -1,  -1, 0x2,  -1,  -1,  -1, }, L2V2_DUPLEX },
+	{ "ptp",			{  -1,  -1, 0x2,  -1,  -1,  -1, }, L2V2_PTP },
+	{ "init",			{  -1,  -1, 0x2,  -1,  -1,  -1, }, L2V2_INIT },
+	{ "los",			{  -1,  -1,  -1, 0x0,  -1,  -1, }, L3V0_LOS },
+	{ "blink",			{ 0xb, 0xb, 0xb, 0xb, 0xb, 0xb, }, COMMON },
+};
+
+struct marvell_leds_info {
+	u32 family;
+	int nleds;
+	u32 flags;
+};
+
+#define LED(fam, n, flg)							\
+	{									\
+		.family = MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E##fam),	\
+		.nleds = (n),							\
+		.flags = (flg),							\
+	}									\
+
+static const struct marvell_leds_info marvell_leds_info[] = {
+	LED(1112,  4, COMMON | L1V0_COPPER | L1V5_100_FIBER | L2V2_INIT | L3V0_LOS | L3V5_TRANS |
+		      L3V7_FIBER),
+	LED(1121R, 3, COMMON | L1V5_100_10),
+	LED(1240,  6, COMMON | L3V5_TRANS),
+	LED(1340S, 6, COMMON | L1V0_COPPER | L1V5_100_FIBER | L2V2_PTP | L3V0_FIBER | L3V7_DUPLEX),
+	LED(1510,  3, COMMON | L1V0_RECV | L1V5_100_FIBER | L2V2_DUPLEX),
+	LED(1545,  6, COMMON | L1V0_COPPER | L1V5_100_FIBER | L3V0_FIBER | L3V7_DUPLEX),
+};
+
+static inline int marvell_led_reg(int led)
+{
+	switch (led) {
+	case 0 ... 3:
+		return MII_PHY_LED_CTRL;
+	case 4 ... 5:
+		return MII_PHY_LED45_CTRL;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int marvell_led_set_regval(struct phy_device *phydev, int led, u16 val)
+{
+	u16 mask;
+	int reg;
+
+	reg = marvell_led_reg(led);
+	if (reg < 0)
+		return reg;
+
+	val <<= (led % 4) * 4;
+	mask = 0xf << ((led % 4) * 4);
+
+	return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, reg, mask, val);
+}
+
+static int marvell_led_get_regval(struct phy_device *phydev, int led)
+{
+	int reg, val;
+
+	reg = marvell_led_reg(led);
+	if (reg < 0)
+		return reg;
+
+	val = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, reg);
+	if (val < 0)
+		return val;
+
+	val >>= (led % 4) * 4;
+	val &= 0xf;
+
+	return val;
+}
+
+static int marvell_led_set_polarity(struct phy_device *phydev, int led, bool active_low,
+				    bool tristate)
+{
+	int reg, shift;
+	u16 mask, val;
+
+	switch (led) {
+	case 0 ... 3:
+		reg = MII_PHY_LED_POLARITY_CTRL;
+		break;
+	case 4 ... 5:
+		reg = MII_PHY_LED45_CTRL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val = 0;
+	if (!active_low)
+		val |= BIT(0);
+	if (tristate)
+		val |= BIT(1);
+
+	shift = led * 2;
+	val <<= shift;
+	mask = 0x3 << shift;
+
+	return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, reg, mask, val);
+}
+
+static int marvell_led_brightness_set(struct device *dev, struct hw_controlled_led *led,
+				      enum led_brightness brightness)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	u8 val;
+
+	/* don't do anything if HW control is enabled */
+	if (led->cdev.trigger == &hw_control_led_trig)
+		return 0;
+
+	val = brightness ? MII_PHY_LED_CTRL_FORCE_ON : MII_PHY_LED_CTRL_FORCE_OFF;
+
+	return marvell_led_set_regval(phydev, led->addr, val);
+}
+
+static inline bool is_valid_led_mode(struct hw_controlled_led *led,
+				     const struct marvell_led_mode_info *mode)
+{
+	const struct marvell_leds_info *info = led->priv;
+
+	return mode->regval[led->addr] != -1 && (info->flags & mode->flags);
+}
+
+static const char *marvell_led_iter_hw_mode(struct device *dev, struct hw_controlled_led *led,
+					    void **iter)
+{
+	const struct marvell_led_mode_info *mode = *iter;
+
+	if (!mode)
+		mode = marvell_led_mode_info;
+
+	if (mode - marvell_led_mode_info == ARRAY_SIZE(marvell_led_mode_info))
+		goto end;
+
+	while (!is_valid_led_mode(led, mode)) {
+		++mode;
+		if (mode - marvell_led_mode_info == ARRAY_SIZE(marvell_led_mode_info))
+			goto end;
+	}
+
+	*iter = (void *)(mode + 1);
+	return mode->name;
+end:
+	*iter = NULL;
+	return NULL;
+}
+
+static int marvell_led_set_hw_mode(struct device *dev, struct hw_controlled_led *led,
+				   const char *name)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	const struct marvell_led_mode_info *mode;
+	int i;
+
+	if (!name)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_led_mode_info); ++i) {
+		mode = &marvell_led_mode_info[i];
+
+		if (!is_valid_led_mode(led, mode))
+			continue;
+
+		if (sysfs_streq(name, mode->name))
+			return marvell_led_set_regval(phydev, led->addr, mode->regval[led->addr]);
+	}
+
+	return -EINVAL;
+}
+
+static const char *marvell_led_get_hw_mode(struct device *dev, struct hw_controlled_led *led)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	const struct marvell_led_mode_info *mode;
+	int i, regval;
+
+	regval = marvell_led_get_regval(phydev, led->addr);
+	if (regval < 0)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_led_mode_info); ++i) {
+		mode = &marvell_led_mode_info[i];
+
+		if (!is_valid_led_mode(led, mode))
+			continue;
+
+		if (mode->regval[led->addr] == regval)
+			return mode->name;
+	}
+
+	return NULL;
+}
+
+static int marvell_led_init(struct device *dev, struct hw_controlled_led *led)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	const struct marvell_leds_info *info = NULL;
+	struct marvell_priv *priv = phydev->priv;
+	int ret, i;
+
+	for (i = 0; i < ARRAY_SIZE(marvell_leds_info); ++i) {
+		if (MARVELL_PHY_FAMILY_ID(phydev->phy_id) == marvell_leds_info[i].family) {
+			info = &marvell_leds_info[i];
+			break;
+		}
+	}
+
+	if (!info)
+		return -EOPNOTSUPP;
+
+	if (led->addr >= info->nleds)
+		return -EINVAL;
+
+	led->priv = (void *)info;
+	led->cdev.max_brightness = 1;
+
+	ret = marvell_led_set_polarity(phydev, led->addr, led->active_low, led->tristate);
+	if (ret < 0)
+		return ret;
+
+	/* ensure marvell_config_led below does not change settings we have set for this LED */
+	if (led->addr < 3)
+		priv->legacy_led_config_mask &= ~(0xf << (led->addr * 4));
+
+	return 0;
+}
+
+static const struct hw_controlled_led_ops marvell_led_ops = {
+	.led_init		= marvell_led_init,
+	.led_brightness_set	= marvell_led_brightness_set,
+	.led_iter_hw_mode	= marvell_led_iter_hw_mode,
+	.led_set_hw_mode	= marvell_led_set_hw_mode,
+	.led_get_hw_mode	= marvell_led_get_hw_mode,
+};
+
+#endif /* IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */
+
 static void marvell_config_led(struct phy_device *phydev)
 {
+	struct marvell_priv *priv = phydev->priv;
 	u16 def_config;
 	int err;
 
@@ -688,8 +990,9 @@ static void marvell_config_led(struct phy_device *phydev)
 		return;
 	}
 
-	err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
-			      def_config);
+	def_config &= priv->legacy_led_config_mask;
+	err = phy_modify_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
+			       priv->legacy_led_config_mask, def_config);
 	if (err < 0)
 		phydev_warn(phydev, "Fail to config marvell phy LED.\n");
 }
@@ -2580,6 +2883,7 @@ static int marvell_probe(struct phy_device *phydev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->legacy_led_config_mask = 0xffff;
 	phydev->priv = priv;
 
 	return 0;
@@ -2656,6 +2960,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		.led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111,
@@ -2717,6 +3022,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		.led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1318S,
@@ -2796,6 +3102,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_sset_count = marvell_get_sset_count,
 		.get_strings = marvell_get_strings,
 		.get_stats = marvell_get_stats,
+		.led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1116R,
@@ -2844,6 +3151,7 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_start = marvell_vct7_cable_test_start,
 		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+		.led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1540,
@@ -2896,6 +3204,7 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_start = marvell_vct7_cable_test_start,
 		.cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
+		.led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E3016,
@@ -2964,6 +3273,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
+		.led_ops = hw_controlled_led_ops_ptr(&marvell_led_ops),
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1548P,
-- 
2.26.2

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On Wed 2020-09-09 18:25:51, Marek Behún wrote:
> This patch adds support for controlling the LEDs connected to several
> families of Marvell PHYs via the PHY HW LED trigger API. These families
> are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
> be added.
> 
> This patch does not yet add support for compound LED modes. This could
> be achieved via the LED multicolor framework.
> 
> Settings such as HW blink rate or pulse stretch duration are not yet
> supported.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

I suggest limiting to "useful" hardware modes, and documenting what
those modes do somewhere.

Best regards,
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Andrew Lunn]

On Thu, Sep 10, 2020 at 02:23:41PM +0200, Pavel Machek wrote:
> On Wed 2020-09-09 18:25:51, Marek Behún wrote:
> > This patch adds support for controlling the LEDs connected to several
> > families of Marvell PHYs via the PHY HW LED trigger API. These families
> > are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
> > be added.
> > 
> > This patch does not yet add support for compound LED modes. This could
> > be achieved via the LED multicolor framework.
> > 
> > Settings such as HW blink rate or pulse stretch duration are not yet
> > supported.
> > 
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> 
> I suggest limiting to "useful" hardware modes, and documenting what
> those modes do somewhere.

I think to keep the YAML DT verification happy, they will need to be
listed in the marvell PHY binding documentation.

       Andrew

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Marek Behún]

On Thu, 10 Sep 2020 15:15:41 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Sep 10, 2020 at 02:23:41PM +0200, Pavel Machek wrote:
> > On Wed 2020-09-09 18:25:51, Marek Behún wrote:  
> > > This patch adds support for controlling the LEDs connected to
> > > several families of Marvell PHYs via the PHY HW LED trigger API.
> > > These families are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510
> > > and 88E1545. More can be added.
> > > 
> > > This patch does not yet add support for compound LED modes. This
> > > could be achieved via the LED multicolor framework.
> > > 
> > > Settings such as HW blink rate or pulse stretch duration are not
> > > yet supported.
> > > 
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>  
> > 
> > I suggest limiting to "useful" hardware modes, and documenting what
> > those modes do somewhere.  
> 
> I think to keep the YAML DT verification happy, they will need to be
> listed in the marvell PHY binding documentation.
> 
>        Andrew

Okay, so the netdev trigger offers modes `link`, `rx`, `tx`.
You can enable/disable either of these (via separate sysfs files). `rx`
and `tx` blink the LED, `link` turns the LED on if the interface is
linked.

The phy_led_trigger subsystem works differently. Instead of registering
one trigger (like netdev) it registers one trigger per PHY device and
per speed. So for a PHY with name XYZ and supported speeds 1Gbps,
100Mbps, 10Mbps it registers 3 triggers:
  XYZ:1Gbps XYZ:100Mbps XYZ:10Mbps

This is especially bad on a system where there are many PHYs and they
have long names derived from device tree path.

I propose that at least these HW modes should be available (and
documented) for ethernet PHY controlled LEDs:
  mode to determine link on:
    - `link`
  mode for activity (these should blink):
    - `activity` (both rx and tx), `rx`, `tx`
  mode for link (on) and activity (blink)
    - `link/activity`, maybe `link/rx` and `link/tx`
  mode for every supported speed:
    - `1Gbps`, `100Mbps`, `10Mbps`, ...
  mode for every supported cable type:
    - `copper`, `fiber`, ... (are there others?)
  mode that allows the user to determine link speed
    - `speed` (or maybe `linkspeed` ?)
    - on some Marvell PHYs the speed can be determined by how fast
      the LED is blinking (ie. 1Gbps blinks with default blinking
      frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps
      of half blinking frequency of 100Mbps)
    - on other Marvell PHYs this is instead:
      1Gpbs blinks 3 times, pause, 3 times, pause, ...
      100Mpbs blinks 2 times, pause, 2 times, pause, ...
      10Mpbs blinks 1 time, pause, 1 time, pause, ...
    - we don't need to differentiate these modes with different names,
      because the important thing is just that this mode allows the
      user to determine the speed from how the LED blinks
  mode to just force blinking
    - `blink`
The nice thing is that all this can be documented and done in software
as well.

Moreover I propose (and am willing to do) this:
  Rewrite phy_led_trigger so that it registers one trigger, `phydev`.
  The identifier of the PHY which should be source of the trigger can be
  set via a separate sysfs file, `device_name`, like in netdev trigger.
  The linked speed on which the trigger should light the LED will be
  selected via sysfs file `mode` (or do you propose another name?
  `trigger_on` or something?)

  Example:
    # cd /sys/class/leds/<LED>
    # echo phydev >trigger
    # echo XYZ >device_name
    # cat mode
    1Gbps 100Mbps 10Mbps
    # echo 1Gbps >mode
    # cat mode
    [1Gbps] 100Mbps 10Mbps

  Also the code should be moved from driver/net/phy to
  drivers/leds/trigger.

  The old API can be declared deprecated or removed, but outright
  removal may cause some people to complain.

What do you think?

Marek

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Andrew Lunn]

> Moreover I propose (and am willing to do) this:
>   Rewrite phy_led_trigger so that it registers one trigger, `phydev`.
>   The identifier of the PHY which should be source of the trigger can be
>   set via a separate sysfs file, `device_name`, like in netdev trigger.
>   The linked speed on which the trigger should light the LED will be
>   selected via sysfs file `mode` (or do you propose another name?
>   `trigger_on` or something?)
> 
>   Example:
>     # cd /sys/class/leds/<LED>
>     # echo phydev >trigger
>     # echo XYZ >device_name
>     # cat mode
>     1Gbps 100Mbps 10Mbps
>     # echo 1Gbps >mode
>     # cat mode
>     [1Gbps] 100Mbps 10Mbps
> 
>   Also the code should be moved from driver/net/phy to
>   drivers/leds/trigger.
> 
>   The old API can be declared deprecated or removed, but outright
>   removal may cause some people to complain.

This is ABI, so you cannot remove it, or change it. You can however
add to it, in a backwards compatible way.

    Andrew

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Andrew Lunn]

> I propose that at least these HW modes should be available (and
> documented) for ethernet PHY controlled LEDs:
>   mode to determine link on:
>     - `link`
>   mode for activity (these should blink):
>     - `activity` (both rx and tx), `rx`, `tx`
>   mode for link (on) and activity (blink)
>     - `link/activity`, maybe `link/rx` and `link/tx`
>   mode for every supported speed:
>     - `1Gbps`, `100Mbps`, `10Mbps`, ...
>   mode for every supported cable type:
>     - `copper`, `fiber`, ... (are there others?)

In theory, there is AUI and BNC, but no modern device will have these.

>   mode that allows the user to determine link speed
>     - `speed` (or maybe `linkspeed` ?)
>     - on some Marvell PHYs the speed can be determined by how fast
>       the LED is blinking (ie. 1Gbps blinks with default blinking
>       frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps
>       of half blinking frequency of 100Mbps)
>     - on other Marvell PHYs this is instead:
>       1Gpbs blinks 3 times, pause, 3 times, pause, ...
>       100Mpbs blinks 2 times, pause, 2 times, pause, ...
>       10Mpbs blinks 1 time, pause, 1 time, pause, ...
>     - we don't need to differentiate these modes with different names,
>       because the important thing is just that this mode allows the
>       user to determine the speed from how the LED blinks
>   mode to just force blinking
>     - `blink`
> The nice thing is that all this can be documented and done in software
> as well.

Have you checked include/dt-bindings/net/microchip-lan78xx.h and
mscc-phy-vsc8531.h ? If you are defining something generic, we need to
make sure the majority of PHYs can actually do it. There is no
standardization in this area. I'm sure there is some similarity, there
is only so many ways you can blink an LED, but i suspect we need a
mixture of standardized modes which we hope most PHYs implement, and
the option to support hardware specific modes.

    Andrew

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Matthias Schiffer]

On Thu, 2020-09-10 at 17:00 +0200, Andrew Lunn wrote:
> > I propose that at least these HW modes should be available (and
> > documented) for ethernet PHY controlled LEDs:
> >   mode to determine link on:
> >     - `link`
> >   mode for activity (these should blink):
> >     - `activity` (both rx and tx), `rx`, `tx`
> >   mode for link (on) and activity (blink)
> >     - `link/activity`, maybe `link/rx` and `link/tx`
> >   mode for every supported speed:
> >     - `1Gbps`, `100Mbps`, `10Mbps`, ...
> >   mode for every supported cable type:
> >     - `copper`, `fiber`, ... (are there others?)
> 
> In theory, there is AUI and BNC, but no modern device will have
> these.
> 
> >   mode that allows the user to determine link speed
> >     - `speed` (or maybe `linkspeed` ?)
> >     - on some Marvell PHYs the speed can be determined by how fast
> >       the LED is blinking (ie. 1Gbps blinks with default blinking
> >       frequency, 100Mbps with half blinking frequeny of 1Gbps,
> > 10Mbps
> >       of half blinking frequency of 100Mbps)
> >     - on other Marvell PHYs this is instead:
> >       1Gpbs blinks 3 times, pause, 3 times, pause, ...
> >       100Mpbs blinks 2 times, pause, 2 times, pause, ...
> >       10Mpbs blinks 1 time, pause, 1 time, pause, ...
> >     - we don't need to differentiate these modes with different
> > names,
> >       because the important thing is just that this mode allows the
> >       user to determine the speed from how the LED blinks
> >   mode to just force blinking
> >     - `blink`
> > The nice thing is that all this can be documented and done in
> > software
> > as well.
> 
> Have you checked include/dt-bindings/net/microchip-lan78xx.h and
> mscc-phy-vsc8531.h ? If you are defining something generic, we need
> to
> make sure the majority of PHYs can actually do it. There is no
> standardization in this area. I'm sure there is some similarity,
> there
> is only so many ways you can blink an LED, but i suspect we need a
> mixture of standardized modes which we hope most PHYs implement, and
> the option to support hardware specific modes.
> 
>     Andrew


FWIW, these are the LED HW trigger modes supported by the TI DP83867
PHY:

- Receive Error
- Receive Error or Transmit Error
- Link established, blink for transmit or receive activity
- Full duplex
- 100/1000BT link established
- 10/100BT link established
- 10BT link established
- 100BT link established
- 1000BT link established
- Collision detected
- Receive activity
- Transmit activity
- Receive or Transmit activity
- Link established

AFAIK, the "Link established, blink for transmit or receive activity"
is the only trigger that involves blinking; all other modes simply make
the LED light up when the condition is met. Setting the output level in
software is also possible.

Regarding the option to emulate unsupported HW triggers in software,
two questions come to my mind:

- Do all PHYs support manual setting of the LED level, or are the PHYs
that can only work with HW triggers?
- Is setting PHY registers always efficiently possible, or should SW
triggers be avoided in certain cases? I'm thinking about setups like
mdio-gpio. I guess this can only become an issue for triggers that
blink.


Kind regards,
Matthias

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Andrew Lunn]

> - Do all PHYs support manual setting of the LED level, or are the PHYs
> that can only work with HW triggers?

There are PHYs with do not have simple on/off.

> - Is setting PHY registers always efficiently possible, or should SW
> triggers be avoided in certain cases? I'm thinking about setups like
> mdio-gpio. I guess this can only become an issue for triggers that
> blink.

There are uses cases where not using software frequently writing
registers would be good. PTP time stamping is one, where the extra
jitter can reduce the accuracy of the clock.

I also think activity blinking in software is unlikely to be
accepted. Nothing extra is allowed in the hot path, when you can be
dealing with a million or more packets per second.

So i would say limit software fallback to link and speed, and don't
assume that is even possible depending on the hardware.

	Andrew

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Marek Behún]

On Fri, 11 Sep 2020 09:12:01 +0200
Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote:

> On Thu, 2020-09-10 at 17:00 +0200, Andrew Lunn wrote:
> > > I propose that at least these HW modes should be available (and
> > > documented) for ethernet PHY controlled LEDs:
> > >   mode to determine link on:
> > >     - `link`
> > >   mode for activity (these should blink):
> > >     - `activity` (both rx and tx), `rx`, `tx`
> > >   mode for link (on) and activity (blink)
> > >     - `link/activity`, maybe `link/rx` and `link/tx`
> > >   mode for every supported speed:
> > >     - `1Gbps`, `100Mbps`, `10Mbps`, ...
> > >   mode for every supported cable type:
> > >     - `copper`, `fiber`, ... (are there others?)  
> > 
> > In theory, there is AUI and BNC, but no modern device will have
> > these.
> >   
> > >   mode that allows the user to determine link speed
> > >     - `speed` (or maybe `linkspeed` ?)
> > >     - on some Marvell PHYs the speed can be determined by how fast
> > >       the LED is blinking (ie. 1Gbps blinks with default blinking
> > >       frequency, 100Mbps with half blinking frequeny of 1Gbps,
> > > 10Mbps
> > >       of half blinking frequency of 100Mbps)
> > >     - on other Marvell PHYs this is instead:
> > >       1Gpbs blinks 3 times, pause, 3 times, pause, ...
> > >       100Mpbs blinks 2 times, pause, 2 times, pause, ...
> > >       10Mpbs blinks 1 time, pause, 1 time, pause, ...
> > >     - we don't need to differentiate these modes with different
> > > names,
> > >       because the important thing is just that this mode allows
> > > the user to determine the speed from how the LED blinks
> > >   mode to just force blinking
> > >     - `blink`
> > > The nice thing is that all this can be documented and done in
> > > software
> > > as well.  
> > 
> > Have you checked include/dt-bindings/net/microchip-lan78xx.h and
> > mscc-phy-vsc8531.h ? If you are defining something generic, we need
> > to
> > make sure the majority of PHYs can actually do it. There is no
> > standardization in this area. I'm sure there is some similarity,
> > there
> > is only so many ways you can blink an LED, but i suspect we need a
> > mixture of standardized modes which we hope most PHYs implement, and
> > the option to support hardware specific modes.
> > 
> >     Andrew  
> 
> 
> FWIW, these are the LED HW trigger modes supported by the TI DP83867
> PHY:
> 
> - Receive Error
> - Receive Error or Transmit Error

Does somebody use this? I would just omit these.

> - Link established, blink for transmit or receive activity

`link/activity`

> - Full duplex

Not needed for now, I think.

> - 100/1000BT link established
> - 10/100BT link established

Disjunctive modes can go f*** themselves :)

> - 10BT link established
> - 100BT link established
> - 1000BT link established

`10Mbps`, `100Mbps`, `1Gbps`

> - Collision detected

Not needed for now.

> - Receive activity
> - Transmit activity

`rx/tx`

> - Receive or Transmit activity

`activity`

> - Link established

`link`

> 
> AFAIK, the "Link established, blink for transmit or receive activity"
> is the only trigger that involves blinking; all other modes simply
> make the LED light up when the condition is met. Setting the output
> level in software is also possible.
> 
> Regarding the option to emulate unsupported HW triggers in software,
> two questions come to my mind:
> 
> - Do all PHYs support manual setting of the LED level, or are the PHYs
> that can only work with HW triggers?
> - Is setting PHY registers always efficiently possible, or should SW
> triggers be avoided in certain cases? I'm thinking about setups like
> mdio-gpio. I guess this can only become an issue for triggers that
> blink.

The software trigger do not have to work with the LED connected to the
PHY. Any other LED on the system can be used. Only the information
about link and speed must come from the PHY, and kernel does have this
information already, either by polling or from interrupt.

> 
> 
> Kind regards,
> Matthias
> 

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Pavel Machek]

Hi!

> Okay, so the netdev trigger offers modes `link`, `rx`, `tx`.
> You can enable/disable either of these (via separate sysfs files). `rx`
> and `tx` blink the LED, `link` turns the LED on if the interface is
> linked.

I wonder if people really need separate rx and tx, but... this sounds
reasonable.

> The phy_led_trigger subsystem works differently. Instead of registering
> one trigger (like netdev) it registers one trigger per PHY device and
> per speed. So for a PHY with name XYZ and supported speeds 1Gbps,
> 100Mbps, 10Mbps it registers 3 triggers:
>   XYZ:1Gbps XYZ:100Mbps XYZ:10Mbps

That is not reasonable.

> I propose that at least these HW modes should be available (and
> documented) for ethernet PHY controlled LEDs:

Ok, and which of these will you actually use?

>   mode to determine link on:
>     - `link`
>   mode for activity (these should blink):
>     - `activity` (both rx and tx), `rx`, `tx`
>   mode for link (on) and activity (blink)
>     - `link/activity`, maybe `link/rx` and `link/tx`
>   mode for every supported speed:
>     - `1Gbps`, `100Mbps`, `10Mbps`, ...
>   mode for every supported cable type:
>     - `copper`, `fiber`, ... (are there others?)

That's ... way too many options.

Can we do it like netdev trigger? link? yes/no. rx? yes/no. tx? yes/no.

If displaying link only for certain speeds is useful, have link_min
and link_max, specifying values in Mbps? Default would be link_min ==
0, and link_max = 25000, so it would react on any link speed.

Is mode for cable type really useful? Then we should have link_fiber?
yes/no. link_copper? yes/no.

>   mode that allows the user to determine link speed
>     - `speed` (or maybe `linkspeed` ?)
>     - on some Marvell PHYs the speed can be determined by how fast
>       the LED is blinking (ie. 1Gbps blinks with default blinking
>       frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps
>       of half blinking frequency of 100Mbps)
>     - on other Marvell PHYs this is instead:
>       1Gpbs blinks 3 times, pause, 3 times, pause, ...
>       100Mpbs blinks 2 times, pause, 2 times, pause, ...
>       10Mpbs blinks 1 time, pause, 1 time, pause, ...
>     - we don't need to differentiate these modes with different names,
>       because the important thing is just that this mode allows the
>       user to determine the speed from how the LED blinks

I'd be very careful. Userspace should know what they are asking
for. I'd propose simply ignoring this feature.

>   mode to just force blinking - `blink`

We already have different support for blinking in LED subsystem. Lets use that.

Best regards,
									Pavel

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Andrew Lunn]

> We already have different support for blinking in LED subsystem. Lets use that.

You are assuming we have full software control of the LED, we can turn
it on and off. That is not always the case. But there is sometimes a
mode which the hardware blinks the LED.

Being able to blink the LED is useful:

ethtool(1):

       -p --identify

       Initiates adapter-specific action intended to enable an
       operator to easily identify the adapter by sight.  Typically
       this involves blinking one or more LEDs on the specific network
       port.

Once we get LED support in, i expect we will make use of this blink
mode for this ethtool option.

      Andrew

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Pavel Machek]

Hi!

> > We already have different support for blinking in LED subsystem. Lets use that.
> 
> You are assuming we have full software control of the LED, we can turn
> it on and off. That is not always the case. But there is sometimes a
> mode which the hardware blinks the LED.

Please see "timer" trigger support in the LED subsystem. We already
have hardware-accelerated blinking for the LEDs, and phy LEDs should
use same mechanism.

> Being able to blink the LED is useful: ethtool(1): -p --identify

...and yes, it should work for ethtool, too.

See leds-ss4200.c: nasgpio_led_set_blink() for an example. (But it may
not be good example).

Best regards,
								Pavel

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Marek Behun]

On Thu, 10 Sep 2020 22:23:45 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > Okay, so the netdev trigger offers modes `link`, `rx`, `tx`.
> > You can enable/disable either of these (via separate sysfs files). `rx`
> > and `tx` blink the LED, `link` turns the LED on if the interface is
> > linked.  
> 
> I wonder if people really need separate rx and tx, but... this sounds
> reasonable.
> 
> > The phy_led_trigger subsystem works differently. Instead of registering
> > one trigger (like netdev) it registers one trigger per PHY device and
> > per speed. So for a PHY with name XYZ and supported speeds 1Gbps,
> > 100Mbps, 10Mbps it registers 3 triggers:
> >   XYZ:1Gbps XYZ:100Mbps XYZ:10Mbps  
> 
> That is not reasonable.
> 
> > I propose that at least these HW modes should be available (and
> > documented) for ethernet PHY controlled LEDs:  
> 
> Ok, and which of these will you actually use?
> 
> >   mode to determine link on:
> >     - `link`
> >   mode for activity (these should blink):
> >     - `activity` (both rx and tx), `rx`, `tx`
> >   mode for link (on) and activity (blink)
> >     - `link/activity`, maybe `link/rx` and `link/tx`
> >   mode for every supported speed:
> >     - `1Gbps`, `100Mbps`, `10Mbps`, ...
> >   mode for every supported cable type:
> >     - `copper`, `fiber`, ... (are there others?)  
> 
> That's ... way too many options.
> 
> Can we do it like netdev trigger? link? yes/no. rx? yes/no. tx? yes/no.
> 
> If displaying link only for certain speeds is useful, have link_min
> and link_max, specifying values in Mbps? Default would be link_min ==
> 0, and link_max = 25000, so it would react on any link speed.
> 
> Is mode for cable type really useful? Then we should have link_fiber?
> yes/no. link_copper? yes/no.
> 

I want to put the speed differentiating mode by default on MOX on one
LED, and activity on other LED.

I think there are devices which have written on the case next to the
LED that this LED is on for this specific speed, that LED is on for
other specific speed. So modes for speed are reasonable, I think.

In my opinion the disjunctive modes the Marvell PHYs support are useless
(like ON when 1000Mbps or 10Mbps).

You can't have link_min and link_max setting. The hardware does not
support it this way. You can tell the hardware to light the LED when
linked on a specific speed, and this is actually used on some devices
(as I have written above, some devices have this written on the case).

In my opinion the set `link`, `link/activity`, `activity`, `speed`,
and one mode for each supported speed on the PHY is reasonable. This could
be also compatible with software triggering via the proposed phydev
trigger.

> >   mode that allows the user to determine link speed
> >     - `speed` (or maybe `linkspeed` ?)
> >     - on some Marvell PHYs the speed can be determined by how fast
> >       the LED is blinking (ie. 1Gbps blinks with default blinking
> >       frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps
> >       of half blinking frequency of 100Mbps)
> >     - on other Marvell PHYs this is instead:
> >       1Gpbs blinks 3 times, pause, 3 times, pause, ...
> >       100Mpbs blinks 2 times, pause, 2 times, pause, ...
> >       10Mpbs blinks 1 time, pause, 1 time, pause, ...
> >     - we don't need to differentiate these modes with different names,
> >       because the important thing is just that this mode allows the
> >       user to determine the speed from how the LED blinks  
> 
> I'd be very careful. Userspace should know what they are asking
> for. I'd propose simply ignoring this feature.

As I wrote above, I think this mode is rather useful when you have just
two LEDs for a port. You can tell speed by looking on one LED and
activity by looking at the other LED. And I want to set this as default
on Turris MOX.

> >   mode to just force blinking - `blink`  
> 
> We already have different support for blinking in LED subsystem. Lets use that.
> 
> Best regards,
> 									Pavel

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Thu 2020-09-10 15:15:41, Andrew Lunn wrote:
> On Thu, Sep 10, 2020 at 02:23:41PM +0200, Pavel Machek wrote:
> > On Wed 2020-09-09 18:25:51, Marek Behún wrote:
> > > This patch adds support for controlling the LEDs connected to several
> > > families of Marvell PHYs via the PHY HW LED trigger API. These families
> > > are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
> > > be added.
> > > 
> > > This patch does not yet add support for compound LED modes. This could
> > > be achieved via the LED multicolor framework.
> > > 
> > > Settings such as HW blink rate or pulse stretch duration are not yet
> > > supported.
> > > 
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > 
> > I suggest limiting to "useful" hardware modes, and documenting what
> > those modes do somewhere.
> 
> I think to keep the YAML DT verification happy, they will need to be
> listed in the marvell PHY binding documentation.

Well, this should really go to the sysfs documenation. Not sure what
to do with DT.

But perhaps driver can set reasonable defaults without DT input?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Andrew Lunn]

On Thu, Sep 10, 2020 at 08:24:34PM +0200, Pavel Machek wrote:
> On Thu 2020-09-10 15:15:41, Andrew Lunn wrote:
> > On Thu, Sep 10, 2020 at 02:23:41PM +0200, Pavel Machek wrote:
> > > On Wed 2020-09-09 18:25:51, Marek Behún wrote:
> > > > This patch adds support for controlling the LEDs connected to several
> > > > families of Marvell PHYs via the PHY HW LED trigger API. These families
> > > > are: 88E1112, 88E1121R, 88E1240, 88E1340S, 88E1510 and 88E1545. More can
> > > > be added.
> > > > 
> > > > This patch does not yet add support for compound LED modes. This could
> > > > be achieved via the LED multicolor framework.
> > > > 
> > > > Settings such as HW blink rate or pulse stretch duration are not yet
> > > > supported.
> > > > 
> > > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > 
> > > I suggest limiting to "useful" hardware modes, and documenting what
> > > those modes do somewhere.
> > 
> > I think to keep the YAML DT verification happy, they will need to be
> > listed in the marvell PHY binding documentation.
> 
> Well, this should really go to the sysfs documenation. Not sure what
> to do with DT.

In DT you can set how you want the LED to blink by default. I expect
that will be the most frequent use cases, and nearly nobody will
change it at runtime.

> But perhaps driver can set reasonable defaults without DT input?

Generally the driver will default to the hardware reset blink
pattern. There are a few PHY drivers which change this at probe, but
not many. The silicon defaults are pretty good.

    Andrew

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Russell King - ARM Linux admin]

On Thu, Sep 10, 2020 at 08:31:54PM +0200, Andrew Lunn wrote:
> Generally the driver will default to the hardware reset blink
> pattern. There are a few PHY drivers which change this at probe, but
> not many. The silicon defaults are pretty good.

The "right" blink pattern can be a matter of how the hardware is
wired.  For example, if you have bi-colour LEDs and the PHY supports
special bi-colour mixing modes.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Marek Behun]

On Thu, 10 Sep 2020 19:34:35 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Thu, Sep 10, 2020 at 08:31:54PM +0200, Andrew Lunn wrote:
> > Generally the driver will default to the hardware reset blink
> > pattern. There are a few PHY drivers which change this at probe, but
> > not many. The silicon defaults are pretty good.  
> 
> The "right" blink pattern can be a matter of how the hardware is
> wired.  For example, if you have bi-colour LEDs and the PHY supports
> special bi-colour mixing modes.
> 

Have you seen such, Russell? This could be achieved via the multicolor
LED framework, but I don't have a device which uses such LEDs, so I
did not write support for this in the Marvell PHY driver.

(I guess I could test it though, since on my device LED0 and LED1
are used, and this to can be put into bi-colour LED mode.)

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Russell King - ARM Linux admin]

On Thu, Sep 10, 2020 at 10:31:12PM +0200, Marek Behun wrote:
> On Thu, 10 Sep 2020 19:34:35 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Thu, Sep 10, 2020 at 08:31:54PM +0200, Andrew Lunn wrote:
> > > Generally the driver will default to the hardware reset blink
> > > pattern. There are a few PHY drivers which change this at probe, but
> > > not many. The silicon defaults are pretty good.  
> > 
> > The "right" blink pattern can be a matter of how the hardware is
> > wired.  For example, if you have bi-colour LEDs and the PHY supports
> > special bi-colour mixing modes.
> > 
> 
> Have you seen such, Russell? This could be achieved via the multicolor
> LED framework, but I don't have a device which uses such LEDs, so I
> did not write support for this in the Marvell PHY driver.
> 
> (I guess I could test it though, since on my device LED0 and LED1
> are used, and this to can be put into bi-colour LED mode.)

I haven't, much to my dismay. The Macchiatobin would have been ideal -
the 10G RJ45s have bi-colour on one side and green on the other. It
would have been useful if they were wired to support the PHYs bi-
colour mode.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

[Marek Behún]

On Thu, 10 Sep 2020 22:44:54 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Thu, Sep 10, 2020 at 10:31:12PM +0200, Marek Behun wrote:
> > On Thu, 10 Sep 2020 19:34:35 +0100
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> >   
> > > On Thu, Sep 10, 2020 at 08:31:54PM +0200, Andrew Lunn wrote:  
> > > > Generally the driver will default to the hardware reset blink
> > > > pattern. There are a few PHY drivers which change this at
> > > > probe, but not many. The silicon defaults are pretty good.    
> > > 
> > > The "right" blink pattern can be a matter of how the hardware is
> > > wired.  For example, if you have bi-colour LEDs and the PHY
> > > supports special bi-colour mixing modes.
> > >   
> > 
> > Have you seen such, Russell? This could be achieved via the
> > multicolor LED framework, but I don't have a device which uses such
> > LEDs, so I did not write support for this in the Marvell PHY driver.
> > 
> > (I guess I could test it though, since on my device LED0 and LED1
> > are used, and this to can be put into bi-colour LED mode.)  
> 
> I haven't, much to my dismay. The Macchiatobin would have been ideal -
> the 10G RJ45s have bi-colour on one side and green on the other. It
> would have been useful if they were wired to support the PHYs bi-
> colour mode.
> 

I have access to a Macchiatobin here at work. I am willing to add
support for bicolor LEDs, but only after we solve and merge this first
proposal.

Marek

[PATCH net-next + mvebu v2 7/7] arm64: dts: armada-3720-turris-mox: add nodes for ethernet PHY LEDs

[Marek Behún]

Add nodes for the green and yellow LEDs that are connected to the
ethernet PHY chip on Turris MOX A.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 .../dts/marvell/armada-3720-turris-mox.dts    | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
index f3a678e0fd99b..6da03b6c69c0a 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -9,6 +9,7 @@
 #include <dt-bindings/bus/moxtet.h>
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>
 #include "armada-372x.dtsi"
 
 / {
@@ -273,6 +274,28 @@ &mdio {
 
 	phy1: ethernet-phy@1 {
 		reg = <1>;
+
+		leds {
+			compatible = "linux,hw-controlled-leds";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@0 {
+				reg = <0>;
+				color = <LED_COLOR_ID_GREEN>;
+				function = LED_FUNCTION_STATUS;
+				linux,default-trigger = "dev-hw-mode";
+				linux,default-hw-mode = "1Gbps/100Mbps/10Mbps";
+			};
+
+			led@1 {
+				reg = <1>;
+				color = <LED_COLOR_ID_YELLOW>;
+				function = LED_FUNCTION_ACTIVITY;
+				linux,default-trigger = "dev-hw-mode";
+				linux,default-hw-mode = "blink-act";
+			};
+		};
 	};
 
 	/* switch nodes are enabled by U-Boot if modules are present */
-- 
2.26.2

Re: [PATCH net-next + mvebu v2 7/7] arm64: dts: armada-3720-turris-mox: add nodes for ethernet PHY LEDs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

Hi!

> Add nodes for the green and yellow LEDs that are connected to the
> ethernet PHY chip on Turris MOX A.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  .../dts/marvell/armada-3720-turris-mox.dts    | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> index f3a678e0fd99b..6da03b6c69c0a 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> @@ -9,6 +9,7 @@
>  #include <dt-bindings/bus/moxtet.h>
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
>  #include "armada-372x.dtsi"
>  
>  / {
> @@ -273,6 +274,28 @@ &mdio {
>  
>  	phy1: ethernet-phy@1 {
>  		reg = <1>;
> +
> +		leds {
> +			compatible = "linux,hw-controlled-leds";

I don't believe this is suitable compatible.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH net-next + leds v2 0/7] PLEASE REVIEW: Add support for LEDs on Marvell PHYs

[Andrew Lunn]

On Wed, Sep 09, 2020 at 06:25:45PM +0200, Marek Behún wrote:
> Hello Andrew and Pavel,
> 
> please review these patches adding support for HW controlled LEDs.
> The main difference from previous version is that the API is now generalized
> and lives in drivers/leds, so that part needs to be reviewed (and maybe even
> applied) by Pavel.
> 
> As discussed previously between you two, I made it so that the devicename
> part of the LED is now in the form `ethernet-phy%i` when the LED is probed
> for an ethernet PHY. Userspace utility wanting to control LEDs for a specific
> network interface can access them via /sys/class/net/eth0/phydev/leds:
> 
>   mox ~ # ls /sys/class/net/eth0/phydev/leds

It is nice they are directly in /sys/class/net/eth0/phydev. Do they
also appear in /sys/class/led?

Have you played with network namespaces? What happens with

ip netns add ns1

ip link set eth0 netns ns1

   Andrew

Re: [PATCH net-next + leds v2 0/7] PLEASE REVIEW: Add support for LEDs on Marvell PHYs

[Marek Behun]

On Wed, 9 Sep 2020 23:42:59 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, Sep 09, 2020 at 06:25:45PM +0200, Marek Behún wrote:
> > Hello Andrew and Pavel,
> > 
> > please review these patches adding support for HW controlled LEDs.
> > The main difference from previous version is that the API is now generalized
> > and lives in drivers/leds, so that part needs to be reviewed (and maybe even
> > applied) by Pavel.
> > 
> > As discussed previously between you two, I made it so that the devicename
> > part of the LED is now in the form `ethernet-phy%i` when the LED is probed
> > for an ethernet PHY. Userspace utility wanting to control LEDs for a specific
> > network interface can access them via /sys/class/net/eth0/phydev/leds:
> > 
> >   mox ~ # ls /sys/class/net/eth0/phydev/leds  
> 
> It is nice they are directly in /sys/class/net/eth0/phydev. Do they
> also appear in /sys/class/led?

They are in /sys/class/net/eth0/phydev/leds by default, because they
are children of the PHY device and are of `leds` class, and the PHY
subsystem creates a symlink `phydev` when PHY is attached to the
interface.
They are in /sys/class/leds/ as symlinks, because AFAIK everything in
/sys/class/<CLASS>/ is a symlink...

> Have you played with network namespaces? What happens with
> 
> ip netns add ns1
> 
> ip link set eth0 netns ns1
> 
>    Andrew

If you move eth0 to other network namespace, naturally the
/sys/class/net/eth0 symlink will disappear, as will the directory it
pointed to.

The symlink phydev does will disappear from /sys/class/net/eth0/
directory after eth0 is moved to ns1, and is lost. It does not return
even if eth0 is moved back to root namespace.

The LED will of course stay in ns1 and also in root namespace, as will
the phydev the LED is a child to. But they are no longer accessible via
/sys/class/net/eth0, instead you can access the LEDs either via
/sys/class/leds or /sys/class/mdio_bus/<MDIO_BUS>/<PHY>/leds, or
without symlinks via /sys/devices/ tree.

Marek

Re: [PATCH net-next + leds v2 0/7] PLEASE REVIEW: Add support for LEDs on Marvell PHYs

[Andrew Lunn]

> They are in /sys/class/net/eth0/phydev/leds by default, because they
> are children of the PHY device and are of `leds` class, and the PHY
> subsystem creates a symlink `phydev` when PHY is attached to the
> interface.
> They are in /sys/class/leds/ as symlinks, because AFAIK everything in
> /sys/class/<CLASS>/ is a symlink...
> 
> > Have you played with network namespaces? What happens with
> > 
> > ip netns add ns1
> > 
> > ip link set eth0 netns ns1
> > 
> >    Andrew
> 
> If you move eth0 to other network namespace, naturally the
> /sys/class/net/eth0 symlink will disappear, as will the directory it
> pointed to.
> 
> The symlink phydev does will disappear from /sys/class/net/eth0/
> directory after eth0 is moved to ns1, and is lost. It does not return
> even if eth0 is moved back to root namespace.

Yes, i just played with this. I would say that is an existing bug. The
link would still work in the namespace, since the mdio_bus is not net
namespace aware and remains accessible from all namespaces.

	Andrew