drivers: led: is31fl3197 Arduino GIGA

[KurtE]

Start of support for the is31fl3197 LED controller used by Arduino in the Arduino Giga Display shield.

So far there is just enough implemented that allows me to control the LEDS, to the same extent as our version of the library we implemented that runs on ArduinoCore-zephyr

@pillo79 @facchinm - I am not sure what direction Arduino is heading with functionality such as this.
Add support to zephyr? Or do it external as libraries within ArduinoCore-zephyr (either integrated within the zephyr builds or external libraries), This is also true, about for some of the other
features of the display, like the display itself. Should there be samples setup to actually
display something on the display, preferably without the need for LVGL.

So far this only has the minimal support for setting up to 4 LEDS (the GIGA only uses 3).

Added sample sketch that runs on the GIGA. Currently it works with just
west build -p -b arduino_giga_r1//m7

I put the device tree into sample/boards. Potentially it should be defined as part of the
overlay in the arduino_giga_display_shield, in which case the west build should
define the shield as part of the command.

Let me know know what you think.
Thanks

[KurtE]

@pillo79 @facchinm and all - As far as I can tell, the basic parts of this PR are working. That is I can set the RGB led on the
giga display, the same as we have setup in the Arduino library. I know that this it currently does not fully implement all
of the features of this LED chip driver, but if this is pulled in, others can add it, especially if there are boards that use some of
the different features.

I am going to mark this as ready to review, however I know there are issues with twister, as it appears like the GIGA board
and guessing probably likewise the Portenta H7 have not been used as a setup for any of the samples. One issue I ran into
was the GIGA was not marked in the yaml file as supporting I2C, which I have added and the twister run goes a lot farther

Currently I have the sample setup to work with just specifying board as arduino_giga_r1//m7, as the shield currently
does not define the LED info. I don't know if one can specify an overlay file for a shield under a samples
boards directory, like maybe set the name as arduino_giga_display_shield.overlay? I may try it again with build line like:
west build -p -b arduino_giga_r1//m7 --shield arduino_giga_display_shield
I believe this can also be made to work with a portenta H7 with the mid carrier board. As I believe it's connector
to for the display shield includes an I2C object connected to those pins. May try that out of curiosity.
But I don't believe any of the Arduino Portenta carrier boards or shields are defined as shields in zephyr.
EDIT: - Portenta connector does not have I2C connections to the Display shield. So that won't work.
(Unless you use jumpers, which I have tried from I2C0 pins on Mid Carrier to the SCL/SDA pins on the
camera connector)

Also should mention: that the code is hard coded similarly to the is31fl3194 code in that it assumes RED/GREEN/BLUE
as the LEDS and it assumes they are on OUT1-3. I have the start of OUT4, that I again hard coded to WHITE:
But suppose I use one of these drivers for a stop light and the leds are: Green, Yellow, Red. Also should the colors
and slots be dictated by the colors passed in. For example if I have only two colors can I specify that I am using Out 2 and 3?

But before I go much further, it would help to know if this is a direction that Arduino wishes to go.

[simonguinot]

Hi @KurtE,

Thanks for this PR !

Can it be merged with the driver is31fl3194 ? into a is31fl319x driver ?

I didn't check but I am asking because in Linux the leds-is31fl319x driver supports the IS31FL319{0,1,3,6,9} models...

[KurtE]

Can it be merged with the driver is31fl3194 ? into a is31fl319x driver ?

I didn't check but I am asking because in Linux the leds-is31fl319x driver supports the IS31FL319{0,1,3,6,9} models...

Good question, both of them have stuff in common, and I started off that way, but then found many of the registers
appear to be different. Like fixed max current here versus set. Also can control which channels are active or not,
maybe you can with the 94 as well. They probably can be combined, but unclear how much they would share in
common.

@pillo79 As the one who originally did the 94 version, What do you think?

[pillo79]

Thanks Kurt! I also think this is worth merging with the existing driver, there's a lot of common code, and the few exceptions look like they can be specifically handled. Take a look at the end of drivers/led/lp50xx.c, for example, in how it defines id and max_leds depending on one of the many HW models. If you need many of these, you can also store id directly or a clever enum index into a register table, etc.

