[PATCH 0/2] platform/x86: Add new Dell UART backlight driver

[PATCH 0/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

Hi All,

I recently learned that some Dell AIOs (1) use a backlight controller board
connected to an UART. Canonical even submitted a driver for this in 2017:
https://lkml.org/lkml/2017/10/26/78

This UART has a DELL0501 HID with CID set to PNP0501 so that the UART is
still handled by 8250_pnp.c. Unfortunately there is no separate ACPI device
with an UartSerialBusV2() resource to model the backlight-controller. An
ACPI quirk has been merged recently to deal with this and create a serdev
controller for the UART despite the missing UartSerialBusV2() resource:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99b572e6136eab69a8c91d72cf8595b256e304b5

Patch 1 in this series adds a driver binding to the "dell-uart-backlight"
device created by this quirk. This drivers creates a serdev-device for
the DELL0501 serdev-controller and registers a serdev backlight driver
which binds to this serdev-device.

Patch 2 contains a small emulator for the UART attached backlight
controller found on this Dell AOIs, I wrote and used this to develop
the driver since I did not have access to such an AOI myself.

This has been successfully tested by Roman Bogoyev (who originally
reported the missing driver to me by email) on a Dell Inspiron 27 7000
(7790) and by Kai-Heng Feng on a newer Dell AOI model.

Regards,

Hans

1) All In One a monitor with a PC builtin


Hans de Goede (2):
  platform/x86: Add new Dell UART backlight driver
  tools arch x86: Add dell-uart-backlight-emulator

 drivers/platform/x86/dell/Kconfig             |  15 +
 drivers/platform/x86/dell/Makefile            |   1 +
 .../platform/x86/dell/dell-uart-backlight.c   | 409 ++++++++++++++++++
 .../dell-uart-backlight-emulator/.gitignore   |   1 +
 .../x86/dell-uart-backlight-emulator/Makefile |  19 +
 .../x86/dell-uart-backlight-emulator/README   |  46 ++
 .../dell-uart-backlight-emulator.c            | 166 +++++++
 7 files changed, 657 insertions(+)
 create mode 100644 drivers/platform/x86/dell/dell-uart-backlight.c
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/.gitignore
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/Makefile
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/README
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c

-- 
2.44.0

[PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

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;
+
+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");
-- 
2.44.0

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Andy Shevchenko]

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

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

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

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

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.

-- 
 i.

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Ilpo Järvinen]

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.

-- 
 i.

