Add generic gpio leds driver with sample #21578

KAGA164 · 2019-12-23T12:44:01Z

This PR adds leds gpio driver which is used to control the LED devices attached to the GPIO pins through the LED API. The driver support led_on , led_off and led_blink. The blink feature is realized by using software timer and can be disabled from compilation. Driver can handle all LEDs defined in DTS which contains id field. The led_set_brightness is not supported and I see no need to add it in case of the GPIO.

There are plans in feature to extend this driver to support power management.

Generation of the common static initializers for child-bindings is needed for this driver. It is provided in the #21393 PR. The first three commits come from there and driver will be rebased if #21393 will be merged, so them are not a part of this PR. Also driver needs additional id field in the DTS led description. This filed is not required to to ensure backward compatibility and it should be added to use this driver.

KAGA164 · 2019-12-23T12:44:29Z

@anangl Please look at this

Mani-Sadhasivam

@KAGA164 Thanks for the work! I don't know if you have noticed or not, I tried to add this driver a while ago (#9511) but didn't get time to complete due to some missing dependencies. But happy to see this finally getting some momentum.

Overall, I like the idea and implementation but there are some comments I have added. Please address those.

drivers/led/CMakeLists.txt

Mani-Sadhasivam · 2019-12-23T18:07:33Z

drivers/led/Kconfig.gpio

For consistency, let's call LED_GPIO everywhere.

Mani-Sadhasivam · 2019-12-23T18:08:27Z

drivers/led/Kconfig

drivers/led/Kconfig.led_gpio

Mani-Sadhasivam · 2019-12-23T18:09:18Z

drivers/led/gpio.c

Logging is broken as I'm getting lot of warnings.

I fix some issue with logging. I tested with debug log and without. You have to build sample for nrf52840_pca10056 board. The DTS descriptions is only aligned for it in this PR.

Mani-Sadhasivam · 2019-12-23T18:10:09Z

drivers/led/gpio.c

Sort includes accordingly

I put this in alphabetical order. Is this what you mean ?

samples/drivers/led_gpio/src/main.c

Mani-Sadhasivam · 2019-12-23T18:29:30Z

samples/drivers/led_gpio/src/main.c

Logging is not registered for this sample.

printk in sample ?

I don't really know what the convention is (logging vs printk in samples). I'm fine with either one but was just making sure it is not broken and only one implementation is used.

samples/drivers/led_gpio/src/main.c

Mani-Sadhasivam · 2019-12-23T18:31:23Z

samples/drivers/led_gpio/README.rst

Why aliases are required?

Aliases are probably more common to use in sample or application. I did change sample to use it.

You might need to mention the use of id property also, since that is a requirement for the driver.

Mani-Sadhasivam · 2019-12-23T18:38:14Z

drivers/led/gpio.c

Please split the software blinking functionality into a separate commit.

Ok. I splitted it but why You want me to do it ?

Just to make it easy to bisect in future and moreover these 2 are different functionalities.

dbkinder · 2019-12-23T21:16:07Z

samples/drivers/led_gpio/README.rst

change to led2, and led3 aliases

Ok. I removed led2 and led3. Sample can handle two leds now to ensure compatibility with more boards.

dbkinder · 2019-12-23T21:17:34Z

drivers/led/Kconfig.gpio

documentation and help text should capitalize LED

Ok. I think that I changed it in all places.

dbkinder · 2019-12-23T21:23:37Z

samples/drivers/led_gpio/README.rst

The reel_board used in your example doesn't have four LEDs on the board?

Right I checked it. So I put there nrf52840_pca10056 board which have aligned dts to use the LED GPIO driver. In future we can use reel_board.

drivers/led/led_gpio.c

zephyrbot · 2020-01-08T13:51:13Z

All checks passed.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

anangl · 2020-01-08T13:36:43Z

drivers/led/led_gpio.c

ARRAY_SIZE(led_dts) would be more adequate in this context.

anangl · 2020-01-08T14:15:55Z

drivers/led/led_gpio.c

What about using someting like this:

u32_t pin_value; if (data->cfg->gpio.polarity & GPIO_INT_ACTIVE_HIGH) { pin_value = (state ? 1 : 0); } else { pin_value = (state ? 0 : 1); }

Could that make things a bit clearer (and shorten the lines so that checkpatch no longer complains)?

I changed it also in init function.

anangl · 2020-01-09T08:20:21Z

#21393 needs to go in first.

stale

KAGA164 · 2020-07-13T08:14:05Z

@mbolivar-nordic I successfully rebased this PR and I was able to retrive all needed LED data using the new DT API. This driver still uses the ID property to identify the LED it seems for me it is ok since we do not create a driver instance for each GPIO port or virtual LED controller. According to @simonguinot comment about doing software blink functionality to common led.c I think that this functionality is dedicated to driving LEDs by GPIO and other hardware LEDs controllers probably needs to implement their own software blink. It seems, like creating the common software blink migth be really hard to implementation and may require many changes to the API.

carlescufi · 2020-07-13T09:51:52Z

@simonguinot are you OK with merging this now?
@mbolivar-nordic please take another look

pabigot

This goes too far changing what gpio-leds means as a compatible.

pabigot · 2020-07-13T13:18:46Z

drivers/led/led_gpio.c

Remove all this. It should be handled by gpio_pin_configure().

pabigot · 2020-07-13T13:18:50Z

drivers/led/led_gpio.c

The pin field here should be type gpio_pin_t. The polarity flag here should be named flags (or dt_flags) and be of type gpio_dt_flags_t.

I don't see why this is a separate structure rather than part of led_dts_cfg.

I changed pin->gpio_pin_t and polarity -> gpio_dt_flags_t. I used a separate structure for readability

pabigot · 2020-07-13T13:19:28Z

drivers/led/led_gpio.c

for pin_value use state.

pabigot · 2020-07-13T13:20:37Z

drivers/led/led_gpio.c

This should be

err = gpio_pin_configure(data[i].gpio_dev, cfg->led[i].gpio.pin, GPIO_OUTPUT_INACTIVE | cfg->led[i].gpio.dt_flags);

pabigot · 2020-07-13T13:20:48Z

drivers/led/led_gpio.c

and this goes away.

pabigot · 2020-07-13T13:21:01Z

drivers/led/led_gpio.c

so does this.

pabigot · 2020-07-13T13:23:39Z

dts/bindings/gpio/gpio-leds.yaml

This is an API change, incompatible with Linux where gpio-leds is defined.

Also the example devicetree modifications use identifiers that start with 1, which is unusual. Is id intended to be an opaque label, or an ordinal identifying the LED based on position in some ordering?

Finally the Zephyr led API requires this to be interpreted as an unsigned value, which should be noted (if this property is acceptable in this compatible).

Why we need to be compatible with the Linux ? I do no have to start from 1, I did this by chance. ID is used to identify the LED by API, because the GPIO port can be different for every LEDs in the leds node so the LEDs can have the same GPIO Pin so we cannot drive LEDs using it. That is why I provide a ID property which only should be unique for each LED. ID is used in the LED API to control the leds for_example led_on(led, id). Do you have any idea how to get ride with this ID property without changing whole led API and maintaining compatibility with Linux ?

Is there something wrong with using the index of the LED in the array as an ID ? This would allow to remove the id property.

I don't believe there's any guarantee that the order of children is preserved across builds, but that would probably be least intrusive way to go.

Well, the "id" property is required to use the led-gpio driver. Without this property, the driver won't work. So it should not be marked as optional in the binding because it is not. But if you allow the driver to work even if the "id" is not provided (by using indexes) then the "id" property becomes truly optional. And the driver becomes compatible with the existing board DTS from Linux as well.

Anyway, I think it would be nice to have this PR based on 26101. This would provide the led_get_info syscall and the index property.

Why we need to be compatible with the Linux ?

This is a documented design goal:

https://docs.zephyrproject.org/latest/guides/dts/design.html#source-compatibility-with-other-operating-systems

I don't believe there's any guarantee that the order of children is preserved across builds, but that would probably be least intrusive way to go.

What if we just added a new devicetree API macro named DT_CHILD_INDEX(node_id) which let you get the index of node_id in its sibling list?

So for example, if this is the dts:

leds { compatible = "gpio-leds"; red_led: led_0 { gpios = < &gpiob 0x16 0x1 >; label = "User LD1"; }; green_led: led_1 { gpios = < &gpioe 0x1a 0x1 >; label = "User LD2"; }; blue_led: led_2 { gpios = < &gpiob 0x15 0x1 >; label = "User LD3"; }; };

then:

DT_CHILD_INDEX(DT_NODELABEL(red_led)) is 0

DT_CHILD_INDEX(DT_NODELABEL(green_led)) is 1

DT_CHILD_INDEX(DT_NODELABEL(blue_led)) is 2

If I understand correctly, that at least is the necessary mechanism.

Or do people just want to wait for and rebase on top of #26101?

If DT_FOREACH_CHILD() is documented to produce the child nodes in tree specified order then we don't need an id property nor a DT_CHILD_INDEX() macro. The index in the leds[] array provides the id that identifies that LED.

. The index in the leds[] array provides the id that identifies that LED.

I see how that takes you from a parent node to an array initializer.

What I don't follow is how to start with a devicetree node referring to a child (i.e. referring to an LED), and get an index. At least, not without something like DT_CHILD_INDEX().

I guess it depends on how this is supposed to be used. But you're right, if you want to be able to identify a specific node by its devicetree name, then DT_CHILD_INDEX() is the way to do it.

pabigot · 2020-07-13T13:24:16Z

dts/bindings/gpio/gpio-leds.yaml

This does not belong here either. If something like this were needed, it'd be called label.

It can be renamed to label or even moved to Kconfig. I would to define here the driver name which can be used for bindings in device_get_binding

The standard way to do that is:

include: base.yaml properties: label: required: true

Agree with @pabigot, this needs to be done with the 'label' property. We have documentation for this here that users rely on:

https://docs.zephyrproject.org/latest/guides/dts/howtos.html#get-a-struct-device-from-a-devicetree-node

pabigot · 2020-07-13T13:26:09Z

@galak Please review this from the perspective of gpio-leds as a compatible and how a driver should be associated with it.

Add driver for controlling the LED devices attached to GPIO pins which are declared in devicetree under leds node and generated as static initializer. The driver takes care of configuring the pins to output state before using it. Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>

Add software LED blink feature for each defined LED. Blink funcionality uses software timer. Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>

Add LED ID for nrf52840_pca10056 board. This id is required for gpio leds driver. Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>

This commit provides a new sample which demonstrate how to use the GPIO LED drivers. Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>

simonguinot

Hi @KAGA164,

Thanks for this update. It is good to see this PR is moving on.

My major comment is about the software blinking support. I still think that implementing software blinking in LED drivers is not a good idea. IMHO a driver should only provide the blink callback if some hardware capabilities are available. Else a software blinking function could be used as a fallback. This way the same software blinking code would be shared between all the drivers. It is how it is done in Linux and I think it is the right way to go.

Please see the following comments as well.

simonguinot · 2020-07-14T20:45:27Z

drivers/led/led_gpio.c

 	struct device *gpio_dev;
 	const struct led_dts_cfg *cfg;
+#if defined(CONFIG_LED_SOFTWARE_BLINK)
+	struct k_timer timer;


Is that OK to use a timer here ? Behind gpio_pin_set(), we can have an I2C GPIO expander which may need to sleep and to wait for interrupts. Is that OK to sleep in a timer handler in Zephyr ?

In Linux, when a LED driver can't configure a LED without sleeping a work queue is used.

Note that the same context issue may apply to all the LED callbacks. AFAIK there is nothing preventing from calling a LED API function from an interrupt context.

It can be a problem. Maybe it was not possible to sleep in the gpio_pin_set or I did not realize that it is possible. Probably in that case we have to use a timer instance and workqueue instance for software blink but if this function can sleep then we cannot use also the system workqueue because it is also not good idea to sleep there so we have to define own workqueue for software blink feature which means creating the new thread for me it would be some overkill in used resource.

I will check, how Linux handle LEDs. I would like to have a possibility to call LED API from irq context.

BTW There is no any word in gpio_pin_set that this function should not be used from IRQ context or that it can sleep.
Do we have any cases where I2C GPIO expander is under it ?

Ok, we have SPI/I2C GPIO expander driver which works synchronously

I checked the Linux. Just as I thought it used combination of software timer and workqueue to handle software blink.

So since I know gpio_pin_set can be blocking we probably need to do some simillar logic here or like you said we can move this to something like generic led.c

I don't see how i2c_sync would help with invoking synchronous GPIO calls from an interrupt

They would not be synchronous. Using work queue adds much more jitter and latency compared to timer. From on chip gpios this can be seen as unnecessary limitation.

If the GPIO API can return the -EWOULDBLOCK when invoked from a context where the operation can't be completed synchronously. I would suppose that timer handler is such context then we can call it again by triggering a workqueue from timer context and there we can call the GPIO API. For pure GPIO driving the GPIO API will be called directly in timer handler because it does not return -EWOULDBLOCK so I think that it will not be a limitation there. From the other side it would not be a good idea to use the system workqueue because blocking there have an impact on other subsystem for example Bluetooth. We can define here a workqueue instance which will be used only by this feature but it means creating a new thread which will significantly increase the volatile memory consumption for such a simple functionality like LED blinking.

Bluetooth should not be affected by items in the system work queue unless those items misbehave. I don't think this driver should define a workqueue, but agree we need better control of which work queue is to be used by drivers that need them.

Again, I think that software LED blinking should be a fallback for all the LED devices/drivers without hardware blinking capabilities, and not only GPIO LEDs.

Moreover context should be handled from the led_set_{on,off,brightness} functions. Software LED blinking is only a user among others. To make a decision between calling the LED driver's callback directly or defer it to a workqueue, this functions need to be aware of the context (interrupt, thread) and also if the driver may sleep. I don't think that calling the driver's callback and handling a -EWOULDBLOCK error is correct. GPIO is not the only backend we have to deal with. It would be better to have a "can_sleep" flag advertised by the LED drivers. But to be efficient this needs some support from all the possible backends (including GPIO)...

I don't think that calling the driver's callback and handling a -EWOULDBLOCK error is correct. GPIO is not the only backend we have to deal with. It would be better to have a "can_sleep" flag advertised by the LED drivers.

GPIO is not the only API where an operation may not be isr-ok. The standard way of nicely supporting code that tries something that can't be done in the calling (interrupt) context is to return -EWOULDBLOCK. This fast-exit should have minimal performance impact, essentially no different to retrieving and checking a flag.

I understand that this seems a little odd, but it is the most flexible/least impact solution we have available. Since this LED driver can't tell whether a particular GPIO has to support can change pin state from interrupt context, so there is no way it could advertise a "can sleep" flag.

simonguinot · 2020-07-14T20:53:19Z

drivers/led/led_gpio.c

+		data->next_state = state;
+
+		atomic_set_bit(&data->flags, LED_BLINK_DISABLE);
+		k_timer_start(&data->timer, K_NO_WAIT, K_NO_WAIT);


I am sure there is a good reason for not stopping the timer here and for handling the set from the timer handler. But since I don't see, I am asking :)

This is here to avoid some race conditions when during the software blink is used, the led migth be set to different state from a higher priority interrupt or thread than a timer handler also the set function can be called when timer is pending. So setting the LEDs from one context and using the atomic flags eliminates this problem.

simonguinot · 2020-07-14T21:04:23Z

drivers/led/led_gpio.c

