Created attachment 308024 [details]
Attached a patch that adds missing I2C pin definitions for Alder Lake. Needs testing on real hardware.

Hey! I tried fixing this as my first attempt at kernel hacking.
I added missing I2C2/I2C3/I2C4 pin definitions based on the Tiger Lake mappings.
This should definitely be tested on real hardware to verify if it resolves the touchpad detection issue.

I confirm that the proposed patch resolves the issue. Thanks.

Created attachment 308025 [details]
Proposed patch with correct pin numbering and comment

Revised patch which does at least introduce the new pins with an acceptable numbering

Please see the attached revised patch: the comment has been improved and the new pins are introduced with at least an acceptable numbering, if you want to avoid reordering the whole list.

Awesome, thanks!
Do you want to submit the patch or shall I?
As I'm learning, I'd like to give it a shot :)

Created attachment 308026 [details]
Second revision of the patch with an example on how to correct the structure bounds

Correct the community structure bounds.

If you submit the patch as it is, it will probably be rejected even if it resolves the issue.

I don't have the Intel specifications, therefore I am not sure how to proceed. 

But please consider that the pin definitions are grouped in communities.

So, the structures at the bottom of the file also need to be updated...

Please see the latest revised patch on how in principle the bounds of the structure definitions need to be updated.

Created attachment 308027 [details]
Revised v2 patch: add missing I2C2/I2C3/I2C4 pins and update community structures

Hi Guido,

Thanks again for the feedback.

I have uploaded a revised v2 patch that corrects both the pin definitions and the community structures as you suggested.

Would appreciate your review before I send it upstream. Thanks!

Folks, I don't know what's going on here, but using the Tiger Lake driver is the correct approach. So, what's the problem?

Note, the patch without shuffling the ACPI IDs does basically nothing, it adds random pins to random driver. I'm utterly confused on how it may solve anything, but adding problems.

Thanks Andy for the clarification.

I'm still learning and don't fully understand all the platform details here yet, but I appreciate you explaining why patching the Alder Lake driver isn't the right approach.

Thanks again for taking the time to review this!

How can using the pinctrl driver for another chipset be the right approach ?

An user which configures the kernel for a machine based on the Intel Alder Lake chipset cannot figure out that the pinctrl driver for another chipset needs to be used.

I have tried searching for the official Intel PCH specification, but it seems that no specifications are available:

https://www.intel.com/content/www/us/en/products/platforms/details/alder-lake-p/docs.html

I agree that the current patch from Ariel does not respect the proper grouping and ordering of communities (which need to be based on official specifications) and that therefore it is not ready for merging, but the solution in my opinion is to fix the patch so that the Alder Lake pinctrl module correctly defines the I2C pins.

Marking it as REOPENED because the issue has not been resolved nor the specifications to resolve it have been provided.

Yeah as it stands, this grouping is based on observation, not official documentation (I think I even wrote that in a comment in my first patch). As I don’t have access to the Intel PCH documentation (and am still getting into Kernel Hacking), these were more or less educated guesses.

Andy if you prefer, I am happy to withdraw the patch and instead suggest documenting the need to use CONFIG_PINCTRL_TIGERLAKE for affected systems.

I do agree with Guido tho that the status quo is sort of confusing.

(In reply to Guido Trentalancia from comment #12)

TL;DR: Yes, we probably need to amend the help texts for the kernel configuration options, no code seem need to be fixed.

> How can using the pinctrl driver for another chipset be the right approach ?

This is the big misunderstanding here. Linux is NOT a Windows in a sense of the device driver model and what the approach is used for the supported platforms. Besides that the granularity of the platform is not a *chipset*, it's *an IP*. And this makes a lot of differences here. The whatever chipset or hardware in a sense may use and re-use the same IP blocks over and over. That's why one driver can span over several generations of the hardware. This approach applies to many vendors and much more hardware IPs than just this case. I suggest you to study this more (in case you want to go deep with it).

(In reply to Guido Trentalancia from comment #13)
> Marking it as REOPENED because the issue has not been resolved nor the
> specifications to resolve it have been provided.

What *issue*?! The provided patch does NOT solve *anything*. It makes things worse. Please, try to explain the issue here, as I am completely out on what's going on here. Currently it's all red herring.

(In reply to Ariel Simulevski from comment #14)
> Yeah as it stands, this grouping is based on observation, not official
> documentation (I think I even wrote that in a comment in my first patch). As
> I don’t have access to the Intel PCH documentation (and am still getting
> into Kernel Hacking), these were more or less educated guesses.
> 
> Andy if you prefer, I am happy to withdraw the patch and instead suggest
> documenting the need to use CONFIG_PINCTRL_TIGERLAKE for affected systems.
> 
> I do agree with Guido tho that the status quo is sort of confusing.

To understand the issue, I need to hear how you reproduce it and how this patch makes it different. The drivers are bound to the devices based on ID tables (ACPI ID table in this case). Your change doesn't alter that and hence it can't be involved in anything related to this case, it can't *fix* anything.

Andy, the patch that Ariel provided did sort the issue of I2C PIXA touchpad detection, although it's pretty clear that it uses duplicated pin numbering that needs proper grouping and ordering within the existing communities.

So Ariel intuition sounds correct to me, in that the pin definitions for I2C SDA and SCL lines are missing in the Alder Lake pinctrl driver.

From an end-user point of view it's illogical and not intuitive that one has to use a kernel module targeting another chipset in order for I2C device detection to work properly.

Steps to reproduce:

 - grab an Intel Alder Lake based laptop using the PixArt touchpad from the list 
   provided at:
   https://linux-hardware.org/?id=ps/2:3848-3848-pixa3848-00-093a-touchpad

 - build a kernel with the pinctrl-alderlake module using the
   CONFIG_PINCTRL_ALDERLAKE kernel configuration option: this is what a normal
   end user does

 - install the newly built kernel and reboot

 - start an X session and realize that the touchpad is not working or otherwise
   use "dmesg | grep -i input" to realize from the kernel log that the I2C
   touchpad has not been detected

Steps to resolve the issue:

 - apply any of the patches provided in this bug report

 - rebuild the kernel, install it and reboot

 - start an X session or type "dmesg | grep -i input" and realize that the I2C
   PixArt touchpad is not detected, as the Alder Lake pinctrl driver now
   includes the definitions for the I2C SDA and SCL lines that were missing in
   the original version of the kernel Alder Lake pinctrl module

Steps that might possibly lead to the creation of a proper patch:

 - edit the drivers/pinctrl/intel/pinctrl-alderlake.c kernel module source code

 - ask Intel to kindly provide the official documentation for the Alder Lake PCH

 - add the definition for the 4 I2C SDA and SCL lines in the proper group and
   community: for example in the Tiger Lake driver that was group GPP_H and
   community 1

 - modify the bounds in the structures at the bottom of the kernel module source
   code in order to account for the newly added I2C SDA and SCL lines

I believe marking this as RESOLVED INVALID is not the way forward.