drivers: led: add led_dac #88259

jeppenodgaard · 2025-04-07T13:38:31Z

Add LED driver support for DAC based LED drivers.

bjarki-andreasen

Using a DAC to drive an LED, not a usual thing to do given how "expensive" those are compared to a PWM :D I'm all for it but we can do better given the "power" we get from using DAC (full control over voltage range, no need for external components, other than the LED of course)

bjarki-andreasen · 2025-04-07T15:10:35Z

drivers/led/led_dac.h

+
+#include <zephyr/device.h>
+
+int led_dac_set_raw(const struct device *dev, uint32_t led, uint32_t value);


The header is not required, the function is only used internally anyway :) if this was to be public it would need to go to include/zephyr/drivers/led, but I don't think this is necessary :)

You are right. It an attempt as a work around until #87737 is resolved/gets response.

Header removed and function made static.

bjarki-andreasen · 2025-04-07T15:11:50Z

dts/bindings/led/dac-leds.yaml

+  / {
+    leds {
+      compatible = "dac-leds";
+      led_0 {
+        dac = <&dac1>;
+        channel = <0>;
+        resolution = <12>;
+      };
+    };
+  };


I would use dts indentation, 8 spaces, not a blocker but I think its neater. Two spaces before / { then 8 spaces

It is more or less copy paste from gpio-leds.yaml so I did not really consider the indentation. leds actually needs to be indented that should be fixed.

Is it allowed to use tabs in comments (I think copy pasting example into a device tree source file looks as if indentation is ok, but it isn't since it is 8 spaces instead of tabs)?

Its not indented to be copied directly to the devicetree, just to look like a devicetree snippet :)

I revisited gpio-leds.yaml and it actually uses tabs. My editor converted them to spaces. I have changed them back to tabs.

Never mind. check_compliance.py wont allow tabs after space. I changed indent to 8 spaces.

bjarki-andreasen · 2025-04-07T15:23:32Z

dts/bindings/led/dac-leds.yaml

+
+child-binding:
+  description: DAC LED child node
+  properties:


I would add a max value here at minimum, possibly even a max and forward current. If we are using a DAC to drive an LED, we should do it right!

So add something like

max-voltage-mv: type: int description: max voltage in millivolt forward-voltage-mv: type: int description: led forward voltage in millivolt

This way, the brightness percentage could be converted to a relative intensity quite nicely!

My intention was to use this with an opamp circuit. E.g. using TI's OPA310 example.
This is not clear at all and should be clarified.

I like your suggestion. Maybe it could be added on top of this driver and bindings so both is possible?

ok, that is a bit of a different story, usually LEDs are tied directly to the pin, with no voltage controlled current source circuit in between. Maybe add a boolean property named linear or current-controlled and voltage-controlled? so the driver knows what "type" of led driving circuit (if any) is used?

The reason this is not the case for PWM controlled LEDs BTW is that they are "fully on" / "fully off", so the voltage/current is not varied to control intensity :)

How about renaming dac-leds.yaml to dac-leds-common.yaml to prevent optional unused properties?
Then include that in dac-leds.yaml which has the two properties you suggest to make it possible to convert pct to a relative intensity.
And add another dac-leds-vccs which just includes common and nothing else.

It might be a good idea to have a current limiting resistor in series with the LED and that should probably also be part of the properties if the LED is directly tied and be part of the conversion.

sounds good :)

I reworked the driver.

jeppenodgaard

Thanks you for the very quick review @bjarki-andreasen ! I am not sure how to change the code so I've replied to your comments.

jeppenodgaard · 2025-04-08T07:12:19Z

drivers/led/led_dac.h

+
+#include <zephyr/device.h>
+
+int led_dac_set_raw(const struct device *dev, uint32_t led, uint32_t value);


You are right. It an attempt as a work around until #87737 is resolved/gets response.

jeppenodgaard · 2025-04-08T07:20:37Z

dts/bindings/led/dac-leds.yaml

+
+child-binding:
+  description: DAC LED child node
+  properties:


My intention was to use this with an opamp circuit. E.g. using TI's OPA310 example.
This is not clear at all and should be clarified.

I like your suggestion. Maybe it could be added on top of this driver and bindings so both is possible?

jeppenodgaard · 2025-04-08T07:44:34Z

dts/bindings/led/dac-leds.yaml

+  / {
+    leds {
+      compatible = "dac-leds";
+      led_0 {
+        dac = <&dac1>;
+        channel = <0>;
+        resolution = <12>;
+      };
+    };
+  };


It is more or less copy paste from gpio-leds.yaml so I did not really consider the indentation. leds actually needs to be indented that should be fixed.

Is it allowed to use tabs in comments (I think copy pasting example into a device tree source file looks as if indentation is ok, but it isn't since it is 8 spaces instead of tabs)?

simonguinot · 2025-04-12T12:31:56Z

Hi @martinjaeger. I am not familiar with DAC. Please, let me know what you think of this LED DAC driver.

Copilot

Copilot reviewed 4 out of 8 changed files in this pull request and generated no comments.

Files not reviewed (4)

drivers/led/CMakeLists.txt: Language not supported
drivers/led/Kconfig: Language not supported
drivers/led/Kconfig.dac: Language not supported
tests/drivers/build_all/led/app.overlay: Language not supported

Comments suppressed due to low confidence (1)

drivers/led/led_dac.c:130

[nitpick] Consider adding a comment clarifying why LED_DAC_MIN is redefined to 0 for dac_leds_vccs configurations to help readers understand the different behavior between dac_leds and dac_leds_vccs.

#define LED_DAC_MIN(n) 0

simonguinot

Hi @jeppenodgaard,

Thanks for this driver. I apologize for the delayed review.

Please find my comments and questions below.

simonguinot · 2025-04-12T12:51:37Z

drivers/led/Kconfig.dac

+	depends on DT_HAS_DAC_LEDS_ENABLED
+	select DAC
+	help
+	  Enable DAC LED driver.


Please, elaborate the Kconfig help description a bit (and the commit message as well).

simonguinot · 2025-04-12T12:53:24Z

drivers/led/Kconfig.dac

+config LED_DAC
+	bool "DAC LED driver"
+	default y
+	depends on DT_HAS_DAC_LEDS_ENABLED


Probably you also want to depend on DT_HAS_DAC_LEDS_VCCS_ENABLED here.

simonguinot · 2025-04-12T12:56:19Z

drivers/led/led_dac.c

+		return -EINVAL;
+	}
+
+	return led_dac_set_raw(dev, led, 0);


Shouldn't you use dac_min instead of 0 here ?

I think it is better to use 0 to turn off the LED off completely.

simonguinot · 2025-04-12T13:14:28Z

drivers/led/led_dac.c

+#undef DT_DRV_COMPAT
+#define DT_DRV_COMPAT dac_leds_vccs
+#undef LED_DAC_MIN
+#define LED_DAC_MIN(n) 0


Honestly, I don't like the idea of having two distinct bindings for LED DAC devices. The code above is confusing and quite messy.

Here we are just computing min and max values. What about letting the user do that with some dedicated DT properties ? With maybe an help message in the binding file explaining how to compute them ?

rruuaanng · 2025-04-13T03:33:39Z

dts/bindings/led/dac-leds.yaml

+# SPDX-License-Identifier: Apache-2.0
+
+description: |
+  Group of DAC-controlled LEDs.


The title keyword has been merged into the main branch, so perhaps this can be moved to the title: Group of DAC-controlled LEDs.

rruuaanng · 2025-04-13T03:35:06Z

dts/bindings/led/dac-leds-vccs.yaml

+# SPDX-License-Identifier: Apache-2.0
+
+description: |
+  Group of DAC-controlled LEDs using voltage control current source (vccs) circuit.


rruuaanng · 2025-04-13T03:37:13Z

dts/bindings/led/dac-leds-common.yaml

+child-binding:
+  description: Common DAC LED child node
+  properties:
+    dac:


Suggested change

dac:

dac-dev:

I will choose this property name.

martinjaeger

I don't think it's a good idea to drive an LED directly with a DAC. A DAC is a voltage output, whereas an LED is current-controlled. Many DACs have a relatively high impedance output (a few kOhms), so it shouldn't even be possible to drive an LED directly. If the DAC has a buffered output, it might be possible, but I still think that controlling the brightness via PWM is a better solution.

