Hi,
On 5/13/24 5:07 PM, Ilpo Järvinen wrote:
> On Mon, 13 May 2024, Hans de Goede wrote:
>
>> Dell All In One (AIO) models released after 2017 use a backlight controller
>> board connected to an UART.
>>
>> In DSDT this uart port will be defined as:
>>
>> Name (_HID, "DELL0501")
>> Name (_CID, EisaId ("PNP0501")
>>
>> Instead of having a separate ACPI device with an UartSerialBusV2() resource
>> to model the backlight-controller, which would be the standard way to do
>> this.
>>
>> The acpi_quirk_skip_serdev_enumeration() has special handling for this
>> and it will make the serial port code create a serdev controller device
>> for the UART instead of a /dev/ttyS0 char-dev. It will also create
>> a dell-uart-backlight driver platform device for this driver to bind too.
>>
>> This new kernel module contains 2 drivers for this:
>>
>> 1. A simple platform driver which creates the actual serdev device
>> (with the serdev controller device as parent)
>>
>> 2. A serdev driver for the created serdev device which exports
>> the backlight functionality uses a standard backlight class device.
>>
>> Reported-by: Roman Bogoyev <roman@xxxxxxxxxxxxxxxxxxxx>
>> Tested-by: Roman Bogoyev <roman@xxxxxxxxxxxxxxxxxxxx>
>> Tested-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
>> Co-developed-by: AceLan Kao <acelan.kao@xxxxxxxxxxxxx>
>> Signed-off-by: AceLan Kao <acelan.kao@xxxxxxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> Changes in v3:
>> - Rework dell_uart_bl_receive() loop (based on suggestions from Ilpo)
>> - s/SOF/DELL_SOF/ to avoid future namespace conflicts
>>
>> Changes in v2:
>> - Address various review remarks from Ilpo and Andy
>> ---
>> drivers/platform/x86/dell/Kconfig | 15 +
>> drivers/platform/x86/dell/Makefile | 1 +
>> .../platform/x86/dell/dell-uart-backlight.c | 398 ++++++++++++++++++
>> 3 files changed, 414 insertions(+)
>> create mode 100644 drivers/platform/x86/dell/dell-uart-backlight.c
>>
>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>> index bd9f445974cc..195a8bf532cc 100644
>> --- a/drivers/platform/x86/dell/Kconfig
>> +++ b/drivers/platform/x86/dell/Kconfig
>> @@ -145,6 +145,21 @@ config DELL_SMO8800
>> To compile this driver as a module, choose M here: the module will
>> be called dell-smo8800.
>>
>> +config DELL_UART_BACKLIGHT
>> + tristate "Dell AIO UART Backlight driver"
>> + depends on ACPI
>> + depends on BACKLIGHT_CLASS_DEVICE
>> + depends on SERIAL_DEV_BUS
>> + help
>> + Say Y here if you want to support Dell AIO UART backlight interface.
>> + The Dell AIO machines released after 2017 come with a UART interface
>> + to communicate with the backlight scalar board. This driver creates
>> + a standard backlight interface and talks to the scalar board through
>> + UART to adjust the AIO screen brightness.
>> +
>> + To compile this driver as a module, choose M here: the module will
>> + be called dell_uart_backlight.
>> +
>> config DELL_WMI
>> tristate "Dell WMI notifications"
>> default m
>> diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
>> index 1b8942426622..8176a257d9c3 100644
>> --- a/drivers/platform/x86/dell/Makefile
>> +++ b/drivers/platform/x86/dell/Makefile
>> @@ -14,6 +14,7 @@ dell-smbios-objs := dell-smbios-base.o
>> dell-smbios-$(CONFIG_DELL_SMBIOS_WMI) += dell-smbios-wmi.o
>> dell-smbios-$(CONFIG_DELL_SMBIOS_SMM) += dell-smbios-smm.o
>> obj-$(CONFIG_DELL_SMO8800) += dell-smo8800.o
>> +obj-$(CONFIG_DELL_UART_BACKLIGHT) += dell-uart-backlight.o
>> obj-$(CONFIG_DELL_WMI) += dell-wmi.o
>> dell-wmi-objs := dell-wmi-base.o
>> dell-wmi-$(CONFIG_DELL_WMI_PRIVACY) += dell-wmi-privacy.o
>> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
>> new file mode 100644
>> index 000000000000..4e73fa035aca
>> --- /dev/null
>> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c
>> @@ -0,0 +1,398 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Dell AIO Serial Backlight Driver
>> + *
>> + * Copyright (C) 2024 Hans de Goede <hansg@xxxxxxxxxx>
>> + * Copyright (C) 2017 AceLan Kao <acelan.kao@xxxxxxxxxxxxx>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/backlight.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/serdev.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +#include <linux/wait.h>
>> +#include "../serdev_helpers.h"
>> +
>> +/* The backlight controller must respond within 1 second */
>> +#define DELL_BL_TIMEOUT msecs_to_jiffies(1000)
>> +#define DELL_BL_MAX_BRIGHTNESS 100
>> +
>> +/* Defines for the commands send to the controller */
>> +
>> +/* 1st byte Start Of Frame 3 MSB bits: cmd-len + 01010 SOF marker */
>> +#define DELL_SOF(len) (((len) << 5) | 0x0a)
>> +#define GET_CMD_LEN 3
>> +#define SET_CMD_LEN 4
>> +
>> +/* 2nd byte command */
>> +#define CMD_GET_VERSION 0x06
>> +#define CMD_SET_BRIGHTNESS 0x0b
>> +#define CMD_GET_BRIGHTNESS 0x0c
>> +#define CMD_SET_BL_POWER 0x0e
>> +
>> +/* Indexes and other defines for response received from the controller */
>> +#define RESP_LEN 0
>> +#define RESP_CMD 1 /* Echo of CMD byte from command */
>> +#define RESP_DATA 2 /* Start of received data */
>> +
>> +#define SET_RESP_LEN 3
>> +#define GET_RESP_LEN 4
>> +#define MIN_RESP_LEN 3
>> +#define MAX_RESP_LEN 80
>> +
>> +struct dell_uart_backlight {
>> + struct mutex mutex;
>> + wait_queue_head_t wait_queue;
>> + struct device *dev;
>> + struct backlight_device *bl;
>> + u8 *resp;
>> + u8 resp_idx;
>> + u8 resp_len;
>> + u8 resp_max_len;
>> + u8 pending_cmd;
>> + int status;
>> + int power;
>> +};
>> +
>> +/* Checksum: SUM(Length and Cmd and Data) xor 0xFF */
>> +static u8 dell_uart_checksum(u8 *buf, int len)
>> +{
>> + u8 val = 0;
>> +
>> + while (len-- > 0)
>> + val += buf[len];
>> +
>> + return val ^ 0xff;
>> +}
>> +
>> +static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
>> + const u8 *cmd, int cmd_len,
>> + u8 *resp, int resp_max_len)
>> +{
>> + int ret;
>> +
>> + ret = mutex_lock_killable(&dell_bl->mutex);
>> + if (ret)
>> + return ret;
>> +
>> + dell_bl->status = -EBUSY;
>> + dell_bl->resp = resp;
>> + dell_bl->resp_idx = 0;
>> + dell_bl->resp_len = -1; /* Invalid / unset */
>> + dell_bl->resp_max_len = resp_max_len;
>> + dell_bl->pending_cmd = cmd[1];
>> +
>> + /* The TTY buffer should be big enough to take the entire cmd in one go */
>> + ret = serdev_device_write_buf(to_serdev_device(dell_bl->dev), cmd, cmd_len);
>> + if (ret != cmd_len) {
>> + dev_err(dell_bl->dev, "Error writing command: %d\n", ret);
>> + ret = (ret < 0) ? ret : -EIO;
>
> This does nothing because you return dell_bl->status.
Ugh, thank you for catching that. This is fallout from changing the initial
status value from 0 to -EBUSY.
Ive squashed the following fix into my review-hans branch to fix this:
diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
index 4e73fa035aca..87d2a20b4cb3 100644
--- a/drivers/platform/x86/dell/dell-uart-backlight.c
+++ b/drivers/platform/x86/dell/dell-uart-backlight.c
@@ -95,7 +95,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
ret = serdev_device_write_buf(to_serdev_device(dell_bl->dev), cmd, cmd_len);
if (ret != cmd_len) {
dev_err(dell_bl->dev, "Error writing command: %d\n", ret);
- ret = (ret < 0) ? ret : -EIO;
+ dell_bl->status = (ret < 0) ? ret : -EIO;
goto out;
}
Regards,
Hans
