Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions boards/st/stm32wb5mm_dk/doc/stm32wb5mm_dk.rst
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,14 @@ Serial Port
STM32WB5MM-DK board has 2 (LP)U(S)ARTs. The Zephyr console output is assigned to USART1.
Default settings are ``115200 8N1``.

LEDs
----
STM32WB5MM-DK has two types of LEDs, The resources coming from STM32WB5MMG are
shared between the RGB and IR LEDs. It is not possible to use them
simultaneously. The selection is done by JP4 and JP5 jumpers.
To use the RGB LED, JP5 must be ON and JP4 OFF. In this configuration,
GPIO_SELECT2 (PH1) is the chip select for this RGB device on SPI1.


Programming and Debugging
*************************
Expand Down
22 changes: 22 additions & 0 deletions boards/st/stm32wb5mm_dk/stm32wb5mm_dk.dts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <st/wb/stm32wb55Xg.dtsi>
#include <st/wb/stm32wb55vgyx-pinctrl.dtsi>
#include <zephyr/dt-bindings/input/input-event-codes.h>
#include <zephyr/dt-bindings/led/led.h>

/ {
model = "STMicroelectronics STM32WB5MM Discovery Development Kit";
Expand All @@ -22,11 +23,23 @@
zephyr,flash = &flash0;
zephyr,code-partition = &slot0_partition;
};

rgb_led_strip: rgb_strip {
compatible = "ti,tlc59731";
gpios = <&gpioa 7 (GPIO_PULL_DOWN | GPIO_ACTIVE_HIGH)>;
chain-length = <1>;
color-mapping = <LED_COLOR_ID_BLUE
LED_COLOR_ID_GREEN
LED_COLOR_ID_RED>;
status = "disabled";
};

aliases {
watchdog0 = &iwdg;
die-temp0 = &die_temp;
volt-sensor0 = &vref;
volt-sensor1 = &vbat;
led-strip = &rgb_led_strip;
};
};

Expand Down Expand Up @@ -136,6 +149,15 @@ zephyr_udc0: &usb {
};
};

&gpioh {
rgb_cs: rgb_cs {
gpios = <1 GPIO_ACTIVE_HIGH>;
gpio-hog;
output-high;
status = "disabled";
};
};

&vref {
status = "okay";
};
Expand Down
1 change: 1 addition & 0 deletions drivers/led_strip/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ zephyr_library_sources_ifdef(CONFIG_WS2812_STRIP_SPI ws2812_spi.c)
zephyr_library_sources_ifdef(CONFIG_WS2812_STRIP_I2S ws2812_i2s.c)
zephyr_library_sources_ifdef(CONFIG_WS2812_STRIP_RPI_PICO_PIO ws2812_rpi_pico_pio.c)
zephyr_library_sources_ifdef(CONFIG_TLC5971_STRIP tlc5971.c)
zephyr_library_sources_ifdef(CONFIG_TLC59731_STRIP tlc59731.c)
2 changes: 2 additions & 0 deletions drivers/led_strip/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,6 @@ source "drivers/led_strip/Kconfig.apa102"

source "drivers/led_strip/Kconfig.tlc5971"

source "drivers/led_strip/Kconfig.tlc59731"

endif # LED_STRIP
11 changes: 11 additions & 0 deletions drivers/led_strip/Kconfig.tlc59731
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# Copyright (c) 2024 Javad Rahimipetroudi <[email protected]>
# SPDX-License-Identifier: Apache-2.0

config TLC59731_STRIP
bool "TLC59731 LED controller"
default y
depends on DT_HAS_TI_TLC59731_ENABLED
select GPIO
help
Enable driver for the Texas Instruments TLC59731 EasySet LED
controllers.
189 changes: 189 additions & 0 deletions drivers/led_strip/tlc59731.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
/*
* Copyright (c) 2024 Javad Rahimipetroudi <[email protected]>
*
* SPDX-License-Identifier: Apache-2.0
*/

#define DT_DRV_COMPAT ti_tlc59731

/**
* @file
* @brief LED driver for the TLC59731 LED driver.
*
* TLC59731 is a 3-Channel, 8-Bit, PWM LED Driver
* With Single-Wire Interface (EasySet)
*
* The EasySet protocol is based on short pulses and the time between
* them. At least one pulse must be sent every T_CYCLE, which can be
* between 1.67us and 50us. We want to go as fast as possible, but
* delays under 1us don't work very well, so we settle on 5us for the
* cycle time.
* A pulse must be high for at least 14ns. In practice, turning a GPIO on
* and immediately off again already takes longer than that, so no delay
* is needed there.
Comment on lines +21 to +23
Copy link
Member

Choose a reason for hiding this comment

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

I have some concerns about this implementation.
As @thedjnK has already commented, I think it is difficult to guarantee the timing.
As commented here, it may work without delay in the current environment, but it does not necessarily work on other boards.
This is undesirable since Zephyr drivers can be used regardless of board or SoC. (I think it's allowed to set it so it can't be used on a specific board, but it needs to be shown that it can't work as a design.)
In cases like this, Existing LED strip drivers mainly create waveforms using SPI or I2S. It's probably the most desirable.

Copy link

Choose a reason for hiding this comment

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

It's true that bitbanging on a GPIO risks messing up the timing. However, if the TLC59731 isn't connected to a SPI-MOSI or I2S pin, then bitbanging over GPIO is the only option.
So I think the driver should eventually get all three interfaces, with device tree properties defining which type of interface to use. But I think this should be taken step by step, and the GPIO version sounds like the simplest one to start with because it's the most generic one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I join @thedjnK and @soburi here.

@javad123javad please consider an SPI implementation. I can see that the TLC59731 controller is connected to a SPI muxed pin on the STM32WB5MM-DK board (the board you are using). So let's use it.

I remember I already made this comment: #68455 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comments, I'm working on the SPI implementation of the driver.

Copy link

Choose a reason for hiding this comment

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

Should the GPIO implementation (i.e. the implementation that also works in cases where the LEDs are not connected to a MOSI or I2S capable pin) be removed completely then? Or should the correct one be chosen in the device tree?

In either case, how should it be modeled in the device tree? Should it be as a sub-node of the SPI master (even though it's not SPI at all)? If so, should the pinmux be dynamically adapted to avoid that the clock is output and potentially confuses other devices? Or should it be left to the device tree writer to disable the default pinmux for the SPI master and instead use something that only enables the MOSI pin? Or do you have a different solution in mind?

Is there an existing driver that abuses SPI to bitbang some other protocol?

I think it is difficult to guarantee the timing.

The spi_bitbang driver and i2c_bitbang driver don't seem to share that concern, and AFAICS they don't do anything like disabling interrupts either. The spi_bitbang driver makes the frequency configurable from device tree, perhaps a similar property should be added?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your comments, I'm working on the SPI implementation of the driver.

Thanks ! That's being said... Since you are almost done with the GPIO driver, I suggest you finish it. I am not sure it will be merged. There are pros and cons to be discussed. But even if it doesn't end up in the Zephyr tree, it is interesting to have this driver ready "somewhere", at least for debugging purpose. For example with the WS2812 and compatibles LED controllers, we have the ws2812_gpio driver. It is not suitable for production needs. But it is sometimes useful to have it around. Since you are close with the GPIO driver, please finish it. I'll review it this week-end.

Copy link
Contributor

@simonguinot simonguinot Mar 23, 2024

Choose a reason for hiding this comment

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

Hi @arnout,

Should the GPIO implementation (i.e. the implementation that also works in cases where the LEDs are not connected to a MOSI or I2S capable pin) be removed completely then? Or should the correct one be chosen in the device tree?

I don't know. I think it is interesting to have the GPIO driver at least for debugging purposes. But it must be clear that it is not adapted to production needs. Let's wait for others to express their opinions.

In either case, how should it be modeled in the device tree? Should it be as a sub-node of the SPI master (even though it's not SPI at all)? If so, should the pinmux be dynamically adapted to avoid that the clock is output and potentially confuses other devices? Or should it be left to the device tree writer to disable the default pinmux for the SPI master and instead use something that only enables the MOSI pin? Or do you have a different solution in mind?

Is there an existing driver that abuses SPI to bitbang some other protocol?

Yes there is. See the ws2812 LED controller. We have 4 compatibles for the same device: worldsemi,ws2812-gpio, worldsemi,ws2812-i2s, worldsemi,ws2812-rpi_pico-pio, worldsemi,ws2812-spi. We also have 4 drivers (one for each compatible) and the ws2812_spi driver abuses the SPI protocol to output a signal comprehensible by stripped WS2812 controllers.

We could use the same method for the TLC59731.

OR we could follow this recommendation from @mbolivar-ampere: #55226 (comment). This would allow to have a single compatible with several drivers. In the driver the DT_ANY_INST_ON_BUS_STATUS_OKAY or DT_INST_ON_BUS macros can be used to check the on-bus property. And in kconfig the dt_compat_on_bus function can be used to enable SPI support if the on-bus property is set to spi.

It is my preferred choice.

I think it is difficult to guarantee the timing.

The spi_bitbang driver and i2c_bitbang driver don't seem to share that concern, and AFAICS they don't do anything like disabling interrupts either. The spi_bitbang driver makes the frequency configurable from device tree, perhaps a similar property should be added?

The SPI implementation should only be used in the case where a TLC59731 controller strip is connected to a SPI MOSI pin. And interrupts won't be an issue. Ideally DMA will be enabled and used. The idea for this driver is to use a SPI engine to output the correct waveform, offloading the CPU. So if no SPI engine can be used, then there is no point in using SPI protocol at all :) In this case the GPIO version of this driver should be preferred.

But in practice, when a such LED controller is built into a board, it is almost always wired to an SPI-muxed pin (as with the STM32WB5MM DK board). And in the case of an add-on module, then it's up to the user to connect it to a SPI MOSI pin on its board. So almost all the time you have a better option than GPIOs to drive a LED strip controller. That's why we are asking @javad123javad for a SPI implementation.

I hope this answers your questions :)

Copy link
Contributor

Choose a reason for hiding this comment

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

#70763 is a workaround that can be used, depending upon the length of time it needs to be locked for determines if it should be used or not

* A zero is represented by no additional pulses within a cycle.
* A one is represented by an additional pulse between 275ns and 2.5us
* (half a cycle) after the first one. We need at least some delay to get to
* 275ns, but because of the limited granularity of k_busy_wait we use a
* full 1us. After the pulse, we wait an additional T_CYCLE_1 to complete
* the cycle. This time can be slightly shorter because the second pulse
* already closes the cycle.
* Finally we need to keep the line low for T_H0 to complete the address
* for a single chip, and T_H1 to complete the write for all chips.
*/