The approach with voltage-controlled current-source makes sense, so this should be the default.

What about just using one generic dac-leds.yaml compatible with the following optional properties (feel free to be a bit more elaborate in the descriptions, I've just drafted something to make the idea clear):

voltage-off-mv:
  type: int
  default: 0
  description: |
    Output voltage to turn off the LED
voltage-min-brightness-mv:
  type: int
  default: 0
  description: |
    Voltage at brightness 0%.
voltage-max-brightness-mv:
  type: int
  description: |
    Voltage at brightness 100%. 
    If not specified, the maximum DAC output voltage is used.
output-buffer:
  type: boolean
  description: |
    Enable the output buffer of the DAC. This is required if it is used to drive an LED directly.

I would suggest to describe that the intention of this driver is to have a VCCS. However, if someone has carefully checked the DAC hardware capabilities and wants to use it to drive an LED directly, it's still possible with above properties.

martinjaeger · 2025-04-14T19:00:49Z

dts/bindings/led/dac-leds.yaml

+    forward-voltage-mv:
+      type: int
+      required: true
+      description: |
+        LED forward voltage in millivolt.


An LED's forward voltage is not constant, but varies with current, as shown in the images posted by @bjarki-andreasen. If we decide to specify the LED like this, we'll at least need a more clear description.

martinjaeger · 2025-04-14T19:02:06Z

dts/bindings/led/dac-leds.yaml

+    resistor-forward-voltage-mv:
+      type: int
+      required: true
+      description: |
+        Current limiting resistor voltage in millivolt at LED forward voltage.
+        LED forward current [mA] * resistor resistance [Ohms]


Why not just specify the resistor value instead of the forward voltage, which is depending on the current?

martinjaeger · 2025-04-14T19:18:14Z

drivers/led/led_dac.c

+		.dac = DEVICE_DT_GET(DT_PHANDLE(n, dac)),                                          \
+		.chan_cfg = {.channel_id = DT_PROP(n, channel),                                    \
+			     .resolution = DT_PROP(n, resolution),                                 \
+			     .buffered = false,                                                    \


It should be made sure that the output buffer is enabled if the DAC is used to drive the LED directly.

jeppenodgaard · 2025-04-23T07:18:34Z

Thanks you for all your reviews! Greatly appreciated!

@martinjaeger your comment makes a lot of sense.
I like your bindings suggestion. It is a lot more understandable.

The only downside I can see is that the LED intensity is not changed linearly.
Maybe this can be a future improvement if anyone will use the DAC directly.

I will start updating the PR.

jeppenodgaard

@martinjaeger The PR has been updated.
I did not add voltage-off-mv as I cannot see why it is needed. Please enlighten me if I am wrong 😄

jeppenodgaard · 2025-04-24T05:30:52Z

drivers/led/led_dac.c

+		return -EINVAL;
+	}
+
+	return led_dac_set_raw(dev, led, 0);


I think it is better to use 0 to turn off the LED off completely.

Add LED driver support for DAC based LED drivers. Signed-off-by: Jeppe Odgaard <[email protected]>

martinjaeger · 2025-04-24T08:27:30Z

drivers/led/led_dac.c

+		.chan_cfg = {.channel_id = DT_PROP(n, channel),                                    \
+			     .resolution = DT_PROP(n, resolution),                                 \
+			     .buffered = DT_PROP(n, output_buffer),                                \
+			     .internal = false},                                                   \
+		.dac_max = LED_DAC_MAX_BRIGHTNESS(n), .dac_min = LED_DAC_MIN_BRIGHTNESS(n)         \


Suggested format to improve readibility (the \ characters still need to be re-aligned):

Suggested change

.chan_cfg = {.channel_id = DT_PROP(n, channel), \

.resolution = DT_PROP(n, resolution), \

.buffered = DT_PROP(n, output_buffer), \

.internal = false}, \

.dac_max = LED_DAC_MAX_BRIGHTNESS(n), .dac_min = LED_DAC_MIN_BRIGHTNESS(n) \

.chan_cfg = {

.channel_id = DT_PROP(n, channel), \

.resolution = DT_PROP(n, resolution), \

.buffered = DT_PROP(n, output_buffer), \

.internal = false, \

}, \

.dac_max = LED_DAC_MAX_BRIGHTNESS(n), \

.dac_min = LED_DAC_MIN_BRIGHTNESS(n), \

martinjaeger · 2025-04-24T08:34:28Z

drivers/led/led_dac.c

+	uint32_t dac_max;
+	uint32_t dac_min;


I'd suggest the following to avoid confusion with the actual dac max output value (e.g. 3.3V).

Suggested change

uint32_t dac_max;

uint32_t dac_min;

uint32_t dac_max_brightness;

uint32_t dac_min_brightness;

martinjaeger · 2025-04-24T08:40:15Z

I did not add voltage-off-mv as I cannot see why it is needed. Please enlighten me if I am wrong 😄

The idea was to define a value where the LED is fully off (similar to the point that @simonguinot made here #88259 (comment)). In theory, the VCCS could have an inverted input, so that the off-state is at high voltage and not 0. However, that's not very likely, so I'm fine if we assume a DAC output of 0 to be the off state.

github-actions bot added area: LED Label to identify LED subsystem area: Devicetree labels Apr 7, 2025

github-actions bot requested review from decsny, galak, Mani-Sadhasivam, rruuaanng and simonguinot April 7, 2025 13:39

github-actions bot assigned Mani-Sadhasivam and simonguinot Apr 7, 2025

jeppenodgaard force-pushed the add_led_dac branch from 13150d1 to 2b78cb3 Compare April 7, 2025 13:53

bjarki-andreasen reviewed Apr 7, 2025

View reviewed changes

jeppenodgaard commented Apr 8, 2025

View reviewed changes

jeppenodgaard force-pushed the add_led_dac branch 2 times, most recently from 80f790c to 771898d Compare April 9, 2025 12:26

jeppenodgaard requested a review from bjarki-andreasen April 9, 2025 12:29

jeppenodgaard force-pushed the add_led_dac branch 2 times, most recently from 39b4813 to af167a8 Compare April 9, 2025 12:53

simonguinot requested review from kartben, soburi, thedjnK and martinjaeger April 12, 2025 11:58

kartben requested a review from Copilot April 12, 2025 12:58

Copilot AI reviewed Apr 12, 2025

View reviewed changes

simonguinot reviewed Apr 12, 2025

View reviewed changes

rruuaanng reviewed Apr 13, 2025

View reviewed changes

martinjaeger requested changes Apr 14, 2025

View reviewed changes

jeppenodgaard force-pushed the add_led_dac branch from af167a8 to 043a291 Compare April 24, 2025 05:27

jeppenodgaard requested a review from rruuaanng April 24, 2025 05:28

jeppenodgaard requested review from simonguinot and martinjaeger April 24, 2025 05:28

jeppenodgaard commented Apr 24, 2025

View reviewed changes

jeppenodgaard force-pushed the add_led_dac branch from 043a291 to cc43ae2 Compare April 24, 2025 06:00

drivers: led: add led_dac

827325e

Add LED driver support for DAC based LED drivers. Signed-off-by: Jeppe Odgaard <[email protected]>

jeppenodgaard force-pushed the add_led_dac branch from cc43ae2 to 827325e Compare April 24, 2025 06:06

martinjaeger reviewed Apr 24, 2025

View reviewed changes


		#include <zephyr/device.h>

		int led_dac_set_raw(const struct device *dev, uint32_t led, uint32_t value);

drivers: led: add led_dac #88259

Are you sure you want to change the base?

drivers: led: add led_dac #88259

Conversation

jeppenodgaard commented Apr 7, 2025

bjarki-andreasen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjarki-andreasen Apr 8, 2025 • edited Loading

Choose a reason for hiding this comment

bjarki-andreasen Apr 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeppenodgaard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonguinot commented Apr 12, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

simonguinot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinjaeger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeppenodgaard commented Apr 23, 2025

jeppenodgaard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinjaeger commented Apr 24, 2025

bjarki-andreasen left a comment •

edited

Loading

bjarki-andreasen Apr 8, 2025 •

edited

Loading

bjarki-andreasen Apr 8, 2025 •

edited

Loading