Add leds_gpio driver and modify blinky application #9511

Mani-Sadhasivam · 2018-08-17T18:50:26Z

This PR adds leds_gpio driver which is used to control the LED devices attached to the GPIO pins through LED API and also modifies the existing blinky application as a consumer.

The leds_gpio driver currently supports only led_on and led_off APIs and there are plans to extend this driver to use timers for software blinking implementation in future.

The driver requires no change in the board configuration at all. It has been designed to reuse the gpio-leds configuration for compatibility and supports only two LED devices for now. Both driver and sample application are validated on 96Boards Carbon board.

Signed-off-by: Manivannan Sadhasivam [email protected]

codecov-io · 2018-08-17T19:20:33Z

Codecov Report

Merging #9511 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9511   +/-   ##
=======================================
  Coverage   48.25%   48.25%           
=======================================
  Files         293      293           
  Lines       44130    44130           
  Branches    10591    10591           
=======================================
  Hits        21296    21296           
  Misses      18567    18567           
  Partials     4267     4267

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1e912b...f42fa54. Read the comment docs.

Mani-Sadhasivam · 2018-08-18T03:37:06Z

@erwango This leds_gpio requires the boards to have leds node for proper working. Without this node, we will run into LED0_GPIO_CONTROLLER undefined error.

Currently we have the following in blinky sample which takes care of this scenario:

/* Change this if you have an LED connected to a custom port */
#ifndef LED0_GPIO_CONTROLLER
#define LED0_GPIO_CONTROLLER    LED0_GPIO_PORT
#endif

Any idea of how we can handle this in driver? Moving the above defines to driver itself?

erwango

Some comments on otherwise good stuf

erwango · 2018-08-20T09:41:49Z

drivers/led/Kconfig.gpio

Can't we use the 'label' to get the instance name?

erwango · 2018-08-20T09:44:04Z

drivers/led/Kconfig.gpio

I'm finding 'LED0_GPIO' a bit confusing, as I read it 'GPIO of LED0'.
Would LED_GPIO_0 be a bit less confusing?

Actually, the naming convention aligns with the DTS generated header (LEDx_GPIO). But anyway, LED_GPIO_x seems to be good.

@Mani-Sadhasivam , sorry but I still find it a bit confusing actually. What about GPIO_LED ?
@MaureenHelm , any opinion on that?

erwango · 2018-08-20T09:48:17Z

drivers/led/leds_gpio.c

Is 'led' argument actually required ?
We should be able to retrieve it from *dev.
And I think this is the interest of the API, so user doesn't care of the PIN

led argument is there only for the purpose of LED API and yes, we can retrieve it from *dev. Anyway, passing the led argument gives us the possibility of checking the correct pin in led_on/led_off as below:

/* Make sure that the LED is valid */ if (led != cfg->gpio_pin) return -EINVAL;

Will include the above check.

@Mani-Sadhasivam ,

Anyway, passing the led argument gives us the possibility of checking the correct pin in led_on/led_off

In the other hand not having 'led' as arguments makes this kind of issue impossible.
I still prefer this is removed from the API.

In the other hand not having 'led' as arguments makes this kind of issue impossible.
I still prefer this is removed from the API.

@erwango No. This only holds true for leds-gpio driver, for other drivers there is no GPIO information in cfg.

I agree the argument should stay and the driver should check it.

Can you check following PR to see if it wouldn't provide similar service and spare the argument?
#12056

I don't see how #12056 relates. I also agree that since this is being done as an implementation for an API that is designed to support multiple LEDs the argument must stay. However, see review summary.

dbkinder · 2018-08-20T15:50:38Z

samples/basic/blinky/README.rst

Change drive LED devices to configure LED devices. To me, the word drive has hardware implications (voltage, resistors, and such), whereas the previous word configure feels better here since it's a software configuration we're discussing, right?

Actually, in this PR I have moved the configuration part inside the LED GPIO driver. So we don't configure pins in this sample anymore.

Mani-Sadhasivam · 2018-08-21T02:47:33Z

@erwango I have updated the PR incorporating your suggestions. Still, we have to figure out how to solve issues with boards without leds node.

dbkinder

OK

MaureenHelm · 2018-08-22T20:08:42Z

drivers/led/leds_gpio.c

Many NXP boards invert the polarity, meaning you have to write a zero to the gpio to turn on the LED.

Okay. Then the best way to address is to have a invert-polarity property on the leds node. What is your opinion?

@MaureenHelm Just a ping over my query.

We've already got information about the polarity in dts, can you use that?

