[PATCH v5 0/2] Apple Magic Keyboard Backlight

[PATCH v5 0/2] Apple Magic Keyboard Backlight

[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.

v4->v5:
- use <tab><space><space> for help in Kconfig
- prepend "hid-" to filename in MAINTAINERS

v3->v4:
- collect reviews from Andy and Thomas
- remove now unused hdev member of apple_magic_backlight

v2->v3:
- remove unneeded header inclusion
- use s32 for report value type
- remove unneeded null check
- don't set drvdata as its never used
- prepend "hid-" to module name

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

v1: https://lore.kernel.org/linux-input/7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com/
v2: https://lore.kernel.org/linux-input/20230216041224.4731-1-orlandoch.dev@gmail.com/
v3: https://lore.kernel.org/linux-input/20230217102319.3419-1-orlandoch.dev@gmail.com/
v4: https://lore.kernel.org/linux-input/20230218090709.7467-1-orlandoch.dev@gmail.com/

Orlando Chamberlain (2):
  Documentation: leds: standardise keyboard backlight led names
  HID: 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/hid-apple-magic-backlight.c | 120 ++++++++++++++++++++++++
 5 files changed, 148 insertions(+)
 create mode 100644 drivers/hid/hid-apple-magic-backlight.c

-- 
2.39.2

[PATCH v5 1/2] Documentation: leds: standardise keyboard backlight led names

[Orlando Chamberlain]

Advice use of either "input*:*:kbd_backlight" or ":*:kbd_backlight". We
don't want people 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.2

[PATCH v5 2/2] HID: hid-apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards

[Orlando Chamberlain]

This driver adds support for the keyboard backlight on Intel T2 Macs
with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1)

Co-developed-by: Kerem Karabay <kekrby@gmail.com>
Signed-off-by: Kerem Karabay <kekrby@gmail.com>
Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
---
v4->v5:
- use <tab><space><space> for help in Kconfig
- prepend "hid-" to filename in MAINTAINERS
 MAINTAINERS                             |   6 ++
 drivers/hid/Kconfig                     |  13 +++
 drivers/hid/Makefile                    |   1 +
 drivers/hid/hid-apple-magic-backlight.c | 120 ++++++++++++++++++++++++
 4 files changed, 140 insertions(+)
 create mode 100644 drivers/hid/hid-apple-magic-backlight.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 6a47510d1592..e004217a12eb 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/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..226210c6293c 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 hid-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..dc8df002bc86 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)	+= hid-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/hid-apple-magic-backlight.c b/drivers/hid/hid-apple-magic-backlight.c
new file mode 100644
index 000000000000..f0fc02ff3b2d
--- /dev/null
+++ b/drivers/hid/hid-apple-magic-backlight.c
@@ -0,0 +1,120 @@
+// 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/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_report *brightness;
+	struct hid_report *power;
+};
+
+static void apple_magic_backlight_report_set(struct hid_report *rep, s32 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[0].usage != HID_USAGE_MAGIC_BL)
+		return -ENODEV;
+
+	backlight = devm_kzalloc(&hdev->dev, sizeof(*backlight), GFP_KERNEL);
+	if (!backlight)
+		return -ENOMEM;
+
+	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->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 = "hid-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.2

Re: [PATCH v5 0/2] Apple Magic Keyboard Backlight

[Jiri Kosina]

On Mon, 20 Feb 2023, Orlando Chamberlain wrote:

