Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Info about the patch - is it still needed? #5

Open
funder7 opened this issue Sep 15, 2019 · 13 comments
Open

Info about the patch - is it still needed? #5

funder7 opened this issue Sep 15, 2019 · 13 comments

Comments

@funder7
Copy link

funder7 commented Sep 15, 2019 •
edited

Hi!

The last week I bought a Mediacom Flexbook 13, which a bigger version of the Flexbook 11, already present into the kernel sources.

You can find it in i2c-hid-dmi-quirks.c line 342

Ok, so I was stuck with the touchpad problem, so I've opened the notebook in order to see the chip on the touchpad, that's how I've found your respository, trough the "sipodev" word. :)

In fact, the touchpad was detected as SYNA3602, but not working.

I decided to build a custom kernel, tailored exactly on the devices present in this pc.
While configuring, I've noticed the presence of feature, "i2c designware device", it was present in the computer, so I've added it to the configuration.

That anyway was AFTER adding to the ...quirks.c file a new entry for this notebook. I've read the discussion about how the touchpad its detected (also a comment by Linus Torvalds somewhere, who wasn't really happy about the way of adding the quirks into the kernel source code).

Later I've checked the source code of the quirks file, and I've seen a different code from your patch, plus the identification of each machine by DMI_SYS_VENDOR and DMI_PRODUCT_NAME.

Ok, sorry for my long message (also for not perfect english!), I was trying to tell you a little more, in order to understand my next question.

I'd like to understand if the new module into the kernel is making the DMI_SYS_VENDOR detection obsolete, by doing what the filter in Microsoft Windows was doing, or not.
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-designware-master.c

I'm telling you this, because after installing the new kernel, a whole lot of new i2c devices where detected on my system. Is the i2c-designware maybe like a master device (something like an i2c hub, watching for i2c slave devices).

From a developer perspective, i think that the DMI_SYS_VENDOR annd PRODUCT_NAME detection method can work, but depending on a string to make decisions, it's a bit weak. IT would be better if the new driver fixed this issue, so it's possible to use it instead of manually passing the quirks. I think that it will be also a good thing for new devices in the future (like mine pc now).

What do you think about this story?
Thanks for your time :)

My name's Federico, nice to meet you, and thank you very much, your work put me on the right path to solve this issue.

Best Regards
@brotfessor
Copy link
Owner

Well fair enough, this patch is upstream since about a year and there isn't any mention of it here, I fixed that now. I also added a short note on how to enable the patch for other affected notebooks.

i2c_designware has nothing to do with this, this is the I2C host controller on these (and a whole lot of other) systems, so yes basically the I2C master. That is why a lot of devices appear if you enable this config option. This driver should probably be enabled on any fairly modern system anyway, because these I2C devices are pretty ubiquitous in modern notebooks.

The driver for the touchpad itself then is the i2c_hid driver and that is where this patch sneaks in. The detection based on the DMI strings is more or less unavoidable, because that is the exact problem we have to fix.

Normally these i2c_hid devices allow reading some configuration from them where they basically explain their capabilities so we only have to have the one i2c_hid driver and not a few dozens of different drivers for every i2c touchpad. But in this case the vendor apparently tried to save like a KB of ROM on the touchpad controller and so on windows a specific driver sneaks in and provides this data instead. And this patch more or less replicates this behavior. So the only way we know if we are dealing with a touchpad of this type is to check against the DMI machine model and the specific ACPI device name (that happens to be the same every time).

The i2c_designware driver does not in any way replace this patch or anything like that.

So TLDR:

  • you should enable i2c_designware anyway, regardless of the touchpad
  • if it works without adding your DMI entries, all is well
  • if it only works after adding your entries to the quirks file, please tell me, so I can submit it upstream and it can also work out of the box for other people

Hope that was useful

@funder7
Copy link
Author

funder7 commented Sep 16, 2019 via email •
edited

@brotfessor
Copy link
Owner

