On Sun, May 12, 2024 at 7:24 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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
