brotfessor / sipodev

Patch for the SIPODEV SP1064 touchpad
https://bugzilla.redhat.com/show_bug.cgi?id=1526312
18 stars 2 forks source link

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

Open funder7 opened 5 years ago

funder7 commented 5 years ago

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 commented 5 years ago

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:

Hope that was useful

funder7 commented 5 years ago

Hello!

Thanks for replying. I read the whole store about the space saving decision made by the manufacturer... Is that really for 1 KB ? I definitely want to help other people with the same configuration! Now that you told me about the i2c designware host, I will try to recompile the kernel without the patch to see if the touchpad continues to work or not.ì, I will try to do it this evening, around 22:00 (GMT-2) stay tuned!

Have you got some informations about that Windows "preloader" ? Does it work the same way as your patch, with machine name, or it uses another technique? I'm not familiar at all with kernels and stuff sorry. Does the kernel have any kind of access to the touchpad prior to driver loading? Because if that its affirmative, maybe I can figure some kind of method to determine the chip id, don't ask me how :-)

brotfessor commented 5 years ago

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 commented 5 years ago

Wow that's a lot of knowledge! Thank you! As i told you, I'm really unexperienced about this topics.... I just know the names of those devices, but not exactly what they're supposed to do. I owned a macbook pro years ago, or built a hackintosh, that's probably the first time that I've heard about the smbus.

I agree with the fact that this is a cheap workaround, but it still sounds a bit strange...where not exactly in the "kilobyte" epoch nowadays. To be honest, in my notebook bios I've found misleading settings ... I've tried to send an email to the manufacturer to have further informations, but I'm pretty sure that they will reply with "we don't have those advanced technical informations". From the bios I see three devices, which are good candidates for a touchpad/touchscreen (my notebook has also that, but it's not working at the moment). I can choose the touchscreen brand from: Goodix

Elan

others

In the /proc/... (don't remeber exatcly where now) I've found: ALPS0001

ELAN221D

ELAN5510 (touchscreen?)

FTSC1000 (looks like a digitizer. Again, touchscreen?)

KIOX010A, KIOX020A (accelerometers)

NXP1001 (NFC)

I was so confused that finally I just opened the bottom cover, and looked at the brand on the touchpad IC, that's how I've found "SIPODEV 1064", and finally your patch on github!

My supposition is that they make one motherboard, which contains all the devices required for their products, and then they configure just the ones required for that particular product. Anyway, I'm a bit impressed by this pc: it's cheap, I've paid 220€ for it (about 235 USD), but its a good piece of hardware. The lcd looks very sharp, and has good colors (it reminds me to my old macbook). It has a 120GB ssd and an internal 32GB MMC, which is a bit slower, I used it for the boot partition, and to save important documents. The battery lasts around 5.30/6 hours, depending on the backlight intensity... The CPU, or SoC as they call it, it's an Intel Celeron N3550, derived from the Atom architecture, it works beautiful. I mean, doesn't seem an Atom at all.

I've bought it as work computer, because I had to move from my home a couple of weeks, and I'm very happy about the purchase. That's why I'd like to help other people with this machine :-) Ok, I'll write you the results then, when I'll test the unpatched kernel. Bye!

funder7 commented 5 years ago

Hi,

I've tested the kernel without the patch, it doesn't work mate! So, thank you very much for your work. Do you think that I can commit the new part which identifies my notebook model? I dont know if the other mediacom model already in there has been added with information based on real data returned on that system, because my device name format is slightly different, it doesn't have spaces in it, but underscores instead. I'm a bit concerned, if the model name isn't right, then the patch will not work for users with the 11" model.

I'd like to commit the update because doing so, I will have a working local repository, attached to the remote, actually I've just decompressed the kernel sources from an archive, it's not linked to the remote repository, so I cannot fetch the updates. I'm also in contact with the Intel Linux team, they have another repository where they will push a change to a driver that I need, do you think that I can link my local repo to many remotes and fetch both of them?

Bye, Federico

On set 16 2019, at 5:32 pm, Federico Ricchiuto fed.ricchiuto@gmail.com wrote:

