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

[Andy Shevchenko <andy@kernel.org>]

On Mon, May 13, 2024 at 01:15:50PM +0200, Hans de Goede wrote:
> Dell All In One (AIO) models released after 2017 use a backlight controller
> board connected to an UART.
> 
> In DSDT this uart port will be defined as:
> 
>    Name (_HID, "DELL0501")
>    Name (_CID, EisaId ("PNP0501")
> 
> Instead of having a separate ACPI device with an UartSerialBusV2() resource
> to model the backlight-controller, which would be the standard way to do
> this.
> 
> The acpi_quirk_skip_serdev_enumeration() has special handling for this
> and it will make the serial port code create a serdev controller device
> for the UART instead of a /dev/ttyS0 char-dev. It will also create
> a dell-uart-backlight driver platform device for this driver to bind too.
> 
> This new kernel module contains 2 drivers for this:
> 
> 1. A simple platform driver which creates the actual serdev device
>    (with the serdev controller device as parent)
> 
> 2. A serdev driver for the created serdev device which exports
>    the backlight functionality uses a standard backlight class device.

...

> Reported-by: Roman Bogoyev <roman@computercheck.com.au>

Privately? I mean no links to the report?

...

> +config DELL_UART_BACKLIGHT
> +	tristate "Dell AIO UART Backlight driver"
> +	depends on ACPI

Can it be compile-tested in non-ACPI kernels?

> +	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on SERIAL_DEV_BUS

...

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Yeah, I don't like this, can we move it into header itself?

> +#include <linux/acpi.h>
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/serdev.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>

> +#include "../serdev_helpers.h"

...

> +/* 1st byte Start Of Frame 3 MSB bits: cmd-len + 01010 SOF marker */
> +#define SOF(len)			(((len) << 5) | 0x0a)

This kinda too short to be somehow unique, potential collision might be if
somebody introduces this in the header which somehow will be chain-included
here. Perhaps a namespace? DELL_SOF?

...

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

Can't be called via cleanup.h?

> +	if (ret)
> +		return ret;
> +
> +	dell_bl->status = -EBUSY;
> +	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 != -EBUSY,
> +				 DELL_BL_TIMEOUT);
> +	if (ret == 0) {
> +		dev_err(dell_bl->dev, "Timed out waiting for response.\n");
> +		ret = -ETIMEDOUT;
> +	} else {
> +		ret = dell_bl->status;
> +	}
> +
> +out:
> +	mutex_unlock(&dell_bl->mutex);
> +	return ret;
> +}

> +static int dell_uart_get_brightness(struct dell_uart_backlight *dell_bl)
> +{

	struct device *dev = dell_bl->dev;

> +	u8 get_brightness[GET_CMD_LEN], resp[GET_RESP_LEN];
> +	int ret;
> +
> +	get_brightness[0] = SOF(GET_CMD_LEN);
> +	get_brightness[1] = CMD_GET_BRIGHTNESS;
> +	get_brightness[2] = dell_uart_checksum(get_brightness, 2);

> +	ret = dell_uart_bl_command(dell_bl, get_brightness, GET_CMD_LEN, resp, GET_RESP_LEN);
> +	if (ret)
> +		return ret;
> +
> +	if (resp[RESP_LEN] != GET_RESP_LEN) {
> +		dev_err(dell_bl->dev, "Unexpected get brightness response length: %d\n",
> +			resp[RESP_LEN]);

		dev_err(dev, "Unexpected get brightness response length: %d\n", resp[RESP_LEN]);

> +		return -EIO;
> +	}
> +
> +	if (resp[RESP_DATA] > DELL_BL_MAX_BRIGHTNESS) {
> +		dev_err(dell_bl->dev, "Unexpected get brightness response: %d\n",
> +			resp[RESP_DATA]);

		dev_err(dev, "Unexpected get brightness response: %d\n", resp[RESP_DATA]);

> +		return -EIO;
> +	}
> +
> +	return resp[RESP_DATA];
> +}

...

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

No default case?

...

> +	dev_dbg(dev, "Firmware version: %.*s\n", resp[RESP_LEN] - 3, resp + RESP_DATA);

I would be on the safest side, i.e. not trusting that it will be NUL-terminated
string, hence something like %*pE?


-- 
With Best Regards,
Andy Shevchenko