Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ili9341: mipi-dbi-mode setable through DT #78321

Closed
wants to merge 1 commit into from

Conversation

ts4ling
Copy link
Contributor

@ts4ling ts4ling commented Sep 12, 2024

Currently ili9xxx only supports SPI, but there is also a 8 and 16-bit interface with different modes, see MIPI-DBI driver APIs. STM calls it flexible memory controller, other vendors will have a similar interface.

My PR allows to set different modes that must be supported by the parent device. Regarding STM it's mipi_dbi_stm32_fmc.c, that checks for MIPI_DBI_MODE_8080_BUS_16_BIT. My fix allows to set the mode accordingly and the configuration check passes.

To not break any existing DTs I made the mode setting optional.

	ili9341: lcd-panel@0 {
		compatible = "ilitek,ili9341";
		reg = <0>;
		pixel-format = <ILI9XXX_PIXEL_FORMAT_RGB565>;
		mipi-mode = <MIPI_DBI_MODE_8080_BUS_16_BIT>;
		mipi-max-frequency = <DT_FREQ_M(20)>;
// (..)
	};

@ts4ling
Copy link
Contributor Author

ts4ling commented Sep 12, 2024

ili9341_compliance_check_failed

I'd solve this, but those error texts don't help.

@danieldegrasse
Copy link
Collaborator

I'd solve this, but those error texts don't help.

Indeed, those don't look so helpful. Not sure why you can't see it but this looks like the issue:

 Devicetree Bindings:'required: false' is redundant, please remove
File:dts/bindings/display/ilitek,ili9xxx-common.yaml
Line:40

@danieldegrasse danieldegrasse self-assigned this Sep 13, 2024
@@ -35,3 +35,7 @@ properties:
description:
Display inversion mode. Every bit is inverted from the frame memory to
the display.

mipi-mode:
Copy link
Collaborator

@danieldegrasse danieldegrasse Sep 13, 2024

Choose a reason for hiding this comment

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

You don't need to add this- the property already exists in mipi-dbi-device.yaml:

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to reformat the #define block (either for personal preference or to get clang-format to stop complaining), that is ok, but please make one commit that performs the reformat and another performing the functional change. Mixing formatting changes with functional ones in a single commit makes the git blame much harder to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to reformat the file, but it also affected the look-up-table in the beginning

const uint8_t ili9xxx_rgb_lut[] = {
	0, 2, 4, 6,
	8, 10, 12, 14,
	16, 18, 20, 22,
	24, 26, 28, 30,

which is more readable this way. git add file.c -p joined the LUT with the next function and turned it into too much work to make "right". So I undid the clang-format and pushed my initial change again.

@ts4ling
Copy link
Contributor Author

ts4ling commented Sep 23, 2024

Since my intended changes were also included in PR-78453, this PR is obsolete.

@ts4ling ts4ling closed this Sep 23, 2024
@ts4ling ts4ling deleted the display/ili9xx branch September 23, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants