| Message ID | 20240310-realtek-led-v1-2-4d9813ce938e@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: fix LED support for rtl8366 | expand |
On 10/03/2024 05:51, Luiz Angelo Daros de Luca wrote: > This switch family supports four LEDs for each of its six ports. Each > LED group is composed of one of these four LEDs from all six ports. LED > groups can be configured to display hardware information, such as link > activity, or manually controlled through a bitmap in registers > RTL8366RB_LED_0_1_CTRL_REG and RTL8366RB_LED_2_3_CTRL_REG. > > After a reset, the default LED group configuration for groups 0 to 3 > indicates, respectively, link activity, link at 1000M, 100M, and 10M, or > RTL8366RB_LED_CTRL_REG as 0x5432. These configurations are commonly used > for LED indications. However, the driver was replacing that > configuration to use manually controlled LEDs (RTL8366RB_LED_FORCE) > without providing a way for the OS to control them. The default > configuration is deemed more useful than fixed, uncontrollable turned-on > LEDs. > > The driver was enabling/disabling LEDs during port_enable/disable. > However, these events occur when the port is administratively controlled > (up or down) and are not related to link presence. Additionally, when a > port N was disabled, the driver was turning off all LEDs for group N, > not only the corresponding LED for port N in any of those 4 groups. In > such cases, if port 0 was brought down, the LEDs for all ports in LED > group 0 would be turned off. As another side effect, the driver was > wrongly warning that port 5 didn't have an LED ("no LED for port 5"). > Since showing the administrative state of ports is not an orthodox way > to use LEDs, it was not worth it to fix it and all this code was > dropped. > > The code to disable LEDs was simplified only changing each LED group to > the RTL8366RB_LED_OFF state. Registers RTL8366RB_LED_0_1_CTRL_REG and > RTL8366RB_LED_2_3_CTRL_REG are only used when the corresponding LED > group is configured with RTL8366RB_LED_FORCE and they don't need to be > cleaned. The code still references an LED controlled by > RTL8366RB_INTERRUPT_CONTROL_REG, but as of now, no test device has > actually used it. Also, some magic numbers were replaced by macros. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> This is the first version, so where did review happen? Best regards, Krzysztof
On Sun, Mar 10, 2024 at 09:01:46AM +0100, Krzysztof Kozlowski wrote: > On 10/03/2024 05:51, Luiz Angelo Daros de Luca wrote: > > This switch family supports four LEDs for each of its six ports. Each > > LED group is composed of one of these four LEDs from all six ports. LED > > groups can be configured to display hardware information, such as link > > activity, or manually controlled through a bitmap in registers > > RTL8366RB_LED_0_1_CTRL_REG and RTL8366RB_LED_2_3_CTRL_REG. > > > > After a reset, the default LED group configuration for groups 0 to 3 > > indicates, respectively, link activity, link at 1000M, 100M, and 10M, or > > RTL8366RB_LED_CTRL_REG as 0x5432. These configurations are commonly used > > for LED indications. However, the driver was replacing that > > configuration to use manually controlled LEDs (RTL8366RB_LED_FORCE) > > without providing a way for the OS to control them. The default > > configuration is deemed more useful than fixed, uncontrollable turned-on > > LEDs. > > > > The driver was enabling/disabling LEDs during port_enable/disable. > > However, these events occur when the port is administratively controlled > > (up or down) and are not related to link presence. Additionally, when a > > port N was disabled, the driver was turning off all LEDs for group N, > > not only the corresponding LED for port N in any of those 4 groups. In > > such cases, if port 0 was brought down, the LEDs for all ports in LED > > group 0 would be turned off. As another side effect, the driver was > > wrongly warning that port 5 didn't have an LED ("no LED for port 5"). > > Since showing the administrative state of ports is not an orthodox way > > to use LEDs, it was not worth it to fix it and all this code was > > dropped. > > > > The code to disable LEDs was simplified only changing each LED group to > > the RTL8366RB_LED_OFF state. Registers RTL8366RB_LED_0_1_CTRL_REG and > > RTL8366RB_LED_2_3_CTRL_REG are only used when the corresponding LED > > group is configured with RTL8366RB_LED_FORCE and they don't need to be > > cleaned. The code still references an LED controlled by > > RTL8366RB_INTERRUPT_CONTROL_REG, but as of now, no test device has > > actually used it. Also, some magic numbers were replaced by macros. > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > This is the first version, so where did review happen? FWIIW, I think this relates to review of an RFC of this patch-set. https://lore.kernel.org/netdev/CACRpkda8tQ2u4+jCrpOQXbBd84oW98vmiDgU+GgdYCHuZfn49A@mail.gmail.com/
On 10/03/2024 12:37, Simon Horman wrote: >>> >>> The code to disable LEDs was simplified only changing each LED group to >>> the RTL8366RB_LED_OFF state. Registers RTL8366RB_LED_0_1_CTRL_REG and >>> RTL8366RB_LED_2_3_CTRL_REG are only used when the corresponding LED >>> group is configured with RTL8366RB_LED_FORCE and they don't need to be >>> cleaned. The code still references an LED controlled by >>> RTL8366RB_INTERRUPT_CONTROL_REG, but as of now, no test device has >>> actually used it. Also, some magic numbers were replaced by macros. >>> >>> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> >> This is the first version, so where did review happen? > > FWIIW, I think this relates to review of an RFC of this patch-set. > > https://lore.kernel.org/netdev/CACRpkda8tQ2u4+jCrpOQXbBd84oW98vmiDgU+GgdYCHuZfn49A@mail.gmail.com/ OK, then this is v2. RFC is state of patch, not some sort of version. Or just use b4 which handles this automatically... Best regards, Krzysztof
On Sun, 10 Mar 2024 12:47:19 +0100 Krzysztof Kozlowski wrote: > > FWIIW, I think this relates to review of an RFC of this patch-set. > > > > https://lore.kernel.org/netdev/CACRpkda8tQ2u4+jCrpOQXbBd84oW98vmiDgU+GgdYCHuZfn49A@mail.gmail.com/ > > OK, then this is v2. RFC is state of patch, not some sort of version. Or > just use b4 which handles this automatically... Eh, hum. He does according to the X-Mailer header. More importantly I thought the RFC to PATCH transition resetting version numbering is how we always operated. Maybe b4 should be fixed? He put the change log in the cover letter and linked to RFC, FWIW.
On 11/03/2024 17:11, Jakub Kicinski wrote: > On Sun, 10 Mar 2024 12:47:19 +0100 Krzysztof Kozlowski wrote: >>> FWIIW, I think this relates to review of an RFC of this patch-set. >>> >>> https://lore.kernel.org/netdev/CACRpkda8tQ2u4+jCrpOQXbBd84oW98vmiDgU+GgdYCHuZfn49A@mail.gmail.com/ >> >> OK, then this is v2. RFC is state of patch, not some sort of version. Or >> just use b4 which handles this automatically... > > Eh, hum. He does according to the X-Mailer header. More importantly > I thought the RFC to PATCH transition resetting version numbering > is how we always operated. Maybe b4 should be fixed? No, it does not reset the version number, because RFC->PATCH does not mean that suddenly there were no reviews or changes. We all count in brains from 1, so whatever we see - RFC, RFT, RFkisses or hugs - is the first version we see. Then next one, whatever it is called PATCH, RF-non-comments, RFmorekisses, is v2. There are RFCs which go to "v4", with significant discussion and review, thus natural next version is "5", not "1". It's extremely confusing for me to be involved in some sort four revisions of RFC and the suddenly see v1. What happened with my comments? Why its state should be the same as new submission state? Plus, people do not understand or do not have the same meaning of RFC. Some people send RFC with meaning "do not apply, just some work-in-progress" but some send as regular patch with intention to apply. I really, really saw exactly these two approaches. So no, after RFC v1, goes PATCH v2, after RFC v5, goes PATCH v6. Best regards, Krzysztof
On Mon, 11 Mar 2024 17:19:59 +0100 Krzysztof Kozlowski wrote: > No, it does not reset the version number, because RFC->PATCH does not > mean that suddenly there were no reviews or changes. We all count in > brains from 1, so whatever we see - RFC, RFT, RFkisses or hugs - is the > first version we see. Then next one, whatever it is called PATCH, > RF-non-comments, RFmorekisses, is v2. I'm describing what I see as the prevalent interpretation on netdev, more than expressing an opinion. It's not based on any guidance from us, people just seem to think that they should reset when they post a PATCH. Whether we want to enforce a different scheme is up for a discussion, I don't really care. But I do see how not resetting is easier for tools, and that I think is a _bad_ reason to add rules. > There are RFCs which go to "v4", with significant discussion and review, > thus natural next version is "5", not "1". > > It's extremely confusing for me to be involved in some sort four > revisions of RFC and the suddenly see v1. What happened with my > comments? Why its state should be the same as new submission state? Well, as I said, changelog with links in the cover letter... > Plus, people do not understand or do not have the same meaning of RFC. > Some people send RFC with meaning "do not apply, just some > work-in-progress" but some send as regular patch with intention to > apply. I really, really saw exactly these two approaches. Yeah, that I do agree with 100%. People get confused by what RFC means. I think it's partially maintainers' fault. Without naming names, there are some people who used to apply RFC postings, if they liked the code :|
On Mon, Mar 11, 2024 at 09:11:11AM -0700, Jakub Kicinski wrote: > > OK, then this is v2. RFC is state of patch, not some sort of version. Or > > just use b4 which handles this automatically... > > Eh, hum. He does according to the X-Mailer header. More importantly > I thought the RFC to PATCH transition resetting version numbering > is how we always operated. Maybe b4 should be fixed? There is no way to make it work reliably the way you propose, so I strongly suggest that we do it the way b4 currently works: - a series can start with RFC in the prefixes to indicate that it's not something to be considered for inclusion - when the author feels that the series is ready for maintainers' consideration, they simply drop the RFC and keep the same change-id and versioning info; e.g. [PATCH RFC v3] -> [PATCH v4] Resetting the versioning requires resetting the change-id of the series, or a lot of automation breaks. -K
On Mon, 11 Mar 2024 14:40:44 -0400 Konstantin Ryabitsev wrote: > On Mon, Mar 11, 2024 at 09:11:11AM -0700, Jakub Kicinski wrote: > > > OK, then this is v2. RFC is state of patch, not some sort of version. Or > > > just use b4 which handles this automatically... > > > > Eh, hum. He does according to the X-Mailer header. More importantly > > I thought the RFC to PATCH transition resetting version numbering > > is how we always operated. Maybe b4 should be fixed? > > There is no way to make it work reliably the way you propose, Could you describe what the problem is? Cover letter + date seems like fairly strong signal to me. > so I strongly suggest that we do it the way b4 currently works: > > - a series can start with RFC in the prefixes to indicate that it's not > something to be considered for inclusion > - when the author feels that the series is ready for maintainers' > consideration, they simply drop the RFC and keep the same change-id and > versioning info; e.g. [PATCH RFC v3] -> [PATCH v4] It's not a pain point for networking. While I have you - it'd be great if the patchwork bot did not repeatedly set patches to Superseded. Sometimes we want to keep and apply non-latest version, because the latest version was posted based on non-expert review, or we changed our mind. > Resetting the versioning requires resetting the change-id of the series, or a > lot of automation breaks. What is "change-id of the series"?
On Mon, Mar 11, 2024 at 11:52:28AM -0700, Jakub Kicinski wrote: > > > Eh, hum. He does according to the X-Mailer header. More importantly > > > I thought the RFC to PATCH transition resetting version numbering > > > is how we always operated. Maybe b4 should be fixed? > > > > There is no way to make it work reliably the way you propose, > > Could you describe what the problem is? > Cover letter + date seems like fairly strong signal to me. The problem is tracking the series all the way from its inception to its final inclusion into the tree. Links to previous versions of the series are sometimes included in the cover letter, but they are free-form and we can't reliably parse them. Specifically, we need to be able to *reliably* retrieve any previous/new versions of the same series: - to allow running range-diff between vN-1 and vN - to allow checking if there is a newer version of the series available - to allow specifying series dependencies (this series depends on series X version Y or newer) If dropping the RFC prefix resets the version count, then the above breaks unless we also use a different change-id, and if we do that, then we lose change history. > > so I strongly suggest that we do it the way b4 currently works: > > > > - a series can start with RFC in the prefixes to indicate that it's not > > something to be considered for inclusion > > - when the author feels that the series is ready for maintainers' > > consideration, they simply drop the RFC and keep the same change-id and > > versioning info; e.g. [PATCH RFC v3] -> [PATCH v4] > > It's not a pain point for networking. > > While I have you - it'd be great if the patchwork bot did not > repeatedly set patches to Superseded. Sometimes we want to keep and > apply non-latest version, because the latest version was posted based > on non-expert review, or we changed our mind. That's a request to helpdesk. :) > > Resetting the versioning requires resetting the change-id of the series, or a > > lot of automation breaks. > > What is "change-id of the series"? It's the line that says "change-id: " at the bottom of the cover letter and doesn't change between v1..vN of the series. This is how we know it's the same series and are able to retrieve the entire series history without guesswork. -K
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c index e10ae94cf771..5ccb1a3a149d 100644 --- a/drivers/net/dsa/realtek/rtl8366rb.c +++ b/drivers/net/dsa/realtek/rtl8366rb.c @@ -185,7 +185,12 @@ #define RTL8366RB_LED_BLINKRATE_222MS 0x0004 #define RTL8366RB_LED_BLINKRATE_446MS 0x0005 +/* LED trigger event for each group */ #define RTL8366RB_LED_CTRL_REG 0x0431 +#define RTL8366RB_LED_CTRL_OFFSET(led_group) \ + (4 * (led_group)) +#define RTL8366RB_LED_CTRL_MASK(led_group) \ + (0xf << RTL8366RB_LED_CTRL_OFFSET(led_group)) #define RTL8366RB_LED_OFF 0x0 #define RTL8366RB_LED_DUP_COL 0x1 #define RTL8366RB_LED_LINK_ACT 0x2 @@ -202,6 +207,11 @@ #define RTL8366RB_LED_LINK_TX 0xd #define RTL8366RB_LED_MASTER 0xe #define RTL8366RB_LED_FORCE 0xf + +/* The RTL8366RB_LED_X_X registers are used to manually set the LED state only + * when the corresponding LED group in RTL8366RB_LED_CTRL_REG is + * RTL8366RB_LED_FORCE. Otherwise, it is ignored. + */ #define RTL8366RB_LED_0_1_CTRL_REG 0x0432 #define RTL8366RB_LED_1_OFFSET 6 #define RTL8366RB_LED_2_3_CTRL_REG 0x0433 @@ -1001,28 +1011,20 @@ static int rtl8366rb_setup(struct dsa_switch *ds) */ if (priv->leds_disabled) { /* Turn everything off */ - regmap_update_bits(priv->map, - RTL8366RB_LED_0_1_CTRL_REG, - 0x0FFF, 0); - regmap_update_bits(priv->map, - RTL8366RB_LED_2_3_CTRL_REG, - 0x0FFF, 0); regmap_update_bits(priv->map, RTL8366RB_INTERRUPT_CONTROL_REG, RTL8366RB_P4_RGMII_LED, 0); - val = RTL8366RB_LED_OFF; - } else { - /* TODO: make this configurable per LED */ - val = RTL8366RB_LED_FORCE; - } - for (i = 0; i < 4; i++) { - ret = regmap_update_bits(priv->map, - RTL8366RB_LED_CTRL_REG, - 0xf << (i * 4), - val << (i * 4)); - if (ret) - return ret; + + for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) { + val = RTL8366RB_LED_OFF << RTL8366RB_LED_CTRL_OFFSET(i); + ret = regmap_update_bits(priv->map, + RTL8366RB_LED_CTRL_REG, + RTL8366RB_LED_CTRL_MASK(i), + val); + if (ret) + return ret; + } } ret = rtl8366_reset_vlan(priv); @@ -1167,52 +1169,6 @@ rtl8366rb_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, } } -static void rb8366rb_set_port_led(struct realtek_priv *priv, - int port, bool enable) -{ - u16 val = enable ? 0x3f : 0; - int ret; - - if (priv->leds_disabled) - return; - - switch (port) { - case 0: - ret = regmap_update_bits(priv->map, - RTL8366RB_LED_0_1_CTRL_REG, - 0x3F, val); - break; - case 1: - ret = regmap_update_bits(priv->map, - RTL8366RB_LED_0_1_CTRL_REG, - 0x3F << RTL8366RB_LED_1_OFFSET, - val << RTL8366RB_LED_1_OFFSET); - break; - case 2: - ret = regmap_update_bits(priv->map, - RTL8366RB_LED_2_3_CTRL_REG, - 0x3F, val); - break; - case 3: - ret = regmap_update_bits(priv->map, - RTL8366RB_LED_2_3_CTRL_REG, - 0x3F << RTL8366RB_LED_3_OFFSET, - val << RTL8366RB_LED_3_OFFSET); - break; - case 4: - ret = regmap_update_bits(priv->map, - RTL8366RB_INTERRUPT_CONTROL_REG, - RTL8366RB_P4_RGMII_LED, - enable ? RTL8366RB_P4_RGMII_LED : 0); - break; - default: - dev_err(priv->dev, "no LED for port %d\n", port); - return; - } - if (ret) - dev_err(priv->dev, "error updating LED on port %d\n", port); -} - static int rtl8366rb_port_enable(struct dsa_switch *ds, int port, struct phy_device *phy) @@ -1226,7 +1182,6 @@ rtl8366rb_port_enable(struct dsa_switch *ds, int port, if (ret) return ret; - rb8366rb_set_port_led(priv, port, true); return 0; } @@ -1241,8 +1196,6 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port) BIT(port)); if (ret) return; - - rb8366rb_set_port_led(priv, port, false); } static int
This switch family supports four LEDs for each of its six ports. Each LED group is composed of one of these four LEDs from all six ports. LED groups can be configured to display hardware information, such as link activity, or manually controlled through a bitmap in registers RTL8366RB_LED_0_1_CTRL_REG and RTL8366RB_LED_2_3_CTRL_REG. After a reset, the default LED group configuration for groups 0 to 3 indicates, respectively, link activity, link at 1000M, 100M, and 10M, or RTL8366RB_LED_CTRL_REG as 0x5432. These configurations are commonly used for LED indications. However, the driver was replacing that configuration to use manually controlled LEDs (RTL8366RB_LED_FORCE) without providing a way for the OS to control them. The default configuration is deemed more useful than fixed, uncontrollable turned-on LEDs. The driver was enabling/disabling LEDs during port_enable/disable. However, these events occur when the port is administratively controlled (up or down) and are not related to link presence. Additionally, when a port N was disabled, the driver was turning off all LEDs for group N, not only the corresponding LED for port N in any of those 4 groups. In such cases, if port 0 was brought down, the LEDs for all ports in LED group 0 would be turned off. As another side effect, the driver was wrongly warning that port 5 didn't have an LED ("no LED for port 5"). Since showing the administrative state of ports is not an orthodox way to use LEDs, it was not worth it to fix it and all this code was dropped. The code to disable LEDs was simplified only changing each LED group to the RTL8366RB_LED_OFF state. Registers RTL8366RB_LED_0_1_CTRL_REG and RTL8366RB_LED_2_3_CTRL_REG are only used when the corresponding LED group is configured with RTL8366RB_LED_FORCE and they don't need to be cleaned. The code still references an LED controlled by RTL8366RB_INTERRUPT_CONTROL_REG, but as of now, no test device has actually used it. Also, some magic numbers were replaced by macros. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/net/dsa/realtek/rtl8366rb.c | 87 +++++++++---------------------------- 1 file changed, 20 insertions(+), 67 deletions(-)