zephyr/boards/arm/frdm_k64f/frdm_k64f.dts

Lines 50 to 51 in c1f54cc

red_led: led@0 {

gpios = <&gpiob 22 GPIO_INT_ACTIVE_LOW>;

Hmm, makes sense. Thanks for the pointer!

Mani-Sadhasivam · 2018-08-26T18:16:30Z

@MaureenHelm Updated the PR with polarity check.

MaureenHelm

Please fix the CI failures.

The merge window is currently closed to new features, so we need to wait to merge this PR until after the 1.13 release is complete.

MaureenHelm · 2018-08-27T18:20:06Z

drivers/led/leds_gpio.c

How about replacing the if/else with:

gpio_pin_write(data->gpio, led, (cfg->gpio_polarity & GPIO_INT_ACTIVE_HIGH) != 0);

yeah, but I just wanted to make it explicit for the user here. Anyway, I can modify this.

drivers/led/leds_gpio.c

MaureenHelm · 2018-08-27T18:24:25Z

drivers/led/leds_gpio.c

Similar to above, try this instead of the if/else:

gpio_pin_write(data->gpio, led, (cfg->gpio_polarity & GPIO_INT_ACTIVE_HIGH) == 0);

Mani-Sadhasivam · 2018-08-28T03:22:26Z

@MaureenHelm CI failure is due to the absence of leds node in board DTS. Currently, the blinky sample has the following defines for the boards without leds node.

/* Change this if you have an LED connected to a custom port */
#ifndef LED0_GPIO_CONTROLLER
#define LED0_GPIO_CONTROLLER    LED0_GPIO_PORT
#endif

We need to add a few more for FLAGS, LABEL etc... So I just asked before that whether we need to have these #ifdefs or any other better solution available?

MaureenHelm · 2018-08-28T18:41:02Z

We need to add a few more for FLAGS, LABEL etc... So I just asked before that whether we need to have these #ifdefs or any other better solution available?

The better solution is to add led nodes to the board dts, otherwise we need the defines.

Mani-Sadhasivam · 2018-08-30T02:21:18Z

@erwango @MaureenHelm PR updated as per the comments. With this PR, blinky sample won't build for boards which don't have leds node in DT. I think this limitation is fine since most of the boards support DT and using defines will turn out be a hack.

MaureenHelm · 2018-09-19T18:35:35Z

drivers/led/leds_gpio.c

I agree the argument should stay and the driver should check it.

MaureenHelm · 2018-09-19T18:41:38Z

@erwango Please have another look

Mani-Sadhasivam · 2018-09-30T14:52:41Z

@erwango Can you please take a look?

Mani-Sadhasivam · 2018-10-09T17:38:13Z

Build failed due to lack of onboard LED support in em_starterkit boards. Do I need to add the onboard led support or is there any work going on?

erwango · 2018-10-30T16:11:27Z

@Mani-Sadhasivam

Build failed due to lack of onboard LED support in em_starterkit boards. Do I need to add the onboard led support or is there any work going on?

I think it is ok if you do.

Add driver for controlling the LED devices attached to GPIO pins which are declared in devicetree under leds node. The driver takes care of configuring the pins to output state before using it. Signed-off-by: Manivannan Sadhasivam <[email protected]>

leds_gpio driver is used to control the LED devices attached to GPIO pins onboard. Hence, convert the blinky application to use this driver instead of toggling the GPIO pin manually. Signed-off-by: Manivannan Sadhasivam <[email protected]>

Since LP3943 is an I2C device, relevant Kconfig symbols should only be used when I2C is enabled. Signed-off-by: Manivannan Sadhasivam <[email protected]>

Mani-Sadhasivam · 2018-12-21T10:57:57Z

@erwango Mind taking a look at the updated rev?

erwango · 2018-12-21T12:17:20Z

@Mani-Sadhasivam ,

@erwango Mind taking a look at the updated rev?

Can you sum up the changeson this update? This has been a while ... :-)

Mani-Sadhasivam · 2018-12-21T13:00:43Z

@erwango Nothing much really... There was a build failure when you last reviewed but that got fixed when LED node was added to em_starterkit board.

Anyway, here is the changeset:

Rebased to master
Fixed the LP3943 Kconfig to enable the default options only when CONFIG_I2C is set. Otherwise,
it will give errors as !HAS_DTS_I2C will become true.

pabigot

I think a GPIO LED driver is very much needed; all existing uses of GPIO LEDs ignore the active level which is annoying when working on cross-platform applications.

