* [PATCHv2 0/2] Apple Magic Keyboard Backlight
@ 2023-02-16 4:12 Orlando Chamberlain
2023-02-16 4:12 ` [PATCH 1/2] Documentation: leds: standardise keyboard backlight led names Orlando Chamberlain
2023-02-16 4:12 ` [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards Orlando Chamberlain
0 siblings, 2 replies; 8+ messages in thread
From: Orlando Chamberlain @ 2023-02-16 4:12 UTC (permalink / raw)
To: linux-input
Cc: Jonathan Corbet, Jiri Kosina, Benjamin Tissoires, linux-doc,
linux-kernel, Pavel Machek, Aditya Garg, Aun-Ali Zaidi,
Kerem Karabay, Andy Shevchenko, Thomas Weißschuh,
Orlando Chamberlain
This patchseries adds support for the internal keyboard backlight of
Macs with Apple's "Magic" keyboard (MacBookPro16,* and MacBookAir9,1),
and also documents what names should be used for keyboard backlight
leds in Documentation/leds/well-known-leds.txt.
A previous version of this patch was sent alongside some other drivers
here[1] but for v2 I'm submitting it separately to make upstreaming
easier.
v1->v2:
- drop unneeded remove function
- combine set functions
- add missing header inclusions
- avoid char as argument in favour of u8
- handful of style/formatting fixes
- use standard led name ":white:kbd_backlight"
- rename USAGE_MAGIC_BL to HID_USAGE_MAGIC_BL
- New patch documenting preferred keyboard backlight names
[1]: https://lore.kernel.org/linux-input/7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com/
Orlando Chamberlain (2):
Documentation: leds: standardise keyboard backlight led names
HID: apple-magic-backlight: Add driver for keyboard backlight on
internal Magic Keyboards
Documentation/leds/well-known-leds.txt | 8 ++
MAINTAINERS | 6 ++
drivers/hid/Kconfig | 13 +++
drivers/hid/Makefile | 1 +
drivers/hid/apple-magic-backlight.c | 125 +++++++++++++++++++++++++
5 files changed, 153 insertions(+)
create mode 100644 drivers/hid/apple-magic-backlight.c
--
2.39.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] Documentation: leds: standardise keyboard backlight led names 2023-02-16 4:12 [PATCHv2 0/2] Apple Magic Keyboard Backlight Orlando Chamberlain @ 2023-02-16 4:12 ` Orlando Chamberlain 2023-02-16 4:12 ` [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards Orlando Chamberlain 1 sibling, 0 replies; 8+ messages in thread From: Orlando Chamberlain @ 2023-02-16 4:12 UTC (permalink / raw) To: linux-input Cc: Jonathan Corbet, Jiri Kosina, Benjamin Tissoires, linux-doc, linux-kernel, Pavel Machek, Aditya Garg, Aun-Ali Zaidi, Kerem Karabay, Andy Shevchenko, Thomas Weißschuh, Orlando Chamberlain Advice use of either "input*:*:kbd_backlight" or ":*:kbd_backlight". We don't want drivers using vendor or product name (e.g. "smc", "apple", "asus") as this information is available from sysfs anyway, and it made the folder names inconsistent. Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com> --- Documentation/leds/well-known-leds.txt | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Documentation/leds/well-known-leds.txt b/Documentation/leds/well-known-leds.txt index 2160382c86be..4e5429fce4d8 100644 --- a/Documentation/leds/well-known-leds.txt +++ b/Documentation/leds/well-known-leds.txt @@ -44,6 +44,14 @@ Legacy: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900) Frontlight/backlight of main keyboard. +Good: ":*:kbd_backlight" +Good: "input*:*:kbd_backlight" +Legacy: "*:*:kbd_backlight" + +Many drivers have the vendor or product name as the first field of the led name, +this makes names inconsistent and is redundant as that information is already in +sysfs. + Legacy: "button-backlight" (Motorola Droid 4) Some phones have touch buttons below screen; it is different from main -- 2.39.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards 2023-02-16 4:12 [PATCHv2 0/2] Apple Magic Keyboard Backlight Orlando Chamberlain 2023-02-16 4:12 ` [PATCH 1/2] Documentation: leds: standardise keyboard backlight led names Orlando Chamberlain @ 2023-02-16 4:12 ` Orlando Chamberlain 2023-02-16 5:41 ` Thomas Weißschuh 2023-02-16 7:10 ` Orlando Chamberlain 1 sibling, 2 replies; 8+ messages in thread From: Orlando Chamberlain @ 2023-02-16 4:12 UTC (permalink / raw) To: linux-input Cc: Jonathan Corbet, Jiri Kosina, Benjamin Tissoires, linux-doc, linux-kernel, Pavel Machek, Aditya Garg, Aun-Ali Zaidi, Kerem Karabay, Andy Shevchenko, Thomas Weißschuh, Orlando Chamberlain 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> --- MAINTAINERS | 6 ++ drivers/hid/Kconfig | 13 +++ drivers/hid/Makefile | 1 + drivers/hid/apple-magic-backlight.c | 125 ++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+) create mode 100644 drivers/hid/apple-magic-backlight.c diff --git a/MAINTAINERS b/MAINTAINERS index fb1471cb5ed3..3319f0c3ed1e 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 e2a5d30c8895..f4702d32ce2f 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -130,6 +130,19 @@ config HID_APPLE Say Y here if you want support for keyboards of Apple iBooks, PowerBooks, MacBooks, MacBook Pros and Apple Aluminum. +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 e8014c1a2f8b..5cbfe85dd31b 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_HID_ACCUTOUCH) += hid-accutouch.o obj-$(CONFIG_HID_ALPS) += hid-alps.o obj-$(CONFIG_HID_ACRUX) += hid-axff.o obj-$(CONFIG_HID_APPLE) += hid-apple.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 000000000000..ed5bcf5bb599 --- /dev/null +++ b/drivers/hid/apple-magic-backlight.c @@ -0,0 +1,125 @@ +// 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 <linux/leds.h> +#include <linux/device.h> +#include <linux/errno.h> +#include <dt-bindings/leds/common.h> + +#include "hid-ids.h" + +#define HID_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_report_set(struct hid_report *rep, u16 value, u8 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_report_set(backlight->power, brightness ? 1 : 0, rate); + if (brightness) + apple_magic_backlight_report_set(backlight->brightness, 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 == HID_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 = ":white:" LED_FUNCTION_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 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, +}; +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.39.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards 2023-02-16 4:12 ` [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards Orlando Chamberlain @ 2023-02-16 5:41 ` Thomas Weißschuh 2023-02-16 5:58 ` Orlando Chamberlain 2023-02-16 11:51 ` Andy Shevchenko 2023-02-16 7:10 ` Orlando Chamberlain 1 sibling, 2 replies; 8+ messages in thread From: Thomas Weißschuh @ 2023-02-16 5:41 UTC (permalink / raw) To: Orlando Chamberlain Cc: linux-input, Jonathan Corbet, Jiri Kosina, Benjamin Tissoires, linux-doc, linux-kernel, Pavel Machek, Aditya Garg, Aun-Ali Zaidi, Kerem Karabay, Andy Shevchenko On Thu, Feb 16, 2023 at 03:12:28PM +1100, Orlando Chamberlain wrote: > 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> The last Signed-off-by should be yours. See Documentation/process/submitting-patches.rst. > --- > MAINTAINERS | 6 ++ > drivers/hid/Kconfig | 13 +++ > drivers/hid/Makefile | 1 + > drivers/hid/apple-magic-backlight.c | 125 ++++++++++++++++++++++++++++ > 4 files changed, 145 insertions(+) > create mode 100644 drivers/hid/apple-magic-backlight.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index fb1471cb5ed3..3319f0c3ed1e 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 e2a5d30c8895..f4702d32ce2f 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -130,6 +130,19 @@ config HID_APPLE > Say Y here if you want support for keyboards of Apple iBooks, PowerBooks, > MacBooks, MacBook Pros and Apple Aluminum. > > +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 e8014c1a2f8b..5cbfe85dd31b 100644 > --- a/drivers/hid/Makefile > +++ b/drivers/hid/Makefile > @@ -26,6 +26,7 @@ obj-$(CONFIG_HID_ACCUTOUCH) += hid-accutouch.o > obj-$(CONFIG_HID_ALPS) += hid-alps.o > obj-$(CONFIG_HID_ACRUX) += hid-axff.o > obj-$(CONFIG_HID_APPLE) += hid-apple.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 000000000000..ed5bcf5bb599 > --- /dev/null > +++ b/drivers/hid/apple-magic-backlight.c > @@ -0,0 +1,125 @@ > +// 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> No need for linux/usb.h. > +#include <linux/leds.h> > +#include <linux/device.h> > +#include <linux/errno.h> > +#include <dt-bindings/leds/common.h> > + > +#include "hid-ids.h" > + > +#define HID_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_report_set(struct hid_report *rep, u16 value, u8 rate) > +{ Could "value" be a s32? "logical_maximum" from the field is a s32, so there is no risk of truncation. > + 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_report_set(backlight->power, brightness ? 1 : 0, rate); > + if (brightness) > + apple_magic_backlight_report_set(backlight->brightness, 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 == HID_USAGE_MAGIC_BL)) > + return -ENODEV; I don't think hdev->collection can ever be NULL. Also personally I would prefer this: !hdev->collection || hdev->collection[0].usage != HID_USAGE_MAGIC_BL Like it is done with the reports below. > + > + backlight = devm_kzalloc(&hdev->dev, sizeof(*backlight), GFP_KERNEL); > + if (!backlight) > + return -ENOMEM; > + > + hid_set_drvdata(hdev, backlight); Is this needed? > + > + 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 = ":white:" LED_FUNCTION_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 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, > +}; > +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.39.1 > Note: Only your cover letter has the "v2" prefix. Normally git format-patch should apply this properly to all patches when using --reroll-count. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards 2023-02-16 5:41 ` Thomas Weißschuh @ 2023-02-16 5:58 ` Orlando Chamberlain 2023-02-16 11:51 ` Andy Shevchenko 1 sibling, 0 replies; 8+ messages in thread From: Orlando Chamberlain @ 2023-02-16 5:58 UTC (permalink / raw) To: Thomas Weißschuh Cc: linux-input, Jonathan Corbet, Jiri Kosina, Benjamin Tissoires, linux-doc, linux-kernel, Pavel Machek, Aditya Garg, Aun-Ali Zaidi, Kerem Karabay, Andy Shevchenko On Thu, 16 Feb 2023 05:41:05 +0000 Thomas Weißschuh <thomas@t-8ch.de> wrote: > On Thu, Feb 16, 2023 at 03:12:28PM +1100, Orlando Chamberlain wrote: > > 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> > > The last Signed-off-by should be yours. > See Documentation/process/submitting-patches.rst. I'll reorder that in v3, thanks for pointing it out. > > > --- > > MAINTAINERS | 6 ++ > > drivers/hid/Kconfig | 13 +++ > > drivers/hid/Makefile | 1 + > > drivers/hid/apple-magic-backlight.c | 125 > > ++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+) > > create mode 100644 drivers/hid/apple-magic-backlight.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index fb1471cb5ed3..3319f0c3ed1e 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 e2a5d30c8895..f4702d32ce2f 100644 > > --- a/drivers/hid/Kconfig > > +++ b/drivers/hid/Kconfig > > @@ -130,6 +130,19 @@ config HID_APPLE > > Say Y here if you want support for keyboards of > > Apple iBooks, PowerBooks, MacBooks, MacBook Pros and Apple Aluminum. > > > > +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 e8014c1a2f8b..5cbfe85dd31b 100644 > > --- a/drivers/hid/Makefile > > +++ b/drivers/hid/Makefile > > @@ -26,6 +26,7 @@ obj-$(CONFIG_HID_ACCUTOUCH) += > > hid-accutouch.o obj-$(CONFIG_HID_ALPS) += hid-alps.o > > obj-$(CONFIG_HID_ACRUX) += hid-axff.o > > obj-$(CONFIG_HID_APPLE) += hid-apple.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 000000000000..ed5bcf5bb599 > > --- /dev/null > > +++ b/drivers/hid/apple-magic-backlight.c > > @@ -0,0 +1,125 @@ > > +// 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> > > No need for linux/usb.h. I'll fix that in v3. > > > +#include <linux/leds.h> > > +#include <linux/device.h> > > +#include <linux/errno.h> > > +#include <dt-bindings/leds/common.h> > > + > > +#include "hid-ids.h" > > + > > +#define HID_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_report_set(struct hid_report > > *rep, u16 value, u8 rate) +{ > > Could "value" be a s32? "logical_maximum" from the field is a s32, > so there is no risk of truncation. That makes sense, I'll put it in v3. > > > + 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_report_set(backlight->power, > > brightness ? 1 : 0, rate); > > + if (brightness) > > + > > apple_magic_backlight_report_set(backlight->brightness, 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 == > > HID_USAGE_MAGIC_BL)) > > + return -ENODEV; > > I don't think hdev->collection can ever be NULL. I'll remove that check, I don't think it ever was null for me. > > Also personally I would prefer this: > > !hdev->collection || hdev->collection[0].usage != HID_USAGE_MAGIC_BL That is neater, I'll use it in v3. > > Like it is done with the reports below. > > > + > > + backlight = devm_kzalloc(&hdev->dev, sizeof(*backlight), > > GFP_KERNEL); > > + if (!backlight) > > + return -ENOMEM; > > + > > + hid_set_drvdata(hdev, backlight); > > Is this needed? Nope, I'll take that out in v3. > > > + > > + 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 = ":white:" > > LED_FUNCTION_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 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, > > +}; > > +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.39.1 > > > > Note: Only your cover letter has the "v2" prefix. > Normally git format-patch should apply this properly to all patches > when using --reroll-count. I realised that after I sent the patches, I had manually added v2 to the cover letter but I forgot about the patches themselves. I didn't know about --reroll-count, thanks! ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards 2023-02-16 5:41 ` Thomas Weißschuh 2023-02-16 5:58 ` Orlando Chamberlain @ 2023-02-16 11:51 ` Andy Shevchenko 1 sibling, 0 replies; 8+ messages in thread From: Andy Shevchenko @ 2023-02-16 11:51 UTC (permalink / raw) To: Thomas Weißschuh Cc: Orlando Chamberlain, linux-input, Jonathan Corbet, Jiri Kosina, Benjamin Tissoires, linux-doc, linux-kernel, Pavel Machek, Aditya Garg, Aun-Ali Zaidi, Kerem Karabay, Andy Shevchenko On Thu, Feb 16, 2023 at 7:41 AM Thomas Weißschuh <thomas@t-8ch.de> wrote: > On Thu, Feb 16, 2023 at 03:12:28PM +1100, Orlando Chamberlain wrote: ... > Note: Only your cover letter has the "v2" prefix. > Normally git format-patch should apply this properly to all patches when > using --reroll-count. There is an option -v<X>, where <X> is a version applied to all patches in the lot. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards 2023-02-16 4:12 ` [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards Orlando Chamberlain 2023-02-16 5:41 ` Thomas Weißschuh @ 2023-02-16 7:10 ` Orlando Chamberlain 2023-02-16 11:49 ` Andy Shevchenko 1 sibling, 1 reply; 8+ messages in thread From: Orlando Chamberlain @ 2023-02-16 7:10 UTC (permalink / raw) To: linux-input Cc: Jonathan Corbet, Jiri Kosina, Benjamin Tissoires, linux-doc, linux-kernel, Pavel Machek, Aditya Garg, Aun-Ali Zaidi, Kerem Karabay, Andy Shevchenko, Thomas Weißschuh On Thu, 16 Feb 2023 15:12:28 +1100 Orlando Chamberlain <orlandoch.dev@gmail.com> wrote: > 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> > --- > MAINTAINERS | 6 ++ > drivers/hid/Kconfig | 13 +++ > drivers/hid/Makefile | 1 + > drivers/hid/apple-magic-backlight.c | 125 A general question to the hid/input folks: Is it alright that this doesn't start with "hid-"? All the other drivers start with that so I'm not sure if its an issue. If it is, then I can rename it to "hid-apple-magic-backlight". > ++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+) > create mode 100644 drivers/hid/apple-magic-backlight.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index fb1471cb5ed3..3319f0c3ed1e 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 e2a5d30c8895..f4702d32ce2f 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -130,6 +130,19 @@ config HID_APPLE > Say Y here if you want support for keyboards of Apple > iBooks, PowerBooks, MacBooks, MacBook Pros and Apple Aluminum. > > +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 e8014c1a2f8b..5cbfe85dd31b 100644 > --- a/drivers/hid/Makefile > +++ b/drivers/hid/Makefile > @@ -26,6 +26,7 @@ obj-$(CONFIG_HID_ACCUTOUCH) += > hid-accutouch.o obj-$(CONFIG_HID_ALPS) += hid-alps.o > obj-$(CONFIG_HID_ACRUX) += hid-axff.o > obj-$(CONFIG_HID_APPLE) += hid-apple.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 000000000000..ed5bcf5bb599 > --- /dev/null > +++ b/drivers/hid/apple-magic-backlight.c > @@ -0,0 +1,125 @@ > +// 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 <linux/leds.h> > +#include <linux/device.h> > +#include <linux/errno.h> > +#include <dt-bindings/leds/common.h> > + > +#include "hid-ids.h" > + > +#define HID_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_report_set(struct hid_report *rep, > u16 value, u8 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_report_set(backlight->power, > brightness ? 1 : 0, rate); > + if (brightness) > + > apple_magic_backlight_report_set(backlight->brightness, 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 == > HID_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 = ":white:" LED_FUNCTION_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 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, > +}; > +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"); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards 2023-02-16 7:10 ` Orlando Chamberlain @ 2023-02-16 11:49 ` Andy Shevchenko 0 siblings, 0 replies; 8+ messages in thread From: Andy Shevchenko @ 2023-02-16 11:49 UTC (permalink / raw) To: Orlando Chamberlain Cc: linux-input, Jonathan Corbet, Jiri Kosina, Benjamin Tissoires, linux-doc, linux-kernel, Pavel Machek, Aditya Garg, Aun-Ali Zaidi, Kerem Karabay, Andy Shevchenko, Thomas Weißschuh On Thu, Feb 16, 2023 at 10:15 AM Orlando Chamberlain <orlandoch.dev@gmail.com> wrote: > On Thu, 16 Feb 2023 15:12:28 +1100 > Orlando Chamberlain <orlandoch.dev@gmail.com> wrote: ... > > drivers/hid/apple-magic-backlight.c | 125 > > A general question to the hid/input folks: > > Is it alright that this doesn't start with "hid-"? All the other > drivers start with that so I'm not sure if its an issue. What makes apple so special? > If it is, then > I can rename it to "hid-apple-magic-backlight". If there is a good justification of not doing that, put it into the commit message. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-02-16 11:52 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-16 4:12 [PATCHv2 0/2] Apple Magic Keyboard Backlight Orlando Chamberlain 2023-02-16 4:12 ` [PATCH 1/2] Documentation: leds: standardise keyboard backlight led names Orlando Chamberlain 2023-02-16 4:12 ` [PATCH 2/2] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards Orlando Chamberlain 2023-02-16 5:41 ` Thomas Weißschuh 2023-02-16 5:58 ` Orlando Chamberlain 2023-02-16 11:51 ` Andy Shevchenko 2023-02-16 7:10 ` Orlando Chamberlain 2023-02-16 11:49 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).