| Message ID | 20240512162353.46693-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/x86: Add new Dell UART backlight driver | expand |
On Sun, May 12, 2024 at 7:24 PM Hans de Goede <hdegoede@redhat.com> 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. ... > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt How is this being used? ... > +#include <linux/acpi.h> + array_size.h > +#include <linux/backlight.h> > +#include <linux/delay.h> + device.h // devm_kzalloc(), dev_err() et al. + err.h > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/platform_device.h> > +#include <linux/serdev.h> + string.h + types.h > +#include <linux/wait.h> > +/* The backlight controller must respond within 1 second */ > +#define DELL_BL_TIMEOUT msecs_to_jiffies(1000) ... > +static int dell_uart_get_brightness(struct dell_uart_backlight *dell_bl) > +{ > + /* > + * Get Brightness level: Application uses this command to get brightness. > + * Command: 0x6A 0x0C 0x89 (Length:3 Type:0x0A Cmd:0x0C Checksum:0x89) > + * Return data: 0x04 0x0C Data Checksum > + * (Length:4 Cmd:0x0C Data:<brightness level> > + * Checksum: SUM(Length and Cmd and Data) xor 0xFF) > + * <brightness level> ranges from 0~100. > + */ > + const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 }; > + u8 resp[4]; > + int ret; > + > + ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness), > + resp, ARRAY_SIZE(resp)); > + if (ret) > + return ret; > + > + if (resp[0] != 4) { ARRAY_SIZE() as you used it in many other similar places. > + dev_err(dell_bl->dev, "Unexpected get brightness response length: %d\n", resp[0]); > + return -EIO; > + } > + if (resp[2] > 100) { (see also below about this number) > + dev_err(dell_bl->dev, "Unexpected get brightness response: %d\n", resp[2]); > + return -EIO; > + } > + > + return resp[2]; > +} > + > +static int dell_uart_set_bl_power(struct dell_uart_backlight *dell_bl, int power) > +{ > + /* > + * Screen ON/OFF Control: Application uses this command to control screen ON or OFF. > + * Command: 0x8A 0x0E Data Checksum (Length:4 Type:0x0A Cmd:0x0E) where > + * Data=0 to turn OFF the screen. > + * Data=1 to turn ON the screen. > + * Other value of Data is reserved and invalid. values are reserved > + * Return data: 0x03 0x0E 0xEE (Length:3 Cmd:0x0E Checksum:0xEE) > + */ > + u8 set_power[] = { 0x8A, 0x0E, 0x00, 0x00 }; > + u8 resp[3]; > + int ret; > + > + set_power[2] = (power == FB_BLANK_UNBLANK) ? 1 : 0; > + set_power[3] = dell_uart_checksum(set_power, 3); > + > + ret = dell_uart_bl_command(dell_bl, set_power, ARRAY_SIZE(set_power), > + resp, ARRAY_SIZE(resp)); > + if (ret) > + return ret; > + > + dell_bl->power = power; > + return 0; > +} ... > +static int dell_uart_update_status(struct backlight_device *bd) > +{ > + struct dell_uart_backlight *dell_bl = bl_get_data(bd); > + int ret; > + > + ret = dell_uart_set_brightness(dell_bl, bd->props.brightness); > + if (ret) > + return ret; > + > + if (bd->props.power != dell_uart_get_bl_power(dell_bl)) > + ret = dell_uart_set_bl_power(dell_bl, bd->props.power); return ...; > + return ret; return 0; ? > +} ... > + props.max_brightness = 100; Isn't it the same number (semantically) that is used in one of the above functions? Perhaps define it? ... > + if (IS_ERR(dell_bl->bl)) > + return PTR_ERR(dell_bl->bl); > + > + return 0; return PTR_ERR_OR_ZERO(...); ... Haven't noticed MODULE_DEVICE_TABLE(). Is it supposed to be autoloaded? If so, how would it happen? Ah, okay, you are using MODULE_ALIAS(). -- With Best Regards, Andy Shevchenko
On Sun, 12 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. > > Co-developed-by: AceLan Kao <acelan.kao@canonical.com> > Signed-off-by: AceLan Kao <acelan.kao@canonical.com> > Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/dell/Kconfig | 15 + > drivers/platform/x86/dell/Makefile | 1 + > .../platform/x86/dell/dell-uart-backlight.c | 409 ++++++++++++++++++ > 3 files changed, 425 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..d3ffea9e6270 > --- /dev/null > +++ b/drivers/platform/x86/dell/dell-uart-backlight.c > @@ -0,0 +1,409 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Dell AIO Serial Backlight Driver > + * > + * Copyright (C) 2024 Hans de Goede <hansg@kernel.org> > + * Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com> > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/acpi.h> > +#include <linux/backlight.h> > +#include <linux/delay.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/platform_device.h> > +#include <linux/serdev.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_MIN_RESP_SIZE 3 > + > +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 = 0; > + dell_bl->resp = resp; > + dell_bl->resp_idx = 0; > + 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; > + goto out; > + } > + > + ret = wait_event_timeout(dell_bl->wait_queue, dell_bl->status, DELL_BL_TIMEOUT); > + if (ret == 0) { > + dev_err(dell_bl->dev, "Timed out waiting for response.\n"); > + dell_bl->status = -ETIMEDOUT; > + } > + > + if (dell_bl->status == 1) > + ret = 0; > + else > + ret = dell_bl->status; I wonder if it would make dell_bl->status easier to follow if you'd first make it -EBUSY instead of 0 and set it to 0 on success? It would basically be normal errno behavior without extra values then and you wouldn't need to map it into return value here. > +out: > + mutex_unlock(&dell_bl->mutex); > + return ret; > +} > + > +static int dell_uart_set_brightness(struct dell_uart_backlight *dell_bl, int brightness) > +{ > + /* > + * Set Brightness level: Application uses this command to set brightness. > + * Command: 0x8A 0x0B <brightness-level> Checksum (Length:4 Type:0x0A Cmd:0x0B) > + * <brightness-level> ranges from 0~100. Why ~ character, is this just - ? > + * Return data: 0x03 0x0B 0xF1 (Length:3 Cmd:0x0B Checksum:0xF1) All these commands return header + echo cmd + (optional) data + checksum. I'm not sure why they all need a comment about it... It's also slightly misleading to call it "Return data" which can be misinterpreted to mean the return value of this function which is not correct (code calls it resp(onse) anyway so if it's necessary, use response data instead). > + */ > + u8 set_brightness[] = { 0x8A, 0x0B, 0x00, 0x00 }; Use #defines instead of literals. I think it makes the entire comments about the commands mostly useless when these are converted into properly named defines. > + u8 resp[3]; > + > + set_brightness[2] = brightness; > + set_brightness[3] = dell_uart_checksum(set_brightness, 3); Also, couldn't these be accessed through a struct to eliminate most of the magic indexes? > + return dell_uart_bl_command(dell_bl, set_brightness, ARRAY_SIZE(set_brightness), > + resp, ARRAY_SIZE(resp)); > +} > + > +static int dell_uart_get_brightness(struct dell_uart_backlight *dell_bl) > +{ > + /* > + * Get Brightness level: Application uses this command to get brightness. > + * Command: 0x6A 0x0C 0x89 (Length:3 Type:0x0A Cmd:0x0C Checksum:0x89) > + * Return data: 0x04 0x0C Data Checksum > + * (Length:4 Cmd:0x0C Data:<brightness level> > + * Checksum: SUM(Length and Cmd and Data) xor 0xFF) > + * <brightness level> ranges from 0~100. ~ -> - ? > + */ > + const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 }; > + u8 resp[4]; > + int ret; > + > + ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness), > + resp, ARRAY_SIZE(resp)); > + if (ret) > + return ret; > + > + if (resp[0] != 4) { sizeof(resp), but isn't this already checked when reading it?? > + dev_err(dell_bl->dev, "Unexpected get brightness response length: %d\n", resp[0]); > + return -EIO; > + } > + > + if (resp[2] > 100) { Add #define. > + dev_err(dell_bl->dev, "Unexpected get brightness response: %d\n", resp[2]); > + return -EIO; > + } > + > + return resp[2]; > +} > + > +static int dell_uart_set_bl_power(struct dell_uart_backlight *dell_bl, int power) > +{ > + /* > + * Screen ON/OFF Control: Application uses this command to control screen ON or OFF. > + * Command: 0x8A 0x0E Data Checksum (Length:4 Type:0x0A Cmd:0x0E) where > + * Data=0 to turn OFF the screen. > + * Data=1 to turn ON the screen. > + * Other value of Data is reserved and invalid. > + * Return data: 0x03 0x0E 0xEE (Length:3 Cmd:0x0E Checksum:0xEE) > + */ > + u8 set_power[] = { 0x8A, 0x0E, 0x00, 0x00 }; > + u8 resp[3]; > + int ret; > + > + set_power[2] = (power == FB_BLANK_UNBLANK) ? 1 : 0; > + set_power[3] = dell_uart_checksum(set_power, 3); > + > + ret = dell_uart_bl_command(dell_bl, set_power, ARRAY_SIZE(set_power), > + resp, ARRAY_SIZE(resp)); > + if (ret) > + return ret; > + > + dell_bl->power = power; > + return 0; > +} > + > +/* > + * There is no command to get backlight power status, > + * so we set the backlight power to "on" while initializing, > + * and then track and report its status by power variable Missing . > + */ > +static int dell_uart_get_bl_power(struct dell_uart_backlight *dell_bl) > +{ > + return dell_bl->power; > +} > + > +static int dell_uart_update_status(struct backlight_device *bd) > +{ > + struct dell_uart_backlight *dell_bl = bl_get_data(bd); > + int ret; > + > + ret = dell_uart_set_brightness(dell_bl, bd->props.brightness); > + if (ret) > + return ret; > + > + if (bd->props.power != dell_uart_get_bl_power(dell_bl)) > + ret = dell_uart_set_bl_power(dell_bl, bd->props.power); > + > + return ret; > +} > + > +static int dell_uart_get_brightness_op(struct backlight_device *bd) > +{ > + return dell_uart_get_brightness(bl_get_data(bd)); > +} > + > +static const struct backlight_ops dell_uart_backlight_ops = { > + .update_status = dell_uart_update_status, > + .get_brightness = dell_uart_get_brightness_op, > +}; > + > +static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, size_t len) > +{ > + struct dell_uart_backlight *dell_bl = serdev_device_get_drvdata(serdev); > + size_t i; > + u8 csum; > + > + dev_dbg(dell_bl->dev, "Recv: %*ph\n", (int)len, data); > + > + /* Throw away unexpected bytes / remainder of response after an error */ > + if (dell_bl->status) { As mentioned above, != -EBUSY ? > + dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n"); > + return len; > + } > + > + for (i = 0; i < len; i++) { > + dell_bl->resp[dell_bl->resp_idx] = data[i]; > + > + switch (dell_bl->resp_idx) { > + case 0: /* Length byte */ > + dell_bl->resp_len = dell_bl->resp[0]; > + if (dell_bl->resp_len < DELL_BL_MIN_RESP_SIZE) { > + dev_err(dell_bl->dev, "Response length too small %d < %d\n", > + dell_bl->resp_len, DELL_BL_MIN_RESP_SIZE); > + dell_bl->status = -EIO; > + goto wakeup; > + } else if (dell_bl->resp_len > dell_bl->resp_max_len) { Unnecessary else because of the goto. > + dev_err(dell_bl->dev, "Response length too big %d > %d\n", > + dell_bl->resp_len, dell_bl->resp_max_len); > + dell_bl->status = -EIO; > + goto wakeup; > + } > + break; > + case 1: /* CMD byte */ > + if (dell_bl->resp[1] != dell_bl->pending_cmd) { > + dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", > + dell_bl->resp[1], dell_bl->pending_cmd); > + dell_bl->status = -EIO; > + goto wakeup; > + } > + break; > + } > + > + dell_bl->resp_idx++; > + if (dell_bl->resp_idx < dell_bl->resp_len) > + continue; > + > + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > + dell_bl->resp[dell_bl->resp_len - 1], csum); > + dell_bl->status = -EIO; > + goto wakeup; > + } Why is the checksum calculation and check inside the loop?? > + dell_bl->status = 1; /* Success */ As mentioned above, change this to = 0 ? > + goto wakeup; Huh? Now I'm totally lost how the control flow is supposed to go in this function. Can you rethink this loop so it actual makes sense and doesn't misuse gotos like this? > + } > + > + return len; > + > +wakeup: > + wake_up(&dell_bl->wait_queue); > + return i + 1; > +} > + > +static const struct serdev_device_ops dell_uart_bl_serdev_ops = { > + .receive_buf = dell_uart_bl_receive, > + .write_wakeup = serdev_device_write_wakeup, > +}; > + > +static int dell_uart_bl_serdev_probe(struct serdev_device *serdev) > +{ > + /* > + * Get Firmware Version: Tool uses this command to get firmware version. > + * Command: 0x6A 0x06 0x8F (Length:3 Type:0x0A Cmd:6 Checksum:0x8F) > + * Return data: 0x0D 0x06 Data Checksum (Length:13 Cmd:0x06 > + * Data:F/W version(APRILIA=APR27-Vxxx/PHINE=PHI23-Vxxx) > + * Checksum:SUM(Length and Cmd and Data) xor 0xFF) > + */ > + const u8 get_firmware_ver[] = { 0x6A, 0x06, 0x8F }; > + struct dell_uart_backlight *dell_bl; > + struct backlight_properties props; > + struct device *dev = &serdev->dev; > + u8 get_firmware_ver_resp[80]; > + int ret; > + > + dell_bl = devm_kzalloc(dev, sizeof(*dell_bl), GFP_KERNEL); > + if (!dell_bl) > + return -ENOMEM; > + > + mutex_init(&dell_bl->mutex); > + init_waitqueue_head(&dell_bl->wait_queue); > + dell_bl->dev = dev; > + > + ret = devm_serdev_device_open(dev, serdev); > + if (ret) > + return dev_err_probe(dev, ret, "opening UART device\n"); > + > + /* 9600 bps, no flow control, these are the default but set them to be sure */ > + serdev_device_set_baudrate(serdev, 9600); > + serdev_device_set_flow_control(serdev, false); > + serdev_device_set_drvdata(serdev, dell_bl); > + serdev_device_set_client_ops(serdev, &dell_uart_bl_serdev_ops); > + > + ret = dell_uart_bl_command(dell_bl, get_firmware_ver, ARRAY_SIZE(get_firmware_ver), > + get_firmware_ver_resp, ARRAY_SIZE(get_firmware_ver_resp)); > + if (ret) > + return dev_err_probe(dev, ret, "getting firmware version\n"); > + > + dev_dbg(dev, "Firmware version: %.*s\n", get_firmware_ver_resp[0] - 3, > + get_firmware_ver_resp + 2); > + > + /* Initialize bl_power to a known value */ > + ret = dell_uart_set_bl_power(dell_bl, FB_BLANK_UNBLANK); > + if (ret) > + return ret; > + > + ret = dell_uart_get_brightness(dell_bl); > + if (ret < 0) > + return ret; > + > + memset(&props, 0, sizeof(struct backlight_properties)); Just assigned = {} when declaring. > + props.type = BACKLIGHT_PLATFORM; > + props.brightness = ret; > + props.max_brightness = 100; Use #define. > + props.power = dell_bl->power; > + > + dell_bl->bl = devm_backlight_device_register(dev, "dell_uart_backlight", > + dev, dell_bl, > + &dell_uart_backlight_ops, > + &props); > + if (IS_ERR(dell_bl->bl)) > + return PTR_ERR(dell_bl->bl); > + > + return 0; > +} > + > +struct serdev_device_driver dell_uart_bl_serdev_driver = { > + .probe = dell_uart_bl_serdev_probe, > + .driver = { > + .name = KBUILD_MODNAME, > + }, > +}; > + > +static int dell_uart_bl_pdev_probe(struct platform_device *pdev) > +{ > + struct serdev_device *serdev; > + struct device *ctrl_dev; > + int ret; > + > + ctrl_dev = get_serdev_controller("DELL0501", NULL, 0, "serial0"); > + if (IS_ERR(ctrl_dev)) > + return PTR_ERR(ctrl_dev); > + > + serdev = serdev_device_alloc(to_serdev_controller(ctrl_dev)); > + put_device(ctrl_dev); > + if (!serdev) > + return -ENOMEM; > + > + ret = serdev_device_add(serdev); > + if (ret) { > + dev_err(&pdev->dev, "error %d adding serdev\n", ret); > + serdev_device_put(serdev); > + return ret; > + } > + > + ret = serdev_device_driver_register(&dell_uart_bl_serdev_driver); > + if (ret) { > + serdev_device_remove(serdev); > + return ret; > + } > + > + /* > + * serdev device <-> driver matching relies on OF or ACPI matches and > + * neither is available here, manually bind the driver. > + */ > + ret = device_driver_attach(&dell_uart_bl_serdev_driver.driver, &serdev->dev); > + if (ret) { > + serdev_device_driver_unregister(&dell_uart_bl_serdev_driver); > + serdev_device_remove(serdev); > + return ret; > + } The last two error branch could use normal rollback with goto.
Hi Ilpo, Thank you for the review. On 5/13/24 10:34 AM, Ilpo Järvinen wrote: > On Sun, 12 May 2024, Hans de Goede wrote: <snip> >> + dell_bl->status = 0; >> + dell_bl->resp = resp; >> + dell_bl->resp_idx = 0; >> + 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; >> + goto out; >> + } >> + >> + ret = wait_event_timeout(dell_bl->wait_queue, dell_bl->status, DELL_BL_TIMEOUT); >> + if (ret == 0) { >> + dev_err(dell_bl->dev, "Timed out waiting for response.\n"); >> + dell_bl->status = -ETIMEDOUT; >> + } >> + >> + if (dell_bl->status == 1) >> + ret = 0; >> + else >> + ret = dell_bl->status; > > I wonder if it would make dell_bl->status easier to follow if you'd first > make it -EBUSY instead of 0 and set it to 0 on success? > > It would basically be normal errno behavior without extra values then and > you wouldn't need to map it into return value here. Ack, done for v2. > >> +out: >> + mutex_unlock(&dell_bl->mutex); >> + return ret; >> +} >> + >> +static int dell_uart_set_brightness(struct dell_uart_backlight *dell_bl, int brightness) >> +{ >> + /* >> + * Set Brightness level: Application uses this command to set brightness. >> + * Command: 0x8A 0x0B <brightness-level> Checksum (Length:4 Type:0x0A Cmd:0x0B) >> + * <brightness-level> ranges from 0~100. > > Why ~ character, is this just - ? Yes just -, the ~ came from the original driver I based this on I'll fix this for v2. >> + * Return data: 0x03 0x0B 0xF1 (Length:3 Cmd:0x0B Checksum:0xF1) > > All these commands return header + echo cmd + (optional) data + checksum. > I'm not sure why they all need a comment about it... > > It's also slightly misleading to call it "Return data" which can be > misinterpreted to mean the return value of this function which is not > correct (code calls it resp(onse) anyway so if it's necessary, use > response data instead). > >> + */ >> + u8 set_brightness[] = { 0x8A, 0x0B, 0x00, 0x00 }; > > Use #defines instead of literals. Ack, will fix for v2. > > I think it makes the entire comments about the commands mostly useless > when these are converted into properly named defines. Ack, will drop. > >> + u8 resp[3]; >> + >> + set_brightness[2] = brightness; >> + set_brightness[3] = dell_uart_checksum(set_brightness, 3); > > Also, couldn't these be accessed through a struct to eliminate most of the > magic indexes? With the checksum at the end, this would require a VLA in the middle of the struct (get_version return buffer contains more then 1 dat byte) We could treat the checksum as an extra data byte, but then we are mixing struct usage for some fields + using an array of bytes approach for the data + checksum. For consistency I prefer to just stick with one approach which means using the array of bytes approach for everything. > >> + return dell_uart_bl_command(dell_bl, set_brightness, ARRAY_SIZE(set_brightness), >> + resp, ARRAY_SIZE(resp)); >> +} >> + >> +static int dell_uart_get_brightness(struct dell_uart_backlight *dell_bl) >> +{ >> + /* >> + * Get Brightness level: Application uses this command to get brightness. >> + * Command: 0x6A 0x0C 0x89 (Length:3 Type:0x0A Cmd:0x0C Checksum:0x89) >> + * Return data: 0x04 0x0C Data Checksum >> + * (Length:4 Cmd:0x0C Data:<brightness level> >> + * Checksum: SUM(Length and Cmd and Data) xor 0xFF) >> + * <brightness level> ranges from 0~100. > > ~ -> - ? > >> + */ >> + const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 }; >> + u8 resp[4]; >> + int ret; >> + >> + ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness), >> + resp, ARRAY_SIZE(resp)); >> + if (ret) >> + return ret; >> + >> + if (resp[0] != 4) { > > sizeof(resp) Ack. > but isn't this already checked when reading it?? No dell_uart_bl_receive() checks that the response will fit in the supplied buffer and that it has a valid checksum but the controller may send a response smaller then the passed in buffer and it will actually do this for the get_version command. >> + dev_err(dell_bl->dev, "Unexpected get brightness response length: %d\n", resp[0]); >> + return -EIO; >> + } >> + >> + if (resp[2] > 100) { > > Add #define. Ack. <snip> >> +/* >> + * There is no command to get backlight power status, >> + * so we set the backlight power to "on" while initializing, >> + * and then track and report its status by power variable > > Missing . Ack. <snip> >> +static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, size_t len) >> +{ >> + struct dell_uart_backlight *dell_bl = serdev_device_get_drvdata(serdev); >> + size_t i; >> + u8 csum; >> + >> + dev_dbg(dell_bl->dev, "Recv: %*ph\n", (int)len, data); >> + >> + /* Throw away unexpected bytes / remainder of response after an error */ >> + if (dell_bl->status) { > > As mentioned above, != -EBUSY ? > Ack, done for v2. >> + dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n"); >> + return len; >> + } >> + >> + for (i = 0; i < len; i++) { >> + dell_bl->resp[dell_bl->resp_idx] = data[i]; >> + >> + switch (dell_bl->resp_idx) { >> + case 0: /* Length byte */ >> + dell_bl->resp_len = dell_bl->resp[0]; >> + if (dell_bl->resp_len < DELL_BL_MIN_RESP_SIZE) { >> + dev_err(dell_bl->dev, "Response length too small %d < %d\n", >> + dell_bl->resp_len, DELL_BL_MIN_RESP_SIZE); >> + dell_bl->status = -EIO; >> + goto wakeup; >> + } else if (dell_bl->resp_len > dell_bl->resp_max_len) { > > Unnecessary else because of the goto. > >> + dev_err(dell_bl->dev, "Response length too big %d > %d\n", >> + dell_bl->resp_len, dell_bl->resp_max_len); >> + dell_bl->status = -EIO; >> + goto wakeup; >> + } >> + break; >> + case 1: /* CMD byte */ >> + if (dell_bl->resp[1] != dell_bl->pending_cmd) { >> + dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", >> + dell_bl->resp[1], dell_bl->pending_cmd); >> + dell_bl->status = -EIO; >> + goto wakeup; >> + } >> + break; >> + } >> + >> + dell_bl->resp_idx++; >> + if (dell_bl->resp_idx < dell_bl->resp_len) >> + continue; >> + >> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); >> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { >> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", >> + dell_bl->resp[dell_bl->resp_len - 1], csum); >> + dell_bl->status = -EIO; >> + goto wakeup; >> + } > > Why is the checksum calculation and check inside the loop?? The loop iterates over received bytes, which may contain extra data after the response, the: dell_bl->resp_idx++; if (dell_bl->resp_idx < dell_bl->resp_len) continue; continues looping until we have received all the expected bytes. So here, past this check, we are are at the point where we have a complete response and then we verify it. And on successful verification wake-up any waiters. > >> + dell_bl->status = 1; /* Success */ > > As mentioned above, change this to = 0 Ack, done. ? > >> + goto wakeup; > > Huh? Now I'm totally lost how the control flow is supposed to go in this > function. Can you rethink this loop so it actual makes sense and doesn't > misuse gotos like this? This is the receive() callback from the UART the loop consumes bytes received by the UART. The gotos stop consuming bytes in 2 cases: 1. An error (unexpected data) is encountered. 2. A complete frame has been successfully received. The checking of the checksum + goto wakeup at the end of the loop is for 2. The: return len; after the loop indicates to the UART / tty-layer that all passed data has been consumed and this path gets hit when the driver needs to wait for more data because the response is not complete yet. >> + >> +wakeup: >> + wake_up(&dell_bl->wait_queue); >> + return i + 1; >> +} >> + <snip> >> + memset(&props, 0, sizeof(struct backlight_properties)); > > Just assigned = {} when declaring. Ack. >> + props.type = BACKLIGHT_PLATFORM; >> + props.brightness = ret; >> + props.max_brightness = 100; > > Use #define. Ack. <snip> >> + ret = serdev_device_add(serdev); >> + if (ret) { >> + dev_err(&pdev->dev, "error %d adding serdev\n", ret); >> + serdev_device_put(serdev); >> + return ret; >> + } >> + >> + ret = serdev_device_driver_register(&dell_uart_bl_serdev_driver); >> + if (ret) { >> + serdev_device_remove(serdev); >> + return ret; >> + } >> + >> + /* >> + * serdev device <-> driver matching relies on OF or ACPI matches and >> + * neither is available here, manually bind the driver. >> + */ >> + ret = device_driver_attach(&dell_uart_bl_serdev_driver.driver, &serdev->dev); >> + if (ret) { >> + serdev_device_driver_unregister(&dell_uart_bl_serdev_driver); >> + serdev_device_remove(serdev); >> + return ret; >> + } > > The last two error branch could use normal rollback with goto. Ack, will fix for v2. Regards, Hans
Hi, On 5/12/24 9:35 PM, Andy Shevchenko wrote: > On Sun, May 12, 2024 at 7:24 PM Hans de Goede <hdegoede@redhat.com> 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. > > ... > >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > How is this being used? #include "../serdev_helpers.h" uses this. > > ... > >> +#include <linux/acpi.h> > > + array_size.h > >> +#include <linux/backlight.h> >> +#include <linux/delay.h> > > + device.h // devm_kzalloc(), dev_err() et al. > > + err.h > >> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/platform_device.h> >> +#include <linux/serdev.h> > > + string.h > + types.h > >> +#include <linux/wait.h> Ack, all includes added, thanks. <snip> >> +static int dell_uart_update_status(struct backlight_device *bd) >> +{ >> + struct dell_uart_backlight *dell_bl = bl_get_data(bd); >> + int ret; >> + >> + ret = dell_uart_set_brightness(dell_bl, bd->props.brightness); >> + if (ret) >> + return ret; >> + >> + if (bd->props.power != dell_uart_get_bl_power(dell_bl)) >> + ret = dell_uart_set_bl_power(dell_bl, bd->props.power); > > return ...; > >> + return ret; > > return 0; > > ? Ack, fixed for v2. Regards, Hans
On Mon, 13 May 2024, Hans de Goede wrote: > Hi Ilpo, > > Thank you for the review. > > On 5/13/24 10:34 AM, Ilpo Järvinen wrote: > > On Sun, 12 May 2024, Hans de Goede wrote: > >> + u8 resp[3]; > >> + > >> + set_brightness[2] = brightness; > >> + set_brightness[3] = dell_uart_checksum(set_brightness, 3); > > > > Also, couldn't these be accessed through a struct to eliminate most of the > > magic indexes? > > With the checksum at the end, this would require a VLA in the middle > of the struct (get_version return buffer contains more then 1 dat byte) > We could treat the checksum as an extra data byte, but then we are > mixing struct usage for some fields + using an array of bytes > approach for the data + checksum. For consistency I prefer to just > stick with one approach which means using the array of bytes approach > for everything. Ok. > >> + const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 }; > >> + u8 resp[4]; > >> + int ret; > >> + > >> + ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness), > >> + resp, ARRAY_SIZE(resp)); > >> + if (ret) > >> + return ret; > >> + > >> + if (resp[0] != 4) { > > > > sizeof(resp) > > Ack. > > > but isn't this already checked when reading it?? > > No dell_uart_bl_receive() checks that the response will fit in the supplied > buffer and that it has a valid checksum but the controller may send a > response smaller then the passed in buffer and it will actually do this for > the get_version command. Ah, I see now that it checked against constant rather than the actual value. > >> + dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n"); > >> + return len; > >> + } > >> + > >> + for (i = 0; i < len; i++) { > >> + dell_bl->resp[dell_bl->resp_idx] = data[i]; > >> + > >> + switch (dell_bl->resp_idx) { > >> + case 0: /* Length byte */ > >> + dell_bl->resp_len = dell_bl->resp[0]; > >> + if (dell_bl->resp_len < DELL_BL_MIN_RESP_SIZE) { > >> + dev_err(dell_bl->dev, "Response length too small %d < %d\n", > >> + dell_bl->resp_len, DELL_BL_MIN_RESP_SIZE); > >> + dell_bl->status = -EIO; > >> + goto wakeup; > >> + } else if (dell_bl->resp_len > dell_bl->resp_max_len) { > >> + dev_err(dell_bl->dev, "Response length too big %d > %d\n", > >> + dell_bl->resp_len, dell_bl->resp_max_len); > >> + dell_bl->status = -EIO; > >> + goto wakeup; > >> + } > >> + break; > >> + case 1: /* CMD byte */ > >> + if (dell_bl->resp[1] != dell_bl->pending_cmd) { > >> + dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", > >> + dell_bl->resp[1], dell_bl->pending_cmd); > >> + dell_bl->status = -EIO; > >> + goto wakeup; > >> + } > >> + break; > >> + } > >> + > >> + dell_bl->resp_idx++; > >> + if (dell_bl->resp_idx < dell_bl->resp_len) > >> + continue; > >> + > >> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > >> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > >> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > >> + dell_bl->resp[dell_bl->resp_len - 1], csum); > >> + dell_bl->status = -EIO; > >> + goto wakeup; > >> + } > > > > Why is the checksum calculation and check inside the loop?? > > The loop iterates over received bytes, which may contain extra data > after the response, the: > > dell_bl->resp_idx++; > if (dell_bl->resp_idx < dell_bl->resp_len) > continue; > > continues looping until we have received all the expected bytes. So here, past this > check, we are are at the point where we have a complete response and then we verify it. > > And on successful verification wake-up any waiters. So effectively you want to terminate the loop on two conditions here: a) dell_bl->resp_idx == dell_bl->resp_len (complete frame) a) if i == len (not yet received a full frame) Why not code those rather than the current goto & continue madness? Then, after the loop, you can test: if (dell_bl->resp_idx == dell_bl->resp_len) { // calc checksum, etc. } ? > >> + dell_bl->status = 1; /* Success */ > >> + goto wakeup; > > > > Huh? Now I'm totally lost how the control flow is supposed to go in this > > function. Can you rethink this loop so it actual makes sense and doesn't > > misuse gotos like this? > > This is the receive() callback from the UART the loop consumes bytes received > by the UART. The gotos stop consuming bytes in 2 cases: > > 1. An error (unexpected data) is encountered. > 2. A complete frame has been successfully received. > > The checking of the checksum + goto wakeup at the end of the loop is for 2. > > The: > > return len; > > after the loop indicates to the UART / tty-layer that all passed data > has been consumed and this path gets hit when the driver needs to wait > for more data because the response is not complete yet. > > >> + > >> +wakeup: > >> + wake_up(&dell_bl->wait_queue); > >> + return i + 1; > >> +}
On Mon, 13 May 2024, Ilpo Järvinen wrote: > On Mon, 13 May 2024, Hans de Goede wrote: > > > Hi Ilpo, > > > > Thank you for the review. > > > > On 5/13/24 10:34 AM, Ilpo Järvinen wrote: > > > On Sun, 12 May 2024, Hans de Goede wrote: > > > >> + u8 resp[3]; > > >> + > > >> + set_brightness[2] = brightness; > > >> + set_brightness[3] = dell_uart_checksum(set_brightness, 3); > > > > > > Also, couldn't these be accessed through a struct to eliminate most of the > > > magic indexes? > > > > With the checksum at the end, this would require a VLA in the middle > > of the struct (get_version return buffer contains more then 1 dat byte) > > We could treat the checksum as an extra data byte, but then we are > > mixing struct usage for some fields + using an array of bytes > > approach for the data + checksum. For consistency I prefer to just > > stick with one approach which means using the array of bytes approach > > for everything. > > Ok. > > > >> + const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 }; > > >> + u8 resp[4]; > > >> + int ret; > > >> + > > >> + ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness), > > >> + resp, ARRAY_SIZE(resp)); > > >> + if (ret) > > >> + return ret; > > >> + > > >> + if (resp[0] != 4) { > > > > > > sizeof(resp) > > > > Ack. > > > > > but isn't this already checked when reading it?? > > > > No dell_uart_bl_receive() checks that the response will fit in the supplied > > buffer and that it has a valid checksum but the controller may send a > > response smaller then the passed in buffer and it will actually do this for > > the get_version command. > > Ah, I see now that it checked against constant rather than the actual > value. > > > >> + dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n"); > > >> + return len; > > >> + } > > >> + > > >> + for (i = 0; i < len; i++) { > > >> + dell_bl->resp[dell_bl->resp_idx] = data[i]; > > >> + > > >> + switch (dell_bl->resp_idx) { > > >> + case 0: /* Length byte */ > > >> + dell_bl->resp_len = dell_bl->resp[0]; > > >> + if (dell_bl->resp_len < DELL_BL_MIN_RESP_SIZE) { > > >> + dev_err(dell_bl->dev, "Response length too small %d < %d\n", > > >> + dell_bl->resp_len, DELL_BL_MIN_RESP_SIZE); > > >> + dell_bl->status = -EIO; > > >> + goto wakeup; > > >> + } else if (dell_bl->resp_len > dell_bl->resp_max_len) { > > >> + dev_err(dell_bl->dev, "Response length too big %d > %d\n", > > >> + dell_bl->resp_len, dell_bl->resp_max_len); > > >> + dell_bl->status = -EIO; > > >> + goto wakeup; > > >> + } > > >> + break; > > >> + case 1: /* CMD byte */ > > >> + if (dell_bl->resp[1] != dell_bl->pending_cmd) { > > >> + dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", > > >> + dell_bl->resp[1], dell_bl->pending_cmd); > > >> + dell_bl->status = -EIO; > > >> + goto wakeup; > > >> + } > > >> + break; > > >> + } > > >> + > > >> + dell_bl->resp_idx++; > > >> + if (dell_bl->resp_idx < dell_bl->resp_len) > > >> + continue; > > >> + > > >> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > > >> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > > >> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > > >> + dell_bl->resp[dell_bl->resp_len - 1], csum); > > >> + dell_bl->status = -EIO; > > >> + goto wakeup; > > >> + } > > > > > > Why is the checksum calculation and check inside the loop?? > > > > The loop iterates over received bytes, which may contain extra data > > after the response, the: > > > > dell_bl->resp_idx++; > > if (dell_bl->resp_idx < dell_bl->resp_len) > > continue; > > > > continues looping until we have received all the expected bytes. So here, past this > > check, we are are at the point where we have a complete response and then we verify it. > > > > And on successful verification wake-up any waiters. > > So effectively you want to terminate the loop on two conditions here: > > a) dell_bl->resp_idx == dell_bl->resp_len (complete frame) > a) if i == len (not yet received a full frame) > > Why not code those rather than the current goto & continue madness? > > Then, after the loop, you can test: > > if (dell_bl->resp_idx == dell_bl->resp_len) { > // calc checksum, etc. > } > > ? + the len has been received check: if (dell_bl->resp_idx && dell_bl->resp_idx == dell_bl->resp_len) { ... }
On Mon, May 13, 2024 at 12:01:55PM +0200, Hans de Goede wrote: > On 5/12/24 9:35 PM, Andy Shevchenko wrote: > > On Sun, May 12, 2024 at 7:24 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > How is this being used? > > #include "../serdev_helpers.h" > > uses this. Yet another evidence why C code in the *.h is not a good idea.
On Mon, 13 May 2024, Andy Shevchenko wrote: > On Mon, May 13, 2024 at 12:01:55PM +0200, Hans de Goede wrote: > > On 5/12/24 9:35 PM, Andy Shevchenko wrote: > > > On Sun, May 12, 2024 at 7:24 PM Hans de Goede <hdegoede@redhat.com> wrote: > > ... > > > >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > > > How is this being used? > > > > #include "../serdev_helpers.h" > > > > uses this. > > Yet another evidence why C code in the *.h is not a good idea. get_serdev_controller() function is quite complex anyway to be inlined, and if there's going to be another user now, it should be uninlined.
Hi, On 5/13/24 2:12 PM, Ilpo Järvinen wrote: > On Mon, 13 May 2024, Hans de Goede wrote: > >> Hi Ilpo, >> >> Thank you for the review. >> >> On 5/13/24 10:34 AM, Ilpo Järvinen wrote: >>> On Sun, 12 May 2024, Hans de Goede wrote: > >>>> + u8 resp[3]; >>>> + >>>> + set_brightness[2] = brightness; >>>> + set_brightness[3] = dell_uart_checksum(set_brightness, 3); >>> >>> Also, couldn't these be accessed through a struct to eliminate most of the >>> magic indexes? >> >> With the checksum at the end, this would require a VLA in the middle >> of the struct (get_version return buffer contains more then 1 dat byte) >> We could treat the checksum as an extra data byte, but then we are >> mixing struct usage for some fields + using an array of bytes >> approach for the data + checksum. For consistency I prefer to just >> stick with one approach which means using the array of bytes approach >> for everything. > > Ok. > >>>> + const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 }; >>>> + u8 resp[4]; >>>> + int ret; >>>> + >>>> + ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness), >>>> + resp, ARRAY_SIZE(resp)); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + if (resp[0] != 4) { >>> >>> sizeof(resp) >> >> Ack. >> >>> but isn't this already checked when reading it?? >> >> No dell_uart_bl_receive() checks that the response will fit in the supplied >> buffer and that it has a valid checksum but the controller may send a >> response smaller then the passed in buffer and it will actually do this for >> the get_version command. > > Ah, I see now that it checked against constant rather than the actual > value. > >>>> + dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n"); >>>> + return len; >>>> + } >>>> + >>>> + for (i = 0; i < len; i++) { >>>> + dell_bl->resp[dell_bl->resp_idx] = data[i]; >>>> + >>>> + switch (dell_bl->resp_idx) { >>>> + case 0: /* Length byte */ >>>> + dell_bl->resp_len = dell_bl->resp[0]; >>>> + if (dell_bl->resp_len < DELL_BL_MIN_RESP_SIZE) { >>>> + dev_err(dell_bl->dev, "Response length too small %d < %d\n", >>>> + dell_bl->resp_len, DELL_BL_MIN_RESP_SIZE); >>>> + dell_bl->status = -EIO; >>>> + goto wakeup; >>>> + } else if (dell_bl->resp_len > dell_bl->resp_max_len) { >>>> + dev_err(dell_bl->dev, "Response length too big %d > %d\n", >>>> + dell_bl->resp_len, dell_bl->resp_max_len); >>>> + dell_bl->status = -EIO; >>>> + goto wakeup; >>>> + } >>>> + break; >>>> + case 1: /* CMD byte */ >>>> + if (dell_bl->resp[1] != dell_bl->pending_cmd) { >>>> + dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", >>>> + dell_bl->resp[1], dell_bl->pending_cmd); >>>> + dell_bl->status = -EIO; >>>> + goto wakeup; >>>> + } >>>> + break; >>>> + } >>>> + >>>> + dell_bl->resp_idx++; >>>> + if (dell_bl->resp_idx < dell_bl->resp_len) >>>> + continue; >>>> + >>>> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); >>>> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { >>>> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", >>>> + dell_bl->resp[dell_bl->resp_len - 1], csum); >>>> + dell_bl->status = -EIO; >>>> + goto wakeup; >>>> + } >>> >>> Why is the checksum calculation and check inside the loop?? >> >> The loop iterates over received bytes, which may contain extra data >> after the response, the: >> >> dell_bl->resp_idx++; >> if (dell_bl->resp_idx < dell_bl->resp_len) >> continue; >> >> continues looping until we have received all the expected bytes. So here, past this >> check, we are are at the point where we have a complete response and then we verify it. >> >> And on successful verification wake-up any waiters. > > So effectively you want to terminate the loop on two conditions here: > > a) dell_bl->resp_idx == dell_bl->resp_len (complete frame) > a) if i == len (not yet received a full frame) > > Why not code those rather than the current goto & continue madness? > > Then, after the loop, you can test: > > if (dell_bl->resp_idx == dell_bl->resp_len) { > // calc checksum, etc. > } > > ? Ok, I've added the following change for v3: diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c index bf5b12efcb19..66d8c6ddcb83 100644 --- a/drivers/platform/x86/dell/dell-uart-backlight.c +++ b/drivers/platform/x86/dell/dell-uart-backlight.c @@ -87,6 +87,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl, 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]; @@ -219,7 +219,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, return len; } - for (i = 0; i < len; i++) { + for (i = 0; i < len && dell_bl->resp_idx != dell_bl->resp_len; i++, dell_bl->resp_idx++) { dell_bl->resp[dell_bl->resp_idx] = data[i]; switch (dell_bl->resp_idx) { @@ -228,46 +228,41 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, if (dell_bl->resp_len < MIN_RESP_LEN) { dev_err(dell_bl->dev, "Response length too small %d < %d\n", dell_bl->resp_len, MIN_RESP_LEN); - dell_bl->status = -EIO; - goto wakeup; + goto error; } if (dell_bl->resp_len > dell_bl->resp_max_len) { dev_err(dell_bl->dev, "Response length too big %d > %d\n", dell_bl->resp_len, dell_bl->resp_max_len); - dell_bl->status = -EIO; - goto wakeup; + goto error; } break; case RESP_CMD: /* CMD byte */ if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); - dell_bl->status = -EIO; - goto wakeup; + goto error; } break; } + } - dell_bl->resp_idx++; - if (dell_bl->resp_idx < dell_bl->resp_len) - continue; - + if (dell_bl->resp_idx == dell_bl->resp_len) { csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", dell_bl->resp[dell_bl->resp_len - 1], csum); - dell_bl->status = -EIO; - goto wakeup; + goto error; } - dell_bl->status = 0; /* Success */ - goto wakeup; + wake_up(&dell_bl->wait_queue); + return i; } return len; -wakeup: +error: + dell_bl->status = -EIO; wake_up(&dell_bl->wait_queue); return i + 1; } Regards, Hans > >>>> + dell_bl->status = 1; /* Success */ >>>> + goto wakeup; >>> >>> Huh? Now I'm totally lost how the control flow is supposed to go in this >>> function. Can you rethink this loop so it actual makes sense and doesn't >>> misuse gotos like this? >> >> This is the receive() callback from the UART the loop consumes bytes received >> by the UART. The gotos stop consuming bytes in 2 cases: >> >> 1. An error (unexpected data) is encountered. >> 2. A complete frame has been successfully received. >> >> The checking of the checksum + goto wakeup at the end of the loop is for 2. >> >> The: >> >> return len; >> >> after the loop indicates to the UART / tty-layer that all passed data >> has been consumed and this path gets hit when the driver needs to wait >> for more data because the response is not complete yet. >> >>>> + >>>> +wakeup: >>>> + wake_up(&dell_bl->wait_queue); >>>> + return i + 1; >>>> +} > >
On Mon, 13 May 2024, Hans de Goede wrote: > On 5/13/24 2:12 PM, Ilpo Järvinen wrote: > > On Mon, 13 May 2024, Hans de Goede wrote: > >> On 5/13/24 10:34 AM, Ilpo Järvinen wrote: > >>> On Sun, 12 May 2024, Hans de Goede wrote: > >>>> + > >>>> + dell_bl->resp_idx++; > >>>> + if (dell_bl->resp_idx < dell_bl->resp_len) > >>>> + continue; > >>>> + > >>>> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > >>>> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > >>>> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > >>>> + dell_bl->resp[dell_bl->resp_len - 1], csum); > >>>> + dell_bl->status = -EIO; > >>>> + goto wakeup; > >>>> + } > >>> > >>> Why is the checksum calculation and check inside the loop?? > >> > >> The loop iterates over received bytes, which may contain extra data > >> after the response, the: > >> > >> dell_bl->resp_idx++; > >> if (dell_bl->resp_idx < dell_bl->resp_len) > >> continue; > >> > >> continues looping until we have received all the expected bytes. So here, past this > >> check, we are are at the point where we have a complete response and then we verify it. > >> > >> And on successful verification wake-up any waiters. > > > > So effectively you want to terminate the loop on two conditions here: > > > > a) dell_bl->resp_idx == dell_bl->resp_len (complete frame) > > a) if i == len (not yet received a full frame) > > > > Why not code those rather than the current goto & continue madness? > > > > Then, after the loop, you can test: > > > > if (dell_bl->resp_idx == dell_bl->resp_len) { > > // calc checksum, etc. > > } > > > > ? > > Ok, I've added the following change for v3: > > diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c > index bf5b12efcb19..66d8c6ddcb83 100644 > --- a/drivers/platform/x86/dell/dell-uart-backlight.c > +++ b/drivers/platform/x86/dell/dell-uart-backlight.c > @@ -87,6 +87,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl, > 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]; > > @@ -219,7 +219,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, > return len; > } > > - for (i = 0; i < len; i++) { > + for (i = 0; i < len && dell_bl->resp_idx != dell_bl->resp_len; i++, dell_bl->resp_idx++) { > dell_bl->resp[dell_bl->resp_idx] = data[i]; > > switch (dell_bl->resp_idx) { > @@ -228,46 +228,41 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, > if (dell_bl->resp_len < MIN_RESP_LEN) { > dev_err(dell_bl->dev, "Response length too small %d < %d\n", > dell_bl->resp_len, MIN_RESP_LEN); > - dell_bl->status = -EIO; > - goto wakeup; > + goto error; > } > > if (dell_bl->resp_len > dell_bl->resp_max_len) { > dev_err(dell_bl->dev, "Response length too big %d > %d\n", > dell_bl->resp_len, dell_bl->resp_max_len); > - dell_bl->status = -EIO; > - goto wakeup; > + goto error; > } > break; > case RESP_CMD: /* CMD byte */ > if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { > dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", > dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); > - dell_bl->status = -EIO; > - goto wakeup; > + goto error; > } > break; > } > + } > > - dell_bl->resp_idx++; > - if (dell_bl->resp_idx < dell_bl->resp_len) > - continue; > - > + if (dell_bl->resp_idx == dell_bl->resp_len) { > csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > dell_bl->resp[dell_bl->resp_len - 1], csum); > - dell_bl->status = -EIO; > - goto wakeup; > + goto error; > } > - > dell_bl->status = 0; /* Success */ > - goto wakeup; > + wake_up(&dell_bl->wait_queue); > + return i; > } > > return len; > > -wakeup: > +error: > + dell_bl->status = -EIO; > wake_up(&dell_bl->wait_queue); > return i + 1; > } Thanks, this is way easier to follow.
Hi, On 5/13/24 3:14 PM, Ilpo Järvinen wrote: > On Mon, 13 May 2024, Hans de Goede wrote: >> On 5/13/24 2:12 PM, Ilpo Järvinen wrote: >>> On Mon, 13 May 2024, Hans de Goede wrote: >>>> On 5/13/24 10:34 AM, Ilpo Järvinen wrote: >>>>> On Sun, 12 May 2024, Hans de Goede wrote: > >>>>>> + >>>>>> + dell_bl->resp_idx++; >>>>>> + if (dell_bl->resp_idx < dell_bl->resp_len) >>>>>> + continue; >>>>>> + >>>>>> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); >>>>>> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { >>>>>> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", >>>>>> + dell_bl->resp[dell_bl->resp_len - 1], csum); >>>>>> + dell_bl->status = -EIO; >>>>>> + goto wakeup; >>>>>> + } >>>>> >>>>> Why is the checksum calculation and check inside the loop?? >>>> >>>> The loop iterates over received bytes, which may contain extra data >>>> after the response, the: >>>> >>>> dell_bl->resp_idx++; >>>> if (dell_bl->resp_idx < dell_bl->resp_len) >>>> continue; >>>> >>>> continues looping until we have received all the expected bytes. So here, past this >>>> check, we are are at the point where we have a complete response and then we verify it. >>>> >>>> And on successful verification wake-up any waiters. >>> >>> So effectively you want to terminate the loop on two conditions here: >>> >>> a) dell_bl->resp_idx == dell_bl->resp_len (complete frame) >>> a) if i == len (not yet received a full frame) >>> >>> Why not code those rather than the current goto & continue madness? >>> >>> Then, after the loop, you can test: >>> >>> if (dell_bl->resp_idx == dell_bl->resp_len) { >>> // calc checksum, etc. >>> } >>> >>> ? >> >> Ok, I've added the following change for v3: >> >> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c >> index bf5b12efcb19..66d8c6ddcb83 100644 >> --- a/drivers/platform/x86/dell/dell-uart-backlight.c >> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c >> @@ -87,6 +87,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl, >> 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]; >> >> @@ -219,7 +219,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, >> return len; >> } >> >> - for (i = 0; i < len; i++) { >> + for (i = 0; i < len && dell_bl->resp_idx != dell_bl->resp_len; i++, dell_bl->resp_idx++) { >> dell_bl->resp[dell_bl->resp_idx] = data[i]; >> >> switch (dell_bl->resp_idx) { >> @@ -228,46 +228,41 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, >> if (dell_bl->resp_len < MIN_RESP_LEN) { >> dev_err(dell_bl->dev, "Response length too small %d < %d\n", >> dell_bl->resp_len, MIN_RESP_LEN); >> - dell_bl->status = -EIO; >> - goto wakeup; >> + goto error; >> } >> >> if (dell_bl->resp_len > dell_bl->resp_max_len) { >> dev_err(dell_bl->dev, "Response length too big %d > %d\n", >> dell_bl->resp_len, dell_bl->resp_max_len); >> - dell_bl->status = -EIO; >> - goto wakeup; >> + goto error; >> } >> break; >> case RESP_CMD: /* CMD byte */ >> if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { >> dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", >> dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); >> - dell_bl->status = -EIO; >> - goto wakeup; >> + goto error; >> } >> break; >> } >> + } >> >> - dell_bl->resp_idx++; >> - if (dell_bl->resp_idx < dell_bl->resp_len) >> - continue; >> - >> + if (dell_bl->resp_idx == dell_bl->resp_len) { >> csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); >> if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { >> dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", >> dell_bl->resp[dell_bl->resp_len - 1], csum); >> - dell_bl->status = -EIO; >> - goto wakeup; >> + goto error; >> } >> - >> dell_bl->status = 0; /* Success */ >> - goto wakeup; >> + wake_up(&dell_bl->wait_queue); >> + return i; >> } >> >> return len; >> >> -wakeup: >> +error: >> + dell_bl->status = -EIO; >> wake_up(&dell_bl->wait_queue); >> return i + 1; >> } > > Thanks, this is way easier to follow. I'm glad you like it. There is a little bug in this version though, the goto error on the checksum fail case returns i + i, which should be i in that case, I'll just drop the goto there and instead always use the return i already present at the end of the "if (dell_bl->resp_idx == dell_bl->resp_len) { }" block. Regards, hans >
On Mon, 13 May 2024, Hans de Goede wrote: > Hi, > > On 5/13/24 3:14 PM, Ilpo Järvinen wrote: > > On Mon, 13 May 2024, Hans de Goede wrote: > >> On 5/13/24 2:12 PM, Ilpo Järvinen wrote: > >>> On Mon, 13 May 2024, Hans de Goede wrote: > >>>> On 5/13/24 10:34 AM, Ilpo Järvinen wrote: > >>>>> On Sun, 12 May 2024, Hans de Goede wrote: > > > >>>>>> + > >>>>>> + dell_bl->resp_idx++; > >>>>>> + if (dell_bl->resp_idx < dell_bl->resp_len) > >>>>>> + continue; > >>>>>> + > >>>>>> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > >>>>>> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > >>>>>> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > >>>>>> + dell_bl->resp[dell_bl->resp_len - 1], csum); > >>>>>> + dell_bl->status = -EIO; > >>>>>> + goto wakeup; > >>>>>> + } > >>>>> > >>>>> Why is the checksum calculation and check inside the loop?? > >>>> > >>>> The loop iterates over received bytes, which may contain extra data > >>>> after the response, the: > >>>> > >>>> dell_bl->resp_idx++; > >>>> if (dell_bl->resp_idx < dell_bl->resp_len) > >>>> continue; > >>>> > >>>> continues looping until we have received all the expected bytes. So here, past this > >>>> check, we are are at the point where we have a complete response and then we verify it. > >>>> > >>>> And on successful verification wake-up any waiters. > >>> > >>> So effectively you want to terminate the loop on two conditions here: > >>> > >>> a) dell_bl->resp_idx == dell_bl->resp_len (complete frame) > >>> a) if i == len (not yet received a full frame) > >>> > >>> Why not code those rather than the current goto & continue madness? > >>> > >>> Then, after the loop, you can test: > >>> > >>> if (dell_bl->resp_idx == dell_bl->resp_len) { > >>> // calc checksum, etc. > >>> } > >>> > >>> ? > >> > >> Ok, I've added the following change for v3: > >> > >> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c > >> index bf5b12efcb19..66d8c6ddcb83 100644 > >> --- a/drivers/platform/x86/dell/dell-uart-backlight.c > >> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c > >> @@ -87,6 +87,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl, > >> 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]; > >> > >> @@ -219,7 +219,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, > >> return len; > >> } > >> > >> - for (i = 0; i < len; i++) { > >> + for (i = 0; i < len && dell_bl->resp_idx != dell_bl->resp_len; i++, dell_bl->resp_idx++) { > >> dell_bl->resp[dell_bl->resp_idx] = data[i]; > >> > >> switch (dell_bl->resp_idx) { > >> @@ -228,46 +228,41 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, > >> if (dell_bl->resp_len < MIN_RESP_LEN) { > >> dev_err(dell_bl->dev, "Response length too small %d < %d\n", > >> dell_bl->resp_len, MIN_RESP_LEN); > >> - dell_bl->status = -EIO; > >> - goto wakeup; > >> + goto error; > >> } > >> > >> if (dell_bl->resp_len > dell_bl->resp_max_len) { > >> dev_err(dell_bl->dev, "Response length too big %d > %d\n", > >> dell_bl->resp_len, dell_bl->resp_max_len); > >> - dell_bl->status = -EIO; > >> - goto wakeup; > >> + goto error; > >> } > >> break; > >> case RESP_CMD: /* CMD byte */ > >> if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { > >> dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", > >> dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); > >> - dell_bl->status = -EIO; > >> - goto wakeup; > >> + goto error; > >> } > >> break; > >> } > >> + } > >> > >> - dell_bl->resp_idx++; > >> - if (dell_bl->resp_idx < dell_bl->resp_len) > >> - continue; > >> - > >> + if (dell_bl->resp_idx == dell_bl->resp_len) { > >> csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > >> if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > >> dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > >> dell_bl->resp[dell_bl->resp_len - 1], csum); > >> - dell_bl->status = -EIO; > >> - goto wakeup; > >> + goto error; > >> } > >> - > >> dell_bl->status = 0; /* Success */ > >> - goto wakeup; > >> + wake_up(&dell_bl->wait_queue); > >> + return i; > >> } > >> > >> return len; > >> > >> -wakeup: > >> +error: > >> + dell_bl->status = -EIO; > >> wake_up(&dell_bl->wait_queue); > >> return i + 1; > >> } > > > > Thanks, this is way easier to follow. > > I'm glad you like it. > > There is a little bug in this version though, the goto error on the checksum fail > case returns i + i, which should be i in that case, I'll just drop the goto there and > instead always use the return i already present at the end of the > "if (dell_bl->resp_idx == dell_bl->resp_len) { }" block. It could have been solved more logically incrementing i and resp_idx here: dell_bl->resp[dell_bl->resp_idx] = data[i]; dell_bl->resp_idx++; i++; so that the inconsistent state is eliminated. I also realized (I know I was the one who suggested it) that reverse logic would be better for the incomplete frame check: if (dell_bl->resp_idx < dell_bl->resp_len) return len; // checksum logic... Perhaps the success and error return paths could then be merged too.
Hi, On 5/13/24 3:34 PM, Ilpo Järvinen wrote: > On Mon, 13 May 2024, Hans de Goede wrote: > >> Hi, >> >> On 5/13/24 3:14 PM, Ilpo Järvinen wrote: >>> On Mon, 13 May 2024, Hans de Goede wrote: >>>> On 5/13/24 2:12 PM, Ilpo Järvinen wrote: >>>>> On Mon, 13 May 2024, Hans de Goede wrote: >>>>>> On 5/13/24 10:34 AM, Ilpo Järvinen wrote: >>>>>>> On Sun, 12 May 2024, Hans de Goede wrote: >>> >>>>>>>> + >>>>>>>> + dell_bl->resp_idx++; >>>>>>>> + if (dell_bl->resp_idx < dell_bl->resp_len) >>>>>>>> + continue; >>>>>>>> + >>>>>>>> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); >>>>>>>> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { >>>>>>>> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", >>>>>>>> + dell_bl->resp[dell_bl->resp_len - 1], csum); >>>>>>>> + dell_bl->status = -EIO; >>>>>>>> + goto wakeup; >>>>>>>> + } >>>>>>> >>>>>>> Why is the checksum calculation and check inside the loop?? >>>>>> >>>>>> The loop iterates over received bytes, which may contain extra data >>>>>> after the response, the: >>>>>> >>>>>> dell_bl->resp_idx++; >>>>>> if (dell_bl->resp_idx < dell_bl->resp_len) >>>>>> continue; >>>>>> >>>>>> continues looping until we have received all the expected bytes. So here, past this >>>>>> check, we are are at the point where we have a complete response and then we verify it. >>>>>> >>>>>> And on successful verification wake-up any waiters. >>>>> >>>>> So effectively you want to terminate the loop on two conditions here: >>>>> >>>>> a) dell_bl->resp_idx == dell_bl->resp_len (complete frame) >>>>> a) if i == len (not yet received a full frame) >>>>> >>>>> Why not code those rather than the current goto & continue madness? >>>>> >>>>> Then, after the loop, you can test: >>>>> >>>>> if (dell_bl->resp_idx == dell_bl->resp_len) { >>>>> // calc checksum, etc. >>>>> } >>>>> >>>>> ? >>>> >>>> Ok, I've added the following change for v3: >>>> >>>> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c >>>> index bf5b12efcb19..66d8c6ddcb83 100644 >>>> --- a/drivers/platform/x86/dell/dell-uart-backlight.c >>>> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c >>>> @@ -87,6 +87,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl, >>>> 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]; >>>> >>>> @@ -219,7 +219,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, >>>> return len; >>>> } >>>> >>>> - for (i = 0; i < len; i++) { >>>> + for (i = 0; i < len && dell_bl->resp_idx != dell_bl->resp_len; i++, dell_bl->resp_idx++) { >>>> dell_bl->resp[dell_bl->resp_idx] = data[i]; >>>> >>>> switch (dell_bl->resp_idx) { >>>> @@ -228,46 +228,41 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, >>>> if (dell_bl->resp_len < MIN_RESP_LEN) { >>>> dev_err(dell_bl->dev, "Response length too small %d < %d\n", >>>> dell_bl->resp_len, MIN_RESP_LEN); >>>> - dell_bl->status = -EIO; >>>> - goto wakeup; >>>> + goto error; >>>> } >>>> >>>> if (dell_bl->resp_len > dell_bl->resp_max_len) { >>>> dev_err(dell_bl->dev, "Response length too big %d > %d\n", >>>> dell_bl->resp_len, dell_bl->resp_max_len); >>>> - dell_bl->status = -EIO; >>>> - goto wakeup; >>>> + goto error; >>>> } >>>> break; >>>> case RESP_CMD: /* CMD byte */ >>>> if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { >>>> dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", >>>> dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); >>>> - dell_bl->status = -EIO; >>>> - goto wakeup; >>>> + goto error; >>>> } >>>> break; >>>> } >>>> + } >>>> >>>> - dell_bl->resp_idx++; >>>> - if (dell_bl->resp_idx < dell_bl->resp_len) >>>> - continue; >>>> - >>>> + if (dell_bl->resp_idx == dell_bl->resp_len) { >>>> csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); >>>> if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { >>>> dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", >>>> dell_bl->resp[dell_bl->resp_len - 1], csum); >>>> - dell_bl->status = -EIO; >>>> - goto wakeup; >>>> + goto error; >>>> } >>>> - >>>> dell_bl->status = 0; /* Success */ >>>> - goto wakeup; >>>> + wake_up(&dell_bl->wait_queue); >>>> + return i; >>>> } >>>> >>>> return len; >>>> >>>> -wakeup: >>>> +error: >>>> + dell_bl->status = -EIO; >>>> wake_up(&dell_bl->wait_queue); >>>> return i + 1; >>>> } >>> >>> Thanks, this is way easier to follow. >> >> I'm glad you like it. >> >> There is a little bug in this version though, the goto error on the checksum fail >> case returns i + i, which should be i in that case, I'll just drop the goto there and >> instead always use the return i already present at the end of the >> "if (dell_bl->resp_idx == dell_bl->resp_len) { }" block. > > It could have been solved more logically incrementing i and resp_idx here: > > dell_bl->resp[dell_bl->resp_idx] = data[i]; > dell_bl->resp_idx++; > i++; > > so that the inconsistent state is eliminated. > > I also realized (I know I was the one who suggested it) that reverse logic > would be better for the incomplete frame check: > > if (dell_bl->resp_idx < dell_bl->resp_len) > return len; > > // checksum logic... > > Perhaps the success and error return paths could then be merged too. Interesting suggestion, I also realized that the 2 response-length checks are a range check so I've folded those together. So here is what I have now for v3, note that the i++ is now done when copying data over: i = 0; while (i < len && dell_bl->resp_idx != dell_bl->resp_len) { dell_bl->resp[dell_bl->resp_idx] = data[i++]; switch (dell_bl->resp_idx) { case RESP_LEN: /* Length byte */ dell_bl->resp_len = dell_bl->resp[RESP_LEN]; if (!in_range(dell_bl->resp_len, MIN_RESP_LEN, dell_bl->resp_max_len)) { dev_err(dell_bl->dev, "Response length %d out if range %d - %d\n", dell_bl->resp_len, MIN_RESP_LEN, dell_bl->resp_max_len); dell_bl->status = -EIO; goto wakeup; } break; case RESP_CMD: /* CMD byte */ if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); dell_bl->status = -EIO; goto wakeup; } break; } dell_bl->resp_idx++; } if (dell_bl->resp_idx != dell_bl->resp_len) return len; /* Response not complete yet */ csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); if (dell_bl->resp[dell_bl->resp_len - 1] == csum) { dell_bl->status = 0; /* Success */ } else { dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", dell_bl->resp[dell_bl->resp_len - 1], csum); dell_bl->status = -EIO; } wakeup: wake_up(&dell_bl->wait_queue); return i; } Regards, Hans
On Mon, 13 May 2024, Hans de Goede wrote: > On 5/13/24 3:34 PM, Ilpo Järvinen wrote: > > On Mon, 13 May 2024, Hans de Goede wrote: > >> On 5/13/24 3:14 PM, Ilpo Järvinen wrote: > >>> On Mon, 13 May 2024, Hans de Goede wrote: > >>>> On 5/13/24 2:12 PM, Ilpo Järvinen wrote: > >>>>> On Mon, 13 May 2024, Hans de Goede wrote: > >>>>>> On 5/13/24 10:34 AM, Ilpo Järvinen wrote: > >>>>>>> On Sun, 12 May 2024, Hans de Goede wrote: > >>> > >>>>>>>> + > >>>>>>>> + dell_bl->resp_idx++; > >>>>>>>> + if (dell_bl->resp_idx < dell_bl->resp_len) > >>>>>>>> + continue; > >>>>>>>> + > >>>>>>>> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > >>>>>>>> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > >>>>>>>> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > >>>>>>>> + dell_bl->resp[dell_bl->resp_len - 1], csum); > >>>>>>>> + dell_bl->status = -EIO; > >>>>>>>> + goto wakeup; > >>>>>>>> + } > >>>>>>> > >>>>>>> Why is the checksum calculation and check inside the loop?? > >>>>>> > >>>>>> The loop iterates over received bytes, which may contain extra data > >>>>>> after the response, the: > >>>>>> > >>>>>> dell_bl->resp_idx++; > >>>>>> if (dell_bl->resp_idx < dell_bl->resp_len) > >>>>>> continue; > >>>>>> > >>>>>> continues looping until we have received all the expected bytes. So here, past this > >>>>>> check, we are are at the point where we have a complete response and then we verify it. > >>>>>> > >>>>>> And on successful verification wake-up any waiters. > >>>>> > >>>>> So effectively you want to terminate the loop on two conditions here: > >>>>> > >>>>> a) dell_bl->resp_idx == dell_bl->resp_len (complete frame) > >>>>> a) if i == len (not yet received a full frame) > >>>>> > >>>>> Why not code those rather than the current goto & continue madness? > >>>>> > >>>>> Then, after the loop, you can test: > >>>>> > >>>>> if (dell_bl->resp_idx == dell_bl->resp_len) { > >>>>> // calc checksum, etc. > >>>>> } > >>>>> > >>>>> ? > >>>> > >>>> Ok, I've added the following change for v3: > >>>> > >>>> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c > >>>> index bf5b12efcb19..66d8c6ddcb83 100644 > >>>> --- a/drivers/platform/x86/dell/dell-uart-backlight.c > >>>> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c > >>>> @@ -87,6 +87,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl, > >>>> 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]; > >>>> > >>>> @@ -219,7 +219,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, > >>>> return len; > >>>> } > >>>> > >>>> - for (i = 0; i < len; i++) { > >>>> + for (i = 0; i < len && dell_bl->resp_idx != dell_bl->resp_len; i++, dell_bl->resp_idx++) { > >>>> dell_bl->resp[dell_bl->resp_idx] = data[i]; > >>>> > >>>> switch (dell_bl->resp_idx) { > >>>> @@ -228,46 +228,41 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, > >>>> if (dell_bl->resp_len < MIN_RESP_LEN) { > >>>> dev_err(dell_bl->dev, "Response length too small %d < %d\n", > >>>> dell_bl->resp_len, MIN_RESP_LEN); > >>>> - dell_bl->status = -EIO; > >>>> - goto wakeup; > >>>> + goto error; > >>>> } > >>>> > >>>> if (dell_bl->resp_len > dell_bl->resp_max_len) { > >>>> dev_err(dell_bl->dev, "Response length too big %d > %d\n", > >>>> dell_bl->resp_len, dell_bl->resp_max_len); > >>>> - dell_bl->status = -EIO; > >>>> - goto wakeup; > >>>> + goto error; > >>>> } > >>>> break; > >>>> case RESP_CMD: /* CMD byte */ > >>>> if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { > >>>> dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", > >>>> dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); > >>>> - dell_bl->status = -EIO; > >>>> - goto wakeup; > >>>> + goto error; > >>>> } > >>>> break; > >>>> } > >>>> + } > >>>> > >>>> - dell_bl->resp_idx++; > >>>> - if (dell_bl->resp_idx < dell_bl->resp_len) > >>>> - continue; > >>>> - > >>>> + if (dell_bl->resp_idx == dell_bl->resp_len) { > >>>> csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); > >>>> if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { > >>>> dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", > >>>> dell_bl->resp[dell_bl->resp_len - 1], csum); > >>>> - dell_bl->status = -EIO; > >>>> - goto wakeup; > >>>> + goto error; > >>>> } > >>>> - > >>>> dell_bl->status = 0; /* Success */ > >>>> - goto wakeup; > >>>> + wake_up(&dell_bl->wait_queue); > >>>> + return i; > >>>> } > >>>> > >>>> return len; > >>>> > >>>> -wakeup: > >>>> +error: > >>>> + dell_bl->status = -EIO; > >>>> wake_up(&dell_bl->wait_queue); > >>>> return i + 1; > >>>> } > >>> > >>> Thanks, this is way easier to follow. > >> > >> I'm glad you like it. > >> > >> There is a little bug in this version though, the goto error on the checksum fail > >> case returns i + i, which should be i in that case, I'll just drop the goto there and > >> instead always use the return i already present at the end of the > >> "if (dell_bl->resp_idx == dell_bl->resp_len) { }" block. > > > > It could have been solved more logically incrementing i and resp_idx here: > > > > dell_bl->resp[dell_bl->resp_idx] = data[i]; > > dell_bl->resp_idx++; > > i++; > > > > so that the inconsistent state is eliminated. > > > > I also realized (I know I was the one who suggested it) that reverse logic > > would be better for the incomplete frame check: > > > > if (dell_bl->resp_idx < dell_bl->resp_len) > > return len; > > > > // checksum logic... > > > > Perhaps the success and error return paths could then be merged too. > > Interesting suggestion, I also realized that the 2 response-length checks are a > range check so I've folded those together. So here is what I have now for v3, > note that the i++ is now done when copying data over: > > i = 0; > while (i < len && dell_bl->resp_idx != dell_bl->resp_len) { > dell_bl->resp[dell_bl->resp_idx] = data[i++]; > > switch (dell_bl->resp_idx) { > case RESP_LEN: /* Length byte */ > dell_bl->resp_len = dell_bl->resp[RESP_LEN]; > if (!in_range(dell_bl->resp_len, MIN_RESP_LEN, dell_bl->resp_max_len)) { in_range() takes start and len, not start and end. I really hate that helper because it has that trap and would often require "+ min" to be added. > dev_err(dell_bl->dev, "Response length %d out if range %d - %d\n", > dell_bl->resp_len, MIN_RESP_LEN, dell_bl->resp_max_len); > dell_bl->status = -EIO; > goto wakeup; > } > break; > case RESP_CMD: /* CMD byte */ > if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { > dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", > dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); > dell_bl->status = -EIO; > goto wakeup; > } > break; > } > dell_bl->resp_idx++; Good, I didn't realize the switch used the index.
Hi, On 5/13/24 4:36 PM, Ilpo Järvinen wrote: > On Mon, 13 May 2024, Hans de Goede wrote: >> On 5/13/24 3:34 PM, Ilpo Järvinen wrote: >>> On Mon, 13 May 2024, Hans de Goede wrote: >>>> On 5/13/24 3:14 PM, Ilpo Järvinen wrote: >>>>> On Mon, 13 May 2024, Hans de Goede wrote: >>>>>> On 5/13/24 2:12 PM, Ilpo Järvinen wrote: >>>>>>> On Mon, 13 May 2024, Hans de Goede wrote: >>>>>>>> On 5/13/24 10:34 AM, Ilpo Järvinen wrote: >>>>>>>>> On Sun, 12 May 2024, Hans de Goede wrote: >>>>> >>>>>>>>>> + >>>>>>>>>> + dell_bl->resp_idx++; >>>>>>>>>> + if (dell_bl->resp_idx < dell_bl->resp_len) >>>>>>>>>> + continue; >>>>>>>>>> + >>>>>>>>>> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); >>>>>>>>>> + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { >>>>>>>>>> + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", >>>>>>>>>> + dell_bl->resp[dell_bl->resp_len - 1], csum); >>>>>>>>>> + dell_bl->status = -EIO; >>>>>>>>>> + goto wakeup; >>>>>>>>>> + } >>>>>>>>> >>>>>>>>> Why is the checksum calculation and check inside the loop?? >>>>>>>> >>>>>>>> The loop iterates over received bytes, which may contain extra data >>>>>>>> after the response, the: >>>>>>>> >>>>>>>> dell_bl->resp_idx++; >>>>>>>> if (dell_bl->resp_idx < dell_bl->resp_len) >>>>>>>> continue; >>>>>>>> >>>>>>>> continues looping until we have received all the expected bytes. So here, past this >>>>>>>> check, we are are at the point where we have a complete response and then we verify it. >>>>>>>> >>>>>>>> And on successful verification wake-up any waiters. >>>>>>> >>>>>>> So effectively you want to terminate the loop on two conditions here: >>>>>>> >>>>>>> a) dell_bl->resp_idx == dell_bl->resp_len (complete frame) >>>>>>> a) if i == len (not yet received a full frame) >>>>>>> >>>>>>> Why not code those rather than the current goto & continue madness? >>>>>>> >>>>>>> Then, after the loop, you can test: >>>>>>> >>>>>>> if (dell_bl->resp_idx == dell_bl->resp_len) { >>>>>>> // calc checksum, etc. >>>>>>> } >>>>>>> >>>>>>> ? >>>>>> >>>>>> Ok, I've added the following change for v3: >>>>>> >>>>>> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c >>>>>> index bf5b12efcb19..66d8c6ddcb83 100644 >>>>>> --- a/drivers/platform/x86/dell/dell-uart-backlight.c >>>>>> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c >>>>>> @@ -87,6 +87,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl, >>>>>> 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]; >>>>>> >>>>>> @@ -219,7 +219,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, >>>>>> return len; >>>>>> } >>>>>> >>>>>> - for (i = 0; i < len; i++) { >>>>>> + for (i = 0; i < len && dell_bl->resp_idx != dell_bl->resp_len; i++, dell_bl->resp_idx++) { >>>>>> dell_bl->resp[dell_bl->resp_idx] = data[i]; >>>>>> >>>>>> switch (dell_bl->resp_idx) { >>>>>> @@ -228,46 +228,41 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, >>>>>> if (dell_bl->resp_len < MIN_RESP_LEN) { >>>>>> dev_err(dell_bl->dev, "Response length too small %d < %d\n", >>>>>> dell_bl->resp_len, MIN_RESP_LEN); >>>>>> - dell_bl->status = -EIO; >>>>>> - goto wakeup; >>>>>> + goto error; >>>>>> } >>>>>> >>>>>> if (dell_bl->resp_len > dell_bl->resp_max_len) { >>>>>> dev_err(dell_bl->dev, "Response length too big %d > %d\n", >>>>>> dell_bl->resp_len, dell_bl->resp_max_len); >>>>>> - dell_bl->status = -EIO; >>>>>> - goto wakeup; >>>>>> + goto error; >>>>>> } >>>>>> break; >>>>>> case RESP_CMD: /* CMD byte */ >>>>>> if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) { >>>>>> dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", >>>>>> dell_bl->resp[RESP_CMD], dell_bl->pending_cmd); >>>>>> - dell_bl->status = -EIO; >>>>>> - goto wakeup; >>>>>> + goto error; >>>>>> } >>>>>> break; >>>>>> } >>>>>> + } >>>>>> >>>>>> - dell_bl->resp_idx++; >>>>>> - if (dell_bl->resp_idx < dell_bl->resp_len) >>>>>> - continue; >>>>>> - >>>>>> + if (dell_bl->resp_idx == dell_bl->resp_len) { >>>>>> csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); >>>>>> if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { >>>>>> dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", >>>>>> dell_bl->resp[dell_bl->resp_len - 1], csum); >>>>>> - dell_bl->status = -EIO; >>>>>> - goto wakeup; >>>>>> + goto error; >>>>>> } >>>>>> - >>>>>> dell_bl->status = 0; /* Success */ >>>>>> - goto wakeup; >>>>>> + wake_up(&dell_bl->wait_queue); >>>>>> + return i; >>>>>> } >>>>>> >>>>>> return len; >>>>>> >>>>>> -wakeup: >>>>>> +error: >>>>>> + dell_bl->status = -EIO; >>>>>> wake_up(&dell_bl->wait_queue); >>>>>> return i + 1; >>>>>> } >>>>> >>>>> Thanks, this is way easier to follow. >>>> >>>> I'm glad you like it. >>>> >>>> There is a little bug in this version though, the goto error on the checksum fail >>>> case returns i + i, which should be i in that case, I'll just drop the goto there and >>>> instead always use the return i already present at the end of the >>>> "if (dell_bl->resp_idx == dell_bl->resp_len) { }" block. >>> >>> It could have been solved more logically incrementing i and resp_idx here: >>> >>> dell_bl->resp[dell_bl->resp_idx] = data[i]; >>> dell_bl->resp_idx++; >>> i++; >>> >>> so that the inconsistent state is eliminated. >>> >>> I also realized (I know I was the one who suggested it) that reverse logic >>> would be better for the incomplete frame check: >>> >>> if (dell_bl->resp_idx < dell_bl->resp_len) >>> return len; >>> >>> // checksum logic... >>> >>> Perhaps the success and error return paths could then be merged too. >> >> Interesting suggestion, I also realized that the 2 response-length checks are a >> range check so I've folded those together. So here is what I have now for v3, >> note that the i++ is now done when copying data over: >> >> i = 0; >> while (i < len && dell_bl->resp_idx != dell_bl->resp_len) { >> dell_bl->resp[dell_bl->resp_idx] = data[i++]; >> >> switch (dell_bl->resp_idx) { >> case RESP_LEN: /* Length byte */ >> dell_bl->resp_len = dell_bl->resp[RESP_LEN]; >> if (!in_range(dell_bl->resp_len, MIN_RESP_LEN, dell_bl->resp_max_len)) { > > in_range() takes start and len, not start and end. I really hate that > helper because it has that trap and would often require "+ min" to be > added. Thank you for caching that. I'll just switch to open coding the range check then for v3. Regards, Hans
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..d3ffea9e6270 --- /dev/null +++ b/drivers/platform/x86/dell/dell-uart-backlight.c @@ -0,0 +1,409 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Dell AIO Serial Backlight Driver + * + * Copyright (C) 2024 Hans de Goede <hansg@kernel.org> + * Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com> + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/acpi.h> +#include <linux/backlight.h> +#include <linux/delay.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/platform_device.h> +#include <linux/serdev.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_MIN_RESP_SIZE 3 + +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 = 0; + dell_bl->resp = resp; + dell_bl->resp_idx = 0; + 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; + goto out; + } + + ret = wait_event_timeout(dell_bl->wait_queue, dell_bl->status, DELL_BL_TIMEOUT); + if (ret == 0) { + dev_err(dell_bl->dev, "Timed out waiting for response.\n"); + dell_bl->status = -ETIMEDOUT; + } + + if (dell_bl->status == 1) + ret = 0; + else + ret = dell_bl->status; + +out: + mutex_unlock(&dell_bl->mutex); + return ret; +} + +static int dell_uart_set_brightness(struct dell_uart_backlight *dell_bl, int brightness) +{ + /* + * Set Brightness level: Application uses this command to set brightness. + * Command: 0x8A 0x0B <brightness-level> Checksum (Length:4 Type:0x0A Cmd:0x0B) + * <brightness-level> ranges from 0~100. + * Return data: 0x03 0x0B 0xF1 (Length:3 Cmd:0x0B Checksum:0xF1) + */ + u8 set_brightness[] = { 0x8A, 0x0B, 0x00, 0x00 }; + u8 resp[3]; + + set_brightness[2] = brightness; + set_brightness[3] = dell_uart_checksum(set_brightness, 3); + + return dell_uart_bl_command(dell_bl, set_brightness, ARRAY_SIZE(set_brightness), + resp, ARRAY_SIZE(resp)); +} + +static int dell_uart_get_brightness(struct dell_uart_backlight *dell_bl) +{ + /* + * Get Brightness level: Application uses this command to get brightness. + * Command: 0x6A 0x0C 0x89 (Length:3 Type:0x0A Cmd:0x0C Checksum:0x89) + * Return data: 0x04 0x0C Data Checksum + * (Length:4 Cmd:0x0C Data:<brightness level> + * Checksum: SUM(Length and Cmd and Data) xor 0xFF) + * <brightness level> ranges from 0~100. + */ + const u8 get_brightness[] = { 0x6A, 0x0C, 0x89 }; + u8 resp[4]; + int ret; + + ret = dell_uart_bl_command(dell_bl, get_brightness, ARRAY_SIZE(get_brightness), + resp, ARRAY_SIZE(resp)); + if (ret) + return ret; + + if (resp[0] != 4) { + dev_err(dell_bl->dev, "Unexpected get brightness response length: %d\n", resp[0]); + return -EIO; + } + + if (resp[2] > 100) { + dev_err(dell_bl->dev, "Unexpected get brightness response: %d\n", resp[2]); + return -EIO; + } + + return resp[2]; +} + +static int dell_uart_set_bl_power(struct dell_uart_backlight *dell_bl, int power) +{ + /* + * Screen ON/OFF Control: Application uses this command to control screen ON or OFF. + * Command: 0x8A 0x0E Data Checksum (Length:4 Type:0x0A Cmd:0x0E) where + * Data=0 to turn OFF the screen. + * Data=1 to turn ON the screen. + * Other value of Data is reserved and invalid. + * Return data: 0x03 0x0E 0xEE (Length:3 Cmd:0x0E Checksum:0xEE) + */ + u8 set_power[] = { 0x8A, 0x0E, 0x00, 0x00 }; + u8 resp[3]; + int ret; + + set_power[2] = (power == FB_BLANK_UNBLANK) ? 1 : 0; + set_power[3] = dell_uart_checksum(set_power, 3); + + ret = dell_uart_bl_command(dell_bl, set_power, ARRAY_SIZE(set_power), + resp, ARRAY_SIZE(resp)); + if (ret) + return ret; + + dell_bl->power = power; + return 0; +} + +/* + * There is no command to get backlight power status, + * so we set the backlight power to "on" while initializing, + * and then track and report its status by power variable + */ +static int dell_uart_get_bl_power(struct dell_uart_backlight *dell_bl) +{ + return dell_bl->power; +} + +static int dell_uart_update_status(struct backlight_device *bd) +{ + struct dell_uart_backlight *dell_bl = bl_get_data(bd); + int ret; + + ret = dell_uart_set_brightness(dell_bl, bd->props.brightness); + if (ret) + return ret; + + if (bd->props.power != dell_uart_get_bl_power(dell_bl)) + ret = dell_uart_set_bl_power(dell_bl, bd->props.power); + + return ret; +} + +static int dell_uart_get_brightness_op(struct backlight_device *bd) +{ + return dell_uart_get_brightness(bl_get_data(bd)); +} + +static const struct backlight_ops dell_uart_backlight_ops = { + .update_status = dell_uart_update_status, + .get_brightness = dell_uart_get_brightness_op, +}; + +static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, size_t len) +{ + struct dell_uart_backlight *dell_bl = serdev_device_get_drvdata(serdev); + size_t i; + u8 csum; + + dev_dbg(dell_bl->dev, "Recv: %*ph\n", (int)len, data); + + /* Throw away unexpected bytes / remainder of response after an error */ + if (dell_bl->status) { + dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n"); + return len; + } + + for (i = 0; i < len; i++) { + dell_bl->resp[dell_bl->resp_idx] = data[i]; + + switch (dell_bl->resp_idx) { + case 0: /* Length byte */ + dell_bl->resp_len = dell_bl->resp[0]; + if (dell_bl->resp_len < DELL_BL_MIN_RESP_SIZE) { + dev_err(dell_bl->dev, "Response length too small %d < %d\n", + dell_bl->resp_len, DELL_BL_MIN_RESP_SIZE); + dell_bl->status = -EIO; + goto wakeup; + } else if (dell_bl->resp_len > dell_bl->resp_max_len) { + dev_err(dell_bl->dev, "Response length too big %d > %d\n", + dell_bl->resp_len, dell_bl->resp_max_len); + dell_bl->status = -EIO; + goto wakeup; + } + break; + case 1: /* CMD byte */ + if (dell_bl->resp[1] != dell_bl->pending_cmd) { + dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n", + dell_bl->resp[1], dell_bl->pending_cmd); + dell_bl->status = -EIO; + goto wakeup; + } + break; + } + + dell_bl->resp_idx++; + if (dell_bl->resp_idx < dell_bl->resp_len) + continue; + + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1); + if (dell_bl->resp[dell_bl->resp_len - 1] != csum) { + dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n", + dell_bl->resp[dell_bl->resp_len - 1], csum); + dell_bl->status = -EIO; + goto wakeup; + } + + dell_bl->status = 1; /* Success */ + goto wakeup; + } + + return len; + +wakeup: + wake_up(&dell_bl->wait_queue); + return i + 1; +} + +static const struct serdev_device_ops dell_uart_bl_serdev_ops = { + .receive_buf = dell_uart_bl_receive, + .write_wakeup = serdev_device_write_wakeup, +}; + +static int dell_uart_bl_serdev_probe(struct serdev_device *serdev) +{ + /* + * Get Firmware Version: Tool uses this command to get firmware version. + * Command: 0x6A 0x06 0x8F (Length:3 Type:0x0A Cmd:6 Checksum:0x8F) + * Return data: 0x0D 0x06 Data Checksum (Length:13 Cmd:0x06 + * Data:F/W version(APRILIA=APR27-Vxxx/PHINE=PHI23-Vxxx) + * Checksum:SUM(Length and Cmd and Data) xor 0xFF) + */ + const u8 get_firmware_ver[] = { 0x6A, 0x06, 0x8F }; + struct dell_uart_backlight *dell_bl; + struct backlight_properties props; + struct device *dev = &serdev->dev; + u8 get_firmware_ver_resp[80]; + int ret; + + dell_bl = devm_kzalloc(dev, sizeof(*dell_bl), GFP_KERNEL); + if (!dell_bl) + return -ENOMEM; + + mutex_init(&dell_bl->mutex); + init_waitqueue_head(&dell_bl->wait_queue); + dell_bl->dev = dev; + + ret = devm_serdev_device_open(dev, serdev); + if (ret) + return dev_err_probe(dev, ret, "opening UART device\n"); + + /* 9600 bps, no flow control, these are the default but set them to be sure */ + serdev_device_set_baudrate(serdev, 9600); + serdev_device_set_flow_control(serdev, false); + serdev_device_set_drvdata(serdev, dell_bl); + serdev_device_set_client_ops(serdev, &dell_uart_bl_serdev_ops); + + ret = dell_uart_bl_command(dell_bl, get_firmware_ver, ARRAY_SIZE(get_firmware_ver), + get_firmware_ver_resp, ARRAY_SIZE(get_firmware_ver_resp)); + if (ret) + return dev_err_probe(dev, ret, "getting firmware version\n"); + + dev_dbg(dev, "Firmware version: %.*s\n", get_firmware_ver_resp[0] - 3, + get_firmware_ver_resp + 2); + + /* Initialize bl_power to a known value */ + ret = dell_uart_set_bl_power(dell_bl, FB_BLANK_UNBLANK); + if (ret) + return ret; + + ret = dell_uart_get_brightness(dell_bl); + if (ret < 0) + return ret; + + memset(&props, 0, sizeof(struct backlight_properties)); + props.type = BACKLIGHT_PLATFORM; + props.brightness = ret; + props.max_brightness = 100; + props.power = dell_bl->power; + + dell_bl->bl = devm_backlight_device_register(dev, "dell_uart_backlight", + dev, dell_bl, + &dell_uart_backlight_ops, + &props); + if (IS_ERR(dell_bl->bl)) + return PTR_ERR(dell_bl->bl); + + return 0; +} + +struct serdev_device_driver dell_uart_bl_serdev_driver = { + .probe = dell_uart_bl_serdev_probe, + .driver = { + .name = KBUILD_MODNAME, + }, +}; + +static int dell_uart_bl_pdev_probe(struct platform_device *pdev) +{ + struct serdev_device *serdev; + struct device *ctrl_dev; + int ret; + + ctrl_dev = get_serdev_controller("DELL0501", NULL, 0, "serial0"); + if (IS_ERR(ctrl_dev)) + return PTR_ERR(ctrl_dev); + + serdev = serdev_device_alloc(to_serdev_controller(ctrl_dev)); + put_device(ctrl_dev); + if (!serdev) + return -ENOMEM; + + ret = serdev_device_add(serdev); + if (ret) { + dev_err(&pdev->dev, "error %d adding serdev\n", ret); + serdev_device_put(serdev); + return ret; + } + + ret = serdev_device_driver_register(&dell_uart_bl_serdev_driver); + if (ret) { + serdev_device_remove(serdev); + return ret; + } + + /* + * serdev device <-> driver matching relies on OF or ACPI matches and + * neither is available here, manually bind the driver. + */ + ret = device_driver_attach(&dell_uart_bl_serdev_driver.driver, &serdev->dev); + if (ret) { + serdev_device_driver_unregister(&dell_uart_bl_serdev_driver); + serdev_device_remove(serdev); + return ret; + } + + /* So that dell_uart_bl_pdev_remove() can remove the serdev */ + platform_set_drvdata(pdev, serdev); + return 0; +} + +static void dell_uart_bl_pdev_remove(struct platform_device *pdev) +{ + struct serdev_device *serdev = platform_get_drvdata(pdev); + + serdev_device_driver_unregister(&dell_uart_bl_serdev_driver); + serdev_device_remove(serdev); +} + +static struct platform_driver dell_uart_bl_pdev_driver = { + .probe = dell_uart_bl_pdev_probe, + .remove_new = dell_uart_bl_pdev_remove, + .driver = { + .name = "dell-uart-backlight", + }, +}; +module_platform_driver(dell_uart_bl_pdev_driver); + +MODULE_ALIAS("platform:dell-uart-backlight"); +MODULE_DESCRIPTION("Dell AIO Serial Backlight driver"); +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>"); +MODULE_LICENSE("GPL");
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. Co-developed-by: AceLan Kao <acelan.kao@canonical.com> Signed-off-by: AceLan Kao <acelan.kao@canonical.com> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/dell/Kconfig | 15 + drivers/platform/x86/dell/Makefile | 1 + .../platform/x86/dell/dell-uart-backlight.c | 409 ++++++++++++++++++ 3 files changed, 425 insertions(+) create mode 100644 drivers/platform/x86/dell/dell-uart-backlight.c