| Message ID | 0DDD5C22-A42A-49F2-984C-F3595F71AB1C@live.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Touch Bar support for T2 Macs | expand |
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;
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
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) },