diff mbox series

[1/1,SRU,Bionic,OEM-B] UBUNTU: SAUCE: platform/x86: dell-uart-backlight: new backlight driver for DELL AIO

Message ID 20180713072906.18184-2-acelan.kao@canonical.com
State New
Headers show
Series Dell new AIO requires a new uart backlight driver | expand

Commit Message

AceLan Kao July 13, 2018, 7:29 a.m. UTC
BugLink: https://bugs.launchpad.net/bugs/1727235

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.

In DSDT this uart port will be defined as
   Name (_HID, "DELL0501")
   Name (_CID, EisaId ("PNP0501")
The 8250 PNP driver will be loaded by default, and this driver uses
"DELL0501" to confirm the uart port is a backlight interface and
leverage the port created by 8250 PNP driver to communicate with
the scalar board.

Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/platform/x86/Kconfig               |  14 +
 drivers/platform/x86/Makefile              |   1 +
 drivers/platform/x86/dell-uart-backlight.c | 394 +++++++++++++++++++++
 drivers/platform/x86/dell-uart-backlight.h |  88 +++++
 4 files changed, 497 insertions(+)
 create mode 100644 drivers/platform/x86/dell-uart-backlight.c
 create mode 100644 drivers/platform/x86/dell-uart-backlight.h

Comments

Colin Ian King July 13, 2018, 8:16 a.m. UTC | #1
On 13/07/18 08:29, AceLan Kao wrote:
> BugLink: https://bugs.launchpad.net/bugs/1727235
> 
> 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.
> 
> In DSDT this uart port will be defined as
>    Name (_HID, "DELL0501")
>    Name (_CID, EisaId ("PNP0501")
> The 8250 PNP driver will be loaded by default, and this driver uses
> "DELL0501" to confirm the uart port is a backlight interface and
> leverage the port created by 8250 PNP driver to communicate with
> the scalar board.
> 
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> ---
>  drivers/platform/x86/Kconfig               |  14 +
>  drivers/platform/x86/Makefile              |   1 +
>  drivers/platform/x86/dell-uart-backlight.c | 394 +++++++++++++++++++++
>  drivers/platform/x86/dell-uart-backlight.h |  88 +++++
>  4 files changed, 497 insertions(+)
>  create mode 100644 drivers/platform/x86/dell-uart-backlight.c
>  create mode 100644 drivers/platform/x86/dell-uart-backlight.h
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 683a875f3b6c..6edfa935a434 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -199,6 +199,20 @@ config DELL_RBTN
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called dell-rbtn.
>  
> +config DELL_UART_BACKLIGHT
> +	tristate "Dell AIO UART Backlight driver"
> +	depends on SERIAL_8250
> +	depends on ACPI
> +	---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 FUJITSU_LAPTOP
>  	tristate "Fujitsu Laptop Extras"
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index c32b34a72467..85329fd26afd 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_DELL_WMI_AIO)	+= dell-wmi-aio.o
>  obj-$(CONFIG_DELL_WMI_LED)	+= dell-wmi-led.o
>  obj-$(CONFIG_DELL_SMO8800)	+= dell-smo8800.o
>  obj-$(CONFIG_DELL_RBTN)		+= dell-rbtn.o
> +obj-$(CONFIG_DELL_UART_BACKLIGHT) += dell-uart-backlight.o
>  obj-$(CONFIG_ACER_WMI)		+= acer-wmi.o
>  obj-$(CONFIG_ACERHDF)		+= acerhdf.o
>  obj-$(CONFIG_HP_ACCEL)		+= hp_accel.o
> diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
> new file mode 100644
> index 000000000000..06b847a002c7
> --- /dev/null
> +++ b/drivers/platform/x86/dell-uart-backlight.c
> @@ -0,0 +1,394 @@
> +/*
> + *  Dell AIO Serial Backlight Driver
> + *
> + *  Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/serial_8250.h>
> +#include <linux/delay.h>
> +#include <linux/backlight.h>
> +#include <acpi/video.h>
> +
> +#include "dell-uart-backlight.h"
> +
> +struct dell_uart_backlight {
> +	struct device *dev;
> +	struct backlight_device *dell_uart_bd;
> +	struct mutex brightness_mutex;
> +	int line;
> +	int bl_power;
> +};
> +struct uart_8250_port *serial8250_get_port(int line);
> +struct tty_struct *tty;
> +struct file *ftty;
> +
> +unsigned int (*io_serial_in)(struct uart_port *p, int offset);
> +int (*uart_write)(struct tty_struct *tty, const unsigned char *buf, int count);
> +int (*uart_chars_in_buffer)(struct tty_struct *tty);
> +
> +static int dell_uart_write(struct uart_8250_port *up, __u8 *buf, int len)
> +{
> +	int actual = 0;
> +	struct uart_port *port = &up->port;
> +
> +	tty_port_tty_wakeup(&port->state->port);
> +	tty = tty_port_tty_get(&port->state->port);
> +	actual = uart_write(tty, buf, len);
> +	while (uart_chars_in_buffer(tty)) {
> +		udelay(10);
> +	}

Is is possible for uart_chars_in_buffer() to always return non-zero and
hence we get in an infinite loop?

> +
> +	return actual;
> +}
> +
> +static int dell_uart_read(struct uart_8250_port *up, __u8 *buf, int len)
> +{
> +	int i, retry;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&up->port.lock, flags);
> +	for (i = 0; i < len; i++) {
> +		retry = 10;
> +		while (!(io_serial_in(&up->port, UART_LSR) & UART_LSR_DR)) {
> +			if (--retry == 0)
> +				break;
> +			mdelay(20);
> +		}
> +
> +		if (retry == 0)
> +			break;
> +		buf[i] = io_serial_in(&up->port, UART_RX);
> +	}
> +	spin_unlock_irqrestore(&up->port.lock, flags);

spin locks should be used for small length delays, this seems to have
some length delays so potentially we could have some CPU eating spin
lock contention here.

> +
> +	return i;
> +}
> +
> +static void dell_uart_dump_cmd(const char *func, const char *prefix,
> +			       const char *cmd, int len)
> +{
> +	char buf[80];
> +
> +	snprintf(buf, 80, "dell_uart_backlight:%s:%s", func, prefix);
> +	if (len != 0)
> +		print_hex_dump_debug(buf, DUMP_PREFIX_NONE,
> +					16, 1, cmd, len, false);
> +	else
> +		pr_debug("dell_uart_backlight:%s:%sNULL\n", func, prefix);
> +
> +}
> +
> +/*
> + * checksum: SUM(Length and Cmd and Data)xor 0xFF)
> + */
> +static unsigned char dell_uart_checksum(unsigned char *buf, int len)
> +{
> +	unsigned char val = 0;
> +
> +	while (len-- > 0)
> +		val += buf[len];
> +
> +	return val ^ 0xff;
> +}
> +
> +/*
> + * 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 bl_power variable
> + */
> +static int dell_uart_get_bl_power(struct dell_uart_backlight *dell_pdata)
> +{
> +	return dell_pdata->bl_power;
> +}

perhaps making the above function inline because it is a small helper
function.

> +
> +static int dell_uart_set_bl_power(struct backlight_device *bd, int power)
> +{
> +	struct dell_uart_bl_cmd *bl_cmd =
> +		&uart_cmd[DELL_UART_SET_BACKLIGHT_POWER];
> +	struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
> +	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
> +	int rx_len;
> +
> +	if (power != FB_BLANK_POWERDOWN)
> +		power = FB_BLANK_UNBLANK;
> +
> +	bl_cmd->cmd[2] = power?0:1;

Minor nitpick, whitespaces could be added here, e,g. power ? 0 : 1;

> +	bl_cmd->cmd[3] = dell_uart_checksum(bl_cmd->cmd, bl_cmd->tx_len - 1);
> +
> +	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
> +
> +	if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
> +		pr_debug("Failed to get mutex_lock");
> +		return 0;
> +	}
> +
> +	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> +	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> +
> +	mutex_unlock(&dell_pdata->brightness_mutex);
> +
> +	dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
> +
> +	bd->props.power = power;
> +	dell_pdata->bl_power = power;
> +
> +	return 0;
> +}
> +
> +static int dell_uart_get_brightness(struct backlight_device *bd)
> +{
> +	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_BRIGHTNESS];
> +	struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
> +	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
> +	int rx_len, brightness = 0;
> +
> +	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
> +
> +	if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
> +		pr_debug("Failed to get mutex_lock");
> +		return 0;
> +	}
> +
> +	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> +	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> +
> +	mutex_unlock(&dell_pdata->brightness_mutex);
> +
> +	dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
> +
> +	brightness = (unsigned int)bl_cmd->ret[2];
> +
> +	return brightness;
> +}
> +
> +static int dell_uart_update_status(struct backlight_device *bd)
> +{
> +	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_SET_BRIGHTNESS];
> +	struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
> +	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
> +	int rx_len;
> +
> +	bl_cmd->cmd[2] = bd->props.brightness;
> +	bl_cmd->cmd[3] = dell_uart_checksum(bl_cmd->cmd, bl_cmd->tx_len - 1);
> +
> +	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
> +
> +	if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
> +		pr_debug("Failed to get mutex_lock");
> +		return 0;
> +	}
> +
> +	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> +	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> +
> +	mutex_unlock(&dell_pdata->brightness_mutex);
> +
> +	dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
> +
> +	if (bd->props.power != dell_uart_get_bl_power(dell_pdata))
> +		dell_uart_set_bl_power(bd, bd->props.power);
> +
> +	return 0;
> +}
> +
> +static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
> +{
> +	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
> +	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
> +	int rx_len, status = 1; /* assume the scalar IC controls backlight if the command failed */
> +
> +	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
> +
> +	if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
> +		pr_debug("Failed to get mutex_lock");
> +		return 0;
> +	}
> +
> +	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> +	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> +
> +	mutex_unlock(&dell_pdata->brightness_mutex);
> +
> +	dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
> +
> +	if (rx_len == 4)
> +		status = (unsigned int)bl_cmd->ret[2];
> +
> +	return status;
> +}
> +
> +static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata)
> +{
> +	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_FIRMWARE_VER];
> +	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
> +	int rx_len = 0, retry = 10;
> +
> +	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
> +
> +	if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
> +		pr_debug("Failed to get mutex_lock");
> +		return -1;
> +	}
> +
> +	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> +	while (retry-- > 0) {
> +		/* first byte is data length */
> +		dell_uart_read(uart, bl_cmd->ret, 1);
> +		rx_len = (int)bl_cmd->ret[0];
> +		if (bl_cmd->ret[0] > 80 || bl_cmd->ret[0] <= 0) {
> +			pr_debug("Failed to get firmware version\n");
> +			if (retry == 0) {
> +				mutex_unlock(&dell_pdata->brightness_mutex);
> +				return -1;
> +			}
> +			msleep(100);
> +			continue;
> +		}
> +
> +		dell_uart_read(uart, bl_cmd->ret+1, rx_len-1);
> +		break;
> +	}
> +	mutex_unlock(&dell_pdata->brightness_mutex);
> +
> +	dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
> +
> +	pr_debug("Firmare str(%d)= %s\n", (int)bl_cmd->ret[0], bl_cmd->ret+2);
> +	return rx_len;
> +}
> +
> +static const struct backlight_ops dell_uart_backlight_ops = {
> +	.get_brightness = dell_uart_get_brightness,
> +	.update_status = dell_uart_update_status,
> +};
> +
> +static int dell_uart_startup(struct dell_uart_backlight *dell_pdata)
> +{
> +	struct uart_8250_port *uartp;
> +	struct uart_port *port;
> +
> +	dell_pdata->line = 0;
> +	uartp = serial8250_get_port(dell_pdata->line);
> +	port = &uartp->port;
> +	tty = port->state->port.tty;
> +	io_serial_in = port->serial_in;
> +	uart_write = tty->driver->ops->write;
> +	uart_chars_in_buffer = tty->driver->ops->chars_in_buffer;
> +
> +	return 0;
> +}
> +
> +static int dell_uart_bl_add(struct acpi_device *dev)
> +{
> +	struct dell_uart_backlight *dell_pdata;
> +	struct backlight_properties props;
> +	struct backlight_device *dell_uart_bd;
> +
> +	dell_pdata = kzalloc(sizeof(struct dell_uart_backlight), GFP_KERNEL);

always check for failed allocations please

> +	dell_pdata->dev = &dev->dev;
> +	dell_uart_startup(dell_pdata);
> +	dev->driver_data = dell_pdata;
> +
> +	mutex_init(&dell_pdata->brightness_mutex);
> +
> +	if (!dell_uart_get_scalar_status(dell_pdata)) {
> +		udelay(50);
> +		/* try another command to make sure there is no scalar IC */
> +		if (!dell_uart_show_firmware_ver(dell_pdata) <= 0) {
> +			pr_debug("Scalar doesn't in charge of brightness adjustment.\n");
> +			return -1;
> +		}
> +	}
> +
> +	memset(&props, 0, sizeof(struct backlight_properties));
> +	props.type = BACKLIGHT_PLATFORM;
> +	props.max_brightness = 100;
> +
> +	dell_uart_bd = backlight_device_register("dell_uart_backlight",
> +						 &dev->dev,
> +						 dell_pdata,
> +						 &dell_uart_backlight_ops,
> +						 &props);

check for failure, e.g.

	if (IS_ERR(pdell_uart_bd))
		....

> +	dell_pdata->dell_uart_bd = dell_uart_bd;
> +
> +	dell_uart_set_bl_power(dell_uart_bd, FB_BLANK_UNBLANK);
> +	dell_uart_bd->props.brightness = 100;
> +	backlight_update_status(dell_uart_bd);
> +
> +	/* unregister acpi backlight interface */
> +	acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
> +
> +	return 0;
> +}
> +
> +static int dell_uart_bl_remove(struct acpi_device *dev)
> +{
> +	struct dell_uart_backlight *dell_pdata = dev->driver_data;
> +
> +	backlight_device_unregister(dell_pdata->dell_uart_bd);
> +
> +	return 0;
> +}
> +
> +static int dell_uart_bl_suspend(struct device *dev)
> +{
> +	filp_close(ftty, NULL);
> +	return 0;
> +}
> +
> +static int dell_uart_bl_resume(struct device *dev)
> +{
> +	ftty = filp_open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY, 0);