However, I also really don't like having a distinct driver instance for each GPIO LED on the board: the boards I use most have 4 to 6 LEDs. IMO this should be reworked so that each led-gpio instance supports multiple LEDs, just like all the other implementations of the generic LED API. I think it would be fair to require that the LEDs be located on the same GPIO device and have the same active level so that information need not be replicated; only the pin selector for each LED would be needed.

This does get a little tricky to do generically until device-tree support is more robust, though if there's agreement this is worth pursuing I do have some ideas. But honestly I'd rather not have this driver available than have it merged with this design, which is why I'm currently nacking the PR. If I'm the only one who feels this way I'll remove the nack.

The only board I'm immediately aware of that has GPIO LEDs on different gpio devices are the Particle boards proposed in #12195; for there there would have to be two devices, but the LEDs on each gpio device have different roles so that's not a bad thing.

pabigot · 2018-12-27T13:02:41Z

drivers/led/leds_gpio.c

I don't see how #12056 relates. I also agree that since this is being done as an implementation for an API that is designed to support multiple LEDs the argument must stay. However, see review summary.

@MaureenHelm

Dismissing as @MaureenHelm and@pabigot are aligned with proposition (at least on the point I disagreed)

Mani-Sadhasivam · 2020-01-22T16:44:34Z

Closing at it got superseded.

Mani-Sadhasivam requested a review from dbkinder as a code owner August 17, 2018 18:50

Mani-Sadhasivam requested review from MaureenHelm, erwango and galak August 17, 2018 18:50

Mani-Sadhasivam added the area: LED Label to identify LED subsystem label Aug 17, 2018

erwango previously requested changes Aug 20, 2018

View reviewed changes

dbkinder suggested changes Aug 20, 2018

View reviewed changes

Mani-Sadhasivam force-pushed the gpio_led branch from e673e79 to 3ebb598 Compare August 20, 2018 17:17

dbkinder approved these changes Aug 21, 2018

View reviewed changes

MaureenHelm reviewed Aug 22, 2018

View reviewed changes

Mani-Sadhasivam force-pushed the gpio_led branch 2 times, most recently from cb519b1 to 748b004 Compare August 26, 2018 18:13

MaureenHelm requested changes Aug 27, 2018

View reviewed changes

Mani-Sadhasivam force-pushed the gpio_led branch from 748b004 to 6960b8a Compare August 28, 2018 03:07

Mani-Sadhasivam requested a review from nashif as a code owner August 29, 2018 18:05

MaureenHelm approved these changes Sep 19, 2018

View reviewed changes

drivers/led/leds_gpio.c Outdated

Copy link

Member

MaureenHelm Sep 19, 2018

Choose a reason for hiding this comment

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

I agree the argument should stay and the driver should check it.

galak mentioned this pull request Oct 2, 2018

boards: Cleanup gpio flags in device tree (fixes button example) #10300

Closed

Mani-Sadhasivam force-pushed the gpio_led branch from 399cfe4 to 1e3e05a Compare October 9, 2018 17:05

Mani-Sadhasivam added 2 commits December 21, 2018 15:13

led: lp3943: Make default Kconfig definitions depend on I2C

f42fa54

Since LP3943 is an I2C device, relevant Kconfig symbols should only be used when I2C is enabled. Signed-off-by: Manivannan Sadhasivam <[email protected]>

Mani-Sadhasivam force-pushed the gpio_led branch from 1e3e05a to f42fa54 Compare December 21, 2018 09:44

pabigot self-requested a review December 27, 2018 12:58

pabigot suggested changes Dec 27, 2018

View reviewed changes

Mani-Sadhasivam closed this Jan 22, 2020

Add leds_gpio driver and modify blinky application #9511

Add leds_gpio driver and modify blinky application #9511

Uh oh!

Conversation

Mani-Sadhasivam commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Mani-Sadhasivam commented Aug 18, 2018

Uh oh!

erwango left a comment

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!

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 commented Aug 21, 2018

Uh oh!

dbkinder left a comment

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!

Mani-Sadhasivam commented Aug 26, 2018

Uh oh!

MaureenHelm left a comment

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!

Mani-Sadhasivam commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaureenHelm commented Aug 28, 2018

Uh oh!

Mani-Sadhasivam commented Aug 30, 2018

Uh oh!

Mani-Sadhasivam commented Aug 17, 2018 •

edited

Loading

codecov-io commented Aug 17, 2018 •

edited

Loading

Mani-Sadhasivam commented Aug 28, 2018 •

edited

Loading