| Message ID | 7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Touch Bar and Keyboard backlight driver for Intel Macs | expand |
On Fri, Feb 10, 2023 at 03:45:15AM +0000, Aditya Garg wrote: > From: Orlando Chamberlain <orlandoch.dev@gmail.com> > > This driver adds support for the keyboard backlight on Intel T2 Macs > with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1) > > Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com> > Co-developed-by: Kerem Karabay <kekrby@gmail.com> > Signed-off-by: Kerem Karabay <kekrby@gmail.com> > Signed-off-by: Aditya Garg <gargaditya08@live.com> > --- > MAINTAINERS | 6 ++ > drivers/hid/Kconfig | 13 +++ > drivers/hid/Makefile | 1 + > drivers/hid/apple-magic-backlight.c | 143 ++++++++++++++++++++++++++++ > 4 files changed, 163 insertions(+) > create mode 100644 drivers/hid/apple-magic-backlight.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index fb1471cb5..3319f0c3e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9201,6 +9201,12 @@ F: include/linux/pm.h > F: include/linux/suspend.h > F: kernel/power/ > > +HID APPLE MAGIC BACKLIGHT DRIVER > +M: Orlando Chamberlain <orlandoch.dev@gmail.com> > +L: linux-input@vger.kernel.org > +S: Maintained > +F: drivers/hid/apple-magic-backlight.c > + > HID CORE LAYER > M: Jiri Kosina <jikos@kernel.org> > M: Benjamin Tissoires <benjamin.tissoires@redhat.com> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 4ec669267..ad4612ec5 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -156,6 +156,19 @@ config HID_APPLE_TOUCHBAR > To compile this driver as a module, choose M here: the > module will be called apple-touchbar. > > +config HID_APPLE_MAGIC_BACKLIGHT > + tristate "Apple Magic Keyboard Backlight" > + depends on USB_HID > + depends on LEDS_CLASS > + depends on NEW_LEDS > + help > + Say Y here if you want support for the keyboard backlight on Macs with > + the magic keyboard (MacBookPro16,x and MacBookAir9,1). Note that this > + driver is not for external magic keyboards. > + > + To compile this driver as a module, choose M here: the > + module will be called apple-magic-backlight. > + > config HID_APPLEIR > tristate "Apple infrared receiver" > depends on (USB_HID) > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile > index c792e42fe..a961914ec 100644 > --- a/drivers/hid/Makefile > +++ b/drivers/hid/Makefile > @@ -28,6 +28,7 @@ obj-$(CONFIG_HID_ACRUX) += hid-axff.o > obj-$(CONFIG_HID_APPLE) += hid-apple.o > obj-$(CONFIG_HID_APPLE_IBRIDGE) += apple-ibridge.o > obj-$(CONFIG_HID_APPLE_TOUCHBAR) += apple-touchbar.o > +obj-$(CONFIG_HID_APPLE_MAGIC_BACKLIGHT) += apple-magic-backlight.o > obj-$(CONFIG_HID_APPLEIR) += hid-appleir.o > obj-$(CONFIG_HID_CREATIVE_SB0540) += hid-creative-sb0540.o > obj-$(CONFIG_HID_ASUS) += hid-asus.o > diff --git a/drivers/hid/apple-magic-backlight.c b/drivers/hid/apple-magic-backlight.c > new file mode 100644 > index 000000000..9b128f6df > --- /dev/null > +++ b/drivers/hid/apple-magic-backlight.c > @@ -0,0 +1,143 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Apple Magic Keyboard Backlight Driver > + * > + * For Intel Macs with internal Magic Keyboard (MacBookPro16,1-4 and MacBookAir9,1) > + * > + * Copyright (c) 2022 Kerem Karabay <kekrby@gmail.com> > + * Copyright (c) 2023 Orlando Chamberlain <orlandoch.dev@gmail.com> > + */ This patch doesn't seem to depend on the others at all and is much simpler. Maybe split it out from the series so it can get merged on its own and you don't have to carry it around anymore. > + > +#include <linux/hid.h> > +#include <linux/usb.h> > + > +#include "hid-ids.h" > + > +#define USAGE_MAGIC_BL 0xff00000f > + > +#define APPLE_MAGIC_REPORT_ID_POWER 3 > +#define APPLE_MAGIC_REPORT_ID_BRIGHTNESS 1 > + > +struct apple_magic_backlight { > + struct led_classdev cdev; > + struct hid_device *hdev; > + struct hid_report *brightness; > + struct hid_report *power; > +}; > + > +static void apple_magic_backlight_power_set(struct apple_magic_backlight *backlight, > + char power, char rate) > +{ > + struct hid_report *rep = backlight->power; > + > + rep->field[0]->value[0] = power ? 1 : 0; > + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > + rep->field[1]->value[0] |= rate << 8; > + > + hid_hw_request(backlight->hdev, backlight->power, HID_REQ_SET_REPORT); > +} > + > +static void apple_magic_backlight_brightness_set(struct apple_magic_backlight *backlight, > + int brightness, char rate) > +{ > + struct hid_report *rep = backlight->brightness; > + > + rep->field[0]->value[0] = brightness; > + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > + rep->field[1]->value[0] |= rate << 8; > + > + hid_hw_request(backlight->hdev, backlight->brightness, HID_REQ_SET_REPORT); > + The two functions above are nearly identical. > + > +static void apple_magic_backlight_set(struct apple_magic_backlight *backlight, > + int brightness, char rate) > +{ > + apple_magic_backlight_power_set(backlight, brightness, rate); > + if (brightness) > + apple_magic_backlight_brightness_set(backlight, brightness, rate); > +} > + > +static int apple_magic_backlight_led_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + struct apple_magic_backlight *backlight = container_of(led_cdev, > + struct apple_magic_backlight, cdev); > + > + apple_magic_backlight_set(backlight, brightness, 1); > + return 0; > +} > + > +static int apple_magic_backlight_probe(struct hid_device *hdev, > + const struct hid_device_id *id) > +{ > + struct apple_magic_backlight *backlight; > + int rc; > + > + rc = hid_parse(hdev); > + if (rc) > + return rc; > + > + /* Ensure this usb endpoint is for the keyboard backlight, not touchbar > + * backlight. > + */ > + if (!(hdev->collection && hdev->collection[0].usage == USAGE_MAGIC_BL)) > + return -ENODEV; > + > + backlight = devm_kzalloc(&hdev->dev, sizeof(*backlight), GFP_KERNEL); > + > + if (!backlight) > + return -ENOMEM; > + > + hid_set_drvdata(hdev, backlight); > + > + rc = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > + if (rc) > + return rc; > + > + backlight->brightness = hid_register_report(hdev, HID_FEATURE_REPORT, > + APPLE_MAGIC_REPORT_ID_BRIGHTNESS, 0); > + backlight->power = hid_register_report(hdev, HID_FEATURE_REPORT, > + APPLE_MAGIC_REPORT_ID_POWER, 0); > + > + if (!backlight->brightness || !backlight->power) { > + rc = -ENODEV; > + goto hw_stop; > + } > + > + backlight->hdev = hdev; > + backlight->cdev.name = "apple::kbd_backlight"; > + backlight->cdev.max_brightness = backlight->brightness->field[0]->logical_maximum; > + backlight->cdev.brightness_set_blocking = apple_magic_backlight_led_set; > + > + apple_magic_backlight_set(backlight, 0, 0); > + > + return devm_led_classdev_register(&hdev->dev, &backlight->cdev); > + > +hw_stop: > + hid_hw_stop(hdev); > + return rc; > +} > + > +static void apple_magic_backlight_remove(struct hid_device *hdev) > +{ > + hid_hw_stop(hdev); > +} > + > +static const struct hid_device_id apple_magic_backlight_hid_ids[] = { > + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) }, > + { } > +}; > +MODULE_DEVICE_TABLE(hid, apple_magic_backlight_hid_ids); > + > +static struct hid_driver apple_magic_backlight_hid_driver = { > + .name = "apple-magic-backlight", > + .id_table = apple_magic_backlight_hid_ids, > + .probe = apple_magic_backlight_probe, > + .remove = apple_magic_backlight_remove, Drop the .remove, it does the same as the default. > +}; > + > +module_hid_driver(apple_magic_backlight_hid_driver); > + > +MODULE_DESCRIPTION("MacBook Magic Keyboard Backlight"); > +MODULE_AUTHOR("Orlando Chamberlain <orlandoch.dev@gmail.com>"); > +MODULE_LICENSE("GPL"); > -- > 2.37.2 >
On Fri, 10 Feb 2023 16:25:18 +0000 Thomas Weißschuh <thomas@t-8ch.de> wrote: > On Fri, Feb 10, 2023 at 03:45:15AM +0000, Aditya Garg wrote: > > From: Orlando Chamberlain <orlandoch.dev@gmail.com> > > > > This driver adds support for the keyboard backlight on Intel T2 Macs > > with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1) > > > > Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com> > > Co-developed-by: Kerem Karabay <kekrby@gmail.com> > > Signed-off-by: Kerem Karabay <kekrby@gmail.com> > > Signed-off-by: Aditya Garg <gargaditya08@live.com> > > --- > > MAINTAINERS | 6 ++ > > drivers/hid/Kconfig | 13 +++ > > drivers/hid/Makefile | 1 + > > drivers/hid/apple-magic-backlight.c | 143 > > ++++++++++++++++++++++++++++ 4 files changed, 163 insertions(+) > > create mode 100644 drivers/hid/apple-magic-backlight.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index fb1471cb5..3319f0c3e 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -9201,6 +9201,12 @@ F: include/linux/pm.h > > F: include/linux/suspend.h > > F: kernel/power/ > > > > +HID APPLE MAGIC BACKLIGHT DRIVER > > +M: Orlando Chamberlain <orlandoch.dev@gmail.com> > > +L: linux-input@vger.kernel.org > > +S: Maintained > > +F: drivers/hid/apple-magic-backlight.c > > + > > HID CORE LAYER > > M: Jiri Kosina <jikos@kernel.org> > > M: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > > index 4ec669267..ad4612ec5 100644 > > --- a/drivers/hid/Kconfig > > +++ b/drivers/hid/Kconfig > > @@ -156,6 +156,19 @@ config HID_APPLE_TOUCHBAR > > To compile this driver as a module, choose M here: the > > module will be called apple-touchbar. > > > > +config HID_APPLE_MAGIC_BACKLIGHT > > + tristate "Apple Magic Keyboard Backlight" > > + depends on USB_HID > > + depends on LEDS_CLASS > > + depends on NEW_LEDS > > + help > > + Say Y here if you want support for the keyboard backlight > > on Macs with > > + the magic keyboard (MacBookPro16,x and MacBookAir9,1). > > Note that this > > + driver is not for external magic keyboards. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called apple-magic-backlight. > > + > > config HID_APPLEIR > > tristate "Apple infrared receiver" > > depends on (USB_HID) > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile > > index c792e42fe..a961914ec 100644 > > --- a/drivers/hid/Makefile > > +++ b/drivers/hid/Makefile > > @@ -28,6 +28,7 @@ obj-$(CONFIG_HID_ACRUX) += > > hid-axff.o obj-$(CONFIG_HID_APPLE) += hid-apple.o > > obj-$(CONFIG_HID_APPLE_IBRIDGE) += apple-ibridge.o > > obj-$(CONFIG_HID_APPLE_TOUCHBAR) += apple-touchbar.o > > +obj-$(CONFIG_HID_APPLE_MAGIC_BACKLIGHT) += > > apple-magic-backlight.o obj-$(CONFIG_HID_APPLEIR) += > > hid-appleir.o obj-$(CONFIG_HID_CREATIVE_SB0540) += > > hid-creative-sb0540.o obj-$(CONFIG_HID_ASUS) += > > hid-asus.o diff --git a/drivers/hid/apple-magic-backlight.c > > b/drivers/hid/apple-magic-backlight.c new file mode 100644 > > index 000000000..9b128f6df > > --- /dev/null > > +++ b/drivers/hid/apple-magic-backlight.c > > @@ -0,0 +1,143 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Apple Magic Keyboard Backlight Driver > > + * > > + * For Intel Macs with internal Magic Keyboard (MacBookPro16,1-4 > > and MacBookAir9,1) > > + * > > + * Copyright (c) 2022 Kerem Karabay <kekrby@gmail.com> > > + * Copyright (c) 2023 Orlando Chamberlain <orlandoch.dev@gmail.com> > > + */ > > This patch doesn't seem to depend on the others at all and is much > simpler. > Maybe split it out from the series so it can get merged on its own and > you don't have to carry it around anymore. Good point, I might send v2 separately from the touchbar and ibridge patches. > > > + > > +#include <linux/hid.h> > > +#include <linux/usb.h> > > + > > +#include "hid-ids.h" > > + > > +#define USAGE_MAGIC_BL 0xff00000f > > + > > +#define APPLE_MAGIC_REPORT_ID_POWER 3 > > +#define APPLE_MAGIC_REPORT_ID_BRIGHTNESS 1 > > + > > +struct apple_magic_backlight { > > + struct led_classdev cdev; > > + struct hid_device *hdev; > > + struct hid_report *brightness; > > + struct hid_report *power; > > +}; > > + > > +static void apple_magic_backlight_power_set(struct > > apple_magic_backlight *backlight, > > + char power, char rate) > > +{ > > + struct hid_report *rep = backlight->power; > > + > > + rep->field[0]->value[0] = power ? 1 : 0; > > + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > > + rep->field[1]->value[0] |= rate << 8; > > + > > + hid_hw_request(backlight->hdev, backlight->power, > > HID_REQ_SET_REPORT); +} > > + > > +static void apple_magic_backlight_brightness_set(struct > > apple_magic_backlight *backlight, > > + int brightness, > > char rate) +{ > > + struct hid_report *rep = backlight->brightness; > > + > > + rep->field[0]->value[0] = brightness; > > + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > > + rep->field[1]->value[0] |= rate << 8; > > + > > + hid_hw_request(backlight->hdev, backlight->brightness, > > HID_REQ_SET_REPORT); > > + > > The two functions above are nearly identical. They are indeed quite similar, and I can turn the backlight off with the brightness one, but when I logged the usb packets Windows used, it used both so I've done the same in the Linux driver to (hopefully) ensure it works with any other models or firmware updates that the Windows driver works on. > > > + > > +static void apple_magic_backlight_set(struct apple_magic_backlight > > *backlight, > > + int brightness, char rate) > > +{ > > + apple_magic_backlight_power_set(backlight, brightness, > > rate); > > + if (brightness) > > + apple_magic_backlight_brightness_set(backlight, > > brightness, rate); +} > > + > > +static int apple_magic_backlight_led_set(struct led_classdev > > *led_cdev, > > + enum led_brightness > > brightness) +{ > > + struct apple_magic_backlight *backlight = > > container_of(led_cdev, > > + struct apple_magic_backlight, cdev); > > + > > + apple_magic_backlight_set(backlight, brightness, 1); > > + return 0; > > +} > > + > > +static int apple_magic_backlight_probe(struct hid_device *hdev, > > + const struct hid_device_id > > *id) +{ > > + struct apple_magic_backlight *backlight; > > + int rc; > > + > > + rc = hid_parse(hdev); > > + if (rc) > > + return rc; > > + > > + /* Ensure this usb endpoint is for the keyboard backlight, > > not touchbar > > + * backlight. > > + */ > > + if (!(hdev->collection && hdev->collection[0].usage == > > USAGE_MAGIC_BL)) > > + return -ENODEV; > > + > > + backlight = devm_kzalloc(&hdev->dev, sizeof(*backlight), > > GFP_KERNEL); + > > + if (!backlight) > > + return -ENOMEM; > > + > > + hid_set_drvdata(hdev, backlight); > > + > > + rc = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > > + if (rc) > > + return rc; > > + > > + backlight->brightness = hid_register_report(hdev, > > HID_FEATURE_REPORT, > > + APPLE_MAGIC_REPORT_ID_BRIGHTNESS, 0); > > + backlight->power = hid_register_report(hdev, > > HID_FEATURE_REPORT, > > + APPLE_MAGIC_REPORT_ID_POWER, 0); > > + > > + if (!backlight->brightness || !backlight->power) { > > + rc = -ENODEV; > > + goto hw_stop; > > + } > > + > > + backlight->hdev = hdev; > > + backlight->cdev.name = "apple::kbd_backlight"; > > + backlight->cdev.max_brightness = > > backlight->brightness->field[0]->logical_maximum; > > + backlight->cdev.brightness_set_blocking = > > apple_magic_backlight_led_set; + > > + apple_magic_backlight_set(backlight, 0, 0); > > + > > + return devm_led_classdev_register(&hdev->dev, > > &backlight->cdev); + > > +hw_stop: > > + hid_hw_stop(hdev); > > + return rc; > > +} > > + > > +static void apple_magic_backlight_remove(struct hid_device *hdev) > > +{ > > + hid_hw_stop(hdev); > > +} > > + > > +static const struct hid_device_id apple_magic_backlight_hid_ids[] > > = { > > + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, > > USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(hid, apple_magic_backlight_hid_ids); > > + > > +static struct hid_driver apple_magic_backlight_hid_driver = { > > + .name = "apple-magic-backlight", > > + .id_table = apple_magic_backlight_hid_ids, > > + .probe = apple_magic_backlight_probe, > > + .remove = apple_magic_backlight_remove, > > Drop the .remove, it does the same as the default. > I didn't realise that! I will make that change in a v2. > > +}; > > + > > +module_hid_driver(apple_magic_backlight_hid_driver); > > + > > +MODULE_DESCRIPTION("MacBook Magic Keyboard Backlight"); > > +MODULE_AUTHOR("Orlando Chamberlain <orlandoch.dev@gmail.com>"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.37.2 > >
On Sat, Feb 11, 2023 at 10:24:25AM +1100, Orlando Chamberlain wrote: > On Fri, 10 Feb 2023 16:25:18 +0000 > Thomas Weißschuh <thomas@t-8ch.de> wrote: > > > On Fri, Feb 10, 2023 at 03:45:15AM +0000, Aditya Garg wrote: > > > From: Orlando Chamberlain <orlandoch.dev@gmail.com> > > > +static void apple_magic_backlight_power_set(struct > > > apple_magic_backlight *backlight, > > > + char power, char rate) > > > +{ > > > + struct hid_report *rep = backlight->power; > > > + > > > + rep->field[0]->value[0] = power ? 1 : 0; > > > + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > > > + rep->field[1]->value[0] |= rate << 8; > > > + > > > + hid_hw_request(backlight->hdev, backlight->power, > > > HID_REQ_SET_REPORT); +} > > > + > > > +static void apple_magic_backlight_brightness_set(struct > > > apple_magic_backlight *backlight, > > > + int brightness, > > > char rate) +{ > > > + struct hid_report *rep = backlight->brightness; > > > + > > > + rep->field[0]->value[0] = brightness; > > > + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > > > + rep->field[1]->value[0] |= rate << 8; > > > + > > > + hid_hw_request(backlight->hdev, backlight->brightness, > > > HID_REQ_SET_REPORT); > > > + > > > > The two functions above are nearly identical. > > They are indeed quite similar, and I can turn the backlight off with the > brightness one, but when I logged the usb packets Windows used, it used > both so I've done the same in the Linux driver to (hopefully) ensure it > works with any other models or firmware updates that the Windows driver > works on. I didn't mean to suggest changing the logic, just the way the code is organized: static void apple_magic_backlight_report_set(struct apple_magic_backlight *backlight, struct hid_report *rep, char value, char rate) { rep->field[0]->value[0] = value; rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ rep->field[1]->value[0] |= rate << 8; hid_hw_request(backlight->hdev, rep, HID_REQ_SET_REPORT); } static void apple_magic_backlight_set(struct apple_magic_backlight *backlight, int brightness, char rate) { apple_magic_backlight_report_set(backlight, backlight->power, !!brightness, rate); if (brightness) apple_magic_backlight_report_set(backlight, backlight->brightness, brightness, rate); } This way you can get rid of the duplicated code. > > > > > + > > > +static void apple_magic_backlight_set(struct apple_magic_backlight > > > *backlight, > > > + int brightness, char rate) > > > +{ > > > + apple_magic_backlight_power_set(backlight, brightness, > > > rate); > > > + if (brightness) > > > + apple_magic_backlight_brightness_set(backlight, > > > brightness, rate); +} > > > +
On Sat, Feb 11, 2023 at 02:23:42AM +0000, Thomas Weißschuh wrote: > On Sat, Feb 11, 2023 at 10:24:25AM +1100, Orlando Chamberlain wrote: > > On Fri, 10 Feb 2023 16:25:18 +0000 > > Thomas Weißschuh <thomas@t-8ch.de> wrote: > > > > > On Fri, Feb 10, 2023 at 03:45:15AM +0000, Aditya Garg wrote: > > > > From: Orlando Chamberlain <orlandoch.dev@gmail.com> > > > > +static void apple_magic_backlight_power_set(struct > > > > apple_magic_backlight *backlight, > > > > + char power, char rate) > > > > +{ > > > > + struct hid_report *rep = backlight->power; > > > > + > > > > + rep->field[0]->value[0] = power ? 1 : 0; > > > > + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > > > > + rep->field[1]->value[0] |= rate << 8; > > > > + > > > > + hid_hw_request(backlight->hdev, backlight->power, > > > > HID_REQ_SET_REPORT); +} > > > > + > > > > +static void apple_magic_backlight_brightness_set(struct > > > > apple_magic_backlight *backlight, > > > > + int brightness, > > > > char rate) +{ > > > > + struct hid_report *rep = backlight->brightness; > > > > + > > > > + rep->field[0]->value[0] = brightness; > > > > + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > > > > + rep->field[1]->value[0] |= rate << 8; > > > > + > > > > + hid_hw_request(backlight->hdev, backlight->brightness, > > > > HID_REQ_SET_REPORT); > > > > + > > > > > > The two functions above are nearly identical. > > > > They are indeed quite similar, and I can turn the backlight off with the > > brightness one, but when I logged the usb packets Windows used, it used > > both so I've done the same in the Linux driver to (hopefully) ensure it > > works with any other models or firmware updates that the Windows driver > > works on. > > I didn't mean to suggest changing the logic, just the way the code is > organized: > > static void apple_magic_backlight_report_set(struct apple_magic_backlight *backlight, > struct hid_report *rep, char value, char rate) > { > rep->field[0]->value[0] = value; > rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ > rep->field[1]->value[0] |= rate << 8; > > hid_hw_request(backlight->hdev, rep, HID_REQ_SET_REPORT); > } > > static void apple_magic_backlight_set(struct apple_magic_backlight *backlight, > int brightness, char rate) > { > apple_magic_backlight_report_set(backlight, backlight->power, !!brightness, rate); > if (brightness) > apple_magic_backlight_report_set(backlight, backlight->brightness, brightness, rate); > } > > This way you can get rid of the duplicated code. Or even better, get rid of the struct apple_magic_backlight parameter altogether: static void apple_magic_backlight_report_set(struct hid_report *rep, char value, char rate) { rep->field[0]->value[0] = value; rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ rep->field[1]->value[0] |= rate << 8; hid_hw_request(rep->device, rep, HID_REQ_SET_REPORT); } > > > > > > > > + > > > > +static void apple_magic_backlight_set(struct apple_magic_backlight > > > > *backlight, > > > > + int brightness, char rate) > > > > +{ > > > > + apple_magic_backlight_power_set(backlight, brightness, > > > > rate); > > > > + if (brightness) > > > > + apple_magic_backlight_brightness_set(backlight, > > > > brightness, rate); +} > > > > +
Hi! > From: Orlando Chamberlain <orlandoch.dev@gmail.com> > > This driver adds support for the keyboard backlight on Intel T2 Macs > with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1) > + backlight->hdev = hdev; > + backlight->cdev.name = "apple::kbd_backlight"; ":white:kbd_backlight", plus document this in Documentation/leds/well-known-leds.txt so that we keep it consistent accross machines? Thanks, Pavel
On Sat, 11 Feb 2023 17:56:15 +0100 Pavel Machek <pavel@ucw.cz> wrote: > Hi! > > > From: Orlando Chamberlain <orlandoch.dev@gmail.com> > > > > This driver adds support for the keyboard backlight on Intel T2 Macs > > with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1) > > > + backlight->hdev = hdev; > > + backlight->cdev.name = "apple::kbd_backlight"; > > ":white:kbd_backlight", plus document this in > Documentation/leds/well-known-leds.txt so that we keep it consistent > accross machines? As in "apple:white:kbd_backlight" or just ":white:kbd_backlight"? It seems like most examples I can see by grepping around use "apple::kbd_backlight", "asus::kbd_backlight" etc, without any colour information or anything between the two colons. Would we want to start having the led names in new drivers (including this one) as "$vendor:$colour:$location_or_description"? I do notice you've also suggested doing that here https://lore.kernel.org/all/Y+I7xNqkq%2FX6Lag+@duo.ucw.cz/, and that conversation suggests adding the colour won't stop userspace tools from finding the backlight, which is good. One alternative would be to have the colour of the led stored in a sysfs file in the led's sysfs folder, not sure if that'd be better (it does mean the names stay the same), but also I'm not sure what the empty space between the two colons in the current led names is there for, if not colour. > > Thanks, > Pavel
> On 11-Feb-2023, at 10:26 PM, Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > >> From: Orlando Chamberlain <orlandoch.dev@gmail.com> >> >> This driver adds support for the keyboard backlight on Intel T2 Macs >> with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1) > >> + backlight->hdev = hdev; >> + backlight->cdev.name = "apple::kbd_backlight"; > > ":white:kbd_backlight", plus document this in > Documentation/leds/well-known-leds.txt so that we keep it consistent > accross machines? We had used the same name for butterfly keyboards, the code being in hid-apple Will that also be needed to be changed? > > Thanks, > Pavel > -- > People of Russia, stop Putin before his war on Ukraine escalates.
On Fri, Feb 10, 2023 at 03:45:15AM +0000, Aditya Garg wrote: > From: Orlando Chamberlain <orlandoch.dev@gmail.com> > > This driver adds support for the keyboard backlight on Intel T2 Macs > with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1) ... > +#include <linux/hid.h> > +#include <linux/usb.h> Seems lack of some header inclusions, e.g. where struct led_classdev is defined or -ERRNO codes. > +#include "hid-ids.h" ... > +static void apple_magic_backlight_power_set(struct apple_magic_backlight *backlight, > + char power, char rate) char is a beast, can we use u8 here and in similar cases? ... > + /* Ensure this usb endpoint is for the keyboard backlight, not touchbar > + * backlight. > + */ /* * Multi-line comment style * goes like this. */ ... > + backlight = devm_kzalloc(&hdev->dev, sizeof(*backlight), GFP_KERNEL); > + Redundant blank line. > + if (!backlight) > + return -ENOMEM; ... > +static struct hid_driver apple_magic_backlight_hid_driver = { > + .name = "apple-magic-backlight", > + .id_table = apple_magic_backlight_hid_ids, > + .probe = apple_magic_backlight_probe, > + .remove = apple_magic_backlight_remove, > +}; > + Redundant blank line. > +module_hid_driver(apple_magic_backlight_hid_driver);
On Sun, 12 Feb 2023 13:18:33 +0200 Andy Shevchenko <andy@infradead.org> wrote: > On Fri, Feb 10, 2023 at 03:45:15AM +0000, Aditya Garg wrote: > > From: Orlando Chamberlain <orlandoch.dev@gmail.com> > > > > This driver adds support for the keyboard backlight on Intel T2 Macs > > with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1) > > ... > > > +#include <linux/hid.h> > > +#include <linux/usb.h> > > Seems lack of some header inclusions, e.g. where struct led_classdev > is defined or -ERRNO codes. > > > +#include "hid-ids.h" > > ... > > > +static void apple_magic_backlight_power_set(struct > > apple_magic_backlight *backlight, > > + char power, char rate) > > char is a beast, can we use u8 here and in similar cases? > > ... > > > + /* Ensure this usb endpoint is for the keyboard backlight, > > not touchbar > > + * backlight. > > + */ > > /* > * Multi-line comment style > * goes like this. > */ > > ... > > > + backlight = devm_kzalloc(&hdev->dev, sizeof(*backlight), > > GFP_KERNEL); > > > + > > Redundant blank line. > > > + if (!backlight) > > + return -ENOMEM; > > ... > > > +static struct hid_driver apple_magic_backlight_hid_driver = { > > + .name = "apple-magic-backlight", > > + .id_table = apple_magic_backlight_hid_ids, > > + .probe = apple_magic_backlight_probe, > > + .remove = apple_magic_backlight_remove, > > +}; > > > + > > Redundant blank line. > > > +module_hid_driver(apple_magic_backlight_hid_driver); > Thanks for pointing out all these, I'll make those changes in v2.
diff --git a/MAINTAINERS b/MAINTAINERS index fb1471cb5..3319f0c3e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9201,6 +9201,12 @@ F: include/linux/pm.h F: include/linux/suspend.h F: kernel/power/ +HID APPLE MAGIC BACKLIGHT DRIVER +M: Orlando Chamberlain <orlandoch.dev@gmail.com> +L: linux-input@vger.kernel.org +S: Maintained +F: drivers/hid/apple-magic-backlight.c + HID CORE LAYER M: Jiri Kosina <jikos@kernel.org> M: Benjamin Tissoires <benjamin.tissoires@redhat.com> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 4ec669267..ad4612ec5 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -156,6 +156,19 @@ config HID_APPLE_TOUCHBAR To compile this driver as a module, choose M here: the module will be called apple-touchbar. +config HID_APPLE_MAGIC_BACKLIGHT + tristate "Apple Magic Keyboard Backlight" + depends on USB_HID + depends on LEDS_CLASS + depends on NEW_LEDS + help + Say Y here if you want support for the keyboard backlight on Macs with + the magic keyboard (MacBookPro16,x and MacBookAir9,1). Note that this + driver is not for external magic keyboards. + + To compile this driver as a module, choose M here: the + module will be called apple-magic-backlight. + config HID_APPLEIR tristate "Apple infrared receiver" depends on (USB_HID) diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index c792e42fe..a961914ec 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -28,6 +28,7 @@ obj-$(CONFIG_HID_ACRUX) += hid-axff.o obj-$(CONFIG_HID_APPLE) += hid-apple.o obj-$(CONFIG_HID_APPLE_IBRIDGE) += apple-ibridge.o obj-$(CONFIG_HID_APPLE_TOUCHBAR) += apple-touchbar.o +obj-$(CONFIG_HID_APPLE_MAGIC_BACKLIGHT) += apple-magic-backlight.o obj-$(CONFIG_HID_APPLEIR) += hid-appleir.o obj-$(CONFIG_HID_CREATIVE_SB0540) += hid-creative-sb0540.o obj-$(CONFIG_HID_ASUS) += hid-asus.o diff --git a/drivers/hid/apple-magic-backlight.c b/drivers/hid/apple-magic-backlight.c new file mode 100644 index 000000000..9b128f6df --- /dev/null +++ b/drivers/hid/apple-magic-backlight.c @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Apple Magic Keyboard Backlight Driver + * + * For Intel Macs with internal Magic Keyboard (MacBookPro16,1-4 and MacBookAir9,1) + * + * Copyright (c) 2022 Kerem Karabay <kekrby@gmail.com> + * Copyright (c) 2023 Orlando Chamberlain <orlandoch.dev@gmail.com> + */ + +#include <linux/hid.h> +#include <linux/usb.h> + +#include "hid-ids.h" + +#define USAGE_MAGIC_BL 0xff00000f + +#define APPLE_MAGIC_REPORT_ID_POWER 3 +#define APPLE_MAGIC_REPORT_ID_BRIGHTNESS 1 + +struct apple_magic_backlight { + struct led_classdev cdev; + struct hid_device *hdev; + struct hid_report *brightness; + struct hid_report *power; +}; + +static void apple_magic_backlight_power_set(struct apple_magic_backlight *backlight, + char power, char rate) +{ + struct hid_report *rep = backlight->power; + + rep->field[0]->value[0] = power ? 1 : 0; + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ + rep->field[1]->value[0] |= rate << 8; + + hid_hw_request(backlight->hdev, backlight->power, HID_REQ_SET_REPORT); +} + +static void apple_magic_backlight_brightness_set(struct apple_magic_backlight *backlight, + int brightness, char rate) +{ + struct hid_report *rep = backlight->brightness; + + rep->field[0]->value[0] = brightness; + rep->field[1]->value[0] = 0x5e; /* Mimic Windows */ + rep->field[1]->value[0] |= rate << 8; + + hid_hw_request(backlight->hdev, backlight->brightness, HID_REQ_SET_REPORT); +} + +static void apple_magic_backlight_set(struct apple_magic_backlight *backlight, + int brightness, char rate) +{ + apple_magic_backlight_power_set(backlight, brightness, rate); + if (brightness) + apple_magic_backlight_brightness_set(backlight, brightness, rate); +} + +static int apple_magic_backlight_led_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct apple_magic_backlight *backlight = container_of(led_cdev, + struct apple_magic_backlight, cdev); + + apple_magic_backlight_set(backlight, brightness, 1); + return 0; +} + +static int apple_magic_backlight_probe(struct hid_device *hdev, + const struct hid_device_id *id) +{ + struct apple_magic_backlight *backlight; + int rc; + + rc = hid_parse(hdev); + if (rc) + return rc; + + /* Ensure this usb endpoint is for the keyboard backlight, not touchbar + * backlight. + */ + if (!(hdev->collection && hdev->collection[0].usage == USAGE_MAGIC_BL)) + return -ENODEV; + + backlight = devm_kzalloc(&hdev->dev, sizeof(*backlight), GFP_KERNEL); + + if (!backlight) + return -ENOMEM; + + hid_set_drvdata(hdev, backlight); + + rc = hid_hw_start(hdev, HID_CONNECT_DEFAULT); + if (rc) + return rc; + + backlight->brightness = hid_register_report(hdev, HID_FEATURE_REPORT, + APPLE_MAGIC_REPORT_ID_BRIGHTNESS, 0); + backlight->power = hid_register_report(hdev, HID_FEATURE_REPORT, + APPLE_MAGIC_REPORT_ID_POWER, 0); + + if (!backlight->brightness || !backlight->power) { + rc = -ENODEV; + goto hw_stop; + } + + backlight->hdev = hdev; + backlight->cdev.name = "apple::kbd_backlight"; + backlight->cdev.max_brightness = backlight->brightness->field[0]->logical_maximum; + backlight->cdev.brightness_set_blocking = apple_magic_backlight_led_set; + + apple_magic_backlight_set(backlight, 0, 0); + + return devm_led_classdev_register(&hdev->dev, &backlight->cdev); + +hw_stop: + hid_hw_stop(hdev); + return rc; +} + +static void apple_magic_backlight_remove(struct hid_device *hdev) +{ + hid_hw_stop(hdev); +} + +static const struct hid_device_id apple_magic_backlight_hid_ids[] = { + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) }, + { } +}; +MODULE_DEVICE_TABLE(hid, apple_magic_backlight_hid_ids); + +static struct hid_driver apple_magic_backlight_hid_driver = { + .name = "apple-magic-backlight", + .id_table = apple_magic_backlight_hid_ids, + .probe = apple_magic_backlight_probe, + .remove = apple_magic_backlight_remove, +}; + +module_hid_driver(apple_magic_backlight_hid_driver); + +MODULE_DESCRIPTION("MacBook Magic Keyboard Backlight"); +MODULE_AUTHOR("Orlando Chamberlain <orlandoch.dev@gmail.com>"); +MODULE_LICENSE("GPL");