what happens whet ftty open fails?

> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(dell_uart_bl_pm, dell_uart_bl_suspend, dell_uart_bl_resume);
> +
> +static const struct acpi_device_id dell_uart_bl_ids[] = {
> +	{"DELL0501", 0},
> +	{"", 0},
> +};
> +
> +static struct acpi_driver dell_uart_backlight_driver = {
> +	.name = "Dell AIO serial backlight",
> +	.ids = dell_uart_bl_ids,
> +	.ops = {
> +		.add = dell_uart_bl_add,
> +		.remove = dell_uart_bl_remove,
> +	},
> +	.drv.pm = &dell_uart_bl_pm,
> +};
> +
> +static int __init dell_uart_bl_init(void)
> +{
> +	ftty = filp_open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY, 0);

what happens when ftty open fails?

> +
> +	return acpi_bus_register_driver(&dell_uart_backlight_driver);
> +}
> +
> +static void __exit dell_uart_bl_exit(void)
> +{
> +	filp_close(ftty, NULL);
> +
> +	acpi_bus_unregister_driver(&dell_uart_backlight_driver);
> +}
> +
> +module_init(dell_uart_bl_init);
> +module_exit(dell_uart_bl_exit);
> +MODULE_DEVICE_TABLE(acpi, dell_uart_bl_ids);
> +MODULE_DESCRIPTION("Dell AIO Serial Backlight module");
> +MODULE_AUTHOR("AceLan Kao <acelan.kao@canonical.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell-uart-backlight.h b/drivers/platform/x86/dell-uart-backlight.h
> new file mode 100644
> index 000000000000..b8bca1230e56
> --- /dev/null
> +++ b/drivers/platform/x86/dell-uart-backlight.h
> @@ -0,0 +1,88 @@
> +/*
> + *  Dell AIO Serial Backlight Driver
> + *
> + *  Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef _DELL_UART_BACKLIGHT_H_
> +#define _DELL_UART_BACKLIGHT_H_
> +
> +enum {
> +	DELL_UART_GET_FIRMWARE_VER,
> +	DELL_UART_GET_BRIGHTNESS,
> +	DELL_UART_SET_BRIGHTNESS,
> +	DELL_UART_SET_BACKLIGHT_POWER,
> +};
> +
> +struct dell_uart_bl_cmd {
> +	unsigned char	cmd[10];
> +	unsigned char	ret[80];
> +	unsigned short	tx_len;
> +	unsigned short	rx_len;
> +};
> +


below: should these be in the main source rather than a header file?

Also, make static structs static const if they are not going to be modified.

> +static struct dell_uart_bl_cmd uart_cmd[] = {
> +	/*
> +	 * 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 .
> +	 */
> +	[DELL_UART_GET_FIRMWARE_VER] = {
> +		.cmd	= {0x6A, 0x06, 0x8F},
> +		.tx_len	= 3,
> +	},
> +	/*
> +	 * Get Brightness level: Application uses this command for scaler 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 which ranges from 0~100.
> +	 */
> +	[DELL_UART_GET_BRIGHTNESS] = {
> +		.cmd	= {0x6A, 0x0C, 0x89},
> +		.ret	= {0x04, 0x0C, 0x00, 0x00},
> +		.tx_len	= 3,
> +		.rx_len	= 4,
> +	},
> +	/* Set Brightness level: Application uses this command for scaler to set brightness.
> +	 * Command: 0x8A 0x0B Byte2 Checksum (Length:4 Type: 0x0A, Cmd:0x0B)
> +	 * 	    where Byte2 is the brightness level which ranges from 0~100.
> +	 * Return data: 0x03 0x0B 0xF1(Length:3,Cmd:B,checksum:0xF1)
> +	 * Scaler must send the 3bytes ack within 1 second when success, other value if error
> +	 */
> +	[DELL_UART_SET_BRIGHTNESS] = {
> +		.cmd	= {0x8A, 0x0B, 0x0, 0x0},
> +		.ret	= {0x03, 0x0B, 0xF1},
> +		.tx_len	= 4,
> +		.rx_len	= 3,
> +	},
> +	/*
> +	 * Screen ON/OFF Control: Application uses this command to control screen ON or OFF.
> +	 * Command: 0x8A 0x0E Byte2 Checksum (Length:4 Type: 0x0A, Cmd:0x0E) where
> +	 * 	    Byte2=0 to turn OFF the screen.
> +	 * 	    Byte2=1 to turn ON the screen
> +	 * 	    Other value of Byte2 is reserved and invalid.
> +	 * Return data: 0x03 0x0E 0xEE(Length:3,Cmd:E,checksum:0xEE)
> +	 */
> +	[DELL_UART_SET_BACKLIGHT_POWER] = {
> +		.cmd	= {0x8A, 0x0E, 0x00, 0x0},
> +		.ret	= {0x03, 0x0E, 0xEE},
> +		.tx_len	= 4,
> +		.rx_len	= 3,
> +	},
> +};
> +
> +#endif /* _DELL_UART_BACKLIGHT_H_ */
> 

Colin
AceLan Kao July 13, 2018, 9:34 a.m. UTC | #2
2018-07-13 16:16 GMT+08:00 Colin Ian King <colin.king@canonical.com>:
> On 13/07/18 08:29, AceLan Kao wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1727235
>>
>> 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.
>>
>> In DSDT this uart port will be defined as
>>    Name (_HID, "DELL0501")
>>    Name (_CID, EisaId ("PNP0501")
>> The 8250 PNP driver will be loaded by default, and this driver uses
>> "DELL0501" to confirm the uart port is a backlight interface and
>> leverage the port created by 8250 PNP driver to communicate with
>> the scalar board.
>>
>> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
>> ---
>>  drivers/platform/x86/Kconfig               |  14 +
>>  drivers/platform/x86/Makefile              |   1 +
>>  drivers/platform/x86/dell-uart-backlight.c | 394 +++++++++++++++++++++
>>  drivers/platform/x86/dell-uart-backlight.h |  88 +++++
>>  4 files changed, 497 insertions(+)
>>  create mode 100644 drivers/platform/x86/dell-uart-backlight.c
>>  create mode 100644 drivers/platform/x86/dell-uart-backlight.h
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 683a875f3b6c..6edfa935a434 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -199,6 +199,20 @@ config DELL_RBTN
>>         To compile this driver as a module, choose M here: the module will
>>         be called dell-rbtn.
>>
>> +config DELL_UART_BACKLIGHT
>> +     tristate "Dell AIO UART Backlight driver"
>> +     depends on SERIAL_8250
>> +     depends on ACPI
>> +     ---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 FUJITSU_LAPTOP
>>       tristate "Fujitsu Laptop Extras"
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index c32b34a72467..85329fd26afd 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -22,6 +22,7 @@ obj-$(CONFIG_DELL_WMI_AIO)  += dell-wmi-aio.o
>>  obj-$(CONFIG_DELL_WMI_LED)   += dell-wmi-led.o
>>  obj-$(CONFIG_DELL_SMO8800)   += dell-smo8800.o
>>  obj-$(CONFIG_DELL_RBTN)              += dell-rbtn.o
>> +obj-$(CONFIG_DELL_UART_BACKLIGHT) += dell-uart-backlight.o
>>  obj-$(CONFIG_ACER_WMI)               += acer-wmi.o
>>  obj-$(CONFIG_ACERHDF)                += acerhdf.o
>>  obj-$(CONFIG_HP_ACCEL)               += hp_accel.o
>> diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
>> new file mode 100644
>> index 000000000000..06b847a002c7
>> --- /dev/null
>> +++ b/drivers/platform/x86/dell-uart-backlight.c
>> @@ -0,0 +1,394 @@
>> +/*
>> + *  Dell AIO Serial Backlight Driver
>> + *
>> + *  Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; either version 2 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/acpi.h>
>> +#include <linux/serial_8250.h>
>> +#include <linux/delay.h>
>> +#include <linux/backlight.h>
>> +#include <acpi/video.h>
>> +
>> +#include "dell-uart-backlight.h"
>> +
>> +struct dell_uart_backlight {
>> +     struct device *dev;
>> +     struct backlight_device *dell_uart_bd;
>> +     struct mutex brightness_mutex;
>> +     int line;
>> +     int bl_power;
>> +};
>> +struct uart_8250_port *serial8250_get_port(int line);
>> +struct tty_struct *tty;
>> +struct file *ftty;
>> +
>> +unsigned int (*io_serial_in)(struct uart_port *p, int offset);
>> +int (*uart_write)(struct tty_struct *tty, const unsigned char *buf, int count);
>> +int (*uart_chars_in_buffer)(struct tty_struct *tty);
>> +
>> +static int dell_uart_write(struct uart_8250_port *up, __u8 *buf, int len)
>> +{
>> +     int actual = 0;
>> +     struct uart_port *port = &up->port;
>> +
>> +     tty_port_tty_wakeup(&port->state->port);
>> +     tty = tty_port_tty_get(&port->state->port);
>> +     actual = uart_write(tty, buf, len);
>> +     while (uart_chars_in_buffer(tty)) {
>> +             udelay(10);
>> +     }
>
> Is is possible for uart_chars_in_buffer() to always return non-zero and
> hence we get in an infinite loop?
It doesn't look like to return non-zero value, the value will be mask
with (1<<12) - 1
#define CIRC_CNT(head,tail,size) (((head) - (tail)) & ((size)-1))

>
>> +
>> +     return actual;
>> +}
>> +
>> +static int dell_uart_read(struct uart_8250_port *up, __u8 *buf, int len)
>> +{
>> +     int i, retry;
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&up->port.lock, flags);
>> +     for (i = 0; i < len; i++) {
>> +             retry = 10;
>> +             while (!(io_serial_in(&up->port, UART_LSR) & UART_LSR_DR)) {
>> +                     if (--retry == 0)
>> +                             break;
>> +                     mdelay(20);
>> +             }
>> +
>> +             if (retry == 0)
>> +                     break;
>> +             buf[i] = io_serial_in(&up->port, UART_RX);
>> +     }
>> +     spin_unlock_irqrestore(&up->port.lock, flags);
>
> spin locks should be used for small length delays, this seems to have
> some length delays so potentially we could have some CPU eating spin
> lock contention here.
When we call read function that means we just write out commands and
is trying to read back.
In this case, we don't want to let other processes read out the data,
so we have to use spin lock here.
Busy waiting is bad, but I don't know if we have any other choices here.

>
>> +
>> +     return i;
>> +}
>> +
>> +static void dell_uart_dump_cmd(const char *func, const char *prefix,
>> +                            const char *cmd, int len)
>> +{
>> +     char buf[80];
>> +
>> +     snprintf(buf, 80, "dell_uart_backlight:%s:%s", func, prefix);
>> +     if (len != 0)
>> +             print_hex_dump_debug(buf, DUMP_PREFIX_NONE,
>> +                                     16, 1, cmd, len, false);
>> +     else
>> +             pr_debug("dell_uart_backlight:%s:%sNULL\n", func, prefix);
>> +
>> +}
>> +
>> +/*
>> + * checksum: SUM(Length and Cmd and Data)xor 0xFF)
>> + */
>> +static unsigned char dell_uart_checksum(unsigned char *buf, int len)
>> +{
>> +     unsigned char val = 0;
>> +
>> +     while (len-- > 0)
>> +             val += buf[len];
>> +
>> +     return val ^ 0xff;
>> +}
>> +
>> +/*
>> + * 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 bl_power variable
>> + */
>> +static int dell_uart_get_bl_power(struct dell_uart_backlight *dell_pdata)
>> +{
>> +     return dell_pdata->bl_power;
>> +}
>
> perhaps making the above function inline because it is a small helper
> function.
Okay.

>
>> +
>> +static int dell_uart_set_bl_power(struct backlight_device *bd, int power)
>> +{
>> +     struct dell_uart_bl_cmd *bl_cmd =
>> +             &uart_cmd[DELL_UART_SET_BACKLIGHT_POWER];
>> +     struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>> +     int rx_len;
>> +
>> +     if (power != FB_BLANK_POWERDOWN)
>> +             power = FB_BLANK_UNBLANK;
>> +
>> +     bl_cmd->cmd[2] = power?0:1;
>
> Minor nitpick, whitespaces could be added here, e,g. power ? 0 : 1;
Okay

>
>> +     bl_cmd->cmd[3] = dell_uart_checksum(bl_cmd->cmd, bl_cmd->tx_len - 1);
>> +
>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>> +
>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>> +             pr_debug("Failed to get mutex_lock");
>> +             return 0;
>> +     }
>> +
>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>> +
>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>> +
>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>> +
>> +     bd->props.power = power;
>> +     dell_pdata->bl_power = power;
>> +
>> +     return 0;
>> +}
>> +
>> +static int dell_uart_get_brightness(struct backlight_device *bd)
>> +{
>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_BRIGHTNESS];
>> +     struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>> +     int rx_len, brightness = 0;
>> +
>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>> +
>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>> +             pr_debug("Failed to get mutex_lock");
>> +             return 0;
>> +     }
>> +
>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>> +
>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>> +
>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>> +
>> +     brightness = (unsigned int)bl_cmd->ret[2];
>> +
>> +     return brightness;
>> +}
>> +
>> +static int dell_uart_update_status(struct backlight_device *bd)
>> +{
>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_SET_BRIGHTNESS];
>> +     struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>> +     int rx_len;
>> +
>> +     bl_cmd->cmd[2] = bd->props.brightness;
>> +     bl_cmd->cmd[3] = dell_uart_checksum(bl_cmd->cmd, bl_cmd->tx_len - 1);
>> +
>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>> +
>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>> +             pr_debug("Failed to get mutex_lock");
>> +             return 0;
>> +     }
>> +
>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>> +
>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>> +
>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>> +
>> +     if (bd->props.power != dell_uart_get_bl_power(dell_pdata))
>> +             dell_uart_set_bl_power(bd, bd->props.power);
>> +
>> +     return 0;
>> +}
>> +
>> +static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
>> +{
>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>> +     int rx_len, status = 1; /* assume the scalar IC controls backlight if the command failed */
>> +
>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>> +
>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>> +             pr_debug("Failed to get mutex_lock");
>> +             return 0;
>> +     }
>> +
>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>> +
>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>> +
>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>> +
>> +     if (rx_len == 4)
>> +             status = (unsigned int)bl_cmd->ret[2];
>> +
>> +     return status;
>> +}
>> +
>> +static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata)
>> +{
>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_FIRMWARE_VER];
>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>> +     int rx_len = 0, retry = 10;
>> +
>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>> +
>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>> +             pr_debug("Failed to get mutex_lock");
>> +             return -1;
>> +     }
>> +
>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>> +     while (retry-- > 0) {
>> +             /* first byte is data length */
>> +             dell_uart_read(uart, bl_cmd->ret, 1);
>> +             rx_len = (int)bl_cmd->ret[0];
>> +             if (bl_cmd->ret[0] > 80 || bl_cmd->ret[0] <= 0) {
>> +                     pr_debug("Failed to get firmware version\n");
>> +                     if (retry == 0) {
>> +                             mutex_unlock(&dell_pdata->brightness_mutex);
>> +                             return -1;
>> +                     }
>> +                     msleep(100);
>> +                     continue;
>> +             }
>> +
>> +             dell_uart_read(uart, bl_cmd->ret+1, rx_len-1);
>> +             break;
>> +     }
>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>> +
>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>> +
>> +     pr_debug("Firmare str(%d)= %s\n", (int)bl_cmd->ret[0], bl_cmd->ret+2);
>> +     return rx_len;
>> +}
>> +
>> +static const struct backlight_ops dell_uart_backlight_ops = {
>> +     .get_brightness = dell_uart_get_brightness,
>> +     .update_status = dell_uart_update_status,
>> +};
>> +
>> +static int dell_uart_startup(struct dell_uart_backlight *dell_pdata)
>> +{
>> +     struct uart_8250_port *uartp;
>> +     struct uart_port *port;
>> +
>> +     dell_pdata->line = 0;
>> +     uartp = serial8250_get_port(dell_pdata->line);
>> +     port = &uartp->port;
>> +     tty = port->state->port.tty;
>> +     io_serial_in = port->serial_in;
>> +     uart_write = tty->driver->ops->write;
>> +     uart_chars_in_buffer = tty->driver->ops->chars_in_buffer;
>> +
>> +     return 0;
>> +}
>> +
>> +static int dell_uart_bl_add(struct acpi_device *dev)
>> +{
>> +     struct dell_uart_backlight *dell_pdata;
>> +     struct backlight_properties props;
>> +     struct backlight_device *dell_uart_bd;
>> +
>> +     dell_pdata = kzalloc(sizeof(struct dell_uart_backlight), GFP_KERNEL);
>
> always check for failed allocations please
Okay

>
>> +     dell_pdata->dev = &dev->dev;
>> +     dell_uart_startup(dell_pdata);
>> +     dev->driver_data = dell_pdata;
>> +
>> +     mutex_init(&dell_pdata->brightness_mutex);
>> +
>> +     if (!dell_uart_get_scalar_status(dell_pdata)) {
>> +             udelay(50);
>> +             /* try another command to make sure there is no scalar IC */
>> +             if (!dell_uart_show_firmware_ver(dell_pdata) <= 0) {
>> +                     pr_debug("Scalar doesn't in charge of brightness adjustment.\n");
>> +                     return -1;
>> +             }
>> +     }
>> +
>> +     memset(&props, 0, sizeof(struct backlight_properties));
>> +     props.type = BACKLIGHT_PLATFORM;
>> +     props.max_brightness = 100;
>> +
>> +     dell_uart_bd = backlight_device_register("dell_uart_backlight",
>> +                                              &dev->dev,
>> +                                              dell_pdata,
>> +                                              &dell_uart_backlight_ops,
>> +                                              &props);
>
> check for failure, e.g.
>
>         if (IS_ERR(pdell_uart_bd))
>                 ....
>
Okay

>> +     dell_pdata->dell_uart_bd = dell_uart_bd;
>> +
>> +     dell_uart_set_bl_power(dell_uart_bd, FB_BLANK_UNBLANK);
>> +     dell_uart_bd->props.brightness = 100;
>> +     backlight_update_status(dell_uart_bd);
>> +
>> +     /* unregister acpi backlight interface */
>> +     acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
>> +
>> +     return 0;
>> +}
>> +
>> +static int dell_uart_bl_remove(struct acpi_device *dev)
>> +{
>> +     struct dell_uart_backlight *dell_pdata = dev->driver_data;
>> +
>> +     backlight_device_unregister(dell_pdata->dell_uart_bd);
>> +
>> +     return 0;
>> +}
>> +
>> +static int dell_uart_bl_suspend(struct device *dev)
>> +{
>> +     filp_close(ftty, NULL);
>> +     return 0;
>> +}
>> +
>> +static int dell_uart_bl_resume(struct device *dev)
>> +{
>> +     ftty = filp_open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY, 0);
>
> what happens whet ftty open fails?
It's not possible to fail here, if it fails to open it should fail at
the beginning.

>
>> +     return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(dell_uart_bl_pm, dell_uart_bl_suspend, dell_uart_bl_resume);
>> +
>> +static const struct acpi_device_id dell_uart_bl_ids[] = {
>> +     {"DELL0501", 0},
>> +     {"", 0},
>> +};
>> +
>> +static struct acpi_driver dell_uart_backlight_driver = {
>> +     .name = "Dell AIO serial backlight",
>> +     .ids = dell_uart_bl_ids,
>> +     .ops = {
>> +             .add = dell_uart_bl_add,
>> +             .remove = dell_uart_bl_remove,
>> +     },
>> +     .drv.pm = &dell_uart_bl_pm,
>> +};
>> +
>> +static int __init dell_uart_bl_init(void)
>> +{
>> +     ftty = filp_open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY, 0);
>
> what happens when ftty open fails?
When the driver is to be loaded, there should be _HID("DELL0501")
defined in DSDT, and also there should be "PNP0501" defined, too.
In this case, we can assume that there is /dev/ttyS0 be generated by
serial_pnp driver which is a build-in driver and will be loaded before
our driver.

There should be no such case that it'll fail to open /dev/ttyS0, the
device should be ready before our driver is loaded.