> +
> +	/* 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");
> 

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

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

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5107 bytes --]

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;
> >> +}


-- 
 i.

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4585 bytes --]

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) {
		...
	}

-- 
 i.

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

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;
>>>> +}
> 
> 

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4465 bytes --]

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.

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

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


> 

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5734 bytes --]

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.

-- 
 i.

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

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

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 8029 bytes --]

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.

-- 
 i.

> 	}
> 
> 	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
> 
> 

Re: [PATCH 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

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

[PATCH 2/2] tools arch x86: Add dell-uart-backlight-emulator

[Hans de Goede]

Dell All In One (AIO) models released after 2017 use a backlight controller
board connected to an UART.

Add a small emulator to allow development and testing of
the drivers/platform/x86/dell/dell-uart-backlight.c driver for
this board, without requiring access to an actual Dell All In One.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../dell-uart-backlight-emulator/.gitignore   |   1 +
 .../x86/dell-uart-backlight-emulator/Makefile |  19 ++
 .../x86/dell-uart-backlight-emulator/README   |  46 +++++
 .../dell-uart-backlight-emulator.c            | 166 ++++++++++++++++++
 4 files changed, 232 insertions(+)
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/.gitignore
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/Makefile
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/README
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c

diff --git a/tools/arch/x86/dell-uart-backlight-emulator/.gitignore b/tools/arch/x86/dell-uart-backlight-emulator/.gitignore
new file mode 100644
index 000000000000..5c8cad8d72b9
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/.gitignore
@@ -0,0 +1 @@
+dell-uart-backlight-emulator
diff --git a/tools/arch/x86/dell-uart-backlight-emulator/Makefile b/tools/arch/x86/dell-uart-backlight-emulator/Makefile
new file mode 100644
index 000000000000..6ea1d9fd534b
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/Makefile
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0
+# Makefile for Intel Software Defined Silicon provisioning tool
+
+dell-uart-backlight-emulator: dell-uart-backlight-emulator.c
+
+BINDIR ?= /usr/bin
+
+override CFLAGS += -O2 -Wall
+
+%: %.c
+	$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS)
+
+.PHONY : clean
+clean :
+	@rm -f dell-uart-backlight-emulator
+
+install : dell-uart-backlight-emulator
+	install -d $(DESTDIR)$(BINDIR)
+	install -m 755 -p dell-uart-backlight-emulator $(DESTDIR)$(BINDIR)/dell-uart-backlight-emulator
diff --git a/tools/arch/x86/dell-uart-backlight-emulator/README b/tools/arch/x86/dell-uart-backlight-emulator/README
new file mode 100644
index 000000000000..c0d8e52046ee
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/README
@@ -0,0 +1,46 @@
+Emulator for DELL0501 UART attached backlight controller
+--------------------------------------------------------
+
+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")
+
+With the DELL0501 indicating that we are dealing with an UART with
+the backlight controller board attached.
+
+This small emulator allows testing
+the drivers/platform/x86/dell/dell-uart-backlight.c driver without access
+to an actual Dell All In One.
+
+This requires:
+1. A (desktop) PC with a 16550 UART on the motherboard and a standard DB9
+   connector connected to this UART.
+2. A DB9 NULL modem cable.
+3. A second DB9 serial port, this can e.g. be a USB to serial converter
+   with a DB9 connector plugged into the same desktop PC.
+4. A DSDT overlay for the desktop PC replacing the _HID of the 16550 UART
+   ACPI Device() with "DELL0501" and adding a _CID of "PNP0501", see
+   DSDT.patch for an example of the necessary DSDT changes.
+
+With everything setup and the NULL modem cable connected between
+the 2 serial ports run:
+
+./dell-uart-backlight-emulator <path-to-/dev/tty*S#-for-second-port>
+
+For example when using an USB to serial converter for the second port:
+
+./dell-uart-backlight-emulator /dev/ttyUSB0
+
+And then (re)load the dell-uart-backlight driver:
+
+sudo rmmod dell-uart-backlight; sudo modprobe dell-uart-backlight dyndbg
+
+After this check "dmesg" to see if the driver correctly received
+the firmware version string from the emulator. If this works there
+should be a /sys/class/backlight/dell_uart_backlight/ directory now
+and writes to the brightness or bl_power files should be reflected
+by matching output from the emulator.
diff --git a/tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c b/tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c
new file mode 100644
index 000000000000..35c77ca3c695
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Dell AIO Serial Backlight board emulator for testing
+ * the Linux dell-uart-backlight driver.
+ *
+ * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
+ */
+#include <errno.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/un.h>
+#include <termios.h>
+#include <unistd.h>
+
+int serial_fd;
+int brightness = 50;
+
+static unsigned char dell_uart_checksum(unsigned char *buf, int len)
+{
+	unsigned char val = 0;
+
+	while (len-- > 0)
+		val += buf[len];
+
+	return val ^ 0xff;
+}
+
+/* read() will return -1 on SIGINT / SIGTERM causing the mainloop to cleanly exit */
+void signalhdlr(int signum)
+{
+}
+
+int main(int argc, char *argv[])
+{
+	struct sigaction sigact = { .sa_handler = signalhdlr };
+	unsigned char buf[4], csum, response[32];
+	const char *version_str = "PHI23-V321";
+	struct termios tty, saved_tty;
+	int ret, idx, len = 0;
+
+	if (argc != 2) {
+		fprintf(stderr, "Invalid or missing arguments\n");
+		fprintf(stderr, "Usage: %s <serial-port>\n", argv[0]);
+		return 1;
+	}
+
+	serial_fd = open(argv[1], O_RDWR | O_NOCTTY);
+	if (serial_fd == -1) {
+		fprintf(stderr, "Error opening %s: %s\n", argv[1], strerror(errno));
+		return 1;
+	}
+
+	ret = tcgetattr(serial_fd, &tty);
+	if (ret == -1) {
+		fprintf(stderr, "Error getting tcattr: %s\n", strerror(errno));
+		goto out_close;
+	}
+	saved_tty = tty;
+
+	cfsetspeed(&tty, 9600);
+	cfmakeraw(&tty);
+	tty.c_cflag &= ~CSTOPB;
+	tty.c_cflag &= ~CRTSCTS;
+	tty.c_cflag |= CLOCAL | CREAD;
+
+	ret = tcsetattr(serial_fd, TCSANOW, &tty);
+	if (ret == -1) {
+		fprintf(stderr, "Error setting tcattr: %s\n", strerror(errno));
+		goto out_restore;
+	}
+
+	sigaction(SIGINT, &sigact, 0);
+	sigaction(SIGTERM, &sigact, 0);
+
+	idx = 0;
+	while (read(serial_fd, &buf[idx], 1) == 1) {
+		if (idx == 0) {
+			switch (buf[0]) {
+			/* 3 MSB bits: cmd-len + 01010 SOF marker */
+			case 0x6a: len = 3; break;
+			case 0x8a: len = 4; break;
+			default:
+				fprintf(stderr, "Error unexpected first byte: 0x%02x\n", buf[0]);
+				continue; /* Try to sync up with sender */
+			}
+		}
+
+		/* Process msg when len bytes have been received */
+		if (idx != (len - 1)) {
+			idx++;
+			continue;
+		}
+
+		/* Reset idx for next command */
+		idx = 0;
+
+		csum = dell_uart_checksum(buf, len - 1);
+		if (buf[len - 1] != csum) {
+			fprintf(stderr, "Error checksum mismatch got 0x%02x expected 0x%02x\n",
+				buf[len - 1], csum);
+			continue;
+		}
+
+		switch ((buf[0] << 8) | buf[1]) {
+		case 0x6a06:
+			/* cmd = 0x06, get version */
+			len = strlen(version_str);
+			strcpy((char *)&response[2], version_str);
+			printf("Get version, reply: %s\n", version_str);
+			break;
+		case 0x8a0b: /* 3 MSB bits: cmd-len + 01010 SOF marker */
+			/* cmd = 0x0b, set brightness */
+			if (buf[2] > 100) {
+				fprintf(stderr, "Error invalid brightness param: %d\n", buf[2]);
+				continue;
+			}
+
+			len = 0;
+			brightness = buf[2];
+			printf("Set brightness %d\n", brightness);
+			break;
+		case 0x6a0c:
+			/* cmd = 0x0c, get brightness */
+			len = 1;
+			response[2] = brightness;
+			printf("Get brightness, reply: %d\n", brightness);
+			break;
+		case 0x8a0e:
+			/* cmd = 0x0e, set backlight power */
+			if (buf[2] != 0 && buf[2] != 1) {
+				fprintf(stderr, "Error invalid set power param: %d\n", buf[2]);
+				continue;
+			}
+
+			len = 0;
+			printf("Set power %d\n", buf[2]);
+			break;
+		default:
+			fprintf(stderr, "Error unknown cmd 0x%04x\n",
+				(buf[0] << 8) | buf[1]);
+			continue;
+		}
+
+		/* Respond with <total-len> <cmd> <data...> <csum> */
+		response[0] = len + 3; /* response length in bytes */
+		response[1] = buf[1];  /* ack cmd */
+		csum = dell_uart_checksum(response, len + 2);
+		response[len + 2] = csum;
+		ret = write(serial_fd, response, len + 3);
+		if (ret != (len + 3))
+			fprintf(stderr, "Error writing %d bytes: %d\n",
+				len + 3, ret);
+	}
+
+out_restore:
+	tcsetattr(serial_fd, TCSANOW, &saved_tty);
+out_close:
+	close(serial_fd);
+	return ret;
+}
-- 
2.44.0

