Skip to content

Add generic gpio leds driver with sample #21578

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Add generic gpio leds driver with sample #21578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d30f4dc
driver: Add LED GPIO driver
Jan 2, 2020
cd065bd
driver: gpio_led: Software LED blink
Jan 2, 2020
8c29e95
board: LED id for nrf52840_pca10056
Dec 20, 2019
9e72854
samples: driver: Add LED GPIO sample
Dec 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions boards/arm/nrf52840dk_nrf52840/nrf52840dk_nrf52840.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,22 @@
led0: led_0 {
gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
label = "Green LED 0";
id = <1>;
};
led1: led_1 {
gpios = <&gpio0 14 GPIO_ACTIVE_LOW>;
label = "Green LED 1";
id = <2>;
};
led2: led_2 {
gpios = <&gpio0 15 GPIO_ACTIVE_LOW>;
label = "Green LED 2";
id = <3>;
};
led3: led_3 {
gpios = <&gpio0 16 GPIO_ACTIVE_LOW>;
label = "Green LED 3";
id = <4>;
};
};

Expand Down
1 change: 1 addition & 0 deletions drivers/led/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ zephyr_sources_ifdef(CONFIG_HT16K33 ht16k33.c)
zephyr_sources_ifdef(CONFIG_LP3943 lp3943.c)
zephyr_sources_ifdef(CONFIG_LP5562 lp5562.c)
zephyr_sources_ifdef(CONFIG_PCA9633 pca9633.c)
zephyr_sources_ifdef(CONFIG_LED_GPIO led_gpio.c)

zephyr_sources_ifdef(CONFIG_USERSPACE led_handlers.c)
1 change: 1 addition & 0 deletions drivers/led/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,6 @@ source "drivers/led/Kconfig.ht16k33"
source "drivers/led/Kconfig.lp3943"
source "drivers/led/Kconfig.lp5562"
source "drivers/led/Kconfig.pca9633"
source "drivers/led/Kconfig.led_gpio"

endif # LED
24 changes: 24 additions & 0 deletions drivers/led/Kconfig.led_gpio
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Copyright (c) 2019 Nordic Semiconductor ASA
# SPDX-License-Identifier: Apache-2.0

config LED_GPIO
bool "LED GPIO driver"
depends on GPIO
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".


if LED_GPIO

config LED_SOFTWARE_BLINK
bool "Software blink"
help
Support for an LED software blink based on
system timers

module = LED_GPIO
module-str = LED GPIO
source "${ZEPHYR_BASE}/subsys/logging/Kconfig.template.log_config"

endif # LED_GPIO
264 changes: 264 additions & 0 deletions drivers/led/led_gpio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,264 @@
/*
* Copyright (c) 2019 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/
#include <drivers/gpio.h>
#include <drivers/led.h>
#include <logging/log.h>

LOG_MODULE_REGISTER(led_gpio, CONFIG_LED_GPIO_LOG_LEVEL);

#define DT_DRV_COMPAT gpio_leds
#define LED_DATA(node_id) \
{ \
{ \
DT_GPIO_LABEL(node_id, gpios), \
DT_GPIO_PIN(node_id, gpios), \
DT_GPIO_FLAGS(node_id, gpios) \
}, \
DT_LABEL(node_id), \
DT_PROP(node_id, id) \
},
#define LED_SW_BLINK BIT(0)
#define LED_BLINK_DISABLE BIT(1)

struct gpio_cfg {
Copy link
Contributor

@pabigot pabigot Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@KAGA164 KAGA164 Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const char *port;
gpio_pin_t pin;
gpio_dt_flags_t flags;
};

struct led_dts_cfg {
struct gpio_cfg gpio;
const char *label;
uint32_t id;
};

struct led_gpio_data {
struct device *gpio_dev;
const struct led_dts_cfg *cfg;
#if defined(CONFIG_LED_SOFTWARE_BLINK)
Copy link
Contributor

@simonguinot simonguinot Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be interesting to move this software blink support directly into the LED API. This way every LED controllers without hardware blinking capability could use it (and not only led_gpio). If the feature is enabled, then led_blink() could run the hardware method 'api->blink()' if available and fallback on the software implementation else.

struct k_timer timer;
Copy link
Contributor

@simonguinot simonguinot Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@KAGA164 KAGA164 Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Author

@KAGA164 KAGA164 Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@KAGA164 KAGA164 Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@KAGA164 KAGA164 Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@nordic-krch nordic-krch Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@KAGA164 KAGA164 Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pabigot pabigot Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@simonguinot simonguinot Jul 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)...

Copy link
Contributor

@pabigot pabigot Jul 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

atomic_t flags;
uint32_t blink_delay_on;
uint32_t blink_delay_off;
bool next_state;
#endif
bool state;
};

struct led_gpio_cfg {
const struct led_dts_cfg *led;
uint32_t led_cnt;
};

static inline struct led_gpio_data *get_dev_data(struct device *dev)
{
return dev->driver_data;
}

static inline const struct led_gpio_cfg *get_dev_config(struct device *dev)
{
return dev->config_info;
}

static struct led_gpio_data *led_parameters_get(struct device *dev,
uint32_t led)
{
const struct led_gpio_cfg *cfg = get_dev_config(dev);
struct led_gpio_data *data = get_dev_data(dev);

for (size_t i = 0; i < cfg->led_cnt; i++) {
if (data[i].cfg->id == led) {
return &data[i];
}
}

return NULL;
}

static int gpio_led_set_no_delay(struct led_gpio_data *data, bool state)
{
int err;

data->state = state;

err = gpio_pin_set(data->gpio_dev, data->cfg->gpio.pin,
state);
if (err) {
LOG_DBG("%s turn %s error %d",
data->cfg->label,
state ? "on" : "off",
err);
} else {
LOG_DBG("%s turn %s", data->cfg->label,
state ? "on" : "off");
}

return err;
}

static int gpio_led_set(struct led_gpio_data *data, bool state)
{
#if defined(CONFIG_LED_SOFTWARE_BLINK)
if (atomic_test_bit(&data->flags, LED_SW_BLINK)) {
data->next_state = state;

atomic_set_bit(&data->flags, LED_BLINK_DISABLE);
k_timer_start(&data->timer, K_NO_WAIT, K_NO_WAIT);
Copy link
Contributor

@simonguinot simonguinot Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Author

@KAGA164 KAGA164 Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return 0;

}
#endif /* defined(CONFIG_LED_SOFTWARE_BLINK) */

return gpio_led_set_no_delay(data, state);
}

static int gpio_led_on(struct device *dev, uint32_t led)
{
struct led_gpio_data *data = led_parameters_get(dev, led);

if (!data) {
return -EINVAL;
}

return gpio_led_set(data, true);
}

static int gpio_led_off(struct device *dev, uint32_t led)
{
struct led_gpio_data *data = led_parameters_get(dev, led);

if (!data) {
return -EINVAL;
}

return gpio_led_set(data, false);
}

#if defined(CONFIG_LED_SOFTWARE_BLINK)
static void stop_sw_blink(struct led_gpio_data *data)
{
k_timer_stop(&data->timer);
data->blink_delay_on = 0;
data->blink_delay_off = 0;
atomic_clear_bit(&data->flags, LED_SW_BLINK);
}

static void led_timer_handler(struct k_timer *timer)
{
struct led_gpio_data *data;
uint32_t delay;
bool state;
int err;

data = CONTAINER_OF(timer, struct led_gpio_data, timer);

if (atomic_test_and_clear_bit(&data->flags, LED_BLINK_DISABLE)) {
stop_sw_blink(data);
gpio_led_set_no_delay(data, data->next_state);

return;
}

if (data->state) {
delay = data->blink_delay_off;
state = 0;
} else {
delay = data->blink_delay_on;
state = 1;
}

err = gpio_led_set_no_delay(data, state);
if (err) {
LOG_DBG("%s blink error: %d", data->cfg->label, err);
}

k_timer_start(&data->timer, K_MSEC(delay), K_NO_WAIT);
}

static int gpio_led_blink(struct device *dev, uint32_t led,
uint32_t delay_on, uint32_t delay_off)
{
struct led_gpio_data *data = led_parameters_get(dev, led);

if (!delay_on) {
return gpio_led_off(dev, led);
}

if (!delay_off) {
return gpio_led_on(dev, led);
}

data->blink_delay_on = delay_on;
data->blink_delay_off = delay_off;

atomic_set_bit(&data->flags, LED_SW_BLINK);

LOG_DBG("%s blink start, delay on: %d, delay off: %d",
data->cfg->label, delay_on, delay_off);

k_timer_start(&data->timer, K_NO_WAIT, K_NO_WAIT);

return 0;
}
#endif /* defined(CONFIG_LED_SOFTWARE_BLINK) */

static const struct led_driver_api led_api = {
#if defined(CONFIG_LED_SOFTWARE_BLINK)
.blink = gpio_led_blink,
#endif
.on = gpio_led_on,
.off = gpio_led_off,
Copy link
Contributor

@simonguinot simonguinot Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@KAGA164 KAGA164 Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@simonguinot simonguinot Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

static int gpio_led_init(struct device *dev)
{
int err;
const struct led_gpio_cfg *cfg = get_dev_config(dev);
struct led_gpio_data *data = get_dev_data(dev);

for (size_t i = 0; i < cfg->led_cnt; i++) {
data[i].gpio_dev = device_get_binding(cfg->led[i].gpio.port);
if (!data[i].gpio_dev) {
LOG_DBG("Failed to get GPIO device for port %s pin %d",
cfg->led[i].gpio.port, cfg->led[i].gpio.pin);
return -EINVAL;
}

/* Set all leds pin as output */
err = gpio_pin_configure(data[i].gpio_dev,
cfg->led[i].gpio.pin,
(GPIO_OUTPUT_INACTIVE |
cfg->led[i].gpio.flags));
if (err) {
LOG_DBG("Failed to set GPIO port %s pin %d as output",
cfg->led[i].gpio.port, cfg->led[i].gpio.pin);
return err;
}


data[i].cfg = &cfg->led[i];

#if defined(CONFIG_LED_SOFTWARE_BLINK)
k_timer_init(&data[i].timer, led_timer_handler, NULL);
#endif
}

return 0;
}

static const struct led_dts_cfg led_dts[] = {DT_INST_FOREACH_CHILD(0,
LED_DATA)};
static struct led_gpio_data led_data[ARRAY_SIZE(led_dts)];
static const struct led_gpio_cfg led_cfg = {
.led_cnt = ARRAY_SIZE(led_dts),
.led = led_dts
};

DEVICE_AND_API_INIT(gpio_led, DT_PROP(DT_DRV_INST(0), driver),
Copy link
Contributor

@simonguinot simonguinot Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@KAGA164 KAGA164 Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

@simonguinot simonguinot Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

gpio_led_init, led_data,
&led_cfg, POST_KERNEL, CONFIG_LED_INIT_PRIORITY,
&led_api);
11 changes: 11 additions & 0 deletions dts/bindings/gpio/gpio-leds.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ description: GPIO LEDs parent node

compatible: "gpio-leds"

properties:
Copy link
Contributor

@pabigot pabigot Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@KAGA164 KAGA164 Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pabigot pabigot Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard way to do that is:

include: base.yaml

properties:
    label:
      required: true

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

driver:
required: false
type: string
description: GPIO LED driver name
default: "LED_GPIO_0"

child-binding:
description: GPIO LED child node
properties:
Expand All @@ -15,3 +22,7 @@ child-binding:
required: true
type: string
description: Human readable string describing the device (used as device_get_binding() argument)
id:
Copy link
Contributor

@pabigot pabigot Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pabigot pabigot Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

@KAGA164 KAGA164 Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

@simonguinot simonguinot Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pabigot pabigot Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@simonguinot simonguinot Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@pabigot pabigot Jul 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic Aug 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

. 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().

Copy link
Contributor

@pabigot pabigot Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

required: false
type: int
description: Id needed for the GPIO LED driver to identify an LED
8 changes: 4 additions & 4 deletions include/drivers/led.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ __subsystem struct led_driver_api {
* This routine starts blinking an LED forever with the given time period
*
* @param dev LED device
* @param led LED channel/pin
* @param led LED channel/pin/id
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

* @param delay_on Time period (in milliseconds) an LED should be ON
* @param delay_off Time period (in milliseconds) an LED should be OFF
* @return 0 on success, negative on error
Expand All @@ -97,7 +97,7 @@ static inline int z_impl_led_blink(struct device *dev, uint32_t led,
* Calling this function after led_blink() won't affect blinking.
*
* @param dev LED device
* @param led LED channel/pin
* @param led LED channel/pin/id
* @param value Brightness value to set in percent
* @return 0 on success, negative on error
*/
Expand All @@ -119,7 +119,7 @@ static inline int z_impl_led_set_brightness(struct device *dev, uint32_t led,
* This routine turns on an LED
*
* @param dev LED device
* @param led LED channel/pin
* @param led LED channel/pin/id
* @return 0 on success, negative on error
*/
__syscall int led_on(struct device *dev, uint32_t led);
Expand All @@ -138,7 +138,7 @@ static inline int z_impl_led_on(struct device *dev, uint32_t led)
* This routine turns off an LED
*
* @param dev LED device
* @param led LED channel/pin
* @param led LED channel/pin/id
* @return 0 on success, negative on error
*/
__syscall int led_off(struct device *dev, uint32_t led);
Expand Down
9 changes: 9 additions & 0 deletions samples/drivers/led_gpio/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# SPDX-License-Identifier: Apache-2.0

cmake_minimum_required(VERSION 3.13.1)

include($ENV{ZEPHYR_BASE}/cmake/app/boilerplate.cmake NO_POLICY_SCOPE)
project(led_gpio)

FILE(GLOB app_sources src/*.c)
target_sources(app PRIVATE ${app_sources})
Loading