| Message ID | 20231128232135.358638-1-andrew@lunn.ch (mailing list archive) |
|---|---|
| Headers | show |
| Series | DSA LED infrastructure, mv88e6xxx and QCA8K | expand |
On Wed, Nov 29, 2023 at 12:21:27AM +0100, Andrew Lunn wrote: > This patchset extends the DSA core to add support for port LEDs being > controlled via sys/class/leds, and offloading blinking via > ledtrig-netdev. The core parses the device tree binding, and registers > LEDs. The DSA switch ops structure is extended with the needed > functions. > > The mv88e6xxx support is partially added. Support for setting the > brightness and blinking is provided, but offloading of blinking is not > yet available. To demonstrate this, the wrt1900ac device tree is > extended with LEDs. > > The existing QCA8K code is refactored to make use of this shared code. > > RFC: > > Linus, can you rework your code into this for offloading blinking ? > And test with ports 5 & 6. > > Christian: Please test QCA8K. I would not be surprised if there is an > off-by-one. Good news! I tested this and with the requested change, the thing works correctly. > > This code can also be found in > > https://github.com/lunn/ v6.7-rc2-net-next-mv88e6xxx-leds > > Andrew Lunn (8): > net: dsa: mv88e6xxx: Add helpers for 6352 LED blink and brightness > net: dsa: mv88e6xxx: Tie the low level LED functions to device ops > net: dsa: Plumb LED brightnes and blink into switch API > dsa: Create port LEDs based on DT binding > dsa: Plumb in LED calls needed for hardware offload > dsa: mv88e6xxx: Plumb in LED offload functions > arm: boot: dts: mvebu: linksys-mamba: Add Ethernet LEDs > dsa: qca8k: Use DSA common code for LEDs > > .../dts/marvell/armada-xp-linksys-mamba.dts | 66 +++++ > drivers/net/dsa/mv88e6xxx/chip.c | 103 +++++++ > drivers/net/dsa/mv88e6xxx/chip.h | 14 + > drivers/net/dsa/mv88e6xxx/port.c | 99 +++++++ > drivers/net/dsa/mv88e6xxx/port.h | 76 +++++- > drivers/net/dsa/qca/qca8k-8xxx.c | 11 +- > drivers/net/dsa/qca/qca8k-leds.c | 255 +++--------------- > drivers/net/dsa/qca/qca8k.h | 9 - > drivers/net/dsa/qca/qca8k_leds.h | 21 +- > include/net/dsa.h | 17 ++ > net/dsa/dsa.c | 190 +++++++++++++ > 11 files changed, 620 insertions(+), 241 deletions(-) > > -- > 2.42.0 >
Hi Andrew, On Wed, Nov 29, 2023 at 12:21:27AM +0100, Andrew Lunn wrote: > This patchset extends the DSA core to add support for port LEDs being > controlled via sys/class/leds, and offloading blinking via > ledtrig-netdev. The core parses the device tree binding, and registers > LEDs. The DSA switch ops structure is extended with the needed > functions. > > The mv88e6xxx support is partially added. Support for setting the > brightness and blinking is provided, but offloading of blinking is not > yet available. To demonstrate this, the wrt1900ac device tree is > extended with LEDs. > > The existing QCA8K code is refactored to make use of this shared code. > > RFC: > > Linus, can you rework your code into this for offloading blinking ? > And test with ports 5 & 6. > > Christian: Please test QCA8K. I would not be surprised if there is an > off-by-one. > > This code can also be found in > > https://github.com/lunn/ v6.7-rc2-net-next-mv88e6xxx-leds I am disappointed to see the dsa_switch_ops API polluted with odds and ends which have nothing to do with Ethernet-connected Ethernet switches (DSA's focus). Looking at the code, I don't see why dsa_port_leds_setup() cannot be rebranded as library code usable by any netdev driver and which bypasses DSA. Individual DSA switch drivers could call it directly while providing their struct device for the port, and a smaller ops structure for the cdev. But more importantly, other non-DSA drivers could do the same. I think it comes as no surprise that driver authors prefer using the DSA API as their first choice even for technically non-DSA switches, seeing how we tend to cram all sorts of unrelated stuff into the monolithic struct dsa_switch_ops, and how that makes the API attractive. But then we push them away from DSA for valid reasons, and they end up copying its support code word for word. Maybe this sounds a bit harsh, but NACK from me for the approach.
> I am disappointed to see the dsa_switch_ops API polluted with odds and > ends which have nothing to do with Ethernet-connected Ethernet switches > (DSA's focus). > > Looking at the code, I don't see why dsa_port_leds_setup() cannot be > rebranded as library code usable by any netdev driver and which bypasses DSA. > Individual DSA switch drivers could call it directly while providing > their struct device for the port, and a smaller ops structure for the > cdev. But more importantly, other non-DSA drivers could do the same. > > I think it comes as no surprise that driver authors prefer using the DSA > API as their first choice even for technically non-DSA switches, seeing > how we tend to cram all sorts of unrelated stuff into the monolithic > struct dsa_switch_ops, and how that makes the API attractive. But then > we push them away from DSA for valid reasons, and they end up copying > its support code word for word. O.K, i need to think about this. What is not obvious to me at the moment is how we glue the bits together. I don't want each DSA driver having to parse the DSA part of the DT representation. So the DSA core needs to call into this library while parsing the DT to create the LEDs. We also need an ops structure in the DSA driver which this library can use. We then need to associate the ops structure the driver has with the LEDs the DSA core creates in the library. Maybe we can use ds->dev as a cookie. Before i get too deep into code, i will post the basic API idea for a quick review. Andrew
On Wed, Nov 29, 2023 at 04:13:00PM +0100, Andrew Lunn wrote: > O.K, i need to think about this. > > What is not obvious to me at the moment is how we glue the bits > together. I don't want each DSA driver having to parse the DSA part of > the DT representation. So the DSA core needs to call into this library > while parsing the DT to create the LEDs. We also need an ops structure > in the DSA driver which this library can use. We then need to > associate the ops structure the driver has with the LEDs the DSA core > creates in the library. Maybe we can use ds->dev as a cookie. > > Before i get too deep into code, i will post the basic API idea for a > quick review. What is the DSA portion of the DT representation? I see "leds" goes under the generic ethernet-controller.yaml.
On Wed, Nov 29, 2023 at 05:43:36PM +0200, Vladimir Oltean wrote: > On Wed, Nov 29, 2023 at 04:13:00PM +0100, Andrew Lunn wrote: > > O.K, i need to think about this. > > > > What is not obvious to me at the moment is how we glue the bits > > together. I don't want each DSA driver having to parse the DSA part of > > the DT representation. So the DSA core needs to call into this library > > while parsing the DT to create the LEDs. We also need an ops structure > > in the DSA driver which this library can use. We then need to > > associate the ops structure the driver has with the LEDs the DSA core > > creates in the library. Maybe we can use ds->dev as a cookie. > > > > Before i get too deep into code, i will post the basic API idea for a > > quick review. > > What is the DSA portion of the DT representation? I see "leds" goes > under the generic ethernet-controller.yaml. I agree the properties are well defined. The problem is finding them. switch@0 { compatible = "marvell,mv88e6085"; #address-cells = <1>; #size-cells = <0>; reg = <0>; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; label = "lan4"; leds { #address-cells = <1>; #size-cells = <0>; led@0 { reg = <0>; color = <LED_COLOR_ID_WHITE>; function = LED_FUNCTION_LAN; label = "front"; default-state = "keep"; }; }; }; port@1 { reg = <1>; label = "lan3"; leds { #address-cells = <1>; #size-cells = <0>; led@0 { reg = <0>; color = <LED_COLOR_ID_WHITE>; function = LED_FUNCTION_LAN; label = "front"; default-state = "keep"; }; }; }; I don't want each DSA driver having to walk this tree to find the leds node to pass it to a library to create the LEDs. We already have code do to this walk in the DSA core. So one option would be the DSA core does the call to the library as it performs the walk. Now that i've looked at the code, the core does set dp->dn to point to the port node. So setup_port() could do the call into the library to create the LEDs, and pass it the ops structure. That seems clean, and should avoid DSA core changes you don't like. Andrew
On Wed, Nov 29, 2023 at 05:27:00PM +0100, Andrew Lunn wrote: > I don't want each DSA driver having to walk this tree to find the leds > node to pass it to a library to create the LEDs. We already have code > do to this walk in the DSA core. So one option would be the DSA core > does the call to the library as it performs the walk. > > Now that i've looked at the code, the core does set dp->dn to point to > the port node. So setup_port() could do the call into the library to > create the LEDs, and pass it the ops structure. That seems clean, and > should avoid DSA core changes you don't like. > > Andrew Yeah, there's nothing to find, they're already found, and available in of_fwnode_handle(dp->dn) during any ds->ops method you wish. The library code for netdev LEDs can take a reference on this fwnode for as long as it wants. Absolutely not a reason to call back into the DSA framework.
On Wed, Nov 29, 2023 at 12:21 AM Andrew Lunn <andrew@lunn.ch> wrote: > Linus, can you rework your code into this for offloading blinking ? > And test with ports 5 & 6. I see Vladimir wants some reworking of it into a stand-alone library, so I will wait a while until it he seems happy, then I will surely do this :) Yours, Linus Walleij