| Message ID | 20230307170046.28917-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | net: Add basic LED support for switch/phy | expand |
On Wed, Mar 08, 2023 at 12:16:13AM +0100, Andrew Lunn wrote: > > +qca8k_setup_led_ctrl(struct qca8k_priv *priv) > > +{ > > + struct fwnode_handle *ports, *port; > > + int port_num; > > + int ret; > > + > > + ports = device_get_named_child_node(priv->dev, "ports"); > > + if (!ports) { > > + dev_info(priv->dev, "No ports node specified in device tree!\n"); > > + return 0; > > + } > > + > > + fwnode_for_each_child_node(ports, port) { > > + struct fwnode_handle *phy_node, *reg_port_node = port; > > + > > + phy_node = fwnode_find_reference(port, "phy-handle", 0); > > + if (!IS_ERR(phy_node)) > > + reg_port_node = phy_node; > > I don't understand this bit. Why are you looking at the phy-handle? > > > + > > + if (fwnode_property_read_u32(reg_port_node, "reg", &port_num)) > > + continue; > > I would of expect port, not reg_port_node. I'm missing something > here.... > It's really not to implement ugly things like "reg - 1" On qca8k the port index goes from 0 to 6. 0 is cpu port 1 1 is port0 at mdio reg 0 2 is port1 at mdio reg 1 ... 6 is cpu port 2 Each port have a phy-handle that refer to a phy node with the correct reg and that reflect the correct port index. Tell me if this looks wrong, for qca8k we have qca8k_port_to_phy() and at times we introduced the mdio thing to describe the port - 1 directly in DT. If needed I can drop the additional fwnode and use this function but I would love to use what is defined in DT thatn a simple - 1.
On Wed, Mar 08, 2023 at 12:17:51AM +0100, Andrew Lunn wrote: > On Tue, Mar 07, 2023 at 06:00:38PM +0100, Christian Marangi wrote: > > From: Andrew Lunn <andrew@lunn.ch> > > > > Define common binding parsing for all PHY drivers with LEDs using > > phylib. Parse the DT as part of the phy_probe and add LEDs to the > > linux LED class infrastructure. For the moment, provide a dummy > > brightness function, which will later be replaced with a call into the > > PHY driver. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Hi Christian > > Since you are submitting this, you need to add your own Signed-off-by: > after mine. > Tought it was needed only for patch where I have put any change. (case of 2 patch in this series where there was a whitespace error and had to change a binding) Think I need do to this for every other patch right?
On Wed, Mar 08, 2023 at 12:20:17AM +0100, Andrew Lunn wrote: > On Tue, Mar 07, 2023 at 06:00:46PM +0100, Christian Marangi wrote: > > From: Andrew Lunn <andrew@lunn.ch> > > > > The WAN port of the 370-RD has a Marvell PHY, with one LED on > > the front panel. List this LED in the device tree. > > > > Set the LED default state to "keep" to not change any blink rule > > set by default. > > Hi Christian > > What board are you using for testing? It would be good to patch its > .dts file to enable its LEDs. We don't normally had new code without a > user. > ipq806x, my specific device is not present upstream but we have a rb3011 that currently use the qca8k dsa switch upstream. If needed I can add support there by checking the leds supported... Also looking at the dt I think I need to use "port - 1" after all since it does use legacy way... Sad me...
On Wed, Mar 08, 2023 at 01:49:55AM +0100, Andrew Lunn wrote: > On Tue, Mar 07, 2023 at 06:57:10PM +0100, Christian Marangi wrote: > > On Wed, Mar 08, 2023 at 12:16:13AM +0100, Andrew Lunn wrote: > > > > +qca8k_setup_led_ctrl(struct qca8k_priv *priv) > > > > +{ > > > > + struct fwnode_handle *ports, *port; > > > > + int port_num; > > > > + int ret; > > > > + > > > > + ports = device_get_named_child_node(priv->dev, "ports"); > > > > + if (!ports) { > > > > + dev_info(priv->dev, "No ports node specified in device tree!\n"); > > > > + return 0; > > > > + } > > > > + > > > > + fwnode_for_each_child_node(ports, port) { > > > > + struct fwnode_handle *phy_node, *reg_port_node = port; > > > > + > > > > + phy_node = fwnode_find_reference(port, "phy-handle", 0); > > > > + if (!IS_ERR(phy_node)) > > > > + reg_port_node = phy_node; > > > > > > I don't understand this bit. Why are you looking at the phy-handle? > > > > > > > + > > > > + if (fwnode_property_read_u32(reg_port_node, "reg", &port_num)) > > > > + continue; > > > > > > I would of expect port, not reg_port_node. I'm missing something > > > here.... > > > > > > > It's really not to implement ugly things like "reg - 1" > > > > On qca8k the port index goes from 0 to 6. > > 0 is cpu port 1 > > 1 is port0 at mdio reg 0 > > 2 is port1 at mdio reg 1 > > ... > > 6 is cpu port 2 > > > > Each port have a phy-handle that refer to a phy node with the correct > > reg and that reflect the correct port index. > > > > Tell me if this looks wrong, for qca8k we have qca8k_port_to_phy() and > > at times we introduced the mdio thing to describe the port - 1 directly > > in DT. If needed I can drop the additional fwnode and use this function > > but I would love to use what is defined in DT thatn a simple - 1. > > This comes back to the off list discussion earlier today. What you > actually have here are MAC LEDs, not PHY LEDs. They are implemented in > the MAC, not the PHY. To the end user, it should not matter, they > blink when you would expect. > > So your addressing should be based around the MAC port number, not the > PHY. Ok will drop this. > > Also, at the moment, all we are adding are a bunch of LEDs. There is > no link to a netdev at this point. At least, i don't see one. Be once > we start using ledtrig-netdev we will need that link to a netdev. Take > a look in my git tree at the last four patch. They add an additional > call to get the device an LED is attached to. > No currently we have no link for netdev, hence we are setting keep and not setting a default trigger in DT. Just checked them, interesting concept, guess we can think of something also for the interval setting. That would effectively make all the setting of the trigger set. Just my concern is that they may be too much specific to netdev trigger and may be problematic for other kind of hw control. (one main argument that was made for this feature was that some stuff were too much specific and actually not that generic)
> +qca8k_setup_led_ctrl(struct qca8k_priv *priv) > +{ > + struct fwnode_handle *ports, *port; > + int port_num; > + int ret; > + > + ports = device_get_named_child_node(priv->dev, "ports"); > + if (!ports) { > + dev_info(priv->dev, "No ports node specified in device tree!\n"); > + return 0; > + } > + > + fwnode_for_each_child_node(ports, port) { > + struct fwnode_handle *phy_node, *reg_port_node = port; > + > + phy_node = fwnode_find_reference(port, "phy-handle", 0); > + if (!IS_ERR(phy_node)) > + reg_port_node = phy_node; I don't understand this bit. Why are you looking at the phy-handle? > + > + if (fwnode_property_read_u32(reg_port_node, "reg", &port_num)) > + continue; I would of expect port, not reg_port_node. I'm missing something here.... Andrew
On Tue, Mar 07, 2023 at 06:00:38PM +0100, Christian Marangi wrote: > From: Andrew Lunn <andrew@lunn.ch> > > Define common binding parsing for all PHY drivers with LEDs using > phylib. Parse the DT as part of the phy_probe and add LEDs to the > linux LED class infrastructure. For the moment, provide a dummy > brightness function, which will later be replaced with a call into the > PHY driver. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> Hi Christian Since you are submitting this, you need to add your own Signed-off-by: after mine. Andrew
On Tue, Mar 07, 2023 at 06:00:46PM +0100, Christian Marangi wrote: > From: Andrew Lunn <andrew@lunn.ch> > > The WAN port of the 370-RD has a Marvell PHY, with one LED on > the front panel. List this LED in the device tree. > > Set the LED default state to "keep" to not change any blink rule > set by default. Hi Christian What board are you using for testing? It would be good to patch its .dts file to enable its LEDs. We don't normally had new code without a user. Andrew
On Tue, Mar 07, 2023 at 06:57:10PM +0100, Christian Marangi wrote: > On Wed, Mar 08, 2023 at 12:16:13AM +0100, Andrew Lunn wrote: > > > +qca8k_setup_led_ctrl(struct qca8k_priv *priv) > > > +{ > > > + struct fwnode_handle *ports, *port; > > > + int port_num; > > > + int ret; > > > + > > > + ports = device_get_named_child_node(priv->dev, "ports"); > > > + if (!ports) { > > > + dev_info(priv->dev, "No ports node specified in device tree!\n"); > > > + return 0; > > > + } > > > + > > > + fwnode_for_each_child_node(ports, port) { > > > + struct fwnode_handle *phy_node, *reg_port_node = port; > > > + > > > + phy_node = fwnode_find_reference(port, "phy-handle", 0); > > > + if (!IS_ERR(phy_node)) > > > + reg_port_node = phy_node; > > > > I don't understand this bit. Why are you looking at the phy-handle? > > > > > + > > > + if (fwnode_property_read_u32(reg_port_node, "reg", &port_num)) > > > + continue; > > > > I would of expect port, not reg_port_node. I'm missing something > > here.... > > > > It's really not to implement ugly things like "reg - 1" > > On qca8k the port index goes from 0 to 6. > 0 is cpu port 1 > 1 is port0 at mdio reg 0 > 2 is port1 at mdio reg 1 > ... > 6 is cpu port 2 > > Each port have a phy-handle that refer to a phy node with the correct > reg and that reflect the correct port index. > > Tell me if this looks wrong, for qca8k we have qca8k_port_to_phy() and > at times we introduced the mdio thing to describe the port - 1 directly > in DT. If needed I can drop the additional fwnode and use this function > but I would love to use what is defined in DT thatn a simple - 1. This comes back to the off list discussion earlier today. What you actually have here are MAC LEDs, not PHY LEDs. They are implemented in the MAC, not the PHY. To the end user, it should not matter, they blink when you would expect. So your addressing should be based around the MAC port number, not the PHY. Also, at the moment, all we are adding are a bunch of LEDs. There is no link to a netdev at this point. At least, i don't see one. Be once we start using ledtrig-netdev we will need that link to a netdev. Take a look in my git tree at the last four patch. They add an additional call to get the device an LED is attached to. Andrew
On Tue, Mar 07, 2023 at 07:00:08PM +0100, Christian Marangi wrote: > On Wed, Mar 08, 2023 at 12:17:51AM +0100, Andrew Lunn wrote: > > On Tue, Mar 07, 2023 at 06:00:38PM +0100, Christian Marangi wrote: > > > From: Andrew Lunn <andrew@lunn.ch> > > > > > > Define common binding parsing for all PHY drivers with LEDs using > > > phylib. Parse the DT as part of the phy_probe and add LEDs to the > > > linux LED class infrastructure. For the moment, provide a dummy > > > brightness function, which will later be replaced with a call into the > > > PHY driver. > > > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > > > Hi Christian > > > > Since you are submitting this, you need to add your own Signed-off-by: > > after mine. > > > > Tought it was needed only for patch where I have put any change. (case > of 2 patch in this series where there was a whitespace error and had to > change a binding) > > Think I need do to this for every other patch right? https://www.kernel.org/doc/html/latest/process/submitting-patches.html says: Any further SoBs (Signed-off-by:’s) following the author’s SoB are from people handling and transporting the patch, but were not involved in its development. SoB chains should reflect the real route a patch took as it was propagated to the maintainers and ultimately to Linus, with the first SoB entry signalling primary authorship of a single author. So yes, you need to add your Signed-off-by to all my patches, independent of if you make changes or not. Andrew
On Tue, Mar 07, 2023 at 06:00:43PM +0100, Christian Marangi wrote: > Document support for LEDs node in dsa port. > Switch may support different LEDs that can be configured for different > operation like blinking on traffic event or port link. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > Documentation/devicetree/bindings/net/dsa/dsa-port.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml > index 480120469953..f813e1f64f75 100644 > --- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml > @@ -59,6 +59,13 @@ properties: > - rtl8_4t > - seville > > + leds: > + type: object > + > + patternProperties: > + '^led(@[a-f0-9]+)?$': > + $ref: /schemas/leds/common.yaml# Please could you add a description here documenting that these are LEDs the switch controls in its MACs. They are not LEDs in the possibly integrated PHY, which the PHY driver controls. We got this wrong, so i'm sure others will as well. So a bit of documentation should help avoid this. Andrew
> Just checked them, interesting concept, guess we can think of something > also for the interval setting. That would effectively make all the > setting of the trigger set. Just my concern is that they may be too much > specific to netdev trigger and may be problematic for other kind of hw > control. (one main argument that was made for this feature was that some > stuff were too much specific and actually not that generic) I deliberately made this API return a struct device, not a struct net_device. That should keep it generic. The LED could then be attached to an disk device, an mtd device, or a tty device, each of which have an ledtrig-*.c file. Andrew
On Tue, Mar 07, 2023 at 06:00:35PM +0100, Christian Marangi wrote: > This is a continue of [1]. It was decided to take a more gradual > approach to implement LEDs support for switch and phy starting with > basic support and then implementing the hw control part when we have all > the prereq done. In the end, there are likely to be 3 or 4 patchsets. There are going to be patches to both the LED subsystem and the netdev subsystem, plus device tree bindings and some ARM DT patches. Ideally we would like all the patches to go through one tree, so we can keep everything together and buildable. We will cross post patches to both major subsystems, but my guess is, merging via netdev will be best. If not, a stable branch for the LED subsystem which can be pulled into netdev could maybe made be to work. Andrew
On 07/03/2023 18:00, Christian Marangi wrote: > Document support for LEDs node in dsa port. > Switch may support different LEDs that can be configured for different > operation like blinking on traffic event or port link. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > Documentation/devicetree/bindings/net/dsa/dsa-port.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml > index 480120469953..f813e1f64f75 100644 > --- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml > @@ -59,6 +59,13 @@ properties: > - rtl8_4t > - seville > > + leds: > + type: object additionalProperties: false Best regards, Krzysztof