RE: [PATCH] mei: me: add ice lake point LP iTouch DID

[Usyskin, Alexander]

> Subject: [PATCH] mei: me: add ice lake point LP iTouch DID
> 
> Add the device ID for the Intel Precise Touch & Stylus (iTouch) function
> on Ice Lake Point LP. It appears as PCI device 8086:34e4, next to the
> regular MEI function at 8086:34e0, on Microsoft Surface Pro 7 and
> Surface Laptop 3.
> 
> Without this entry mei_me does not bind, the touch controller is
> unreachable, and neither the touchscreen nor the pen work. Users
> currently work around this by binding the device by hand at boot:
> 
>   echo "8086 34e4 ffffffff ffffffff 0 0 9" > \
>           /sys/bus/pci/drivers/mei_me/new_id
> 
> where the trailing 9 is the mei_me_cfgs[] index of MEI_ME_PCH12_CFG.
> That is fragile by construction, since the index is positional and any
> reordering of the config array silently changes which config the device
> gets.
> 
> This ID was first reported in June 2020, together with Sunrise Point's
> 0x9d3e, in "mei: me: Add itouch device IDs for SPT / ICP" [1]. That
> submission was declined over the server platform ID conflict, and the
> Sunrise Point half was subsequently added in commit 372a829862e5 ("mei:
> me: add MEI device for SPT with ITPS capability"). Ice Lake Point was
> never added.
> 
> Use MEI_ME_PCH12_CFG, matching the adjacent MEI_ICP_LP and MEI_ICP_N
> entries.
> 
> Link: https://lore.kernel.org/all/20200601155954.764558-1-
> dorian.stoll@xxxxxxx/ [1]
> Signed-off-by: Antonio Vázquez Blanco <antoniovazquezblanco@xxxxxxxxx>
> ---
> Notes for review:
> 
> Built on char-misc-next (x86_64 defconfig + CONFIG_INTEL_MEI_ME=m);
> mei/mei-me
> compile clean with no new warnings, and checkpatch --strict is clean.
> 
> I verified the resulting table entry in pci-me.o rather than just trusting the
> macro: vendor=0x8086 device=0x34e4 subvendor/subdevice=PCI_ANY_ID
> driver_data=9, i.e. byte-identical to what the hand-written new_id
> workaround
> above produces. So this should be a no-op for anyone already using that,
> other
> than making it unnecessary.
> 
> I do not have Ice Lake hardware, so this is NOT tested on a real device.
> 
> On the config choice: I used MEI_ME_PCH12_CFG to match the adjacent
> MEI_ICP_LP
> and MEI_ICP_N entries, and because it is what users are already selecting by
> hand. It does mean the device reports kind "mei" rather than "itouch" in sysfs.
> The only existing Gen12 iTouch config, MEI_ME_PCH12_SPS_ITOUCH_CFG,
> also
> carries MEI_CFG_FW_SPS_IGN and drops MEI_CFG_DMA_128, and I did not
> want to
> assume either is correct for an LP client part, particularly as the DMA ring
> is negotiated at runtime via hbm_f_dr_supported anyway. If you would prefer
> the
> device kind reported correctly I can add a plain Gen12 iTouch config instead;
> please say which you want.
> 

Please use MEI_ME_PCH12_SPS_ITOUCH_CFG as right kind report is important.
And about servers' exclusion - better be safe then sorry here.

- - 
Thanks,
Sasha



>  drivers/misc/mei/hw-me-regs.h | 1 +
>  drivers/misc/mei/pci-me.c     | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/misc/mei/hw-me-regs.h b/drivers/misc/mei/hw-me-regs.h
> index b9a67629496b..3972f9504f12 100644
> --- a/drivers/misc/mei/hw-me-regs.h
> +++ b/drivers/misc/mei/hw-me-regs.h
> @@ -94,6 +94,7 @@
>  #define PCI_DEVICE_ID_INTEL_MEI_CDF        0x18D3  /* Cedar Fork */
> 
>  #define PCI_DEVICE_ID_INTEL_MEI_ICP_LP     0x34E0  /* Ice Lake Point LP */
> +#define PCI_DEVICE_ID_INTEL_MEI_ICP_LP_3   0x34E4  /* Ice Lake Point LP 3
> (iTouch) */
>  #define PCI_DEVICE_ID_INTEL_MEI_ICP_N      0x38E0  /* Ice Lake Point N */
> 
>  #define PCI_DEVICE_ID_INTEL_MEI_JSP_N      0x4DE0  /* Jasper Lake Point N
> */
> diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c
> index 790a4647811b..e8805bcd3695 100644
> --- a/drivers/misc/mei/pci-me.c
> +++ b/drivers/misc/mei/pci-me.c
> @@ -98,6 +98,7 @@ static const struct pci_device_id mei_me_pci_tbl[] = {
>  	{ PCI_DEVICE_DATA(INTEL, MEI_CMP_H_3,
> MEI_ME_PCH8_ITOUCH_CFG) },
> 
>  	{ PCI_DEVICE_DATA(INTEL, MEI_ICP_LP, MEI_ME_PCH12_CFG) },
> +	{ PCI_DEVICE_DATA(INTEL, MEI_ICP_LP_3, MEI_ME_PCH12_CFG) },
>  	{ PCI_DEVICE_DATA(INTEL, MEI_ICP_N, MEI_ME_PCH12_CFG) },
> 
>  	{ PCI_DEVICE_DATA(INTEL, MEI_TGP_LP, MEI_ME_PCH15_CFG) },
> --
> 2.56.0