>
>> +
>> +     return acpi_bus_register_driver(&dell_uart_backlight_driver);
>> +}
>> +
>> +static void __exit dell_uart_bl_exit(void)
>> +{
>> +     filp_close(ftty, NULL);
>> +
>> +     acpi_bus_unregister_driver(&dell_uart_backlight_driver);
>> +}
>> +
>> +module_init(dell_uart_bl_init);
>> +module_exit(dell_uart_bl_exit);
>> +MODULE_DEVICE_TABLE(acpi, dell_uart_bl_ids);
>> +MODULE_DESCRIPTION("Dell AIO Serial Backlight module");
>> +MODULE_AUTHOR("AceLan Kao <acelan.kao@canonical.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/platform/x86/dell-uart-backlight.h b/drivers/platform/x86/dell-uart-backlight.h
>> new file mode 100644
>> index 000000000000..b8bca1230e56
>> --- /dev/null
>> +++ b/drivers/platform/x86/dell-uart-backlight.h
>> @@ -0,0 +1,88 @@
>> +/*
>> + *  Dell AIO Serial Backlight Driver
>> + *
>> + *  Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; either version 2 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#ifndef _DELL_UART_BACKLIGHT_H_
>> +#define _DELL_UART_BACKLIGHT_H_
>> +
>> +enum {
>> +     DELL_UART_GET_FIRMWARE_VER,
>> +     DELL_UART_GET_BRIGHTNESS,
>> +     DELL_UART_SET_BRIGHTNESS,
>> +     DELL_UART_SET_BACKLIGHT_POWER,
>> +};
>> +
>> +struct dell_uart_bl_cmd {
>> +     unsigned char   cmd[10];
>> +     unsigned char   ret[80];
>> +     unsigned short  tx_len;
>> +     unsigned short  rx_len;
>> +};
>> +
>
>
> below: should these be in the main source rather than a header file?
>
> Also, make static structs static const if they are not going to be modified.
Okay

>
>> +static struct dell_uart_bl_cmd uart_cmd[] = {
>> +     /*
>> +      * 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 .
>> +      */
>> +     [DELL_UART_GET_FIRMWARE_VER] = {
>> +             .cmd    = {0x6A, 0x06, 0x8F},
>> +             .tx_len = 3,
>> +     },
>> +     /*
>> +      * Get Brightness level: Application uses this command for scaler 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 which ranges from 0~100.
>> +      */
>> +     [DELL_UART_GET_BRIGHTNESS] = {
>> +             .cmd    = {0x6A, 0x0C, 0x89},
>> +             .ret    = {0x04, 0x0C, 0x00, 0x00},
>> +             .tx_len = 3,
>> +             .rx_len = 4,
>> +     },
>> +     /* Set Brightness level: Application uses this command for scaler to set brightness.
>> +      * Command: 0x8A 0x0B Byte2 Checksum (Length:4 Type: 0x0A, Cmd:0x0B)
>> +      *          where Byte2 is the brightness level which ranges from 0~100.
>> +      * Return data: 0x03 0x0B 0xF1(Length:3,Cmd:B,checksum:0xF1)
>> +      * Scaler must send the 3bytes ack within 1 second when success, other value if error
>> +      */
>> +     [DELL_UART_SET_BRIGHTNESS] = {
>> +             .cmd    = {0x8A, 0x0B, 0x0, 0x0},
>> +             .ret    = {0x03, 0x0B, 0xF1},
>> +             .tx_len = 4,
>> +             .rx_len = 3,
>> +     },
>> +     /*
>> +      * Screen ON/OFF Control: Application uses this command to control screen ON or OFF.
>> +      * Command: 0x8A 0x0E Byte2 Checksum (Length:4 Type: 0x0A, Cmd:0x0E) where
>> +      *          Byte2=0 to turn OFF the screen.
>> +      *          Byte2=1 to turn ON the screen
>> +      *          Other value of Byte2 is reserved and invalid.
>> +      * Return data: 0x03 0x0E 0xEE(Length:3,Cmd:E,checksum:0xEE)
>> +      */
>> +     [DELL_UART_SET_BACKLIGHT_POWER] = {
>> +             .cmd    = {0x8A, 0x0E, 0x00, 0x0},
>> +             .ret    = {0x03, 0x0E, 0xEE},
>> +             .tx_len = 4,
>> +             .rx_len = 3,
>> +     },
>> +};
>> +
>> +#endif /* _DELL_UART_BACKLIGHT_H_ */
>>
>
> Colin
Colin Ian King July 13, 2018, 9:40 a.m. UTC | #3
On 13/07/18 10:34, AceLan Kao wrote:
> 2018-07-13 16:16 GMT+08:00 Colin Ian King <colin.king@canonical.com>:
>> On 13/07/18 08:29, AceLan Kao wrote:
>>> BugLink: https://bugs.launchpad.net/bugs/1727235
>>>
>>> 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.
>>>
>>> In DSDT this uart port will be defined as
>>>    Name (_HID, "DELL0501")
>>>    Name (_CID, EisaId ("PNP0501")
>>> The 8250 PNP driver will be loaded by default, and this driver uses
>>> "DELL0501" to confirm the uart port is a backlight interface and
>>> leverage the port created by 8250 PNP driver to communicate with
>>> the scalar board.
>>>
>>> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
>>> ---
>>>  drivers/platform/x86/Kconfig               |  14 +
>>>  drivers/platform/x86/Makefile              |   1 +
>>>  drivers/platform/x86/dell-uart-backlight.c | 394 +++++++++++++++++++++
>>>  drivers/platform/x86/dell-uart-backlight.h |  88 +++++
>>>  4 files changed, 497 insertions(+)
>>>  create mode 100644 drivers/platform/x86/dell-uart-backlight.c
>>>  create mode 100644 drivers/platform/x86/dell-uart-backlight.h
>>>
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index 683a875f3b6c..6edfa935a434 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -199,6 +199,20 @@ config DELL_RBTN
>>>         To compile this driver as a module, choose M here: the module will
>>>         be called dell-rbtn.
>>>
>>> +config DELL_UART_BACKLIGHT
>>> +     tristate "Dell AIO UART Backlight driver"
>>> +     depends on SERIAL_8250
>>> +     depends on ACPI
>>> +     ---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 FUJITSU_LAPTOP
>>>       tristate "Fujitsu Laptop Extras"
>>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>>> index c32b34a72467..85329fd26afd 100644
>>> --- a/drivers/platform/x86/Makefile
>>> +++ b/drivers/platform/x86/Makefile
>>> @@ -22,6 +22,7 @@ obj-$(CONFIG_DELL_WMI_AIO)  += dell-wmi-aio.o
>>>  obj-$(CONFIG_DELL_WMI_LED)   += dell-wmi-led.o
>>>  obj-$(CONFIG_DELL_SMO8800)   += dell-smo8800.o
>>>  obj-$(CONFIG_DELL_RBTN)              += dell-rbtn.o
>>> +obj-$(CONFIG_DELL_UART_BACKLIGHT) += dell-uart-backlight.o
>>>  obj-$(CONFIG_ACER_WMI)               += acer-wmi.o
>>>  obj-$(CONFIG_ACERHDF)                += acerhdf.o
>>>  obj-$(CONFIG_HP_ACCEL)               += hp_accel.o
>>> diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
>>> new file mode 100644
>>> index 000000000000..06b847a002c7
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/dell-uart-backlight.c
>>> @@ -0,0 +1,394 @@
>>> +/*
>>> + *  Dell AIO Serial Backlight Driver
>>> + *
>>> + *  Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
>>> + *
>>> + *  This program is free software; you can redistribute it and/or modify
>>> + *  it under the terms of the GNU General Public License as published by
>>> + *  the Free Software Foundation; either version 2 of the License, or
>>> + *  (at your option) any later version.
>>> + *
>>> + *  This program is distributed in the hope that it will be useful,
>>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + *  GNU General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/acpi.h>
>>> +#include <linux/serial_8250.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/backlight.h>
>>> +#include <acpi/video.h>
>>> +
>>> +#include "dell-uart-backlight.h"
>>> +
>>> +struct dell_uart_backlight {
>>> +     struct device *dev;
>>> +     struct backlight_device *dell_uart_bd;
>>> +     struct mutex brightness_mutex;
>>> +     int line;
>>> +     int bl_power;
>>> +};
>>> +struct uart_8250_port *serial8250_get_port(int line);
>>> +struct tty_struct *tty;
>>> +struct file *ftty;
>>> +
>>> +unsigned int (*io_serial_in)(struct uart_port *p, int offset);
>>> +int (*uart_write)(struct tty_struct *tty, const unsigned char *buf, int count);
>>> +int (*uart_chars_in_buffer)(struct tty_struct *tty);
>>> +
>>> +static int dell_uart_write(struct uart_8250_port *up, __u8 *buf, int len)
>>> +{
>>> +     int actual = 0;
>>> +     struct uart_port *port = &up->port;
>>> +
>>> +     tty_port_tty_wakeup(&port->state->port);
>>> +     tty = tty_port_tty_get(&port->state->port);
>>> +     actual = uart_write(tty, buf, len);
>>> +     while (uart_chars_in_buffer(tty)) {
>>> +             udelay(10);
>>> +     }
>>
>> Is is possible for uart_chars_in_buffer() to always return non-zero and
>> hence we get in an infinite loop?
> It doesn't look like to return non-zero value, the value will be mask
> with (1<<12) - 1
> #define CIRC_CNT(head,tail,size) (((head) - (tail)) & ((size)-1))
> 
>>
>>> +
>>> +     return actual;
>>> +}
>>> +
>>> +static int dell_uart_read(struct uart_8250_port *up, __u8 *buf, int len)
>>> +{
>>> +     int i, retry;
>>> +     unsigned long flags;
>>> +
>>> +     spin_lock_irqsave(&up->port.lock, flags);
>>> +     for (i = 0; i < len; i++) {
>>> +             retry = 10;
>>> +             while (!(io_serial_in(&up->port, UART_LSR) & UART_LSR_DR)) {
>>> +                     if (--retry == 0)
>>> +                             break;
>>> +                     mdelay(20);
>>> +             }
>>> +
>>> +             if (retry == 0)
>>> +                     break;
>>> +             buf[i] = io_serial_in(&up->port, UART_RX);
>>> +     }
>>> +     spin_unlock_irqrestore(&up->port.lock, flags);
>>
>> spin locks should be used for small length delays, this seems to have
>> some length delays so potentially we could have some CPU eating spin
>> lock contention here.
> When we call read function that means we just write out commands and
> is trying to read back.
> In this case, we don't want to let other processes read out the data,
> so we have to use spin lock here.
> Busy waiting is bad, but I don't know if we have any other choices here.
> 
>>
>>> +
>>> +     return i;
>>> +}
>>> +
>>> +static void dell_uart_dump_cmd(const char *func, const char *prefix,
>>> +                            const char *cmd, int len)
>>> +{
>>> +     char buf[80];
>>> +
>>> +     snprintf(buf, 80, "dell_uart_backlight:%s:%s", func, prefix);
>>> +     if (len != 0)
>>> +             print_hex_dump_debug(buf, DUMP_PREFIX_NONE,
>>> +                                     16, 1, cmd, len, false);
>>> +     else
>>> +             pr_debug("dell_uart_backlight:%s:%sNULL\n", func, prefix);
>>> +
>>> +}
>>> +
>>> +/*
>>> + * checksum: SUM(Length and Cmd and Data)xor 0xFF)
>>> + */
>>> +static unsigned char dell_uart_checksum(unsigned char *buf, int len)
>>> +{
>>> +     unsigned char val = 0;
>>> +
>>> +     while (len-- > 0)
>>> +             val += buf[len];
>>> +
>>> +     return val ^ 0xff;
>>> +}
>>> +
>>> +/*
>>> + * 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 bl_power variable
>>> + */
>>> +static int dell_uart_get_bl_power(struct dell_uart_backlight *dell_pdata)
>>> +{
>>> +     return dell_pdata->bl_power;
>>> +}
>>
>> perhaps making the above function inline because it is a small helper
>> function.
> Okay.
> 
>>
>>> +
>>> +static int dell_uart_set_bl_power(struct backlight_device *bd, int power)
>>> +{
>>> +     struct dell_uart_bl_cmd *bl_cmd =
>>> +             &uart_cmd[DELL_UART_SET_BACKLIGHT_POWER];
>>> +     struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
>>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>>> +     int rx_len;
>>> +
>>> +     if (power != FB_BLANK_POWERDOWN)
>>> +             power = FB_BLANK_UNBLANK;
>>> +
>>> +     bl_cmd->cmd[2] = power?0:1;
>>
>> Minor nitpick, whitespaces could be added here, e,g. power ? 0 : 1;
> Okay
> 
>>
>>> +     bl_cmd->cmd[3] = dell_uart_checksum(bl_cmd->cmd, bl_cmd->tx_len - 1);
>>> +
>>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>>> +
>>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>>> +             pr_debug("Failed to get mutex_lock");
>>> +             return 0;
>>> +     }
>>> +
>>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>>> +
>>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>>> +
>>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>>> +
>>> +     bd->props.power = power;
>>> +     dell_pdata->bl_power = power;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int dell_uart_get_brightness(struct backlight_device *bd)
>>> +{
>>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_BRIGHTNESS];
>>> +     struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
>>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>>> +     int rx_len, brightness = 0;
>>> +
>>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>>> +
>>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>>> +             pr_debug("Failed to get mutex_lock");
>>> +             return 0;
>>> +     }
>>> +
>>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>>> +
>>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>>> +
>>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>>> +
>>> +     brightness = (unsigned int)bl_cmd->ret[2];
>>> +
>>> +     return brightness;
>>> +}
>>> +
>>> +static int dell_uart_update_status(struct backlight_device *bd)
>>> +{
>>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_SET_BRIGHTNESS];
>>> +     struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
>>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>>> +     int rx_len;
>>> +
>>> +     bl_cmd->cmd[2] = bd->props.brightness;
>>> +     bl_cmd->cmd[3] = dell_uart_checksum(bl_cmd->cmd, bl_cmd->tx_len - 1);
>>> +
>>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>>> +
>>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>>> +             pr_debug("Failed to get mutex_lock");
>>> +             return 0;
>>> +     }
>>> +
>>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>>> +
>>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>>> +
>>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>>> +
>>> +     if (bd->props.power != dell_uart_get_bl_power(dell_pdata))
>>> +             dell_uart_set_bl_power(bd, bd->props.power);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
>>> +{
>>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
>>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>>> +     int rx_len, status = 1; /* assume the scalar IC controls backlight if the command failed */
>>> +
>>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>>> +
>>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>>> +             pr_debug("Failed to get mutex_lock");
>>> +             return 0;
>>> +     }
>>> +
>>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>>> +
>>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>>> +
>>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>>> +
>>> +     if (rx_len == 4)
>>> +             status = (unsigned int)bl_cmd->ret[2];
>>> +
>>> +     return status;
>>> +}
>>> +
>>> +static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata)
>>> +{
>>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_FIRMWARE_VER];
>>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>>> +     int rx_len = 0, retry = 10;
>>> +
>>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>>> +
>>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>>> +             pr_debug("Failed to get mutex_lock");
>>> +             return -1;
>>> +     }
>>> +
>>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>>> +     while (retry-- > 0) {
>>> +             /* first byte is data length */
>>> +             dell_uart_read(uart, bl_cmd->ret, 1);
>>> +             rx_len = (int)bl_cmd->ret[0];
>>> +             if (bl_cmd->ret[0] > 80 || bl_cmd->ret[0] <= 0) {
>>> +                     pr_debug("Failed to get firmware version\n");
>>> +                     if (retry == 0) {
>>> +                             mutex_unlock(&dell_pdata->brightness_mutex);
>>> +                             return -1;
>>> +                     }
>>> +                     msleep(100);
>>> +                     continue;
>>> +             }
>>> +
>>> +             dell_uart_read(uart, bl_cmd->ret+1, rx_len-1);
>>> +             break;
>>> +     }
>>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>>> +
>>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>>> +
>>> +     pr_debug("Firmare str(%d)= %s\n", (int)bl_cmd->ret[0], bl_cmd->ret+2);
>>> +     return rx_len;
>>> +}
>>> +
>>> +static const struct backlight_ops dell_uart_backlight_ops = {
>>> +     .get_brightness = dell_uart_get_brightness,
>>> +     .update_status = dell_uart_update_status,
>>> +};
>>> +
>>> +static int dell_uart_startup(struct dell_uart_backlight *dell_pdata)
>>> +{
>>> +     struct uart_8250_port *uartp;
>>> +     struct uart_port *port;
>>> +
>>> +     dell_pdata->line = 0;
>>> +     uartp = serial8250_get_port(dell_pdata->line);
>>> +     port = &uartp->port;
>>> +     tty = port->state->port.tty;
>>> +     io_serial_in = port->serial_in;
>>> +     uart_write = tty->driver->ops->write;
>>> +     uart_chars_in_buffer = tty->driver->ops->chars_in_buffer;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int dell_uart_bl_add(struct acpi_device *dev)
>>> +{
>>> +     struct dell_uart_backlight *dell_pdata;
>>> +     struct backlight_properties props;
>>> +     struct backlight_device *dell_uart_bd;
>>> +
>>> +     dell_pdata = kzalloc(sizeof(struct dell_uart_backlight), GFP_KERNEL);
>>
>> always check for failed allocations please
> Okay
> 
>>
>>> +     dell_pdata->dev = &dev->dev;
>>> +     dell_uart_startup(dell_pdata);
>>> +     dev->driver_data = dell_pdata;
>>> +
>>> +     mutex_init(&dell_pdata->brightness_mutex);
>>> +
>>> +     if (!dell_uart_get_scalar_status(dell_pdata)) {
>>> +             udelay(50);
>>> +             /* try another command to make sure there is no scalar IC */
>>> +             if (!dell_uart_show_firmware_ver(dell_pdata) <= 0) {
>>> +                     pr_debug("Scalar doesn't in charge of brightness adjustment.\n");
>>> +                     return -1;
>>> +             }
>>> +     }
>>> +
>>> +     memset(&props, 0, sizeof(struct backlight_properties));
>>> +     props.type = BACKLIGHT_PLATFORM;
>>> +     props.max_brightness = 100;
>>> +
>>> +     dell_uart_bd = backlight_device_register("dell_uart_backlight",
>>> +                                              &dev->dev,
>>> +                                              dell_pdata,
>>> +                                              &dell_uart_backlight_ops,
>>> +                                              &props);
>>
>> check for failure, e.g.
>>
>>         if (IS_ERR(pdell_uart_bd))
>>                 ....
>>
> Okay
> 
>>> +     dell_pdata->dell_uart_bd = dell_uart_bd;
>>> +
>>> +     dell_uart_set_bl_power(dell_uart_bd, FB_BLANK_UNBLANK);
>>> +     dell_uart_bd->props.brightness = 100;
>>> +     backlight_update_status(dell_uart_bd);
>>> +
>>> +     /* unregister acpi backlight interface */
>>> +     acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int dell_uart_bl_remove(struct acpi_device *dev)
>>> +{
>>> +     struct dell_uart_backlight *dell_pdata = dev->driver_data;
>>> +
>>> +     backlight_device_unregister(dell_pdata->dell_uart_bd);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int dell_uart_bl_suspend(struct device *dev)
>>> +{
>>> +     filp_close(ftty, NULL);
>>> +     return 0;
>>> +}
>>> +
>>> +static int dell_uart_bl_resume(struct device *dev)
>>> +{
>>> +     ftty = filp_open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY, 0);
>>
>> what happens whet ftty open fails?
> It's not possible to fail here, if it fails to open it should fail at
> the beginning.
> 
>>
>>> +     return 0;
>>> +}
>>> +
>>> +static SIMPLE_DEV_PM_OPS(dell_uart_bl_pm, dell_uart_bl_suspend, dell_uart_bl_resume);
>>> +
>>> +static const struct acpi_device_id dell_uart_bl_ids[] = {
>>> +     {"DELL0501", 0},
>>> +     {"", 0},
>>> +};
>>> +
>>> +static struct acpi_driver dell_uart_backlight_driver = {
>>> +     .name = "Dell AIO serial backlight",
>>> +     .ids = dell_uart_bl_ids,
>>> +     .ops = {
>>> +             .add = dell_uart_bl_add,
>>> +             .remove = dell_uart_bl_remove,
>>> +     },
>>> +     .drv.pm = &dell_uart_bl_pm,
>>> +};
>>> +
>>> +static int __init dell_uart_bl_init(void)
>>> +{
>>> +     ftty = filp_open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY, 0);
>>
>> what happens when ftty open fails?
> When the driver is to be loaded, there should be _HID("DELL0501")
> defined in DSDT, and also there should be "PNP0501" defined, too.
> In this case, we can assume that there is /dev/ttyS0 be generated by
> serial_pnp driver which is a build-in driver and will be loaded before
> our driver.

Well, "assume nothing" is my rule of thumb, things can fail, so I'd
prefer if a paranoid check is added just in case.

> 
> There should be no such case that it'll fail to open /dev/ttyS0, the
> device should be ready before our driver is loaded.
> 
>>
>>> +
>>> +     return acpi_bus_register_driver(&dell_uart_backlight_driver);
>>> +}
>>> +
>>> +static void __exit dell_uart_bl_exit(void)
>>> +{
>>> +     filp_close(ftty, NULL);
>>> +
>>> +     acpi_bus_unregister_driver(&dell_uart_backlight_driver);
>>> +}
>>> +
>>> +module_init(dell_uart_bl_init);
>>> +module_exit(dell_uart_bl_exit);
>>> +MODULE_DEVICE_TABLE(acpi, dell_uart_bl_ids);
>>> +MODULE_DESCRIPTION("Dell AIO Serial Backlight module");
>>> +MODULE_AUTHOR("AceLan Kao <acelan.kao@canonical.com>");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/platform/x86/dell-uart-backlight.h b/drivers/platform/x86/dell-uart-backlight.h
>>> new file mode 100644
>>> index 000000000000..b8bca1230e56
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/dell-uart-backlight.h
>>> @@ -0,0 +1,88 @@
>>> +/*
>>> + *  Dell AIO Serial Backlight Driver
>>> + *
>>> + *  Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
>>> + *
>>> + *  This program is free software; you can redistribute it and/or modify
>>> + *  it under the terms of the GNU General Public License as published by
>>> + *  the Free Software Foundation; either version 2 of the License, or
>>> + *  (at your option) any later version.
>>> + *
>>> + *  This program is distributed in the hope that it will be useful,
>>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + *  GNU General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +#ifndef _DELL_UART_BACKLIGHT_H_
>>> +#define _DELL_UART_BACKLIGHT_H_
>>> +
>>> +enum {
>>> +     DELL_UART_GET_FIRMWARE_VER,
>>> +     DELL_UART_GET_BRIGHTNESS,
>>> +     DELL_UART_SET_BRIGHTNESS,
>>> +     DELL_UART_SET_BACKLIGHT_POWER,
>>> +};
>>> +
>>> +struct dell_uart_bl_cmd {
>>> +     unsigned char   cmd[10];
>>> +     unsigned char   ret[80];
>>> +     unsigned short  tx_len;
>>> +     unsigned short  rx_len;
>>> +};
>>> +
>>
>>
>> below: should these be in the main source rather than a header file?
>>
>> Also, make static structs static const if they are not going to be modified.
> Okay
> 
>>
>>> +static struct dell_uart_bl_cmd uart_cmd[] = {
>>> +     /*
>>> +      * 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 .
>>> +      */
>>> +     [DELL_UART_GET_FIRMWARE_VER] = {
>>> +             .cmd    = {0x6A, 0x06, 0x8F},
>>> +             .tx_len = 3,
>>> +     },
>>> +     /*
>>> +      * Get Brightness level: Application uses this command for scaler 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 which ranges from 0~100.
>>> +      */
>>> +     [DELL_UART_GET_BRIGHTNESS] = {
>>> +             .cmd    = {0x6A, 0x0C, 0x89},
>>> +             .ret    = {0x04, 0x0C, 0x00, 0x00},
>>> +             .tx_len = 3,
>>> +             .rx_len = 4,
>>> +     },
>>> +     /* Set Brightness level: Application uses this command for scaler to set brightness.
>>> +      * Command: 0x8A 0x0B Byte2 Checksum (Length:4 Type: 0x0A, Cmd:0x0B)
>>> +      *          where Byte2 is the brightness level which ranges from 0~100.
>>> +      * Return data: 0x03 0x0B 0xF1(Length:3,Cmd:B,checksum:0xF1)
>>> +      * Scaler must send the 3bytes ack within 1 second when success, other value if error
>>> +      */
>>> +     [DELL_UART_SET_BRIGHTNESS] = {
>>> +             .cmd    = {0x8A, 0x0B, 0x0, 0x0},
>>> +             .ret    = {0x03, 0x0B, 0xF1},
>>> +             .tx_len = 4,
>>> +             .rx_len = 3,
>>> +     },
>>> +     /*
>>> +      * Screen ON/OFF Control: Application uses this command to control screen ON or OFF.
>>> +      * Command: 0x8A 0x0E Byte2 Checksum (Length:4 Type: 0x0A, Cmd:0x0E) where
>>> +      *          Byte2=0 to turn OFF the screen.
>>> +      *          Byte2=1 to turn ON the screen
>>> +      *          Other value of Byte2 is reserved and invalid.
>>> +      * Return data: 0x03 0x0E 0xEE(Length:3,Cmd:E,checksum:0xEE)
>>> +      */
>>> +     [DELL_UART_SET_BACKLIGHT_POWER] = {
>>> +             .cmd    = {0x8A, 0x0E, 0x00, 0x0},
>>> +             .ret    = {0x03, 0x0E, 0xEE},
>>> +             .tx_len = 4,
>>> +             .rx_len = 3,
>>> +     },
>>> +};
>>> +
>>> +#endif /* _DELL_UART_BACKLIGHT_H_ */
>>>
>>
>> Colin
AceLan Kao July 17, 2018, 7:13 a.m. UTC | #4
Okay, I add a check there.
Please review the v2 patch.
Thanks.