I don't have any experience with the windows driver model, but the way I understand this works is, that when this driver is installed, it basically tells windows "Hey this device is mine now and don't bother using the generic i2c_hid driver", the same way it happens if you install e.g. a graphics card driver from the manufacturer and it replaces the generic windows VESA driver or something like that. And then it more or less provides another virtual i2c_hid port and to this the windows driver binds like it would have done in the first place.

I really don't know how exactly this matching is done but it could very well be that it is something similar since the driver is automatically pulled via windows update IIRC. Maybe it binds only on SYNA3602 acpi ID.

Btw to only match on that in the kernel would not be a good way, because that SYNA prefix actually belongs to Synaptics (https://uefi.org/ACPI_ID_List), another touchpad manufacturer and we cannot guarantee that they might use this ID at some point or even already do it. So we restrict it to the notebooks we know use this particular touchpad. Of course if they had not chosen this ACPI id, but something like SIPO1064, this would be the better way. Although I believe that is up to the notebook manufacturer, since it is in the ACPI tables.

Anyway, I don't believe there is another method for matching by talking to the touchpad. I poked the device a little bit in my experimentation and it seems to completely ignore the register address when asked (these are standard smbus read commands). That is part of the reason why the error message looks like it does. The kernel asks the device for the descriptors but gets the raw touchpad data back, which is obviously not what the kernel wants. So it seems as you cannot get anything out of the device except the hid report.

So this saves the manufacturer not only the memory, but the whole i2c addressing logic, so quite a considerable amount of chip space. The device just pushes out its data after x number of clock impulses, regardless of what it got as input, it's basically a serializer at this point :)

And yes obviously this is quite a shitty strategy and completely against the i2c-hid spec, but that's the price one has to pay when buying cheap notebooks, I guess...

@funder7
Copy link
Author

funder7 commented Sep 16, 2019 via email •
edited

@funder7
Copy link
Author

funder7 commented Sep 17, 2019 via email

@brotfessor
Copy link
Owner

brotfessor commented Sep 17, 2019 via email

@brotfessor
Copy link
Owner

Anyway, I have a very similar notebook too and can't complain regarding price/performance either.

Yes, the BIOS is kinda weird on these devices, it seems like a generic version or something like that. Most of these devices you can enable or disable do not even exist and even some of the switches that represent an existing device do not work. I think it is best to leave these settings alone as it seems that all devices are detected correctly (at least in my case).

@funder7
Copy link
Author

funder7 commented Sep 17, 2019 via email

@brotfessor
Copy link
Owner

brotfessor commented Sep 17, 2019 via email

@funder7
Copy link
Author

funder7 commented Sep 17, 2019 via email

@funder7
Copy link
Author

funder7 commented Jun 15, 2020

Hi @brotfessor,
I hope that you're doing well. I've just read again the whole thread, now the whole story is definitely clearer to me :-)

After all this time, I've finally managed to send the patch, I feel better now :-D
Just in case someone needs it while the patch gets approved, can I share it here?
If you want I can open a new issue just for that.

There's a lot of informations in this thread, maybe renaming the issue to something more indicative would help other people to find them. For me there's no problem if you want to rename the discussion.

Here's my patch, tested & working on a mediacom FlexBook edge 13

      {
               .ident = "Mediacom FlexBook edge 13",
               .matches = {
                       DMI_EXACT_MATCH(DMI_SYS_VENDOR, "MEDIACOM"),
                       DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "FlexBook_edge13-M-FBE13"),
               },
               .driver_data = (void *)&sipodev_desc
       },

Bye
F.

@amsalby
Copy link

amsalby commented Aug 1, 2020

Great @funder7 I'm just 15 days late in publishing exactly your same patch!
I was trying to solve your same issue on a PC of my friend.

Thanks for your work

@funder7
Copy link
Author

funder7 commented Aug 4, 2020

Grazie Alberto ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@amsalby @brotfessor @funder7