[PATCH 1/1][SRU][B][E][F][OEM-OSP1-B] UBUNTU: SAUCE: platform/x86: dell-uart-backlight: add get_display_mode command
AceLan Kao
acelan.kao at canonical.com
Mon Mar 9 06:23:37 UTC 2020
Thadeu Lima de Souza Cascardo <cascardo at canonical.com> 於 2020年3月6日 週五 下午10:34寫道:
>
> This doesn't apply in bionic.
Ah, it should be the previous patch doesn't get applied on bionic.
>
> Also, see some inline comments.
>
> On Wed, Mar 04, 2020 at 02:18:16PM +0800, AceLan Kao wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1865402
> >
> > ODM asks us to use get_display_mode command to confirm the scalar's
> > behavior, and Windows use this command, too.
> > To align the behavior with Windows, remove get_scalar_status command and
> > replace it with get_display_mode.
> >
> > Signed-off-by: AceLan Kao <acelan.kao at canonical.com>
> > ---
> > drivers/platform/x86/dell-uart-backlight.c | 90 +++++++++++-----------
> > drivers/platform/x86/dell-uart-backlight.h | 7 +-
> > 2 files changed, 52 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
> > index 9b2b219f3b8f..aa08974900ea 100644
> > --- a/drivers/platform/x86/dell-uart-backlight.c
> > +++ b/drivers/platform/x86/dell-uart-backlight.c
> > @@ -57,18 +57,6 @@ static struct dell_uart_bl_cmd uart_cmd[] = {
> > .cmd = {0x6A, 0x06, 0x8F},
> > .tx_len = 3,
> > },
> > - /*
> > - * Get Scalar Status: Tool uses this command to check if scalar IC controls brightness.
> > - * Command: 0x6A 0x1F 0x8F (Length:3 Type: 0x0A, Cmd:0x1F Checksum:0x76)
> > - * Return data: 0x04 0x1F Data checksum
> > - * (Data = 0: scalar cannot adjust brightness, Data = 1: scalar can adjust brightness)
> > - */
> > - [DELL_UART_GET_SCALAR] = {
> > - .cmd = {0x6A,0x1F,0x76},
> > - .ret = {0x04,0x1F,0x00,0x00},
> > - .tx_len = 3,
> > - .rx_len = 4,
> > - },
> > /*
> > * Get Brightness level: Application uses this command for scaler to
> > * get brightness.
> > @@ -115,6 +103,21 @@ static struct dell_uart_bl_cmd uart_cmd[] = {
> > .tx_len = 4,
> > .rx_len = 3,
> > },
> > + /*
> > + * Get display mode: Application uses this command to get scaler
> > + * display mode.
> > + * Command: 0x6A 0x10 0x85 (Length:3 Type: 0x0A, Cmd:0x10)
> > + * Return data: 0x04 0x10 Data checksum
> > + * (Length:4 Cmd:0x10 Data: mode checksum: SUM
> > + * mode =0 if PC mode
> > + * mode =1 if AV(HDMI) mode
> > + */
> > + [DELL_UART_GET_DISPLAY_MODE] = {
> > + .cmd = {0x6A, 0x10, 0x85},
> > + .ret = {0x04, 0x10, 0x00, 0x00},
> > + .tx_len = 3,
> > + .rx_len = 4,
> > + },
> > };
> >
> > static const struct dmi_system_id dell_uart_backlight_alpha_platform[] __initconst = {
> > @@ -313,36 +316,6 @@ static int dell_uart_update_status(struct backlight_device *bd)
> > return 0;
> > }
> >
> > -static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
> > -{
> > - struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
> > - struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
> > - int rx_len;
> > - int status = 0, retry = 50;
> > -
> > - do {
> > - dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
> > -
> > - if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
> > - pr_debug("Failed to get mutex_lock");
> > - return 0;
> > - }
> > -
> > - dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> > - msleep(100);
> > - rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> > -
> > - mutex_unlock(&dell_pdata->brightness_mutex);
> > -
> > - dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
> > - } while (rx_len == 0 && --retry);
> > -
> > - if (rx_len == 4)
> > - status = (unsigned int)bl_cmd->ret[2];
> > -
> > - return status;
> > -}
> > -
> > static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata)
> > {
> > struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_FIRMWARE_VER];
> > @@ -382,6 +355,36 @@ static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata)
> > return rx_len;
> > }
> >
> > +static int dell_uart_get_display_mode(struct dell_uart_backlight *dell_pdata)
> > +{
> > + struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_DISPLAY_MODE];
> > + struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
> > + int rx_len;
> > + int status = 0, retry = 10;
> > +
> > + do {
> > + dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
> > +
> > + if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
> > + pr_debug("Failed to get mutex_lock");
> > + return 0;
> > + }
> > +
> > + dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> > + rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> > +
> > + mutex_unlock(&dell_pdata->brightness_mutex);
> > +
> > + dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
> > + mdelay(1000);
>
> Above, a msleep(100) was being used after the write, before the read. Isn't
> this necessary for this command?
No, it sometimes leads to issues.
We found that the response time from scalar vary. First we found that
it requires
more than 1 second to respond the first command, so we insert a delay
before read().
But recently(after BIOS updated or H/W reworked), we found it responds
the first command
within 100ms, so the delay make it miss the response.
>
> And now it's a mdelay(1000). One second doing some busy wait between retries.
> Can it really take like 10s before a response? If 1s is okay, an msleep(100)
> would be better here. And comparing this to bionic's version (well, really the
> other command), you only retry reads. Is retrying writes followed by reads
> okay? Or doesn't it make it more prone to failures?
Yes, you are right, I should use msleep() here. Will send v2 patch.
Only retry read was to fix the long response time issue. The scalar is
busy during booting up
and can't respond UART command quick enough. But later we found scalar
may miss our
command and never responds, so it'd be more reliable to re-send our
command, too.
The scalar becomes stable after around 12 seconds of the kernel time,
so to delay 1 second
per retry is intentional, it make it has a better change to wait the
scalar to be not so busy
and responds the command within 200ms(the retry timeout in dell_uart_read())
>
> Cascardo.
>
> > + } while (rx_len == 0 && --retry);
> > +
> > + if (rx_len == 4)
> > + status = ((unsigned int)bl_cmd->ret[2] == PC_MODE);
> > +
> > + return status;
> > +}
> > +
> > static const struct backlight_ops dell_uart_backlight_ops = {
> > .get_brightness = dell_uart_get_brightness,
> > .update_status = dell_uart_update_status,
> > @@ -429,13 +432,12 @@ static int dell_uart_bl_add(struct acpi_device *dev)
> > return -ENODEV;
> > }
> > }
> > - else if (!dell_uart_get_scalar_status(dell_pdata)) {
> > + else if (!dell_uart_get_display_mode(dell_pdata)) {
> > pr_debug("Scalar is not in charge of brightness adjustment.\n");
> > kzfree(dell_pdata);
> > return -ENODEV;
> > }
> > }
> > - dell_uart_show_firmware_ver(dell_pdata);
> >
> > memset(&props, 0, sizeof(struct backlight_properties));
> > props.type = BACKLIGHT_PLATFORM;
> > diff --git a/drivers/platform/x86/dell-uart-backlight.h b/drivers/platform/x86/dell-uart-backlight.h
> > index e4fe74fae6a8..5c732d362d4b 100644
> > --- a/drivers/platform/x86/dell-uart-backlight.h
> > +++ b/drivers/platform/x86/dell-uart-backlight.h
> > @@ -20,10 +20,15 @@
> >
> > enum {
> > DELL_UART_GET_FIRMWARE_VER,
> > - DELL_UART_GET_SCALAR,
> > DELL_UART_GET_BRIGHTNESS,
> > DELL_UART_SET_BRIGHTNESS,
> > DELL_UART_SET_BACKLIGHT_POWER,
> > + DELL_UART_GET_DISPLAY_MODE,
> > +};
> > +
> > +enum {
> > + PC_MODE,
> > + AV_MODE,
> > };
> >
> > struct dell_uart_bl_cmd {
> > --
> > 2.17.1
> >
> >
> > --
> > kernel-team mailing list
> > kernel-team at lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list