* Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.
From: Aditya Garg @ 2023-02-10 13:07 UTC (permalink / raw)
To: Orlando Chamberlain
Cc: Thomas Weißschuh, Jiri Kosina, jkosina@suse.cz,
benjamin.tissoires@redhat.com, Andy Shevchenko,
andy.shevchenko@gmail.com, LKML, linux-input@vger.kernel.org,
ronald@innovation.ch, kekrby@gmail.com
In-Reply-To: <20230210232043.18483401@redecorated-mbp>
> Were the changes needed for these structs to be const in the
> linux-input tree for 6.3? If so then if you're applying the patches
> onto linus' tree that might be why there are errors about consts.
I’d want the maintainers comment on this. Imo, these 2 structures needn’t be constantified.
Also, it would be nice if we could get a review on the other 2 patches, so that a v2 can be prepared.
^ permalink raw reply
* Re: [PATCH v2 3/3] arm64: dts: qcom: sdm845-oneplus: add tri-state-key
From: Gergo Koteles @ 2023-02-10 13:45 UTC (permalink / raw)
To: Krzysztof Kozlowski, Dmitry Torokhov, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Jiri Kosina,
Benjamin Tissoires
Cc: linux-input, linux-kernel, linux-arm-msm, devicetree,
Caleb Connolly
In-Reply-To: <dd77a886-5ab4-c6d3-bb84-5849c411aa7b@linaro.org>
On 2023. 02. 10. 12:33, Krzysztof Kozlowski wrote:
> On 10/02/2023 00:25, Gergo Koteles wrote:
>> The tri-state-key is a sound profile switch found on the OnePlus 6,
>> Android maps the states to "mute", "vibrate" and "ring". Expose them as
>> ABS_SND_PROFILE events.
>> The previous GPIO numbers were wrong. Update them to the correct
>> ones.
>>
>> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
>> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
>> Signed-off-by: Gergo Koteles <soyer@irl.hu>
>
> Where are other patches? I got only 3/3.
>
Hi Krzysztof,
Sorry, I missed the --thread option for git format-patch.
>> ---
>> .../boot/dts/qcom/sdm845-oneplus-common.dtsi | 39 ++++++++++++++++++-
>> 1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>> index 64638ea94db7..e45d4fdead82 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>> @@ -52,6 +52,43 @@ key-vol-up {
>> };
>> };
>>
>> + tri-state-key {
>> + compatible = "gpio-keys";
>> + label = "Tri-state key";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&tri_state_key_default>;
>
> Missing blank line.
>
I'll add it to v3.
>> + state-top {
>
> Does not look like you tested the DTS against bindings. Please run `make
> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
> for instructions).
>
I ran dtbs_check with
DT_SCHEMA_FILES=Documentation/devicetree/bindings/arm/qcom.yaml. It only
shows warnings for msm8996-oneplus3, but not for sdm845-oneplus phones.
Is there anything else I need to check?
...
arch/arm64/boot/dts/qcom/msm8996-oneplus3.dtb: /: qcom,board-id: 'oneOf'
conditional failed, one must be fixed:
[8, 0, 15801, 15, 8, 0, 15801, 16] is too long
From schema: /Documentation/devicetree/bindings/arm/qcom.yaml
...
DTC_CHK arch/arm64/boot/dts/qcom/sdm845-oneplus-enchilada.dtb
DTC_CHK arch/arm64/boot/dts/qcom/sdm845-oneplus-fajita.dtb
Thanks,
Gergo
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH v2 3/3] arm64: dts: qcom: sdm845-oneplus: add tri-state-key
From: Konrad Dybcio @ 2023-02-10 13:53 UTC (permalink / raw)
To: Gergo Koteles, Krzysztof Kozlowski, Dmitry Torokhov, Andy Gross,
Bjorn Andersson, Rob Herring, Krzysztof Kozlowski, Jiri Kosina,
Benjamin Tissoires
Cc: linux-input, linux-kernel, linux-arm-msm, devicetree,
Caleb Connolly
In-Reply-To: <59ea3842-1c9d-11d1-8dd9-17d5d2308357@irl.hu>
On 10.02.2023 14:45, Gergo Koteles wrote:
> On 2023. 02. 10. 12:33, Krzysztof Kozlowski wrote:
>> On 10/02/2023 00:25, Gergo Koteles wrote:
>>> The tri-state-key is a sound profile switch found on the OnePlus 6,
>>> Android maps the states to "mute", "vibrate" and "ring". Expose them as
>>> ABS_SND_PROFILE events.
>>> The previous GPIO numbers were wrong. Update them to the correct
>>> ones.
>>>
>>> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
>>> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
>>> Signed-off-by: Gergo Koteles <soyer@irl.hu>
>>
>> Where are other patches? I got only 3/3.
>>
> Hi Krzysztof,
>
> Sorry, I missed the --thread option for git format-patch.
>
>>> ---
>>> .../boot/dts/qcom/sdm845-oneplus-common.dtsi | 39 ++++++++++++++++++-
>>> 1 file changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>> index 64638ea94db7..e45d4fdead82 100644
>>> --- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>> @@ -52,6 +52,43 @@ key-vol-up {
>>> };
>>> };
>>> + tri-state-key {
>>> + compatible = "gpio-keys";
>>> + label = "Tri-state key";
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&tri_state_key_default>;
>>
>> Missing blank line.
>>
>
> I'll add it to v3.
While at it, please put pinctrl-names after pinctrl-0.
>
>>> + state-top {
>>
>> Does not look like you tested the DTS against bindings. Please run `make
>> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
>> for instructions).
>>
>
> I ran dtbs_check with DT_SCHEMA_FILES=Documentation/devicetree/bindings/arm/qcom.yaml. It only shows warnings for msm8996-oneplus3, but not for sdm845-oneplus phones. Is there anything else I need to check?
You're only checking against a schema file which validates msm-id and
machine compatibles. The goal is to not introduce *any* new warnings.
You want to run:
make (your make args) CHECK_DTBS=1 qcom/sdm845-oneplus-enchilada.dtb
pre and post your patch.
Konrad
>
>
> ...
> arch/arm64/boot/dts/qcom/msm8996-oneplus3.dtb: /: qcom,board-id: 'oneOf' conditional failed, one must be fixed:
> [8, 0, 15801, 15, 8, 0, 15801, 16] is too long
> From schema: /Documentation/devicetree/bindings/arm/qcom.yaml
> ...
> DTC_CHK arch/arm64/boot/dts/qcom/sdm845-oneplus-enchilada.dtb
> DTC_CHK arch/arm64/boot/dts/qcom/sdm845-oneplus-fajita.dtb
>
>
> Thanks,
> Gergo
>
>>
>> Best regards,
>> Krzysztof
>>
>
^ permalink raw reply
* Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.
From: Benjamin Tissoires @ 2023-02-10 14:01 UTC (permalink / raw)
To: Aditya Garg
Cc: Orlando Chamberlain, Thomas Weißschuh, Jiri Kosina,
jkosina@suse.cz, Andy Shevchenko, andy.shevchenko@gmail.com, LKML,
linux-input@vger.kernel.org, ronald@innovation.ch,
kekrby@gmail.com
In-Reply-To: <BM1PR01MB0931E8F14D84CFB59EABEBA4B8DE9@BM1PR01MB0931.INDPRD01.PROD.OUTLOOK.COM>
On Feb 10 2023, Aditya Garg wrote:
>
> > Were the changes needed for these structs to be const in the
> > linux-input tree for 6.3? If so then if you're applying the patches
> > onto linus' tree that might be why there are errors about consts.
>
> I’d want the maintainers comment on this. Imo, these 2 structures needn’t be constantified.
The struct hid_ll_driver has to be constified, because otherwise it will
introduce an error/warning when this patch is merged in the hid tree.
For the struct hid_driver, as mentioned previously I don't think we have
the hid-core changes for that, and so you can't really constify them.
Cheers,
Benjamin
>
> Also, it would be nice if we could get a review on the other 2 patches, so that a v2 can be prepared.
^ permalink raw reply
* Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers
From: Benjamin Tissoires @ 2023-02-10 14:11 UTC (permalink / raw)
To: Hillf Danton
Cc: Pietro Borrello, Jiri Kosina, Hanno Zulla, linux-input,
linux-kernel
In-Reply-To: <20230210132017.2497-1-hdanton@sina.com>
On Fri, Feb 10, 2023 at 2:24 PM Hillf Danton <hdanton@sina.com> wrote:
>
> On Thu, 09 Feb 2023 23:58:55 +0000 Pietro Borrello <borrello@diag.uniroma1.it>
> > Use spinlocks to deal with workers introducing a wrapper
> > bigben_schedule_work(), and several spinlock checks.
> > Otherwise, bigben_set_led() may schedule bigben->worker after the
> > structure has been freed, causing a use-after-free.
> >
> > Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal")
>
> Given the flag added in 4eb1b01de5b9 and the spinlock added in this
> patchset, devm_led_classdev_register() looks to not work for you.
Actually, looking at the code now, it is clear that we need that lock.
The current code is happily changing the struct bigben_device from
multiple contexts, and pulls that without any barrier in the work
struct which should produce some interesting results :)
And we can probably abuse that lock to prevent scheduling a new work
as it is done in hid-playstation.c
I'll comment in the patch which parts need to be changed, because it
is true that this patch is definitely not mergeable as such and will
need another revision.
>
> How about replacing the advanced devm_ method with the traditional plain
> pair of led_classdev_un/register(), with the flag mentioned cut off but
> without bothering to add another lock?
>
As mentioned above, the lock is needed anyway, and will probably need
to be added in a separate patch.
Reverting to a non devm version of the led class would complexify the
driver for the error paths, and is probably not the best move IMO.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers
From: Benjamin Tissoires @ 2023-02-10 14:26 UTC (permalink / raw)
To: Pietro Borrello
Cc: Jiri Kosina, Hanno Zulla, Carlo Caione, Cristiano Giuffrida,
Bos, H.J., Jakob Koschel, Hillf Danton, Roderick Colenbrander,
linux-input, linux-kernel
In-Reply-To: <20230125-hid-unregister-leds-v3-1-0a52ac225e00@diag.uniroma1.it>
On Feb 09 2023, Pietro Borrello wrote:
> Use spinlocks to deal with workers introducing a wrapper
> bigben_schedule_work(), and several spinlock checks.
> Otherwise, bigben_set_led() may schedule bigben->worker after the
> structure has been freed, causing a use-after-free.
>
> Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal")
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> ---
> drivers/hid/hid-bigbenff.c | 34 +++++++++++++++++++++++++++++-----
> 1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c
> index e8b16665860d..28769aa7fed6 100644
> --- a/drivers/hid/hid-bigbenff.c
> +++ b/drivers/hid/hid-bigbenff.c
> @@ -174,6 +174,7 @@ static __u8 pid0902_rdesc_fixed[] = {
> struct bigben_device {
> struct hid_device *hid;
> struct hid_report *report;
> + spinlock_t lock;
> bool removed;
> u8 led_state; /* LED1 = 1 .. LED4 = 8 */
> u8 right_motor_on; /* right motor off/on 0/1 */
> @@ -184,15 +185,24 @@ struct bigben_device {
> struct work_struct worker;
> };
>
> +static inline void bigben_schedule_work(struct bigben_device *bigben)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&bigben->lock, flags);
> + if (!bigben->removed)
> + schedule_work(&bigben->worker);
> + spin_unlock_irqrestore(&bigben->lock, flags);
> +}
>
> static void bigben_worker(struct work_struct *work)
> {
> struct bigben_device *bigben = container_of(work,
> struct bigben_device, worker);
> struct hid_field *report_field = bigben->report->field[0];
> + unsigned long flags;
>
> - if (bigben->removed || !report_field)
You are removing an important test here: if (!report_field), please keep
it.
> - return;
> + spin_lock_irqsave(&bigben->lock, flags);
>
> if (bigben->work_led) {
> bigben->work_led = false;
> @@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work)
> report_field->value[7] = 0x00; /* padding */
> hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT);
> }
> +
> + spin_unlock_irqrestore(&bigben->lock, flags);
Ouch, having hid_hw_request() called whithin a spinlock is definitely not
something that should be done.
However, the spinlock should be protecting 2 kinds of things:
- any access to any value of struct bigben_device, but in an atomic way
(i.e. copy everything you need locally in a spinlock, then release it
and never read that struct again in that function).
- the access to bigben->removed, which should be checked only in
bigben_schedule_work() and in the .remove() function.
Please note that this is what the playstation driver does: it prepares
the report under the spinlock (which is really fast) before sending the
report to the device which can be slow and be interrupted.
With that being said, it is clear that we need 2 patches for this one:
- the first one introduces the spinlock and protects the concurrent
accesses to struct bigben_device (which is roughly everything below
with the changes I just said)
- the second one introduces bigben_schedule_work() and piggy backs on
top of that new lock.
Cheers,
Benjamin
> }
>
> static int hid_bigben_play_effect(struct input_dev *dev, void *data,
> @@ -228,6 +240,7 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data,
> struct bigben_device *bigben = hid_get_drvdata(hid);
> u8 right_motor_on;
> u8 left_motor_force;
> + unsigned long flags;
>
> if (!bigben) {
> hid_err(hid, "no device data\n");
> @@ -242,10 +255,13 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data,
>
> if (right_motor_on != bigben->right_motor_on ||
> left_motor_force != bigben->left_motor_force) {
> + spin_lock_irqsave(&bigben->lock, flags);
> bigben->right_motor_on = right_motor_on;
> bigben->left_motor_force = left_motor_force;
> bigben->work_ff = true;
> - schedule_work(&bigben->worker);
> + spin_unlock_irqrestore(&bigben->lock, flags);
> +
> + bigben_schedule_work(bigben);
> }
>
> return 0;
> @@ -259,6 +275,7 @@ static void bigben_set_led(struct led_classdev *led,
> struct bigben_device *bigben = hid_get_drvdata(hid);
> int n;
> bool work;
> + unsigned long flags;
>
> if (!bigben) {
> hid_err(hid, "no device data\n");
> @@ -267,6 +284,7 @@ static void bigben_set_led(struct led_classdev *led,
>
> for (n = 0; n < NUM_LEDS; n++) {
> if (led == bigben->leds[n]) {
> + spin_lock_irqsave(&bigben->lock, flags);
> if (value == LED_OFF) {
> work = (bigben->led_state & BIT(n));
> bigben->led_state &= ~BIT(n);
> @@ -274,10 +292,11 @@ static void bigben_set_led(struct led_classdev *led,
> work = !(bigben->led_state & BIT(n));
> bigben->led_state |= BIT(n);
> }
> + spin_unlock_irqrestore(&bigben->lock, flags);
>
> if (work) {
> bigben->work_led = true;
> - schedule_work(&bigben->worker);
> + bigben_schedule_work(bigben);
> }
> return;
> }
> @@ -307,8 +326,12 @@ static enum led_brightness bigben_get_led(struct led_classdev *led)
> static void bigben_remove(struct hid_device *hid)
> {
> struct bigben_device *bigben = hid_get_drvdata(hid);
> + unsigned long flags;
>
> + spin_lock_irqsave(&bigben->lock, flags);
> bigben->removed = true;
> + spin_unlock_irqrestore(&bigben->lock, flags);
> +
> cancel_work_sync(&bigben->worker);
> hid_hw_stop(hid);
> }
> @@ -362,6 +385,7 @@ static int bigben_probe(struct hid_device *hid,
> set_bit(FF_RUMBLE, hidinput->input->ffbit);
>
> INIT_WORK(&bigben->worker, bigben_worker);
> + spin_lock_init(&bigben->lock);
>
> error = input_ff_create_memless(hidinput->input, NULL,
> hid_bigben_play_effect);
> @@ -402,7 +426,7 @@ static int bigben_probe(struct hid_device *hid,
> bigben->left_motor_force = 0;
> bigben->work_led = true;
> bigben->work_ff = true;
> - schedule_work(&bigben->worker);
> + bigben_schedule_work(bigben);
>
> hid_info(hid, "LED and force feedback support for BigBen gamepad\n");
>
>
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH v3 2/2] HID: asus: use spinlock to safely schedule workers
From: Benjamin Tissoires @ 2023-02-10 14:58 UTC (permalink / raw)
To: Pietro Borrello
Cc: Jiri Kosina, Hanno Zulla, Carlo Caione, Cristiano Giuffrida,
Bos, H.J., Jakob Koschel, Jiri Kosina, Roderick Colenbrander,
linux-input, linux-kernel
In-Reply-To: <20230125-hid-unregister-leds-v3-2-0a52ac225e00@diag.uniroma1.it>
On Feb 09 2023, Pietro Borrello wrote:
> Use spinlocks to deal with workers introducing a wrapper
> asus_schedule_work(), and several spinlock checks.
> Otherwise, asus_kbd_backlight_set() may schedule led->work after the
> structure has been freed, causing a use-after-free.
>
> Fixes: af22a610bc38 ("HID: asus: support backlight on USB keyboards")
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> ---
> drivers/hid/hid-asus.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index f99752b998f3..30e194803bd7 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -98,6 +98,7 @@ struct asus_kbd_leds {
> struct hid_device *hdev;
> struct work_struct work;
> unsigned int brightness;
> + spinlock_t lock;
> bool removed;
> };
>
> @@ -490,13 +491,23 @@ static int rog_nkey_led_init(struct hid_device *hdev)
> return ret;
> }
>
> +static void asus_schedule_work(struct asus_kbd_leds *led)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&led->lock, flags);
> + if (!led->removed)
> + schedule_work(&led->work);
> + spin_unlock_irqrestore(&led->lock, flags);
> +}
> +
> static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
> enum led_brightness brightness)
> {
> struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
> cdev);
> led->brightness = brightness;
> - schedule_work(&led->work);
> + asus_schedule_work(led);
> }
>
> static enum led_brightness asus_kbd_backlight_get(struct led_classdev *led_cdev)
> @@ -512,15 +523,17 @@ static void asus_kbd_backlight_work(struct work_struct *work)
> struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
> u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
> int ret;
> + unsigned long flags;
>
> - if (led->removed)
> - return;
> + spin_lock_irqsave(&led->lock, flags);
>
> buf[4] = led->brightness;
>
> ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf));
> if (ret < 0)
> hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
> +
> + spin_unlock_irqrestore(&led->lock, flags);
Same as in 1/2, please only keep "buf[4] = led->brightness;" under
spinlock.
Which also raises the question on why the other accesses of
led->brightness are not protected by the spinlock :)
Note that we could use an atomic to not use the spinlock, but we need
the spinlock anyway...
Cheers,
Benjamin
> }
>
> /* WMI-based keyboard backlight LED control (via asus-wmi driver) takes
> @@ -584,6 +597,7 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> drvdata->kbd_backlight->cdev.brightness_set = asus_kbd_backlight_set;
> drvdata->kbd_backlight->cdev.brightness_get = asus_kbd_backlight_get;
> INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
> + spin_lock_init(&drvdata->kbd_backlight->lock);
>
> ret = devm_led_classdev_register(&hdev->dev, &drvdata->kbd_backlight->cdev);
> if (ret < 0) {
> @@ -1119,9 +1133,13 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> static void asus_remove(struct hid_device *hdev)
> {
> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> + unsigned long flags;
>
> if (drvdata->kbd_backlight) {
> + spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
> drvdata->kbd_backlight->removed = true;
> + spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
> +
> cancel_work_sync(&drvdata->kbd_backlight->work);
> }
>
>
> --
> 2.25.1
>
^ permalink raw reply
* [PATCH] Input: synaptics - enable InterTouch on Panasonic Let's Note CF-RZ6
From: Dale Whinham @ 2023-02-10 15:12 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input, linux-kernel; +Cc: Dale Whinham
SYN0505 confirmed working reliably on the CF-RZ6 using
'psmouse.synaptics_intertouch=1'.
This ID may also be used by other models in the Panasonic Let's Note
/Toughbook range.
Signed-off-by: Dale Whinham <daleyo@gmail.com>
---
drivers/input/mouse/synaptics.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index b0f776448a1c..7570f6770ed2 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -188,6 +188,7 @@ static const char * const smbus_pnp_ids[] = {
"LEN2054", /* E480 */
"LEN2055", /* E580 */
"LEN2068", /* T14 Gen 1 */
+ "SYN0505", /* Panasonic Let's Note CF-RZ6 */
"SYN3052", /* HP EliteBook 840 G4 */
"SYN3221", /* HP 15-ay000 */
"SYN323d", /* HP Spectre X360 13-w013dx */
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.
From: Thomas Weißschuh @ 2023-02-10 15:33 UTC (permalink / raw)
To: Aditya Garg
Cc: Jiri Kosina, jkosina@suse.cz, benjamin.tissoires@redhat.com,
Andy Shevchenko, andy.shevchenko@gmail.com, LKML,
linux-input@vger.kernel.org, ronald@innovation.ch,
kekrby@gmail.com, Orlando Chamberlain
In-Reply-To: <B9E319F8-6047-40E5-BD9F-D90D6504AA9E@live.com>
Responses inline
On Fri, Feb 10, 2023 at 12:05:13PM +0000, Aditya Garg wrote:
> > On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> >
> > Hi,
> >
> > some comments inline.
> >
> > On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> >
> >> +
> >> +static struct {
> >> + unsigned int usage;
> >> + struct hid_device_id *dev_id;
> >> +} appleib_usage_map[] = {
> >> + /* Default iBridge configuration, key inputs and mode settings */
> >> + { 0x00010006, &appleib_sub_hid_ids[0] },
> >> + /* OS X iBridge configuration, digitizer inputs */
> >> + { 0x000D0005, &appleib_sub_hid_ids[0] },
> >> + /* All iBridge configurations, display/DFR settings */
> >> + { 0xFF120001, &appleib_sub_hid_ids[0] },
> >> + /* All iBridge configurations, ALS */
> >> + { 0x00200041, &appleib_sub_hid_ids[1] },
> >> +};
> >
> > const
> >
>
> Constantifying this results in compiler giving warnings
>
> drivers/hid/apple-ibridge.c:78:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> 78 | { 0x00200041, &appleib_sub_hid_ids[1] },
For this you also have to constify the hid_device_id *dev_id in
appleib_usage_map. And then propagate this change to some functions and
variables.
> | ^
> drivers/hid/apple-ibridge.c: In function 'appleib_add_sub_dev':
> drivers/hid/apple-ibridge.c:363:29: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> 363 | sub_hdev->ll_driver = &appleib_ll_driver;
As Benjamin said this is because your changes are based on Linus' tree
but they will break as soon as they will be merged into the HID tree.
You should base your changes off of the HID tree:
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core
This issue is essentially unlucky timing.
> | ^
> drivers/hid/apple-ibridge.c: In function 'appleib_hid_probe':
> drivers/hid/apple-ibridge.c:436:12: error: expected '(' before 'hid_is_usb'
> 436 | if hid_is_usb(hdev)
> | ^~~~~~~~~~
> | (
As the error message indicates, this is invalid syntax and missing a
'('.
What you want to do is to check for
if (!hid_is_usb(hdev))
return -ENODEV;
*before* calling hid_to_usb_dev(hdev);
> In file included from drivers/hid/apple-ibridge.c:48:
> drivers/hid/apple-ibridge.c: In function 'appleib_probe':
> drivers/hid/apple-ibridge.c:544:35: warning: passing argument 1 of '__hid_register_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> 544 | ret = hid_register_driver(&appleib_hid_driver);
> | ^~~~~~~~~~~~~~~~~~~
> ./include/linux/hid.h:898:31: note: in definition of macro 'hid_register_driver'
> 898 | __hid_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
> | ^~~~~~
> ./include/linux/hid.h:893:47: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
> 893 | extern int __must_check __hid_register_driver(struct hid_driver *,
> | ^~~~~~~~~~~~~~~~~~~
> drivers/hid/apple-ibridge.c: In function 'appleib_remove':
> drivers/hid/apple-ibridge.c:558:31: warning: passing argument 1 of 'hid_unregister_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
> 558 | hid_unregister_driver(&appleib_hid_driver);
> | ^~~~~~~~~~~~~~~~~~~
> ./include/linux/hid.h:900:35: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
> 900 | extern void hid_unregister_driver(struct hid_driver *);
> | ^~~~~~~~~~~~~~~~~~~
These are all because applib_hid_driver can not be const.
Sorry for the wrong advice.
Benjamin:
HID drivers can not be const because they embed a 'struct driver' that
is needed by the driver core to be mutable.
Fixing this is probably a larger enterprise.
> make[6]: *** [scripts/Makefile.build:250: drivers/hid/apple-ibridge.o] Error 1
> make[5]: *** [scripts/Makefile.build:500: drivers/hid] Error 2
> make[5]: *** Waiting for unfinished jobs….
>
> Some warnings are also due to a typo in if and constantifying `static struct hid_driver`, although they probably can
> be fixed.
>
> In short, Thomas, do you really want me to constantify the structure I
> am talking about in this email, as well `static struct hid_driver`?
struct hid_driver: Don't constify
all others: Do constify
Thomas
^ permalink raw reply
* Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.
From: Aditya Garg @ 2023-02-10 15:49 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Jiri Kosina, jkosina@suse.cz, benjamin.tissoires@redhat.com,
Andy Shevchenko, andy.shevchenko@gmail.com, LKML,
linux-input@vger.kernel.org, ronald@innovation.ch,
kekrby@gmail.com, Orlando Chamberlain
In-Reply-To: <20230210153356.zdj7gw7ztbgz2qx7@t-8ch.de>
> On 10-Feb-2023, at 9:04 PM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
>
> Responses inline
>
> On Fri, Feb 10, 2023 at 12:05:13PM +0000, Aditya Garg wrote:
>>>> On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
>>>
>>> Hi,
>>>
>>> some comments inline.
>>>
>>>> On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
>>>
>>>> +
>>>> +static struct {
>>>> + unsigned int usage;
>>>> + struct hid_device_id *dev_id;
>>>> +} appleib_usage_map[] = {
>>>> + /* Default iBridge configuration, key inputs and mode settings */
>>>> + { 0x00010006, &appleib_sub_hid_ids[0] },
>>>> + /* OS X iBridge configuration, digitizer inputs */
>>>> + { 0x000D0005, &appleib_sub_hid_ids[0] },
>>>> + /* All iBridge configurations, display/DFR settings */
>>>> + { 0xFF120001, &appleib_sub_hid_ids[0] },
>>>> + /* All iBridge configurations, ALS */
>>>> + { 0x00200041, &appleib_sub_hid_ids[1] },
>>>> +};
>>>
>>> const
>>>
>>
>> Constantifying this results in compiler giving warnings
>>
>> drivers/hid/apple-ibridge.c:78:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>> 78 | { 0x00200041, &appleib_sub_hid_ids[1] },
>
> For this you also have to constify the hid_device_id *dev_id in
> appleib_usage_map. And then propagate this change to some functions and
> variables.
>
>> | ^
>> drivers/hid/apple-ibridge.c: In function 'appleib_add_sub_dev':
>> drivers/hid/apple-ibridge.c:363:29: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>> 363 | sub_hdev->ll_driver = &appleib_ll_driver;
>
> As Benjamin said this is because your changes are based on Linus' tree
> but they will break as soon as they will be merged into the HID tree.
> You should base your changes off of the HID tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core
>
> This issue is essentially unlucky timing.
>
>> | ^
>> drivers/hid/apple-ibridge.c: In function 'appleib_hid_probe':
>> drivers/hid/apple-ibridge.c:436:12: error: expected '(' before 'hid_is_usb'
>> 436 | if hid_is_usb(hdev)
>> | ^~~~~~~~~~
>> | (
>
> As the error message indicates, this is invalid syntax and missing a
> '('.
> What you want to do is to check for
>
> if (!hid_is_usb(hdev))
> return -ENODEV;
It was a typo on my part
+ /* check and set usb config first */
+ if (hid_is_usb(hdev))
+ udev = hid_to_usb_dev(hdev);
+ else
+ return -EINVAL;
This is what I have in my patch set now.
If there is something wrong with this, then do tell me
Thanks
>
> *before* calling hid_to_usb_dev(hdev);
>
>> In file included from drivers/hid/apple-ibridge.c:48:
>> drivers/hid/apple-ibridge.c: In function 'appleib_probe':
>> drivers/hid/apple-ibridge.c:544:35: warning: passing argument 1 of '__hid_register_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>> 544 | ret = hid_register_driver(&appleib_hid_driver);
>> | ^~~~~~~~~~~~~~~~~~~
>> ./include/linux/hid.h:898:31: note: in definition of macro 'hid_register_driver'
>> 898 | __hid_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
>> | ^~~~~~
>> ./include/linux/hid.h:893:47: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>> 893 | extern int __must_check __hid_register_driver(struct hid_driver *,
>> | ^~~~~~~~~~~~~~~~~~~
>> drivers/hid/apple-ibridge.c: In function 'appleib_remove':
>> drivers/hid/apple-ibridge.c:558:31: warning: passing argument 1 of 'hid_unregister_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>> 558 | hid_unregister_driver(&appleib_hid_driver);
>> | ^~~~~~~~~~~~~~~~~~~
>> ./include/linux/hid.h:900:35: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>> 900 | extern void hid_unregister_driver(struct hid_driver *);
>> | ^~~~~~~~~~~~~~~~~~~
>
> These are all because applib_hid_driver can not be const.
> Sorry for the wrong advice.
>
> Benjamin:
> HID drivers can not be const because they embed a 'struct driver' that
> is needed by the driver core to be mutable.
> Fixing this is probably a larger enterprise.
>
>> make[6]: *** [scripts/Makefile.build:250: drivers/hid/apple-ibridge.o] Error 1
>> make[5]: *** [scripts/Makefile.build:500: drivers/hid] Error 2
>> make[5]: *** Waiting for unfinished jobs….
>>
>> Some warnings are also due to a typo in if and constantifying `static struct hid_driver`, although they probably can
>> be fixed.
>>
>> In short, Thomas, do you really want me to constantify the structure I
>> am talking about in this email, as well `static struct hid_driver`?
>
> struct hid_driver: Don't constify
> all others: Do constify
>
> Thomas
^ permalink raw reply
* Re: [PATCH 2/3] HID: apple-touchbar: Add driver for the Touch Bar on MacBook Pros
From: Thomas Weißschuh @ 2023-02-10 16:13 UTC (permalink / raw)
To: Aditya Garg
Cc: Jiri Kosina, jkosina@suse.cz, benjamin.tissoires@redhat.com,
Andy Shevchenko, andy.shevchenko@gmail.com, LKML,
linux-input@vger.kernel.org, ronald@innovation.ch,
kekrby@gmail.com, Orlando Chamberlain
In-Reply-To: <868AA58D-2399-4E4A-A6C6-73F88DB13992@live.com>
On Fri, Feb 10, 2023 at 03:44:26AM +0000, Aditya Garg wrote:
> From: Ronald Tschalär <ronald@innovation.ch>
>
> This driver enables basic touch bar functionality: enabling it, switching
> between modes on FN key press, and dimming and turning the display
> off/on when idle/active.
>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> [Kerem Karabay: use USB product IDs from hid-ids.h]
> [Kerem Karabay: use hid_hw_raw_request except when setting the touchbar mode on T1 Macs]
> [Kerem Karabay: update Kconfig description]
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> [Orlando Chamberlain: add usage check to not bind to keyboard backlight interface]
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> [Aditya Garg: check if apple-touchbar is enabled in the special driver list]
> [Aditya Garg: fix suspend on T2 Macs]
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
> drivers/hid/Kconfig | 11 +
> drivers/hid/Makefile | 1 +
> drivers/hid/apple-touchbar.c | 1500 ++++++++++++++++++++++++++++++++++
> drivers/hid/hid-quirks.c | 6 +-
> 4 files changed, 1516 insertions(+), 2 deletions(-)
> create mode 100644 drivers/hid/apple-touchbar.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index e69afa5f4..4ec669267 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -134,6 +134,7 @@ config HID_APPLE_IBRIDGE
> tristate "Apple iBridge"
> depends on USB_HID
> depends on (X86 && ACPI) || COMPILE_TEST
> + imply HID_APPLE_TOUCHBAR
> imply HID_SENSOR_HUB
> imply HID_SENSOR_ALS
> help
> @@ -145,6 +146,16 @@ config HID_APPLE_IBRIDGE
> To compile this driver as a module, choose M here: the
> module will be called apple-ibridge.
>
> +config HID_APPLE_TOUCHBAR
> + tristate "Apple Touch Bar"
> + depends on USB_HID
> + help
> + Say Y here if you want support for the Touch Bar on x86
> + MacBook Pros.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple-touchbar.
> +
> config HID_APPLEIR
> tristate "Apple infrared receiver"
> depends on (USB_HID)
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index b61373cd8..c792e42fe 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_HID_ALPS) += hid-alps.o
> obj-$(CONFIG_HID_ACRUX) += hid-axff.o
> obj-$(CONFIG_HID_APPLE) += hid-apple.o
> obj-$(CONFIG_HID_APPLE_IBRIDGE) += apple-ibridge.o
> +obj-$(CONFIG_HID_APPLE_TOUCHBAR) += apple-touchbar.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-touchbar.c b/drivers/hid/apple-touchbar.c
> new file mode 100644
> index 000000000..ff6a8493b
> --- /dev/null
> +++ b/drivers/hid/apple-touchbar.c
> @@ -0,0 +1,1500 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Touch Bar Driver
> + *
> + * Copyright (c) 2017-2018 Ronald Tschalär
> + */
> +
> +/*
> + * Recent MacBookPro models (MacBookPro 13,[23] and later) have a touch bar,
> + * which is exposed via several USB interfaces. MacOS supports a fancy mode
> + * where arbitrary buttons can be defined; this driver currently only
> + * supports the simple mode that consists of 3 predefined layouts
> + * (escape-only, esc + special keys, and esc + function keys).
> + *
> + * The first USB HID interface supports two reports, an input report that
> + * is used to report the key presses, and an output report which can be
> + * used to set the touch bar "mode": touch bar off (in which case no touches
> + * are reported at all), escape key only, escape + 12 function keys, and
> + * escape + several special keys (including brightness, audio volume,
> + * etc). The second interface supports several, complex reports, most of
> + * which are unknown at this time, but one of which has been determined to
> + * allow for controlling of the touch bar's brightness: off (though touches
> + * are still reported), dimmed, and full brightness. This driver makes
> + * use of these two reports.
> + */
> +
> +#define dev_fmt(fmt) "tb: " fmt
This is a bit nondescriptive name. Maybe use KBUILD_MODNAME or the name
of the HID driver as prefix?
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/input.h>
> +#include <linux/jiffies.h>
> +#include <linux/ktime.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb.h>
> +#include <linux/workqueue.h>
> +
> +#include "hid-ids.h"
> +#include "apple-ibridge.h"
> +
> +#define HID_UP_APPLE 0xff120000
> +#define HID_USAGE_MODE (HID_UP_CUSTOM | 0x0004)
> +#define HID_USAGE_APPLE_APP (HID_UP_APPLE | 0x0001)
> +#define HID_USAGE_DISP (HID_UP_APPLE | 0x0021)
> +#define HID_USAGE_DISP_AUX1 (HID_UP_APPLE | 0x0020)
> +
> +#define APPLETB_MAX_TB_KEYS 13 /* ESC, F1-F12 */
> +
> +#define APPLETB_CMD_MODE_ESC 0
> +#define APPLETB_CMD_MODE_FN 1
> +#define APPLETB_CMD_MODE_SPCL 2
> +#define APPLETB_CMD_MODE_OFF 3
> +#define APPLETB_CMD_MODE_UPD 254
> +#define APPLETB_CMD_MODE_NONE 255
> +
> +#define APPLETB_CMD_DISP_ON 1
> +#define APPLETB_CMD_DISP_DIM 2
> +#define APPLETB_CMD_DISP_OFF 4
> +#define APPLETB_CMD_DISP_UPD 254
> +#define APPLETB_CMD_DISP_NONE 255
> +
> +#define APPLETB_FN_MODE_FKEYS 0
> +#define APPLETB_FN_MODE_NORM 1
> +#define APPLETB_FN_MODE_INV 2
> +#define APPLETB_FN_MODE_SPCL 3
> +#define APPLETB_FN_MODE_ESC 4
> +#define APPLETB_FN_MODE_MAX APPLETB_FN_MODE_ESC
> +
> +#define APPLETB_DEVID_KEYBOARD 1
> +#define APPLETB_DEVID_TOUCHPAD 2
> +
> +#define APPLETB_MAX_DIM_TIME 30
> +
> +#define APPLETB_FEATURE_IS_T1 BIT(0)
> +
> +static int appletb_tb_def_idle_timeout = 5 * 60;
> +module_param_named(idle_timeout, appletb_tb_def_idle_timeout, int, 0444);
> +MODULE_PARM_DESC(idle_timeout, "Default touch bar idle timeout:\n"
> + " [>0] - turn touch bar display off after no keyboard, trackpad, or touch bar input has been received for this many seconds;\n"
> + " the display will be turned back on as soon as new input is received\n"
> + " 0 - turn touch bar display off (input does not turn it on again)\n"
> + " -1 - turn touch bar display on (does not turn off automatically)\n"
> + " -2 - disable touch bar completely");
> +
> +static int appletb_tb_def_dim_timeout = -2;
> +module_param_named(dim_timeout, appletb_tb_def_dim_timeout, int, 0444);
> +MODULE_PARM_DESC(dim_timeout, "Default touch bar dim timeout:\n"
> + " >0 - dim touch bar display after no keyboard, trackpad, or touch bar input has been received for this many seconds\n"
> + " the display will be returned to full brightness as soon as new input is received\n"
> + " 0 - dim touch bar display (input does not return it to full brightness)\n"
> + " -1 - disable timeout (touch bar never dimmed)\n"
> + " [-2] - calculate timeout based on idle-timeout");
> +
> +static int appletb_tb_def_fn_mode = APPLETB_FN_MODE_NORM;
> +module_param_named(fnmode, appletb_tb_def_fn_mode, int, 0444);
> +MODULE_PARM_DESC(fnmode, "Default Fn key mode:\n"
> + " 0 - function-keys only\n"
> + " [1] - fn key switches from special to function-keys\n"
> + " 2 - inverse of 1\n"
> + " 3 - special keys only\n"
> + " 4 - escape key only");
> +
> +static ssize_t idle_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +static ssize_t idle_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size);
> +static DEVICE_ATTR_RW(idle_timeout);
> +
> +static ssize_t dim_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +static ssize_t dim_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size);
> +static DEVICE_ATTR_RW(dim_timeout);
> +
> +static ssize_t fnmode_show(struct device *dev, struct device_attribute *attr,
> + char *buf);
> +static ssize_t fnmode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t size);
> +static DEVICE_ATTR_RW(fnmode);
> +
> +static struct attribute *appletb_attrs[] = {
> + &dev_attr_idle_timeout.attr,
> + &dev_attr_dim_timeout.attr,
> + &dev_attr_fnmode.attr,
> + NULL,
No comma.
> +};
> +
> +static const struct attribute_group appletb_attr_group = {
> + .attrs = appletb_attrs,
> +};
> +
> +struct appletb_device {
> + bool active;
> + struct device *log_dev;
> +
> + struct hid_field *mode_field;
> + struct hid_field *disp_field;
> + struct hid_field *disp_field_aux1;
> + struct appletb_iface_info {
> + struct hid_device *hdev;
> + struct usb_interface *usb_iface;
> + bool suspended;
> + } mode_iface, disp_iface;
> +
> + struct input_handler inp_handler;
> + struct input_handle kbd_handle;
> + struct input_handle tpd_handle;
> +
> + bool last_tb_keys_pressed[APPLETB_MAX_TB_KEYS];
> + bool last_tb_keys_translated[APPLETB_MAX_TB_KEYS];
> + bool last_fn_pressed;
> +
> + ktime_t last_event_time;
> +
> + unsigned char cur_tb_mode;
> + unsigned char pnd_tb_mode;
> + unsigned char cur_tb_disp;
> + unsigned char pnd_tb_disp;
> + bool tb_autopm_off;
> + bool restore_autopm;
> + struct delayed_work tb_work;
> + /* protects most of the above */
> + spinlock_t tb_lock;
> +
> + int dim_timeout;
> + int idle_timeout;
> + bool dim_to_is_calc;
> + int fn_mode;
> +
> + bool is_t1;
> +};
> +
> +struct appletb_key_translation {
> + u16 from;
> + u16 to;
> +};
> +
> +static const struct appletb_key_translation appletb_fn_codes[] = {
> + { KEY_F1, KEY_BRIGHTNESSDOWN },
> + { KEY_F2, KEY_BRIGHTNESSUP },
> + { KEY_F3, KEY_SCALE }, /* not used */
> + { KEY_F4, KEY_DASHBOARD }, /* not used */
> + { KEY_F5, KEY_KBDILLUMDOWN },
> + { KEY_F6, KEY_KBDILLUMUP },
> + { KEY_F7, KEY_PREVIOUSSONG },
> + { KEY_F8, KEY_PLAYPAUSE },
> + { KEY_F9, KEY_NEXTSONG },
> + { KEY_F10, KEY_MUTE },
> + { KEY_F11, KEY_VOLUMEDOWN },
> + { KEY_F12, KEY_VOLUMEUP },
> +};
This should be able to make use of sparse_keymap from sparse-keymap.h.
It makes the code shorter and provides a bit more functionality.
static const struct key_entry appletb_fn_keymap = {
{ KE_KEY, KEY_F1, KEY_BRIGHTNESSDOWN },
...
{ KE_END, 0 }
};
> +static struct appletb_device *appletb_dev;
> +
> +static bool appletb_disable_autopm(struct hid_device *hdev)
> +{
> + int rc;
> +
> + rc = hid_hw_power(hdev, PM_HINT_FULLON);
> +
> + if (rc == 0)
> + return true;
> +
> + hid_err(hdev,
> + "Failed to disable auto-pm on touch bar device (%d)\n", rc);
You can use "%pE" and ERR_PTR(rc) to produce nicer error strings.
> + return false;
> +}
> +
> +/*
> + * While the mode functionality is listed as a valid hid report in the usb
> + * interface descriptor, on a T1 it's not sent that way. Instead it's sent with
> + * different request-type and without a leading report-id in the data. Hence
> + * we need to send it as a custom usb control message rather via any of the
> + * standard hid_hw_*request() functions. The device might return EPIPE for a
> + * while after setting the display mode on T1 models, so retrying should be
> + * done on those models.
> + */
> +static int appletb_set_tb_mode(struct appletb_device *tb_dev,
> + unsigned char mode)
> +{
> + struct hid_report *report;
> + void *buf;
> + bool autopm_off = false;
> + int rc;
> +
> + if (!tb_dev->mode_iface.hdev)
> + return -ENOTCONN;
> +
> + report = tb_dev->mode_field->report;
> +
> + if (tb_dev->is_t1) {
> + buf = kmemdup(&mode, 1, GFP_KERNEL);
> + } else {
> + char data[] = { report->id, mode };
> +
> + buf = kmemdup(data, sizeof(data), GFP_KERNEL);
These allocations don't seem to be necessary. They could just be local
buffers.
> + }
> + if (!buf)
> + return -ENOMEM;
> +
> + autopm_off = appletb_disable_autopm(tb_dev->mode_iface.hdev);
> +
> + if (tb_dev->is_t1) {
> + int tries = 0;
> + struct usb_device *dev = interface_to_usbdev(tb_dev->mode_iface.usb_iface);
> + __u8 ifnum = tb_dev->mode_iface.usb_iface->cur_altsetting->desc.bInterfaceNumber;
> +
> + do {
> + rc = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), HID_REQ_SET_REPORT,
> + USB_DIR_OUT | USB_RECIP_INTERFACE | USB_TYPE_VENDOR,
> + (report->type + 1) << 8 | report->id,
> + ifnum, buf, 1, 2000);
> +
> + if (rc != -EPIPE)
> + break;
> +
> + usleep_range(1000 << tries, 3000 << tries);
> + } while (++tries < 5);
> + } else {
> + rc = hid_hw_raw_request(tb_dev->mode_iface.hdev, report->id,
> + (__u8 *) buf, 2, report->type,
> + HID_REQ_SET_REPORT);
> + }
> +
> + if (rc < 0)
> + dev_err(tb_dev->log_dev,
> + "Failed to set touch bar mode to %u (%d)\n", mode, rc);
> +
> + if (autopm_off)
> + hid_hw_power(tb_dev->mode_iface.hdev, PM_HINT_NORMAL);
> +
> + kfree(buf);
> +
> + return rc;
> +}
> +
> +static int appletb_set_tb_disp(struct appletb_device *tb_dev,
> + unsigned char disp)
> +{
> + struct hid_report *report;
> + int rc;
> +
> + if (!tb_dev->disp_iface.hdev)
> + return -ENOTCONN;
> +
> + report = tb_dev->disp_field->report;
> +
> + rc = hid_set_field(tb_dev->disp_field_aux1, 0, 1);
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Failed to set display report field (%d)\n", rc);
> + return rc;
> + }
> +
> + rc = hid_set_field(tb_dev->disp_field, 0, disp);
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Failed to set display report field (%d)\n", rc);
> + return rc;
> + }
> +
> + /*
> + * Keep the USB interface powered on while the touch bar display is on
> + * for better responsiveness.
> + */
> + if (disp != APPLETB_CMD_DISP_OFF && !tb_dev->tb_autopm_off)
> + tb_dev->tb_autopm_off =
> + appletb_disable_autopm(report->device);
> +
> + hid_hw_request(tb_dev->disp_iface.hdev, report, HID_REQ_SET_REPORT);
> +
> + if (disp == APPLETB_CMD_DISP_OFF && tb_dev->tb_autopm_off) {
> + hid_hw_power(tb_dev->disp_iface.hdev, PM_HINT_NORMAL);
> + tb_dev->tb_autopm_off = false;
> + }
> +
> + return rc;
> +}
> +
> +static bool appletb_any_tb_key_pressed(struct appletb_device *tb_dev)
> +{
> + return !!memchr_inv(tb_dev->last_tb_keys_pressed, 0,
> + sizeof(tb_dev->last_tb_keys_pressed));
> +}
> +
> +static void appletb_schedule_tb_update(struct appletb_device *tb_dev, s64 secs)
> +{
> + schedule_delayed_work(&tb_dev->tb_work, msecs_to_jiffies(secs * 1000));
> +}
> +
> +static void appletb_set_tb_worker(struct work_struct *work)
> +{
> + struct appletb_device *tb_dev =
> + container_of(work, struct appletb_device, tb_work.work);
> + s64 time_left = 0, min_timeout, time_to_off;
> + unsigned char pending_mode;
> + unsigned char pending_disp;
> + unsigned char current_disp;
> + bool restore_autopm;
> + bool any_tb_key_pressed, need_reschedule;
> + int rc1 = 1, rc2 = 1;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + /* handle explicit mode-change request */
> + pending_mode = tb_dev->pnd_tb_mode;
> + pending_disp = tb_dev->pnd_tb_disp;
> + restore_autopm = tb_dev->restore_autopm;
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + if (pending_mode != APPLETB_CMD_MODE_NONE)
> + rc1 = appletb_set_tb_mode(tb_dev, pending_mode);
> + if (pending_mode != APPLETB_CMD_MODE_NONE &&
> + pending_disp != APPLETB_CMD_DISP_NONE)
> + msleep(25);
> + if (pending_disp != APPLETB_CMD_DISP_NONE)
> + rc2 = appletb_set_tb_disp(tb_dev, pending_disp);
> +
> + if (restore_autopm && tb_dev->tb_autopm_off)
> + appletb_disable_autopm(tb_dev->disp_field->report->device);
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + need_reschedule = false;
> +
> + if (rc1 == 0) {
> + tb_dev->cur_tb_mode = pending_mode;
> +
> + if (tb_dev->pnd_tb_mode == pending_mode)
> + tb_dev->pnd_tb_mode = APPLETB_CMD_MODE_NONE;
> + else
> + need_reschedule = true;
> + }
> +
> + if (rc2 == 0) {
> + tb_dev->cur_tb_disp = pending_disp;
> +
> + if (tb_dev->pnd_tb_disp == pending_disp)
> + tb_dev->pnd_tb_disp = APPLETB_CMD_DISP_NONE;
> + else
> + need_reschedule = true;
> + }
> + current_disp = tb_dev->cur_tb_disp;
> +
> + tb_dev->restore_autopm = false;
> +
> + /* calculate time left to next timeout */
> + if (tb_dev->idle_timeout == -2 || tb_dev->idle_timeout == 0)
> + min_timeout = -1;
> + else if (tb_dev->idle_timeout == -1)
> + min_timeout = tb_dev->dim_timeout;
> + else if (tb_dev->dim_timeout <= 0)
> + min_timeout = tb_dev->idle_timeout;
> + else
> + min_timeout = min(tb_dev->dim_timeout, tb_dev->idle_timeout);
> +
> + if (min_timeout > 0) {
> + s64 idle_time =
> + (ktime_ms_delta(ktime_get(), tb_dev->last_event_time) +
> + 500) / 1000;
> +
> + time_left = max(min_timeout - idle_time, 0LL);
> + if (tb_dev->idle_timeout <= 0)
> + time_to_off = -1;
> + else if (idle_time >= tb_dev->idle_timeout)
> + time_to_off = 0;
> + else
> + time_to_off = tb_dev->idle_timeout - idle_time;
> + } else {
> + /* not used - just to appease the compiler */
> + time_to_off = 0;
> + }
> +
> + any_tb_key_pressed = appletb_any_tb_key_pressed(tb_dev);
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + dev_dbg(tb_dev->log_dev, "timeout calc: idle_timeout=%d dim_timeout=%d min_timeout=%lld time_left=%lld need_reschedule=%d any_tb_key_pressed=%d\n",
> + tb_dev->idle_timeout, tb_dev->dim_timeout, min_timeout,
> + time_left, need_reschedule, any_tb_key_pressed);
> +
> + /* a new command arrived while we were busy - handle it */
> + if (need_reschedule) {
> + appletb_schedule_tb_update(tb_dev, 0);
> + return;
> + }
> +
> + /* if no idle/dim timeout, we're done */
> + if (min_timeout <= 0)
> + return;
> +
> + /* manage idle/dim timeout */
> + if (time_left > 0) {
> + /* we fired too soon or had a mode-change - re-schedule */
> + appletb_schedule_tb_update(tb_dev, time_left);
> + } else if (any_tb_key_pressed) {
> + /* keys are still pressed - re-schedule */
> + appletb_schedule_tb_update(tb_dev, min_timeout);
> + } else {
> + /* dim or idle timeout reached */
> + int next_disp = (time_to_off == 0) ? APPLETB_CMD_DISP_OFF :
> + APPLETB_CMD_DISP_DIM;
> + if (next_disp != current_disp &&
> + appletb_set_tb_disp(tb_dev, next_disp) == 0) {
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> + tb_dev->cur_tb_disp = next_disp;
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> + }
> +
> + if (time_to_off > 0)
> + appletb_schedule_tb_update(tb_dev, time_to_off);
> + }
> +}
> +
> +static u16 appletb_fn_to_special(u16 code)
> +{
> + int idx;
> +
> + for (idx = 0; idx < ARRAY_SIZE(appletb_fn_codes); idx++) {
> + if (appletb_fn_codes[idx].from == code)
> + return appletb_fn_codes[idx].to;
> + }
> +
> + return 0;
> +}
> +
> +static unsigned char appletb_get_cur_tb_mode(struct appletb_device *tb_dev)
> +{
> + return tb_dev->pnd_tb_mode != APPLETB_CMD_MODE_NONE ?
> + tb_dev->pnd_tb_mode : tb_dev->cur_tb_mode;
> +}
> +
> +static unsigned char appletb_get_cur_tb_disp(struct appletb_device *tb_dev)
> +{
> + return tb_dev->pnd_tb_disp != APPLETB_CMD_DISP_NONE ?
> + tb_dev->pnd_tb_disp : tb_dev->cur_tb_disp;
> +}
> +
> +static unsigned char appletb_get_fn_tb_mode(struct appletb_device *tb_dev)
> +{
> + switch (tb_dev->fn_mode) {
> + case APPLETB_FN_MODE_ESC:
> + return APPLETB_CMD_MODE_ESC;
> +
> + case APPLETB_FN_MODE_FKEYS:
> + return APPLETB_CMD_MODE_FN;
> +
> + case APPLETB_FN_MODE_SPCL:
> + return APPLETB_CMD_MODE_SPCL;
> +
> + case APPLETB_FN_MODE_INV:
> + return (tb_dev->last_fn_pressed) ? APPLETB_CMD_MODE_SPCL :
> + APPLETB_CMD_MODE_FN;
> +
> + case APPLETB_FN_MODE_NORM:
> + default:
> + return (tb_dev->last_fn_pressed) ? APPLETB_CMD_MODE_FN :
> + APPLETB_CMD_MODE_SPCL;
> + }
> +}
> +
> +/*
> + * Switch touch bar mode and display when mode or display not the desired ones.
> + */
> +static void appletb_update_touchbar_no_lock(struct appletb_device *tb_dev,
> + bool force)
> +{
> + unsigned char want_mode;
> + unsigned char want_disp;
> + bool need_update = false;
> +
> + /*
> + * Calculate the new modes:
> + * idle_timeout:
> + * -2 mode/disp off
> + * -1 mode on, disp on/dim
> + * 0 mode on, disp off
> + * >0 mode on, disp off after idle_timeout seconds
> + * dim_timeout (only valid if idle_timeout > 0 || idle_timeout == -1):
> + * -1 disp never dimmed
> + * 0 disp always dimmed
> + * >0 disp dim after dim_timeout seconds
> + */
> + if (tb_dev->idle_timeout == -2) {
> + want_mode = APPLETB_CMD_MODE_OFF;
> + want_disp = APPLETB_CMD_DISP_OFF;
> + } else {
> + want_mode = appletb_get_fn_tb_mode(tb_dev);
> + want_disp = tb_dev->idle_timeout == 0 ? APPLETB_CMD_DISP_OFF :
> + tb_dev->dim_timeout == 0 ? APPLETB_CMD_DISP_DIM :
> + APPLETB_CMD_DISP_ON;
> + }
> +
> + /*
> + * See if we need to update the touch bar, taking into account that we
> + * generally don't want to switch modes while a touch bar key is
> + * pressed.
> + */
> + if (appletb_get_cur_tb_mode(tb_dev) != want_mode &&
> + !appletb_any_tb_key_pressed(tb_dev)) {
> + tb_dev->pnd_tb_mode = want_mode;
> + need_update = true;
> + }
> +
> + if (appletb_get_cur_tb_disp(tb_dev) != want_disp &&
> + (!appletb_any_tb_key_pressed(tb_dev) ||
> + want_disp != APPLETB_CMD_DISP_OFF)) {
> + tb_dev->pnd_tb_disp = want_disp;
> + need_update = true;
> + }
> +
> + if (force)
> + need_update = true;
> +
> + /* schedule the update if desired */
> + dev_dbg_ratelimited(tb_dev->log_dev,
> + "update: need_update=%d, want_mode=%d, cur-mode=%d, want_disp=%d, cur-disp=%d\n",
> + need_update, want_mode, tb_dev->cur_tb_mode,
> + want_disp, tb_dev->cur_tb_disp);
> +
> + if (need_update) {
> + cancel_delayed_work(&tb_dev->tb_work);
> + appletb_schedule_tb_update(tb_dev, 0);
> + }
> +}
> +
> +static void appletb_update_touchbar(struct appletb_device *tb_dev, bool force)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (tb_dev->active)
> + appletb_update_touchbar_no_lock(tb_dev, force);
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +}
> +
> +static void appletb_set_idle_timeout(struct appletb_device *tb_dev, int new)
> +{
> + tb_dev->idle_timeout = new;
> +
> + if (tb_dev->dim_to_is_calc && tb_dev->idle_timeout > 0)
> + tb_dev->dim_timeout = new - min(APPLETB_MAX_DIM_TIME, new / 3);
> + else if (tb_dev->dim_to_is_calc)
> + tb_dev->dim_timeout = -1;
> +}
> +
> +static ssize_t idle_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", tb_dev->idle_timeout);
> +}
> +
> +static ssize_t idle_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> + long new;
> + int rc;
> +
> + rc = kstrtol(buf, 0, &new);
> + if (rc || new > INT_MAX || new < -2)
> + return -EINVAL;
> +
> + appletb_set_idle_timeout(tb_dev, new);
> + appletb_update_touchbar(tb_dev, true);
> +
> + return size;
> +}
> +
> +static void appletb_set_dim_timeout(struct appletb_device *tb_dev, int new)
> +{
> + if (new == -2) {
> + tb_dev->dim_to_is_calc = true;
> + appletb_set_idle_timeout(tb_dev, tb_dev->idle_timeout);
> + } else {
> + tb_dev->dim_to_is_calc = false;
> + tb_dev->dim_timeout = new;
> + }
> +}
> +
> +static ssize_t dim_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n",
> + tb_dev->dim_to_is_calc ? -2 : tb_dev->dim_timeout);
> +}
> +
> +static ssize_t dim_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> + long new;
> + int rc;
> +
> + rc = kstrtol(buf, 0, &new);
> + if (rc || new > INT_MAX || new < -2)
> + return -EINVAL;
> +
> + appletb_set_dim_timeout(tb_dev, new);
> + appletb_update_touchbar(tb_dev, true);
> +
> + return size;
> +}
> +
> +static ssize_t fnmode_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", tb_dev->fn_mode);
> +}
> +
> +static ssize_t fnmode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> + long new;
> + int rc;
> +
> + rc = kstrtol(buf, 0, &new);
> + if (rc || new > APPLETB_FN_MODE_MAX || new < 0)
> + return -EINVAL;
> +
> + tb_dev->fn_mode = new;
> + appletb_update_touchbar(tb_dev, false);
> +
> + return size;
> +}
> +
> +static int appletb_tb_key_to_slot(unsigned int code)
> +{
> + switch (code) {
> + case KEY_ESC:
> + return 0;
> + case KEY_F1:
> + case KEY_F2:
> + case KEY_F3:
> + case KEY_F4:
> + case KEY_F5:
> + case KEY_F6:
> + case KEY_F7:
> + case KEY_F8:
> + case KEY_F9:
> + case KEY_F10:
> + return code - KEY_F1 + 1;
> + case KEY_F11:
> + case KEY_F12:
> + return code - KEY_F11 + 11;
> + default:
> + return -1;
> + }
> +}
> +
> +static int appletb_hid_event(struct hid_device *hdev, struct hid_field *field,
> + struct hid_usage *usage, __s32 value)
> +{
> + struct appletb_device *tb_dev = hid_get_drvdata(hdev);
> + unsigned int new_code = 0;
> + unsigned long flags;
> + bool send_dummy = false;
> + bool send_trnsl = false;
> + int slot;
> + int rc = 0;
> +
> + if ((usage->hid & HID_USAGE_PAGE) != HID_UP_KEYBOARD ||
> + usage->type != EV_KEY)
> + return 0;
> +
> + /*
> + * Skip non-touch-bar keys.
> + *
> + * Either the touch bar itself or usbhid generate a slew of key-down
> + * events for all the meta keys. None of which we're at all interested
> + * in.
> + */
> + slot = appletb_tb_key_to_slot(usage->code);
> + if (slot < 0)
> + return 0;
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (!tb_dev->active) {
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> + return 0;
> + }
> +
> + new_code = appletb_fn_to_special(usage->code);
> +
> + if (value != 2)
> + tb_dev->last_tb_keys_pressed[slot] = value;
> +
> + tb_dev->last_event_time = ktime_get();
> +
> + appletb_update_touchbar_no_lock(tb_dev, false);
> +
> + /*
> + * We want to suppress touch bar keys while the touch bar is off, but
> + * we do want to wake up the screen if it's asleep, so generate a dummy
> + * event in that case.
> + */
> + if (tb_dev->cur_tb_mode == APPLETB_CMD_MODE_OFF ||
> + tb_dev->cur_tb_disp == APPLETB_CMD_DISP_OFF) {
> + send_dummy = true;
> + rc = 1;
> + /* translate special keys */
> + } else if (new_code &&
> + ((value > 0 &&
> + appletb_get_cur_tb_mode(tb_dev) == APPLETB_CMD_MODE_SPCL)
> + ||
> + (value == 0 && tb_dev->last_tb_keys_translated[slot]))) {
> + tb_dev->last_tb_keys_translated[slot] = true;
> + send_trnsl = true;
> + rc = 1;
> + /* everything else handled normally */
> + } else {
> + tb_dev->last_tb_keys_translated[slot] = false;
> + }
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + /*
> + * Need to send these input events outside of the lock, as otherwise
> + * we can run into the following deadlock:
> + * Task 1 Task 2
> + * appletb_hid_event() input_event()
> + * acquire tb_lock acquire dev->event_lock
> + * input_event() appletb_inp_event()
> + * acquire dev->event_lock acquire tb_lock
> + */
> + if (send_dummy) {
> + input_event(field->hidinput->input, EV_KEY, KEY_UNKNOWN, 1);
> + input_event(field->hidinput->input, EV_KEY, KEY_UNKNOWN, 0);
> + } else if (send_trnsl) {
> + input_event(field->hidinput->input, usage->type, new_code,
> + value);
> + }
> +
> + return rc;
> +}
> +
> +static void appletb_inp_event(struct input_handle *handle, unsigned int type,
> + unsigned int code, int value)
> +{
> + struct appletb_device *tb_dev = handle->private;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (!tb_dev->active) {
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> + return;
> + }
> +
> + if (type == EV_KEY && code == KEY_FN && value != 2)
> + tb_dev->last_fn_pressed = value;
> +
> + tb_dev->last_event_time = ktime_get();
> +
> + appletb_update_touchbar_no_lock(tb_dev, false);
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +}
> +
> +/* Find and save the usb-device associated with the touch bar input device */
> +static struct usb_interface *appletb_get_usb_iface(struct hid_device *hdev)
> +{
> + struct device *dev = &hdev->dev;
> +
> + while (dev && !(dev->type && dev->type->name &&
> + !strcmp(dev->type->name, "usb_interface")))
Check with is_usb_interface(dev)
> + dev = dev->parent;
> +
> + return dev ? to_usb_interface(dev) : NULL;
> +}
> +
> +static int appletb_inp_connect(struct input_handler *handler,
> + struct input_dev *dev,
> + const struct input_device_id *id)
> +{
> + struct appletb_device *tb_dev = handler->private;
> + struct input_handle *handle;
> + int rc;
> +
> + if (id->driver_info == APPLETB_DEVID_KEYBOARD) {
> + handle = &tb_dev->kbd_handle;
> + handle->name = "tbkbd";
> + } else if (id->driver_info == APPLETB_DEVID_TOUCHPAD) {
> + handle = &tb_dev->tpd_handle;
> + handle->name = "tbtpad";
> + } else {
> + dev_err(tb_dev->log_dev, "Unknown device id (%lu)\n",
> + id->driver_info);
> + return -ENOENT;
> + }
> +
> + if (handle->dev) {
> + dev_err(tb_dev->log_dev,
> + "Duplicate connect to %s input device\n", handle->name);
> + return -EEXIST;
> + }
> +
> + handle->open = 0;
> + handle->dev = input_get_device(dev);
> + handle->handler = handler;
> + handle->private = tb_dev;
> +
> + rc = input_register_handle(handle);
> + if (rc)
> + goto err_free_dev;
> +
> + rc = input_open_device(handle);
> + if (rc)
> + goto err_unregister_handle;
> +
> + dev_dbg(tb_dev->log_dev, "Connected to %s input device\n",
> + handle == &tb_dev->kbd_handle ? "keyboard" : "touchpad");
> +
> + return 0;
> +
> + err_unregister_handle:
> + input_unregister_handle(handle);
> + err_free_dev:
> + input_put_device(handle->dev);
> + handle->dev = NULL;
> + return rc;
> +}
> +
> +static void appletb_inp_disconnect(struct input_handle *handle)
> +{
> + struct appletb_device *tb_dev = handle->private;
> +
> + input_close_device(handle);
> + input_unregister_handle(handle);
> +
> + dev_dbg(tb_dev->log_dev, "Disconnected from %s input device\n",
> + handle == &tb_dev->kbd_handle ? "keyboard" : "touchpad");
> +
> + input_put_device(handle->dev);
> + handle->dev = NULL;
> +}
> +
> +static int appletb_input_configured(struct hid_device *hdev,
> + struct hid_input *hidinput)
> +{
> + int idx;
> + struct input_dev *input = hidinput->input;
> +
> + /*
> + * Clear various input capabilities that are blindly set by the hid
> + * driver (usbkbd.c)
> + */
> + memset(input->evbit, 0, sizeof(input->evbit));
> + memset(input->keybit, 0, sizeof(input->keybit));
> + memset(input->ledbit, 0, sizeof(input->ledbit));
> +
> + /* set our actual capabilities */
> + __set_bit(EV_KEY, input->evbit);
> + __set_bit(EV_REP, input->evbit);
> + __set_bit(EV_MSC, input->evbit); /* hid-input generates MSC_SCAN */
> +
> + for (idx = 0; idx < ARRAY_SIZE(appletb_fn_codes); idx++) {
> + input_set_capability(input, EV_KEY, appletb_fn_codes[idx].from);
> + input_set_capability(input, EV_KEY, appletb_fn_codes[idx].to);
> + }
> +
> + input_set_capability(input, EV_KEY, KEY_ESC);
> + input_set_capability(input, EV_KEY, KEY_UNKNOWN);
> +
> + return 0;
> +}
> +
> +static struct appletb_iface_info *
> +appletb_get_iface_info(struct appletb_device *tb_dev, struct hid_device *hdev)
> +{
> + if (hdev == tb_dev->mode_iface.hdev)
> + return &tb_dev->mode_iface;
> + if (hdev == tb_dev->disp_iface.hdev)
> + return &tb_dev->disp_iface;
> + return NULL;
> +}
> +
> +/**
> + * appletb_find_report_field() - Find the field in the report with the given
> + * usage.
> + * @report: the report to search
> + * @field_usage: the usage of the field to search for
> + *
> + * Returns: the hid field if found, or NULL if none found.
> + */
> +static struct hid_field *appletb_find_report_field(struct hid_report *report,
> + unsigned int field_usage)
> +{
> + int f, u;
> +
> + for (f = 0; f < report->maxfield; f++) {
> + struct hid_field *field = report->field[f];
> +
> + if (field->logical == field_usage)
> + return field;
> +
> + for (u = 0; u < field->maxusage; u++) {
> + if (field->usage[u].hid == field_usage)
> + return field;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * appletb_find_hid_field() - Search all the reports of the device for the
> + * field with the given usage.
> + * @hdev: the device whose reports to search
> + * @application: the usage of application collection that the field must
> + * belong to
> + * @field_usage: the usage of the field to search for
> + *
> + * Returns: the hid field if found, or NULL if none found.
> + */
> +static struct hid_field *appletb_find_hid_field(struct hid_device *hdev,
> + unsigned int application,
> + unsigned int field_usage)
> +{
> + static const int report_types[] = { HID_INPUT_REPORT, HID_OUTPUT_REPORT,
> + HID_FEATURE_REPORT };
> + struct hid_report *report;
> + struct hid_field *field;
> + int t;
> +
> + for (t = 0; t < ARRAY_SIZE(report_types); t++) {
> + struct list_head *report_list =
> + &hdev->report_enum[report_types[t]].report_list;
> + list_for_each_entry(report, report_list, list) {
> + if (report->application != application)
> + continue;
> +
> + field = appletb_find_report_field(report, field_usage);
> + if (field)
> + return field;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static int appletb_extract_report_and_iface_info(struct appletb_device *tb_dev,
> + struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appletb_iface_info *iface_info;
> + struct usb_interface *usb_iface;
> + struct hid_field *field;
> +
> + field = appletb_find_hid_field(hdev, HID_GD_KEYBOARD, HID_USAGE_MODE);
> + if (field) {
> + iface_info = &tb_dev->mode_iface;
> + tb_dev->mode_field = field;
> + tb_dev->is_t1 = !!(id->driver_data & APPLETB_FEATURE_IS_T1);
> + } else {
> + field = appletb_find_hid_field(hdev, HID_USAGE_APPLE_APP,
> + HID_USAGE_DISP);
> + if (!field)
> + return 0;
> +
> + iface_info = &tb_dev->disp_iface;
> + tb_dev->disp_field = field;
> + tb_dev->disp_field_aux1 =
> + appletb_find_hid_field(hdev, HID_USAGE_APPLE_APP,
> + HID_USAGE_DISP_AUX1);
> +
> + if (!tb_dev->disp_field_aux1 ||
> + tb_dev->disp_field_aux1->report !=
> + tb_dev->disp_field->report) {
> + dev_err(tb_dev->log_dev,
> + "Unexpected report structure for report %u in device %s\n",
> + tb_dev->disp_field->report->id,
> + dev_name(&hdev->dev));
> + return -ENODEV;
> + }
> + }
> +
> + usb_iface = appletb_get_usb_iface(hdev);
> + if (!usb_iface) {
> + dev_err(tb_dev->log_dev,
> + "Failed to find usb interface for hid device %s\n",
> + dev_name(&hdev->dev));
> + return -ENODEV;
> + }
> +
> + iface_info->hdev = hdev;
> + iface_info->usb_iface = usb_get_intf(usb_iface);
> + iface_info->suspended = false;
> +
> + return 1;
> +}
> +
> +static void appletb_clear_iface_info(struct appletb_device *tb_dev,
> + struct hid_device *hdev)
> +{
> + struct appletb_iface_info *iface_info;
> +
> + iface_info = appletb_get_iface_info(tb_dev, hdev);
> + if (iface_info) {
> + usb_put_intf(iface_info->usb_iface);
> + iface_info->usb_iface = NULL;
> + iface_info->hdev = NULL;
> + }
> +}
> +
> +static bool appletb_test_and_mark_active(struct appletb_device *tb_dev)
> +{
> + unsigned long flags;
> + bool activated = false;
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (tb_dev->mode_iface.hdev && tb_dev->disp_iface.hdev &&
> + !tb_dev->active) {
> + tb_dev->active = true;
> + activated = true;
> + }
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + return activated;
> +}
> +
> +static bool appletb_test_and_mark_inactive(struct appletb_device *tb_dev,
> + struct hid_device *hdev)
> +{
> + unsigned long flags;
> + bool deactivated = false;
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (tb_dev->mode_iface.hdev && tb_dev->disp_iface.hdev &&
> + tb_dev->active &&
> + (hdev == tb_dev->mode_iface.hdev ||
> + hdev == tb_dev->disp_iface.hdev)) {
> + tb_dev->active = false;
> + deactivated = true;
> + }
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + return deactivated;
> +}
> +
> +static const struct input_device_id appletb_input_devices[] = {
> + {
> + .flags = INPUT_DEVICE_ID_MATCH_BUS |
> + INPUT_DEVICE_ID_MATCH_KEYBIT,
> + .bustype = BUS_SPI,
> + .keybit = { [BIT_WORD(KEY_FN)] = BIT_MASK(KEY_FN) },
> + .driver_info = APPLETB_DEVID_KEYBOARD,
> + }, /* Builtin SPI keyboard device */
> + {
> + .flags = INPUT_DEVICE_ID_MATCH_BUS |
> + INPUT_DEVICE_ID_MATCH_KEYBIT,
> + .bustype = BUS_SPI,
> + .keybit = { [BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH) },
> + .driver_info = APPLETB_DEVID_TOUCHPAD,
> + }, /* Builtin SPI touchpad device */
> + {
> + .flags = INPUT_DEVICE_ID_MATCH_BUS |
> + INPUT_DEVICE_ID_MATCH_VENDOR |
> + INPUT_DEVICE_ID_MATCH_KEYBIT,
> + .bustype = BUS_USB,
> + .vendor = 0x05ac /* USB_VENDOR_ID_APPLE */,
> + .keybit = { [BIT_WORD(KEY_FN)] = BIT_MASK(KEY_FN) },
> + .driver_info = APPLETB_DEVID_KEYBOARD,
> + }, /* Builtin USB keyboard device */
> + {
> + .flags = INPUT_DEVICE_ID_MATCH_BUS |
> + INPUT_DEVICE_ID_MATCH_VENDOR |
> + INPUT_DEVICE_ID_MATCH_KEYBIT,
> + .bustype = BUS_USB,
> + .vendor = 0x05ac /* USB_VENDOR_ID_APPLE */,
> + .keybit = { [BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH) },
> + .driver_info = APPLETB_DEVID_TOUCHPAD,
> + }, /* Builtin USB touchpad device */
> + { }, /* Terminating zero entry */
> +};
> +
> +static bool appletb_match_internal_device(struct input_handler *handler,
> + struct input_dev *inp_dev)
> +{
> + struct device *dev = &inp_dev->dev;
> +
> + if (inp_dev->id.bustype == BUS_SPI)
> + return true;
> +
> + /* in kernel: dev && !is_usb_device(dev) */
> + while (dev && !(dev->type && dev->type->name &&
> + !strcmp(dev->type->name, "usb_device")))
> + dev = dev->parent;
> +
> + /*
> + * Apple labels all their internal keyboards and trackpads as such,
> + * instead of maintaining an ever expanding list of product-id's we
> + * just look at the device's product name.
> + */
> + if (dev)
> + return !!strstr(to_usb_device(dev)->product, "Internal Keyboard");
> +
> + return false;
> +}
> +
> +static int appletb_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appletb_device *tb_dev = appletb_dev;
> + unsigned long flags;
> + int rc;
> +
> + /* initialize the report info */
> + rc = hid_parse(hdev);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hid parse failed (%d)\n", rc);
> + goto error;
> + }
> +
> + /* Ensure this usb endpoint is for the touchbar backlight, not keyboard
> + * backlight.
> + */
> + if ((hdev->product == USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) &&
> + !(hdev->collection && hdev->collection[0].usage ==
> + HID_USAGE_APPLE_APP)) {
> + return -ENODEV;
> + }
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (!tb_dev->log_dev)
> + tb_dev->log_dev = &hdev->dev;
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + hid_set_drvdata(hdev, tb_dev);
> +
> + rc = appletb_extract_report_and_iface_info(tb_dev, hdev, id);
> + if (rc < 0)
> + goto error;
> +
> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER | HID_CONNECT_HIDINPUT);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hw start failed (%d)\n", rc);
> + goto clear_iface_info;
> + }
> +
> + rc = hid_hw_open(hdev);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hw open failed (%d)\n", rc);
> + goto stop_hid;
> + }
> +
> + /* do setup if we have both interfaces */
> + if (appletb_test_and_mark_active(tb_dev)) {
> + /* initialize the touch bar */
> + if (appletb_tb_def_fn_mode >= 0 &&
> + appletb_tb_def_fn_mode <= APPLETB_FN_MODE_MAX)
> + tb_dev->fn_mode = appletb_tb_def_fn_mode;
> + else
> + tb_dev->fn_mode = APPLETB_FN_MODE_NORM;
> + appletb_set_idle_timeout(tb_dev, appletb_tb_def_idle_timeout);
> + appletb_set_dim_timeout(tb_dev, appletb_tb_def_dim_timeout);
> + tb_dev->last_event_time = ktime_get();
> +
> + tb_dev->pnd_tb_mode = APPLETB_CMD_MODE_UPD;
> + tb_dev->pnd_tb_disp = APPLETB_CMD_DISP_UPD;
> +
> + appletb_update_touchbar(tb_dev, false);
> +
> + /* set up the input handler */
> + tb_dev->inp_handler.event = appletb_inp_event;
> + tb_dev->inp_handler.connect = appletb_inp_connect;
> + tb_dev->inp_handler.disconnect = appletb_inp_disconnect;
> + tb_dev->inp_handler.name = "appletb";
> + tb_dev->inp_handler.id_table = appletb_input_devices;
> + tb_dev->inp_handler.match = appletb_match_internal_device;
> + tb_dev->inp_handler.private = tb_dev;
> +
> + rc = input_register_handler(&tb_dev->inp_handler);
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Unable to register keyboard handler (%d)\n",
> + rc);
> + goto mark_inactive;
> + }
> +
> + /* initialize sysfs attributes */
> + rc = sysfs_create_group(&tb_dev->mode_iface.hdev->dev.kobj,
> + &appletb_attr_group);
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Failed to create sysfs attributes (%d)\n", rc);
> + goto unreg_handler;
> + }
> +
> + dev_dbg(tb_dev->log_dev, "Touchbar activated\n");
> + }
> +
> + return 0;
> +
> +unreg_handler:
> + input_unregister_handler(&tb_dev->inp_handler);
> +mark_inactive:
> + appletb_test_and_mark_inactive(tb_dev, hdev);
> + cancel_delayed_work_sync(&tb_dev->tb_work);
> + hid_hw_close(hdev);
> +stop_hid:
> + hid_hw_stop(hdev);
> +clear_iface_info:
> + appletb_clear_iface_info(tb_dev, hdev);
> +error:
> + return rc;
> +}
> +
> +static void appletb_remove(struct hid_device *hdev)
> +{
> + struct appletb_device *tb_dev = hid_get_drvdata(hdev);
> + unsigned long flags;
> +
> + if (appletb_test_and_mark_inactive(tb_dev, hdev)) {
> + sysfs_remove_group(&tb_dev->mode_iface.hdev->dev.kobj,
> + &appletb_attr_group);
> +
> + input_unregister_handler(&tb_dev->inp_handler);
> +
> + cancel_delayed_work_sync(&tb_dev->tb_work);
> + appletb_set_tb_mode(tb_dev, APPLETB_CMD_MODE_OFF);
> + appletb_set_tb_disp(tb_dev, APPLETB_CMD_DISP_ON);
> +
> + if (tb_dev->tb_autopm_off)
> + hid_hw_power(tb_dev->disp_iface.hdev, PM_HINT_NORMAL);
> +
> + dev_info(tb_dev->log_dev, "Touchbar deactivated\n");
> + }
> +
> + hid_hw_close(hdev);
> + hid_hw_stop(hdev);
> + appletb_clear_iface_info(tb_dev, hdev);
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (tb_dev->log_dev == &hdev->dev) {
> + if (tb_dev->mode_iface.hdev)
> + tb_dev->log_dev = &tb_dev->mode_iface.hdev->dev;
> + else if (tb_dev->disp_iface.hdev)
> + tb_dev->log_dev = &tb_dev->disp_iface.hdev->dev;
> + else
> + tb_dev->log_dev = NULL;
> + }
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +}
> +
> +#ifdef CONFIG_PM
> +static int appletb_suspend(struct hid_device *hdev, pm_message_t message)
> +{
> + struct appletb_device *tb_dev = hid_get_drvdata(hdev);
> + struct appletb_iface_info *iface_info;
> + unsigned long flags;
> + bool all_suspended = false;
> +
> + if (message.event != PM_EVENT_SUSPEND &&
> + message.event != PM_EVENT_FREEZE)
> + return 0;
> +
> + if (tb_dev->is_t1) {
> +
> + /*
> + * Wait for both interfaces to be suspended and no more async work
> + * in progress.
> + */
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + if (!tb_dev->mode_iface.suspended && !tb_dev->disp_iface.suspended) {
> + tb_dev->active = false;
> + cancel_delayed_work(&tb_dev->tb_work);
> + }
> +
> + iface_info = appletb_get_iface_info(tb_dev, hdev);
> + if (iface_info)
> + iface_info->suspended = true;
> +
> + if ((!tb_dev->mode_iface.hdev || tb_dev->mode_iface.suspended) &&
> + (!tb_dev->disp_iface.hdev || tb_dev->disp_iface.suspended))
> + all_suspended = true;
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + flush_delayed_work(&tb_dev->tb_work);
> +
> + if (!all_suspended)
> + return 0;
> +
> + /*
> + * The touch bar device itself remembers the last state when suspended
> + * in some cases, but in others (e.g. when mode != off and disp == off)
> + * it resumes with a different state; furthermore it may be only
> + * partially responsive in that state. By turning both mode and disp
> + * off we ensure it is in a good state when resuming (and this happens
> + * to be the same state after booting/resuming-from-hibernate, so less
> + * special casing between the two).
> + */
> + if (message.event == PM_EVENT_SUSPEND) {
> + appletb_set_tb_mode(tb_dev, APPLETB_CMD_MODE_OFF);
> + appletb_set_tb_disp(tb_dev, APPLETB_CMD_DISP_OFF);
> + }
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + tb_dev->cur_tb_mode = APPLETB_CMD_MODE_OFF;
> + tb_dev->cur_tb_disp = APPLETB_CMD_DISP_OFF;
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + dev_info(tb_dev->log_dev, "Touchbar suspended.\n");
> + } else {
> + dev_info(tb_dev->log_dev, "T2 Mac detected, not handling suspend.\n");
> + }
> +
> + return 0;
> +}
> +
> +static int appletb_reset_resume(struct hid_device *hdev)
> +{
> + struct appletb_device *tb_dev = hid_get_drvdata(hdev);
> + struct appletb_iface_info *iface_info;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tb_dev->tb_lock, flags);
> +
> + iface_info = appletb_get_iface_info(tb_dev, hdev);
> + if (iface_info)
> + iface_info->suspended = false;
> +
> + if ((tb_dev->mode_iface.hdev && !tb_dev->mode_iface.suspended) &&
> + (tb_dev->disp_iface.hdev && !tb_dev->disp_iface.suspended)) {
> + /*
> + * Restore touch bar state. Note that autopm state is not
> + * preserved, so need explicitly restore that here.
> + */
> + tb_dev->active = true;
> + tb_dev->restore_autopm = true;
> + tb_dev->last_event_time = ktime_get();
> +
> + appletb_update_touchbar_no_lock(tb_dev, true);
> +
> + dev_info(tb_dev->log_dev, "Touchbar resumed.\n");
> + }
> +
> + spin_unlock_irqrestore(&tb_dev->tb_lock, flags);
> +
> + return 0;
> +}
> +#endif
> +
> +static struct appletb_device *appletb_alloc_device(void)
> +{
> + struct appletb_device *tb_dev;
> +
> + tb_dev = kzalloc(sizeof(*tb_dev), GFP_KERNEL);
> + if (!tb_dev)
> + return NULL;
> +
> + spin_lock_init(&tb_dev->tb_lock);
> + INIT_DELAYED_WORK(&tb_dev->tb_work, appletb_set_tb_worker);
> +
> + return tb_dev;
> +}
> +
> +static void appletb_free_device(struct appletb_device *tb_dev)
> +{
> + cancel_delayed_work_sync(&tb_dev->tb_work);
> + kfree(tb_dev);
> +}
> +
> +static const struct hid_device_id appletb_hid_ids[] = {
> + /* MacBook Pro's 2016, 2017, with T1 chip */
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_TB),
> + .driver_data = APPLETB_FEATURE_IS_T1 },
> + /* MacBook Pro's 2018, 2019, with T2 chip: iBridge DFR brightness */
> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
> + USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
> + /* MacBook Pro's 2018, 2019, with T2 chip: iBridge Display */
> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
> + USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
> + { },
Also no comma here
> +};
> +
> +MODULE_DEVICE_TABLE(hid, appletb_hid_ids);
> +
> +static struct hid_driver appletb_hid_driver = {
> + .name = "apple-touchbar",
> + .id_table = appletb_hid_ids,
> + .probe = appletb_probe,
> + .remove = appletb_remove,
> + .event = appletb_hid_event,
> + .input_configured = appletb_input_configured,
> +#ifdef CONFIG_PM
> + .suspend = appletb_suspend,
> + .reset_resume = appletb_reset_resume,
> +#endif
> +};
> +
> +static int __init appletb_init(void)
> +{
> + struct appletb_device *tb_dev;
> + int rc;
> +
> + tb_dev = appletb_alloc_device();
> + if (!tb_dev)
> + return -ENOMEM;
> +
> + appletb_dev = tb_dev;
> +
> + rc = hid_register_driver(&appletb_hid_driver);
> + if (rc)
> + goto error;
> +
> + return 0;
> +
> +error:
> + appletb_free_device(tb_dev);
> + return rc;
> +}
> +
> +static void __exit appletb_exit(void)
> +{
> + hid_unregister_driver(&appletb_hid_driver);
> + appletb_free_device(appletb_dev);
> +}
You can remove the need for the "static struct appletb_device
*appletb_dev" by allocating it from the HID drivers probe function and
freeing it from the remove function.
Then the whole setup can be removed and replaced by
hid_module_driver(&appletb_hid_driver)
> +module_init(appletb_init);
> +module_exit(appletb_exit);
> +
> +MODULE_AUTHOR("Ronald Tschalär");
> +MODULE_DESCRIPTION("MacBookPro Touch Bar driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index c03535c4b..e620190b5 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -316,12 +316,14 @@ static const struct hid_device_id hid_have_special_driver[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
> { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_2021) },
> { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_FINGERPRINT_2021) },
> - { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
> - { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
> #endif
> #if IS_ENABLED(CONFIG_HID_APPLE_IBRIDGE)
> { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
> #endif
> +#if IS_ENABLED(CONFIG_HID_APPLE_TOUCHBAR)
> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
> +#endif
> #if IS_ENABLED(CONFIG_HID_APPLEIR)
> { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL) },
> { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL2) },
> --
> 2.37.2
>
^ permalink raw reply
* Re: [PATCH 3/3] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards
From: Thomas Weißschuh @ 2023-02-10 16:25 UTC (permalink / raw)
To: Aditya Garg
Cc: Jiri Kosina, jkosina@suse.cz, benjamin.tissoires@redhat.com,
Andy Shevchenko, andy.shevchenko@gmail.com, LKML,
linux-input@vger.kernel.org, ronald@innovation.ch,
kekrby@gmail.com, Orlando Chamberlain
In-Reply-To: <7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com>
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
>
^ permalink raw reply
* [PATCH] HID: cp2112: Fix driver not registering GPIO IRQ chip as threaded
From: Danny Kaehn @ 2023-02-10 17:00 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, ethan.twardy
The CP2112 generates interrupts from a polling routine on a thread,
and can only support threaded interrupts. This patch configures the
gpiochip irq chip with this flag, disallowing consumers to request
a hard IRQ from this driver, which resulted in a segfault previously.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
---
Note -- this patch was first submitted with the patchset:
"DeviceTree Support for USB-HID Devices and CP2112"
It is now submitted separately upon request, as it is not coupled to the
other patches in that series.
drivers/hid/hid-cp2112.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 1e16b0fa310d..27cadadda7c9 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1354,6 +1354,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
girq->parents = NULL;
girq->default_type = IRQ_TYPE_NONE;
girq->handler = handle_simple_irq;
+ girq->threaded = true;
ret = gpiochip_add_data(&dev->gc, dev);
if (ret < 0) {
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.
From: Thomas Weißschuh @ 2023-02-10 18:36 UTC (permalink / raw)
To: Aditya Garg
Cc: Jiri Kosina, jkosina, benjamin.tissoires, Andy Shevchenko,
andy.shevchenko, LKML, linux-input, ronald, kekrby,
Orlando Chamberlain
In-Reply-To: <BM1PR01MB09315DC70770D236DD29D007B8DE9@BM1PR01MB0931.INDPRD01.PROD.OUTLOOK.COM>
Feb 10, 2023 08:50:12 Aditya Garg <gargaditya08@live.com>:
>
>
>> On 10-Feb-2023, at 9:04 PM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
>>
>> Responses inline
>>
>> On Fri, Feb 10, 2023 at 12:05:13PM +0000, Aditya Garg wrote:
>>>>> On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@t-8ch.de> wrote:
>>>>
>>>> Hi,
>>>>
>>>> some comments inline.
>>>>
>>>>> On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
>>>>
>>>>> +
>>>>> +static struct {
>>>>> + unsigned int usage;
>>>>> + struct hid_device_id *dev_id;
>>>>> +} appleib_usage_map[] = {
>>>>> + /* Default iBridge configuration, key inputs and mode settings */
>>>>> + { 0x00010006, &appleib_sub_hid_ids[0] },
>>>>> + /* OS X iBridge configuration, digitizer inputs */
>>>>> + { 0x000D0005, &appleib_sub_hid_ids[0] },
>>>>> + /* All iBridge configurations, display/DFR settings */
>>>>> + { 0xFF120001, &appleib_sub_hid_ids[0] },
>>>>> + /* All iBridge configurations, ALS */
>>>>> + { 0x00200041, &appleib_sub_hid_ids[1] },
>>>>> +};
>>>>
>>>> const
>>>>
>>>
>>> Constantifying this results in compiler giving warnings
>>>
>>> drivers/hid/apple-ibridge.c:78:23: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>> 78 | { 0x00200041, &appleib_sub_hid_ids[1] },
>>
>> For this you also have to constify the hid_device_id *dev_id in
>> appleib_usage_map. And then propagate this change to some functions and
>> variables.
>>
>>> | ^
>>> drivers/hid/apple-ibridge.c: In function 'appleib_add_sub_dev':
>>> drivers/hid/apple-ibridge.c:363:29: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>> 363 | sub_hdev->ll_driver = &appleib_ll_driver;
>>
>> As Benjamin said this is because your changes are based on Linus' tree
>> but they will break as soon as they will be merged into the HID tree.
>> You should base your changes off of the HID tree:
>> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core
>>
>> This issue is essentially unlucky timing.
>>
>>> | ^
>>> drivers/hid/apple-ibridge.c: In function 'appleib_hid_probe':
>>> drivers/hid/apple-ibridge.c:436:12: error: expected '(' before 'hid_is_usb'
>>> 436 | if hid_is_usb(hdev)
>>> | ^~~~~~~~~~
>>> | (
>>
>> As the error message indicates, this is invalid syntax and missing a
>> '('.
>> What you want to do is to check for
>>
>> if (!hid_is_usb(hdev))
>> return -ENODEV;
>
> It was a typo on my part
>
> + /* check and set usb config first */
> + if (hid_is_usb(hdev))
> + udev = hid_to_usb_dev(hdev);
> + else
> + return -EINVAL;
I would prefer
if (!hid_is_usb(hdev))
return -EINVAL;
udev = hid_to_usb_dev(hdev);
>
> This is what I have in my patch set now.
>
> If there is something wrong with this, then do tell me
>
> Thanks
>>
>> *before* calling hid_to_usb_dev(hdev);
>>
>>> In file included from drivers/hid/apple-ibridge.c:48:
>>> drivers/hid/apple-ibridge.c: In function 'appleib_probe':
>>> drivers/hid/apple-ibridge.c:544:35: warning: passing argument 1 of '__hid_register_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>> 544 | ret = hid_register_driver(&appleib_hid_driver);
>>> | ^~~~~~~~~~~~~~~~~~~
>>> ./include/linux/hid.h:898:31: note: in definition of macro 'hid_register_driver'
>>> 898 | __hid_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
>>> | ^~~~~~
>>> ./include/linux/hid.h:893:47: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>>> 893 | extern int __must_check __hid_register_driver(struct hid_driver *,
>>> | ^~~~~~~~~~~~~~~~~~~
>>> drivers/hid/apple-ibridge.c: In function 'appleib_remove':
>>> drivers/hid/apple-ibridge.c:558:31: warning: passing argument 1 of 'hid_unregister_driver' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>>> 558 | hid_unregister_driver(&appleib_hid_driver);
>>> | ^~~~~~~~~~~~~~~~~~~
>>> ./include/linux/hid.h:900:35: note: expected 'struct hid_driver *' but argument is of type 'const struct hid_driver *'
>>> 900 | extern void hid_unregister_driver(struct hid_driver *);
>>> | ^~~~~~~~~~~~~~~~~~~
>>
>> These are all because applib_hid_driver can not be const.
>> Sorry for the wrong advice.
>>
>> Benjamin:
>> HID drivers can not be const because they embed a 'struct driver' that
>> is needed by the driver core to be mutable.
>> Fixing this is probably a larger enterprise.
>>
>>> make[6]: *** [scripts/Makefile.build:250: drivers/hid/apple-ibridge.o] Error 1
>>> make[5]: *** [scripts/Makefile.build:500: drivers/hid] Error 2
>>> make[5]: *** Waiting for unfinished jobs….
>>>
>>> Some warnings are also due to a typo in if and constantifying `static struct hid_driver`, although they probably can
>>> be fixed.
>>>
>>> In short, Thomas, do you really want me to constantify the structure I
>>> am talking about in this email, as well `static struct hid_driver`?
>>
>> struct hid_driver: Don't constify
>> all others: Do constify
>>
>> Thomas
^ permalink raw reply
* Re: [PATCH 1/3] Input: xpad - Treat Qanba controllers as Xbox360 controllers
From: Lyude Paul @ 2023-02-10 21:45 UTC (permalink / raw)
To: Vicki Pfau, Jiri Kosina, Benjamin Tissoires, linux-input
Cc: Pierre-Loup A. Griffais, Cristian Ciocaltea
In-Reply-To: <20230203022758.3982393-1-vi@endrift.com>
LGTM. For onlookers curious: the audio interface being disabled here isn't
currently supported in xpad quite yet, but is supported out of tree -
supposedly with plans to eventually merge it into the mainline kernel. So
patch 2/3 here should be fine.
Anyway,
Reviewed-by: Lyude Paul <lyude@redhat.com>
On Thu, 2023-02-02 at 18:27 -0800, Vicki Pfau wrote:
> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>
> They act that way in PC mode.
>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> drivers/input/joystick/xpad.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 2959d80f7fdb..bb65c8268963 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -492,6 +492,7 @@ static const struct usb_device_id xpad_table[] = {
> XPAD_XBOXONE_VENDOR(0x24c6), /* PowerA Controllers */
> XPAD_XBOX360_VENDOR(0x2563), /* OneXPlayer Gamepad */
> XPAD_XBOX360_VENDOR(0x260d), /* Dareu H101 */
> + XPAD_XBOX360_VENDOR(0x2c22), /* Qanba Controllers */
> XPAD_XBOXONE_VENDOR(0x2dc8), /* 8BitDo Pro 2 Wired Controller for Xbox */
> XPAD_XBOXONE_VENDOR(0x2e24), /* Hyperkin Duke X-Box One pad */
> XPAD_XBOX360_VENDOR(0x2f24), /* GameSir Controllers */
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
^ permalink raw reply
* Re: [PATCH 1/3] Input: xpad - Treat Qanba controllers as Xbox360 controllers
From: Lyude Paul @ 2023-02-10 21:46 UTC (permalink / raw)
To: Vicki Pfau, Jiri Kosina, Benjamin Tissoires, linux-input
Cc: Pierre-Loup A. Griffais, Cristian Ciocaltea
In-Reply-To: <97e640ce45cc116b9877efda43117640fd323855.camel@redhat.com>
Almost forgot to mention - the Reviewed-by here is for the whole series, not
just the first patch :)
On Fri, 2023-02-10 at 16:45 -0500, Lyude Paul wrote:
> LGTM. For onlookers curious: the audio interface being disabled here isn't
> currently supported in xpad quite yet, but is supported out of tree -
> supposedly with plans to eventually merge it into the mainline kernel. So
> patch 2/3 here should be fine.
>
> Anyway,
>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
>
> On Thu, 2023-02-02 at 18:27 -0800, Vicki Pfau wrote:
> > From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> >
> > They act that way in PC mode.
> >
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > ---
> > drivers/input/joystick/xpad.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> > index 2959d80f7fdb..bb65c8268963 100644
> > --- a/drivers/input/joystick/xpad.c
> > +++ b/drivers/input/joystick/xpad.c
> > @@ -492,6 +492,7 @@ static const struct usb_device_id xpad_table[] = {
> > XPAD_XBOXONE_VENDOR(0x24c6), /* PowerA Controllers */
> > XPAD_XBOX360_VENDOR(0x2563), /* OneXPlayer Gamepad */
> > XPAD_XBOX360_VENDOR(0x260d), /* Dareu H101 */
> > + XPAD_XBOX360_VENDOR(0x2c22), /* Qanba Controllers */
> > XPAD_XBOXONE_VENDOR(0x2dc8), /* 8BitDo Pro 2 Wired Controller for Xbox */
> > XPAD_XBOXONE_VENDOR(0x2e24), /* Hyperkin Duke X-Box One pad */
> > XPAD_XBOX360_VENDOR(0x2f24), /* GameSir Controllers */
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
^ permalink raw reply
* [PATCH v5 0/3] Firmware Support for USB-HID Devices and CP2112
From: Danny Kaehn @ 2023-02-10 22:36 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, jikos, benjamin.tissoires
Cc: bartosz.golaszewski, andriy.shevchenko, dmitry.torokhov,
devicetree, linux-input, ethan.twardy
This patchset allows USB-HID devices to have DeviceTree bindings through sharing
the USB fwnode with the HID driver, and adds such a binding and driver
implementation for the CP2112 USB to SMBus Bridge (which necessitated the
USB-HID change). This change allows a CP2112 permanently attached in hardware to
be described in firmware and interoperate with other drivers.
Changes in v5:
- Use fwnode API instead of of_node api in hid-core.c and hid-cp2112.c
- Include sda-gpios and scl-gpios in silabs,cp2112.yaml
- Additional fixups to silabs,cp2112.yaml to address comments
- Submit threaded interrupt bugfix separately from this patchset, as requested
Changes in v4:
- Moved silabs,cp2112.yaml to /Documentation/devicetree/bindings/i2c
Changes in v3:
- Additional fixups to silabs,cp2112.yaml to address comments
Changes in v2:
- Added more detail to silabs,cp2112.yaml dt-binding
- Moved silabs,cp2112.yaml to /Documentation/devicetree/bindings/input
- Added support for setting smbus clock-frequency from DT in hid-cp2112.c
- Added freeing of of_nodes on error paths of _probe in hid-cp2112.c
Danny Kaehn (3):
dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
HID: usbhid: Share USB device firmware node with child HID device
HID: cp2112: Fwnode Support
.../bindings/i2c/silabs,cp2112.yaml | 113 ++++++++++++++++++
drivers/hid/hid-cp2112.c | 6 +
drivers/hid/usbhid/hid-core.c | 2 +
3 files changed, 121 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
--
2.25.1
^ permalink raw reply
* [PATCH v5 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
From: Danny Kaehn @ 2023-02-10 22:36 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, jikos, benjamin.tissoires
Cc: bartosz.golaszewski, andriy.shevchenko, dmitry.torokhov,
devicetree, linux-input, ethan.twardy
In-Reply-To: <20230210223638.12796-1-kaehndan@gmail.com>
This is a USB HID device which includes an I2C controller and 8 GPIO pins.
The binding allows describing the chip's gpio and i2c controller in DT
using the subnodes named "gpio" and "i2c", respectively. This is
intended to be used in configurations where the CP2112 is permanently
connected in hardware.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
---
.../bindings/i2c/silabs,cp2112.yaml | 113 ++++++++++++++++++
1 file changed, 113 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
diff --git a/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
new file mode 100644
index 000000000000..81eda01ebda5
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
@@ -0,0 +1,113 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/silabs,cp2112.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CP2112 HID USB to SMBus/I2C Bridge
+
+maintainers:
+ - Danny Kaehn <kaehndan@gmail.com>
+
+description:
+ The CP2112 is a USB HID device which includes an integrated I2C controller
+ and 8 GPIO pins. Its GPIO pins can each be configured as inputs, open-drain
+ outputs, or push-pull outputs.
+
+properties:
+ compatible:
+ const: usb10c4,ea90
+
+ reg:
+ maxItems: 1
+ description: The USB port number on the host controller
+
+ i2c:
+ description: The SMBus/I2C controller node for the CP2112
+ $ref: /schemas/i2c/i2c-controller.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ sda-gpios:
+ maxItems: 1
+
+ scl-gpios:
+ maxItems: 1
+
+ clock-frequency:
+ minimum: 10000
+ default: 100000
+ maximum: 400000
+
+ gpio:
+ description: The GPIO controller node for the CP2112
+ type: object
+ unevaluatedProperties: false
+
+ properties:
+ interrupt-controller: true
+ "#interrupt-cells":
+ const: 2
+
+ gpio-controller: true
+ "#gpio-cells":
+ const: 2
+
+ gpio-line-names:
+ minItems: 1
+ maxItems: 8
+
+ patternProperties:
+ "^(.+-hog(-[0-9]+)?)$":
+ type: object
+
+ required:
+ - gpio-hog
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ usb {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ device@1 {
+ compatible = "usb10c4,ea90";
+ reg = <1>;
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ sda-gpios = <&cp2112_gpio 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ scl-gpios = <&cp2112_gpio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+
+ temp@48 {
+ compatible = "national,lm75";
+ reg = <0x48>;
+ };
+ };
+
+ cp2112_gpio: gpio {
+ gpio-controller;
+ interrupt-controller;
+ #gpio-cells = <2>;
+ gpio-line-names = "CP2112_SDA", "CP2112_SCL", "TEST2",
+ "TEST3","TEST4", "TEST5", "TEST6";
+
+ fan-rst-hog {
+ gpio-hog;
+ gpios = <7 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "FAN_RST";
+ };
+ };
+ };
+ };
--
2.25.1
^ permalink raw reply related
* [PATCH v5 2/3] HID: usbhid: Share USB device firmware node with child HID device
From: Danny Kaehn @ 2023-02-10 22:36 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, jikos, benjamin.tissoires
Cc: bartosz.golaszewski, andriy.shevchenko, dmitry.torokhov,
devicetree, linux-input, ethan.twardy
In-Reply-To: <20230210223638.12796-1-kaehndan@gmail.com>
USB HID core now shares its fwnode with its child HID device.
Since there can only be one HID device on a USB interface, it is redundant
to specify a hid node under the USB device. This allows usb HID device
drivers to be described in firmware and make use of device properties.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
---
drivers/hid/usbhid/hid-core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index be4c731aaa65..a0859c49d9f4 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -19,6 +19,7 @@
#include <linux/list.h>
#include <linux/mm.h>
#include <linux/mutex.h>
+#include <linux/property.h>
#include <linux/spinlock.h>
#include <asm/unaligned.h>
#include <asm/byteorder.h>
@@ -1369,6 +1370,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
hid->hiddev_report_event = hiddev_report_event;
#endif
hid->dev.parent = &intf->dev;
+ device_set_node(&hid->dev, dev_fwnode(&intf->dev));
hid->bus = BUS_USB;
hid->vendor = le16_to_cpu(dev->descriptor.idVendor);
hid->product = le16_to_cpu(dev->descriptor.idProduct);
--
2.25.1
^ permalink raw reply related
* [PATCH v5 3/3] HID: cp2112: Fwnode Support
From: Danny Kaehn @ 2023-02-10 22:36 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, jikos, benjamin.tissoires
Cc: bartosz.golaszewski, andriy.shevchenko, dmitry.torokhov,
devicetree, linux-input, ethan.twardy
In-Reply-To: <20230210223638.12796-1-kaehndan@gmail.com>
Bind i2c and gpio interfaces to subnodes with names
"i2c" and "gpio" if they exist, respectively. This
allows the gpio and i2c controllers to be described
in firmware as usual. Additionally, support configuring the
i2c bus speed from the clock-frequency device property.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
---
drivers/hid/hid-cp2112.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 27cadadda7c9..23c4518ec016 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1234,6 +1234,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
u8 buf[3];
struct cp2112_smbus_config_report config;
struct gpio_irq_chip *girq;
+ struct i2c_timings timings;
int ret;
dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
@@ -1292,6 +1293,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
goto err_power_normal;
}
+ device_set_node(&dev->adap.dev, device_get_named_child_node(&hdev->dev, "i2c"));
+ i2c_parse_fw_timings(&dev->adap.dev, &timings, true);
+
+ config.clock_speed = cpu_to_be32(timings.bus_freq_hz);
config.retry_time = cpu_to_be16(1);
ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config),
@@ -1336,6 +1341,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
dev->gc.ngpio = 8;
dev->gc.can_sleep = 1;
dev->gc.parent = &hdev->dev;
+ dev->gc.fwnode = device_get_named_child_node(&hdev->dev, "gpio");
dev->irq.name = "cp2112-gpio";
dev->irq.irq_startup = cp2112_gpio_irq_startup;
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 3/3] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards
From: Orlando Chamberlain @ 2023-02-10 23:24 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Aditya Garg, Jiri Kosina, jkosina@suse.cz,
benjamin.tissoires@redhat.com, Andy Shevchenko,
andy.shevchenko@gmail.com, LKML, linux-input@vger.kernel.org,
ronald@innovation.ch, kekrby@gmail.com
In-Reply-To: <20230210162518.pe7ipe44falu3j3k@t-8ch.de>
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
> >
^ permalink raw reply
* Re: [PATCH 00/24 v2] Documentation: correct lots of spelling errors (series 1)
From: patchwork-bot+netdevbpf @ 2023-02-11 0:30 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-kernel, corbet, linux, axboe, olteanv, steffen.klassert,
daniel.m.jordan, akinobu.mita, deller, dmitry.torokhov, rydberg,
isdn, jikos, mbenes, pmladek, jpoimboe, peterz, mingo, will,
jglisse, naoya.horiguchi, linmiaohe, jonas, stefan.kristiansson,
shorne, bhelgaas, lpieralisi, maz, mpe, hca, gor, agordeev,
juri.lelli, vincent.guittot, dhowells, jarkko, paul, jmorris,
serge, bristot, rostedt, mhiramat, mathieu.poirier,
suzuki.poulose, zbr, fenghua.yu, reinette.chatre, tglx, bp, chris,
jcmvbkbc, coresight, dri-devel, keyrings, linux-block,
linux-crypto, linux-doc, linux-fbdev, linux-input, linux-pci,
linux-s390, linux-scsi, linux-sgx, linux-trace-devel,
linux-trace-kernel, live-patching, linux-security-module,
linux-usb, netdev, linux-mm, openrisc, linux-arm-kernel,
linux-xtensa, linuxppc-dev, x86
In-Reply-To: <20230209071400.31476-1-rdunlap@infradead.org>
Hello:
This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 8 Feb 2023 23:13:36 -0800 you wrote:
> Correct many spelling errors in Documentation/ as reported by codespell.
>
> Maintainers of specific kernel subsystems are only Cc-ed on their
> respective patches, not the entire series.
>
> These patches are based on linux-next-20230209.
>
> [...]
Here is the summary with links:
- [03/24] Documentation: core-api: correct spelling
(no matching commit)
- [08/24] Documentation: isdn: correct spelling
https://git.kernel.org/netdev/net-next/c/d12f9ad02806
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH 3/3] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards
From: Thomas Weißschuh @ 2023-02-11 2:23 UTC (permalink / raw)
To: Orlando Chamberlain
Cc: Aditya Garg, Jiri Kosina, jkosina@suse.cz,
benjamin.tissoires@redhat.com, Andy Shevchenko,
andy.shevchenko@gmail.com, LKML, linux-input@vger.kernel.org,
ronald@innovation.ch, kekrby@gmail.com
In-Reply-To: <20230211102425.178e1c78@redecorated-mbp>
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); +}
> > > +
^ permalink raw reply
* Re: [PATCH 3/3] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards
From: Thomas Weißschuh @ 2023-02-11 2:42 UTC (permalink / raw)
To: Orlando Chamberlain
Cc: Aditya Garg, Jiri Kosina, jkosina@suse.cz,
benjamin.tissoires@redhat.com, Andy Shevchenko,
andy.shevchenko@gmail.com, LKML, linux-input@vger.kernel.org,
ronald@innovation.ch, kekrby@gmail.com
In-Reply-To: <20230211022342.7xyvd3qsa42lwrkq@t-8ch.de>
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); +}
> > > > +
^ permalink raw reply
* [PATCH v1 0/1] HID: ft260: add GPIO support
From: Michael Zaidman @ 2023-02-11 11:57 UTC (permalink / raw)
To: jikos; +Cc: linux-kernel, linux-input, Michael Zaidman
The FTDI FT260 chip implements USB to I2C/UART bridges through two USB
HID class interfaces. The first is for I2C, and the second is for UART.
Each interface is independent, and the kernel detects it as a separate
USB hidraw device. In addition, the chip implements 14 GPIOs via
multifunctional pins.
This patch set adds GPIO support.
Michael Zaidman (1):
HID: ft260: add GPIO support
drivers/hid/hid-ft260.c | 476 +++++++++++++++++++++++++++++++++++++---
1 file changed, 447 insertions(+), 29 deletions(-)
--
2.34.1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox