From: Kerem Karabay <kekrby@gmail.com>

This commit adds a driver for the backlight of Apple Touch Bars on x86
Macs. Note that currently only T2 Macs are supported.

This driver is based on previous work done by Ronald Tschalär
<ronald@innovation.ch>.

Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Co-developed-by: Aditya Garg <gargaditya08@live.com>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 MAINTAINERS                  |   6 +
 drivers/hid/Kconfig          |  10 ++
 drivers/hid/Makefile         |   1 +
 drivers/hid/hid-appletb-bl.c | 206 +++++++++++++++++++++++++++++++++++
 drivers/hid/hid-quirks.c     |   4 +-
 5 files changed, 226 insertions(+), 1 deletion(-)
 create mode 100644 drivers/hid/hid-appletb-bl.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8766f3e5e..ac27f41d4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9931,6 +9931,12 @@ F:	include/linux/pm.h
 F:	include/linux/suspend.h
 F:	kernel/power/
 
+HID APPLE TOUCH BAR DRIVERS
+M:	Kerem Karabay <kekrby@gmail.com>
+L:	linux-input@vger.kernel.org
+S:	Maintained
+F:	drivers/hid/hid-appletb-*
+
 HID CORE LAYER
 M:	Jiri Kosina <jikos@kernel.org>
 M:	Benjamin Tissoires <bentiss@kernel.org>
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 08446c89e..4988c1fb2 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -148,6 +148,16 @@ config HID_APPLEIR
 
 	Say Y here if you want support for Apple infrared remote control.
 
+config HID_APPLETB_BL
+	tristate "Apple Touch Bar Backlight"
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want support for the backlight of Touch Bars on x86
+	  MacBook Pros.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called hid-appletb-bl.
+
 config HID_ASUS
 	tristate "Asus"
 	depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index e40f1ddeb..1d825a474 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_HID_ALPS)		+= hid-alps.o
 obj-$(CONFIG_HID_ACRUX)		+= hid-axff.o
 obj-$(CONFIG_HID_APPLE)		+= hid-apple.o
 obj-$(CONFIG_HID_APPLEIR)	+= hid-appleir.o
+obj-$(CONFIG_HID_APPLETB_BL)	+= hid-appletb-bl.o
 obj-$(CONFIG_HID_CREATIVE_SB0540)	+= hid-creative-sb0540.o
 obj-$(CONFIG_HID_ASUS)		+= hid-asus.o
 obj-$(CONFIG_HID_AUREAL)	+= hid-aureal.o
diff --git a/drivers/hid/hid-appletb-bl.c b/drivers/hid/hid-appletb-bl.c
new file mode 100644
index 000000000..00bbe45df
--- /dev/null
+++ b/drivers/hid/hid-appletb-bl.c
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Apple Touch Bar Backlight Driver
+ *
+ * Copyright (c) 2017-2018 Ronald Tschalär
+ * Copyright (c) 2022-2023 Kerem Karabay <kekrby@gmail.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/hid.h>
+#include <linux/backlight.h>
+
+#include "hid-ids.h"
+
+#define APPLETB_BL_ON			1
+#define APPLETB_BL_DIM			3
+#define APPLETB_BL_OFF			4
+
+#define HID_UP_APPLEVENDOR_TB_BL	0xff120000
+
+#define HID_VD_APPLE_TB_BRIGHTNESS	0xff120001
+#define HID_USAGE_AUX1			0xff120020
+#define HID_USAGE_BRIGHTNESS		0xff120021
+
+static int appletb_bl_def_brightness = 2;
+module_param_named(brightness, appletb_bl_def_brightness, int, 0444);
+MODULE_PARM_DESC(brightness, "Default brightness:\n"
+			 "    0 - Touchbar is off\n"
+			 "    1 - Dim brightness\n"
+			 "    [2] - Full brightness");
+
+struct appletb_bl {
+	struct hid_field *aux1_field, *brightness_field;
+	struct backlight_device *bdev;
+
+	bool full_on;
+};
+
+const u8 appletb_bl_brightness_map[] = {
+	APPLETB_BL_OFF,
+	APPLETB_BL_DIM,
+	APPLETB_BL_ON
+};
+
+static int appletb_bl_set_brightness(struct appletb_bl *bl, u8 brightness)
+{
+	struct hid_report *report = bl->brightness_field->report;
+	struct hid_device *hdev = report->device;
+	int ret;
+
+	ret = hid_set_field(bl->aux1_field, 0, 1);
+	if (ret) {
+		hid_err(hdev, "Failed to set auxiliary field (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	ret = hid_set_field(bl->brightness_field, 0, brightness);
+	if (ret) {
+		hid_err(hdev, "Failed to set brightness field (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	if (!bl->full_on) {
+		ret = hid_hw_power(hdev, PM_HINT_FULLON);
+		if (ret < 0) {
+			hid_err(hdev, "Device didn't power on (%pe)\n", ERR_PTR(ret));
+			return ret;
+		}
+
+		bl->full_on = true;
+	}
+
+	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
+
+	if (brightness == APPLETB_BL_OFF) {
+		hid_hw_power(hdev, PM_HINT_NORMAL);
+		bl->full_on = false;
+	}
+
+	return 0;
+}
+
+static int appletb_bl_update_status(struct backlight_device *bdev)
+{
+	struct appletb_bl *bl = bl_get_data(bdev);
+	u16 brightness;
+
+	if (bdev->props.state & BL_CORE_SUSPENDED)
+		brightness = 0;
+	else
+		brightness = backlight_get_brightness(bdev);
+
+	return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
+}
+
+static const struct backlight_ops appletb_bl_backlight_ops = {
+	.options = BL_CORE_SUSPENDRESUME,
+	.update_status = appletb_bl_update_status,
+};
+
+static int appletb_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	struct hid_field *aux1_field, *brightness_field;
+	struct backlight_properties bl_props = { 0 };
+	struct device *dev = &hdev->dev;
+	struct appletb_bl *bl;
+	int ret;
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "HID parse failed\n");
+
+	aux1_field = hid_find_field(hdev, HID_FEATURE_REPORT,
+				    HID_VD_APPLE_TB_BRIGHTNESS, HID_USAGE_AUX1);
+
+	brightness_field = hid_find_field(hdev, HID_FEATURE_REPORT,
+					  HID_VD_APPLE_TB_BRIGHTNESS, HID_USAGE_BRIGHTNESS);
+
+	if (!aux1_field || !brightness_field)
+		return -ENODEV;
+
+	if (aux1_field->report != brightness_field->report)
+		return dev_err_probe(dev, -ENODEV, "Encountered unexpected report structure\n");
+
+	bl = devm_kzalloc(dev, sizeof(*bl), GFP_KERNEL);
+	if (!bl)
+		return -ENOMEM;
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
+	if (ret)
+		return dev_err_probe(dev, ret, "HID hardware start failed\n");
+
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		dev_err_probe(dev, ret, "HID hardware open failed\n");
+		goto stop_hw;
+	}
+
+	bl->aux1_field = aux1_field;
+	bl->brightness_field = brightness_field;
+
+	if (appletb_bl_def_brightness == 0)
+		ret = appletb_bl_set_brightness(bl, APPLETB_BL_OFF);
+	else if (appletb_bl_def_brightness == 1)
+		ret = appletb_bl_set_brightness(bl, APPLETB_BL_DIM);
+	else
+		ret = appletb_bl_set_brightness(bl, APPLETB_BL_ON);
+
+	if (ret) {
+		dev_err_probe(dev, ret, "Failed to set touch bar brightness to off\n");
+		goto close_hw;
+	}
+
+	bl_props.type = BACKLIGHT_RAW;
+	bl_props.max_brightness = ARRAY_SIZE(appletb_bl_brightness_map) - 1;
+
+	bl->bdev = devm_backlight_device_register(dev, "appletb_backlight", dev, bl,
+						  &appletb_bl_backlight_ops, &bl_props);
+	if (IS_ERR(bl->bdev)) {
+		ret = PTR_ERR(bl->bdev);
+		dev_err_probe(dev, ret, "Failed to register backlight device\n");
+		goto close_hw;
+	}
+
+	hid_set_drvdata(hdev, bl);
+
+	return 0;
+
+close_hw:
+	hid_hw_close(hdev);
+stop_hw:
+	hid_hw_stop(hdev);
+
+	return ret;
+}
+
+static void appletb_bl_remove(struct hid_device *hdev)
+{
+	struct appletb_bl *bl = hid_get_drvdata(hdev);
+
+	appletb_bl_set_brightness(bl, APPLETB_BL_OFF);
+
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
+}
+
+static const struct hid_device_id appletb_bl_hid_ids[] = {
+	/* MacBook Pro's 2018, 2019, with T2 chip: iBridge DFR Brightness */
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, appletb_bl_hid_ids);
+
+static struct hid_driver appletb_bl_hid_driver = {
+	.name = "hid-appletb-bl",
+	.id_table = appletb_bl_hid_ids,
+	.probe = appletb_bl_probe,
+	.remove = appletb_bl_remove,
+};
+module_hid_driver(appletb_bl_hid_driver);
+
+MODULE_AUTHOR("Ronald Tschalär");
+MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
+MODULE_DESCRIPTION("MacBookPro Touch Bar Backlight Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index e0bbf0c63..818d41a35 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -328,7 +328,6 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_2021) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_FINGERPRINT_2021) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
 #endif
 #if IS_ENABLED(CONFIG_HID_APPLEIR)
@@ -338,6 +337,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL5) },
 #endif
+#if IS_ENABLED(CONFIG_HID_APPLETB_BL)
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
+#endif
 #if IS_ENABLED(CONFIG_HID_ASUS)
 	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_I2C_KEYBOARD) },
 	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_I2C_TOUCHPAD) },
-- 
2.43.0

On 2024-08-08 13:50:33+0000, Aditya Garg wrote:
> From: Kerem Karabay <kekrby@gmail.com>
> 
> This commit adds a driver for the backlight of Apple Touch Bars on x86
> Macs. Note that currently only T2 Macs are supported.
> 
> This driver is based on previous work done by Ronald Tschalär
> <ronald@innovation.ch>.
> 
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> Co-developed-by: Aditya Garg <gargaditya08@live.com>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
>  MAINTAINERS                  |   6 +
>  drivers/hid/Kconfig          |  10 ++
>  drivers/hid/Makefile         |   1 +
>  drivers/hid/hid-appletb-bl.c | 206 +++++++++++++++++++++++++++++++++++
>  drivers/hid/hid-quirks.c     |   4 +-
>  5 files changed, 226 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/hid/hid-appletb-bl.c
> 

<snip>

> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index e40f1ddeb..1d825a474 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_HID_ALPS)		+= hid-alps.o
>  obj-$(CONFIG_HID_ACRUX)		+= hid-axff.o
>  obj-$(CONFIG_HID_APPLE)		+= hid-apple.o
>  obj-$(CONFIG_HID_APPLEIR)	+= hid-appleir.o
> +obj-$(CONFIG_HID_APPLETB_BL)	+= hid-appletb-bl.o
>  obj-$(CONFIG_HID_CREATIVE_SB0540)	+= hid-creative-sb0540.o
>  obj-$(CONFIG_HID_ASUS)		+= hid-asus.o
>  obj-$(CONFIG_HID_AUREAL)	+= hid-aureal.o
> diff --git a/drivers/hid/hid-appletb-bl.c b/drivers/hid/hid-appletb-bl.c
> new file mode 100644
> index 000000000..00bbe45df
> --- /dev/null
> +++ b/drivers/hid/hid-appletb-bl.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Touch Bar Backlight Driver
> + *
> + * Copyright (c) 2017-2018 Ronald Tschalär
> + * Copyright (c) 2022-2023 Kerem Karabay <kekrby@gmail.com>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

This is unused.

#include <linux/device.h>

> +#include <linux/hid.h>
> +#include <linux/backlight.h>
> +
> +#include "hid-ids.h"
> +
> +#define APPLETB_BL_ON			1
> +#define APPLETB_BL_DIM			3
> +#define APPLETB_BL_OFF			4
> +
> +#define HID_UP_APPLEVENDOR_TB_BL	0xff120000
> +
> +#define HID_VD_APPLE_TB_BRIGHTNESS	0xff120001
> +#define HID_USAGE_AUX1			0xff120020
> +#define HID_USAGE_BRIGHTNESS		0xff120021
> +
> +static int appletb_bl_def_brightness = 2;
> +module_param_named(brightness, appletb_bl_def_brightness, int, 0444);
> +MODULE_PARM_DESC(brightness, "Default brightness:\n"
> +			 "    0 - Touchbar is off\n"
> +			 "    1 - Dim brightness\n"
> +			 "    [2] - Full brightness");
> +
> +struct appletb_bl {
> +	struct hid_field *aux1_field, *brightness_field;
> +	struct backlight_device *bdev;
> +
> +	bool full_on;
> +};
> +
> +const u8 appletb_bl_brightness_map[] = {

static?

> +	APPLETB_BL_OFF,
> +	APPLETB_BL_DIM,
> +	APPLETB_BL_ON

The last element is not a sentinel element, so it should have comma.

> +};
> +
> +static int appletb_bl_set_brightness(struct appletb_bl *bl, u8 brightness)
> +{
> +	struct hid_report *report = bl->brightness_field->report;
> +	struct hid_device *hdev = report->device;
> +	int ret;
> +
> +	ret = hid_set_field(bl->aux1_field, 0, 1);
> +	if (ret) {
> +		hid_err(hdev, "Failed to set auxiliary field (%pe)\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = hid_set_field(bl->brightness_field, 0, brightness);
> +	if (ret) {
> +		hid_err(hdev, "Failed to set brightness field (%pe)\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	if (!bl->full_on) {
> +		ret = hid_hw_power(hdev, PM_HINT_FULLON);
> +		if (ret < 0) {
> +			hid_err(hdev, "Device didn't power on (%pe)\n", ERR_PTR(ret));
> +			return ret;
> +		}
> +
> +		bl->full_on = true;
> +	}
> +
> +	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> +
> +	if (brightness == APPLETB_BL_OFF) {
> +		hid_hw_power(hdev, PM_HINT_NORMAL);
> +		bl->full_on = false;
> +	}
> +
> +	return 0;
> +}
> +
> +static int appletb_bl_update_status(struct backlight_device *bdev)
> +{
> +	struct appletb_bl *bl = bl_get_data(bdev);
> +	u16 brightness;
> +
> +	if (bdev->props.state & BL_CORE_SUSPENDED)
> +		brightness = 0;

From backlight.h:

 * backlight drivers are expected to use backlight_is_blank()
 * in their update_status() operation rather than reading the
 * state property.

Seems to be applicable here.

Also the hardcoded "0" as index into appletb_bl_brightness_map could be
avoided by some restructuring.

> +	else
> +		brightness = backlight_get_brightness(bdev);
> +
> +	return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
> +}
> +
> +static const struct backlight_ops appletb_bl_backlight_ops = {
> +	.options = BL_CORE_SUSPENDRESUME,
> +	.update_status = appletb_bl_update_status,
> +};
> +
> +static int appletb_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct hid_field *aux1_field, *brightness_field;
> +	struct backlight_properties bl_props = { 0 };
> +	struct device *dev = &hdev->dev;
> +	struct appletb_bl *bl;
> +	int ret;
> +
> +	ret = hid_parse(hdev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "HID parse failed\n");
> +
> +	aux1_field = hid_find_field(hdev, HID_FEATURE_REPORT,
> +				    HID_VD_APPLE_TB_BRIGHTNESS, HID_USAGE_AUX1);
> +
> +	brightness_field = hid_find_field(hdev, HID_FEATURE_REPORT,
> +					  HID_VD_APPLE_TB_BRIGHTNESS, HID_USAGE_BRIGHTNESS);
> +
> +	if (!aux1_field || !brightness_field)
> +		return -ENODEV;
> +
> +	if (aux1_field->report != brightness_field->report)
> +		return dev_err_probe(dev, -ENODEV, "Encountered unexpected report structure\n");
> +
> +	bl = devm_kzalloc(dev, sizeof(*bl), GFP_KERNEL);
> +	if (!bl)
> +		return -ENOMEM;
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "HID hardware start failed\n");
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "HID hardware open failed\n");
> +		goto stop_hw;
> +	}
> +
> +	bl->aux1_field = aux1_field;
> +	bl->brightness_field = brightness_field;
> +
> +	if (appletb_bl_def_brightness == 0)
> +		ret = appletb_bl_set_brightness(bl, APPLETB_BL_OFF);
> +	else if (appletb_bl_def_brightness == 1)
> +		ret = appletb_bl_set_brightness(bl, APPLETB_BL_DIM);
> +	else
> +		ret = appletb_bl_set_brightness(bl, APPLETB_BL_ON);
> +
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to set touch bar brightness to off\n");
> +		goto close_hw;
> +	}
> +
> +	bl_props.type = BACKLIGHT_RAW;
> +	bl_props.max_brightness = ARRAY_SIZE(appletb_bl_brightness_map) - 1;
> +
> +	bl->bdev = devm_backlight_device_register(dev, "appletb_backlight", dev, bl,
> +						  &appletb_bl_backlight_ops, &bl_props);
> +	if (IS_ERR(bl->bdev)) {
> +		ret = PTR_ERR(bl->bdev);
> +		dev_err_probe(dev, ret, "Failed to register backlight device\n");
> +		goto close_hw;
> +	}
> +
> +	hid_set_drvdata(hdev, bl);
> +
> +	return 0;
> +
> +close_hw:
> +	hid_hw_close(hdev);
> +stop_hw:
> +	hid_hw_stop(hdev);
> +
> +	return ret;
> +}
> +
> +static void appletb_bl_remove(struct hid_device *hdev)
> +{
> +	struct appletb_bl *bl = hid_get_drvdata(hdev);
> +
> +	appletb_bl_set_brightness(bl, APPLETB_BL_OFF);
> +
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id appletb_bl_hid_ids[] = {
> +	/* MacBook Pro's 2018, 2019, with T2 chip: iBridge DFR Brightness */
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, appletb_bl_hid_ids);
> +
> +static struct hid_driver appletb_bl_hid_driver = {
> +	.name = "hid-appletb-bl",
> +	.id_table = appletb_bl_hid_ids,
> +	.probe = appletb_bl_probe,
> +	.remove = appletb_bl_remove,
> +};
> +module_hid_driver(appletb_bl_hid_driver);
> +
> +MODULE_AUTHOR("Ronald Tschalär");
> +MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
> +MODULE_DESCRIPTION("MacBookPro Touch Bar Backlight Driver");
> +MODULE_LICENSE("GPL");

<snip>

Hi Thomas

> 
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index e40f1ddeb..1d825a474 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_HID_ALPS) += hid-alps.o
>> obj-$(CONFIG_HID_ACRUX) += hid-axff.o
>> obj-$(CONFIG_HID_APPLE) += hid-apple.o
>> obj-$(CONFIG_HID_APPLEIR) += hid-appleir.o
>> +obj-$(CONFIG_HID_APPLETB_BL) += hid-appletb-bl.o
>> obj-$(CONFIG_HID_CREATIVE_SB0540) += hid-creative-sb0540.o
>> obj-$(CONFIG_HID_ASUS) += hid-asus.o
>> obj-$(CONFIG_HID_AUREAL) += hid-aureal.o
>> diff --git a/drivers/hid/hid-appletb-bl.c b/drivers/hid/hid-appletb-bl.c
>> new file mode 100644
>> index 000000000..00bbe45df
>> --- /dev/null
>> +++ b/drivers/hid/hid-appletb-bl.c
>> @@ -0,0 +1,206 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Apple Touch Bar Backlight Driver
>> + *
>> + * Copyright (c) 2017-2018 Ronald Tschalär
>> + * Copyright (c) 2022-2023 Kerem Karabay <kekrby@gmail.com>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> This is unused.
> 
> #include <linux/device.h>

Can you clarify what you mean here?
> 
>> +#include <linux/hid.h>
>> +#include <linux/backlight.h>
>> +
>> +#include "hid-ids.h"
>> +
>> +#define APPLETB_BL_ON 1
>> +#define APPLETB_BL_DIM 3
>> +#define APPLETB_BL_OFF 4
>> +
>> +#define HID_UP_APPLEVENDOR_TB_BL 0xff120000
>> +
>> +#define HID_VD_APPLE_TB_BRIGHTNESS 0xff120001
>> +#define HID_USAGE_AUX1 0xff120020
>> +#define HID_USAGE_BRIGHTNESS 0xff120021
>> +
>> +static int appletb_bl_def_brightness = 2;
>> +module_param_named(brightness, appletb_bl_def_brightness, int, 0444);
>> +MODULE_PARM_DESC(brightness, "Default brightness:\n"
>> +  "    0 - Touchbar is off\n"
>> +  "    1 - Dim brightness\n"
>> +  "    [2] - Full brightness");
>> +
>> +struct appletb_bl {
>> + struct hid_field *aux1_field, *brightness_field;
>> + struct backlight_device *bdev;
>> +
>> + bool full_on;
>> +};
>> +
>> +const u8 appletb_bl_brightness_map[] = {
> 
> static?
> 
>> + APPLETB_BL_OFF,
>> + APPLETB_BL_DIM,
>> + APPLETB_BL_ON
> 
> The last element is not a sentinel element, so it should have comma.

static const u8 appletb_bl_brightness_map[] = {
	APPLETB_BL_OFF,
	APPLETB_BL_DIM,
	APPLETB_BL_ON,
};

This?
> 
>> +};
>> +
>> +static int appletb_bl_set_brightness(struct appletb_bl *bl, u8 brightness)
>> +{
>> + struct hid_report *report = bl->brightness_field->report;
>> + struct hid_device *hdev = report->device;
>> + int ret;
>> +
>> + ret = hid_set_field(bl->aux1_field, 0, 1);
>> + if (ret) {
>> + hid_err(hdev, "Failed to set auxiliary field (%pe)\n", ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + ret = hid_set_field(bl->brightness_field, 0, brightness);
>> + if (ret) {
>> + hid_err(hdev, "Failed to set brightness field (%pe)\n", ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + if (!bl->full_on) {
>> + ret = hid_hw_power(hdev, PM_HINT_FULLON);
>> + if (ret < 0) {
>> + hid_err(hdev, "Device didn't power on (%pe)\n", ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + bl->full_on = true;
>> + }
>> +
>> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>> +
>> + if (brightness == APPLETB_BL_OFF) {
>> + hid_hw_power(hdev, PM_HINT_NORMAL);
>> + bl->full_on = false;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int appletb_bl_update_status(struct backlight_device *bdev)
>> +{
>> + struct appletb_bl *bl = bl_get_data(bdev);
>> + u16 brightness;
>> +
>> + if (bdev->props.state & BL_CORE_SUSPENDED)
>> + brightness = 0;
> 
> From backlight.h:
> 
> * backlight drivers are expected to use backlight_is_blank()
> * in their update_status() operation rather than reading the
> * state property.
> 
> Seems to be applicable here.
> 
> Also the hardcoded "0" as index into appletb_bl_brightness_map could be
> avoided by some restructuring.

static int appletb_bl_update_status(struct backlight_device *bdev)
{
	struct appletb_bl *bl = bl_get_data(bdev);
	u16 brightness;

	if (backlight_is_blank(bdev))
		brightness = APPLETB_BL_OFF;
	else
		brightness = backlight_get_brightness(bdev);

	return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
}

This?
> 
>> + else
>> + brightness = backlight_get_brightness(bdev);
>> +
>> + return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
>> +}
>> +
>> +static const struct backlight_ops appletb_bl_backlight_ops = {
>> + .options = BL_CORE_SUSPENDRESUME,
>> + .update_status = appletb_bl_update_status,
>> +};
>> +
>> +static int appletb_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> +{
>> + struct hid_field *aux1_field, *brightness_field;
>> + struct backlight_properties bl_props = { 0 };
>> + struct device *dev = &hdev->dev;
>> + struct appletb_bl *bl;
>> + int ret;
>> +
>> + ret = hid_parse(hdev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "HID parse failed\n");
>> +
>> + aux1_field = hid_find_field(hdev, HID_FEATURE_REPORT,
>> +     HID_VD_APPLE_TB_BRIGHTNESS, HID_USAGE_AUX1);
>> +
>> + brightness_field = hid_find_field(hdev, HID_FEATURE_REPORT,
>> +   HID_VD_APPLE_TB_BRIGHTNESS, HID_USAGE_BRIGHTNESS);
>> +
>> + if (!aux1_field || !brightness_field)
>> + return -ENODEV;
>> +
>> + if (aux1_field->report != brightness_field->report)
>> + return dev_err_probe(dev, -ENODEV, "Encountered unexpected report structure\n");
>> +
>> + bl = devm_kzalloc(dev, sizeof(*bl), GFP_KERNEL);
>> + if (!bl)
>> + return -ENOMEM;
>> +
>> + ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "HID hardware start failed\n");
>> +
>> + ret = hid_hw_open(hdev);
>> + if (ret) {
>> + dev_err_probe(dev, ret, "HID hardware open failed\n");
>> + goto stop_hw;
>> + }
>> +
>> + bl->aux1_field = aux1_field;
>> + bl->brightness_field = brightness_field;
>> +
>> + if (appletb_bl_def_brightness == 0)
>> + ret = appletb_bl_set_brightness(bl, APPLETB_BL_OFF);
>> + else if (appletb_bl_def_brightness == 1)
>> + ret = appletb_bl_set_brightness(bl, APPLETB_BL_DIM);
>> + else
>> + ret = appletb_bl_set_brightness(bl, APPLETB_BL_ON);
>> +
>> + if (ret) {
>> + dev_err_probe(dev, ret, "Failed to set touch bar brightness to off\n");
>> + goto close_hw;
>> + }
>> +
>> + bl_props.type = BACKLIGHT_RAW;
>> + bl_props.max_brightness = ARRAY_SIZE(appletb_bl_brightness_map) - 1;
>> +
>> + bl->bdev = devm_backlight_device_register(dev, "appletb_backlight", dev, bl,
>> +   &appletb_bl_backlight_ops, &bl_props);
>> + if (IS_ERR(bl->bdev)) {
>> + ret = PTR_ERR(bl->bdev);
>> + dev_err_probe(dev, ret, "Failed to register backlight device\n");
>> + goto close_hw;
>> + }
>> +
>> + hid_set_drvdata(hdev, bl);
>> +
>> + return 0;
>> +
>> +close_hw:
>> + hid_hw_close(hdev);
>> +stop_hw:
>> + hid_hw_stop(hdev);
>> +
>> + return ret;
>> +}
>> +
>> +static void appletb_bl_remove(struct hid_device *hdev)
>> +{
>> + struct appletb_bl *bl = hid_get_drvdata(hdev);
>> +
>> + appletb_bl_set_brightness(bl, APPLETB_BL_OFF);
>> +
>> + hid_hw_close(hdev);
>> + hid_hw_stop(hdev);
>> +}
>> +
>> +static const struct hid_device_id appletb_bl_hid_ids[] = {
>> + /* MacBook Pro's 2018, 2019, with T2 chip: iBridge DFR Brightness */
>> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(hid, appletb_bl_hid_ids);
>> +
>> +static struct hid_driver appletb_bl_hid_driver = {
>> + .name = "hid-appletb-bl",
>> + .id_table = appletb_bl_hid_ids,
>> + .probe = appletb_bl_probe,
>> + .remove = appletb_bl_remove,
>> +};
>> +module_hid_driver(appletb_bl_hid_driver);
>> +
>> +MODULE_AUTHOR("Ronald Tschalär");
>> +MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
>> +MODULE_DESCRIPTION("MacBookPro Touch Bar Backlight Driver");
>> +MODULE_LICENSE("GPL");
> 
> <snip>

Thanks
Aditya

On 2024-08-10 13:23:30+0000, Aditya Garg wrote:
> Hi Thomas
> 
> >> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> >> index e40f1ddeb..1d825a474 100644
> >> --- a/drivers/hid/Makefile
> >> +++ b/drivers/hid/Makefile
> >> @@ -29,6 +29,7 @@ obj-$(CONFIG_HID_ALPS) += hid-alps.o
> >> obj-$(CONFIG_HID_ACRUX) += hid-axff.o
> >> obj-$(CONFIG_HID_APPLE) += hid-apple.o
> >> obj-$(CONFIG_HID_APPLEIR) += hid-appleir.o
> >> +obj-$(CONFIG_HID_APPLETB_BL) += hid-appletb-bl.o
> >> obj-$(CONFIG_HID_CREATIVE_SB0540) += hid-creative-sb0540.o
> >> obj-$(CONFIG_HID_ASUS) += hid-asus.o
> >> obj-$(CONFIG_HID_AUREAL) += hid-aureal.o
> >> diff --git a/drivers/hid/hid-appletb-bl.c b/drivers/hid/hid-appletb-bl.c
> >> new file mode 100644
> >> index 000000000..00bbe45df
> >> --- /dev/null
> >> +++ b/drivers/hid/hid-appletb-bl.c
> >> @@ -0,0 +1,206 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Apple Touch Bar Backlight Driver
> >> + *
> >> + * Copyright (c) 2017-2018 Ronald Tschalär
> >> + * Copyright (c) 2022-2023 Kerem Karabay <kekrby@gmail.com>
> >> + */
> >> +
> >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > 
> > This is unused.
> > 
> > #include <linux/device.h>
> 
> Can you clarify what you mean here?

Also include linux/device.h as you are using functions from there.
Like devm_kcalloc().

> > 
> >> +#include <linux/hid.h>
> >> +#include <linux/backlight.h>
> >> +
> >> +#include "hid-ids.h"
> >> +
> >> +#define APPLETB_BL_ON 1
> >> +#define APPLETB_BL_DIM 3
> >> +#define APPLETB_BL_OFF 4
> >> +
> >> +#define HID_UP_APPLEVENDOR_TB_BL 0xff120000
> >> +
> >> +#define HID_VD_APPLE_TB_BRIGHTNESS 0xff120001
> >> +#define HID_USAGE_AUX1 0xff120020
> >> +#define HID_USAGE_BRIGHTNESS 0xff120021
> >> +
> >> +static int appletb_bl_def_brightness = 2;
> >> +module_param_named(brightness, appletb_bl_def_brightness, int, 0444);
> >> +MODULE_PARM_DESC(brightness, "Default brightness:\n"
> >> +  "    0 - Touchbar is off\n"
> >> +  "    1 - Dim brightness\n"
> >> +  "    [2] - Full brightness");
> >> +
> >> +struct appletb_bl {
> >> + struct hid_field *aux1_field, *brightness_field;
> >> + struct backlight_device *bdev;
> >> +
> >> + bool full_on;
> >> +};
> >> +
> >> +const u8 appletb_bl_brightness_map[] = {
> > 
> > static?
> > 
> >> + APPLETB_BL_OFF,
> >> + APPLETB_BL_DIM,
> >> + APPLETB_BL_ON
> > 
> > The last element is not a sentinel element, so it should have comma.
> 
> static const u8 appletb_bl_brightness_map[] = {
> 	APPLETB_BL_OFF,
> 	APPLETB_BL_DIM,
> 	APPLETB_BL_ON,
> };
> 
> This?

Yes.

> > 
> >> +};
> >> +
> >> +static int appletb_bl_set_brightness(struct appletb_bl *bl, u8 brightness)
> >> +{
> >> + struct hid_report *report = bl->brightness_field->report;
> >> + struct hid_device *hdev = report->device;
> >> + int ret;
> >> +
> >> + ret = hid_set_field(bl->aux1_field, 0, 1);
> >> + if (ret) {
> >> + hid_err(hdev, "Failed to set auxiliary field (%pe)\n", ERR_PTR(ret));
> >> + return ret;
> >> + }
> >> +
> >> + ret = hid_set_field(bl->brightness_field, 0, brightness);
> >> + if (ret) {
> >> + hid_err(hdev, "Failed to set brightness field (%pe)\n", ERR_PTR(ret));
> >> + return ret;
> >> + }
> >> +
> >> + if (!bl->full_on) {
> >> + ret = hid_hw_power(hdev, PM_HINT_FULLON);
> >> + if (ret < 0) {
> >> + hid_err(hdev, "Device didn't power on (%pe)\n", ERR_PTR(ret));
> >> + return ret;
> >> + }
> >> +
> >> + bl->full_on = true;
> >> + }
> >> +
> >> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> >> +
> >> + if (brightness == APPLETB_BL_OFF) {
> >> + hid_hw_power(hdev, PM_HINT_NORMAL);
> >> + bl->full_on = false;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int appletb_bl_update_status(struct backlight_device *bdev)
> >> +{
> >> + struct appletb_bl *bl = bl_get_data(bdev);
> >> + u16 brightness;
> >> +
> >> + if (bdev->props.state & BL_CORE_SUSPENDED)
> >> + brightness = 0;
> > 
> > From backlight.h:
> > 
> > * backlight drivers are expected to use backlight_is_blank()
> > * in their update_status() operation rather than reading the
> > * state property.
> > 
> > Seems to be applicable here.
> > 
> > Also the hardcoded "0" as index into appletb_bl_brightness_map could be
> > avoided by some restructuring.
> 
> static int appletb_bl_update_status(struct backlight_device *bdev)
> {
> 	struct appletb_bl *bl = bl_get_data(bdev);
> 	u16 brightness;
> 
> 	if (backlight_is_blank(bdev))
> 		brightness = APPLETB_BL_OFF;
> 	else
> 		brightness = backlight_get_brightness(bdev);
> 
> 	return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
> }
> 
> This?

This now looks to be a different logic than before.
Below should be the same as in the original patch.

static int appletb_bl_update_status(struct backlight_device *bdev)
{
	struct appletb_bl *bl = bl_get_data(bdev);
	u8 brightness;

	if (backlight_is_blank(bdev))
		brightness = APPLETB_BL_OFF;
	else
		brightness = appletb_bl_brightness_map[backlight_get_brightness(bdev)]);

	return appletb_bl_set_brightness(bl, brightness);
}

Maybe it's worth to make APPLETB_BL_* an enum for more clarity.

> >> + else
> >> + brightness = backlight_get_brightness(bdev);
> >> +
> >> + return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
> >> +}
> >> +

<snip>

> 
> Also include linux/device.h as you are using functions from there.
> Like devm_kcalloc().

Alright, I’ll add that
> 
>>> 
>>>> +#include <linux/hid.h>
>>>> +#include <linux/backlight.h>
>>>> +
>>>> +#include "hid-ids.h"
>>>> +
>>>> +#define APPLETB_BL_ON 1
>>>> +#define APPLETB_BL_DIM 3
>>>> +#define APPLETB_BL_OFF 4
>>>> +
>>>> +#define HID_UP_APPLEVENDOR_TB_BL 0xff120000
>>>> +
>>>> +#define HID_VD_APPLE_TB_BRIGHTNESS 0xff120001
>>>> +#define HID_USAGE_AUX1 0xff120020
>>>> +#define HID_USAGE_BRIGHTNESS 0xff120021
>>>> +
>>>> +static int appletb_bl_def_brightness = 2;
>>>> +module_param_named(brightness, appletb_bl_def_brightness, int, 0444);
>>>> +MODULE_PARM_DESC(brightness, "Default brightness:\n"
>>>> +  "    0 - Touchbar is off\n"
>>>> +  "    1 - Dim brightness\n"
>>>> +  "    [2] - Full brightness");
>>>> +
>>>> +struct appletb_bl {
>>>> + struct hid_field *aux1_field, *brightness_field;
>>>> + struct backlight_device *bdev;
>>>> +
>>>> + bool full_on;
>>>> +};
>>>> +
>>>> +const u8 appletb_bl_brightness_map[] = {
>>> 
>>> static?
>>> 
>>>> + APPLETB_BL_OFF,
>>>> + APPLETB_BL_DIM,
>>>> + APPLETB_BL_ON
>>> 
>>> The last element is not a sentinel element, so it should have comma.
>> 
>> static const u8 appletb_bl_brightness_map[] = {
>> APPLETB_BL_OFF,
>> APPLETB_BL_DIM,
>> APPLETB_BL_ON,
>> };
>> 
>> This?
> 
> Yes.
> 
>>> 
>>>> +};
>>>> +
>>>> +static int appletb_bl_set_brightness(struct appletb_bl *bl, u8 brightness)
>>>> +{
>>>> + struct hid_report *report = bl->brightness_field->report;
>>>> + struct hid_device *hdev = report->device;
>>>> + int ret;
>>>> +
>>>> + ret = hid_set_field(bl->aux1_field, 0, 1);
>>>> + if (ret) {
>>>> + hid_err(hdev, "Failed to set auxiliary field (%pe)\n", ERR_PTR(ret));
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = hid_set_field(bl->brightness_field, 0, brightness);
>>>> + if (ret) {
>>>> + hid_err(hdev, "Failed to set brightness field (%pe)\n", ERR_PTR(ret));
>>>> + return ret;
>>>> + }
>>>> +
>>>> + if (!bl->full_on) {
>>>> + ret = hid_hw_power(hdev, PM_HINT_FULLON);
>>>> + if (ret < 0) {
>>>> + hid_err(hdev, "Device didn't power on (%pe)\n", ERR_PTR(ret));
>>>> + return ret;
>>>> + }
>>>> +
>>>> + bl->full_on = true;
>>>> + }
>>>> +
>>>> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>>>> +
>>>> + if (brightness == APPLETB_BL_OFF) {
>>>> + hid_hw_power(hdev, PM_HINT_NORMAL);
>>>> + bl->full_on = false;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int appletb_bl_update_status(struct backlight_device *bdev)
>>>> +{
>>>> + struct appletb_bl *bl = bl_get_data(bdev);
>>>> + u16 brightness;
>>>> +
>>>> + if (bdev->props.state & BL_CORE_SUSPENDED)
>>>> + brightness = 0;
>>> 
>>> From backlight.h:
>>> 
>>> * backlight drivers are expected to use backlight_is_blank()
>>> * in their update_status() operation rather than reading the
>>> * state property.
>>> 
>>> Seems to be applicable here.
>>> 
>>> Also the hardcoded "0" as index into appletb_bl_brightness_map could be
>>> avoided by some restructuring.
>> 
>> static int appletb_bl_update_status(struct backlight_device *bdev)
>> {
>> struct appletb_bl *bl = bl_get_data(bdev);
>> u16 brightness;
>> 
>> if (backlight_is_blank(bdev))
>> brightness = APPLETB_BL_OFF;
>> else
>> brightness = backlight_get_brightness(bdev);
>> 
>> return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
>> }
>> 
>> This?
> 
> This now looks to be a different logic than before.
> Below should be the same as in the original patch.
> 
> static int appletb_bl_update_status(struct backlight_device *bdev)
> {
> struct appletb_bl *bl = bl_get_data(bdev);
> u8 brightness;
> 
> if (backlight_is_blank(bdev))
> brightness = APPLETB_BL_OFF;
> else
> brightness = appletb_bl_brightness_map[backlight_get_brightness(bdev)]);
> 
> return appletb_bl_set_brightness(bl, brightness);
> }

I’ll replace the original code with this

> 
> Maybe it's worth to make APPLETB_BL_* an enum for more clarity.

I don’t think there is a specific need for an enum here

If you are fine with these changes, I’ll send a v5
> 
>>>> + else
>>>> + brightness = backlight_get_brightness(bdev);
>>>> +
>>>> + return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
>>>> +}
>>>> +
> 
> <snip>

Thanks
Aditya

On 2024-08-10 15:30:58+0000, Aditya Garg wrote:
> 
> > 
> > Also include linux/device.h as you are using functions from there.
> > Like devm_kcalloc().
> 
> Alright, I’ll add that
> > 
> >>> 
> >>>> +#include <linux/hid.h>
> >>>> +#include <linux/backlight.h>
> >>>> +
> >>>> +#include "hid-ids.h"
> >>>> +
> >>>> +#define APPLETB_BL_ON 1
> >>>> +#define APPLETB_BL_DIM 3
> >>>> +#define APPLETB_BL_OFF 4
> >>>> +
> >>>> +#define HID_UP_APPLEVENDOR_TB_BL 0xff120000
> >>>> +
> >>>> +#define HID_VD_APPLE_TB_BRIGHTNESS 0xff120001
> >>>> +#define HID_USAGE_AUX1 0xff120020
> >>>> +#define HID_USAGE_BRIGHTNESS 0xff120021
> >>>> +
> >>>> +static int appletb_bl_def_brightness = 2;
> >>>> +module_param_named(brightness, appletb_bl_def_brightness, int, 0444);
> >>>> +MODULE_PARM_DESC(brightness, "Default brightness:\n"
> >>>> +  "    0 - Touchbar is off\n"
> >>>> +  "    1 - Dim brightness\n"
> >>>> +  "    [2] - Full brightness");
> >>>> +
> >>>> +struct appletb_bl {
> >>>> + struct hid_field *aux1_field, *brightness_field;
> >>>> + struct backlight_device *bdev;
> >>>> +
> >>>> + bool full_on;
> >>>> +};
> >>>> +
> >>>> +const u8 appletb_bl_brightness_map[] = {
> >>> 
> >>> static?
> >>> 
> >>>> + APPLETB_BL_OFF,
> >>>> + APPLETB_BL_DIM,
> >>>> + APPLETB_BL_ON
> >>> 
> >>> The last element is not a sentinel element, so it should have comma.
> >> 
> >> static const u8 appletb_bl_brightness_map[] = {
> >> APPLETB_BL_OFF,
> >> APPLETB_BL_DIM,
> >> APPLETB_BL_ON,
> >> };
> >> 
> >> This?
> > 
> > Yes.
> > 
> >>> 
> >>>> +};
> >>>> +
> >>>> +static int appletb_bl_set_brightness(struct appletb_bl *bl, u8 brightness)
> >>>> +{
> >>>> + struct hid_report *report = bl->brightness_field->report;
> >>>> + struct hid_device *hdev = report->device;
> >>>> + int ret;
> >>>> +
> >>>> + ret = hid_set_field(bl->aux1_field, 0, 1);
> >>>> + if (ret) {
> >>>> + hid_err(hdev, "Failed to set auxiliary field (%pe)\n", ERR_PTR(ret));
> >>>> + return ret;
> >>>> + }
> >>>> +
> >>>> + ret = hid_set_field(bl->brightness_field, 0, brightness);
> >>>> + if (ret) {
> >>>> + hid_err(hdev, "Failed to set brightness field (%pe)\n", ERR_PTR(ret));
> >>>> + return ret;
> >>>> + }
> >>>> +
> >>>> + if (!bl->full_on) {
> >>>> + ret = hid_hw_power(hdev, PM_HINT_FULLON);
> >>>> + if (ret < 0) {
> >>>> + hid_err(hdev, "Device didn't power on (%pe)\n", ERR_PTR(ret));
> >>>> + return ret;
> >>>> + }
> >>>> +
> >>>> + bl->full_on = true;
> >>>> + }
> >>>> +
> >>>> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> >>>> +
> >>>> + if (brightness == APPLETB_BL_OFF) {
> >>>> + hid_hw_power(hdev, PM_HINT_NORMAL);
> >>>> + bl->full_on = false;
> >>>> + }
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +static int appletb_bl_update_status(struct backlight_device *bdev)
> >>>> +{
> >>>> + struct appletb_bl *bl = bl_get_data(bdev);
> >>>> + u16 brightness;
> >>>> +
> >>>> + if (bdev->props.state & BL_CORE_SUSPENDED)
> >>>> + brightness = 0;
> >>> 
> >>> From backlight.h:
> >>> 
> >>> * backlight drivers are expected to use backlight_is_blank()
> >>> * in their update_status() operation rather than reading the
> >>> * state property.
> >>> 
> >>> Seems to be applicable here.
> >>> 
> >>> Also the hardcoded "0" as index into appletb_bl_brightness_map could be
> >>> avoided by some restructuring.
> >> 
> >> static int appletb_bl_update_status(struct backlight_device *bdev)
> >> {
> >> struct appletb_bl *bl = bl_get_data(bdev);
> >> u16 brightness;
> >> 
> >> if (backlight_is_blank(bdev))
> >> brightness = APPLETB_BL_OFF;
> >> else
> >> brightness = backlight_get_brightness(bdev);
> >> 
> >> return appletb_bl_set_brightness(bl, appletb_bl_brightness_map[brightness]);
> >> }
> >> 
> >> This?
> > 
> > This now looks to be a different logic than before.
> > Below should be the same as in the original patch.
> > 
> > static int appletb_bl_update_status(struct backlight_device *bdev)
> > {
> > struct appletb_bl *bl = bl_get_data(bdev);
> > u8 brightness;
> > 
> > if (backlight_is_blank(bdev))
> > brightness = APPLETB_BL_OFF;
> > else
> > brightness = appletb_bl_brightness_map[backlight_get_brightness(bdev)]);
> > 
> > return appletb_bl_set_brightness(bl, brightness);
> > }
> 
> I’ll replace the original code with this
> 
> > 
> > Maybe it's worth to make APPLETB_BL_* an enum for more clarity.
> 
> I don’t think there is a specific need for an enum here
> 
> If you are fine with these changes, I’ll send a v5

Sounds good.

FYI I'll also review the other patches soonish.
So sending a v5 right away is not necessary.

> 
> Sounds good.
> 
> FYI I'll also review the other patches soonish.
> So sending a v5 right away is not necessary.

Alright, fair enough.
You’d want to review the v4 of this patchset then, although the only difference is presence of 1 extra patch in v4.

https://lore.kernel.org/linux-input/20190CD7-46CE-400D-9C58-29798479660E@live.com/T/#t