* [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation
[not found] <20250902-leds-v1-0-4a31e125276b@vinarskis.com>
@ 2025-09-02 11:10 ` Aleksandrs Vinarskis
2025-09-02 17:57 ` Rob Herring (Arm)
2025-09-02 18:21 ` Rob Herring
2025-09-02 11:10 ` [PATCH 2/2] leds: led-class: Add devicetree support to led_get() Aleksandrs Vinarskis
1 sibling, 2 replies; 17+ messages in thread
From: Aleksandrs Vinarskis @ 2025-09-02 11:10 UTC (permalink / raw)
To: Hans de Goede, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bryan O'Donoghue
Cc: linux-leds, devicetree, linux-kernel, Aleksandrs Vinarskis
Currently supports passing 'led-names' used to map LED devices to their
respective functions.
Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
---
.../devicetree/bindings/leds/leds-consumer.yaml | 69 ++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/leds-consumer.yaml b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..a63e78417df84609e279835f7dae62e3ad2f0bf5
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-consumer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common leds consumer
+
+maintainers:
+ - Aleksandrs Vinarskis <alex@vinarskis.com>
+
+description:
+ Some LED defined in DT are required by other DT consumers, for example
+ v4l2 subnode may require privacy or flash LED.
+
+ Document LED properties that its consumers may define.
+
+properties:
+ leds:
+ description:
+ Phandle to LED device(s) required by particular consumer.
+ $ref: /schemas/types.yaml#/definitions/phandle
+ led-names:
+ description:
+ List of device name(s). Used to map LED devices to their respective
+ functions, when consumer requires more than one LED.
+
+additionalProperties: true
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/leds/common.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ camera@36 {
+ compatible = "ovti,ov02c10";
+ reg = <0x36>;
+
+ reset-gpios = <&tlmm 237 GPIO_ACTIVE_LOW>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&cam_rgb_default>;
+
+ led-names = "privacy-led";
+ leds = <&privacy_led>;
+
+ clocks = <&ov02e10_clk>;
+
+ assigned-clocks = <&ov02e10_clk>;
+ assigned-clock-rates = <19200000>;
+
+ avdd-supply = <&vreg_l7b_2p8>;
+ dvdd-supply = <&vreg_l7b_2p8>;
+ dovdd-supply = <&vreg_cam_1p8>;
+
+ port {
+ ov02e10_ep: endpoint {
+ data-lanes = <1 2>;
+ link-frequencies = /bits/ 64 <400000000>;
+ remote-endpoint = <&csiphy4_ep>;
+ };
+ };
+ };
+ };
+
+...
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] leds: led-class: Add devicetree support to led_get()
[not found] <20250902-leds-v1-0-4a31e125276b@vinarskis.com>
2025-09-02 11:10 ` [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation Aleksandrs Vinarskis
@ 2025-09-02 11:10 ` Aleksandrs Vinarskis
2025-09-02 12:25 ` Hans de Goede
` (2 more replies)
1 sibling, 3 replies; 17+ messages in thread
From: Aleksandrs Vinarskis @ 2025-09-02 11:10 UTC (permalink / raw)
To: Hans de Goede, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bryan O'Donoghue
Cc: linux-leds, devicetree, linux-kernel, Aleksandrs Vinarskis,
Andy Shevchenko, Linus Walleij, Hans de Goede
From: Hans de Goede <hansg@kernel.org>
Turn of_led_get() into a more generic __of_led_get() helper function,
which can lookup LEDs in devicetree by either name or index.
And use this new helper to add devicetree support to the generic
(non devicetree specific) [devm_]led_get() function.
This uses the standard devicetree pattern of adding a -names string array
to map names to the indexes for an array of resources.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Lee Jones <lee@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Aleksandrs Vinarskis <alex@vinarskis.com>
---
drivers/leds/led-class.c | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 15633fbf3c166aa4f521774d245f6399a642bced..6f2ef4fa556b44ed3bf69dff556ae16fd2b7652b 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -248,19 +248,18 @@ static const struct class leds_class = {
.pm = &leds_class_dev_pm_ops,
};
-/**
- * of_led_get() - request a LED device via the LED framework
- * @np: device node to get the LED device from
- * @index: the index of the LED
- *
- * Returns the LED device parsed from the phandle specified in the "leds"
- * property of a device tree node or a negative error-code on failure.
- */
-static struct led_classdev *of_led_get(struct device_node *np, int index)
+static struct led_classdev *__of_led_get(struct device_node *np, int index,
+ const char *name)
{
struct device *led_dev;
struct device_node *led_node;
+ /*
+ * For named LEDs, first look up the name in the "led-names" property.
+ * If it cannot be found, then of_parse_phandle() will propagate the error.
+ */
+ if (name)
+ index = of_property_match_string(np, "led-names", name);
led_node = of_parse_phandle(np, "leds", index);
if (!led_node)
return ERR_PTR(-ENOENT);
@@ -271,6 +270,20 @@ static struct led_classdev *of_led_get(struct device_node *np, int index)
return led_module_get(led_dev);
}
+/**
+ * of_led_get() - request a LED device via the LED framework
+ * @np: device node to get the LED device from
+ * @index: the index of the LED
+ *
+ * Returns the LED device parsed from the phandle specified in the "leds"
+ * property of a device tree node or a negative error-code on failure.
+ */
+struct led_classdev *of_led_get(struct device_node *np, int index)
+{
+ return __of_led_get(np, index, NULL);
+}
+EXPORT_SYMBOL_GPL(of_led_get);
+
/**
* led_put() - release a LED device
* @led_cdev: LED device
@@ -342,9 +355,16 @@ EXPORT_SYMBOL_GPL(devm_of_led_get);
struct led_classdev *led_get(struct device *dev, char *con_id)
{
struct led_lookup_data *lookup;
+ struct led_classdev *led_cdev;
const char *provider = NULL;
struct device *led_dev;
+ if (dev->of_node) {
+ led_cdev = __of_led_get(dev->of_node, -1, con_id);
+ if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
+ return led_cdev;
+ }
+
mutex_lock(&leds_lookup_lock);
list_for_each_entry(lookup, &leds_lookup_list, list) {
if (!strcmp(lookup->dev_id, dev_name(dev)) &&
--
2.48.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()
2025-09-02 11:10 ` [PATCH 2/2] leds: led-class: Add devicetree support to led_get() Aleksandrs Vinarskis
@ 2025-09-02 12:25 ` Hans de Goede
2025-09-03 23:01 ` Aleksandrs Vinarskis
2025-09-04 7:08 ` Hans de Goede
2025-09-02 22:22 ` Linus Walleij
2025-09-03 3:31 ` kernel test robot
2 siblings, 2 replies; 17+ messages in thread
From: Hans de Goede @ 2025-09-02 12:25 UTC (permalink / raw)
To: Aleksandrs Vinarskis, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bryan O'Donoghue
Cc: linux-leds, devicetree, linux-kernel, Andy Shevchenko,
Linus Walleij
Hi Aleksandrs,
Thank you for working on this.
On 2-Sep-25 1:10 PM, Aleksandrs Vinarskis wrote:
> From: Hans de Goede <hansg@kernel.org>
>
> Turn of_led_get() into a more generic __of_led_get() helper function,
> which can lookup LEDs in devicetree by either name or index.
>
> And use this new helper to add devicetree support to the generic
> (non devicetree specific) [devm_]led_get() function.
>
> This uses the standard devicetree pattern of adding a -names string array
> to map names to the indexes for an array of resources.
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Lee Jones <lee@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Please update this to:
Reviewed-by: Hans de Goede <hansg@kernel.org>
to match the update of the author which you already did.
Also note that checkpatch should complain about the mismatch,
please ensure to run checkpatch before posting v2.
> Tested-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> ---
> drivers/leds/led-class.c | 38 +++++++++++++++++++++++++++++---------
> 1 file changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 15633fbf3c166aa4f521774d245f6399a642bced..6f2ef4fa556b44ed3bf69dff556ae16fd2b7652b 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -248,19 +248,18 @@ static const struct class leds_class = {
> .pm = &leds_class_dev_pm_ops,
> };
>
> -/**
> - * of_led_get() - request a LED device via the LED framework
> - * @np: device node to get the LED device from
> - * @index: the index of the LED
> - *
> - * Returns the LED device parsed from the phandle specified in the "leds"
> - * property of a device tree node or a negative error-code on failure.
> - */
> -static struct led_classdev *of_led_get(struct device_node *np, int index)
> +static struct led_classdev *__of_led_get(struct device_node *np, int index,
> + const char *name)
> {
> struct device *led_dev;
> struct device_node *led_node;
>
> + /*
> + * For named LEDs, first look up the name in the "led-names" property.
> + * If it cannot be found, then of_parse_phandle() will propagate the error.
> + */
> + if (name)
> + index = of_property_match_string(np, "led-names", name);
> led_node = of_parse_phandle(np, "leds", index);
> if (!led_node)
> return ERR_PTR(-ENOENT);
> @@ -271,6 +270,20 @@ static struct led_classdev *of_led_get(struct device_node *np, int index)
> return led_module_get(led_dev);
> }
>
> +/**
> + * of_led_get() - request a LED device via the LED framework
> + * @np: device node to get the LED device from
> + * @index: the index of the LED
> + *
> + * Returns the LED device parsed from the phandle specified in the "leds"
> + * property of a device tree node or a negative error-code on failure.
> + */
> +struct led_classdev *of_led_get(struct device_node *np, int index)
> +{
> + return __of_led_get(np, index, NULL);
> +}
> +EXPORT_SYMBOL_GPL(of_led_get);
I probably did this myself, but since of_led_get() is private now
(I guess it was not private before?) and since we are moving away from
"of" specific functions to using generic dev_xxxx functions in the kernel
in general, I think this just should be a static helper.
Or probably even better: just add the name argument to of_led_get()
before without renaming it, update the existing callers to pass
an extra NULL arg and completely drop this wrapper.
Also notice that adding the EXPORT_SYMBOL_GPL() while there was
none before should go hand in hand with adding a prototype to
a relevant .h file. However please just keep this private and
drop the wrapper.
Regards,
Hans
> +
> /**
> * led_put() - release a LED device
> * @led_cdev: LED device
> @@ -342,9 +355,16 @@ EXPORT_SYMBOL_GPL(devm_of_led_get);
> struct led_classdev *led_get(struct device *dev, char *con_id)
> {
> struct led_lookup_data *lookup;
> + struct led_classdev *led_cdev;
> const char *provider = NULL;
> struct device *led_dev;
>
> + if (dev->of_node) {
> + led_cdev = __of_led_get(dev->of_node, -1, con_id);
> + if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
> + return led_cdev;
> + }
> +
> mutex_lock(&leds_lookup_lock);
> list_for_each_entry(lookup, &leds_lookup_list, list) {
> if (!strcmp(lookup->dev_id, dev_name(dev)) &&
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation
2025-09-02 11:10 ` [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation Aleksandrs Vinarskis
@ 2025-09-02 17:57 ` Rob Herring (Arm)
2025-09-02 18:21 ` Rob Herring
1 sibling, 0 replies; 17+ messages in thread
From: Rob Herring (Arm) @ 2025-09-02 17:57 UTC (permalink / raw)
To: Aleksandrs Vinarskis
Cc: Krzysztof Kozlowski, Hans de Goede, Lee Jones, devicetree,
linux-leds, linux-kernel, Conor Dooley, Bryan O'Donoghue,
Pavel Machek
On Tue, 02 Sep 2025 11:10:51 +0000, Aleksandrs Vinarskis wrote:
> Currently supports passing 'led-names' used to map LED devices to their
> respective functions.
>
> Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> ---
> .../devicetree/bindings/leds/leds-consumer.yaml | 69 ++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/leds/leds-consumer.example.dtb: camera@36 (ovti,ov02c10): Unevaluated properties are not allowed ('led-names', 'leds' were unexpected)
from schema $id: http://devicetree.org/schemas/media/i2c/ovti,ov02e10.yaml#
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/010201990a1f5ad8-fc97fc84-9ef9-4a03-bf1c-2d54423c6497-000000@eu-west-1.amazonses.com
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation
2025-09-02 11:10 ` [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation Aleksandrs Vinarskis
2025-09-02 17:57 ` Rob Herring (Arm)
@ 2025-09-02 18:21 ` Rob Herring
2025-09-03 23:56 ` Aleksandrs Vinarskis
1 sibling, 1 reply; 17+ messages in thread
From: Rob Herring @ 2025-09-02 18:21 UTC (permalink / raw)
To: Aleksandrs Vinarskis
Cc: Hans de Goede, Lee Jones, Pavel Machek, Krzysztof Kozlowski,
Conor Dooley, Bryan O'Donoghue, linux-leds, devicetree,
linux-kernel
On Tue, Sep 02, 2025 at 11:10:51AM +0000, Aleksandrs Vinarskis wrote:
> Currently supports passing 'led-names' used to map LED devices to their
> respective functions.
>
> Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> ---
> .../devicetree/bindings/leds/leds-consumer.yaml | 69 ++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-consumer.yaml b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..a63e78417df84609e279835f7dae62e3ad2f0bf5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-consumer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common leds consumer
> +
> +maintainers:
> + - Aleksandrs Vinarskis <alex@vinarskis.com>
> +
> +description:
> + Some LED defined in DT are required by other DT consumers, for example
> + v4l2 subnode may require privacy or flash LED.
> +
> + Document LED properties that its consumers may define.
We already have the trigger-source binding for "attaching" LEDs to
devices. Why does that not work here?
Rob
> +
> +properties:
> + leds:
> + description:
> + Phandle to LED device(s) required by particular consumer.
> + $ref: /schemas/types.yaml#/definitions/phandle
> + led-names:
> + description:
> + List of device name(s). Used to map LED devices to their respective
> + functions, when consumer requires more than one LED.
> +
> +additionalProperties: true
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/leds/common.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + camera@36 {
> + compatible = "ovti,ov02c10";
> + reg = <0x36>;
> +
> + reset-gpios = <&tlmm 237 GPIO_ACTIVE_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&cam_rgb_default>;
> +
> + led-names = "privacy-led";
> + leds = <&privacy_led>;
> +
> + clocks = <&ov02e10_clk>;
> +
> + assigned-clocks = <&ov02e10_clk>;
> + assigned-clock-rates = <19200000>;
> +
> + avdd-supply = <&vreg_l7b_2p8>;
> + dvdd-supply = <&vreg_l7b_2p8>;
> + dovdd-supply = <&vreg_cam_1p8>;
> +
> + port {
> + ov02e10_ep: endpoint {
> + data-lanes = <1 2>;
> + link-frequencies = /bits/ 64 <400000000>;
> + remote-endpoint = <&csiphy4_ep>;
> + };
> + };
> + };
> + };
> +
> +...
>
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()
2025-09-02 11:10 ` [PATCH 2/2] leds: led-class: Add devicetree support to led_get() Aleksandrs Vinarskis
2025-09-02 12:25 ` Hans de Goede
@ 2025-09-02 22:22 ` Linus Walleij
2025-09-03 6:58 ` Lee Jones
2025-09-03 3:31 ` kernel test robot
2 siblings, 1 reply; 17+ messages in thread
From: Linus Walleij @ 2025-09-02 22:22 UTC (permalink / raw)
To: Aleksandrs Vinarskis
Cc: Hans de Goede, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bryan O'Donoghue,
linux-leds, devicetree, linux-kernel, Andy Shevchenko
On Tue, Sep 2, 2025 at 1:10 PM Aleksandrs Vinarskis <alex@vinarskis.com> wrote:
> From: Hans de Goede <hansg@kernel.org>
>
> Turn of_led_get() into a more generic __of_led_get() helper function,
> which can lookup LEDs in devicetree by either name or index.
I don't really like __inner_function() as naming since it
is easily confused with __COMPILER_FLAGS__ and also
with __non_atomic_bitset().
I would name it of_led_get_inner() simply.
Admitted it's nitpicky, feel free to ignore this remark, my
reviewed-by holds.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()
2025-09-02 11:10 ` [PATCH 2/2] leds: led-class: Add devicetree support to led_get() Aleksandrs Vinarskis
2025-09-02 12:25 ` Hans de Goede
2025-09-02 22:22 ` Linus Walleij
@ 2025-09-03 3:31 ` kernel test robot
2 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2025-09-03 3:31 UTC (permalink / raw)
To: Aleksandrs Vinarskis, Hans de Goede, Lee Jones, Pavel Machek,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bryan O'Donoghue
Cc: oe-kbuild-all, linux-leds, devicetree, linux-kernel,
Aleksandrs Vinarskis, Andy Shevchenko, Linus Walleij
Hi Aleksandrs,
kernel test robot noticed the following build warnings:
[auto build test WARNING on lee-leds/for-leds-next]
[also build test WARNING on linus/master v6.17-rc4 next-20250902]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Aleksandrs-Vinarskis/leds-led-class-Add-devicetree-support-to-led_get/20250902-191342
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git for-leds-next
patch link: https://lore.kernel.org/r/010201990a1f6559-9e836a40-f534-4535-bd59-5e967d80559a-000000%40eu-west-1.amazonses.com
patch subject: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20250903/202509031142.WBnVOXZW-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250903/202509031142.WBnVOXZW-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509031142.WBnVOXZW-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/leds/led-class.c:281:22: warning: no previous prototype for 'of_led_get' [-Wmissing-prototypes]
281 | struct led_classdev *of_led_get(struct device_node *np, int index)
| ^~~~~~~~~~
vim +/of_led_get +281 drivers/leds/led-class.c
272
273 /**
274 * of_led_get() - request a LED device via the LED framework
275 * @np: device node to get the LED device from
276 * @index: the index of the LED
277 *
278 * Returns the LED device parsed from the phandle specified in the "leds"
279 * property of a device tree node or a negative error-code on failure.
280 */
> 281 struct led_classdev *of_led_get(struct device_node *np, int index)
282 {
283 return __of_led_get(np, index, NULL);
284 }
285 EXPORT_SYMBOL_GPL(of_led_get);
286
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()
2025-09-02 22:22 ` Linus Walleij
@ 2025-09-03 6:58 ` Lee Jones
0 siblings, 0 replies; 17+ messages in thread
From: Lee Jones @ 2025-09-03 6:58 UTC (permalink / raw)
To: Linus Walleij
Cc: Aleksandrs Vinarskis, Hans de Goede, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bryan O'Donoghue,
linux-leds, devicetree, linux-kernel, Andy Shevchenko
On Wed, 03 Sep 2025, Linus Walleij wrote:
> On Tue, Sep 2, 2025 at 1:10 PM Aleksandrs Vinarskis <alex@vinarskis.com> wrote:
>
> > From: Hans de Goede <hansg@kernel.org>
> >
> > Turn of_led_get() into a more generic __of_led_get() helper function,
> > which can lookup LEDs in devicetree by either name or index.
>
> I don't really like __inner_function() as naming since it
> is easily confused with __COMPILER_FLAGS__ and also
> with __non_atomic_bitset().
>
> I would name it of_led_get_inner() simply.
I'm personally okay with '__' to mean private. There are many examples
of this already. I'm generally less happy with '__' functions being
exported or otherwise used outside of the subsystem / core that they
reside in.
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()
2025-09-02 12:25 ` Hans de Goede
@ 2025-09-03 23:01 ` Aleksandrs Vinarskis
2025-09-04 7:08 ` Hans de Goede
1 sibling, 0 replies; 17+ messages in thread
From: Aleksandrs Vinarskis @ 2025-09-03 23:01 UTC (permalink / raw)
To: hansg, Aleksandrs Vinarskis, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bryan O'Donoghue
Cc: andy.shevchenko, devicetree, linus.walleij, linux-kernel,
linux-leds
> Hi Aleksandrs,
>
> Thank you for working on this.
>
> On 2-Sep-25 1:10 PM, Aleksandrs Vinarskis wrote:
> > From: Hans de Goede <hansg@kernel.org>
> >
> > Turn of_led_get() into a more generic __of_led_get() helper function,
> > which can lookup LEDs in devicetree by either name or index.
> >
> > And use this new helper to add devicetree support to the generic
> > (non devicetree specific) [devm_]led_get() function.
> >
> > This uses the standard devicetree pattern of adding a -names string array
> > to map names to the indexes for an array of resources.
> >
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Reviewed-by: Lee Jones <lee@kernel.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Please update this to:
>
> Reviewed-by: Hans de Goede <hansg@kernel.org>
>
> to match the update of the author which you already did.
>
> Also note that checkpatch should complain about the mismatch,
> please ensure to run checkpatch before posting v2.
Hi,
ahh, I actually did not even see that email got changed, apologies. Seems
'b4' auto-corrected it when sending, which would explain why checkpatch
did not catch it, as I run it before importing and sending via 'b4'. Sure,
will fix - did you mean to change your signoff to R-by, or is it a mistake?
>
> > Tested-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> > ---
> > drivers/leds/led-class.c | 38 +++++++++++++++++++++++++++++---------
> > 1 file changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index 15633fbf3c166aa4f521774d245f6399a642bced..6f2ef4fa556b44ed3bf69dff556ae16fd2b7652b 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -248,19 +248,18 @@ static const struct class leds_class = {
> > .pm = &leds_class_dev_pm_ops,
> > };
> >
> > -/**
> > - * of_led_get() - request a LED device via the LED framework
> > - * @np: device node to get the LED device from
> > - * @index: the index of the LED
> > - *
> > - * Returns the LED device parsed from the phandle specified in the "leds"
> > - * property of a device tree node or a negative error-code on failure.
> > - */
> > -static struct led_classdev *of_led_get(struct device_node *np, int index)
> > +static struct led_classdev *__of_led_get(struct device_node *np, int index,
> > + const char *name)
> > {
> > struct device *led_dev;
> > struct device_node *led_node;
> >
> > + /*
> > + * For named LEDs, first look up the name in the "led-names" property.
> > + * If it cannot be found, then of_parse_phandle() will propagate the error.
> > + */
> > + if (name)
> > + index = of_property_match_string(np, "led-names", name);
> > led_node = of_parse_phandle(np, "leds", index);
> > if (!led_node)
> > return ERR_PTR(-ENOENT);
> > @@ -271,6 +270,20 @@ static struct led_classdev *of_led_get(struct device_node *np, int index)
> > return led_module_get(led_dev);
> > }
> >
> > +/**
> > + * of_led_get() - request a LED device via the LED framework
> > + * @np: device node to get the LED device from
> > + * @index: the index of the LED
> > + *
> > + * Returns the LED device parsed from the phandle specified in the "leds"
> > + * property of a device tree node or a negative error-code on failure.
> > + */
> > +struct led_classdev *of_led_get(struct device_node *np, int index)
> > +{
> > + return __of_led_get(np, index, NULL);
> > +}
> > +EXPORT_SYMBOL_GPL(of_led_get);
>
> I probably did this myself, but since of_led_get() is private now
> (I guess it was not private before?) and since we are moving away from
> "of" specific functions to using generic dev_xxxx functions in the kernel
> in general, I think this just should be a static helper.
>
> Or probably even better: just add the name argument to of_led_get()
> before without renaming it, update the existing callers to pass
> an extra NULL arg and completely drop this wrapper.
>
That indeed sounds like a better and cleaner option, will change it.
This way also incorporates the rest of the feedback on this series.
> Also notice that adding the EXPORT_SYMBOL_GPL() while there was
> none before should go hand in hand with adding a prototype to
> a relevant .h file. However please just keep this private and
> drop the wrapper.
Thanks for the clarification, missed that, and the review.
Alex
>
> Regards,
>
> Hans
>
>
>
>
> > +
> > /**
> > * led_put() - release a LED device
> > * @led_cdev: LED device
> > @@ -342,9 +355,16 @@ EXPORT_SYMBOL_GPL(devm_of_led_get);
> > struct led_classdev *led_get(struct device *dev, char *con_id)
> > {
> > struct led_lookup_data *lookup;
> > + struct led_classdev *led_cdev;
> > const char *provider = NULL;
> > struct device *led_dev;
> >
> > + if (dev->of_node) {
> > + led_cdev = __of_led_get(dev->of_node, -1, con_id);
> > + if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
> > + return led_cdev;
> > + }
> > +
> > mutex_lock(&leds_lookup_lock);
> > list_for_each_entry(lookup, &leds_lookup_list, list) {
> > if (!strcmp(lookup->dev_id, dev_name(dev)) &&
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation
2025-09-02 18:21 ` Rob Herring
@ 2025-09-03 23:56 ` Aleksandrs Vinarskis
2025-09-04 6:41 ` Krzysztof Kozlowski
0 siblings, 1 reply; 17+ messages in thread
From: Aleksandrs Vinarskis @ 2025-09-03 23:56 UTC (permalink / raw)
To: robh, Aleksandrs Vinarskis
Cc: bryan.odonoghue, conor+dt, devicetree, hansg, krzk+dt, lee,
linux-kernel, linux-leds, pavel
> On Tue, Sep 02, 2025 at 11:10:51AM +0000, Aleksandrs Vinarskis wrote:
> > Currently supports passing 'led-names' used to map LED devices to their
> > respective functions.
> >
> > Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> > ---
> > .../devicetree/bindings/leds/leds-consumer.yaml | 69 ++++++++++++++++++++++
> > 1 file changed, 69 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-consumer.yaml b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..a63e78417df84609e279835f7dae62e3ad2f0bf5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/leds-consumer.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Common leds consumer
> > +
> > +maintainers:
> > + - Aleksandrs Vinarskis <alex@vinarskis.com>
> > +
> > +description:
> > + Some LED defined in DT are required by other DT consumers, for example
> > + v4l2 subnode may require privacy or flash LED.
> > +
> > + Document LED properties that its consumers may define.
>
> We already have the trigger-source binding for "attaching" LEDs to
> devices. Why does that not work here?
I have not actually considered this, as the existing privacy-led solution
from the original series is not trigger based. At least one of the reasons
for that is that trigger source can be rather easily altered from user
space, which would've been bad for this use case. If v4l2 acquires control
over the LED it actually removes triggers and disables sysfs on that LED.
Regarding DT check that is failing because 'ovti,ov02e10.yaml' does not
allow for additional properties - the same issue would apply to basically
any camera, I missed it. So would need to either include this new binding
in 'video-interface-devices.yaml', or drop new binding and directly include
these new generic LED related properties in the video one. However, in this
case it gets a bit ugly, as the latter already contains 'flash-leds' for
flash specifically, and we would be adding a more generic way only used for
privacy LED, at least for now... not too sure whats the best way here,
leaning towards 1st option.
Let me know what you think,
Alex
>
> Rob
>
> > +
> > +properties:
> > + leds:
> > + description:
> > + Phandle to LED device(s) required by particular consumer.
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + led-names:
> > + description:
> > + List of device name(s). Used to map LED devices to their respective
> > + functions, when consumer requires more than one LED.
> > +
> > +additionalProperties: true
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > + #include <dt-bindings/leds/common.h>
> > +
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + camera@36 {
> > + compatible = "ovti,ov02c10";
> > + reg = <0x36>;
> > +
> > + reset-gpios = <&tlmm 237 GPIO_ACTIVE_LOW>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&cam_rgb_default>;
> > +
> > + led-names = "privacy-led";
> > + leds = <&privacy_led>;
> > +
> > + clocks = <&ov02e10_clk>;
> > +
> > + assigned-clocks = <&ov02e10_clk>;
> > + assigned-clock-rates = <19200000>;
> > +
> > + avdd-supply = <&vreg_l7b_2p8>;
> > + dvdd-supply = <&vreg_l7b_2p8>;
> > + dovdd-supply = <&vreg_cam_1p8>;
> > +
> > + port {
> > + ov02e10_ep: endpoint {
> > + data-lanes = <1 2>;
> > + link-frequencies = /bits/ 64 <400000000>;
> > + remote-endpoint = <&csiphy4_ep>;
> > + };
> > + };
> > + };
> > + };
> > +
> > +...
> >
> > --
> > 2.48.1
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation
2025-09-03 23:56 ` Aleksandrs Vinarskis
@ 2025-09-04 6:41 ` Krzysztof Kozlowski
2025-09-04 7:26 ` Hans de Goede
0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2025-09-04 6:41 UTC (permalink / raw)
To: Aleksandrs Vinarskis
Cc: robh, bryan.odonoghue, conor+dt, devicetree, hansg, krzk+dt, lee,
linux-kernel, linux-leds, pavel
On Thu, Sep 04, 2025 at 01:56:15AM +0200, Aleksandrs Vinarskis wrote:
> > On Tue, Sep 02, 2025 at 11:10:51AM +0000, Aleksandrs Vinarskis wrote:
> > > Currently supports passing 'led-names' used to map LED devices to their
> > > respective functions.
> > >
> > > Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
> > > ---
> > > .../devicetree/bindings/leds/leds-consumer.yaml | 69 ++++++++++++++++++++++
> > > 1 file changed, 69 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/leds/leds-consumer.yaml b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..a63e78417df84609e279835f7dae62e3ad2f0bf5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
> > > @@ -0,0 +1,69 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/leds/leds-consumer.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Common leds consumer
> > > +
> > > +maintainers:
> > > + - Aleksandrs Vinarskis <alex@vinarskis.com>
> > > +
> > > +description:
> > > + Some LED defined in DT are required by other DT consumers, for example
> > > + v4l2 subnode may require privacy or flash LED.
> > > +
> > > + Document LED properties that its consumers may define.
> >
> > We already have the trigger-source binding for "attaching" LEDs to
> > devices. Why does that not work here?
>
> I have not actually considered this, as the existing privacy-led solution
> from the original series is not trigger based. At least one of the reasons
> for that is that trigger source can be rather easily altered from user
> space, which would've been bad for this use case. If v4l2 acquires control
> over the LED it actually removes triggers and disables sysfs on that LED.
So does that mean that v4l2 solves the problem of "trigger source can be
rather easily altered from user space"?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] leds: led-class: Add devicetree support to led_get()
2025-09-02 12:25 ` Hans de Goede
2025-09-03 23:01 ` Aleksandrs Vinarskis
@ 2025-09-04 7:08 ` Hans de Goede
1 sibling, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2025-09-04 7:08 UTC (permalink / raw)
To: Aleksandrs Vinarskis, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bryan O'Donoghue
Cc: andy.shevchenko, devicetree, linus.walleij, linux-kernel,
linux-leds
Hi,
On 4-Sep-25 1:01 AM, Aleksandrs Vinarskis wrote:
>> Hi Aleksandrs,
>>
>> Thank you for working on this.
>>
>> On 2-Sep-25 1:10 PM, Aleksandrs Vinarskis wrote:
>>> From: Hans de Goede <hansg@kernel.org>
>>>
>>> Turn of_led_get() into a more generic __of_led_get() helper function,
>>> which can lookup LEDs in devicetree by either name or index.
>>>
>>> And use this new helper to add devicetree support to the generic
>>> (non devicetree specific) [devm_]led_get() function.
>>>
>>> This uses the standard devicetree pattern of adding a -names string array
>>> to map names to the indexes for an array of resources.
>>>
>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> Reviewed-by: Lee Jones <lee@kernel.org>
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Please update this to:
>>
>> Reviewed-by: Hans de Goede <hansg@kernel.org>
>>
>> to match the update of the author which you already did.
>>
>> Also note that checkpatch should complain about the mismatch,
>> please ensure to run checkpatch before posting v2.
>
> Hi,
>
> ahh, I actually did not even see that email got changed, apologies. Seems
> 'b4' auto-corrected it when sending,
I already wondered if it might be something like that. b4 probably did
this because of the .mailmap entry mapping my Red Hat address (which
I've stopped using since I'm leaving Red Hat) to hansg@kernel.org .
> which would explain why checkpatch
> did not catch it, as I run it before importing and sending via 'b4'. Sure,
> will fix - did you mean to change your signoff to R-by, or is it a mistake?
It is a mistake please keep it as S-o-b.
>
>>
>>> Tested-by: Aleksandrs Vinarskis <alex@vinarskis.com>
>>> ---
>>> drivers/leds/led-class.c | 38 +++++++++++++++++++++++++++++---------
>>> 1 file changed, 29 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index 15633fbf3c166aa4f521774d245f6399a642bced..6f2ef4fa556b44ed3bf69dff556ae16fd2b7652b 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -248,19 +248,18 @@ static const struct class leds_class = {
>>> .pm = &leds_class_dev_pm_ops,
>>> };
>>>
>>> -/**
>>> - * of_led_get() - request a LED device via the LED framework
>>> - * @np: device node to get the LED device from
>>> - * @index: the index of the LED
>>> - *
>>> - * Returns the LED device parsed from the phandle specified in the "leds"
>>> - * property of a device tree node or a negative error-code on failure.
>>> - */
>>> -static struct led_classdev *of_led_get(struct device_node *np, int index)
>>> +static struct led_classdev *__of_led_get(struct device_node *np, int index,
>>> + const char *name)
>>> {
>>> struct device *led_dev;
>>> struct device_node *led_node;
>>>
>>> + /*
>>> + * For named LEDs, first look up the name in the "led-names" property.
>>> + * If it cannot be found, then of_parse_phandle() will propagate the error.
>>> + */
>>> + if (name)
>>> + index = of_property_match_string(np, "led-names", name);
>>> led_node = of_parse_phandle(np, "leds", index);
>>> if (!led_node)
>>> return ERR_PTR(-ENOENT);
>>> @@ -271,6 +270,20 @@ static struct led_classdev *of_led_get(struct device_node *np, int index)
>>> return led_module_get(led_dev);
>>> }
>>>
>>> +/**
>>> + * of_led_get() - request a LED device via the LED framework
>>> + * @np: device node to get the LED device from
>>> + * @index: the index of the LED
>>> + *
>>> + * Returns the LED device parsed from the phandle specified in the "leds"
>>> + * property of a device tree node or a negative error-code on failure.
>>> + */
>>> +struct led_classdev *of_led_get(struct device_node *np, int index)
>>> +{
>>> + return __of_led_get(np, index, NULL);
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_led_get);
>>
>> I probably did this myself, but since of_led_get() is private now
>> (I guess it was not private before?) and since we are moving away from
>> "of" specific functions to using generic dev_xxxx functions in the kernel
>> in general, I think this just should be a static helper.
>>
>> Or probably even better: just add the name argument to of_led_get()
>> before without renaming it, update the existing callers to pass
>> an extra NULL arg and completely drop this wrapper.
>>
>
> That indeed sounds like a better and cleaner option, will change it.
> This way also incorporates the rest of the feedback on this series.
Sounds good.
Regards,
Hans
>>> +
>>> /**
>>> * led_put() - release a LED device
>>> * @led_cdev: LED device
>>> @@ -342,9 +355,16 @@ EXPORT_SYMBOL_GPL(devm_of_led_get);
>>> struct led_classdev *led_get(struct device *dev, char *con_id)
>>> {
>>> struct led_lookup_data *lookup;
>>> + struct led_classdev *led_cdev;
>>> const char *provider = NULL;
>>> struct device *led_dev;
>>>
>>> + if (dev->of_node) {
>>> + led_cdev = __of_led_get(dev->of_node, -1, con_id);
>>> + if (!IS_ERR(led_cdev) || PTR_ERR(led_cdev) != -ENOENT)
>>> + return led_cdev;
>>> + }
>>> +
>>> mutex_lock(&leds_lookup_lock);
>>> list_for_each_entry(lookup, &leds_lookup_list, list) {
>>> if (!strcmp(lookup->dev_id, dev_name(dev)) &&
>>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation
2025-09-04 6:41 ` Krzysztof Kozlowski
@ 2025-09-04 7:26 ` Hans de Goede
2025-09-04 9:45 ` Krzysztof Kozlowski
0 siblings, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2025-09-04 7:26 UTC (permalink / raw)
To: Krzysztof Kozlowski, Aleksandrs Vinarskis
Cc: robh, bryan.odonoghue, conor+dt, devicetree, krzk+dt, lee,
linux-kernel, linux-leds, pavel
Hi,
On 4-Sep-25 8:41 AM, Krzysztof Kozlowski wrote:
> On Thu, Sep 04, 2025 at 01:56:15AM +0200, Aleksandrs Vinarskis wrote:
>>> On Tue, Sep 02, 2025 at 11:10:51AM +0000, Aleksandrs Vinarskis wrote:
>>>> Currently supports passing 'led-names' used to map LED devices to their
>>>> respective functions.
>>>>
>>>> Signed-off-by: Aleksandrs Vinarskis <alex@vinarskis.com>
>>>> ---
>>>> .../devicetree/bindings/leds/leds-consumer.yaml | 69 ++++++++++++++++++++++
>>>> 1 file changed, 69 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-consumer.yaml b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
>>>> new file mode 100644
>>>> index 0000000000000000000000000000000000000000..a63e78417df84609e279835f7dae62e3ad2f0bf5
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-consumer.yaml
>>>> @@ -0,0 +1,69 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/leds/leds-consumer.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Common leds consumer
>>>> +
>>>> +maintainers:
>>>> + - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>> +
>>>> +description:
>>>> + Some LED defined in DT are required by other DT consumers, for example
>>>> + v4l2 subnode may require privacy or flash LED.
>>>> +
>>>> + Document LED properties that its consumers may define.
>>>
>>> We already have the trigger-source binding for "attaching" LEDs to
>>> devices. Why does that not work here?
>>
>> I have not actually considered this, as the existing privacy-led solution
>> from the original series is not trigger based. At least one of the reasons
>> for that is that trigger source can be rather easily altered from user
>> space, which would've been bad for this use case. If v4l2 acquires control
>> over the LED it actually removes triggers and disables sysfs on that LED.
>
> So does that mean that v4l2 solves the problem of "trigger source can be
> rather easily altered from user space"?
Yes, currently the v4l2-core already does:
sd->privacy_led = led_get(sd->dev, "privacy-led")
led_sysfs_disable(sd->privacy_led);
Which disallows changing the LED state or trigger from
userspace. This is similar to how flash-LEDs are handled
which also involves directly controlling the LED rather
then using triggers and which also calls led_sysfs_disable().
led_get() already works for this on x86 and is already used
for this there which is why this code is already there.
I guess the difference with triggers is that triggers are
more of a soft binding between LED and controller of
the LED and where here we need more of a hard binding.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation
2025-09-04 7:26 ` Hans de Goede
@ 2025-09-04 9:45 ` Krzysztof Kozlowski
2025-09-04 10:29 ` Hans de Goede
0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2025-09-04 9:45 UTC (permalink / raw)
To: Hans de Goede, Aleksandrs Vinarskis
Cc: robh, bryan.odonoghue, conor+dt, devicetree, krzk+dt, lee,
linux-kernel, linux-leds, pavel
On 04/09/2025 09:26, Hans de Goede wrote:
>>>>> +maintainers:
>>>>> + - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>>> +
>>>>> +description:
>>>>> + Some LED defined in DT are required by other DT consumers, for example
>>>>> + v4l2 subnode may require privacy or flash LED.
>>>>> +
>>>>> + Document LED properties that its consumers may define.
>>>>
>>>> We already have the trigger-source binding for "attaching" LEDs to
>>>> devices. Why does that not work here?
>>>
>>> I have not actually considered this, as the existing privacy-led solution
>>> from the original series is not trigger based. At least one of the reasons
>>> for that is that trigger source can be rather easily altered from user
>>> space, which would've been bad for this use case. If v4l2 acquires control
>>> over the LED it actually removes triggers and disables sysfs on that LED.
>>
>> So does that mean that v4l2 solves the problem of "trigger source can be
>> rather easily altered from user space"?
>
> Yes, currently the v4l2-core already does:
Thanks, I understand that it solves the problem described in the patch,
so the patch can be dropped.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation
2025-09-04 9:45 ` Krzysztof Kozlowski
@ 2025-09-04 10:29 ` Hans de Goede
2025-09-04 10:47 ` Krzysztof Kozlowski
0 siblings, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2025-09-04 10:29 UTC (permalink / raw)
To: Krzysztof Kozlowski, Aleksandrs Vinarskis
Cc: robh, bryan.odonoghue, conor+dt, devicetree, krzk+dt, lee,
linux-kernel, linux-leds, pavel
Hi Krzysztof,
On 4-Sep-25 11:45 AM, Krzysztof Kozlowski wrote:
> On 04/09/2025 09:26, Hans de Goede wrote:
>>>>>> +maintainers:
>>>>>> + - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>>>> +
>>>>>> +description:
>>>>>> + Some LED defined in DT are required by other DT consumers, for example
>>>>>> + v4l2 subnode may require privacy or flash LED.
>>>>>> +
>>>>>> + Document LED properties that its consumers may define.
>>>>>
>>>>> We already have the trigger-source binding for "attaching" LEDs to
>>>>> devices. Why does that not work here?
>>>>
>>>> I have not actually considered this, as the existing privacy-led solution
>>>> from the original series is not trigger based. At least one of the reasons
>>>> for that is that trigger source can be rather easily altered from user
>>>> space, which would've been bad for this use case. If v4l2 acquires control
>>>> over the LED it actually removes triggers and disables sysfs on that LED.
>>>
>>> So does that mean that v4l2 solves the problem of "trigger source can be
>>> rather easily altered from user space"?
>>
>> Yes, currently the v4l2-core already does:
>
> Thanks, I understand that it solves the problem described in the patch,
> so the patch can be dropped.
I'm a bit confused now, do you mean that this dt-bindings patch can
be dropped ?
The existing v4l2-core code solves getting the privacy-LED on ACPI/x86_64,
on DT there is no official bindings-docs for directly getting a LED with
led_get() AFAICT and I believe that having a binding is mandatory before
we just start adding leds and led-names properties to DT nodes for sensors ?
Maybe for v2 of this patch-set Aleksanders should also add a patch
actually using the new binding in a dts file to make clear that
the intent is to also start using privacy-LEDs in the same way
on DT systems ?
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation
2025-09-04 10:29 ` Hans de Goede
@ 2025-09-04 10:47 ` Krzysztof Kozlowski
2025-09-04 11:47 ` Hans de Goede
0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2025-09-04 10:47 UTC (permalink / raw)
To: Hans de Goede, Aleksandrs Vinarskis
Cc: robh, bryan.odonoghue, conor+dt, devicetree, krzk+dt, lee,
linux-kernel, linux-leds, pavel
On 04/09/2025 12:29, Hans de Goede wrote:
> Hi Krzysztof,
>
> On 4-Sep-25 11:45 AM, Krzysztof Kozlowski wrote:
>> On 04/09/2025 09:26, Hans de Goede wrote:
>>>>>>> +maintainers:
>>>>>>> + - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>>>>> +
>>>>>>> +description:
>>>>>>> + Some LED defined in DT are required by other DT consumers, for example
>>>>>>> + v4l2 subnode may require privacy or flash LED.
>>>>>>> +
>>>>>>> + Document LED properties that its consumers may define.
>>>>>>
>>>>>> We already have the trigger-source binding for "attaching" LEDs to
>>>>>> devices. Why does that not work here?
>>>>>
>>>>> I have not actually considered this, as the existing privacy-led solution
>>>>> from the original series is not trigger based. At least one of the reasons
>>>>> for that is that trigger source can be rather easily altered from user
>>>>> space, which would've been bad for this use case. If v4l2 acquires control
>>>>> over the LED it actually removes triggers and disables sysfs on that LED.
>>>>
>>>> So does that mean that v4l2 solves the problem of "trigger source can be
>>>> rather easily altered from user space"?
>>>
>>> Yes, currently the v4l2-core already does:
>>
>> Thanks, I understand that it solves the problem described in the patch,
>> so the patch can be dropped.
>
> I'm a bit confused now, do you mean that this dt-bindings patch can
> be dropped ?
Yes.
Alex's explanation to Rob felt confusing, so I asked for clarification.
You clarfiied that that v4l2 solves the problem, therefore there is no
problem to be solved.
If there is no problem to be solved, this patch is not needed.
If this patch is needed, just describe the problem accurately.
>
> The existing v4l2-core code solves getting the privacy-LED on ACPI/x86_64,
> on DT there is no official bindings-docs for directly getting a LED with
There are and Rob pointed to them. If Rob's answer is not enough, make
it explicit.
Really, there are here some long explanations which do not really
explain this in simple terms. Simple term is: "existing property foo
does not work because <here goes the reason>".
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation
2025-09-04 10:47 ` Krzysztof Kozlowski
@ 2025-09-04 11:47 ` Hans de Goede
0 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2025-09-04 11:47 UTC (permalink / raw)
To: Krzysztof Kozlowski, Aleksandrs Vinarskis
Cc: robh, bryan.odonoghue, conor+dt, devicetree, krzk+dt, lee,
linux-kernel, linux-leds, pavel
Hi Krzysztof,
On 4-Sep-25 12:47 PM, Krzysztof Kozlowski wrote:
> On 04/09/2025 12:29, Hans de Goede wrote:
>> Hi Krzysztof,
>>
>> On 4-Sep-25 11:45 AM, Krzysztof Kozlowski wrote:
>>> On 04/09/2025 09:26, Hans de Goede wrote:
>>>>>>>> +maintainers:
>>>>>>>> + - Aleksandrs Vinarskis <alex@vinarskis.com>
>>>>>>>> +
>>>>>>>> +description:
>>>>>>>> + Some LED defined in DT are required by other DT consumers, for example
>>>>>>>> + v4l2 subnode may require privacy or flash LED.
>>>>>>>> +
>>>>>>>> + Document LED properties that its consumers may define.
>>>>>>>
>>>>>>> We already have the trigger-source binding for "attaching" LEDs to
>>>>>>> devices. Why does that not work here?
>>>>>>
>>>>>> I have not actually considered this, as the existing privacy-led solution
>>>>>> from the original series is not trigger based. At least one of the reasons
>>>>>> for that is that trigger source can be rather easily altered from user
>>>>>> space, which would've been bad for this use case. If v4l2 acquires control
>>>>>> over the LED it actually removes triggers and disables sysfs on that LED.
>>>>>
>>>>> So does that mean that v4l2 solves the problem of "trigger source can be
>>>>> rather easily altered from user space"?
>>>>
>>>> Yes, currently the v4l2-core already does:
>>>
>>> Thanks, I understand that it solves the problem described in the patch,
>>> so the patch can be dropped.
>>
>> I'm a bit confused now, do you mean that this dt-bindings patch can
>> be dropped ?
>
> Yes.
>
> Alex's explanation to Rob felt confusing, so I asked for clarification.
> You clarfiied that that v4l2 solves the problem, therefore there is no
> problem to be solved.
>
> If there is no problem to be solved, this patch is not needed.
>
> If this patch is needed, just describe the problem accurately.
>
>>
>> The existing v4l2-core code solves getting the privacy-LED on ACPI/x86_64,
>> on DT there is no official bindings-docs for directly getting a LED with
>
> There are and Rob pointed to them. If Rob's answer is not enough, make
> it explicit.
>
> Really, there are here some long explanations which do not really
> explain this in simple terms. Simple term is: "existing property foo
> does not work because <here goes the reason>".
The existing trigger-source binding for "attaching" LEDs to
devices does not work because:
1. It depends on the Linux specific LED trigger mechanism where as
DT should describe hw in an OS agnostic manner
2. It puts the world upside down by giving possible event-sources
for the (again) Linux specific trigger rather then allowing
specifying e.g. specific privacy and flash LEDs as part
of a camera dts node. IOW it makes the LED DT note point to
the camera, while the LED is a part of the camera-module.
not the other way around. So it does not properly allow
describing the composition of the camera.
Note that Rob actually put "" around attaching because this
property really is not proper attaching / composition as
we would normally do in dt.
IMHO 1. alone (this being Linux specific) warrants a new better
binding for this.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2025-09-04 11:47 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250902-leds-v1-0-4a31e125276b@vinarskis.com>
2025-09-02 11:10 ` [PATCH 1/2] dt-bindings: leds: add generic LED consumer documentation Aleksandrs Vinarskis
2025-09-02 17:57 ` Rob Herring (Arm)
2025-09-02 18:21 ` Rob Herring
2025-09-03 23:56 ` Aleksandrs Vinarskis
2025-09-04 6:41 ` Krzysztof Kozlowski
2025-09-04 7:26 ` Hans de Goede
2025-09-04 9:45 ` Krzysztof Kozlowski
2025-09-04 10:29 ` Hans de Goede
2025-09-04 10:47 ` Krzysztof Kozlowski
2025-09-04 11:47 ` Hans de Goede
2025-09-02 11:10 ` [PATCH 2/2] leds: led-class: Add devicetree support to led_get() Aleksandrs Vinarskis
2025-09-02 12:25 ` Hans de Goede
2025-09-03 23:01 ` Aleksandrs Vinarskis
2025-09-04 7:08 ` Hans de Goede
2025-09-02 22:22 ` Linus Walleij
2025-09-03 6:58 ` Lee Jones
2025-09-03 3:31 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).