diff mbox series

[3/3] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards

Message ID 7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com
State New
Headers
Series Touch Bar and Keyboard backlight driver for Intel Macs |

Commit Message

Aditya Garg Feb. 10, 2023, 3:45 a.m. UTC 
  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

Comments

Thomas Weißschuh Feb. 10, 2023, 4:25 p.m. UTC | #1 
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
>
Orlando Chamberlain Feb. 10, 2023, 11:24 p.m. UTC | #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
> >
Thomas Weißschuh Feb. 11, 2023, 2:23 a.m. UTC | #3 
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); +}
> > > +
Thomas Weißschuh Feb. 11, 2023, 2:42 a.m. UTC | #4 
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); +}
> > > > +
Pavel Machek Feb. 11, 2023, 4:56 p.m. UTC | #5 
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
Orlando Chamberlain Feb. 12, 2023, 2:28 a.m. UTC | #6 
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
Aditya Garg Feb. 12, 2023, 5:16 a.m. UTC | #7 
> 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.
Andy Shevchenko Feb. 12, 2023, 11:18 a.m. UTC | #8 
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);
Orlando Chamberlain Feb. 16, 2023, 1:17 a.m. UTC | #9 
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 mbox series

Patch

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");