Oh, and re/ Arduino attitude, upstream is the way to go so 👍 on that! By all means make it part of the Giga Display shield, that way it will be automatically available everywhere you use that and not only in the test code 🙂

[mjs513]

@KurtE
Had to pretty much do the same thing for trying to merge the OV7675 into the OV7670 driver. Heres the link I used as a base to the LED driver:

zephyr/drivers/led/lp50xx.c

Lines 391 to 413 in 8843fd9

	#undef DT_DRV_COMPAT
	#define DT_DRV_COMPAT ti_lp5009
	DT_INST_FOREACH_STATUS_OKAY_VARGS(LP50XX_DEVICE, 5009, LP5012_NUM_MODULES)
	#undef DT_DRV_COMPAT
	#define DT_DRV_COMPAT ti_lp5012
	DT_INST_FOREACH_STATUS_OKAY_VARGS(LP50XX_DEVICE, 5012, LP5012_NUM_MODULES)
	#undef DT_DRV_COMPAT
	#define DT_DRV_COMPAT ti_lp5018
	DT_INST_FOREACH_STATUS_OKAY_VARGS(LP50XX_DEVICE, 5018, LP5024_NUM_MODULES)
	#undef DT_DRV_COMPAT
	#define DT_DRV_COMPAT ti_lp5024
	DT_INST_FOREACH_STATUS_OKAY_VARGS(LP50XX_DEVICE, 5024, LP5024_NUM_MODULES)
	#undef DT_DRV_COMPAT
	#define DT_DRV_COMPAT ti_lp5030
	DT_INST_FOREACH_STATUS_OKAY_VARGS(LP50XX_DEVICE, 5030, LP5036_NUM_MODULES)
	#undef DT_DRV_COMPAT
	#define DT_DRV_COMPAT ti_lp5036
	DT_INST_FOREACH_STATUS_OKAY_VARGS(LP50XX_DEVICE, 5036, LP5036_NUM_MODULES)

[KurtE]

Thanks @pillo79 @mjs513,

Thanks Kurt! I also think this is worth merging with the existing driver

Can do. At least the basics, that the display shield uses. Don't have any other things to try more advanced stuff on.

My first thoughts were to hack it and drive everything off of Product ID. I believe the 94 has a fixed ID of 0xce
Whereas the 97s ID is the I2C 8 bit address: Currently checked like:
if (prod_id != (config->bus.addr << 1)) {

Oh, and re/ Arduino attitude, upstream is the way to go so 👍 on that! By all means make it part of the Giga Display shield, that way it will be automatically available everywhere you use that and not only in the test code 🙂

I agree, that in the Zephyr world, having it all here under the shield is the way to go and not have to put the DT info in the
test sketch. I had it there to begin with, with the testing, will move it back there.

Some questions on this, should I then remove the GIGA from the integration section of the sample.yaml? As I don't believe
there are any other examples for them?

Include Portenta H7? I have a overlay I experimented with yesterday, that had the LEDS working on an H7 plugged into a MID
Carrier, which has a display shield plugged into it. But it only worked if I put jumpers to I2C pins. I tried it as the Display Shield
schematic that looked like there was a connection for Portenta with I2C pins... Just not sure how.
Guessing probably punt for now.

Main Secondary Question:

Currently with ArduinoCore-zephyr if I wish to build for a GIGA, it includes all of the Giga Shield stuff, regardless if I actually have a shield.

With the MBED version, only the basics for the display are included with a board release in the library Arduino_H7_video, The
rest of the functionality is contained in libraries you need to install.

I decided to try first with the LEDS as it is pretty self contained and not very large. Our zephyr version of Arduino library for this:

#include "GigaDisplayRGB.h"

GigaDisplayRGB::GigaDisplayRGB(){
}

void GigaDisplayRGB::begin()
{
    Wire1.begin();
    writeByte(0x1, 0xF1);
    writeByte(0x2, 0xFF);
}

void GigaDisplayRGB::on(uint8_t r, uint8_t g, uint8_t b)
{
    writeByte(0x10, r);
    writeByte(0x11, g);
    writeByte(0x12, b);
    writeByte(0x2b, 0xc5);
}

void GigaDisplayRGB::off()
{
    on(0, 0, 0);
}

void GigaDisplayRGB::writeByte(uint8_t subAddress, uint8_t data)
{
    Wire1.beginTransmission(0x50);
    Wire1.write(subAddress);
    Wire1.write(data);
    Wire1.endTransmission();
}

Longer term question: how much of the stuff in the ArduinoCore-zephyr overlays/config files maybe should be
migrated upstream, but that is beyond this scope.

[KurtE]

Quick question:

Current driver is pretty hard coded to RED/GREEN/BLUE and if I read it correctly it is assumed that the user
always has to pass in 3 colors in that order, but the output code may change the order of them as maybe the
channels to the hardware are not wired that way?

Now with the 97 it can handle up to 4 channels, I started off hard coding the 4th channel to WHITE for maybe
RGBW type LEDS.

But the LED Header file defines other colors as well:

#define LED_COLOR_ID_WHITE      0
#define LED_COLOR_ID_RED        1
#define LED_COLOR_ID_GREEN      2
#define LED_COLOR_ID_BLUE       3
#define LED_COLOR_ID_AMBER      4
#define LED_COLOR_ID_VIOLET     5
#define LED_COLOR_ID_YELLOW     6
#define LED_COLOR_ID_IR         7
#define LED_COLOR_ID_MAX        8

There feels like there should be way to specify different colors? And maybe mappings?
Ok to ignore for now?

Also is there any reason things are hard coded to have to set all of the LEDS each time? That is should it be
allowed to only set the Green LED, by passing in index 1, count 1?
That is can there be a few different pieces of code that maybe change a state of one LED. Sort of along the line that
GIGA has similar like LED (minus driver)

And each of the three colors are often driven by different places in the code base.

Thanks

[KurtE]

Sorry, another quick question: Is there any Boards other than the Nicla Sense ME that uses is31fl3194?
(I don't have one of those to test that I did not break it)

[KurtE]

Pushed up first part of renaming. Note I did not yet do the KConfig... file was having issues with it not building
Will look at again, but should not hold up to part 2...

Now for Part 2:

@simonguinot food for thought in your review: given that it's the user's responsibility to properly map colors, what do we gain by limiting to either [1]+[1]+[1] or [3]? Why not allow [1]+[1]? IMO you should be free to do what you prefer in DT, as long as you don't define more LEDs than channels... 😅 or am I missing something?

I am assuming from this conversation along this lines, For example with 97 with 4 channels, I should be able to:
have something like:

    is31fl3197@50 {
      compatible = "issi,is31fl3194";
      reg = <0x50>;

      led_0 {
        label = "RGB LED";
        color-mapping =
          <LED_COLOR_ID_RED>,
          <LED_COLOR_ID_GREEN>,
          <LED_COLOR_ID_BLUE>;
      };
      led_1 {
        label = "Other LED";
        color-mapping = <LED_COLOR_ID_WHITE>;
      };
    };

Assumption of the above: led_0 uses channels 0-2 and led_1 uses channel 3.
Driver code could care less about the actual colors, other than knowing how many channels to reserve.

If I understand correctly:

ret = led_set_color(led, 0, 3, &(color_sequence[i].r));
the code will set channel 0 to color_sequence[0] 1 to [1] and 2[2];

If I call it like:
ret = led_set_color(led, 1, 1, &color));
It will set channel 3 on the board to the color specified.

And if implemented, should also be able to do:
led_set_channel(led, 3, color);

To set the last LED (WHITE)

Is this reasonably correct?
Thanks

[KurtE]

@pillo79 @mjs513 @simonguinot - I split this up into multiple commits.
I also redid the color mapping... i.e. there is none.

I have the GIGA display shield working with it.
The Example: I updated to look through the channels, to figure out which was the R and G and B,
And then the sample remaps the colors.

[KurtE]

Please retry analysis of this Pull-Request directly on SonarQube Cloud

how?

[pillo79]

Quite a better shape, thanks for the effort! 👍

Note that commit ' drivers: led: is31fl319x - redo color to channel ' introduces changes to the sample README.rst that are later removed and then re-added in the following commits. 👀 Ideally, at that stage there should be no mention of other chips yet.

[pillo79]

Missed the Kconfig rename:CONFIG_IS31FL3194 -> CONFIG_IS31FL319X in the generalization commit. Should edit CMakeLists, Kconfig and Kconfig.is31fl314x (renamed from '94).

[KurtE]

Thanks,

When I was refactoring, I tried renaming the CONFIG file as mentioned, but at that time it would not build,
I got some strange error about missing some node or the like, when I did not rename the config file it worked.
Maybe something else was going wrong.

I will try again.

[KurtE]

@pillo79 - Tried again and I currently get a build error on the sample sketch:

.c.obj -MF CMakeFiles\app.dir\src\main.c.obj.d -o CMakeFiles/app.dir/src/main.c.obj -c D:/zephyrproject/zephyr/samples/drivers/led/is31fl319x/src/main.c
←[01m←[KD:/zephyrproject/zephyr/samples/drivers/led/is31fl319x/src/main.c:←[m←[K In function '←[01m←[Kmain←[m←[K':
←[01m←[KD:/zephyrproject/zephyr/samples/drivers/led/is31fl319x/src/main.c:58:30:←[m←[K ←[01;31m←[Kerror: ←[m←[K'←[01m←[Kled_dev←[m←[K' undeclared (first use in this function)
   58 |         if (!device_is_ready(←[01;31m←[Kled_dev←[m←[K)) {
      |                              ←[01;31m←[K^~~~~~~←[m←[K
←[01m←[KD:/zephyrproject/zephyr/samples/drivers/led/is31fl319x/src/main.c:58:30:←[m←[K ←[01;36m←[Knote: ←[m←[Keach undeclared identifier is reported only once for each function it appears in
[

My updated Kconfig.is31fl319x

# Copyright (c) 2024 Arduino SA
# SPDX-License-Identifier: Apache-2.0

config IS31FL319X
	bool "IS31FL319X LED driver"
	default y
	depends on DT_HAS_ISSI_IS31FL3194_ENABLED || DT_HAS_ISSI_IS31FL3197_ENABLED
	select I2C
	help
	  Enable LED driver for Lumissil Microsystems (a division of ISSI)
	  IS31FL3194. This chip supports one RGB LED or 3 independent LEDs.
	  IS31FL3197. This chip supports 4 LEDs.

The problem is that the defines like: CONFIG_IS31FL3194 and CONFIG_IS31FL3197 are not longer defined.
Instead:
#define CONFIG_IS31FL319X
Thoughts?

EDIT: Could probably check using ones like:

#define CONFIG_DT_HAS_ISSI_IS31FL3194_ENABLED 1

EDIT2:

changed the main.c to check these instead and now fails with:
c && C:\WINDOWS\system32\cmd.exe /C "cd /D D:\zephyrproject\zephyr\samples\drivers\led\is31fl319x\build\zephyr && "C:\Program Files\CMake\bin\cmake.exe" -E true""
d:/users/kurte/downloads/zephyr-sdk-0.16.8/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/12.2.0/../../../../arm-zephyr-eabi/bin/ld.bfd.exe: app/libapp.a(main.c.obj):D:/zephyrproject/zephyr/samples/drivers/led/is31fl319x/src/main.c:37: undefined reference to `__device_dts_ord_86'
collect2.exe: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
←[91mFATAL ERROR: command exited with status 1: 'C:\Program Files\CMake\bin\cmake.EXE' --build 'D:\zephyrproject\zephyr\samples\drivers\led\is31fl319x\build'
←

[pillo79]

Nit: you could wrap the #defines above as well, to make sure you catch any stray uses. Same thing for 97 below.

[pillo79]

Do not use magic numbers in code. Please consider the solution of keeping track of the expected chip in the static config and testing for that instead.

[KurtE]

Can do, I sort of resisted that earlier as have not looked through other chips in same family to know if these
differences are unique or similar for others as well. The other way I thought about doing it was:
add new item to config structure, like: uint8_t features and have either defines or enum with bit flags.
Like:
#define FEATURE_ID_IS_ADDR 0x01
#define FEATURE_SET_CURRENT 0x02

And have the enumerator at end for each chip pass in the appropriate flags.
Worth doing? Or wait until (if) other chips are supported?

[pillo79]

typo: is31fl3197

[pillo79]

Instead of adding a 3197 file you can patch the Kconfig 3194 (that should be 319x after the generalization comit) to have a
depends on DT_HAS_ISSI_IS31FL3194_ENABLED || DT_HAS_ISSI_IS31FL3197_ENABLED

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud