On 26/10/17 16:54, mario.limoncie...@dell.com wrote:
+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);
+
+       memset(&props, 0, sizeof(struct backlight_properties));
+       props.type = BACKLIGHT_PLATFORM;
+       props.max_brightness = 100;

Is the memset of zero actually necessary?  Of the items in
backlight_properties:
* you set brightness a few lines later
(after it's been memcpy'ed in the backlight device)
* You set max brightness explicitly
* You set power a few lines below

So the only attribute missing is fb_blank.  That's marked "to be removed".

fb_blank may be scheduled for removal but it is still honored by the backlight core so it should definitely not be left undefined.

I reviewed this recently and concluded that all drivers default it to unblanked (mostly by a memset like the one above). Without this then I think there is a risk that some code paths through DRM drivers won't turn the backlight on.


Daniel.


+
+       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_show_firmware_ver(dell_pdata);
+       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);

Since you change this when the driver is loaded, you should cache the old
value and restore it when the driver is unloaded too.

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

I guess this is to prevent other userspace apps from accessing the port
simultaneously right?  I think you'll need to check if the open failed
IS_ERR(ftty)
or similar to make sure that this driver would fail to load if the file
didn't exist or was opened by userspace with an exclusive lock or something.

+
+       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....@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 0000000..b8bca12
--- /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....@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_ */
--
2.7.4


Reply via email to