Skip to content

Conversation

@archseer
Copy link
Member

@archseer archseer commented Nov 7, 2019

@archseer
Copy link
Member Author

archseer commented Nov 7, 2019

Also fixes caps lock light on SL3, refs #8. Potentially also working for SL1/2 if the same iid/cid is used but we need someone to verify.

@mistay
Copy link

mistay commented Nov 8, 2019

Really nice work, Thanks @archseer!

@archseer archseer force-pushed the feature/sl3 branch 5 times, most recently from f9cef17 to b096b85 Compare November 10, 2019 08:40
@archseer
Copy link
Member Author

archseer commented Nov 10, 2019

@qzed So I was about to say that this PR is ready, but there seems to be a kernel panic going on. I can trigger it by swiping the touchpad extremely fast, but I've also had it happen when running insmod.

(edit: resolved)


platform_set_drvdata(pdev, drvdata);

status = hid_add_device(hid);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qzed Likely we want to do the hid_add_device even later, right before returning?

hid-multitouch will start doing things as soon as the HID device is detected

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should (mostly) work as is. The core driver is able to send output reports and set/get feature reports without the events on the EC being disabled. It breaks when the driver expects some input report being sent from the EC as response (as that's not enabled at this point). I don't have any idea if that's possible or done in practice, so we might want to change that.

If we want to change that: I think we should first set up the EC events and enable them, and then register the HID device to ensure other drivers can actually use it before we add it. Similarly on removing, first de-register/destroy the HID device and then remove and disable the events. That will require us to check if the HID device is set up in the event handler function, Which should be possible via an atomic bool or something.

@archseer archseer force-pushed the feature/sl3 branch 2 times, most recently from e6ff25a to 3d92c5b Compare November 11, 2019 12:53
@qzed qzed self-requested a review November 11, 2019 14:00
Copy link
Member

@qzed qzed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, mostly minor things. I'm not sure about the placement of the hid_add_device call and will check that. Good work so far!

@archseer archseer force-pushed the feature/sl3 branch 2 times, most recently from ec39f0d to eecddb2 Compare November 12, 2019 01:08
@mistay
Copy link

mistay commented Nov 12, 2019

@qzed, @archseer thanks for this session!

@archseer archseer requested a review from qzed November 12, 2019 01:10
@qzed
Copy link
Member

qzed commented Nov 12, 2019

Thanks!

Also you're alright with me adding

MODULE_AUTHOR("Blaž Hrastnik <blaz@mxxn.io>");
MODULE_DESCRIPTION("Driver for HID devices connected via Surface SAM");
MODULE_LICENSE("GPL v2");

(for integration into the kernel)?

Edit: Approved via IRC.

@qzed qzed merged commit bc08f7b into linux-surface:master Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Surface Laptop 3: Keyboard/Touchpad support

3 participants