Re: [PATCH 2/2] tools arch x86: Add dell-uart-backlight-emulator

[Andy Shevchenko]

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.
>
> Add a small emulator to allow development and testing of
> the drivers/platform/x86/dell/dell-uart-backlight.c driver for
> this board, without requiring access to an actual Dell All In One.

...

> +++ b/tools/arch/x86/dell-uart-backlight-emulator/Makefile
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Makefile for Intel Software Defined Silicon provisioning tool
> +
> +dell-uart-backlight-emulator: dell-uart-backlight-emulator.c
> +
> +BINDIR ?= /usr/bin
> +
> +override CFLAGS += -O2 -Wall
> +
> +%: %.c
> +       $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS)
> +
> +.PHONY : clean
> +clean :
> +       @rm -f dell-uart-backlight-emulator
> +
> +install : dell-uart-backlight-emulator
> +       install -d $(DESTDIR)$(BINDIR)
> +       install -m 755 -p dell-uart-backlight-emulator $(DESTDIR)$(BINDIR)/dell-uart-backlight-emulator

Is it possible to fix this to (at least) honour `make O=...` cases?
(See, e.g., tools/gpio.)

...

> +/* read() will return -1 on SIGINT / SIGTERM causing the mainloop to cleanly exit */

Interesting...  usually we handle error codes, such as EAGAIN and
EINTR from read() syscall separately.

> +void signalhdlr(int signum)
> +{
> +}

...

> +               fprintf(stderr, "Error opening %s: %s\n", argv[1], strerror(errno));

> +               fprintf(stderr, "Error getting tcattr: %s\n", strerror(errno));

(and so on)

Wouldn't perror() call be better?

...

> +               switch ((buf[0] << 8) | buf[1]) {

byteorder.h is part of UAPI, you can use it, but OTOH it might be too
complicated for the small thing like this.

> +               }

