[PATCH 1/1][SRU][M] UBUNTU: SAUCE: platform/x86: dell-uart-backlight: add small delay after write command

[AceLan Kao]

Andrea Righi <andrea.righi at canonical.com> 於 2023年9月28日 週四 上午3:13寫道：
>
> On Thu, Sep 14, 2023 at 02:00:13PM +0200, Andrea Righi wrote:
> > On Thu, Sep 14, 2023 at 06:13:41PM +0800, AceLan Kao wrote:
> > > From: "Chia-Lin Kao (AceLan)" <acelan.kao at canonical.com>
> > >
> > > BugLink: https://bugs.launchpad.net/bug/2035299
> > >
> > > After applying fbf84fb368923 ("UBUNTU: SAUCE: platform/x86:
> > > dell-uart-backlight: replace chars_in_buffer() with flush_chars()"), it
> > > seems that the read() command may fail to receive a response, even when
> > > increasing the retry times. It never gets a response when it fails.
> > >
> > > To fix this, try adding a small delay after the write() function as a workaround.
> >
> > I'm wondering if there's a better way to determine when the "write"
> > actually completed, so that we can safely issue a read without doing the
> > msleep() that is often a bit unpredictable.
> >
> > If msleep() is really the only way to do it, why don't we put that at
> > the end of dell_uart_write()? I suppose any write would be affected by
> > this issue potentially...
> >
> > Thanks,
> > -Andrea
>
> Any opinion/update on this? This patch is still unapplied.
Ah, sorry, I missed this thread.

The issue is kind of weird that doing more retry in the
dell_uart_read() doesn't work,
i've tried adjusting the retry mdelay() and the retry times in the
read function, but it doesn't help.
The only workaround is to make a short sleep after write().

I don't think this is the right way to fix the issue and on old
platforms we don't have to do this,
so I don't want to add the short sleep in the write function.
Move the sleep() into write function makes the code cleaner, but I
want it more verbose to remind me
there is a sleep between write and read, and not to hide the
workaround in the write function.
So that, maybe someday I can see the verbose code and check if there
is a better solution with the new kernel.

>
> Thanks,
> -Andrea
>
> >
> > >
> > > Fixes: fbf84fb368923 ("UBUNTU: SAUCE: platform/x86: dell-uart-backlight: replace chars_in_buffer() with flush_chars()")
> > > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao at canonical.com>
> > > ---
> > >  drivers/platform/x86/dell/dell-uart-backlight.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
> > > index 120701e5b8b13..c489f7997767f 100644
> > > --- a/drivers/platform/x86/dell/dell-uart-backlight.c
> > > +++ b/drivers/platform/x86/dell/dell-uart-backlight.c
> > > @@ -248,6 +248,7 @@ static int dell_uart_set_bl_power(struct backlight_device *bd, int power)
> > >     }
> > >
> > >     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> > > +   msleep(1);
> > >     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> > >
> > >     mutex_unlock(&dell_pdata->brightness_mutex);
> > > @@ -275,6 +276,7 @@ static int dell_uart_get_brightness(struct backlight_device *bd)
> > >     }
> > >
> > >     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> > > +   msleep(1);
> > >     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> > >
> > >     mutex_unlock(&dell_pdata->brightness_mutex);
> > > @@ -304,6 +306,7 @@ static int dell_uart_update_status(struct backlight_device *bd)
> > >     }
> > >
> > >     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> > > +   msleep(1);
> > >     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> > >
> > >     mutex_unlock(&dell_pdata->brightness_mutex);
> > > @@ -330,6 +333,7 @@ static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata)
> > >     }
> > >
> > >     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> > > +   msleep(1);
> > >     while (retry-- > 0) {
> > >             /* first byte is data length */
> > >             dell_uart_read(uart, bl_cmd->ret, 1);
> > > @@ -371,6 +375,7 @@ static int dell_uart_get_display_mode(struct dell_uart_backlight *dell_pdata)
> > >             }
> > >
> > >             dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> > > +           msleep(1);
> > >             rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> > >
> > >             mutex_unlock(&dell_pdata->brightness_mutex);
> > > --
> > > 2.34.1
> > >
> > >
> > > --
> > > kernel-team mailing list
> > > kernel-team at lists.ubuntu.com
> > > https://lists.ubuntu.com/mailman/listinfo/kernel-team