| Message ID | 20200304061816.10056-2-acelan.kao@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/1,SRU,B,E,F,OEM-OSP1-B] UBUNTU: SAUCE: platform/x86: dell-uart-backlight: add get_display_mode command | expand |
This doesn't apply in 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@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? 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? 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@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Thadeu Lima de Souza Cascardo <cascardo@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@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@lists.ubuntu.com > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 09.03.20 07:23, AceLan Kao wrote: > Thadeu Lima de Souza Cascardo <cascardo@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. > Hi AceLan, Does it mean that this patch has a pre-req? If that's the case please always state it in the cover letter of the thread otherwise we cannot know there's an order with the patches sent. Thanks, Kleber >> >> 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@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@lists.ubuntu.com >>> https://lists.ubuntu.com/mailman/listinfo/kernel-team >
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); + } 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 {
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@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(-)