[PATCH 1/1][SRU][B][E][F][OEM-OSP1-B] UBUNTU: SAUCE: platform/x86: dell-uart-backlight: add get_display_mode command
Kleber Souza
kleber.souza at canonical.com
Mon Mar 9 08:42:04 UTC 2020
On 09.03.20 07:23, AceLan Kao wrote:
> 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.
>
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 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