+
+static const struct led_driver_api led_api = {
+	.on = gpio_led_on,
+	.off = gpio_led_off,


Maybe you should define the set_brightness callback as well ?

It is not related with this patch but I don't see the purpose of this on/off callbacks. It seems to me that the set_brightness callback is enough to turn a LED off or on.

I can define it but I would not remove this on/off callback I rather prefer to use it instead of set_brightness knowing that I use the GPIO LED driver where LED can be only on or off. It is less confusing for me

Of course. I am just saying you could add the set_brightness callback.

Maybe just add a callback that returns -ENOTSUP? The led.h API, at least as it is currently defined, requires set_brightness to be defined. So this is technically an API violation, even though I agree with @KAGA164 that it doesn't make sense for an LED connected to a GPIO to do anything with set_brightness.

simonguinot · 2020-07-14T21:20:33Z

drivers/led/led_gpio.c

+	.led = led_dts
+};
+
+DEVICE_AND_API_INIT(gpio_led, DT_PROP(DT_DRV_INST(0), driver),


Is there a reason for not supporting several device/driver instances ? I think we should allow a board to define several GPIO LED nodes.

I don't mind many instances of this driver. I just wonder if it's really needed. I can add support for it if you want to. Is it possible to have many leds node instance ?

I think it is not super useful but I don't see any reason for not supporting it.

As an example, let's imagine a board with two distinct arrays of LEDs. If each array can have its own led-gpio node, then the LED indexes (or IDs) will reflect their hardware position in the array. It could be more meaningful for a user than having a big single node with the LED indexes not related with the hardware.

In my opinion we can just support one instance to start with and make the driver multi-instance later on if we need to, since we don't have a use case for more right now.

Nothing will break and it will be totally backwards compatible as long as we access the devices using label properties as discussed elsewhere in these comments.

simonguinot · 2020-07-14T21:37:28Z

dts/bindings/gpio/gpio-leds.yaml

Is there something wrong with using the index of the LED in the array as an ID ? This would allow to remove the id property.

simonguinot · 2020-07-15T11:09:05Z

@simonguinot are you OK with merging this now?
@mbolivar-nordic please take another look

Hi @carlescufi. It is not my place to say but thanks for asking. I think there are good things in this PR but I would not merge it right now. I would wait for reaching an agreement on the "id" property thing. I believe it is still possible. I also have a disagreement about software blinking. I don't think it should be implemented in the led-gpio driver (or any other LED driver). In addition, I sent a couple of comments to @KAGA164.

mbolivar-nordic · 2020-07-21T22:52:50Z

samples/drivers/led_gpio/README.rst

@@ -0,0 +1,32 @@
+.. _gpio-led-sample:
+
+LED GPIO Sample


I'm not sure about this. Why not just update the existing samples/basic/blinky to use the new driver?

Right now, getting people to blink a gpio-leds LED with zephyr requires them to understand devicetree, the driver model, and the GPIO API, and that's what we see in the blinky sample. It's a lot of abstractions for something simple, which your new driver makes even simpler.

I think it would be nicer if blinky/src/main.c was rewritten to work using the LED driver abstraction instead.

The basic samples are a common entry point into zephyr (well, for non-NCS users) and I think it's worthwhile having them be as simple as possible and show off the various pieces of driver API in the simplest way.

mbolivar-nordic · 2020-07-21T22:56:35Z

drivers/led/Kconfig.led_gpio

+
+config LED_GPIO
+	bool "LED GPIO driver"
+	depends on GPIO


Suggested change

depends on GPIO

depends on GPIO && $(dt_compat_enabled,gpio-leds)

mbolivar-nordic · 2020-07-21T22:57:17Z

drivers/led/Kconfig.led_gpio

+	help
+	  This option enables support for the LEDs connected to GPIO
+	  outputs. To be useful the particular board must have LEDs
+	  and they must be connected to the GPIO lines.


Suggested change

and they must be connected to the GPIO lines.

and they must be connected to the GPIO lines.

The board's devicetree should declare these in its /leds

node, which should have compatible "gpio-leds".

drivers/led/led_gpio.c

mbolivar-nordic · 2020-07-21T23:04:59Z

dts/bindings/gpio/gpio-leds.yaml

Agree with @pabigot, this needs to be done with the 'label' property. We have documentation for this here that users rely on:

https://docs.zephyrproject.org/latest/guides/dts/howtos.html#get-a-struct-device-from-a-devicetree-node

mbolivar-nordic · 2020-07-21T23:07:44Z

dts/bindings/gpio/gpio-leds.yaml

Why we need to be compatible with the Linux ?

This is a documented design goal:

https://docs.zephyrproject.org/latest/guides/dts/design.html#source-compatibility-with-other-operating-systems

I don't believe there's any guarantee that the order of children is preserved across builds, but that would probably be least intrusive way to go.

What if we just added a new devicetree API macro named DT_CHILD_INDEX(node_id) which let you get the index of node_id in its sibling list?

So for example, if this is the dts:

leds { compatible = "gpio-leds"; red_led: led_0 { gpios = < &gpiob 0x16 0x1 >; label = "User LD1"; }; green_led: led_1 { gpios = < &gpioe 0x1a 0x1 >; label = "User LD2"; }; blue_led: led_2 { gpios = < &gpiob 0x15 0x1 >; label = "User LD3"; }; };

then:

DT_CHILD_INDEX(DT_NODELABEL(red_led)) is 0

DT_CHILD_INDEX(DT_NODELABEL(green_led)) is 1

DT_CHILD_INDEX(DT_NODELABEL(blue_led)) is 2

If I understand correctly, that at least is the necessary mechanism.

Or do people just want to wait for and rebase on top of #26101?

mbolivar-nordic · 2020-07-21T23:14:16Z

include/drivers/led.h

 *
 * @param dev LED device
- * @param led LED channel/pin
+ * @param led LED channel/pin/id


This part I think conflicts with #26101 here:

db8213c

(The shortlog is led: move API functions to LED objects in case that commit SHA goes stale due to rebasing.)

I honestly like the #26101 phrasing a bit better. Could we adopt that here?

mbolivar-nordic · 2020-07-21T23:15:23Z

drivers/led/led_gpio.c

+	.led = led_dts
+};
+
+DEVICE_AND_API_INIT(gpio_led, DT_PROP(DT_DRV_INST(0), driver),


In my opinion we can just support one instance to start with and make the driver multi-instance later on if we need to, since we don't have a use case for more right now.

Nothing will break and it will be totally backwards compatible as long as we access the devices using label properties as discussed elsewhere in these comments.

mbolivar-nordic · 2020-07-21T23:17:55Z

drivers/led/led_gpio.c

+
+static const struct led_driver_api led_api = {
+	.on = gpio_led_on,
+	.off = gpio_led_off,


Maybe just add a callback that returns -ENOTSUP? The led.h API, at least as it is currently defined, requires set_brightness to be defined. So this is technically an API violation, even though I agree with @KAGA164 that it doesn't make sense for an LED connected to a GPIO to do anything with set_brightness.

mbolivar-nordic · 2020-07-21T23:21:37Z

@KAGA164 thanks very much for your update. I think it's looking much better. I have some comments and ideas I've left above. Sorry for the long review time; I was working on downstream tasks.

simonguinot · 2020-10-22T15:12:14Z

Hi @KAGA164. Are you still working on this PR ?

Mani-Sadhasivam · 2021-01-17T06:03:56Z

@KAGA164 Is this PR still active?

KAGA164 · 2021-01-20T12:54:59Z

Hi @Mani-Sadhasivam @simonguinot Sorry for the late answer. Unfortunately, I haven't had much time to work on this PR lately. I have read all of the comments and have some conclusions:

The software blink functionality can be moved to separated file and can be used as fallback for all LEDs drivers if those drivers don't set a led_blink(if set it means hardware blink is supported) API. It would need to sw timer and workqueue instance be added to every led data structure if software blink is enabled.
Since the GPIO API or other led API can block. We would need to have a software timer and workqueue instance per each led. Where a software timer is used to measure a delay_on/delay_off time. Next the led_on/led_off API is called from a timer handler(irq context) when it returns -EWOULDBLOCK then the workqueue will be used to call led_on/led_off from no interrupt context.
This driver was supposed to support the boards led devicetree node which based on the gpio-leds.yaml bindings since I cannot add there a id/index property like I did in this PR. How can we use a LED API to to select the correct LED ? Example of current LED node:

        leds {
                compatible = "gpio-leds";
                red_led: led_0 {
                        gpios = < &gpiob 0x16 0x1 >;
                        label = "User LD1";
                };
                green_led: led_1 {
                        gpios = < &gpioe 0x1a 0x1 >;
                        label = "User LD2";
                };
                blue_led: led_2 {
                        gpios = < &gpiob 0x15 0x1 >;
                        label = "User LD3";
                };
        };

Can we add DT_CHILD_INDEX macro do the devicetree as @mbolivar-nordic suggest ?
And then use:

    DT_CHILD_INDEX(DT_NODELABEL(red_led)) is 0
    DT_CHILD_INDEX(DT_NODELABEL(green_led)) is 1
    DT_CHILD_INDEX(DT_NODELABEL(blue_led)) is 2

There is also new bindings for led controller(led-controller.yaml) which would be suitable for this driver and then we can also add support for multi-instance but how can we control a LEDs from the boards devicetree then ? Should we add a led_controller to every board which has a leds node instead of it ?

What do you think about this @Mani-Sadhasivam @simonguinot @mbolivar-nordic ?

mbolivar-nordic · 2021-02-10T02:06:27Z

@KAGA164 I am happy to add DT_CHILD_INDEX if that's useful, yes.

KAGA164 requested review from Mani-Sadhasivam, carlescufi, dbkinder, galak, ioannisg, lemrey, nashif and ulfalizer as code owners December 23, 2019 12:44

Mani-Sadhasivam previously requested changes Dec 23, 2019

View reviewed changes

dbkinder previously requested changes Dec 23, 2019

View reviewed changes

KAGA164 force-pushed the generic_gpio_driver branch 2 times, most recently from a8d47da to 2a8c733 Compare January 2, 2020 11:17

anangl requested changes Jan 3, 2020

View reviewed changes

KAGA164 force-pushed the generic_gpio_driver branch 2 times, most recently from d2360ef to 53bdae5 Compare January 8, 2020 13:48

zephyrbot added area: Devicetree area: Boards area: API Changes to public APIs area: Samples Samples labels Jan 8, 2020

anangl reviewed Jan 8, 2020

View reviewed changes

KAGA164 force-pushed the generic_gpio_driver branch 3 times, most recently from c330ac7 to 59f21e8 Compare January 9, 2020 08:11

anangl approved these changes Jan 9, 2020

View reviewed changes

anangl added the Blocked Blocked by another PR or issue label Jan 9, 2020

KAGA164 force-pushed the generic_gpio_driver branch from 59f21e8 to 2c191b7 Compare January 9, 2020 10:41

github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 2, 2020

KAGA164 force-pushed the generic_gpio_driver branch from d77e161 to 9e05c5a Compare July 10, 2020 13:52

pabigot suggested changes Jul 13, 2020

View reviewed changes

Kamil Gawor added 4 commits July 14, 2020 10:41

driver: gpio_led: Software LED blink

cd065bd

Add software LED blink feature for each defined LED. Blink funcionality uses software timer. Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>

board: LED id for nrf52840_pca10056

8c29e95

Add LED ID for nrf52840_pca10056 board. This id is required for gpio leds driver. Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>

samples: driver: Add LED GPIO sample

9e72854

This commit provides a new sample which demonstrate how to use the GPIO LED drivers. Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>

KAGA164 force-pushed the generic_gpio_driver branch from 9e05c5a to 9e72854 Compare July 14, 2020 08:49

simonguinot reviewed Jul 14, 2020

View reviewed changes

nordic-krch mentioned this pull request Jul 16, 2020

RFC: asynchronous i2c and spi API #21538

Closed

mbolivar-nordic reviewed Jul 21, 2020

View reviewed changes

zephyrbot added platform: nRF Nordic nRFx area: LED Label to identify LED subsystem labels Jan 8, 2021

zephyrbot requested review from Mani-Sadhasivam and mbolivar-nordic January 8, 2021 22:54

zephyrbot assigned Mani-Sadhasivam Jan 8, 2021

This was referenced Apr 22, 2021

drivers: led: Add LED GPIO driver #34417

Merged

boards: thingy52_nrf52832: Update gpio-leds binding #34412

Merged

KAGA164 closed this Apr 28, 2021

simonguinot mentioned this pull request Apr 28, 2021

drivers: led_gpio: minor tweaks #34612

Merged

	depends on GPIO
	depends on GPIO && $(dt_compat_enabled,gpio-leds)

Add generic gpio leds driver with sample #21578

Add generic gpio leds driver with sample #21578

Uh oh!

Conversation

KAGA164 commented Dec 23, 2019

Uh oh!

KAGA164 commented Dec 23, 2019

Uh oh!

Mani-Sadhasivam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zephyrbot commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anangl commented Jan 9, 2020

Uh oh!

zephyrbot commented Jan 8, 2020 •

edited

Loading

carlescufi commented Jul 13, 2020 •

edited

Loading