larixer / hid-asus-dkms

ASUS HID FTE100* DKMS Driver
GNU General Public License v2.0
68 stars 10 forks source link

Remove Focaltech mentions in the driver #13

Closed bentiss closed 7 years ago

bentiss commented 7 years ago

I can't seem to find a way to do a proper review of the driver, so I'll just open some random issues.

First one it to remove "FocalTech" from the driver. As mentioned in comment 19 of the kernel bug (https://bugzilla.kernel.org/show_bug.cgi?id=120181#c19), the device has nothing to do with focaltech, it's a pure Asus device. FTE could mean anything, like "the Failed Touch Element", or anything you find funny :)

So please change the mentions in the code and the name of the driver to hid-asus.c only. Though there is already a hid-asus.c driver in the tree, so please merge to two of them (we generally prefer having only one driver per hardware maker).

larixer commented 7 years ago

@bentiss

Benjamin, nice to see you here!

Thank you very much for finding time to review this code!

Though I think issues are fine, I have prepared fake pull request for code review. I bet it would be easier for you to comment the code here: https://github.com/vlasenko/hid-asus-fte-dkms/pull/15/files

Please let me know if you need anything else. Like push access to the repository, etc.

redmcg commented 7 years ago

The mentions of FocalTech came from my reading of comment 43 (https://bugzilla.kernel.org/show_bug.cgi?id=120181#c43) which had:

[VENDOR_FOCALTECH_I2C]
FTE1

Obviously the device reports itself as FTE1001 so I made the assumption it was a FocalTech I2C device.

Having said that, I have no affiliation with FocalTech so I have no problem with removing their mention.

As for merging with hid-asus.c (and for further issues related to merging this driver in to the kernel tree) - I think we should fork https://github.com/torvalds/linux and begin making commits to that fork. That way you can comment on the individual commits we make as if they were individual patch files.

This repository can remain available for those looking for a DKMS driver module.

But I'm just throwing ideas out there. There's probably an established process which I'm also happy to follow.

bentiss commented 7 years ago

The mentions of FocalTech came from my reading of comment 43

Well, we are not entirely sure about if FTE is used for FocalTech here. And I am pretty sure they just don't care about Linux too. Given that the VendorID reported by the touchpad is the Asus one, we should simply stick to Asus.

One more reason to stick to Asus is that I have been reported today a bug for the Asus Transformer T300 CHI. I have a strong feeling that the touchpad is the same, but over Bluetooth. So better just skip FocalTech and focus on Asus :)

As for merging with hid-asus.c (and for further issues related to merging this driver in to the kernel tree) - I think we should fork https://github.com/torvalds/linux and begin making commits to that fork. That way you can comment on the individual commits we make as if they were individual patch files.

I'd say it's actually more convenient for now to gather testers with this single out of the tree driver. You can work on your local tree to integrate with Linus' tree, and even attach the current patch in this tree, but as long as the code you ship here in hid-asus.c is the same than the one you will be sending to linux-input@vger.kenrel.org, you should be fine.

Once you feel like submitting and merging your driver, simply send your patch to the appropriate mailing lists and follow the usual kernel patching guidelines (Documentation/SubmittingPatches).

redmcg commented 7 years ago

@vlasenko

I have made a fork of https://github.com/torvalds/linux and added our driver. You can find the commit here: https://github.com/redmcg/linux/commit/4ada5285f94e30f23817a9fc346e4b22e0c22f46

Take a look and let me know if you think it's ready for submission to the mailing list. I'll add you as a Collaborator so if you want to make any changes you can.

larixer commented 7 years ago

Yes, looks good to me. Thank you Brendan.

I think it's ready for submission into kernel mailing list, but still is not fully complete. The things below are minor and perhaps require quite an effort to implement properly, but I think they might be missing:

  1. i2c_hid error messages should not be produced, as discussed in https://github.com/vlasenko/hid-asus-fte-dkms/issues/16
  2. Device actually might have Host optimized runtime power management as specified in section 8.2 of Microsoft HID over I2C protocol and we could utilize it.
  3. Device might have the feature to wake up the HOST, when you touch it and the HOST is sleeping and we could utilize it.

I think the things above is minor and we can continue to evaluate them after submission of current driver code to kernel mailing list.

Victor

redmcg commented 7 years ago

Thanks Victor. I agree - we can revisit the driver to fix those outstanding issues later. But hopefully we can at least get the multi-touch support in to version 4.10 of the kernel.

larixer commented 7 years ago

Yes, sure