drivers: led_strip: modernize ws2812 drivers #20393
Conversation
|
@mnkp this driver is exactly why I would like to have a "fast" gpio API, for reference. |
There was a problem hiding this comment.
This looks trivial, but I believe the desire is to take advantage of the fact we're touching these drivers to also complete the conversion to devicetree at the same time.
Looking at this my immediate questions are "what's the active level" (which would normally be or'd in from the devicetree node), and "should this be GPIO_OUTPUT_INIT_LOW or _HIGH".
If I'm going to do that, I want to know if people think I should create a unified set of bindings for this and the other driver for the same LED strip. Apart from the GPIO driver there is also a SPI driver for the same hardware. I was thinking of unifying the bindings similarly to how devices with I2C or SPI based drivers work (e.g. https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/iio/accel/lis302.txt). Worth doing? |
I couldn't say. The bitbang implementation seems significantly more limited than the SPI one, e.g. wrt pixel order. If the bindings are unified, should we try to support common functionality, or just blow up if the devicetree properties don't match the implemented behavior? Is there value in evolving the bitbang driver or is it obsolete so we should deprecate it and let it expire unmolested (beyond the minimal necessary changes currently in this PR)? If it's worth enhancing it then a unified binding seems to make sense. E.g. the way lis2dh (or eeprom in #19972) is done where the interface yaml includes common yaml. |
@mbolivar not actively, but I do have the HW. It was used for some micro:bit demos: https://www.kitronik.co.uk/5625-zip-halo-for-the-bbc-microbit.html |
I ultimately want the bit-banging driver to be the "main" driver, but I can't do that portably without fast generic GPIO routines, which of course we have deferred work on. (The SPI driver's use of memory is an obscenity, but it's the only portable way to manage this thing that I could find within Zephyr's current driver APIs.) So I guess what I'll do is create a separate binding for the SPI driver, and let the bit-bang driver take the main
OK, let's keep it then, and generalize it when we have the GPIO driver API to make it possible to get rid of the inline assembly. |
|
Stumbled across this issue today. You may want to have a look at this repo if you are not already familiar with it. Looks like a pretty well used and designed led driver api. |
|
This driver would be a good testcase for fast GPIO API (yes, we need it). The PR could be approved as is, but @mbolivar if you're planning to add DTS bindings we're going to wait :-) |
I'm halfway done with this. I have the SPI driver "fully" dt-ized (right now the conversion was only partial) and still need to do the DT one. I'll try to get this done tomorrow. |
Thanks for the pointer. We are familiar with that library -- you can see the discussion in #11917 for details, for example it is mentioned here: FastLED makes extensive use of C++ templates to get its job done, though, so we need a different approach for Zephyr. |
|
Thank you @mbolivar for the link. Interesting read indeed. I do not quite understand the meaning of your last comment regarding C++. Is Zephyr not intended to be used in a C++ environment? |
|
By policy Zephyr currently disallows use of C++ except within applications, so it's not suitable for a raw GPIO interface. |
|
Right -- we can only use C (and assembly) from drivers, which is the layer we're discussing here. |
I would be happy if this driver continues to use assembler, it is a very nice example. |
I would be unhappy if the driver continues to use assembler :). It is not portable. This implementation is nrf51 only. |
a) anyway a good example b) can be made portable, at least for Cortex-Mx |
drivers/ is not a place for example code.
The point of #11917 is to make the GPIO portions portable for all devices supported by zephyr, not just some. The nop-loops in the assembly are not GPIO related of course, but I see the precision timeout work that was recently merged as a good starting point for making that portable too, also without assembly. |
I think it would not help, you need to lock interrupts anyway to fulfill the timings. |
Of course you need to call irq_lock() while you are sending the pulses. My point is that now we have a portable way to convert nanoseconds to cycles, e.g. |
No, that is indeed great, I plan to use it in bit-bang swd driver to determine the number of nops. But TBH I have no idea how it would be useful to use it with C code. |
cbcfc94 to
a49276a
Compare
|
kicking ci |
aa9d226 to
c98ab1a
Compare
|
@mnkp please take another look at ws2812_gpio.c when you get a chance. I've made it multi-instance, but deleted the color channel order control. If that's OK for you I'll update the SPI driver in the same way and we should be good to go. Thanks! |
|
@jhedberg I've moved the assembly around a bit so it'll be easier to swap out when we get the "fast gpio" routines. In doing so, I noticed that this constraint also looked incorrect: This should be "l", not "r", AFAICT: the Arm ARM I checked said both Rd and Rn need to be r0-r7 for the str instruction. Mostly an FYI, but please check if you want. |
|
@mbolivar looks nice and clean! Are you verifying the correctness of the generated signal with actual HW, a logic analyser, or some other way? I'll try to find some time to test this PR on the ZIP Halo I have. |
Thanks!
I've got a Saleae hooked up to an nrf51_pca10028 for GPIO driver timing.
The 1 bits are pushing the limit if you believe the datasheet, but are well within the empirical limits in the blog post. I could drop a nop from T1H if you'd like to make that tighter. I am also visually testing on a 16 pixel GRBW ring from Adafruit. |
|
Marked DNM until @mnkp has a chance to review, since I need to make SPI driver changes depending on his response. |
I second! Sorry for a delayed review, not much free time these days. |
No problem! There was no rush, I just didn't want it merged halfway done. I've updated the SPI driver to match the GPIO changes. I had to put back the Kconfig file in the led_ws2812 sample to make the build system select the SPI driver by default. Leaving DNM on for now as I'm traveling for a US holiday and won't re-test the SPI driver until next week. |
Convert the GPIO based driver to the new GPIO API. (Only the gpio_configure() call is affected). Move configuration to DT where appropriate for both SPI and GPIO drivers, only leaving the SPI vs. GPIO decision in Kconfig (in addition to the basic enable for the driver.) Move some files around to clean up as a result of this change. led_ws2812 sample changes: - make the pattern easier to look at by emitting less light - use led_strip alias from DT to get strip device, allocate appropriate struct led_rgb buffer, etc. - move the pins around and remove 96b_carbon support (I have no board to test with) GPIO driver specific changes: - str is required to write OUTSET/OUTCLR, not strb. The registers are word-sized. - the str[b] registers must all be in r0-r7, so "l" is the correct GCC inline assembly constraint for both "base" and "pin" SPI driver specific changes: - match the GPIO driver in not supporting the update_channels API method, which never made sense for this type of strip - return -ENOMEM when the user tries to send more pixel data than we have buffer space for instead of -EINVAL Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
|
I've retested on nrf51_pca10028 and nrf52_pca10040, so removing DNM. @mnkp I've responded to your comments, please take another look when you get a chance. |
This is the only led_strip driver using GPIO.
@jhedberg are you still using this, btw?