drivers/lcd: Add ioctl to get LCD panel ID#7103
drivers/lcd: Add ioctl to get LCD panel ID#7103seokhun-eom24 wants to merge 2 commits intoSamsung:masterfrom
Conversation
| }; | ||
|
|
||
| static struct mipi_lcd_dev_s g_lcdcdev; | ||
| static bool g_panelid_initialized = false; |
There was a problem hiding this comment.
we can use private struct variable instead of global.?
There was a problem hiding this comment.
Or, we can add it into get_lcdinfo.
There was a problem hiding this comment.
I modified it to use a member variable of mipi_lcd_dev_s instead of a global variable.
I separated get_lcdinfo and created a separate displayid because lcdinfo uses a static CONFIG value, while displayid dynamically retrieves the ID of the connected display.
| }; | ||
|
|
||
| static struct mipi_lcd_dev_s g_lcdcdev; | ||
| static bool g_panelid_initialized = false; |
There was a problem hiding this comment.
Or, we can add it into get_lcdinfo.
| FAR struct mipi_lcd_dev_s *priv = (FAR struct mipi_lcd_dev_s *)dev; | ||
| int status; | ||
| lcd_mode_t original_mode; | ||
| lcm_setting_table_t read_id_cmd = {0x04, 0, {0x00}}; |
There was a problem hiding this comment.
is This command DSC?
If it is specific command, we can wrapping this command to configure or new lcd sepecific api
There was a problem hiding this comment.
Yes it's DSC.
So I tired to implement mipi_dsi_dcs_get_display_id function in mipi_dsi_device.c like below.
int mipi_dsi_dcs_get_display_id(FAR struct mipi_dsi_device *device, FAR uint32_t *displayid)
{
int ret;
uint8_t rxbuf[3];
uint8_t length = sizeof(rxbuf) / sizeof(rxbuf[0]);
ret = mipi_dsi_set_maximum_return_packet_size(device, length);
if (ret < 0) {
return ret;
}
ret = mipi_dsi_generic_read(device, MIPI_DCS_GET_DISPLAY_ID, 0, rxbuf, length);
if (ret < 0) {
return ret;
}
*displayid = (rxbuf[0] << 16) | (rxbuf[1] << 8) | rxbuf[2];
return OK;
}But we have issue with this approach.
In mipi_lcd.c command is stored in msg.channel like msg.channel = cmd;.
In mipi_dsi_device.c command is stored in tx_buf like msg.tx_buf = &cmd;.
In BSP code, we assume cmd is stored in msg.channel and also stored in packet.header[0] use that value in amebasmart_mipi_transfer.
But mipi_dsi_device.c's approach is standard.
So, before we wrapping this command with dcs function like mipi_dsi_dcs_get_display_id, we should refactor mipi_lcd.c and bsp code to follow dcs command format.
ee6b0c4 to
0839329
Compare
| priv->displayid = (rxbuf[0] << 16) | (rxbuf[1] << 8) | rxbuf[2]; | ||
| *displayid = priv->displayid; | ||
| priv->displayid_initialized = true; | ||
| } |
There was a problem hiding this comment.
Is there no chance to fail the function read_response or set_return_packet_len?
There was a problem hiding this comment.
Of course, the read_response and set_return_packet_len functions can also fail.
In this case, the if case will not be executed, but the cleanup operation must still be performed in the same way, so no separate else handling was added.
| if (argc == 2) { | ||
| if (!strncmp(argv[1], "lcdinfo", 8)) { | ||
| return lcd_get_info(); | ||
| } else if (!strncmp(argv[1], "displayid", 8)) { |
There was a problem hiding this comment.
Third argument need to be 9.
| } else if (!strncmp(argv[1], "displayid", 8)) { | |
| } else if (!strncmp(argv[1], "displayid", 9)) { |
There was a problem hiding this comment.
Thank you for finding this, as I missed this part while changing the command from panelid to displayid.
I corrected it to 10 (length + 1) for an exact string match.
0839329 to
e075c4a
Compare
| priv->config = config; | ||
| priv->power = 0; | ||
| priv->lcdonoff = LCD_OFF; | ||
| priv->displayid = 0; |
There was a problem hiding this comment.
if displayid is used often, it's better to store id to ram.
But, i think, this id is not used often.
So, we don't need to store
There was a problem hiding this comment.
I agree with Mr Nam, display id will not be used frequently from ioctl so we might skip storing it and just read it directly from h/w. Do we know which scenario it will be used ?
There was a problem hiding this comment.
I updated to not store displayid in RAM.
We support different vendors for LCD. So when we have a hardware issue with an LCD panel, we want to know which vendor’s LCD it is. In this scenario we need to know the displayid.
729e131 to
68876c8
Compare
- Add LCDDEVIO_GETDISPLAYID ioctl command to read 24-bit display ID. - Implement getdisplayid in MIPI LCD driver. - Require CMD mode for display ID read operation. Signed-off-by: seokhun-eom <seokhun.eom@samsung.com>
- Add lcd_get_displayid() to retrieve and display the 24-bit LCD display ID. - Update show_usage() and main() to support the "displayid" command. [USAGE] ``` TASH>>lcd_test displayid TASH>>LCD Panel ID: 0x010000 ``` Signed-off-by: seokhun-eom <seokhun.eom@samsung.com>
68876c8 to
b4a3366
Compare
| @@ -65,6 +65,7 @@ | |||
| #define LCDDEVIO_INIT _LCDIOC(18) | |||
| #define LCDDEVIO_SETORIENTATION _LCDIOC(19) /* Arg: int */ | |||
| #define LCDDEVIO_GETLCDINFO _LCDIOC(20) /* Arg: struct lcd_info_s* Description: Get the static LCD characteristics such as width, height, size and DPI. */ | |||
There was a problem hiding this comment.
lcd init is already exist.
how about use it ?
| return ret; | ||
| } | ||
|
|
||
| static int lcd_get_displayid(void) |
There was a problem hiding this comment.
App should not take care of vendor of LCD, it must be abstracted.
Even if there are some types of LCD, framework & App should not change api of code. otherwise it is not good driver & framework model.
Hence I think you should not remove ioctl, and print out id of lcd when LCD initailized, that is enough.
|
I'll handle this PR after merge #7018 |
drivers/lcd
examples
[USAGE]