Wow that's a lot of knowledge! Thank you! As i told you, I'm really unexperienced about this topics.... I just know the names of those devices, but not exactly what they're supposed to do. I owned a macbook pro years ago, or built a hackintosh, that's probably the first time that I've heard about the smbus.

I agree with the fact that this is a cheap workaround, but it still sounds a bit strange...where not exactly in the "kilobyte" epoch nowadays. To be honest, in my notebook bios I've found misleading settings ... I've tried to send an email to the manufacturer to have further informations, but I'm pretty sure that they will reply with "we don't have those advanced technical informations". From the bios I see three devices, which are good candidates for a touchpad/touchscreen (my notebook has also that, but it's not working at the moment). I can choose the touchscreen brand from: Goodix

Elan

others

In the /proc/... (don't remeber exatcly where now) I've found: ALPS0001

ELAN221D

ELAN5510 (touchscreen?)

FTSC1000 (looks like a digitizer. Again, touchscreen?)

KIOX010A, KIOX020A (accelerometers)

NXP1001 (NFC)

I was so confused that finally I just opened the bottom cover, and looked at the brand on the touchpad IC, that's how I've found "SIPODEV 1064", and finally your patch on github!

My supposition is that they make one motherboard, which contains all the devices required for their products, and then they configure just the ones required for that particular product. Anyway, I'm a bit impressed by this pc: it's cheap, I've paid 220€ for it (about 235 USD), but its a good piece of hardware. The lcd looks very sharp, and has good colors (it reminds me to my old macbook). It has a 120GB ssd and an internal 32GB MMC, which is a bit slower, I used it for the boot partition, and to save important documents. The battery lasts around 5.30/6 hours, depending on the backlight intensity... The CPU, or SoC as they call it, it's an Intel Celeron N3550, derived from the Atom architecture, it works beautiful. I mean, doesn't seem an Atom at all.

I've bought it as work computer, because I had to move from my home a couple of weeks, and I'm very happy about the purchase. That's why I'd like to help other people with this machine :-) Ok, I'll write you the results then, when I'll test the unpatched kernel. Bye! Federico Ricchiuto Fullstack developer fed.ricchiuto@gmail.com (mailto:fed.ricchiuto@gmail.com) +39 347 552 08 36 (tel:+39%20347%20552%2008%2036) Via Montello, 122 (https://maps.google.com/?q=Via%20Montello%2C%20122) https://www.italdev.it

On set 16 2019, at 3:39 pm, Brotfessor notifications@github.com wrote:

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... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub (https://github.com/brotfessor/sipodev/issues/5?email_source=notifications&email_token=AC7MTNV2GEKSGELNVSHJYQTQJ6ECLA5CNFSM4IW2WH6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6ZFKSI#issuecomment-531780937), or mute the thread (https://github.com/notifications/unsubscribe-auth/AC7MTNQ4KT5L5QA7D6MYIFTQJ6ECLANCNFSM4IW2WH6A).

brotfessor commented 5 years ago

Federick notifications@github.com writes:

I've tested the kernel without the patch, it doesn't work mate! So, thank you very much for your work. Do you think that I can commit the new part which identifies my notebook model? Yeah you can if you want to. It's not really obvious how to do it, read https://www.kernel.org/doc/html/v5.3/process/development-process.html It is explained there. If you don't want to go through this, you can also tell me your exact DMI strings and I will submit a patch. (and credit you in the commit message if you want).

I'd like to commit the update because doing so, I will have a working local repository, attached to the remote, actually I've just decompressed the kernel sources from an archive, it's not linked to the remote repository, so I cannot fetch the updates. I'm also in contact with the Intel Linux team, they have another repository where they will push a change to a driver that I need, do you think that I can link my local repo to many remotes and fetch both of them? Yes, you can commit your own stuff and pull from multiple sources, not a problem. You might have to read some documentation on how git works in that respect, I can't explain that here in detail though, because it is way too complex.

I dont know if the other mediacom model already in there has been added with information based on real data returned on that system, because my device name format is slightly different, it doesn't have spaces in it, but underscores instead. I'm a bit concerned, if the model name isn't right, then the patch will not work for users with the 11" model. The mediacom 11 inch model was tested in the very first iteration of the patch, as were all entries, so I have no reason to believe that any of the existing entries are wrong. Manufacturers do not necessarily name these strings consistently, so it is absolutely possible that yours are a bit different.

brotfessor commented 5 years ago

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 commented 5 years ago

Yeah you can if you want to. It's not really obvious how to do it, read https://www.kernel.org/doc/html/v5.3/process/development-process.html It is explained there. If you don't want to go through this, you can also tell me your exact DMI strings and I will submit a patch. (and credit you in the commit message if you want). .... Yes, you can commit your own stuff and pull from multiple sources, not a problem. You might have to read some documentation on how git works in that respect, I can't explain that here in detail though, because it is way too complex.

Thanks for the link, don't worry about explaining me the whole process...I've found a lot of documents online so I think that I will figure it out. It's not much about the credits, I'd like to try sending the patch, because probably in the future I will send other fixes for this computer, so it's better if I can learn how to do it without bothering you everytime!

I'm confused about which repository to use. I'm deciding between this two: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ What do you think? My sources are coming from the stable branch. But I'm not 100% sure that I have to commit on the same branch where I've pulled the sources. I suppose that there's a certain workflow to follow

The mediacom 11 inch model was tested in the very first iteration of the patch, as were all entries, so I have no reason to believe that any of the existing entries are wrong. Manufacturers do not necessarily name these strings consistently, so it is absolutely possible that yours are a bit different.

Ok, if it's tested, I'll leave it as is. I've asked to do a double check.

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).

Same situation. Maybe is the AMI bios, other BIOS brands arey very different one from each other, while the American Megatrends is more standardized. I've found screenshots online of other PCs, they are pretty similar, what's different depends on older bios version, more than on the computer model. (hypotesis) I'll send you an update when I will figure how to send the patch! :) Bye, Federico

brotfessor commented 5 years ago

Federick notifications@github.com writes:

I'm confused about which repository to use. I'm deciding between this two: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ What do you think? My sources are coming from the stable branch. But I'm not 100% sure that I have to commit on the same branch where I've pulled the sources. I suppose that there's a certain workflow to follow When you are doing development, you should always use Linus' main tree, so the first one of the two. There could be some changes in the main tree that have not made it (or will never make it) to the stable tree, that affect your starting point. In this case it probably doesn't matter, because you only touch that specific file, but it's a good rule to follow generally.

funder7 commented 5 years ago

Wonderful, thank you.

I've just found a couple of pictures on my phone ... The first makes me laugh 😂

TMA463-48LQI (The ic in the first picture) https://www.allicdata.com/goods/pdf.html?goods_id=4737055 ...Reading the documents related to the IC, seems that this touchpad it's not transparent, but it's more near to a touchscreen digitizer than to a common touchpad. Well, that's not a surprise, it's a device with multitouch, gestures, etc.

Realted to the bios containing that "ELAN" setting: http://www.emc.com.tw/eng/ts_sth.asp If you watch the video, he's using a stylus to write on it. That matches with the previous pdf, the first row in the table lists the features, stylus comprised. Bye, Federico

On set 17 2019, at 8:30 pm, Brotfessor notifications@github.com wrote:

Federick notifications@github.com writes:

I'm confused about which repository to use. I'm deciding between this two: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ What do you think? My sources are coming from the stable branch. But I'm not 100% sure that I have to commit on the same branch where I've pulled the sources. I suppose that there's a certain workflow to follow When you are doing development, you should always use Linus' main tree, so the first one of the two. There could be some changes in the main tree that have not made it (or will never make it) to the stable tree, that affect your starting point. In this case it probably doesn't matter, because you only touch that specific file, but it's a good rule to follow generally. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub (https://github.com/brotfessor/sipodev/issues/5?email_source=notifications&email_token=AC7MTNTPCTTADMCEBS2EOUDQKEO5JA5CNFSM4IW2WH6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD65PC3I#issuecomment-532345197), or mute the thread (https://github.com/notifications/unsubscribe-auth/AC7MTNXXC6DIFIYITIAOORTQKEO5JANCNFSM4IW2WH6A).

funder7 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Grazie Alberto ;)