os/drivers/lcd: Prevent multiple LCD power on/off operations #7010

seokhun-eom24 · 2025-10-21T00:55:47Z

Add condition check to prevent multiple LCD power on/off operations.

pcs1265

LGTM

sunghan-chang · 2025-10-21T22:13:22Z

os/drivers/lcd/mipi_lcd.c

 *
 ****************************************************************************/

 static int lcd_power_on(FAR struct mipi_lcd_dev_s *priv)


Regardless of this PR, why do off and on APIs have different return type?

sunghan-chang · 2025-10-21T22:15:02Z

os/drivers/lcd/mipi_lcd.c

 *
 ****************************************************************************/

 static int lcd_power_on(FAR struct mipi_lcd_dev_s *priv)


Regardless of this PR, why do off and on APIs have different return type?

Don't you need to verify the priv for null?

Change the return type of both lcd_power_off and lcd_power_on to int, as power operations can fail when send_cmd or check_lcd_vendor_send_init_cmd fail.

Verify priv is not null by adding DEBUGASSERT to all functions to validate non-null arguments in the new commit.

abhinav-s235 · 2025-10-27T08:58:38Z

LGTM

ewoodev · 2025-10-28T02:55:21Z

os/drivers/lcd/mipi_lcd.c

+	DEBUGASSERT(priv);
+	if (priv->lcdonoff == LCD_OFF) {
+		lcddbg("ERROR: LCD already powered off\n");
+		return;


it's void return type

anjana348 · 2025-10-28T06:01:41Z

os/drivers/lcd/mipi_lcd.c

-	send_cmd(priv, display_off_cmd);
-	/* The power off must operate only when LCD is ON */
+	if (send_cmd(priv, display_off_cmd) != OK) {
+		lcddbg("ERROR: LCD power off failed\n");


Here there should be return

Taejun-Kwon · 2025-10-31T02:34:00Z

os/drivers/lcd/mipi_lcd.c

 	lcm_setting_table_t display_off_cmd = {0x28, 0, {0x00}};
-	send_cmd(priv, display_off_cmd);
-	/* The power off must operate only when LCD is ON */
+	if (send_cmd(priv, display_off_cmd) != OK) {


int ret = send_cmd(priv, display_off_cmd); if (ret != OK) { lcddbg("ERROR: LCD power off failed ret : %d\n", ret); }

Thanks for pointing that out.
I modified to log so that errno can be logged.

Taejun-Kwon · 2025-10-31T02:36:23Z

os/drivers/lcd/mipi_lcd.c

+		return ERROR;
+	}
+
+	priv->lcdonoff = LCD_OFF;


very minor, but usually change operation and then change state is more stable

It's a very minor issue, but I think it would be better to fix it.
Thank you.

Taejun-Kwon · 2025-10-31T02:37:09Z

os/drivers/lcd/mipi_lcd.c

+	if (priv->lcdonoff == LCD_ON) {
+		lcddbg("ERROR: LCD already powered on\n");
+		return ERROR;
+	}


after this, priv->lcdonoff need to be changed.

For now, we're setting priv->lcdonoff = LCD_ON when switching mipi video mode and render buffer data timing.
But this distributed state management has a risk, so I refactored it in #7018.

Taejun-Kwon · 2025-10-31T02:40:03Z

os/drivers/lcd/mipi_lcd.c

-	/* The power off must operate only when LCD is ON */
+	if (send_cmd(priv, display_off_cmd) != OK) {
+		lcddbg("ERROR: LCD power off failed\n");
+		return ERROR;


This will finally calls bsp function, and perhaps return value is one of value in errno.h
so you should return one of errno, not ERROR

I modified to return errno from send_cmd.

Taejun-Kwon · 2025-10-31T02:40:41Z

os/drivers/lcd/mipi_lcd.c

 		lcddbg("Powering down the LCD\n");
 		priv->config->backlight(power);
-		lcd_power_off(priv);
+		if (lcd_power_off(priv) != OK) {


same as above comments. debug message include result & type of error

Add condition check to prevent multiple LCD power on/off operations. Signed-off-by: seokhun-eom <seokhun.eom@samsung.com>

Add DEBUGASSERT checks for device pointers and validate other pointer parameters. This null checks for function that can be called by drivers. Signed-off-by: seokhun-eom <seokhun.eom@samsung.com>

pcs1265 approved these changes Oct 21, 2025

View reviewed changes

sunghan-chang reviewed Oct 21, 2025

View reviewed changes

seokhun-eom24 force-pushed the 251021-add-boundary-lcdonoff branch from 08fef23 to a106064 Compare October 27, 2025 06:02

seokhun-eom24 force-pushed the 251021-add-boundary-lcdonoff branch from a106064 to 071cbba Compare October 27, 2025 09:02

ewoodev reviewed Oct 28, 2025

View reviewed changes

anjana348 reviewed Oct 28, 2025

View reviewed changes

seokhun-eom24 force-pushed the 251021-add-boundary-lcdonoff branch from 071cbba to adad52c Compare October 30, 2025 11:17

Taejun-Kwon reviewed Oct 31, 2025

View reviewed changes

seokhun-eom24 force-pushed the 251021-add-boundary-lcdonoff branch 2 times, most recently from cf84f73 to 810e56c Compare October 31, 2025 04:56

seokhun-eom24 added 2 commits December 22, 2025 18:49

os/drivers/lcd: Prevent multiple LCD power on/off operations

c046bac

Add condition check to prevent multiple LCD power on/off operations. Signed-off-by: seokhun-eom <seokhun.eom@samsung.com>

os/drivers/lcd: Add null checks for mipi_lcd.c

79f1b5a

Add DEBUGASSERT checks for device pointers and validate other pointer parameters. This null checks for function that can be called by drivers. Signed-off-by: seokhun-eom <seokhun.eom@samsung.com>

seokhun-eom24 force-pushed the 251021-add-boundary-lcdonoff branch from 810e56c to 79f1b5a Compare December 22, 2025 12:09

sunghan-chang approved these changes Dec 26, 2025

View reviewed changes

sunghan-chang merged commit 58ea648 into Samsung:master Dec 26, 2025
12 checks passed

os/drivers/lcd: Prevent multiple LCD power on/off operations #7010

os/drivers/lcd: Prevent multiple LCD power on/off operations #7010

Conversation

seokhun-eom24 commented Oct 21, 2025

Uh oh!

pcs1265 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinav-s235 commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants