drivers: display: ili9xxx: Add support for ili9342c display controller

[ExtremeGTX]

Driver for ili9342c display controller
Tested on m5stack Core Basic using sample/subsys/display/lvgl

[diegoherranz]

I was about to submit a nearly identical pull request. I'll try to review yours and provide comments.
Thanks!

[diegoherranz]

I've had a look and provided some comments. Let me know if you have any questions.
Thanks!

[ExtremeGTX]

@diegoherranz Thank you for the review comments 👍, I have fixed all of them, and also retested orientation modes.
I don't like the use of #ifdef in there, but I don't have the other ili9xxx chips to play around with these values.

[diegoherranz]

@diegoherranz Thank you for the review comments 👍, I have fixed all of them, and also retested orientation modes. I don't like the use of #ifdef in there, but I don't have the other ili9xxx chips to play around with these values.

Thanks for making these changes. I haven't forgotten about this one, simply I haven't had time to re-review it lately. I will as soon as possible.

[ExtremeGTX]

@galak @jfischer-no since there is always a time difference between me and you, could you please Approve and merge?
When I force push to rebase on the latest main, Github dismisses the review.

[ExtremeGTX]

@carlescufi @jfischer-no @MaureenHelm what is the way forward ?

[diegoherranz]

Just one comment. I tested this again today after the rebase and there are fewer warnings than last time I tested it, but it's still complaining about the unused quirks (those for the display controller models not in use). In my case, I'm using ili9342c, so it complains about the others:

zephyr/drivers/display/display_ili9xxx.c:435:68: warning: 'ili9488_quirks' defined but not used [-Wunused-const-variable=]
  435 | static const struct ili9xxx_quirks ili9340_quirks, ili9341_quirks, ili9488_quirks = {
      |                                                                    ^~~~~~~~~~~~~~
zephyr/drivers/display/display_ili9xxx.c:435:52: warning: 'ili9341_quirks' defined but not used [-Wunused-const-variable=]
  435 | static const struct ili9xxx_quirks ili9340_quirks, ili9341_quirks, ili9488_quirks = {
      |                                                    ^~~~~~~~~~~~~~
zephyr/drivers/display/display_ili9xxx.c:435:36: warning: 'ili9340_quirks' defined but not used [-Wunused-const-variable=]
  435 | static const struct ili9xxx_quirks ili9340_quirks, ili9341_quirks, ili9488_quirks = {

The code in display_ili9xxx.c is:

static const struct ili9xxx_quirks ili9340_quirks, ili9341_quirks, ili9488_quirks = {
	.cmd_set = CMD_SET_1,
};

static const struct ili9xxx_quirks ili9342c_quirks = {
	.cmd_set = CMD_SET_2,
};

Should we do something like?

#ifdef CONFIG_ILI9340
static const struct ili9xxx_quirks ili9340_quirks = {
	.cmd_set = CMD_SET_1,
};
#endif

#ifdef CONFIG_ILI9341
static const struct ili9xxx_quirks ili9341_quirks = {
	.cmd_set = CMD_SET_1,
};
#endif

#ifdef CONFIG_ILI9342C
static const struct ili9xxx_quirks ili9342c_quirks = {
	.cmd_set = CMD_SET_2,
};
#endif

#ifdef CONFIG_ILI9488
static const struct ili9xxx_quirks ili9488_quirks = {
	.cmd_set = CMD_SET_1,
};
#endif

Or even create one single structure and assign the .cmd_set using the defines? Probably finding a better name for ili9xxx_quirks_in_use:

static const struct ili9xxx_quirks ili9xxx_quirks_in_use = {
        #if defined(CONFIG_ILI9340) || defined(CONFIG_ILI9341) || defined(CONFIG_ILI9488)
 	.cmd_set = CMD_SET_1,
        #elif defined(CONFIG_ILI9342C)
        .cmd_set = CMD_SET_2,
        #endif
};

[...]
									       \
	static const struct ili9xxx_config ili9xxx_config_##n = {              \
		.quirks = &ili9xxx_quirks_in_use,  
[...]

Really keen on seeing this merged :)

Thanks!

[MaureenHelm]

Just one comment. I tested this again today after the rebase and there are fewer warnings than last time I tested it, but it's still complaining about the unused quirks (those for the display controller models not in use)

Build warnings need to be addressed. Is there anything in-tree (i.e., sample or test) that builds this driver?

[MaureenHelm]

you can simplify with DT_INST_PROP

[ExtremeGTX]

ili9xxx drivers doesn't include DT_DRV_COMPAT value. so I chose to follow the other variants.

[MaureenHelm]

Defaults need justification in the description

[ExtremeGTX]

Defaults are the manufacturer defaults as mentioned in datasheet.

[ExtremeGTX]

@diegoherranz
I went with the first way so if a system has 2 displays with different controllers will be able to enable both driver instances.
The problem with the 2nd solution is when CONFIG_ILI9340 and CONFIG_ILI9342c are defined at the same time.

@MaureenHelm asking for including this driver in (sample/test) or maybe a board like other ili9xxx drivers.
So, Do you @diegoherranz have a devkit where we can add so it compiles this driver in CI ? right now there isn't any config in Zephyr tree which enables this driver.

[diegoherranz]

I went with the first way so if a system has 2 displays with different controllers will be able to enable both driver instances.
The problem with the 2nd solution is when CONFIG_ILI9340 and CONFIG_ILI9342c are defined at the same time.

Cool. It builds without warning to me now (and it works), using a ili9342c screen.

Do you @diegoherranz have a devkit where we can add so it compiles this driver in CI ? right now there isn't any config in Zephyr tree which enables this driver

I do but it is a proprietary/personal board. I don't mind sharing its board folder (DTS, defconfig, etc.), but not sure if that's OK to include in zephyr, not being a publicly available board. Just let me know.

Thanks!

[MaureenHelm]

Another alternative would be to add display to tests/drivers/build_all

addressed