drivers: led: Add LED GPIO driver #34417

MarekPieta · 2021-04-20T11:53:02Z

Change introduces LED GPIO driver.

pdunaj · 2021-04-20T14:18:55Z

drivers/led/led_gpio.c

can you please do it as
value<50 then off
value>=50 then on

this way any client using brightness will work without having to consider that this driver behaves differently.

pdunaj · 2021-04-20T14:19:26Z

drivers/led/led_gpio.c

I know we don't need it but maybe we could add blink using workqueue?

btw - if function is not supported won't it be enough to not add it to ops

I prefer to keep the driver as simple and small as possible (it could be used by memory-limited devices).
Work in driver would need to be added for LED every color channel. Application could use a single work for whole multicolor LED. Adding blinking to an application would be simple too.

jfischer-no · 2021-04-22T10:02:07Z

There is similar PR, "Add generic gpio leds driver with sample" #21578

jfischer-no · 2021-04-22T10:18:44Z

It does not build in CI, you need to add it to https://github.com/zephyrproject-rtos/zephyr/tree/master/tests/drivers/build_all

gmarull · 2021-04-22T10:13:29Z

drivers/led/led_gpio.c

There's no need for this macro, just use dev->config

gmarull · 2021-04-22T10:14:26Z

drivers/led/led_gpio.c

Can you use the recently introduced GPIO_DT_SPEC_GET utilities? https://docs.zephyrproject.org/latest/reference/peripherals/gpio.html#c.GPIO_DT_SPEC_GET

ok, I will change it

drivers/led/led_gpio.c

gmarull · 2021-04-22T10:20:54Z

drivers/led/led_gpio.c

I think that single return point doesn't make code flow more readable, my opinion though (non-blocking).

Suggested change

if (device_is_ready(led->dev)) {

err = gpio_pin_configure(led->dev, led->pin, led->flags);

if (err) {

LOG_ERR("Cannot configure GPIO (err %d)", err);

}

} else {

LOG_ERR("%s: GPIO device not ready", dev->name);

err = -ENODEV;

}

if (!device_is_ready(led->dev)) {

LOG_ERR("%s: GPIO device not ready", dev->name);

return -ENODEV;

}

err = gpio_pin_configure(led->dev, led->pin, led->flags);

if (err) {

LOG_ERR("Cannot configure GPIO (err %d)", err);

return err;

}

As I don't mind to have direct return instead of multiplying if-else blocks, I am against of returning directly from for loop.

In that case I suggest to leave it as it is. Could we do it @gmarull ?

drivers/led/led_gpio.c

KAGA164 · 2021-04-22T12:58:37Z

@jfischer-no Yes I created a similar PR #21578 sometimes ago with additional software blink feature but I didn't have time to finish it. We also decided there that software blink should be done as generic module which can be used by any driver LED driver which doesn't support hardware blinking. If we are ok with basing on LED order in the board dts as the LED ids which are used for identify LED in the API than I am ok with this PR and I will close my one. If we cannot base on LED order, we could consider basing this driver on the led-controller binding: https://github.com/zephyrproject-rtos/zephyr/blob/master/dts/bindings/led/led-controller.yaml

KAGA164 · 2021-04-22T12:39:33Z

drivers/led/led_gpio.c

Suggested change

err = gpio_pin_configure(led->dev, led->pin, led->flags);

err = gpio_pin_configure(led->dev, led->pin, GPIO_OUTPUT_INACTIVE | led->flags);

Maybe It is worth to add this to be sure that leds will be off after driver initialization

KAGA164 · 2021-04-22T12:42:59Z

drivers/led/Kconfig.gpio

Suggested change

depends on GPIO

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

btw. $(dt_compat_enabled,gpio-leds) - space must be excluded

KAGA164

Looks good

MarekPieta · 2021-04-23T15:17:38Z

@jfischer-no, I added requested test to build_all

gmarull · 2021-04-23T15:19:53Z

drivers/led/led_gpio.c

Any reason why DEVICE_DT_DEFINE is not used?

DEVICE_DT_DEFINE uses node label as a driver name. The label property is not assigned for led-gpio binding (only child nodes have it).

I handled it in the same way as it is done for led_pwm driver

DEVICE_DT_INST_DEFINE does not depend on node label, it uses ordinal information. I have quickly tried:

DEVICE_DT_INST_DEFINE(id, &led_gpio_init, device_pm_control_nop,\ NULL, &led_gpio_config_##id, \ POST_KERNEL, CONFIG_LED_INIT_PRIORITY, \ &led_gpio_api);

and it seems to build correctly. I'd suggest to use n, i, index instead of id.

Ok, changed. I also used i instead of id.

It seems like I looked at old version of docs. In current implementation full node name is used in case the label is not defined.

gmarull · 2021-04-23T20:20:02Z

drivers/led/led_gpio.c

DEVICE_DT_INST_DEFINE does not depend on node label, it uses ordinal information. I have quickly tried:

DEVICE_DT_INST_DEFINE(id, &led_gpio_init, device_pm_control_nop,\ NULL, &led_gpio_config_##id, \ POST_KERNEL, CONFIG_LED_INIT_PRIORITY, \ &led_gpio_api);

and it seems to build correctly. I'd suggest to use n, i, index instead of id.

Mani-Sadhasivam

Just a minor nit. Otherwise LGTM.

drivers/led/Kconfig

Change introduces LED GPIO driver. Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

Change adds LED GPIO driver to build_all test. Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

MarekPieta · 2021-04-26T15:10:28Z

@nashif, can we merge it ?

MarekPieta requested review from eobalski and pdunaj April 20, 2021 11:53

MarekPieta requested a review from Mani-Sadhasivam as a code owner April 20, 2021 11:53

MarekPieta added area: Drivers area: LED Label to identify LED subsystem labels Apr 20, 2021

zephyrbot assigned Mani-Sadhasivam Apr 20, 2021

pdunaj suggested changes Apr 20, 2021

View reviewed changes

MarekPieta force-pushed the gpio_led branch from 71ba13e to 549ce84 Compare April 21, 2021 11:16

pdunaj approved these changes Apr 21, 2021

View reviewed changes

eobalski approved these changes Apr 21, 2021

View reviewed changes

carlescufi requested review from gmarull and mbolivar-nordic April 22, 2021 09:32

MarekPieta requested a review from anangl April 22, 2021 09:57

jfischer-no requested a review from KAGA164 April 22, 2021 10:01

gmarull requested changes Apr 22, 2021

View reviewed changes

anangl reviewed Apr 22, 2021

View reviewed changes

drivers/led/led_gpio.c Outdated Show resolved Hide resolved

drivers/led/led_gpio.c Outdated Show resolved Hide resolved

KAGA164 reviewed Apr 22, 2021

View reviewed changes

KAGA164 approved these changes Apr 23, 2021

View reviewed changes

MarekPieta force-pushed the gpio_led branch 2 times, most recently from 6b780e6 to d174f7b Compare April 23, 2021 14:24

MarekPieta requested a review from nashif as a code owner April 23, 2021 15:16

github-actions bot added area: Devicetree area: Tests Issues related to a particular existing or missing test labels Apr 23, 2021

gmarull reviewed Apr 23, 2021

View reviewed changes

gmarull requested changes Apr 23, 2021

View reviewed changes

MarekPieta force-pushed the gpio_led branch from 2bb4f3c to a87b9e3 Compare April 23, 2021 21:00

gmarull approved these changes Apr 24, 2021

View reviewed changes

MarekPieta force-pushed the gpio_led branch from a87b9e3 to 8964295 Compare April 26, 2021 07:09

anangl approved these changes Apr 26, 2021

View reviewed changes

Mani-Sadhasivam approved these changes Apr 26, 2021

View reviewed changes

drivers/led/Kconfig Outdated Show resolved Hide resolved

MarekPieta added 2 commits April 26, 2021 11:17

drivers: led: Add LED GPIO driver

27a6785

Change introduces LED GPIO driver. Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

tests: drivers: Add LED GPIO driver to build_all

88718bc

Change adds LED GPIO driver to build_all test. Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

MarekPieta force-pushed the gpio_led branch from 8964295 to 88718bc Compare April 26, 2021 09:22

ioannisg merged commit 66b6f07 into zephyrproject-rtos:master Apr 27, 2021

mbolivar-nordic mentioned this pull request Apr 27, 2021

drivers: led_gpio: minor tweaks #34612

Merged

-		if (device_is_ready(led->dev)) {
-			err = gpio_pin_configure(led->dev, led->pin, led->flags);
-			if (err) {
-				LOG_ERR("Cannot configure GPIO (err %d)", err);
-			}
-		} else {
-			LOG_ERR("%s: GPIO device not ready", dev->name);
-			err = -ENODEV;
-		}
+		if (!device_is_ready(led->dev)) {
+			LOG_ERR("%s: GPIO device not ready", dev->name);
+			return -ENODEV;
+		}
+		err = gpio_pin_configure(led->dev, led->pin, led->flags);
+		if (err) {
+			LOG_ERR("Cannot configure GPIO (err %d)", err);
+			return err;
+		}

	err = gpio_pin_configure(led->dev, led->pin, led->flags);
	err = gpio_pin_configure(led->dev, led->pin, GPIO_OUTPUT_INACTIVE \| led->flags);

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

drivers: led: Add LED GPIO driver #34417

drivers: led: Add LED GPIO driver #34417

Uh oh!

Conversation

MarekPieta commented Apr 20, 2021

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!

jfischer-no commented Apr 22, 2021

Uh oh!

jfischer-no commented Apr 22, 2021

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!

gmarull Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

KAGA164 commented Apr 22, 2021

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!

KAGA164 left a comment

Choose a reason for hiding this comment

Uh oh!

MarekPieta commented Apr 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarekPieta Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarekPieta Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Mani-Sadhasivam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarekPieta commented Apr 26, 2021

Uh oh!

gmarull Apr 22, 2021 •

edited

Loading

MarekPieta Apr 23, 2021 •

edited

Loading

MarekPieta Apr 23, 2021 •

edited

Loading