#include <zephyr/drivers/led_strip.h>
#include <zephyr/drivers/gpio.h>
#include <zephyr/device.h>
#include <zephyr/kernel.h>

#include <zephyr/logging/log.h>
LOG_MODULE_REGISTER(tlc59731, CONFIG_LED_STRIP_LOG_LEVEL);

/* Pulse timing */
#define TLC59731_DELAY 0x01 /* us */
#define TLC59731_T_CYCLE_0 0x04 /* us */
#define TLC59731_T_CYCLE_1 0x01 /* us */
#define TLC59731_T_H0 (4 * TLC59731_T_CYCLE_0)
#define TLC59731_T_H1 (8 * TLC59731_T_CYCLE_0)
/* Threshould levels */
#define TLC59731_HIGH 0x01
#define TLC59731_LOW 0x00

/* Write command */
#define TLC59731_WR 0x3A

struct tlc59731_cfg {
struct gpio_dt_spec sdi_gpio;
size_t length;
};

static inline int rgb_pulse(const struct gpio_dt_spec *led_dev)
{
int fret = 0;

fret = gpio_pin_set_dt(led_dev, TLC59731_HIGH);
if (fret != 0) {
return fret;
}

fret = gpio_pin_set_dt(led_dev, TLC59731_LOW);
if (fret != 0) {
return fret;
}

return fret;
}

static int rgb_write_bit(const struct gpio_dt_spec *led_dev, uint8_t data)
{
rgb_pulse(led_dev);

k_busy_wait(TLC59731_DELAY);

if (data) {
rgb_pulse(led_dev);
k_busy_wait(TLC59731_T_CYCLE_1);
} else {
k_busy_wait(TLC59731_T_CYCLE_0);
}

return 0;
}

static int rgb_write_data(const struct gpio_dt_spec *led_dev, uint8_t data)
{
int8_t idx = 7;

while (idx >= 0) {
rgb_write_bit(led_dev, data & BIT((idx--)));
}

return 0;
}

static int tlc59731_led_set_color(const struct device *dev, struct led_rgb *pixel)
{

const struct tlc59731_cfg *tlc_conf = dev->config;
const struct gpio_dt_spec *led_gpio = &tlc_conf->sdi_gpio;

rgb_write_data(led_gpio, TLC59731_WR);
rgb_write_data(led_gpio, pixel->r);
rgb_write_data(led_gpio, pixel->g);
rgb_write_data(led_gpio, pixel->b);

return 0;
}

static int tlc59731_gpio_update_rgb(const struct device *dev, struct led_rgb *pixels,
size_t num_pixels)
{
size_t i;
int err = 0;

for (i = 0; i < num_pixels; i++) {
err = tlc59731_led_set_color(dev, &pixels[i]);
if (err) {
break;
}
}

return err;
}

static size_t tlc59731_length(const struct device *dev)
{
const struct tlc59731_cfg *config = dev->config;

return config->length;
}

static const struct led_strip_driver_api tlc59731_gpio_api = {
.update_rgb = tlc59731_gpio_update_rgb,
.length = tlc59731_length,
};

static int tlc59731_gpio_init(const struct device *dev)
{
const struct tlc59731_cfg *tlc_conf = dev->config;
const struct gpio_dt_spec *led = &tlc_conf->sdi_gpio;
int err = 0;

if (!device_is_ready(led->port)) {
LOG_ERR("%s: no LEDs found (DT child nodes missing)", dev->name);
err = -ENODEV;
goto scape;
}

err = gpio_pin_configure_dt(led, GPIO_OUTPUT_ACTIVE);
if (err < 0) {
LOG_ERR("%s: Unable to setup SDI port", dev->name);
err = -EIO;
goto scape;
}

err = gpio_pin_set_dt(led, TLC59731_LOW);
if (err < 0) {
LOG_ERR("%s: Unable to set the SDI-GPIO)", dev->name);
err = -EIO;
goto scape;
}

gpio_pin_set_dt(led, TLC59731_HIGH);
gpio_pin_set_dt(led, TLC59731_LOW);

k_busy_wait((TLC59731_DELAY + TLC59731_T_CYCLE_0));
scape:
return err;
}

#define TLC59731_DEVICE(i) \
static struct tlc59731_cfg tlc59731_cfg_##i = { \
.sdi_gpio = GPIO_DT_SPEC_INST_GET(i, gpios), \
.length = DT_INST_PROP(i, chain_length), \
}; \
\
DEVICE_DT_INST_DEFINE(i, tlc59731_gpio_init, NULL, NULL, &tlc59731_cfg_##i, POST_KERNEL, \
CONFIG_LED_STRIP_INIT_PRIORITY, &tlc59731_gpio_api);
DT_INST_FOREACH_STATUS_OKAY(TLC59731_DEVICE)
13 changes: 13 additions & 0 deletions dts/bindings/led_strip/ti,tlc59731.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Copyright (c) 2024 Javad Rahimipetroudi <[email protected]>
# SPDX-License-Identifier: Apache-2.0

compatible: "ti,tlc59731"
description: TLC59731 RGB LED Controller
include: [led-strip.yaml]

properties:
gpios:
required: true
type: phandle-array
description: |
GPIO to send command data to the controller.