...

> +       return ret;

Hmm... Hopefully you checked the possible returned codes, in user
space it's only a positive 8-bit value used.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] tools arch x86: Add dell-uart-backlight-emulator

[Hans de Goede]

Hi,

On 5/12/24 9:32 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.
>>
>> Add a small emulator to allow development and testing of
>> the drivers/platform/x86/dell/dell-uart-backlight.c driver for
>> this board, without requiring access to an actual Dell All In One.
> 
> ...
> 
>> +++ b/tools/arch/x86/dell-uart-backlight-emulator/Makefile
>> @@ -0,0 +1,19 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Makefile for Intel Software Defined Silicon provisioning tool
>> +
>> +dell-uart-backlight-emulator: dell-uart-backlight-emulator.c
>> +
>> +BINDIR ?= /usr/bin
>> +
>> +override CFLAGS += -O2 -Wall
>> +
>> +%: %.c
>> +       $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS)
>> +
>> +.PHONY : clean
>> +clean :
>> +       @rm -f dell-uart-backlight-emulator
>> +
>> +install : dell-uart-backlight-emulator
>> +       install -d $(DESTDIR)$(BINDIR)
>> +       install -m 755 -p dell-uart-backlight-emulator $(DESTDIR)$(BINDIR)/dell-uart-backlight-emulator
> 
> Is it possible to fix this to (at least) honour `make O=...` cases?
> (See, e.g., tools/gpio.)

I'll take a look at what the tools/gpio Makefile is doing.

> 
> ...
> 
>> +/* read() will return -1 on SIGINT / SIGTERM causing the mainloop to cleanly exit */
> 
> Interesting...  usually we handle error codes, such as EAGAIN and
> EINTR from read() syscall separately.

EAGAIN cannot happen since the fd is kept in its default blocking
mode. Other errors are also not expected to happen and would likely
lead to aborting the program anyway.

So just having an empty signal handler and then exit on the
EINTR error from read() is a nice KISS way to exit the main loop.

> 
>> +void signalhdlr(int signum)
>> +{
>> +}
> 
> ...
> 
>> +               fprintf(stderr, "Error opening %s: %s\n", argv[1], strerror(errno));
> 
>> +               fprintf(stderr, "Error getting tcattr: %s\n", strerror(errno));
> 
> (and so on)
> 
> Wouldn't perror() call be better?

perror() takes a fixed string, so for your first example it won't work since that
requires printf style formatted string support and once I made the choice there
to use fprintf(stderr, ) I used it everywhere for consistency.

> 
> ...
> 
>> +               switch ((buf[0] << 8) | buf[1]) {
> 
> byteorder.h is part of UAPI, you can use it, but OTOH it might be too
> complicated for the small thing like this.
> 
>> +               }
> 
> ...
> 
>> +       return ret;
> 
> Hmm... Hopefully you checked the possible returned codes, in user
> space it's only a positive 8-bit value used.

That is a good point, actually in normal use ret will be (len + 3)
from the last write() call done in the loop. So you're right that
needs some work.

Regards,

Hans

Re: [PATCH 2/2] tools arch x86: Add dell-uart-backlight-emulator

[Hans de Goede]

Hi,

On 5/12/24 9:32 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.
>>
>> Add a small emulator to allow development and testing of
>> the drivers/platform/x86/dell/dell-uart-backlight.c driver for
>> this board, without requiring access to an actual Dell All In One.
> 
> ...
> 
>> +++ b/tools/arch/x86/dell-uart-backlight-emulator/Makefile
>> @@ -0,0 +1,19 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Makefile for Intel Software Defined Silicon provisioning tool
>> +
>> +dell-uart-backlight-emulator: dell-uart-backlight-emulator.c
>> +
>> +BINDIR ?= /usr/bin
>> +
>> +override CFLAGS += -O2 -Wall
>> +
>> +%: %.c
>> +       $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS)
>> +
>> +.PHONY : clean
>> +clean :
>> +       @rm -f dell-uart-backlight-emulator
>> +
>> +install : dell-uart-backlight-emulator
>> +       install -d $(DESTDIR)$(BINDIR)
>> +       install -m 755 -p dell-uart-backlight-emulator $(DESTDIR)$(BINDIR)/dell-uart-backlight-emulator
> 
> Is it possible to fix this to (at least) honour `make O=...` cases?
> (See, e.g., tools/gpio.)

I have taken a look but this only seems applicable to tools which are listed in tools/Makefile
which includes scripts/Makefile.include. this are mostly tools which distributions may want to
package as part of a kernel-tools package. I don't expect distros to package this, so this
seems to not be applicable to this emulator.

Regards,

hans