This series fixes the LED support for rtl8366. The existing code was not
tested in a device with switch LEDs and it was using a flawed logic.
The driver now keeps the default LED configuration if nothing requests a
different behavior. This may be enough for most devices. This can be
achieved either by omitting the LED from the device-tree or configuring
all LEDs in a group with the default state set to "keep".
The hardware trigger for LEDs in Realtek switches is shared among all
LEDs in a group. This behavior doesn't align well with the Linux LED
API, which controls LEDs individually. Once the OS changes the
brightness of a LED in a group still triggered by the hardware, the
entire group switches to software-controlled LEDs. This shared behavior
also prevents offloading the trigger to the hardware as it would require
an orchestration between LEDs in a group, not currently present in the
LED API.
The assertion of device hardware reset during driver removal was removed
because it was causing an issue with the LED release code. Devres
devices are released after the driver's removal is executed. Asserting
the reset at that point was causing timeout errors during LED release
when it attempted to turn off the LED.
Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
---
Changes in v1:
- Rebased on new relatek DSA drivers
- Improved commit messages
- Added commit to remove the reset assert during .remove
- Link to RFC: https://lore.kernel.org/r/[email protected]
---
Luiz Angelo Daros de Luca (4):
dt-bindings: net: dsa: realtek: describe LED usage
net: dsa: realtek: keep default LED state in rtl8366rb
net: dsa: realtek: do not assert reset on remove
net: dsa: realtek: add LED drivers for rtl8366rb
.../devicetree/bindings/net/dsa/realtek.yaml | 46 +++
drivers/net/dsa/realtek/rtl8366rb.c | 341 ++++++++++++++++-----
drivers/net/dsa/realtek/rtl83xx.c | 7 +-
3 files changed, 306 insertions(+), 88 deletions(-)
---
base-commit: d7e14e534493328cc5f67baaff2b0c23d32b0a57
change-id: 20240212-realtek-led-a665ae39a9c5
Best regards,
--
Luiz Angelo Daros de Luca <[email protected]>
Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
LED reg property will indicate its LED group.
An example of LED usage was included in an existing switch example.
Cc: [email protected]
Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
---
.../devicetree/bindings/net/dsa/realtek.yaml | 46 ++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
index 70b6bda3cf98..45c1656b3fae 100644
--- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
@@ -108,6 +108,32 @@ properties:
compatible:
const: realtek,smi-mdio
+patternProperties:
+ '^(ethernet-)?ports$':
+ type: object
+ additionalProperties: true
+
+ patternProperties:
+ '^(ethernet-)?port@[0-6]$':
+ type: object
+ additionalProperties: true
+
+ properties:
+ leds:
+ description:
+ "LEDs associated with this port"
+
+ patternProperties:
+ '^led@[a-f0-9]+$':
+ type: object
+ additionalProperties: true
+
+ properties:
+ reg:
+ description:
+ "reg indicates the LED group for this LED"
+ enum: [0, 1, 2, 3]
+
if:
required:
- reg
@@ -144,6 +170,7 @@ unevaluatedProperties: false
examples:
- |
#include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/leds/common.h>
#include <dt-bindings/interrupt-controller/irq.h>
platform {
@@ -190,6 +217,25 @@ examples:
reg = <4>;
label = "wan";
phy-handle = <&phy4>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_WAN;
+ default-state = "keep";
+ };
+
+ led@3 {
+ reg = <3>;
+ color = <LED_COLOR_ID_AMBER>;
+ function = LED_FUNCTION_WAN;
+ default-state = "keep";
+ };
+ };
};
port@5 {
reg = <5>;
--
2.44.0
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 <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
---
drivers/net/dsa/realtek/rtl8366rb.c | 87 +++++++++----------------------------
1 file changed, 20 insertions(+), 67 deletions(-)
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
--
2.44.0
The necessity of asserting the reset on removal was previously questioned, as DSA's own cleanup methods should suffice to prevent traffic leakage[1].
When a driver has subdrivers controlled by devres, they will be unregistered after the main driver's .remove is executed. If it asserts a reset, the subdrivers will be unable to communicate with the hardware during their cleanup. For LEDs, this means that they will fail to turn off, resulting in a timeout error.
[1] https://lore.kernel.org/r/[email protected]/
Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
---
drivers/net/dsa/realtek/rtl83xx.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
index d2e876805393..a9c1702431ef 100644
--- a/drivers/net/dsa/realtek/rtl83xx.c
+++ b/drivers/net/dsa/realtek/rtl83xx.c
@@ -290,16 +290,13 @@ EXPORT_SYMBOL_NS_GPL(rtl83xx_shutdown, REALTEK_DSA);
* rtl83xx_remove() - Cleanup a realtek switch driver
* @priv: realtek_priv pointer
*
- * If a method is provided, this function asserts the hard reset of the switch
- * in order to avoid leaking traffic when the driver is gone.
+ * Placehold for common cleanup procedures.
*
- * Context: Might sleep if priv->gdev->chip->can_sleep.
+ * Context: Any
* Return: nothing
*/
void rtl83xx_remove(struct realtek_priv *priv)
{
- /* leave the device reset asserted */
- rtl83xx_reset_assert(priv);
}
EXPORT_SYMBOL_NS_GPL(rtl83xx_remove, REALTEK_DSA);
--
2.44.0
This commit introduces LED drivers for rtl8366rb, enabling LEDs to be
described in the device tree using the same format as qca8k. Each port
can configure up to 4 LEDs.
If all LEDs in a group use the default state "keep", they will use the
default behavior after a reset. Changing the brightness of one LED,
either manually or by a trigger, will disable the default hardware
trigger and switch the entire LED group to manually controlled LEDs.
Once in this mode, there is no way to revert to hardware-controlled LEDs
(except by resetting the switch).
Software triggers function as expected with manually controlled LEDs.
Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
---
drivers/net/dsa/realtek/rtl8366rb.c | 270 ++++++++++++++++++++++++++++++++----
1 file changed, 246 insertions(+), 24 deletions(-)
diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 5ccb1a3a149d..e45773fec17e 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -191,31 +191,21 @@
(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
-#define RTL8366RB_LED_SPD1000 0x3
-#define RTL8366RB_LED_SPD100 0x4
-#define RTL8366RB_LED_SPD10 0x5
-#define RTL8366RB_LED_SPD1000_ACT 0x6
-#define RTL8366RB_LED_SPD100_ACT 0x7
-#define RTL8366RB_LED_SPD10_ACT 0x8
-#define RTL8366RB_LED_SPD100_10_ACT 0x9
-#define RTL8366RB_LED_FIBER 0xa
-#define RTL8366RB_LED_AN_FAULT 0xb
-#define RTL8366RB_LED_LINK_RX 0xc
-#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
-#define RTL8366RB_LED_3_OFFSET 6
+#define RTL8366RB_LED_X_X_CTRL_REG(led_group) \
+ ((led_group) <= 1 ? \
+ RTL8366RB_LED_0_1_CTRL_REG : \
+ RTL8366RB_LED_2_3_CTRL_REG)
+#define RTL8366RB_LED_0_X_CTRL_MASK GENMASK(5, 0)
+#define RTL8366RB_LED_X_1_CTRL_MASK GENMASK(11, 6)
+#define RTL8366RB_LED_2_X_CTRL_MASK GENMASK(5, 0)
+#define RTL8366RB_LED_X_3_CTRL_MASK GENMASK(11, 6)
#define RTL8366RB_MIB_COUNT 33
#define RTL8366RB_GLOBAL_MIB_COUNT 1
@@ -359,14 +349,44 @@
#define RTL8366RB_GREEN_FEATURE_TX BIT(0)
#define RTL8366RB_GREEN_FEATURE_RX BIT(2)
+enum rtl8366_led_mode {
+ RTL8366RB_LED_OFF = 0x0,
+ RTL8366RB_LED_DUP_COL = 0x1,
+ RTL8366RB_LED_LINK_ACT = 0x2,
+ RTL8366RB_LED_SPD1000 = 0x3,
+ RTL8366RB_LED_SPD100 = 0x4,
+ RTL8366RB_LED_SPD10 = 0x5,
+ RTL8366RB_LED_SPD1000_ACT = 0x6,
+ RTL8366RB_LED_SPD100_ACT = 0x7,
+ RTL8366RB_LED_SPD10_ACT = 0x8,
+ RTL8366RB_LED_SPD100_10_ACT = 0x9,
+ RTL8366RB_LED_FIBER = 0xa,
+ RTL8366RB_LED_AN_FAULT = 0xb,
+ RTL8366RB_LED_LINK_RX = 0xc,
+ RTL8366RB_LED_LINK_TX = 0xd,
+ RTL8366RB_LED_MASTER = 0xe,
+ RTL8366RB_LED_FORCE = 0xf,
+
+ __RTL8366RB_LED_MAX
+};
+
+struct rtl8366rb_led {
+ u8 port_num;
+ u8 led_group;
+ struct realtek_priv *priv;
+ struct led_classdev cdev;
+};
+
/**
* struct rtl8366rb - RTL8366RB-specific data
* @max_mtu: per-port max MTU setting
* @pvid_enabled: if PVID is set for respective port
+ * @leds: per-port and per-ledgroup led info
*/
struct rtl8366rb {
unsigned int max_mtu[RTL8366RB_NUM_PORTS];
bool pvid_enabled[RTL8366RB_NUM_PORTS];
+ struct rtl8366rb_led leds[RTL8366RB_NUM_PORTS][RTL8366RB_NUM_LEDGROUPS];
};
static struct rtl8366_mib_counter rtl8366rb_mib_counters[] = {
@@ -809,6 +829,208 @@ static int rtl8366rb_jam_table(const struct rtl8366rb_jam_tbl_entry *jam_table,
return 0;
}
+static int rb8366rb_set_ledgroup_mode(struct realtek_priv *priv,
+ u8 led_group,
+ enum rtl8366_led_mode mode)
+{
+ int ret;
+ u32 val;
+
+ val = mode << RTL8366RB_LED_CTRL_OFFSET(led_group);
+
+ ret = regmap_update_bits(priv->map,
+ RTL8366RB_LED_CTRL_REG,
+ RTL8366RB_LED_CTRL_MASK(led_group),
+ val);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static inline u32 rtl8366rb_led_group_port_mask(u8 led_group, u8 port)
+{
+ switch (led_group) {
+ case 0:
+ return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+ case 1:
+ return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+ case 2:
+ return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+ case 3:
+ return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+ default:
+ return 0;
+ }
+}
+
+static int rb8366rb_get_port_led(struct rtl8366rb_led *led, bool enable)
+{
+ struct realtek_priv *priv = led->priv;
+ u8 led_group = led->led_group;
+ u8 port_num = led->port_num;
+ int ret;
+ u32 val;
+
+ if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
+ dev_err(priv->dev, "Invalid LED group %d for port %d",
+ led_group, port_num);
+ return -EINVAL;
+ }
+
+ ret = regmap_read(priv->map, RTL8366RB_LED_X_X_CTRL_REG(led_group),
+ &val);
+ if (ret) {
+ dev_err(priv->dev, "error reading LED on port %d group %d\n",
+ led_group, port_num);
+ return ret;
+ }
+
+ return !!(val & rtl8366rb_led_group_port_mask(led_group, port_num));
+}
+
+static int rb8366rb_set_port_led(struct rtl8366rb_led *led, bool enable)
+{
+ struct realtek_priv *priv = led->priv;
+ u8 led_group = led->led_group;
+ u8 port_num = led->port_num;
+ int ret;
+
+ if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
+ dev_err(priv->dev, "Invalid LED group %d for port %d",
+ led_group, port_num);
+ return -EINVAL;
+ }
+
+ ret = regmap_update_bits(priv->map,
+ RTL8366RB_LED_X_X_CTRL_REG(led_group),
+ rtl8366rb_led_group_port_mask(led_group,
+ port_num),
+ enable ? 0xffff : 0);
+ if (ret) {
+ dev_err(priv->dev, "error updating LED on port %d group %d\n",
+ led_group, port_num);
+ return ret;
+ }
+
+ /* Change the LED group to manual controlled LEDs if required */
+ ret = rb8366rb_set_ledgroup_mode(priv, led_group, RTL8366RB_LED_FORCE);
+
+ if (ret) {
+ dev_err(priv->dev, "error updating LED GROUP group %d\n",
+ led_group);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int
+rtl8366rb_cled_brightness_set_blocking(struct led_classdev *ldev,
+ enum led_brightness brightness)
+{
+ struct rtl8366rb_led *led = container_of(ldev, struct rtl8366rb_led,
+ cdev);
+
+ return rb8366rb_set_port_led(led, brightness == LED_ON);
+}
+
+static int rtl8366rb_setup_led(struct realtek_priv *priv, struct dsa_port *dp,
+ struct fwnode_handle *led_fwnode)
+{
+ struct rtl8366rb *rb = priv->chip_data;
+ struct led_init_data init_data = { };
+ struct rtl8366rb_led *led;
+ enum led_default_state state;
+ u32 led_group;
+ int ret;
+
+ ret = fwnode_property_read_u32(led_fwnode, "reg", &led_group);
+ if (ret)
+ return ret;
+
+ if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
+ dev_warn(priv->dev, "Invalid LED reg %d defined for port %d",
+ led_group, dp->index);
+ return -EINVAL;
+ }
+
+ led = &rb->leds[dp->index][led_group];
+ led->port_num = dp->index;
+ led->led_group = led_group;
+ led->priv = priv;
+
+ state = led_init_default_state_get(led_fwnode);
+ switch (state) {
+ case LEDS_DEFSTATE_ON:
+ led->cdev.brightness = 1;
+ rb8366rb_set_port_led(led, 1);
+ break;
+ case LEDS_DEFSTATE_KEEP:
+ led->cdev.brightness =
+ rb8366rb_get_port_led(led, 1);
+ break;
+ case LEDS_DEFSTATE_OFF:
+ default:
+ led->cdev.brightness = 0;
+ rb8366rb_set_port_led(led, 0);
+ }
+
+ led->cdev.max_brightness = 1;
+ led->cdev.brightness_set_blocking =
+ rtl8366rb_cled_brightness_set_blocking;
+ init_data.fwnode = led_fwnode;
+ init_data.devname_mandatory = true;
+
+ init_data.devicename = kasprintf(GFP_KERNEL, "Realtek-%d:0%d:%d",
+ dp->ds->index, dp->index, led_group);
+ if (!init_data.devicename)
+ return -ENOMEM;
+
+ ret = devm_led_classdev_register_ext(priv->dev, &led->cdev, &init_data);
+ if (ret) {
+ dev_warn(priv->dev, "Failed to init LED %d for port %d",
+ led_group, dp->index);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int rtl8366rb_setup_leds(struct realtek_priv *priv)
+{
+ struct device_node *leds_np, *led_np;
+ struct dsa_switch *ds = &priv->ds;
+ struct dsa_port *dp;
+ int ret;
+
+ dsa_switch_for_each_port(dp, ds) {
+ if (!dp->dn)
+ continue;
+
+ leds_np = of_get_child_by_name(dp->dn, "leds");
+ if (!leds_np) {
+ dev_dbg(priv->dev, "No leds defined for port %d",
+ dp->index);
+ continue;
+ }
+
+ for_each_child_of_node(leds_np, led_np) {
+ ret = rtl8366rb_setup_led(priv, dp,
+ of_fwnode_handle(led_np));
+ if (ret) {
+ of_node_put(led_np);
+ break;
+ }
+ }
+
+ of_node_put(leds_np);
+ if (ret)
+ return ret;
+ }
+ return 0;
+}
+
static int rtl8366rb_setup(struct dsa_switch *ds)
{
struct realtek_priv *priv = ds->priv;
@@ -817,7 +1039,6 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
u32 chip_ver = 0;
u32 chip_id = 0;
int jam_size;
- u32 val;
int ret;
int i;
@@ -1017,14 +1238,15 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
0);
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);
+ ret = rb8366rb_set_ledgroup_mode(priv, i,
+ RTL8366RB_LED_OFF);
if (ret)
return ret;
}
+ } else {
+ ret = rtl8366rb_setup_leds(priv);
+ if (ret)
+ return ret;
}
ret = rtl8366_reset_vlan(priv);
--
2.44.0
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 <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
This is the first version, so where did review happen?
Best regards,
Krzysztof
On 10/03/2024 05:52, Luiz Angelo Daros de Luca wrote:
> This commit introduces LED drivers for rtl8366rb, enabling LEDs to be
> described in the device tree using the same format as qca8k. Each port
> can configure up to 4 LEDs.
>
> If all LEDs in a group use the default state "keep", they will use the
> default behavior after a reset. Changing the brightness of one LED,
> either manually or by a trigger, will disable the default hardware
> trigger and switch the entire LED group to manually controlled LEDs.
> Once in this mode, there is no way to revert to hardware-controlled LEDs
> (except by resetting the switch).
>
> Software triggers function as expected with manually controlled LEDs.
>
> Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
Again, this is the first version, so how could you have a review?
Best regards,
Krzysztof
On 10/03/2024 05:51, Luiz Angelo Daros de Luca wrote:
> Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> LED reg property will indicate its LED group.
>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.
Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline), work on fork of kernel
(don't, instead use mainline) or you ignore some maintainers (really
don't). Just use b4 and everything should be fine, although remember
about `b4 prep --auto-to-cc` if you added new patches to the patchset.
> An example of LED usage was included in an existing switch example.
>
> Cc: [email protected]
Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
commit msg. There is no single need to store automated output of
get_maintainers.pl in the git log. It can be easily re-created at any
given time, thus its presence in the git history is redundant and
obfuscates the log.
If you need it for your own patch management purposes, keep it under the
--- separator.
> Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
> ---
> .../devicetree/bindings/net/dsa/realtek.yaml | 46 ++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek.yaml b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> index 70b6bda3cf98..45c1656b3fae 100644
> --- a/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek.yaml
> @@ -108,6 +108,32 @@ properties:
> compatible:
> const: realtek,smi-mdio
>
> +patternProperties:
> + '^(ethernet-)?ports$':
> + type: object
> + additionalProperties: true
> +
> + patternProperties:
> + '^(ethernet-)?port@[0-6]$':
> + type: object
> + additionalProperties: true
> +
> + properties:
> + leds:
type: object
additionalProperties: false
> + description:
> + "LEDs associated with this port"
Drop quotes.
> +
> + patternProperties:
> + '^led@[a-f0-9]+$':
[0-3]
> + type: object
> + additionalProperties: true
This cannot be 'true'. Which then will point you to errors and missing
ref to leds schema and need to use unevaluatedProperties: false.
> +
> + properties:
> + reg:
> + description:
> + "reg indicates the LED group for this LED"
Drop quotes
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 <[email protected]>
> > Reviewed-by: Linus Walleij <[email protected]>
>
> 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 <[email protected]>
>>> Reviewed-by: Linus Walleij <[email protected]>
>>
>> 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, Mar 10, 2024 at 01:52:01AM -0300, Luiz Angelo Daros de Luca wrote:
> This commit introduces LED drivers for rtl8366rb, enabling LEDs to be
> described in the device tree using the same format as qca8k. Each port
> can configure up to 4 LEDs.
>
> If all LEDs in a group use the default state "keep", they will use the
> default behavior after a reset. Changing the brightness of one LED,
> either manually or by a trigger, will disable the default hardware
> trigger and switch the entire LED group to manually controlled LEDs.
> Once in this mode, there is no way to revert to hardware-controlled LEDs
> (except by resetting the switch).
>
> Software triggers function as expected with manually controlled LEDs.
>
> Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> ---
> drivers/net/dsa/realtek/rtl8366rb.c | 270 ++++++++++++++++++++++++++++++++----
..
> +static int rtl8366rb_setup_led(struct realtek_priv *priv, struct dsa_port *dp,
> + struct fwnode_handle *led_fwnode)
> +{
> + struct rtl8366rb *rb = priv->chip_data;
> + struct led_init_data init_data = { };
> + struct rtl8366rb_led *led;
> + enum led_default_state state;
> + u32 led_group;
> + int ret;
nit: Please consider using reverse xmas tree - longest line to shortest -
for local variables in networking code.
..
> +static int rtl8366rb_setup_leds(struct realtek_priv *priv)
> +{
> + struct device_node *leds_np, *led_np;
> + struct dsa_switch *ds = &priv->ds;
> + struct dsa_port *dp;
> + int ret;
> +
> + dsa_switch_for_each_port(dp, ds) {
> + if (!dp->dn)
> + continue;
> +
> + leds_np = of_get_child_by_name(dp->dn, "leds");
> + if (!leds_np) {
> + dev_dbg(priv->dev, "No leds defined for port %d",
> + dp->index);
> + continue;
> + }
> +
> + for_each_child_of_node(leds_np, led_np) {
> + ret = rtl8366rb_setup_led(priv, dp,
> + of_fwnode_handle(led_np));
> + if (ret) {
> + of_node_put(led_np);
FWIIW, Coccinelle complans about "probable double put" here.
But it looks correct to me as it's only called when breaking out of
the loop, when it is required AFAIK.
> + break;
> + }
> + }
> +
> + of_node_put(leds_np);
> + if (ret)
I'm unsure if this can happen. But if for_each_child_of_node()
iterates zero times then ret may be uninitialised here.
Flagged by Smatch.
> + return ret;
> + }
> + return 0;
> +}
..
On Sun, Mar 10, 2024 at 01:51:58AM -0300, Luiz Angelo Daros de Luca wrote:
> Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> LED reg property will indicate its LED group.
> + properties:
> + reg:
> + description:
> + "reg indicates the LED group for this LED"
> + enum: [0, 1, 2, 3]
If this identifies the group, what identifies the actual LED? There
are four of them. How do i say LEDs 0 and 1 are unused, 2 and 3 are
wired to LEDs?
It would be much more usual for reg to be the LED number for the
port. And there then be a group property indicating what group the LED
belongs to. However, i'm wondering, is group fixed? Do we actually
need it in DT, or can there just be a table in the driver which maps
port:led to group?
Andrew
On Sun, Mar 10, 2024 at 5:52 AM Luiz Angelo Daros de Luca
<[email protected]> wrote:
> Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> LED reg property will indicate its LED group.
>
> An example of LED usage was included in an existing switch example.
>
> Cc: [email protected]
> Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
But... is this patch even needed at all?
On the top of Documentation/devicetree/bindings/net/dsa/realtek.yaml:
allOf:
- $ref: dsa.yaml#/$defs/ethernet-ports
In Documentation/devicetree/bindings/net/dsa/dsa.yaml:
$defs:
ethernet-ports:
description: A DSA switch without any extra port properties
$ref: '#'
patternProperties:
"^(ethernet-)?ports$":
patternProperties:
"^(ethernet-)?port@[0-9a-f]+$":
description: Ethernet switch ports
$ref: dsa-port.yaml#
unevaluatedProperties: false
(NB, just "ports" is fine.)
In Documentation/devicetree/bindings/net/dsa/dsa-port.yaml:
$ref: /schemas/net/ethernet-switch-port.yaml#
In Documentation/devicetree/bindings/net/ethernet-switch-port.yaml:
$ref: ethernet-controller.yaml#
In Documentation/devicetree/bindings/net/ethernet-controller.yaml:
leds:
description:
Describes the LEDs associated by Ethernet Controller.
These LEDs are not integrated in the PHY and PHY doesn't have any
control on them. Ethernet Controller regs are used to control
these defined LEDs.
type: object
properties:
'#address-cells':
const: 1
'#size-cells':
const: 0
patternProperties:
'^led@[a-f0-9]+$':
$ref: /schemas/leds/common.yaml#
properties:
reg:
maxItems: 1
description:
This define the LED index in the PHY or the MAC. It's really
driver dependent and required for ports that define multiple
LED for the same port.
required:
- reg
unevaluatedProperties: false
additionalProperties: false
Try to introduce small errors in your DTS leds node and it should warn!
I'm not claiming that above include chain is "easy to read"... It makes
perfect sense to machines but maybe is a bit to construed for human
readers.
What you should do instead (IMO) is to just keep the last part that
adds a leds node example.
Yours,
Linus Walleij
On Sun, Mar 10, 2024 at 5:52 AM Luiz Angelo Daros de Luca
<[email protected]> wrote:
> The necessity of asserting the reset on removal was previously questioned, as DSA's own cleanup methods should suffice to prevent traffic leakage[1].
>
> When a driver has subdrivers controlled by devres, they will be unregistered after the main driver's .remove is executed. If it asserts a reset, the subdrivers will be unable to communicate with the hardware during their cleanup. For LEDs, this means that they will fail to turn off, resulting in a timeout error.
>
> [1] https://lore.kernel.org/r/[email protected]/
>
> Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
The commit message needs some linebreaks :D
Other than that:
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Sun, Mar 10, 2024 at 01:52:01AM -0300, Luiz Angelo Daros de Luca wrote:
> This commit introduces LED drivers for rtl8366rb, enabling LEDs to be
> described in the device tree using the same format as qca8k. Each port
> can configure up to 4 LEDs.
>
> If all LEDs in a group use the default state "keep", they will use the
> default behavior after a reset. Changing the brightness of one LED,
> either manually or by a trigger, will disable the default hardware
> trigger and switch the entire LED group to manually controlled LEDs.
The previous patch said:
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.
I could be understanding this wrongly, but to me, it sounds like an
LED is either controlled via the group, or you can take an LED out of
the group and software on/off control it? Ah, after looking at the
code. The group can be put into forced mode, and then each LED in the
group controlled in software.
> Once in this mode, there is no way to revert to hardware-controlled LEDs
> (except by resetting the switch).
Just for my understanding.... This is a software limitation. You could
check if all LEDs in a group are using the same trigger, and then set
the group to that trigger?
I do understand how the current offload concept causes problems here.
You need a call into the trigger to ask it to re-evaluate if offload
can be performed for an LED.
What you have here seems like a good first step, offloaded could be
added later if somebody wants to.
> +enum rtl8366_led_mode {
> + RTL8366RB_LED_OFF = 0x0,
> + RTL8366RB_LED_DUP_COL = 0x1,
> + RTL8366RB_LED_LINK_ACT = 0x2,
> + RTL8366RB_LED_SPD1000 = 0x3,
> + RTL8366RB_LED_SPD100 = 0x4,
> + RTL8366RB_LED_SPD10 = 0x5,
> + RTL8366RB_LED_SPD1000_ACT = 0x6,
> + RTL8366RB_LED_SPD100_ACT = 0x7,
> + RTL8366RB_LED_SPD10_ACT = 0x8,
> + RTL8366RB_LED_SPD100_10_ACT = 0x9,
> + RTL8366RB_LED_FIBER = 0xa,
> + RTL8366RB_LED_AN_FAULT = 0xb,
> + RTL8366RB_LED_LINK_RX = 0xc,
> + RTL8366RB_LED_LINK_TX = 0xd,
> + RTL8366RB_LED_MASTER = 0xe,
> + RTL8366RB_LED_FORCE = 0xf,
This is what the group shows? Maybe put _GROUP_ into the name? This
concept of a group is pretty unusual, so we should be careful with
naming to make it clear when we are referring to one LED or a group of
LEDs. I would also put _group_ into the enum.
> +
> + __RTL8366RB_LED_MAX
> +};
> +
> +struct rtl8366rb_led {
> + u8 port_num;
> + u8 led_group;
> + struct realtek_priv *priv;
> + struct led_classdev cdev;
> +};
> +
> /**
> * struct rtl8366rb - RTL8366RB-specific data
> * @max_mtu: per-port max MTU setting
> * @pvid_enabled: if PVID is set for respective port
> + * @leds: per-port and per-ledgroup led info
> */
> struct rtl8366rb {
> unsigned int max_mtu[RTL8366RB_NUM_PORTS];
> bool pvid_enabled[RTL8366RB_NUM_PORTS];
> + struct rtl8366rb_led leds[RTL8366RB_NUM_PORTS][RTL8366RB_NUM_LEDGROUPS];
> };
>
> static struct rtl8366_mib_counter rtl8366rb_mib_counters[] = {
> @@ -809,6 +829,208 @@ static int rtl8366rb_jam_table(const struct rtl8366rb_jam_tbl_entry *jam_table,
> return 0;
> }
>
> +static int rb8366rb_set_ledgroup_mode(struct realtek_priv *priv,
> + u8 led_group,
> + enum rtl8366_led_mode mode)
> +{
> + int ret;
> + u32 val;
> +
> + val = mode << RTL8366RB_LED_CTRL_OFFSET(led_group);
> +
> + ret = regmap_update_bits(priv->map,
> + RTL8366RB_LED_CTRL_REG,
> + RTL8366RB_LED_CTRL_MASK(led_group),
> + val);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static inline u32 rtl8366rb_led_group_port_mask(u8 led_group, u8 port)
> +{
> + switch (led_group) {
> + case 0:
> + return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> + case 1:
> + return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> + case 2:
> + return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> + case 3:
> + return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> + default:
> + return 0;
> + }
> +}
> +
> +static int rb8366rb_get_port_led(struct rtl8366rb_led *led, bool enable)
enable seems unused here. It also seems an odd parameter to pass to a
_get_ function.
> +{
> + struct realtek_priv *priv = led->priv;
> + u8 led_group = led->led_group;
> + u8 port_num = led->port_num;
> + int ret;
> + u32 val;
> +
> + if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
> + dev_err(priv->dev, "Invalid LED group %d for port %d",
> + led_group, port_num);
> + return -EINVAL;
> + }
This check seems odd. You can validate it once when you create the
struct rtl8366rb_led. After that, just trust it?
Andrew
> + properties:
> + reg:
> + description:
> + "reg indicates the LED group for this LED"
> + enum: [0, 1, 2, 3]
Having read the code, i don't think the binding needs to say anything
about the group. It is implicit. Port LED 0 is in group 0. Port LED 1
is in group 1. So there is nothing here which is outside of the
standard LED binding. So as Linus said, i don't think you need any
YAML at all, the DSA port binding should be sufficient.
Andrew
On Sun, 10 Mar 2024 01:51:58 -0300, Luiz Angelo Daros de Luca wrote:
> Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> LED reg property will indicate its LED group.
>
> An example of LED usage was included in an existing switch example.
>
> Cc: [email protected]
> Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
> ---
> .../devicetree/bindings/net/dsa/realtek.yaml | 46 ++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
/Documentation/devicetree/bindings/net/dsa/realtek.yaml:121:15: [error] string value is redundantly quoted with any quotes (quoted-strings)
/Documentation/devicetree/bindings/net/dsa/realtek.yaml:131:23: [error] string value is redundantly quoted with any quotes (quoted-strings)
dtschema/dtc warnings/errors:
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
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
Hi Krzysztof
> On 10/03/2024 05:51, Luiz Angelo Daros de Luca wrote:
> > Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> > LED reg property will indicate its LED group.
> >
>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
>
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline), work on fork of kernel
> (don't, instead use mainline) or you ignore some maintainers (really
> don't). Just use b4 and everything should be fine, although remember
> about `b4 prep --auto-to-cc` if you added new patches to the patchset.
>
> > An example of LED usage was included in an existing switch example.
> >
> > Cc: [email protected]
>
> Please drop the autogenerated scripts/get_maintainer.pl CC-entries from
> commit msg. There is no single need to store automated output of
> get_maintainers.pl in the git log. It can be easily re-created at any
> given time, thus its presence in the git history is redundant and
> obfuscates the log.
It is a left-over before I adopted b4. I'll do the cleanup.
>
> > +patternProperties:
> > + '^(ethernet-)?ports$':
> > + type: object
> > + additionalProperties: true
> > +
> > + patternProperties:
> > + '^(ethernet-)?port@[0-6]$':
> > + type: object
> > + additionalProperties: true
> > +
> > + properties:
> > + leds:
>
> type: object
> additionalProperties: false
>
> > + description:
> > + "LEDs associated with this port"
>
> Drop quotes.
At some in my frequent system upgrades (rolling release), it
uninstalled the yamllint
warning: python package 'yamllint' not installed, skipping
It should have catched that before.
>
> > +
> > + patternProperties:
> > + '^led@[a-f0-9]+$':
>
> [0-3]
leds are already defined for a port. I'm just trying to add a
restriction to allow only 0-3 leds and use that to identify the group.
These suggestions will redefine the leds property, forcing me to
declare #address-cells, #size-cells for leds and reference the led
schema in led@[0-3]. Is there a way to just add a constraint to what
is already present?
>
> > + type: object
> > + additionalProperties: true
>
> This cannot be 'true'. Which then will point you to errors and missing
> ref to leds schema and need to use unevaluatedProperties: false.
>
>
> > +
> > + properties:
> > + reg:
> > + description:
> > + "reg indicates the LED group for this LED"
>
> Drop quotes
>
>
> Best regards,
> Krzysztof
>
Hi Linus,
>
> > Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> > LED reg property will indicate its LED group.
> >
> > An example of LED usage was included in an existing switch example.
> >
> > Cc: [email protected]
> > Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
>
> But... is this patch even needed at all?
Yes, leds is a property already supported. The only purpose for this
patch is to limit led numbers to 0-3 and describe that the reg number
is the group.
I don't know if there is a better way to add restrictions to something
already defined. Anyway, if that is not necessary, I can drop it.
Regards,
Luiz
Hi Andrew,
>
> > + properties:
> > + reg:
> > + description:
> > + "reg indicates the LED group for this LED"
> > + enum: [0, 1, 2, 3]
>
> Having read the code, i don't think the binding needs to say anything
> about the group. It is implicit. Port LED 0 is in group 0. Port LED 1
> is in group 1. So there is nothing here which is outside of the
> standard LED binding. So as Linus said, i don't think you need any
> YAML at all, the DSA port binding should be sufficient.
The only purpose for this patch is to limit led numbers to 0-3 and
describe that the reg number is the group. If that is not important,
I'll just drop it.
Regards,
Luiz
Hi Rob,
> On Sun, 10 Mar 2024 01:51:58 -0300, Luiz Angelo Daros de Luca wrote:
> > Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> > LED reg property will indicate its LED group.
> >
> > An example of LED usage was included in an existing switch example.
> >
> > Cc: [email protected]
> > Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
> > ---
> > .../devicetree/bindings/net/dsa/realtek.yaml | 46 ++++++++++++++++++++++
> > 1 file changed, 46 insertions(+)
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/net/dsa/realtek.yaml:121:15: [error] string value is redundantly quoted with any quotes (quoted-strings)
> ./Documentation/devicetree/bindings/net/dsa/realtek.yaml:131:23: [error] string value is redundantly quoted with any quotes (quoted-strings)
Thanks for the tip. I just noticed my yamllint got uninstalled,
probably related to a python upgrade, and I missed the warning.
warning: python package 'yamllint' not installed, skipping
Sorry for the fuss.
Regards,
Luiz
Hi Andrew,
Em dom., 10 de mar. de 2024 às 15:02, Andrew Lunn <[email protected]> escreveu:
>> On Sun, Mar 10, 2024 at 01:51:58AM -0300, Luiz Angelo Daros de Luca wrote:
> > Each port can have up to 4 LEDs (3 for current rtl8365mb devices). The
> > LED reg property will indicate its LED group.
> > + properties:
> > + reg:
> > + description:
> > + "reg indicates the LED group for this LED"
> > + enum: [0, 1, 2, 3]
>
> If this identifies the group, what identifies the actual LED? There
> are four of them. How do i say LEDs 0 and 1 are unused, 2 and 3 are
> wired to LEDs?
Do we need to mention a specific HW is not present? I guess we just
omit it, like the led@1 and led@2 in the example.
> It would be much more usual for reg to be the LED number for the
> port. And there then be a group property indicating what group the LED
> belongs to. However, i'm wondering, is group fixed? Do we actually
> need it in DT, or can there just be a table in the driver which maps
> port:led to group?
The LED groups are a fixed HW feature. The chip has dedicated pins for
each LED in each group. Those pins might have an alternative usage (as
an GPIO) but you cannot change a LED from a group to another in SW.
That's why I thought it was a thing to be described in the DT.
> Andrew
Regards,
Luiz
On 21/03/2024 12:56, Luiz Angelo Daros de Luca wrote:
>
>>
>>> +
>>> + patternProperties:
>>> + '^led@[a-f0-9]+$':
>>
>> [0-3]
>
> leds are already defined for a port. I'm just trying to add a
> restriction to allow only 0-3 leds and use that to identify the group.
Where is the restriction, in your original patch?
> These suggestions will redefine the leds property, forcing me to
How? I really do not see it.
> declare #address-cells, #size-cells for leds and reference the led
> schema in led@[0-3]. Is there a way to just add a constraint to what
> is already present?
I don't follow.
>
>>
>>> + type: object
>>> + additionalProperties: true
>>
>> This cannot be 'true'. Which then will point you to errors and missing
>> ref to leds schema and need to use unevaluatedProperties: false.
Best regards,
Krzysztof
Hi Krzysztof,
> >>
> >>> +
> >>> + patternProperties:
> >>> + '^led@[a-f0-9]+$':
> >>
> >> [0-3]
> >
> > leds are already defined for a port. I'm just trying to add a
> > restriction to allow only 0-3 leds and use that to identify the group.
>
> Where is the restriction, in your original patch?
I tried to limit the led index to [0-3] (from the original
'^led@[a-f0-9]+$') and reg also to [0-3] (originally not constrained).
>
> > These suggestions will redefine the leds property, forcing me to
>
> How? I really do not see it.
I thought it was including the previous object definition when I
mentioned the same property again. However, the
"unevaluatedProperties: false" made it clear that it is actually
replacing the previous declaration. Sorry for my lack of experience.
> > declare #address-cells, #size-cells for leds and reference the led
> > schema in led@[0-3]. Is there a way to just add a constraint to what
> > is already present?
>
> I don't follow.
I would like to somehow add a restriction without replacing the
existing subnode definition. I'm not sure if the YAML scheme can
modify an heritaged sub-sub-property without redefining it all over
again or if the parent object requires a specific subobject property.
Anyway, the discussion ended up concluding that it was not worth it to
add such complexity for this situation.
Thank you for your time.
>
> >
> >>
> >>> + type: object
> >>> + additionalProperties: true
> >>
> >> This cannot be 'true'. Which then will point you to errors and missing
> >> ref to leds schema and need to use unevaluatedProperties: false.
> Best regards,
> Krzysztof
>
Regards,
Luiz
> On Sun, Mar 10, 2024 at 01:52:01AM -0300, Luiz Angelo Daros de Luca wrote:
> > This commit introduces LED drivers for rtl8366rb, enabling LEDs to be
> > described in the device tree using the same format as qca8k. Each port
> > can configure up to 4 LEDs.
> >
> > If all LEDs in a group use the default state "keep", they will use the
> > default behavior after a reset. Changing the brightness of one LED,
> > either manually or by a trigger, will disable the default hardware
> > trigger and switch the entire LED group to manually controlled LEDs.
> > Once in this mode, there is no way to revert to hardware-controlled LEDs
> > (except by resetting the switch).
> >
> > Software triggers function as expected with manually controlled LEDs.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <[email protected]>
> > Reviewed-by: Linus Walleij <[email protected]>
> > ---
> > drivers/net/dsa/realtek/rtl8366rb.c | 270 ++++++++++++++++++++++++++++++++----
>
> ...
>
> > +static int rtl8366rb_setup_led(struct realtek_priv *priv, struct dsa_port *dp,
> > + struct fwnode_handle *led_fwnode)
> > +{
> > + struct rtl8366rb *rb = priv->chip_data;
> > + struct led_init_data init_data = { };
> > + struct rtl8366rb_led *led;
> > + enum led_default_state state;
> > + u32 led_group;
> > + int ret;
>
> nit: Please consider using reverse xmas tree - longest line to shortest -
> for local variables in networking code.
Sorry, I might have renamed a variable to a shorter form without
rechecking the order. Thanks for the tip.
> ...
>
> > +static int rtl8366rb_setup_leds(struct realtek_priv *priv)
> > +{
> > + struct device_node *leds_np, *led_np;
> > + struct dsa_switch *ds = &priv->ds;
> > + struct dsa_port *dp;
> > + int ret;
> > +
> > + dsa_switch_for_each_port(dp, ds) {
> > + if (!dp->dn)
> > + continue;
> > +
> > + leds_np = of_get_child_by_name(dp->dn, "leds");
> > + if (!leds_np) {
> > + dev_dbg(priv->dev, "No leds defined for port %d",
> > + dp->index);
> > + continue;
> > + }
> > +
> > + for_each_child_of_node(leds_np, led_np) {
> > + ret = rtl8366rb_setup_led(priv, dp,
> > + of_fwnode_handle(led_np));
> > + if (ret) {
> > + of_node_put(led_np);
>
> FWIIW, Coccinelle complans about "probable double put" here.
> But it looks correct to me as it's only called when breaking out of
> the loop, when it is required AFAIK.
I agree. I do think the put is required when you break the loop as the
put happens in the increment/decrement code, including the last one
when it naturally ends the loop with led_np defined as null.
>
> > + break;
> > + }
> > + }
> > +
> > + of_node_put(leds_np);
> > + if (ret)
>
> I'm unsure if this can happen. But if for_each_child_of_node()
> iterates zero times then ret may be uninitialised here.
>
> Flagged by Smatch.
Yes, I'll initialize it as 0. It could also use if (led_np), as it
will only be defined if the loop was broken (but checking ret seems to
be simpler).
>
> > + return ret;
> > + }
> > + return 0;
> > +}
>
> ...
Hi Andrew,
Thanks for the review.
> > This commit introduces LED drivers for rtl8366rb, enabling LEDs to be
> > described in the device tree using the same format as qca8k. Each port
> > can configure up to 4 LEDs.
> >
> > If all LEDs in a group use the default state "keep", they will use the
> > default behavior after a reset. Changing the brightness of one LED,
> > either manually or by a trigger, will disable the default hardware
> > trigger and switch the entire LED group to manually controlled LEDs.
>
>
> The previous patch said:
>
> 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.
>
> I could be understanding this wrongly, but to me, it sounds like an
> LED is either controlled via the group, or you can take an LED out of
> the group and software on/off control it? Ah, after looking at the
> code. The group can be put into forced mode, and then each LED in the
> group controlled in software.
The group of a LED is a HW property. There are pins for each LED on
each group. You cannot move LEDs out of a group, not even to simply
disable a single LED.
The trigger mode is the same for all LEDs in a group as it is a LED
group property and not a LED characteristic. There is a special
trigger mode (manual) that disables HW triggers and lets the LEDs be
controlled by software triggers using a register bitmap.
> > Once in this mode, there is no way to revert to hardware-controlled LEDs
> > (except by resetting the switch).
>
> Just for my understanding.... This is a software limitation. You could
> check if all LEDs in a group are using the same trigger, and then set
> the group to that trigger?
I tried to implement that but I failed. There was some discussion
about it in the RFC thread. The main issue is that hw offload is only
evaluated when a LED changes its sysfs settings. The driver has
limited control about the hw offload decision, only being notified
when led_cdev->hw_control_is_supported() is called. The driver will
not be notified, for example, if the trigger_data->net_dev was changed
or if hw_control was disabled. However, even if you know a HW trigger
could be enabled, you cannot put those other LEDs in HW offload mode.
It is only enabled from sysfs calls but not from the kernel space and
AFAIK, you should not poke with sysfs from the kernel space.
The incompatibility with LDE API also has some unexpected
side-effects. For example, LEDS_DEFSTATE_KEEP will only really keep
the default vendor state if all LEDs in a group use that setting or
are missing in the DT. If one of them differs, it will switch the
group to manual mode.
> I do understand how the current offload concept causes problems here.
> You need a call into the trigger to ask it to re-evaluate if offload
> can be performed for an LED.
The trigger_data->hw_control is only set from netdev_led_attr_store()
(or during trigger activation). That code is only exposed to sysfs
calls. We'll need an exported function that could set that. Also, the
driver can only control that decision from
led_cdev->hw_control_is_supported. However, it is not enough to
surelly decide if a LED is still in a state that would support HW
offload (i.e. because trigger_data->net_dev could have changed). So,
we need to:
1) expose internal ledtrig-netdev data required for deciding if
offload is supported for any LED at any time
2) expose a way to reevaluate trigger_data->hw_control (or a way to
forcely enable it)
> What you have here seems like a good first step, offloaded could be
> added later if somebody wants to.
Yes, it is, at least, working. The current code is simply not usable.
> > +enum rtl8366_led_mode {
> > + RTL8366RB_LED_OFF = 0x0,
> > + RTL8366RB_LED_DUP_COL = 0x1,
> > + RTL8366RB_LED_LINK_ACT = 0x2,
> > + RTL8366RB_LED_SPD1000 = 0x3,
> > + RTL8366RB_LED_SPD100 = 0x4,
> > + RTL8366RB_LED_SPD10 = 0x5,
> > + RTL8366RB_LED_SPD1000_ACT = 0x6,
> > + RTL8366RB_LED_SPD100_ACT = 0x7,
> > + RTL8366RB_LED_SPD10_ACT = 0x8,
> > + RTL8366RB_LED_SPD100_10_ACT = 0x9,
> > + RTL8366RB_LED_FIBER = 0xa,
> > + RTL8366RB_LED_AN_FAULT = 0xb,
> > + RTL8366RB_LED_LINK_RX = 0xc,
> > + RTL8366RB_LED_LINK_TX = 0xd,
> > + RTL8366RB_LED_MASTER = 0xe,
> > + RTL8366RB_LED_FORCE = 0xf,
>
> This is what the group shows? Maybe put _GROUP_ into the name? This
> concept of a group is pretty unusual, so we should be careful with
> naming to make it clear when we are referring to one LED or a group of
> LEDs. I would also put _group_ into the enum.
I don't know if this concept of group is unusual but LED API
definitely does not handle it well.
OK, I'll add _group_/_GROUP_ both to the enum name and macros. Led
blink rate, for example, is global, used by all groups. However, it
will be difficult to respect the 80 columns limit passing
RTL8366RB_LED_GROUP_OFF to a rb8366rb_set_ledgroup_mode function with
only two levels of indentation. Do you have any recommendations?
>
> > +
> > + __RTL8366RB_LED_MAX
> > +};
> > +
> > +struct rtl8366rb_led {
> > + u8 port_num;
> > + u8 led_group;
> > + struct realtek_priv *priv;
> > + struct led_classdev cdev;
> > +};
> > +
> > /**
> > * struct rtl8366rb - RTL8366RB-specific data
> > * @max_mtu: per-port max MTU setting
> > * @pvid_enabled: if PVID is set for respective port
> > + * @leds: per-port and per-ledgroup led info
> > */
> > struct rtl8366rb {
> > unsigned int max_mtu[RTL8366RB_NUM_PORTS];
> > bool pvid_enabled[RTL8366RB_NUM_PORTS];
> > + struct rtl8366rb_led leds[RTL8366RB_NUM_PORTS][RTL8366RB_NUM_LEDGROUPS];
> > };
> >
> > static struct rtl8366_mib_counter rtl8366rb_mib_counters[] = {
> > @@ -809,6 +829,208 @@ static int rtl8366rb_jam_table(const struct rtl8366rb_jam_tbl_entry *jam_table,
> > return 0;
> > }
> >
> > +static int rb8366rb_set_ledgroup_mode(struct realtek_priv *priv,
> > + u8 led_group,
> > + enum rtl8366_led_mode mode)
> > +{
> > + int ret;
> > + u32 val;
> > +
> > + val = mode << RTL8366RB_LED_CTRL_OFFSET(led_group);
> > +
> > + ret = regmap_update_bits(priv->map,
> > + RTL8366RB_LED_CTRL_REG,
> > + RTL8366RB_LED_CTRL_MASK(led_group),
> > + val);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static inline u32 rtl8366rb_led_group_port_mask(u8 led_group, u8 port)
> > +{
> > + switch (led_group) {
> > + case 0:
> > + return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> > + case 1:
> > + return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> > + case 2:
> > + return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> > + case 3:
> > + return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static int rb8366rb_get_port_led(struct rtl8366rb_led *led, bool enable)
>
> enable seems unused here. It also seems an odd parameter to pass to a
> _get_ function.
Yes, copy/paste mistake. Thanks.
>
> > +{
> > + struct realtek_priv *priv = led->priv;
> > + u8 led_group = led->led_group;
> > + u8 port_num = led->port_num;
> > + int ret;
> > + u32 val;
> > +
> > + if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
> > + dev_err(priv->dev, "Invalid LED group %d for port %d",
> > + led_group, port_num);
> > + return -EINVAL;
> > + }
>
> This check seems odd. You can validate it once when you create the
> struct rtl8366rb_led. After that, just trust it?
Yes, I was redundant. If memory is intact, led->led_group is
guaranteed to be in range. I'll drop it in both get/set.
>
> Andrew
Regards,
Luiz
> OK, I'll add _group_/_GROUP_ both to the enum name and macros. Led
> blink rate, for example, is global, used by all groups. However, it
> will be difficult to respect the 80 columns limit passing
> RTL8366RB_LED_GROUP_OFF to a rb8366rb_set_ledgroup_mode function with
> only two levels of indentation. Do you have any recommendations?
https://www.kernel.org/doc/html/v4.10/process/coding-style.html
Now, some people will claim that having 8-character indentations
makes the code move too far to the right, and makes it hard to read
on a 80-character terminal screen. The answer to that is that if you
need more than 3 levels of indentation, you’re screwed anyway, and
should fix your program.
Functions should be short and sweet, and do just one thing. They
should fit on one or two screenfuls of text (the ISO/ANSI screen
size is 80x24, as we all know), and do one thing and do that well.
Maybe you need to use more helper functions?
Andrew
> > OK, I'll add _group_/_GROUP_ both to the enum name and macros. Led
> > blink rate, for example, is global, used by all groups. However, it
> > will be difficult to respect the 80 columns limit passing
> > RTL8366RB_LED_GROUP_OFF to a rb8366rb_set_ledgroup_mode function with
> > only two levels of indentation. Do you have any recommendations?
Hi Andrew,
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html
>
> Now, some people will claim that having 8-character indentations
> makes the code move too far to the right, and makes it hard to read
> on a 80-character terminal screen. The answer to that is that if you
> need more than 3 levels of indentation, you’re screwed anyway, and
> should fix your program.
I need 3, not more than 3.
> Functions should be short and sweet, and do just one thing. They
> should fit on one or two screenfuls of text (the ISO/ANSI screen
> size is 80x24, as we all know), and do one thing and do that well.
>
> Maybe you need to use more helper functions?
The call that violates (by 1) the limit is to
rb8366rb_set_ledgroup_mode(). With its name (a little long), the now
5-char longer macro/enum and 3 tabs, it has 81 columns when I align
the argument to the opening parenthesis.
static int rtl8366rb_setup(struct dsa_switch *ds)
{
(...)
if (priv->leds_disabled) {
/* Turn everything off */
regmap_update_bits(priv->map,
RTL8366RB_INTERRUPT_CONTROL_REG,
RTL8366RB_P4_RGMII_LED,
0);
for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) {
ret = rb8366rb_set_ledgroup_mode(priv, i,
RTL8366RB_LEDGROUP_OFF);
if (ret)
return ret;
}
}
}
Should I rename the rb8366rb_set_ledgroup_mode function,
RTL8366RB_LEDGROUP_OFF or is the violation here acceptable?
Can I use the double tab indentation here like it appears in
https://elixir.bootlin.com/linux/latest/source/net/8021q/vlanproc.c#L120?
Regards,
Luiz
On 24/03/2024 03:10, Luiz Angelo Daros de Luca wrote:
> Hi Krzysztof,
>
>>>>
>>>>> +
>>>>> + patternProperties:
>>>>> + '^led@[a-f0-9]+$':
>>>>
>>>> [0-3]
>>>
>>> leds are already defined for a port. I'm just trying to add a
>>> restriction to allow only 0-3 leds and use that to identify the group.
>>
>> Where is the restriction, in your original patch?
>
> I tried to limit the led index to [0-3] (from the original
> '^led@[a-f0-9]+$') and reg also to [0-3] (originally not constrained).
Where? I asked where, not what you tried to do. I don't think you tried
to add any restriction on 0-3 leds.
Best regards,
Krzysztof
> static int rtl8366rb_setup(struct dsa_switch *ds)
> {
> (...)
> if (priv->leds_disabled) {
if (!priv->leds_disable)
return;
Saves you one level of indentation.
or
static int rtl8366rb_setup_all_off(foo *priv)
{
> /* Turn everything off */
> regmap_update_bits(priv->map,
> RTL8366RB_INTERRUPT_CONTROL_REG,
> RTL8366RB_P4_RGMII_LED,
> 0);
>
> for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) {
> ret = rb8366rb_set_ledgroup_mode(priv, i,
> RTL8366RB_LEDGROUP_OFF);
> if (ret)
> return ret;
> }
> }
> }
This is what i meant by small helpers. And the function names replaces
the need for the comment.
This is part of the thinking behind the coding style. Function names
can replace comments. And the higher level functions becomes easier to
follow because you don't need to dig into the details to understand:
if (priv->leds_disabled)
rtl8366rb_setup_all_off(priv);
Andrew