2018-07-13 17:40 GMT+08:00 Colin Ian King <colin.king@canonical.com>:
> On 13/07/18 10:34, AceLan Kao wrote:
>> 2018-07-13 16:16 GMT+08:00 Colin Ian King <colin.king@canonical.com>:
>>> On 13/07/18 08:29, AceLan Kao wrote:
>>>> BugLink: https://bugs.launchpad.net/bugs/1727235
>>>>
>>>> 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.
>>>>
>>>> In DSDT this uart port will be defined as
>>>>    Name (_HID, "DELL0501")
>>>>    Name (_CID, EisaId ("PNP0501")
>>>> The 8250 PNP driver will be loaded by default, and this driver uses
>>>> "DELL0501" to confirm the uart port is a backlight interface and
>>>> leverage the port created by 8250 PNP driver to communicate with
>>>> the scalar board.
>>>>
>>>> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
>>>> ---
>>>>  drivers/platform/x86/Kconfig               |  14 +
>>>>  drivers/platform/x86/Makefile              |   1 +
>>>>  drivers/platform/x86/dell-uart-backlight.c | 394 +++++++++++++++++++++
>>>>  drivers/platform/x86/dell-uart-backlight.h |  88 +++++
>>>>  4 files changed, 497 insertions(+)
>>>>  create mode 100644 drivers/platform/x86/dell-uart-backlight.c
>>>>  create mode 100644 drivers/platform/x86/dell-uart-backlight.h
>>>>
>>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>>> index 683a875f3b6c..6edfa935a434 100644
>>>> --- a/drivers/platform/x86/Kconfig
>>>> +++ b/drivers/platform/x86/Kconfig
>>>> @@ -199,6 +199,20 @@ config DELL_RBTN
>>>>         To compile this driver as a module, choose M here: the module will
>>>>         be called dell-rbtn.
>>>>
>>>> +config DELL_UART_BACKLIGHT
>>>> +     tristate "Dell AIO UART Backlight driver"
>>>> +     depends on SERIAL_8250
>>>> +     depends on ACPI
>>>> +     ---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 FUJITSU_LAPTOP
>>>>       tristate "Fujitsu Laptop Extras"
>>>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>>>> index c32b34a72467..85329fd26afd 100644
>>>> --- a/drivers/platform/x86/Makefile
>>>> +++ b/drivers/platform/x86/Makefile
>>>> @@ -22,6 +22,7 @@ obj-$(CONFIG_DELL_WMI_AIO)  += dell-wmi-aio.o
>>>>  obj-$(CONFIG_DELL_WMI_LED)   += dell-wmi-led.o
>>>>  obj-$(CONFIG_DELL_SMO8800)   += dell-smo8800.o
>>>>  obj-$(CONFIG_DELL_RBTN)              += dell-rbtn.o
>>>> +obj-$(CONFIG_DELL_UART_BACKLIGHT) += dell-uart-backlight.o
>>>>  obj-$(CONFIG_ACER_WMI)               += acer-wmi.o
>>>>  obj-$(CONFIG_ACERHDF)                += acerhdf.o
>>>>  obj-$(CONFIG_HP_ACCEL)               += hp_accel.o
>>>> diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
>>>> new file mode 100644
>>>> index 000000000000..06b847a002c7
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/dell-uart-backlight.c
>>>> @@ -0,0 +1,394 @@
>>>> +/*
>>>> + *  Dell AIO Serial Backlight Driver
>>>> + *
>>>> + *  Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
>>>> + *
>>>> + *  This program is free software; you can redistribute it and/or modify
>>>> + *  it under the terms of the GNU General Public License as published by
>>>> + *  the Free Software Foundation; either version 2 of the License, or
>>>> + *  (at your option) any later version.
>>>> + *
>>>> + *  This program is distributed in the hope that it will be useful,
>>>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + *  GNU General Public License for more details.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/acpi.h>
>>>> +#include <linux/serial_8250.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/backlight.h>
>>>> +#include <acpi/video.h>
>>>> +
>>>> +#include "dell-uart-backlight.h"
>>>> +
>>>> +struct dell_uart_backlight {
>>>> +     struct device *dev;
>>>> +     struct backlight_device *dell_uart_bd;
>>>> +     struct mutex brightness_mutex;
>>>> +     int line;
>>>> +     int bl_power;
>>>> +};
>>>> +struct uart_8250_port *serial8250_get_port(int line);
>>>> +struct tty_struct *tty;
>>>> +struct file *ftty;
>>>> +
>>>> +unsigned int (*io_serial_in)(struct uart_port *p, int offset);
>>>> +int (*uart_write)(struct tty_struct *tty, const unsigned char *buf, int count);
>>>> +int (*uart_chars_in_buffer)(struct tty_struct *tty);
>>>> +
>>>> +static int dell_uart_write(struct uart_8250_port *up, __u8 *buf, int len)
>>>> +{
>>>> +     int actual = 0;
>>>> +     struct uart_port *port = &up->port;
>>>> +
>>>> +     tty_port_tty_wakeup(&port->state->port);
>>>> +     tty = tty_port_tty_get(&port->state->port);
>>>> +     actual = uart_write(tty, buf, len);
>>>> +     while (uart_chars_in_buffer(tty)) {
>>>> +             udelay(10);
>>>> +     }
>>>
>>> Is is possible for uart_chars_in_buffer() to always return non-zero and
>>> hence we get in an infinite loop?
>> It doesn't look like to return non-zero value, the value will be mask
>> with (1<<12) - 1
>> #define CIRC_CNT(head,tail,size) (((head) - (tail)) & ((size)-1))
>>
>>>
>>>> +
>>>> +     return actual;
>>>> +}
>>>> +
>>>> +static int dell_uart_read(struct uart_8250_port *up, __u8 *buf, int len)
>>>> +{
>>>> +     int i, retry;
>>>> +     unsigned long flags;
>>>> +
>>>> +     spin_lock_irqsave(&up->port.lock, flags);
>>>> +     for (i = 0; i < len; i++) {
>>>> +             retry = 10;
>>>> +             while (!(io_serial_in(&up->port, UART_LSR) & UART_LSR_DR)) {
>>>> +                     if (--retry == 0)
>>>> +                             break;
>>>> +                     mdelay(20);
>>>> +             }
>>>> +
>>>> +             if (retry == 0)
>>>> +                     break;
>>>> +             buf[i] = io_serial_in(&up->port, UART_RX);
>>>> +     }
>>>> +     spin_unlock_irqrestore(&up->port.lock, flags);
>>>
>>> spin locks should be used for small length delays, this seems to have
>>> some length delays so potentially we could have some CPU eating spin
>>> lock contention here.
>> When we call read function that means we just write out commands and
>> is trying to read back.
>> In this case, we don't want to let other processes read out the data,
>> so we have to use spin lock here.
>> Busy waiting is bad, but I don't know if we have any other choices here.
>>
>>>
>>>> +
>>>> +     return i;
>>>> +}
>>>> +
>>>> +static void dell_uart_dump_cmd(const char *func, const char *prefix,
>>>> +                            const char *cmd, int len)
>>>> +{
>>>> +     char buf[80];
>>>> +
>>>> +     snprintf(buf, 80, "dell_uart_backlight:%s:%s", func, prefix);
>>>> +     if (len != 0)
>>>> +             print_hex_dump_debug(buf, DUMP_PREFIX_NONE,
>>>> +                                     16, 1, cmd, len, false);
>>>> +     else
>>>> +             pr_debug("dell_uart_backlight:%s:%sNULL\n", func, prefix);
>>>> +
>>>> +}
>>>> +
>>>> +/*
>>>> + * checksum: SUM(Length and Cmd and Data)xor 0xFF)
>>>> + */
>>>> +static unsigned char dell_uart_checksum(unsigned char *buf, int len)
>>>> +{
>>>> +     unsigned char val = 0;
>>>> +
>>>> +     while (len-- > 0)
>>>> +             val += buf[len];
>>>> +
>>>> +     return val ^ 0xff;
>>>> +}
>>>> +
>>>> +/*
>>>> + * 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 bl_power variable
>>>> + */
>>>> +static int dell_uart_get_bl_power(struct dell_uart_backlight *dell_pdata)
>>>> +{
>>>> +     return dell_pdata->bl_power;
>>>> +}
>>>
>>> perhaps making the above function inline because it is a small helper
>>> function.
>> Okay.
>>
>>>
>>>> +
>>>> +static int dell_uart_set_bl_power(struct backlight_device *bd, int power)
>>>> +{
>>>> +     struct dell_uart_bl_cmd *bl_cmd =
>>>> +             &uart_cmd[DELL_UART_SET_BACKLIGHT_POWER];
>>>> +     struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
>>>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>>>> +     int rx_len;
>>>> +
>>>> +     if (power != FB_BLANK_POWERDOWN)
>>>> +             power = FB_BLANK_UNBLANK;
>>>> +
>>>> +     bl_cmd->cmd[2] = power?0:1;
>>>
>>> Minor nitpick, whitespaces could be added here, e,g. power ? 0 : 1;
>> Okay
>>
>>>
>>>> +     bl_cmd->cmd[3] = dell_uart_checksum(bl_cmd->cmd, bl_cmd->tx_len - 1);
>>>> +
>>>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>>>> +
>>>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>>>> +             pr_debug("Failed to get mutex_lock");
>>>> +             return 0;
>>>> +     }
>>>> +
>>>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>>>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>>>> +
>>>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>>>> +
>>>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>>>> +
>>>> +     bd->props.power = power;
>>>> +     dell_pdata->bl_power = power;
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int dell_uart_get_brightness(struct backlight_device *bd)
>>>> +{
>>>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_BRIGHTNESS];
>>>> +     struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
>>>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>>>> +     int rx_len, brightness = 0;
>>>> +
>>>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>>>> +
>>>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>>>> +             pr_debug("Failed to get mutex_lock");
>>>> +             return 0;
>>>> +     }
>>>> +
>>>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>>>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>>>> +
>>>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>>>> +
>>>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>>>> +
>>>> +     brightness = (unsigned int)bl_cmd->ret[2];
>>>> +
>>>> +     return brightness;
>>>> +}
>>>> +
>>>> +static int dell_uart_update_status(struct backlight_device *bd)
>>>> +{
>>>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_SET_BRIGHTNESS];
>>>> +     struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
>>>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>>>> +     int rx_len;
>>>> +
>>>> +     bl_cmd->cmd[2] = bd->props.brightness;
>>>> +     bl_cmd->cmd[3] = dell_uart_checksum(bl_cmd->cmd, bl_cmd->tx_len - 1);
>>>> +
>>>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>>>> +
>>>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>>>> +             pr_debug("Failed to get mutex_lock");
>>>> +             return 0;
>>>> +     }
>>>> +
>>>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>>>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>>>> +
>>>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>>>> +
>>>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>>>> +
>>>> +     if (bd->props.power != dell_uart_get_bl_power(dell_pdata))
>>>> +             dell_uart_set_bl_power(bd, bd->props.power);
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
>>>> +{
>>>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
>>>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>>>> +     int rx_len, status = 1; /* assume the scalar IC controls backlight if the command failed */
>>>> +
>>>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>>>> +
>>>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>>>> +             pr_debug("Failed to get mutex_lock");
>>>> +             return 0;
>>>> +     }
>>>> +
>>>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>>>> +     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
>>>> +
>>>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>>>> +
>>>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>>>> +
>>>> +     if (rx_len == 4)
>>>> +             status = (unsigned int)bl_cmd->ret[2];
>>>> +
>>>> +     return status;
>>>> +}
>>>> +
>>>> +static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata)
>>>> +{
>>>> +     struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_FIRMWARE_VER];
>>>> +     struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>>>> +     int rx_len = 0, retry = 10;
>>>> +
>>>> +     dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>>>> +
>>>> +     if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
>>>> +             pr_debug("Failed to get mutex_lock");
>>>> +             return -1;
>>>> +     }
>>>> +
>>>> +     dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
>>>> +     while (retry-- > 0) {
>>>> +             /* first byte is data length */
>>>> +             dell_uart_read(uart, bl_cmd->ret, 1);
>>>> +             rx_len = (int)bl_cmd->ret[0];
>>>> +             if (bl_cmd->ret[0] > 80 || bl_cmd->ret[0] <= 0) {
>>>> +                     pr_debug("Failed to get firmware version\n");
>>>> +                     if (retry == 0) {
>>>> +                             mutex_unlock(&dell_pdata->brightness_mutex);
>>>> +                             return -1;
>>>> +                     }
>>>> +                     msleep(100);
>>>> +                     continue;
>>>> +             }
>>>> +
>>>> +             dell_uart_read(uart, bl_cmd->ret+1, rx_len-1);
>>>> +             break;
>>>> +     }
>>>> +     mutex_unlock(&dell_pdata->brightness_mutex);
>>>> +
>>>> +     dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
>>>> +
>>>> +     pr_debug("Firmare str(%d)= %s\n", (int)bl_cmd->ret[0], bl_cmd->ret+2);
>>>> +     return rx_len;
>>>> +}
>>>> +
>>>> +static const struct backlight_ops dell_uart_backlight_ops = {
>>>> +     .get_brightness = dell_uart_get_brightness,
>>>> +     .update_status = dell_uart_update_status,
>>>> +};
>>>> +
>>>> +static int dell_uart_startup(struct dell_uart_backlight *dell_pdata)
>>>> +{
>>>> +     struct uart_8250_port *uartp;
>>>> +     struct uart_port *port;
>>>> +
>>>> +     dell_pdata->line = 0;
>>>> +     uartp = serial8250_get_port(dell_pdata->line);
>>>> +     port = &uartp->port;
>>>> +     tty = port->state->port.tty;
>>>> +     io_serial_in = port->serial_in;
>>>> +     uart_write = tty->driver->ops->write;
>>>> +     uart_chars_in_buffer = tty->driver->ops->chars_in_buffer;
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int dell_uart_bl_add(struct acpi_device *dev)
>>>> +{
>>>> +     struct dell_uart_backlight *dell_pdata;
>>>> +     struct backlight_properties props;
>>>> +     struct backlight_device *dell_uart_bd;
>>>> +
>>>> +     dell_pdata = kzalloc(sizeof(struct dell_uart_backlight), GFP_KERNEL);
>>>
>>> always check for failed allocations please
>> Okay
>>
>>>
>>>> +     dell_pdata->dev = &dev->dev;
>>>> +     dell_uart_startup(dell_pdata);
>>>> +     dev->driver_data = dell_pdata;
>>>> +
>>>> +     mutex_init(&dell_pdata->brightness_mutex);
>>>> +
>>>> +     if (!dell_uart_get_scalar_status(dell_pdata)) {
>>>> +             udelay(50);
>>>> +             /* try another command to make sure there is no scalar IC */
>>>> +             if (!dell_uart_show_firmware_ver(dell_pdata) <= 0) {
>>>> +                     pr_debug("Scalar doesn't in charge of brightness adjustment.\n");
>>>> +                     return -1;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     memset(&props, 0, sizeof(struct backlight_properties));
>>>> +     props.type = BACKLIGHT_PLATFORM;
>>>> +     props.max_brightness = 100;
>>>> +
>>>> +     dell_uart_bd = backlight_device_register("dell_uart_backlight",
>>>> +                                              &dev->dev,
>>>> +                                              dell_pdata,
>>>> +                                              &dell_uart_backlight_ops,
>>>> +                                              &props);
>>>
>>> check for failure, e.g.
>>>
>>>         if (IS_ERR(pdell_uart_bd))
>>>                 ....
>>>
>> Okay
>>
>>>> +     dell_pdata->dell_uart_bd = dell_uart_bd;
>>>> +
>>>> +     dell_uart_set_bl_power(dell_uart_bd, FB_BLANK_UNBLANK);
>>>> +     dell_uart_bd->props.brightness = 100;
>>>> +     backlight_update_status(dell_uart_bd);
>>>> +
>>>> +     /* unregister acpi backlight interface */
>>>> +     acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int dell_uart_bl_remove(struct acpi_device *dev)
>>>> +{
>>>> +     struct dell_uart_backlight *dell_pdata = dev->driver_data;
>>>> +
>>>> +     backlight_device_unregister(dell_pdata->dell_uart_bd);
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int dell_uart_bl_suspend(struct device *dev)
>>>> +{
>>>> +     filp_close(ftty, NULL);
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int dell_uart_bl_resume(struct device *dev)
>>>> +{
>>>> +     ftty = filp_open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY, 0);
>>>
>>> what happens whet ftty open fails?
>> It's not possible to fail here, if it fails to open it should fail at
>> the beginning.
>>
>>>
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static SIMPLE_DEV_PM_OPS(dell_uart_bl_pm, dell_uart_bl_suspend, dell_uart_bl_resume);
>>>> +
>>>> +static const struct acpi_device_id dell_uart_bl_ids[] = {
>>>> +     {"DELL0501", 0},
>>>> +     {"", 0},
>>>> +};
>>>> +
>>>> +static struct acpi_driver dell_uart_backlight_driver = {
>>>> +     .name = "Dell AIO serial backlight",
>>>> +     .ids = dell_uart_bl_ids,
>>>> +     .ops = {
>>>> +             .add = dell_uart_bl_add,
>>>> +             .remove = dell_uart_bl_remove,
>>>> +     },
>>>> +     .drv.pm = &dell_uart_bl_pm,
>>>> +};
>>>> +
>>>> +static int __init dell_uart_bl_init(void)
>>>> +{
>>>> +     ftty = filp_open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY, 0);
>>>
>>> what happens when ftty open fails?
>> When the driver is to be loaded, there should be _HID("DELL0501")
>> defined in DSDT, and also there should be "PNP0501" defined, too.
>> In this case, we can assume that there is /dev/ttyS0 be generated by
>> serial_pnp driver which is a build-in driver and will be loaded before
>> our driver.
>
> Well, "assume nothing" is my rule of thumb, things can fail, so I'd
> prefer if a paranoid check is added just in case.
>
>>
>> There should be no such case that it'll fail to open /dev/ttyS0, the
>> device should be ready before our driver is loaded.
>>
>>>
>>>> +
>>>> +     return acpi_bus_register_driver(&dell_uart_backlight_driver);
>>>> +}
>>>> +
>>>> +static void __exit dell_uart_bl_exit(void)
>>>> +{
>>>> +     filp_close(ftty, NULL);
>>>> +
>>>> +     acpi_bus_unregister_driver(&dell_uart_backlight_driver);
>>>> +}
>>>> +
>>>> +module_init(dell_uart_bl_init);
>>>> +module_exit(dell_uart_bl_exit);
>>>> +MODULE_DEVICE_TABLE(acpi, dell_uart_bl_ids);
>>>> +MODULE_DESCRIPTION("Dell AIO Serial Backlight module");
>>>> +MODULE_AUTHOR("AceLan Kao <acelan.kao@canonical.com>");
>>>> +MODULE_LICENSE("GPL");
>>>> diff --git a/drivers/platform/x86/dell-uart-backlight.h b/drivers/platform/x86/dell-uart-backlight.h
>>>> new file mode 100644
>>>> index 000000000000..b8bca1230e56
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/dell-uart-backlight.h
>>>> @@ -0,0 +1,88 @@
>>>> +/*
>>>> + *  Dell AIO Serial Backlight Driver
>>>> + *
>>>> + *  Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
>>>> + *
>>>> + *  This program is free software; you can redistribute it and/or modify
>>>> + *  it under the terms of the GNU General Public License as published by
>>>> + *  the Free Software Foundation; either version 2 of the License, or
>>>> + *  (at your option) any later version.
>>>> + *
>>>> + *  This program is distributed in the hope that it will be useful,
>>>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + *  GNU General Public License for more details.
>>>> + *
>>>> + */
>>>> +
>>>> +#ifndef _DELL_UART_BACKLIGHT_H_
>>>> +#define _DELL_UART_BACKLIGHT_H_
>>>> +
>>>> +enum {
>>>> +     DELL_UART_GET_FIRMWARE_VER,
>>>> +     DELL_UART_GET_BRIGHTNESS,
>>>> +     DELL_UART_SET_BRIGHTNESS,
>>>> +     DELL_UART_SET_BACKLIGHT_POWER,
>>>> +};
>>>> +
>>>> +struct dell_uart_bl_cmd {
>>>> +     unsigned char   cmd[10];
>>>> +     unsigned char   ret[80];
>>>> +     unsigned short  tx_len;
>>>> +     unsigned short  rx_len;
>>>> +};
>>>> +
>>>
>>>
>>> below: should these be in the main source rather than a header file?
>>>
>>> Also, make static structs static const if they are not going to be modified.
>> Okay
>>
>>>
>>>> +static struct dell_uart_bl_cmd uart_cmd[] = {
>>>> +     /*
>>>> +      * 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 .
>>>> +      */
>>>> +     [DELL_UART_GET_FIRMWARE_VER] = {
>>>> +             .cmd    = {0x6A, 0x06, 0x8F},
>>>> +             .tx_len = 3,
>>>> +     },
>>>> +     /*
>>>> +      * Get Brightness level: Application uses this command for scaler 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 which ranges from 0~100.
>>>> +      */
>>>> +     [DELL_UART_GET_BRIGHTNESS] = {
>>>> +             .cmd    = {0x6A, 0x0C, 0x89},
>>>> +             .ret    = {0x04, 0x0C, 0x00, 0x00},
>>>> +             .tx_len = 3,
>>>> +             .rx_len = 4,
>>>> +     },
>>>> +     /* Set Brightness level: Application uses this command for scaler to set brightness.
>>>> +      * Command: 0x8A 0x0B Byte2 Checksum (Length:4 Type: 0x0A, Cmd:0x0B)
>>>> +      *          where Byte2 is the brightness level which ranges from 0~100.
>>>> +      * Return data: 0x03 0x0B 0xF1(Length:3,Cmd:B,checksum:0xF1)
>>>> +      * Scaler must send the 3bytes ack within 1 second when success, other value if error
>>>> +      */
>>>> +     [DELL_UART_SET_BRIGHTNESS] = {
>>>> +             .cmd    = {0x8A, 0x0B, 0x0, 0x0},
>>>> +             .ret    = {0x03, 0x0B, 0xF1},
>>>> +             .tx_len = 4,
>>>> +             .rx_len = 3,
>>>> +     },
>>>> +     /*
>>>> +      * Screen ON/OFF Control: Application uses this command to control screen ON or OFF.
>>>> +      * Command: 0x8A 0x0E Byte2 Checksum (Length:4 Type: 0x0A, Cmd:0x0E) where
>>>> +      *          Byte2=0 to turn OFF the screen.
>>>> +      *          Byte2=1 to turn ON the screen
>>>> +      *          Other value of Byte2 is reserved and invalid.
>>>> +      * Return data: 0x03 0x0E 0xEE(Length:3,Cmd:E,checksum:0xEE)
>>>> +      */
>>>> +     [DELL_UART_SET_BACKLIGHT_POWER] = {
>>>> +             .cmd    = {0x8A, 0x0E, 0x00, 0x0},
>>>> +             .ret    = {0x03, 0x0E, 0xEE},
>>>> +             .tx_len = 4,
>>>> +             .rx_len = 3,
>>>> +     },
>>>> +};
>>>> +
>>>> +#endif /* _DELL_UART_BACKLIGHT_H_ */
>>>>
>>>
>>> Colin
>
diff mbox series

Patch

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 683a875f3b6c..6edfa935a434 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -199,6 +199,20 @@  config DELL_RBTN
 	  To compile this driver as a module, choose M here: the module will
 	  be called dell-rbtn.
 
+config DELL_UART_BACKLIGHT
+	tristate "Dell AIO UART Backlight driver"
+	depends on SERIAL_8250
+	depends on ACPI
+	---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 FUJITSU_LAPTOP
 	tristate "Fujitsu Laptop Extras"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index c32b34a72467..85329fd26afd 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -22,6 +22,7 @@  obj-$(CONFIG_DELL_WMI_AIO)	+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_LED)	+= dell-wmi-led.o
 obj-$(CONFIG_DELL_SMO8800)	+= dell-smo8800.o
 obj-$(CONFIG_DELL_RBTN)		+= dell-rbtn.o
+obj-$(CONFIG_DELL_UART_BACKLIGHT) += dell-uart-backlight.o
 obj-$(CONFIG_ACER_WMI)		+= acer-wmi.o
 obj-$(CONFIG_ACERHDF)		+= acerhdf.o
 obj-$(CONFIG_HP_ACCEL)		+= hp_accel.o
diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
new file mode 100644
index 000000000000..06b847a002c7
--- /dev/null
+++ b/drivers/platform/x86/dell-uart-backlight.c
@@ -0,0 +1,394 @@ 
+/*
+ *  Dell AIO Serial Backlight Driver
+ *
+ *  Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/serial_8250.h>
+#include <linux/delay.h>
+#include <linux/backlight.h>
+#include <acpi/video.h>
+
+#include "dell-uart-backlight.h"
+
+struct dell_uart_backlight {
+	struct device *dev;
+	struct backlight_device *dell_uart_bd;
+	struct mutex brightness_mutex;
+	int line;
+	int bl_power;
+};
+struct uart_8250_port *serial8250_get_port(int line);
+struct tty_struct *tty;
+struct file *ftty;
+
+unsigned int (*io_serial_in)(struct uart_port *p, int offset);
+int (*uart_write)(struct tty_struct *tty, const unsigned char *buf, int count);
+int (*uart_chars_in_buffer)(struct tty_struct *tty);
+
+static int dell_uart_write(struct uart_8250_port *up, __u8 *buf, int len)
+{
+	int actual = 0;
+	struct uart_port *port = &up->port;
+
+	tty_port_tty_wakeup(&port->state->port);
+	tty = tty_port_tty_get(&port->state->port);
+	actual = uart_write(tty, buf, len);
+	while (uart_chars_in_buffer(tty)) {
+		udelay(10);
+	}
+
+	return actual;
+}
+
+static int dell_uart_read(struct uart_8250_port *up, __u8 *buf, int len)
+{
+	int i, retry;
+	unsigned long flags;
+
+	spin_lock_irqsave(&up->port.lock, flags);
+	for (i = 0; i < len; i++) {
+		retry = 10;
+		while (!(io_serial_in(&up->port, UART_LSR) & UART_LSR_DR)) {
+			if (--retry == 0)
+				break;
+			mdelay(20);
+		}
+
+		if (retry == 0)
+			break;
+		buf[i] = io_serial_in(&up->port, UART_RX);
+	}
+	spin_unlock_irqrestore(&up->port.lock, flags);
+
+	return i;
+}
+
+static void dell_uart_dump_cmd(const char *func, const char *prefix,
+			       const char *cmd, int len)
+{
+	char buf[80];
+
+	snprintf(buf, 80, "dell_uart_backlight:%s:%s", func, prefix);
+	if (len != 0)
+		print_hex_dump_debug(buf, DUMP_PREFIX_NONE,
+					16, 1, cmd, len, false);
+	else
+		pr_debug("dell_uart_backlight:%s:%sNULL\n", func, prefix);
+
+}
+
+/*
+ * checksum: SUM(Length and Cmd and Data)xor 0xFF)
+ */
+static unsigned char dell_uart_checksum(unsigned char *buf, int len)
+{
+	unsigned char val = 0;
+
+	while (len-- > 0)
+		val += buf[len];
+
+	return val ^ 0xff;
+}
+
+/*
+ * 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 bl_power variable
+ */
+static int dell_uart_get_bl_power(struct dell_uart_backlight *dell_pdata)
+{
+	return dell_pdata->bl_power;
+}
+
+static int dell_uart_set_bl_power(struct backlight_device *bd, int power)
+{
+	struct dell_uart_bl_cmd *bl_cmd =
+		&uart_cmd[DELL_UART_SET_BACKLIGHT_POWER];
+	struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
+	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
+	int rx_len;
+
+	if (power != FB_BLANK_POWERDOWN)
+		power = FB_BLANK_UNBLANK;
+
+	bl_cmd->cmd[2] = power?0:1;
+	bl_cmd->cmd[3] = dell_uart_checksum(bl_cmd->cmd, bl_cmd->tx_len - 1);
+
+	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
+
+	if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
+		pr_debug("Failed to get mutex_lock");
+		return 0;
+	}
+
+	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
+	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
+
+	mutex_unlock(&dell_pdata->brightness_mutex);
+
+	dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
+
+	bd->props.power = power;
+	dell_pdata->bl_power = power;
+
+	return 0;
+}
+
+static int dell_uart_get_brightness(struct backlight_device *bd)
+{
+	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_BRIGHTNESS];
+	struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
+	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
+	int rx_len, brightness = 0;
+
+	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
+
+	if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
+		pr_debug("Failed to get mutex_lock");
+		return 0;
+	}
+
+	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
+	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
+
+	mutex_unlock(&dell_pdata->brightness_mutex);
+
+	dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
+
+	brightness = (unsigned int)bl_cmd->ret[2];
+
+	return brightness;
+}
+
+static int dell_uart_update_status(struct backlight_device *bd)
+{
+	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_SET_BRIGHTNESS];
+	struct dell_uart_backlight *dell_pdata = bl_get_data(bd);
+	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
+	int rx_len;
+
+	bl_cmd->cmd[2] = bd->props.brightness;
+	bl_cmd->cmd[3] = dell_uart_checksum(bl_cmd->cmd, bl_cmd->tx_len - 1);
+
+	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
+
+	if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
+		pr_debug("Failed to get mutex_lock");
+		return 0;
+	}
+
+	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
+	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
+
+	mutex_unlock(&dell_pdata->brightness_mutex);
+
+	dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
+
+	if (bd->props.power != dell_uart_get_bl_power(dell_pdata))
+		dell_uart_set_bl_power(bd, bd->props.power);
+
+	return 0;
+}
+
+static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
+{
+	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
+	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
+	int rx_len, status = 1; /* assume the scalar IC controls backlight if the command failed */
+
+	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
+
+	if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
+		pr_debug("Failed to get mutex_lock");
+		return 0;
+	}
+
+	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
+	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
+
+	mutex_unlock(&dell_pdata->brightness_mutex);
+
+	dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
+
+	if (rx_len == 4)
+		status = (unsigned int)bl_cmd->ret[2];
+
+	return status;
+}
+
+static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata)
+{
+	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_FIRMWARE_VER];
+	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
+	int rx_len = 0, retry = 10;
+
+	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
+
+	if (mutex_lock_killable(&dell_pdata->brightness_mutex) < 0) {
+		pr_debug("Failed to get mutex_lock");
+		return -1;
+	}
+
+	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
+	while (retry-- > 0) {
+		/* first byte is data length */
+		dell_uart_read(uart, bl_cmd->ret, 1);
+		rx_len = (int)bl_cmd->ret[0];
+		if (bl_cmd->ret[0] > 80 || bl_cmd->ret[0] <= 0) {
+			pr_debug("Failed to get firmware version\n");
+			if (retry == 0) {
+				mutex_unlock(&dell_pdata->brightness_mutex);
+				return -1;
+			}
+			msleep(100);
+			continue;
+		}
+
+		dell_uart_read(uart, bl_cmd->ret+1, rx_len-1);
+		break;
+	}
+	mutex_unlock(&dell_pdata->brightness_mutex);
+
+	dell_uart_dump_cmd(__func__, "rx: ", bl_cmd->ret, rx_len);
+
+	pr_debug("Firmare str(%d)= %s\n", (int)bl_cmd->ret[0], bl_cmd->ret+2);
+	return rx_len;
+}
+
+static const struct backlight_ops dell_uart_backlight_ops = {
+	.get_brightness = dell_uart_get_brightness,
+	.update_status = dell_uart_update_status,
+};
+
+static int dell_uart_startup(struct dell_uart_backlight *dell_pdata)
+{
+	struct uart_8250_port *uartp;
+	struct uart_port *port;
+
+	dell_pdata->line = 0;
+	uartp = serial8250_get_port(dell_pdata->line);
+	port = &uartp->port;
+	tty = port->state->port.tty;
+	io_serial_in = port->serial_in;
+	uart_write = tty->driver->ops->write;
+	uart_chars_in_buffer = tty->driver->ops->chars_in_buffer;
+
+	return 0;
+}
+
+static int dell_uart_bl_add(struct acpi_device *dev)
+{
+	struct dell_uart_backlight *dell_pdata;
+	struct backlight_properties props;
+	struct backlight_device *dell_uart_bd;
+
+	dell_pdata = kzalloc(sizeof(struct dell_uart_backlight), GFP_KERNEL);
+	dell_pdata->dev = &dev->dev;
+	dell_uart_startup(dell_pdata);
+	dev->driver_data = dell_pdata;
+
+	mutex_init(&dell_pdata->brightness_mutex);
+
+	if (!dell_uart_get_scalar_status(dell_pdata)) {
+		udelay(50);
+		/* try another command to make sure there is no scalar IC */
+		if (!dell_uart_show_firmware_ver(dell_pdata) <= 0) {
+			pr_debug("Scalar doesn't in charge of brightness adjustment.\n");
+			return -1;
+		}
+	}
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_PLATFORM;
+	props.max_brightness = 100;
+
+	dell_uart_bd = backlight_device_register("dell_uart_backlight",
+						 &dev->dev,
+						 dell_pdata,
+						 &dell_uart_backlight_ops,
+						 &props);
+	dell_pdata->dell_uart_bd = dell_uart_bd;
+
+	dell_uart_set_bl_power(dell_uart_bd, FB_BLANK_UNBLANK);
+	dell_uart_bd->props.brightness = 100;
+	backlight_update_status(dell_uart_bd);
+
+	/* unregister acpi backlight interface */
+	acpi_video_set_dmi_backlight_type(acpi_backlight_vendor);
+
+	return 0;
+}
+
+static int dell_uart_bl_remove(struct acpi_device *dev)
+{
+	struct dell_uart_backlight *dell_pdata = dev->driver_data;
+
+	backlight_device_unregister(dell_pdata->dell_uart_bd);
+
+	return 0;
+}
+
+static int dell_uart_bl_suspend(struct device *dev)
+{
+	filp_close(ftty, NULL);
+	return 0;
+}
+
+static int dell_uart_bl_resume(struct device *dev)
+{
+	ftty = filp_open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY, 0);
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(dell_uart_bl_pm, dell_uart_bl_suspend, dell_uart_bl_resume);
+
+static const struct acpi_device_id dell_uart_bl_ids[] = {
+	{"DELL0501", 0},
+	{"", 0},
+};
+
+static struct acpi_driver dell_uart_backlight_driver = {
+	.name = "Dell AIO serial backlight",
+	.ids = dell_uart_bl_ids,
+	.ops = {
+		.add = dell_uart_bl_add,
+		.remove = dell_uart_bl_remove,
+	},
+	.drv.pm = &dell_uart_bl_pm,
+};
+
+static int __init dell_uart_bl_init(void)
+{
+	ftty = filp_open("/dev/ttyS0", O_RDWR | O_NOCTTY | O_NDELAY, 0);
+
+	return acpi_bus_register_driver(&dell_uart_backlight_driver);
+}
+
+static void __exit dell_uart_bl_exit(void)
+{
+	filp_close(ftty, NULL);
+
+	acpi_bus_unregister_driver(&dell_uart_backlight_driver);
+}
+
+module_init(dell_uart_bl_init);
+module_exit(dell_uart_bl_exit);
+MODULE_DEVICE_TABLE(acpi, dell_uart_bl_ids);
+MODULE_DESCRIPTION("Dell AIO Serial Backlight module");
+MODULE_AUTHOR("AceLan Kao <acelan.kao@canonical.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-uart-backlight.h b/drivers/platform/x86/dell-uart-backlight.h
new file mode 100644
index 000000000000..b8bca1230e56
--- /dev/null
+++ b/drivers/platform/x86/dell-uart-backlight.h
@@ -0,0 +1,88 @@ 
+/*
+ *  Dell AIO Serial Backlight Driver
+ *
+ *  Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+
+#ifndef _DELL_UART_BACKLIGHT_H_
+#define _DELL_UART_BACKLIGHT_H_
+
+enum {
+	DELL_UART_GET_FIRMWARE_VER,
+	DELL_UART_GET_BRIGHTNESS,
+	DELL_UART_SET_BRIGHTNESS,
+	DELL_UART_SET_BACKLIGHT_POWER,
+};
+
+struct dell_uart_bl_cmd {
+	unsigned char	cmd[10];
+	unsigned char	ret[80];
+	unsigned short	tx_len;
+	unsigned short	rx_len;
+};
+
+static struct dell_uart_bl_cmd uart_cmd[] = {
+	/*
+	 * 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 .
+	 */
+	[DELL_UART_GET_FIRMWARE_VER] = {
+		.cmd	= {0x6A, 0x06, 0x8F},
+		.tx_len	= 3,
+	},
+	/*
+	 * Get Brightness level: Application uses this command for scaler 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 which ranges from 0~100.
+	 */
+	[DELL_UART_GET_BRIGHTNESS] = {
+		.cmd	= {0x6A, 0x0C, 0x89},
+		.ret	= {0x04, 0x0C, 0x00, 0x00},
+		.tx_len	= 3,
+		.rx_len	= 4,
+	},
+	/* Set Brightness level: Application uses this command for scaler to set brightness.
+	 * Command: 0x8A 0x0B Byte2 Checksum (Length:4 Type: 0x0A, Cmd:0x0B)
+	 * 	    where Byte2 is the brightness level which ranges from 0~100.
+	 * Return data: 0x03 0x0B 0xF1(Length:3,Cmd:B,checksum:0xF1)
+	 * Scaler must send the 3bytes ack within 1 second when success, other value if error
+	 */
+	[DELL_UART_SET_BRIGHTNESS] = {
+		.cmd	= {0x8A, 0x0B, 0x0, 0x0},
+		.ret	= {0x03, 0x0B, 0xF1},
+		.tx_len	= 4,
+		.rx_len	= 3,
+	},
+	/*
+	 * Screen ON/OFF Control: Application uses this command to control screen ON or OFF.
+	 * Command: 0x8A 0x0E Byte2 Checksum (Length:4 Type: 0x0A, Cmd:0x0E) where
+	 * 	    Byte2=0 to turn OFF the screen.
+	 * 	    Byte2=1 to turn ON the screen
+	 * 	    Other value of Byte2 is reserved and invalid.
+	 * Return data: 0x03 0x0E 0xEE(Length:3,Cmd:E,checksum:0xEE)
+	 */
+	[DELL_UART_SET_BACKLIGHT_POWER] = {
+		.cmd	= {0x8A, 0x0E, 0x00, 0x0},
+		.ret	= {0x03, 0x0E, 0xEE},
+		.tx_len	= 4,
+		.rx_len	= 3,
+	},
+};
+
+#endif /* _DELL_UART_BACKLIGHT_H_ */