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

display: stm32: ltdc: minor fixes #75050

Closed

Conversation

ajarmouni-st
Copy link
Collaborator

@ajarmouni-st ajarmouni-st commented Jun 26, 2024

Renames the display-controller property of st,stm32-ltdc to panel-controller.

Clarifies why NULL checks are necessary for panel controller device inside blanking_on/off functions in LTDC driver,
& fixes return value when the panel controller's phandle is not passed to LTDC in devicetree.

Addresses #68105 (comment)

@ajarmouni-st ajarmouni-st added the platform: STM32 ST Micro STM32 label Jun 26, 2024
@ajarmouni-st ajarmouni-st marked this pull request as ready for review June 26, 2024 15:33
@zephyrbot zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Display labels Jun 26, 2024
erwango
erwango previously approved these changes Jun 27, 2024
decsny
decsny previously requested changes Jun 27, 2024
Copy link
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

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

will need to update a migration guide to change the DT binding property name

@zephyrbot zephyrbot added the Release Notes To be mentioned in the release notes label Jun 27, 2024
@zephyrbot zephyrbot requested review from aescolar, kartben and nashif June 27, 2024 15:06
@ajarmouni-st ajarmouni-st requested review from decsny and erwango June 27, 2024 15:08
@ajarmouni-st ajarmouni-st added this to the v3.7.0 milestone Jul 1, 2024
gautierg-st
gautierg-st previously approved these changes Jul 1, 2024
kartben
kartben previously approved these changes Jul 1, 2024
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

I would squash the commit updating the migration guide into the first commit as there is no reason to keep it separate IMO, but I will make this non-blocking (and doing the update shouldn't dismiss existing reviews anyway)

@ajarmouni-st ajarmouni-st added the DNM This PR should not be merged (Do Not Merge) label Jul 1, 2024
Copy link
Collaborator

@mathieuchopstm mathieuchopstm left a comment

Choose a reason for hiding this comment

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

LGTM besides the following:

drivers/display/display_stm32_ltdc.c Outdated Show resolved Hide resolved
drivers/display/display_stm32_ltdc.c Outdated Show resolved Hide resolved
drivers/display/display_stm32_ltdc.c Outdated Show resolved Hide resolved
drivers/display/display_stm32_ltdc.c Outdated Show resolved Hide resolved
display-controller is ambiguous. change it to panel-controller.
Update LTDC driver & migration guide.

Signed-off-by: Abderrahmane Jarmouni <abderrahmane.jarmouni-ext@st.com>
Blanking On/Off calls should not return 0 when there is no panel
controller to forward them to, instead they should return ENOSYS to signal
to the application that they were not actually executed.

Signed-off-by: Abderrahmane Jarmouni <abderrahmane.jarmouni-ext@st.com>
@ajarmouni-st ajarmouni-st dismissed stale reviews from kartben and gautierg-st via f8b5730 July 1, 2024 12:24
@ajarmouni-st ajarmouni-st added bug The issue is a bug, or the PR is fixing a bug and removed DNM This PR should not be merged (Do Not Merge) labels Jul 1, 2024
@GeorgeCGV
Copy link
Collaborator

@mathieuchopstm, @ajarmouni-st,

The device_is_ready already checks for null:

zephyr/kernel/device.c

Lines 142 to 153 in 9b9d455

bool z_impl_device_is_ready(const struct device *dev)
{
/*
* if an invalid device pointer is passed as argument, this call
* reports the `device` as not ready for usage.
*/
if (dev == NULL) {
return false;
}
return dev->state->initialized && (dev->state->init_res == 0U);
}

No need to add extra device null checks like if (panel_dev == NULL) ...

@ajarmouni-st
Copy link
Collaborator Author

The device_is_ready already checks for null:

No need to add extra device null checks like if (panel_dev == NULL) ...

@GeorgeCGV The idea here is to isolate the dev == NULL case from dev->state->initialized == false case.

@GeorgeCGV
Copy link
Collaborator

GeorgeCGV commented Jul 1, 2024

@ajarmouni-st if the goal is to allow people to control blanking manually in the app, then the stm32_ltdc_init could print an info about panel_controller being null and blanking calls could be way smaller with some macro magic (as that is user's intention). That could reduce the footprint a bit.

If the goal is to not allow that, then it would be better to fail early by replacing DEVICE_DT_GET_OR_NULL with DEVICE_DT_GET. Then the null checks are irrelevant.

But ok.

@ajarmouni-st
Copy link
Collaborator Author

it would be better to fail early by replacing DEVICE_DT_GET_OR_NULL with DEVICE_DT_GET

That would not be desirable because we don't know if the app will actually call a display API function that needs to be forwarded to the panel controller.

@ajarmouni-st
Copy link
Collaborator Author

ajarmouni-st commented Jul 1, 2024

blanking calls could be way smaller with some macro magic

@GeorgeCGV I am not sure I understand what you mean by this.

@GeorgeCGV
Copy link
Collaborator

it would be better to fail early by replacing DEVICE_DT_GET_OR_NULL with DEVICE_DT_GET

That would not be desirable because we don't know if the app will actually call a display API function that needs to be forwarded to the panel controller.

Some drivers call blanking on upon init, some don't. Same goes for blanking off.

blanking calls could be way smaller with some macro magic

@GeorgeCGV I am not sure I understand what you mean by this.

If LTDC's blanking control is intended to be optional, then it is possible to define blanking differently depending on the panel_controller value. That allows to save some bytes when panel_controller is not provided. But that is more into "optimization".

@ajarmouni-st
Copy link
Collaborator Author

ajarmouni-st commented Jul 1, 2024

@GeorgeCGV I don't know if this is relevant or not, but panel-controller is useful for redirecting in a transparent way the call of any display API function that cannot be performed by LTDC itself but it is implemented by the panel controller driver (like set_brightness in the case of otm8009a).

@ajarmouni-st
Copy link
Collaborator Author

@decsny Please take another look, thanks!

@ajarmouni-st ajarmouni-st requested a review from gautierg-st July 2, 2024 07:49
@ajarmouni-st
Copy link
Collaborator Author

@danieldegrasse could you help review this, thanks!

@ajarmouni-st ajarmouni-st dismissed decsny’s stale review July 5, 2024 07:48

Migration guide has been updated

Copy link
Collaborator

@jfischer-no jfischer-no left a comment

Choose a reason for hiding this comment

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

-	display-controller = <&ili9341>;
+	panel-controller = <&ili9341>;

Whether display-controller is ambiguous or not, it is not a bug.

@ajarmouni-st ajarmouni-st deleted the fix_stm32_ltdc branch July 5, 2024 13:59
@jfischer-no
Copy link
Collaborator

@ajarmouni-st But drivers: display: stm32_ltdc: fix return value is still valid, why not just drop the first commit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: Display bug The issue is a bug, or the PR is fixing a bug platform: STM32 ST Micro STM32 Release Notes To be mentioned in the release notes
Projects
None yet
9 participants