| Message ID | 20220503151633.18760-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | Adds support for PHY LEDs with offload triggers | expand |
On Tue, 03 May 2022 17:16:33 +0200, Ansuel Smith wrote: > Add LEDs definition example for qca8k using the offload trigger as the > default trigger and add all the supported offload triggers by the > switch. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > .../devicetree/bindings/net/dsa/qca8k.yaml | 20 +++++++++++++++++++ > 1 file changed, 20 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: dtschema/dtc warnings/errors: Error: Documentation/devicetree/bindings/net/dsa/qca8k.example.dts:209.42-43 syntax error FATAL ERROR: Unable to parse input tree make[1]: *** [scripts/Makefile.lib:364: Documentation/devicetree/bindings/net/dsa/qca8k.example.dtb] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1401: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
On Tue, May 03, 2022 at 05:16:33PM +0200, Ansuel Smith wrote: > Add LEDs definition example for qca8k using the offload trigger as the > default trigger and add all the supported offload triggers by the > switch. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > .../devicetree/bindings/net/dsa/qca8k.yaml | 20 +++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > index f3c88371d76c..9b46ef645a2d 100644 > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > @@ -65,6 +65,8 @@ properties: > internal mdio access is used. > With the legacy mapping the reg corresponding to the internal > mdio is the switch reg with an offset of -1. > + Each phy have at least 3 LEDs connected and can be declared s/at least/up to/ ? Or your example is wrong with only 2. > + using the standard LEDs structure. > > patternProperties: > "^(ethernet-)?ports$": > @@ -287,6 +289,24 @@ examples: > > internal_phy_port1: ethernet-phy@0 { > reg = <0>; > + > + leds { > + led@0 { > + reg = <0>; > + color = <LED_COLOR_ID_WHITE>; > + function = LED_FUNCTION_LAN; > + function-enumerator = <1>; > + linux,default-trigger = "netdev"; > + }; > + > + led@1 { > + reg = <1>; > + color = <LED_COLOR_ID_AMBER>; > + function = LED_FUNCTION_LAN; > + function-enumerator = <1>; > + linux,default-trigger = "netdev"; > + }; > + }; > }; > > internal_phy_port2: ethernet-phy@1 { > -- > 2.34.1 > >
> +In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the > +requested blink mode (flags) or not. -EOPNOTSUPP might be clearer. > +In ZERO hw_control_configure() should return 0 with success operation or error. > + > +The unsigned long flag is specific to the trigger and change across them. It's in the LED > +driver interest know how to elaborate this flag and to declare support for a > +particular trigger. For this exact reason explicit support for the specific > +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode > +with a not supported trigger. > +If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will > +fail as the driver doesn't support that specific offload trigger or doesn't know > +how to handle the provided flags. > + > Known Issues > ============ > > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 09ff1dc6f48d..b5aad67fecfb 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -73,6 +73,16 @@ enum led_blink_modes { > SOFTWARE_HARDWARE_CONTROLLED, > }; > > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL > +enum blink_mode_cmd { > + BLINK_MODE_ENABLE, /* Enable the hardware blink mode */ > + BLINK_MODE_DISABLE, /* Disable the hardware blink mode */ > + BLINK_MODE_READ, /* Read the status of the hardware blink mode */ > + BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */ > + BLINK_MODE_ZERO, /* Disable any hardware blink active */ > +}; > +#endif Skip the #ifdef. The enum itself takes no space if never used, and it makes the driver simpler if they always exist. > + > struct led_classdev { > const char *name; > unsigned int brightness; > @@ -185,6 +195,17 @@ struct led_classdev { > * the old status but that is not mandatory and also putting it off is accepted. > */ > int (*hw_control_stop)(struct led_classdev *led_cdev); > + /* This will be used to configure the various blink modes LED support in hardware > + * mode. > + * The LED driver require to support the active trigger and will elaborate the > + * unsigned long flag and do the operation based on the provided cmd. > + * Current operation are enable,disable,supported and status. > + * A trigger will use this to enable or disable the asked blink mode, check the > + * status of the blink mode or ask if the blink mode can run in hardware mode. > + */ > + int (*hw_control_configure)(struct led_classdev *led_cdev, > + unsigned long flag, > + enum blink_mode_cmd cmd); > #endif > #endif > > @@ -454,6 +475,24 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev) > return led_cdev->trigger_data; > } > > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL > +static inline bool led_trigger_blink_mode_is_supported(struct led_classdev *led_cdev, > + unsigned long flag) > +{ > + int ret; > + > + /* Sanity check: make sure led support hw mode */ > + if (led_cdev->blink_mode == SOFTWARE_CONTROLLED) > + return false; > + > + ret = led_cdev->hw_control_configure(led_cdev, flag, BLINK_MODE_SUPPORTED); > + if (ret > 0) > + return true; > + > + return false; > +} > +#endif Please add a version which returns false when CONFIG_LEDS_HARDWARE_CONTROL is disabled. Does this actually need to be an inline function? Andrew
On Tue, May 03, 2022 at 05:16:25PM +0200, Ansuel Smith wrote: > Drop NETDEV_LED_MODE_LINKUP from mode list and convert to a simple bool > that will be true or false based on the carrier link. No functional > change intended. What is missing from the commit message is an explanation why? Andrew
> +struct netdev_led_attr_detail { > + char *name; > + bool hardware_only; > + enum led_trigger_netdev_modes bit; > +}; > + > +static struct netdev_led_attr_detail attr_details[] = { > + { .name = "link", .bit = TRIGGER_NETDEV_LINK}, > + { .name = "tx", .bit = TRIGGER_NETDEV_TX}, > + { .name = "rx", .bit = TRIGGER_NETDEV_RX}, hardware_only is never set. Maybe it is used in a later patch? If so, please introduce it there. > static void set_baseline_state(struct led_netdev_data *trigger_data) > { > + int i; > int current_brightness; > + struct netdev_led_attr_detail *detail; > struct led_classdev *led_cdev = trigger_data->led_cdev; This file mostly keeps with reverse christmas tree, probably because it was written by a netdev developer. It is probably not required for the LED subsystem, but it would be nice to keep the file consistent. > @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev, > size_t size) > { > struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev); > + struct net_device *old_net = trigger_data->net_dev; > + char old_device_name[IFNAMSIZ]; > > if (size >= IFNAMSIZ) > return -EINVAL; > > + /* Backup old device name */ > + memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ); > + > cancel_delayed_work_sync(&trigger_data->work); > > spin_lock_bh(&trigger_data->lock); > @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev, > trigger_data->net_dev = > dev_get_by_name(&init_net, trigger_data->device_name); > > + if (!validate_baseline_state(trigger_data)) { You probably want to validate trigger_data->net_dev is not NULL first. The current code is a little odd with that, > + /* Restore old net_dev and device_name */ > + if (trigger_data->net_dev) > + dev_put(trigger_data->net_dev); > + > + dev_hold(old_net); This dev_hold() looks wrong. It is trying to undo a dev_put() somewhere? You should not actually do a put until you know you really do not old_net, otherwise there is a danger it disappears and you cannot undo. > @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev, > return ret; > > /* impose some basic bounds on the timer interval */ > - if (value >= 5 && value <= 10000) { > - cancel_delayed_work_sync(&trigger_data->work); > + if (value < 5 || value > 10000) > + return -EINVAL; > + > + cancel_delayed_work_sync(&trigger_data->work); > + > + atomic_set(&trigger_data->interval, msecs_to_jiffies(value)); > > - atomic_set(&trigger_data->interval, msecs_to_jiffies(value)); > - set_baseline_state(trigger_data); /* resets timer */ > + if (!validate_baseline_state(trigger_data)) { > + /* Restore old interval on validation error */ > + atomic_set(&trigger_data->interval, old_interval); > + trigger_data->mode = old_mode; I think you need to schedule the work again, since you cancelled it. It is at the end of the work that the next work is scheduled, and so it will not self recover. Andrew
> +config NET_DSA_QCA8K_LEDS_SUPPORT > + tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support" > + select NET_DSA_QCA8K The should be a depends, not a select. It will then become visible when the NET_DSA_QCA8K directly above it is enabled. > + select LEDS_OFFLOAD_TRIGGERS and this should also be a depends. If the LED core does not have support, the QCA8K driver should not enable its support. > +static int > +qca8k_parse_netdev(unsigned long rules, u32 *offload_trigger, u32 *mask) > +{ > + /* Parsing specific to netdev trigger */ > + if (test_bit(TRIGGER_NETDEV_LINK, &rules)) > + *offload_trigger = QCA8K_LED_LINK_10M_EN_MASK | > + QCA8K_LED_LINK_100M_EN_MASK | > + QCA8K_LED_LINK_1000M_EN_MASK; > + if (test_bit(TRIGGER_NETDEV_LINK_10, &rules)) > + *offload_trigger = QCA8K_LED_LINK_10M_EN_MASK; > + if (test_bit(TRIGGER_NETDEV_LINK_100, &rules)) > + *offload_trigger = QCA8K_LED_LINK_100M_EN_MASK; > + if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules)) > + *offload_trigger = QCA8K_LED_LINK_1000M_EN_MASK; > + if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules)) > + *offload_trigger = QCA8K_LED_HALF_DUPLEX_MASK; > + if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules)) > + *offload_trigger = QCA8K_LED_FULL_DUPLEX_MASK; > + if (test_bit(TRIGGER_NETDEV_TX, &rules)) > + *offload_trigger = QCA8K_LED_TX_BLINK_MASK; > + if (test_bit(TRIGGER_NETDEV_RX, &rules)) > + *offload_trigger = QCA8K_LED_RX_BLINK_MASK; > + if (test_bit(TRIGGER_NETDEV_BLINK_2HZ, &rules)) > + *offload_trigger = QCA8K_LED_BLINK_2HZ; > + if (test_bit(TRIGGER_NETDEV_BLINK_4HZ, &rules)) > + *offload_trigger = QCA8K_LED_BLINK_4HZ; > + if (test_bit(TRIGGER_NETDEV_BLINK_8HZ, &rules)) > + *offload_trigger = QCA8K_LED_BLINK_8HZ; > + > + pr_info("OFFLOAD TRIGGER %x\n", *offload_trigger); leftover debug print. Andrew
> + ret = fwnode_property_read_string(led, "default-state", &state);
You should probably use led_default_state led_init_default_state_get()
Andrew
On Thu, May 05, 2022 at 01:23:36AM +0200, Andrew Lunn wrote: > > +In SUPPORTED hw_control_configure() should return 0 or 1 if the LED driver supports the > > +requested blink mode (flags) or not. > > -EOPNOTSUPP might be clearer. > > > > +In ZERO hw_control_configure() should return 0 with success operation or error. > > + > > +The unsigned long flag is specific to the trigger and change across them. It's in the LED > > +driver interest know how to elaborate this flag and to declare support for a > > +particular trigger. For this exact reason explicit support for the specific > > +trigger is mandatory or the driver returns -EOPNOTSUPP if asked to enter offload mode > > +with a not supported trigger. > > +If the driver returns -EOPNOTSUPP on hw_control_configure(), the trigger activation will > > +fail as the driver doesn't support that specific offload trigger or doesn't know > > +how to handle the provided flags. > > + > > Known Issues > > ============ > > > > diff --git a/include/linux/leds.h b/include/linux/leds.h > > index 09ff1dc6f48d..b5aad67fecfb 100644 > > --- a/include/linux/leds.h > > +++ b/include/linux/leds.h > > @@ -73,6 +73,16 @@ enum led_blink_modes { > > SOFTWARE_HARDWARE_CONTROLLED, > > }; > > > > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL > > +enum blink_mode_cmd { > > + BLINK_MODE_ENABLE, /* Enable the hardware blink mode */ > > + BLINK_MODE_DISABLE, /* Disable the hardware blink mode */ > > + BLINK_MODE_READ, /* Read the status of the hardware blink mode */ > > + BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */ > > + BLINK_MODE_ZERO, /* Disable any hardware blink active */ > > +}; > > +#endif > > Skip the #ifdef. The enum itself takes no space if never used, and it > makes the driver simpler if they always exist. > > > + > > struct led_classdev { > > const char *name; > > unsigned int brightness; > > @@ -185,6 +195,17 @@ struct led_classdev { > > * the old status but that is not mandatory and also putting it off is accepted. > > */ > > int (*hw_control_stop)(struct led_classdev *led_cdev); > > + /* This will be used to configure the various blink modes LED support in hardware > > + * mode. > > + * The LED driver require to support the active trigger and will elaborate the > > + * unsigned long flag and do the operation based on the provided cmd. > > + * Current operation are enable,disable,supported and status. > > + * A trigger will use this to enable or disable the asked blink mode, check the > > + * status of the blink mode or ask if the blink mode can run in hardware mode. > > + */ > > + int (*hw_control_configure)(struct led_classdev *led_cdev, > > + unsigned long flag, > > + enum blink_mode_cmd cmd); > > #endif > > #endif > > > > @@ -454,6 +475,24 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev) > > return led_cdev->trigger_data; > > } > > > > +#ifdef CONFIG_LEDS_HARDWARE_CONTROL > > +static inline bool led_trigger_blink_mode_is_supported(struct led_classdev *led_cdev, > > + unsigned long flag) > > +{ > > + int ret; > > + > > + /* Sanity check: make sure led support hw mode */ > > + if (led_cdev->blink_mode == SOFTWARE_CONTROLLED) > > + return false; > > + > > + ret = led_cdev->hw_control_configure(led_cdev, flag, BLINK_MODE_SUPPORTED); > > + if (ret > 0) > > + return true; > > + > > + return false; > > +} > > +#endif > > Please add a version which returns false when > CONFIG_LEDS_HARDWARE_CONTROL is disabled. > > Does this actually need to be an inline function? Not at all... Originally it was a smaller function. Will drop it. > > Andrew
On Thu, May 05, 2022 at 01:25:31AM +0200, Andrew Lunn wrote: > On Tue, May 03, 2022 at 05:16:25PM +0200, Ansuel Smith wrote: > > Drop NETDEV_LED_MODE_LINKUP from mode list and convert to a simple bool > > that will be true or false based on the carrier link. No functional > > change intended. > > What is missing from the commit message is an explanation why? > > Andrew Will add the reason. Just in case it doesn't make sense... The reason is that putting a state in the mode bitmap doesn't look correct. It's ""acceptable"" if we have only 3 state (rx, tx and link). It become problematic when we start to have 7 modes and a link up state should be handled differently. Does it make sense?
On Thu, May 05, 2022 at 03:00:03AM +0200, Andrew Lunn wrote: > > +struct netdev_led_attr_detail { > > + char *name; > > + bool hardware_only; > > + enum led_trigger_netdev_modes bit; > > +}; > > + > > +static struct netdev_led_attr_detail attr_details[] = { > > + { .name = "link", .bit = TRIGGER_NETDEV_LINK}, > > + { .name = "tx", .bit = TRIGGER_NETDEV_TX}, > > + { .name = "rx", .bit = TRIGGER_NETDEV_RX}, > > hardware_only is never set. Maybe it is used in a later patch? If so, > please introduce it there. > Is it better to introduce the hardware_only bool in the patch where the additional "hardware only" modes are added? > > static void set_baseline_state(struct led_netdev_data *trigger_data) > > { > > + int i; > > int current_brightness; > > + struct netdev_led_attr_detail *detail; > > struct led_classdev *led_cdev = trigger_data->led_cdev; > > This file mostly keeps with reverse christmas tree, probably because > it was written by a netdev developer. It is probably not required for > the LED subsystem, but it would be nice to keep the file consistent. > The order is a bit mixed as you notice. Ok will stick to reverse christmas. > > @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev, > > size_t size) > > { > > struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev); > > + struct net_device *old_net = trigger_data->net_dev; > > + char old_device_name[IFNAMSIZ]; > > > > if (size >= IFNAMSIZ) > > return -EINVAL; > > > > + /* Backup old device name */ > > + memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ); > > + > > cancel_delayed_work_sync(&trigger_data->work); > > > > spin_lock_bh(&trigger_data->lock); > > @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev, > > trigger_data->net_dev = > > dev_get_by_name(&init_net, trigger_data->device_name); > > > > + if (!validate_baseline_state(trigger_data)) { > > You probably want to validate trigger_data->net_dev is not NULL first. The current code > is a little odd with that, > The thing is that net_dev can be NULL and actually is a requirement for hardware_mode to be triggered. (net_dev must be NULL or software mode is forced) > > + /* Restore old net_dev and device_name */ > > + if (trigger_data->net_dev) > > + dev_put(trigger_data->net_dev); > > + > > + dev_hold(old_net); > > This dev_hold() looks wrong. It is trying to undo a dev_put() > somewhere? You should not actually do a put until you know you really > do not old_net, otherwise there is a danger it disappears and you > cannot undo. > Yes if you notice some lines above, the first thing done is to dev_put the current net_dev set. So on validation fail we restore the old state with holding the old_net again and restoring the device_name. But thanks for poiting it out... I should check if old_net is not NULL. Also should i change the logic and just dev_put if all goes well? (for example before the return size?) That way I should be able to skip this additional dev_hold. > > @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev, > > return ret; > > > > /* impose some basic bounds on the timer interval */ > > - if (value >= 5 && value <= 10000) { > > - cancel_delayed_work_sync(&trigger_data->work); > > + if (value < 5 || value > 10000) > > + return -EINVAL; > > + > > + cancel_delayed_work_sync(&trigger_data->work); > > + > > + atomic_set(&trigger_data->interval, msecs_to_jiffies(value)); > > > > - atomic_set(&trigger_data->interval, msecs_to_jiffies(value)); > > - set_baseline_state(trigger_data); /* resets timer */ > > + if (!validate_baseline_state(trigger_data)) { > > + /* Restore old interval on validation error */ > > + atomic_set(&trigger_data->interval, old_interval); > > + trigger_data->mode = old_mode; > > I think you need to schedule the work again, since you cancelled > it. It is at the end of the work that the next work is scheduled, and > so it will not self recover. > Ok I assume the correct way to handle this is to return error and still use the set_baseline_state... Or Also move the validate_baseline_state up before the cancel_delayed_work_sync. But considering we require atomic_set for the validation to work I think the right way is to set_baseline_state even with errors (as it will reschedule the work) > Andrew
On Thu, May 05, 2022 at 03:55:26AM +0200, Andrew Lunn wrote: > > + ret = fwnode_property_read_string(led, "default-state", &state); > > You should probably use led_default_state led_init_default_state_get() > > Andrew Oh, didn't know it was a thing, is this new? Anyway thanks.
On Wed, May 04, 2022 at 12:15:48PM -0500, Rob Herring wrote: > On Tue, May 03, 2022 at 05:16:33PM +0200, Ansuel Smith wrote: > > Add LEDs definition example for qca8k using the offload trigger as the > > default trigger and add all the supported offload triggers by the > > switch. > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > --- > > .../devicetree/bindings/net/dsa/qca8k.yaml | 20 +++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > > index f3c88371d76c..9b46ef645a2d 100644 > > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.yaml > > @@ -65,6 +65,8 @@ properties: > > internal mdio access is used. > > With the legacy mapping the reg corresponding to the internal > > mdio is the switch reg with an offset of -1. > > + Each phy have at least 3 LEDs connected and can be declared > > s/at least/up to/ ? > > Or your example is wrong with only 2. > Up to. Internally the regs are there but 99% of the times OEM just connect 2 of 3 LEDs. Will fix. > > + using the standard LEDs structure. > > > > patternProperties: > > "^(ethernet-)?ports$": > > @@ -287,6 +289,24 @@ examples: > > > > internal_phy_port1: ethernet-phy@0 { > > reg = <0>; > > + > > + leds { > > + led@0 { > > + reg = <0>; > > + color = <LED_COLOR_ID_WHITE>; > > + function = LED_FUNCTION_LAN; > > + function-enumerator = <1>; > > + linux,default-trigger = "netdev"; > > + }; > > + > > + led@1 { > > + reg = <1>; > > + color = <LED_COLOR_ID_AMBER>; > > + function = LED_FUNCTION_LAN; > > + function-enumerator = <1>; > > + linux,default-trigger = "netdev"; > > + }; > > + }; > > }; > > > > internal_phy_port2: ethernet-phy@1 { > > -- > > 2.34.1 > > > >
On Thu, May 05, 2022 at 03:33:07PM +0200, Ansuel Smith wrote: > On Thu, May 05, 2022 at 03:55:26AM +0200, Andrew Lunn wrote: > > > + ret = fwnode_property_read_string(led, "default-state", &state); > > > > You should probably use led_default_state led_init_default_state_get() > > > > Andrew > > Oh, didn't know it was a thing, is this new? Anyway thanks. No idea. But my thinking was, you cannot be the first to implement the binding, there probably exists some helpers somewhere... General rule of thumb: Assume somebody has already been there and done it, you just need to find it and reuse it. Andrew