os/: st7701 LCD driver with MIPI #6148
Conversation
| rtl8730e_st7701_reset_pin(1); | ||
| DelayMs(10); | ||
| rtl8730e_st7701_reset_pin(0); | ||
| DelayMs(10); | ||
| rtl8730e_st7701_reset_pin(1); | ||
| DelayMs(120); |
There was a problem hiding this comment.
There are DelayMss between each controlling the reset pin.
Does they come from the data sheet of ST7701?
There was a problem hiding this comment.
Yes sir, it's from the data-sheet of st7701. Page number: 54
The reset time should be longer than 9ms. And the maximum time for reset is 120ms.
|
@namanjain7 You already implement mipi lowerhalf for amebasmart. Right? @zhongnuo-tang Could you review the first commit for RTL8730e? |
|
Since there are lots of files in one PR, so I have fixed the comments and committed to this PR which added new commits. Please ignore the number of commits, as I will raise a new PR once all the comments are fixed and the code is ready to be pushed. |
|
Hi @namanjain7 , since this PR is verified LCD working MIPI, thus could you leave simple comment regarding performance for different settings? |
| return 0; | ||
| } | ||
|
|
||
| static int amebasmart_mipi_transfer(FAR struct mipi_dsi_host *dsi_host, FAR const struct mipi_dsi_msg *msg) |
There was a problem hiding this comment.
There are 2 modes under MIPI DSI, one is video mode and one is command mode. During initialization stage, we are in reset status which is command mode, and we will send in a table of command list for configuring the LCD. But, if we need to change some configuration during runtime (ie. sending DCS command), I think we should check current mode, if we are under video mode, then we should toggle the state before sending a command, else it might not work.
So, we should add a checker for current DSI mode, and if it's video mode, we should invoke mipidsi_mode_switch(DISABLE); before amebasmart_mipidsi_send_cmd, and mipidsi_mode_switch(ENABLE); after sending command.
There was a problem hiding this comment.
ok, will add the check,
I have a question: Will there be any case that during runtime we will change the LCD configuration. As far as I know, LCD configuration should required to set in bootup. Why anyone will change the configuration when it's running?
There was a problem hiding this comment.
It depends on application usage, but I believe more request will come in along the way, thus we should get prepared here.
From my understanding, there is already a scenario which is when integrating with PM module, we will have to set backlight dimming according to different state (ie. STANDBY state), and that requires sending DCS command during runtime. (ie. This is just one of the example use case, it should not be dependent to just PM module)
In general, this API should be applicable for both initialisation and runtime for configuring LCD, instead of just setting it once during bootup sequence.
There was a problem hiding this comment.
Hello, I tried to switch the mode before and after the transfer function. I observed that it requires some sleep after mode switch. If there is no sleep, then there is a crash. So, we would need to apply sleep twice for both mode switch.
So, call is like:
modeswitch from video -> cmd
some sleep
transfer
modeswitch from cmd -> video
return
I dont' get it why mode switching requires sleep. And if it's required to apply sleep then, would it be good idea to apply sleep in transfer?
There was a problem hiding this comment.
Hello @namanjain7 , I'm not sure why is there a crash for mode switch. Could you attach the crash log via email or some other platforms?
Also, other than mode switch, I think we should also do a
MIPI_DSI_INT_Config(MIPIx, DISABLE, ENABLE, FALSE); to enable the interrupt for command mode.
And do a MIPI_DSI_INT_Config(MIPIx, DISABLE, DISABLE, FALSE); after sending command
There was a problem hiding this comment.
Where can I share the log? I don't have your email...
code:
static int amebasmart_mipi_transfer(FAR struct mipi_dsi_host *dsi_host, FAR const struct mipi_dsi_msg *msg)
{
FAR struct amebasmart_mipi_dsi_host_s *priv = (FAR struct amebasmart_mipi_dsi_host_s *)dsi_host;
struct mipi_dsi_packet packet;
int count = 0;
if(msg->type == MIPI_DSI_END_OF_TRANSMISSION){
MIPI_DSI_INT_Config(g_dsi_host.MIPIx, DISABLE, DISABLE, FALSE);
return OK;
}
mipidsi_mode_switch(false); <------------------
int ret = mipi_dsi_create_packet(&packet, msg);
if (ret != OK) {
return ret;
}
send_cmd_done = 0;
if (mipi_dsi_packet_format_is_short(msg->type)) {
if (packet.header[1] == 0) {
amebasmart_mipidsi_send_cmd(priv->MIPIx, packet.header[0], 0, NULL);
} else {
amebasmart_mipidsi_send_cmd(priv->MIPIx, packet.header[0], 1, packet.header + 1);
}
} else {
amebasmart_mipidsi_send_cmd(priv->MIPIx, packet.header[0], packet.payload_length, packet.payload);
}
while(send_cmd_done != 1){
DelayMs(1);
}
mipidsi_mode_switch(true); <-----------------
return OK;
}
There was a problem hiding this comment.
Hi @namanjain7 , could you please revert this request first? I mean you can ignore the runtime sending DCS command, just leave it as the initial working codes.
I will add the necessary changes together with PM with MIPI in another PR.
Sorry for inconvenience caused.
| @@ -257,4 +257,5 @@ int lcd_test_main(int argc, char *argv[]) | |||
| sleep(1); | |||
| count++; | |||
| } | |||
| return 0; | |||
There was a problem hiding this comment.
Added this due to warning message during build.
| run.data = &spi_data; | ||
| for (i = 0; i <= (NOPIXELS * 2); i += 2) { | ||
| run.data = spi_data; | ||
| for (i = 0; i < (NOPIXELS * 2); i += 2) { |
There was a problem hiding this comment.
Fixed the logical error. It will go out of bound of array.
| amebasmart_mipidsi_send_cmd(priv->MIPIx, packet.header[0], packet.payload_length, packet.payload); | ||
| } | ||
|
|
||
| while (!ameabsmart_is_transfer_completed()) { |
There was a problem hiding this comment.
This seems having much delay (ie. 1ms), are we supposed to wait for such long time? Just polling is not enough?
The intention of adding delay here seems that we are sending a table of command list during initialization stage. But if we are just sending command individually, perhaps the delay is unnecessary.
There was a problem hiding this comment.
I have removed the delay and changed the while loop to this:
I have verified this and it's working.
while(true){
if(send_cmd_done == 1){
break;
}
count++;
if(count >=20){
dbg("Error: amebasmart mipi send cmd timeout\n");
}
}
There was a problem hiding this comment.
I have 2 questions:
- How do you get the value of 20?
- What if send_cmd_done never gets updated? Maybe we should add some error handling instead of just printing timeout logs (ie. return false?)
There was a problem hiding this comment.
Hello, I have changed this code. It seems that it's not possible to exit from while loop without adding sleep. It gets stuck in the while loop. Not sure about the reason. I have added the 1ms delay. It's the lowest I could go when I ran the code in hardware.
The reason it was working before is that I have added some debug logs before the while loop. It seems that debug log results in some delay. When I remove the debug log just before the while loop, then code gets stuck in the while loop.
The new code is working properly , ... I verified many times.
os/drivers/lcd/st7701.c
Outdated
| usleep(table[send_cmd_idx_s].count * 1000); | ||
| break; | ||
| case REGFLAG_END_OF_TABLE: | ||
| printf("LPTX CMD Send Done\n"); |
There was a problem hiding this comment.
Is this log necessary? Even it is, why not using lcdvdbg?
There was a problem hiding this comment.
I think from information perspective, it would be good idea to print when the last command has been transferred.
It should help debug in future that the LCD initialization sequence is completed.
| return -EINVAL; | ||
| } | ||
| if (msg == NULL) { | ||
| printf("msg is NULL \n"); |
There was a problem hiding this comment.
MIPI seems to be an individual driver which is tightly coupled with LCD, maybe you want to define another logging type for it. etc: mipidbg, mipivdbg? Please find os/include/debug.h.
I'm not very sure, maybe @sunghan-chang could provide better comment on this.
There was a problem hiding this comment.
Let's use the printf in an app, let's use xxdbg in the kernel. Please find my previous comment.
…move unwanted MIPI_DSI_INIT
RGB565 : 357.398 FPS |
| priv->MIPI_InitStruct->MIPI_BllpLen = priv->MIPI_InitStruct->MIPI_LineTime / 2; | ||
|
|
||
| if (MIPI_DSI_HSA + MIPI_DSI_HBP + dsi_host->config.XPixels + MIPI_DSI_HFP < (512 + MIPI_DSI_RTNI * 16)) { | ||
| dbg("!!ERROR!!, LCM NOT SUPPORT\n"); |
There was a problem hiding this comment.
We should use xxdbg, not just dbg.
We use xxxdbg to control by a module.
I mean when we need to check scheduler logs, we enable CONFIG_DEBUG, CONFIG_DEBUG_SCHED.
And enable necessary log level, error, warning or verbose.
So, specific module should use specific debug message, not general debug message (dbg).
Please define mipi debug messages, in debug.h, Kconfig.debug.
xxdbg is an error message with CONFIG_DEBUG_XX_ERROR
xxwdbg is a warning with CONFIG_DEBUG_XX_WARN
xxvdbg is an information with CONFIG_DEBUG_XX_INFO
| return -EINVAL; | ||
| } | ||
| if (msg == NULL) { | ||
| printf("msg is NULL \n"); |
There was a problem hiding this comment.
Let's use the printf in an app, let's use xxdbg in the kernel. Please find my previous comment.
|
|
||
| DEBUGASSERT(host != NULL && name != NULL); | ||
|
|
||
| if (channel > 3) { |
There was a problem hiding this comment.
What does 3 mean? Is this an amebasmart specific value?
| start >> 8, | ||
| start & 0xff, | ||
| end >> 8, | ||
| end & 0xff |
There was a problem hiding this comment.
Could you align them? Same for other payloads
|
Hi @namanjain7 , may I know the status of this PR, could you help to clear the remaining comments? FYI, the next step is to integrate PM with MIPI LCD, and that will take time for us to make relevant changes. Thus, this PR is mandatory to be merged asap. |
Hello sir, yes I am clearing the remaining comments. I have fixed almost all of them except in one comment where I am getting some error when adding the changes in debug.h file. I am fixing that and will update the PR by tomorrow for sure. |
No description provided.