> 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.
> 
> v4->v5:
> - use <tab><space><space> for help in Kconfig
> - prepend "hid-" to filename in MAINTAINERS
> 
> v3->v4:
> - collect reviews from Andy and Thomas
> - remove now unused hdev member of apple_magic_backlight
> 
> v2->v3:
> - remove unneeded header inclusion
> - use s32 for report value type
> - remove unneeded null check
> - don't set drvdata as its never used
> - prepend "hid-" to module name
> 
> 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
> 
> v1: https://lore.kernel.org/linux-input/7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com/
> v2: https://lore.kernel.org/linux-input/20230216041224.4731-1-orlandoch.dev@gmail.com/
> v3: https://lore.kernel.org/linux-input/20230217102319.3419-1-orlandoch.dev@gmail.com/
> v4: https://lore.kernel.org/linux-input/20230218090709.7467-1-orlandoch.dev@gmail.com/
> 
> Orlando Chamberlain (2):
>   Documentation: leds: standardise keyboard backlight led names
>   HID: 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/hid-apple-magic-backlight.c | 120 ++++++++++++++++++++++++
>  5 files changed, 148 insertions(+)
>  create mode 100644 drivers/hid/hid-apple-magic-backlight.c

Hi,

thanks for creating the support for backlight.

Is there any reason why not to fold all this into existing hid-apple? I 
don't think we need separate driver for the backlist, separated from the 
rest of hid-apple support.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v5 0/2] Apple Magic Keyboard Backlight

[Aditya Garg]

> Hi,
> 
> thanks for creating the support for backlight.
> 
> Is there any reason why not to fold all this into existing hid-apple? I 
> don't think we need separate driver for the backlist, separated from the 
> rest of hid-apple support.
> 
> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

As far as I know, hid-apple manages the keyboards and trackpads on Macs.

The magic backlight is managed by the touchbar on T2 Macs, so if you wanna integrate the driver in some other one, then it should be the to-be-upstreamed touchbar driver.

But when we did that, the MacBook Air 2020, the model which has magic backlight, but no touchbar faced issues. lsusb interestingly shows presence of touch bar backlight even on this model, but backlight is registered at the 0th interface on Air, and 1st interface on the Pros. So, the co-author, Kerem Karabay suggested using a separate driver.

Although, the authors may give more detailed reason for the same.

Re: [PATCH v5 0/2] Apple Magic Keyboard Backlight

[Orlando Chamberlain]

On Fri, 10 Mar 2023 15:36:34 +0100 (CET)
Jiri Kosina <jikos@kernel.org> wrote:

> On Mon, 20 Feb 2023, Orlando Chamberlain wrote:
> 
> > 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.
> > 
> > v4->v5:
> > - use <tab><space><space> for help in Kconfig
> > - prepend "hid-" to filename in MAINTAINERS
> > 
> > v3->v4:
> > - collect reviews from Andy and Thomas
> > - remove now unused hdev member of apple_magic_backlight
> > 
> > v2->v3:
> > - remove unneeded header inclusion
> > - use s32 for report value type
> > - remove unneeded null check
> > - don't set drvdata as its never used
> > - prepend "hid-" to module name
> > 
> > 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
> > 
> > v1:
> > https://lore.kernel.org/linux-input/7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com/
> > v2:
> > https://lore.kernel.org/linux-input/20230216041224.4731-1-orlandoch.dev@gmail.com/
> > v3:
> > https://lore.kernel.org/linux-input/20230217102319.3419-1-orlandoch.dev@gmail.com/
> > v4:
> > https://lore.kernel.org/linux-input/20230218090709.7467-1-orlandoch.dev@gmail.com/
> > 
> > Orlando Chamberlain (2):
> >   Documentation: leds: standardise keyboard backlight led names
> >   HID: 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/hid-apple-magic-backlight.c | 120
> > ++++++++++++++++++++++++ 5 files changed, 148 insertions(+)
> >  create mode 100644 drivers/hid/hid-apple-magic-backlight.c  
> 
> Hi,
> 
> thanks for creating the support for backlight.
> 
> Is there any reason why not to fold all this into existing hid-apple?
> I don't think we need separate driver for the backlist, separated
> from the rest of hid-apple support.

Hi Jiri,

I think we can do that if we modify hid-apple to support usb endpoints
with only the keyboard backlight and no keyboard, assuming it doesn't
prevent the (not upstream) touchbar driver from using the touchbar
backlight interface (and I don't think it will, given hid-apple lets a
different driver bind to the trackpad interface of the
keyboard/trackpad usb device).

> 
> Thanks,
>