[PATCH RFC 0/4] Driver for Apple Z2 touchscreens.

[PATCH RFC 0/4] Driver for Apple Z2 touchscreens.

[Sasha Finkelstein via B4 Relay]

Hi.

This series adds support for Apple touchscreens using the Z2 protocol.
Those are used as the primary touchscreen on mobile Apple devices, and for the
touchbar on laptops using the M-series chips. (T1/T2 laptops have a coprocessor
in charge of speaking Z2 to the touchbar).

Sending this as a RFC for now, since this series requires the SPI controller
support which is not upstream yet:
https://lore.kernel.org/all/20211212034726.26306-1-marcan@marcan.st/

Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
Sasha Finkelstein (4):
      dt-bindings: input: touchscreen: Add Z2 controller bindings.
      input: apple_z2: Add a driver for Apple Z2 touchscreens
      arm64: dts: apple: t8103: Add touchbar bindings
      MAINTAINERS: Add entries for Apple Z2 touchscreen driver

 .../input/touchscreen/apple,z2-touchscreen.yaml    |  81 ++++
 MAINTAINERS                                        |   2 +
 arch/arm64/boot/dts/apple/t8103-j293.dts           |  20 +
 arch/arm64/boot/dts/apple/t8103.dtsi               |  12 +
 drivers/input/touchscreen/Kconfig                  |  13 +
 drivers/input/touchscreen/Makefile                 |   1 +
 drivers/input/touchscreen/apple_z2.c               | 465 +++++++++++++++++++++
 7 files changed, 594 insertions(+)
---
base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
change-id: 20230223-z2-for-ml-18fb5246f4b8

Best regards,
-- 
Sasha Finkelstein <fnkl.kernel@gmail.com>

[PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Sasha Finkelstein via B4 Relay]

From: Sasha Finkelstein <fnkl.kernel@gmail.com>

Add bindings for touchscreen controllers attached using the Z2 protocol.
Those are present in most Apple devices.

Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 .../input/touchscreen/apple,z2-touchscreen.yaml    | 81 ++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
new file mode 100644
index 000000000000..695594494b1e
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/apple,z2-touchscreen.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple touchscreens attached using the Z2 protocol.
+
+maintainers:
+  - asahi@lists.linux.dev
+  - Sasha Finkelstein <fnkl.kernel@gmail.com>
+
+description: A series of touschscreen controllers used in Apple products.
+
+allOf:
+  - $ref: touchscreen.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: apple,z2-touchscreen
+
+  reg:
+    maxItems: 1
+
+  interrupts-extended:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  cs-gpios:
+    maxItems: 1
+
+  firmware-name:
+    maxItems: 1
+
+  apple,z2-device-name:
+    description: The name to be used for the input device
+    $ref: /schemas/types.yaml#/definitions/string
+
+  touchscreen-size-x: true
+  touchscreen-size-y: true
+  spi-max-frequency: true
+
+required:
+  - compatible
+  - interrupts-extended
+  - reset-gpios
+  - cs-gpios
+  - firmware-name
+  - apple,z2-device-name
+  - touchscreen-size-x
+  - touchscreen-size-y
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            touchscreen@0 {
+                    compatible = "apple,z2-touchscreen";
+                    reg = <0>;
+                    spi-max-frequency = <11500000>;
+                    reset-gpios = <&pinctrl_ap 139 0>;
+                    cs-gpios = <&pinctrl_ap 109 0>;
+                    interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
+                    firmware-name = "apple/dfrmtfw-j293.bin";
+                    touchscreen-size-x = <23045>;
+                    touchscreen-size-y = <640>;
+                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";
+            };
+    };
+
+...

-- 
Git-137.1)

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Mark Kettenis]

> Date: Fri, 24 Feb 2023 11:20:06 +0100
> From: Sasha Finkelstein <fnkl.kernel@gmail.com>

Hi Sasha,

> Add bindings for touchscreen controllers attached using the Z2 protocol.
> Those are present in most Apple devices.
> 
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
>  .../input/touchscreen/apple,z2-touchscreen.yaml    | 81 ++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> new file mode 100644
> index 000000000000..695594494b1e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/apple,z2-touchscreen.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple touchscreens attached using the Z2 protocol.
> +
> +maintainers:
> +  - asahi@lists.linux.dev
> +  - Sasha Finkelstein <fnkl.kernel@gmail.com>
> +
> +description: A series of touschscreen controllers used in Apple products.
> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: apple,z2-touchscreen
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts-extended:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  cs-gpios:
> +    maxItems: 1
> +
> +  firmware-name:
> +    maxItems: 1

What is the motivation for including the firmware name in the device
tree rather than constructing it in the driver like what is done for
the broadcom wireless?

In your example the firmware name includes the directory name.  I
think that is a bad idea since it makes assumptions about the
directory layout used for the firmware files on the OS level.  And in
particular, forcing the firmware to be in a subdirectory named "apple"
would be awkward for the way we handle firmware in OpenBSD.

> +
> +  apple,z2-device-name:
> +    description: The name to be used for the input device
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  touchscreen-size-x: true
> +  touchscreen-size-y: true
> +  spi-max-frequency: true
> +
> +required:
> +  - compatible
> +  - interrupts-extended
> +  - reset-gpios
> +  - cs-gpios
> +  - firmware-name
> +  - apple,z2-device-name
> +  - touchscreen-size-x
> +  - touchscreen-size-y
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            touchscreen@0 {
> +                    compatible = "apple,z2-touchscreen";
> +                    reg = <0>;
> +                    spi-max-frequency = <11500000>;
> +                    reset-gpios = <&pinctrl_ap 139 0>;
> +                    cs-gpios = <&pinctrl_ap 109 0>;
> +                    interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
> +                    firmware-name = "apple/dfrmtfw-j293.bin";
> +                    touchscreen-size-x = <23045>;
> +                    touchscreen-size-y = <640>;
> +                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";
> +            };
> +    };
> +
> +...
> 
> -- 
> Git-137.1)
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Sasha Finkelstein]

On Fri, 24 Feb 2023 at 11:55, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

> What is the motivation for including the firmware name in the device
> tree rather than constructing it in the driver like what is done for
> the broadcom wireless?
There is no way to identify the device subtype before the firmware is
uploaded, and so i need some way of figuring out which firmware to use.

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Sven Peter]

Hi,


On Fri, Feb 24, 2023, at 12:04, Sasha Finkelstein wrote:
> On Fri, 24 Feb 2023 at 11:55, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
>> What is the motivation for including the firmware name in the device
>> tree rather than constructing it in the driver like what is done for
>> the broadcom wireless?
> There is no way to identify the device subtype before the firmware is
> uploaded, and so i need some way of figuring out which firmware to use.

Some Broadcom bluetooth boards use the compatible of the root node (see
btbcm_get_board_name in drivers/bluetooth/btbcm.c) which would be "apple,jXXX"
for Apple Silicon. I believe the Broadcom WiFi driver has similar logic as well
which marcan had to extend to instead of "brcm,board-type" because different
WiFi boards can me matched to different Apple Silicon boards. I don't think
that's the case for this touchscreen though.



Sven

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Hector Martin]

On 24/02/2023 20.08, Sven Peter wrote:
> Hi,
> 
> 
> On Fri, Feb 24, 2023, at 12:04, Sasha Finkelstein wrote:
>> On Fri, 24 Feb 2023 at 11:55, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>
>>> What is the motivation for including the firmware name in the device
>>> tree rather than constructing it in the driver like what is done for
>>> the broadcom wireless?
>> There is no way to identify the device subtype before the firmware is
>> uploaded, and so i need some way of figuring out which firmware to use.
> 
> Some Broadcom bluetooth boards use the compatible of the root node (see
> btbcm_get_board_name in drivers/bluetooth/btbcm.c) which would be "apple,jXXX"
> for Apple Silicon. I believe the Broadcom WiFi driver has similar logic as well
> which marcan had to extend to instead of "brcm,board-type" because different
> WiFi boards can me matched to different Apple Silicon boards. I don't think
> that's the case for this touchscreen though.

The reason why the brcmfmac stuff needs to construct the firmware name
itself is that parts of it come from the OTP contents, so there is no
way to know from the bootloader what the right firmware is.

That is not the case here, so it makes perfect sense to specify the
firmware with `firmware-name` (which is a standard DT property).

As for the layout, both bare names and paths are in common use:

qcom/sm8450-qrd.dts:    firmware-name = "qcom/sm8450/slpi.mbn";
ti/k3-am64-main.dtsi:   firmware-name = "am64-main-r5f0_0-fw";

... but the bare names in particular, judging by some Google searches,
are *actually* mapped to bare files in /lib/firmware anyway. So the
firmware-name property contains the firmware path in the linux-firmware
standard hierarchy, in every case.

I already did the same thing for the touchpad on M2s (which requires
analogous Z2 firmware passed to it, just in a different format):

dts/apple/t8112-j413.dts: firmware-name = "apple/tpmtfw-j413.bin";

Why is having a directory a problem for OpenBSD? Regardless of how
firmware is handled behind the scenes, it seems logical to organize it
by vendor somehow. It seems to me that gratuitously diverging from the
standard firmware hierarchy is only going to cause trouble for OpenBSD.
Obviously it's fine to store it somewhere other than /lib/firmware or
use a completely unrelated mechanism other than files, but why does the
*organization* of the firmware have to diverge? There can only be one DT
binding, so we need to agree on a way of specifying firmwares that works
cross-OS, and I don't see why "apple/foo.bin" couldn't be made to work
for everyone in some way or another.

- Hector

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Mark Kettenis]

> Date: Tue, 28 Feb 2023 11:58:28 +0900
> From: Hector Martin <marcan@marcan.st>
> 
> On 24/02/2023 20.08, Sven Peter wrote:
> > Hi,
> > 
> > 
> > On Fri, Feb 24, 2023, at 12:04, Sasha Finkelstein wrote:
> >> On Fri, 24 Feb 2023 at 11:55, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >>
> >>> What is the motivation for including the firmware name in the device
> >>> tree rather than constructing it in the driver like what is done for
> >>> the broadcom wireless?
> >> There is no way to identify the device subtype before the firmware is
> >> uploaded, and so i need some way of figuring out which firmware to use.
> > 
> > Some Broadcom bluetooth boards use the compatible of the root node (see
> > btbcm_get_board_name in drivers/bluetooth/btbcm.c) which would be "apple,jXXX"
> > for Apple Silicon. I believe the Broadcom WiFi driver has similar logic as well
> > which marcan had to extend to instead of "brcm,board-type" because different
> > WiFi boards can me matched to different Apple Silicon boards. I don't think
> > that's the case for this touchscreen though.
> 
> The reason why the brcmfmac stuff needs to construct the firmware name
> itself is that parts of it come from the OTP contents, so there is no
> way to know from the bootloader what the right firmware is.

The name of the "nvram" file is constructed as well, and that uses the
compatible of the machine (the root of the device tree).  I suppose
what is special in that case is that several files are tried so a
single 'firmware-name" property wouldn't cut it.

> That is not the case here, so it makes perfect sense to specify the
> firmware with `firmware-name` (which is a standard DT property).

It certainly provides the flexibility to cater for all potential
nonsense names Apple comes up with for future hardware.

> As for the layout, both bare names and paths are in common use:
> 
> qcom/sm8450-qrd.dts:    firmware-name = "qcom/sm8450/slpi.mbn";
> ti/k3-am64-main.dtsi:   firmware-name = "am64-main-r5f0_0-fw";
> 
> ... but the bare names in particular, judging by some Google searches,
> are *actually* mapped to bare files in /lib/firmware anyway. So the
> firmware-name property contains the firmware path in the linux-firmware
> standard hierarchy, in every case.

Well, I think the device tree should not be tied to a particular OS
and therefore not be tied to things like linux-firmware.

> I already did the same thing for the touchpad on M2s (which requires
> analogous Z2 firmware passed to it, just in a different format):
> 
> dts/apple/t8112-j413.dts: firmware-name = "apple/tpmtfw-j413.bin";
> 
> Why is having a directory a problem for OpenBSD? Regardless of how
> firmware is handled behind the scenes, it seems logical to organize it
> by vendor somehow. It seems to me that gratuitously diverging from the
> standard firmware hierarchy is only going to cause trouble for OpenBSD.
> Obviously it's fine to store it somewhere other than /lib/firmware or
> use a completely unrelated mechanism other than files, but why does the
> *organization* of the firmware have to diverge? There can only be one DT
> binding, so we need to agree on a way of specifying firmwares that works
> cross-OS, and I don't see why "apple/foo.bin" couldn't be made to work
> for everyone in some way or another.

We organize the firmware by driver.  And driver names in *BSD differ
from Linux since there are different constraints.  The firmware is
organized by driver because we have separate firmware packages for
each driver that get installed as-needed by a tool that matches on the
driver name.

Rather than have the device tree dictate the layout of the firmware
files, I think it would be better to have the OS driver prepend the
directory to match the convention of the OS in question.  This is what
we typically do in OpenBSD.

Now I did indeed forget about the "dockchannel" touchpad firmware that
I already handle in OpenBSD.  That means I could handle the touchbar
firmware in the same way.  But that is mostly because these firmwares
are non-distributable, so we don't have firmware packages for them.
Instead we rely on the Asahi installer to make the firmware available
on the EFI partition and the OpenBSD installer to move the firmware in
place on the root filesystem.

So this isn't a big issue.

Cheers,

Mark

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Hector Martin]

On 01/03/2023 05.53, Mark Kettenis wrote:
>> Date: Tue, 28 Feb 2023 11:58:28 +0900
>> From: Hector Martin <marcan@marcan.st>
>>
>> On 24/02/2023 20.08, Sven Peter wrote:
>>> Hi,
>>>
>>>
>>> On Fri, Feb 24, 2023, at 12:04, Sasha Finkelstein wrote:
>>>> On Fri, 24 Feb 2023 at 11:55, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>>>
>>>>> What is the motivation for including the firmware name in the device
>>>>> tree rather than constructing it in the driver like what is done for
>>>>> the broadcom wireless?
>>>> There is no way to identify the device subtype before the firmware is
>>>> uploaded, and so i need some way of figuring out which firmware to use.
>>>
>>> Some Broadcom bluetooth boards use the compatible of the root node (see
>>> btbcm_get_board_name in drivers/bluetooth/btbcm.c) which would be "apple,jXXX"
>>> for Apple Silicon. I believe the Broadcom WiFi driver has similar logic as well
>>> which marcan had to extend to instead of "brcm,board-type" because different
>>> WiFi boards can me matched to different Apple Silicon boards. I don't think
>>> that's the case for this touchscreen though.
>>
>> The reason why the brcmfmac stuff needs to construct the firmware name
>> itself is that parts of it come from the OTP contents, so there is no
>> way to know from the bootloader what the right firmware is.
> 
> The name of the "nvram" file is constructed as well, and that uses the
> compatible of the machine (the root of the device tree).  I suppose
> what is special in that case is that several files are tried so a
> single 'firmware-name" property wouldn't cut it.

No, if you look at the way the name is constructed, some of it comes
from OTP. The plain compatible stuff is for non-Apple platforms. Apple
platforms need lookup of nvram/etc per specific fields in the OTP, and
then we try multiple firmware names from most to least specific because
the distinction often isn't relevant (but in some cases it is, and this
even changes from macOS version to macOS version). Our firmware
extractor actually attempts to prune the firmware tree by deduplicating
and promoting the most popular variant up towards the root then pruning
redundant branches, because otherwise we'd end up with hundreds of
copies or links (which is what macOS does, they don't try multiple
firmware filenames).

If the OTP were easily readable from the bootloader I'd just have thrown
this in m1n1 and kept a fixed firmware-name property, but that involves
full PCIe init and power-up of the wlan module and that's way too much
junk to put in there. Hence, dynamically computing firmware names in the
kernel driver.

If it were a simple 1:1 mapping from device tree blob to firmware files,
I would certainly have advocated for "firmware-name" instead of the more
complex thing we do now.

>> That is not the case here, so it makes perfect sense to specify the
>> firmware with `firmware-name` (which is a standard DT property).
> 
> It certainly provides the flexibility to cater for all potential
> nonsense names Apple comes up with for future hardware.

We actually make up the firmware names ourselves in the extractor, so
that's not the reason. But if nothing else I'm pretty sure we already
have n:1 mappings (M2 Pro/Max laptops almost certainly share the same
touchpad firmware for at least the same size chassis models, if not all
4 - haven't looked at that yet though), so using a separate property
means we don't have to play symlink/hardlink games.

> 
>> As for the layout, both bare names and paths are in common use:
>>
>> qcom/sm8450-qrd.dts:    firmware-name = "qcom/sm8450/slpi.mbn";
>> ti/k3-am64-main.dtsi:   firmware-name = "am64-main-r5f0_0-fw";
>>
>> ... but the bare names in particular, judging by some Google searches,
>> are *actually* mapped to bare files in /lib/firmware anyway. So the
>> firmware-name property contains the firmware path in the linux-firmware
>> standard hierarchy, in every case.
> 
> Well, I think the device tree should not be tied to a particular OS
> and therefore not be tied to things like linux-firmware.

That's fine, but we need *some* source of truth, and just like the Linux
kernel tree is the system of record for device tree bindings today, I
don't see a good reason not to use linux-firmware as the defacto
standard for firmware organization. There's nothing OS-specific about,
effectively, a list of identifiers that particular firmwares should be
listed under. Think of it as a "path key => expected firmware blob"
mapping. How each OS implements that is up to the OS.

This is similar to the whole vendorfw mechanism I constructed for these
platforms. Sure, it's based on Linuxisms, but the whole thing is trivial
enough to reimplement on any OS without much trouble (just a
fixed-format CPIO archive in the ESP at a known path).

>> I already did the same thing for the touchpad on M2s (which requires
>> analogous Z2 firmware passed to it, just in a different format):
>>
>> dts/apple/t8112-j413.dts: firmware-name = "apple/tpmtfw-j413.bin";
>>
>> Why is having a directory a problem for OpenBSD? Regardless of how
>> firmware is handled behind the scenes, it seems logical to organize it
>> by vendor somehow. It seems to me that gratuitously diverging from the
>> standard firmware hierarchy is only going to cause trouble for OpenBSD.
>> Obviously it's fine to store it somewhere other than /lib/firmware or
>> use a completely unrelated mechanism other than files, but why does the
>> *organization* of the firmware have to diverge? There can only be one DT
>> binding, so we need to agree on a way of specifying firmwares that works
>> cross-OS, and I don't see why "apple/foo.bin" couldn't be made to work
>> for everyone in some way or another.
> 
> We organize the firmware by driver.  And driver names in *BSD differ
> from Linux since there are different constraints.  The firmware is
> organized by driver because we have separate firmware packages for
> each driver that get installed as-needed by a tool that matches on the
> driver name.

That's fair, but you can still have another level of hierarchy after the
driver, no? Or just throw away that level when you parse the
`firmware-name` if you prefer.

> Rather than have the device tree dictate the layout of the firmware
> files, I think it would be better to have the OS driver prepend the
> directory to match the convention of the OS in question.  This is what
> we typically do in OpenBSD.

The thing is that for better or for worse, some drivers drive devices
with firmware provided by multiple vendors, and then it can still make
sense to split off by vendor. E.g. brcmfmac wifi is already in the
process of diverging into three firmware lineages provided by
two(/three?) vendors, even if you ignore the entire Apple special
snowflake case. I expect pain to come out of that one for everyone
involved... (well, at least for Apple we can always special case
conditionals on "has OTP" which is effectively the "is Apple" flag).
ISTR that radeon/amdgpu also ended up with separate roots, but it's
mixed and with different firmware formats for each and a fallback.

> Now I did indeed forget about the "dockchannel" touchpad firmware that
> I already handle in OpenBSD.  That means I could handle the touchbar
> firmware in the same way.  But that is mostly because these firmwares
> are non-distributable, so we don't have firmware packages for them.
> Instead we rely on the Asahi installer to make the firmware available
> on the EFI partition and the OpenBSD installer to move the firmware in
> place on the root filesystem.
> 
> So this isn't a big issue.

:)

(Do let me know if you have any big issues of course, you know I don't
want to gratuitously make your life hard!)

- Hector

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Sven Peter]

On Fri, Feb 24, 2023, at 11:20, Sasha Finkelstein via B4 Relay wrote:
> From: Sasha Finkelstein <fnkl.kernel@gmail.com>
>
> Add bindings for touchscreen controllers attached using the Z2 protocol.
> Those are present in most Apple devices.
>
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
>  .../input/touchscreen/apple,z2-touchscreen.yaml    | 81 ++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>
> diff --git 
> a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml 
> b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> new file mode 100644
> index 000000000000..695594494b1e
> --- /dev/null
> +++ 
> b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: 
> http://devicetree.org/schemas/input/touchscreen/apple,z2-touchscreen.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple touchscreens attached using the Z2 protocol.
> +
> +maintainers:
> +  - asahi@lists.linux.dev
> +  - Sasha Finkelstein <fnkl.kernel@gmail.com>
> +
> +description: A series of touschscreen controllers used in Apple 
> products.
> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: apple,z2-touchscreen
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts-extended:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  cs-gpios:
> +    maxItems: 1
> +
> +  firmware-name:
> +    maxItems: 1
> +
> +  apple,z2-device-name:
> +    description: The name to be used for the input device
> +    $ref: /schemas/types.yaml#/definitions/string

Now that I thought about this again after the brief discussion we already had:
Do we even need to specify the device name? Is there any reason we can't just
always use something like "Apple Z2 TouchBar"?



Best,


Sven

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Sasha Finkelstein]

On Fri, 24 Feb 2023 at 12:04, Sven Peter <sven@svenpeter.dev> wrote:
> Now that I thought about this again after the brief discussion we already had:
> Do we even need to specify the device name? Is there any reason we can't just
> always use something like "Apple Z2 TouchBar"?
A similar protocol is used for primary touchscreen on idevices, which
need different
userspace handling. This is to make the driver potentially useful for
people who run
linux on checkra1n-able devices.

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Rob Herring]

On Fri, Feb 24, 2023 at 12:08:42PM +0100, Sasha Finkelstein wrote:
> On Fri, 24 Feb 2023 at 12:04, Sven Peter <sven@svenpeter.dev> wrote:
> > Now that I thought about this again after the brief discussion we already had:
> > Do we even need to specify the device name? Is there any reason we can't just
> > always use something like "Apple Z2 TouchBar"?
> A similar protocol is used for primary touchscreen on idevices, which

Similar? So close, but somehow different.

> need different
> userspace handling. This is to make the driver potentially useful for
> people who run
> linux on checkra1n-able devices.

It is sounding to me like you need different compatibles. Don't try to 
parameterize the differences with properties. Unless you can define all 
the differences up front (hint: you can't).

Rob

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Rob Herring]

On Fri, Feb 24, 2023 at 11:20:06AM +0100, Sasha Finkelstein wrote:
> Add bindings for touchscreen controllers attached using the Z2 protocol.
> Those are present in most Apple devices.
> 
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
>  .../input/touchscreen/apple,z2-touchscreen.yaml    | 81 ++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> new file mode 100644
> index 000000000000..695594494b1e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/apple,z2-touchscreen.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple touchscreens attached using the Z2 protocol.
> +
> +maintainers:
> +  - asahi@lists.linux.dev
> +  - Sasha Finkelstein <fnkl.kernel@gmail.com>
> +
> +description: A series of touschscreen controllers used in Apple products.
> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: apple,z2-touchscreen

Is 'z2' anything other than a touchscreen? If not, '-touchscreen' is 
redundant. If so, then what else is there? You should be describing 
physical devices, not just a protocol for touchscreen.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts-extended:

Just 'interrupts' here. 'interrupts-extended' is implicitly supported.

> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  cs-gpios:

There is a standard way to do GPIO based chip-selects. It happens to be 
'cs-gpios', but this is in the wrong place. It goes in the SPI 
controller node.

> +    maxItems: 1
> +
> +  firmware-name:
> +    maxItems: 1
> +
> +  apple,z2-device-name:
> +    description: The name to be used for the input device
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  touchscreen-size-x: true
> +  touchscreen-size-y: true
> +  spi-max-frequency: true
> +
> +required:
> +  - compatible
> +  - interrupts-extended
> +  - reset-gpios
> +  - cs-gpios
> +  - firmware-name
> +  - apple,z2-device-name
> +  - touchscreen-size-x
> +  - touchscreen-size-y
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;

4 space indentation is preferred here.

> +
> +            touchscreen@0 {
> +                    compatible = "apple,z2-touchscreen";
> +                    reg = <0>;
> +                    spi-max-frequency = <11500000>;
> +                    reset-gpios = <&pinctrl_ap 139 0>;
> +                    cs-gpios = <&pinctrl_ap 109 0>;
> +                    interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
> +                    firmware-name = "apple/dfrmtfw-j293.bin";
> +                    touchscreen-size-x = <23045>;
> +                    touchscreen-size-y = <640>;
> +                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";

Why do we need this string? If you want a human consumed label for 
some identification, we have a property for that purpose. It's called 
'label'. But when there is only 1 instance, I don't really see the 
point.

Rob

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Sasha Finkelstein]

On Mon, 27 Feb 2023 at 20:51, Rob Herring <robh@kernel.org> wrote:
>
> > +properties:
> > +  compatible:
> > +    const: apple,z2-touchscreen
>
> Is 'z2' anything other than a touchscreen? If not, '-touchscreen' is
> redundant. If so, then what else is there? You should be describing
> physical devices, not just a protocol for touchscreen.
>

This is a class of touchscreen controllers that talk the z2 protocol
over spi.

> > +                    touchscreen-size-y = <640>;
> > +                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";
>
> Why do we need this string? If you want a human consumed label for
> some identification, we have a property for that purpose. It's called
> 'label'. But when there is only 1 instance, I don't really see the
> point.

I want a libinput-consumed label to distinguish between devices
using this protocol. It is used both for 'normal' touchscreens, and,
as is in this example a 'touchbar', which absolutely should not be
treated as a normal touchscreen, and needs special handling in
userspace.

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Rob Herring]

On Mon, Feb 27, 2023 at 09:06:28PM +0100, Sasha Finkelstein wrote:
> On Mon, 27 Feb 2023 at 20:51, Rob Herring <robh@kernel.org> wrote:
> >
> > > +properties:
> > > +  compatible:
> > > +    const: apple,z2-touchscreen
> >
> > Is 'z2' anything other than a touchscreen? If not, '-touchscreen' is
> > redundant. If so, then what else is there? You should be describing
> > physical devices, not just a protocol for touchscreen.
> >
> 
> This is a class of touchscreen controllers that talk the z2 protocol
> over spi.

Yes, you already said that much. So nothing else for this piece of h/w? 
Then 'apple,z2' is sufficient. Well maybe. You are assuming all h/w in 
the world speaking 'z2' is the same (to software). Usually that's not a 
safe assumption, but maybe Apple is better at not changing the h/w...

Normally, the 'protocol' to talk to a device is only part of it. There's 
other pieces like how to turn the device on and off which need h/w 
specific knowledge. If you need any of that, then you need specific 
compatibles. Adding properties for each variation doesn't end up well.


> 
> > > +                    touchscreen-size-y = <640>;
> > > +                    apple,z2-device-name = "MacBookPro17,1 Touch Bar";
> >
> > Why do we need this string? If you want a human consumed label for
> > some identification, we have a property for that purpose. It's called
> > 'label'. But when there is only 1 instance, I don't really see the
> > point.
> 
> I want a libinput-consumed label to distinguish between devices
> using this protocol. 

I know little about libinput, but how would it know about 
'apple,z2-device-name'?

> It is used both for 'normal' touchscreens, and,
> as is in this example a 'touchbar', which absolutely should not be
> treated as a normal touchscreen, and needs special handling in
> userspace.

Meaning there are both touchscreens and touchbars using this? That 
sounds like s/w needs this information. From a DT perspective, 
'compatible' is how DT defines exactly what the h/w is and how to use 
it. That also doesn't sound like a unique issue. Doesn't the kernel 
provide a standard way to tell userspace what's a touchscreen vs. 
touchpad vs. ???

Rob

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Sasha Finkelstein]

On Mon, 27 Feb 2023 at 23:14, Rob Herring <robh@kernel.org> wrote:
> I know little about libinput, but how would it know about
> 'apple,z2-device-name'?
>
The idea was to forward the contents of this property
into the input device name.

Re: [PATCH RFC 1/4] dt-bindings: input: touchscreen: Add Z2 controller bindings.

[Hector Martin]

On 28/02/2023 07.25, Sasha Finkelstein wrote:
> On Mon, 27 Feb 2023 at 23:14, Rob Herring <robh@kernel.org> wrote:
>> I know little about libinput, but how would it know about
>> 'apple,z2-device-name'?
>>
> The idea was to forward the contents of this property
> into the input device name.

Then you want "label", as Rob said.

But I also agree with Rob that we want per-device compatibles, even if
we don't use them upfront in the driver. That's what we do everywhere
else, and it has served us well. I suggest this hierarchy:

compatible = "apple,j293-touchbar", "apple,z2-touchbar",
"apple,z2-multitouch";
label = "Apple J293 Touch Bar";

Then let's say a hypothetical touchscreen + touchbar MacBook comes out,
we end up with:

compatible = "apple,j789-touchbar", "apple,z2-touchbar",
"apple,z2-multitouch";
label = "Apple J789 Touch Bar";

compatible = "apple,j789-touchscreen", "apple,z2-touchscreen",
"apple,z2-multitouch";
label = "Apple J789 Touchscreen";

And it all is nicely future-proof. If libinput needs a hint other than
the device name to figure out what should be treated as a touchscreen or
not, the driver can use the "apple,z2-touchbar" vs
"apple,z2-touchscreen" distinction level for that. And if per-device
quirks are never necessary, we just ignore the model number compatible,
which is already what we do all over the place in other drivers (but the
day it becomes necessary, it's ready for us). And if it turns out we
don't need any of this for a while, the driver can just bind to
"apple,z2-multitouch" and call it a day.

- Hector

[PATCH RFC 2/4] input: apple_z2: Add a driver for Apple Z2 touchscreens

[Sasha Finkelstein via B4 Relay]

From: Sasha Finkelstein <fnkl.kernel@gmail.com>

Adds a driver for Apple touchscreens using the Z2 protocol.

Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 drivers/input/touchscreen/Kconfig    |  13 +
 drivers/input/touchscreen/Makefile   |   1 +
 drivers/input/touchscreen/apple_z2.c | 465 +++++++++++++++++++++++++++++++++++
 3 files changed, 479 insertions(+)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 68d99a112e14..76aeea837ab9 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -103,6 +103,19 @@ config TOUCHSCREEN_ADC
 	  To compile this driver as a module, choose M here: the
 	  module will be called resistive-adc-touch.ko.
 
+config TOUCHSCREEN_APPLE_Z2
+	tristate "Apple Z2 touchscreens"
+	default ARCH_APPLE
+	depends on SPI && INPUT && OF
+	help
+	  Say Y here if you have an Apple device with
+	  a touchscreen or a touchbar.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called apple_z2.
+
 config TOUCHSCREEN_AR1021_I2C
 	tristate "Microchip AR1020/1021 i2c touchscreen"
 	depends on I2C && OF
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 4968c370479a..d885b2c4e3a6 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879_I2C)	+= ad7879-i2c.o
 obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI)	+= ad7879-spi.o
 obj-$(CONFIG_TOUCHSCREEN_ADC)		+= resistive-adc-touch.o
 obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
+obj-$(CONFIG_TOUCHSCREEN_APPLE_Z2)	+= apple_z2.o
 obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C)	+= ar1021_i2c.o
 obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT)	+= atmel_mxt_ts.o
 obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR)	+= auo-pixcir-ts.o
diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
new file mode 100644
index 000000000000..06f1d864a137
--- /dev/null
+++ b/drivers/input/touchscreen/apple_z2.c
@@ -0,0 +1,465 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Apple Z2 touchscreen driver
+ *
+ * Copyright (C) The Asahi Linux Contributors
+ */
+
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+
+#define Z2_NUM_FINGERS_OFFSET 16
+#define Z2_FINGERS_OFFSET 24
+#define Z2_TOUCH_STARTED 3
+#define Z2_TOUCH_MOVED 4
+#define Z2_CMD_READ_INTERRUPT_DATA 0xEB
+#define Z2_HBPP_CMD_BLOB 0x3001
+#define Z2_FW_MAGIC 0x5746325A
+#define LOAD_COMMAND_INIT_PAYLOAD 0
+#define LOAD_COMMAND_SEND_BLOB 1
+#define LOAD_COMMAND_SEND_CALIBRATION 2
+
+struct apple_z2 {
+	struct spi_device *spidev;
+	struct gpio_desc *cs_gpio;
+	struct gpio_desc *reset_gpio;
+	struct input_dev *input_dev;
+	struct completion boot_irq;
+	int booted;
+	int counter;
+	int y_size;
+	const char *fw_name;
+	const char *cal_blob;
+	int cal_size;
+};
+
+struct z2_finger {
+	u8 finger;
+	u8 state;
+	__le16 unknown2;
+	__le16 abs_x;
+	__le16 abs_y;
+	__le16 rel_x;
+	__le16 rel_y;
+	__le16 tool_major;
+	__le16 tool_minor;
+	__le16 orientation;
+	__le16 touch_major;
+	__le16 touch_minor;
+	__le16 unused[2];
+	__le16 pressure;
+	__le16 multi;
+} __packed;
+
+struct z2_hbpp_blob_hdr {
+	u16 cmd;
+	u16 len;
+	u32 addr;
+	u16 checksum;
+} __packed;
+
+struct z2_fw_hdr {
+	u32 magic;
+	u32 version;
+} __packed;
+
+struct z2_read_interrupt_cmd {
+	u8 cmd;
+	u8 counter;
+	u8 unused[12];
+	__le16 checksum;
+} __packed;
+
+static void apple_z2_parse_touches(struct apple_z2 *z2, char *msg, size_t msg_len)
+{
+	int i;
+	int nfingers;
+	int slot;
+	int slot_valid;
+	struct z2_finger *fingers;
+
+	if (msg_len <= Z2_NUM_FINGERS_OFFSET)
+		return;
+	nfingers = msg[Z2_NUM_FINGERS_OFFSET];
+	fingers = (struct z2_finger *)(msg + Z2_FINGERS_OFFSET);
+	for (i = 0; i < nfingers; i++) {
+		slot = input_mt_get_slot_by_key(z2->input_dev, fingers[i].finger);
+		if (slot < 0) {
+			dev_warn(&z2->spidev->dev, "unable to get slot for finger");
+			continue;
+		}
+		slot_valid = fingers[i].state == Z2_TOUCH_STARTED ||
+			fingers[i].state == Z2_TOUCH_MOVED;
+		input_mt_slot(z2->input_dev, slot);
+		input_mt_report_slot_state(z2->input_dev, MT_TOOL_FINGER, slot_valid);
+		if (!slot_valid)
+			continue;
+		input_report_abs(z2->input_dev, ABS_MT_POSITION_X,
+				 le16_to_cpu(fingers[i].abs_x));
+		input_report_abs(z2->input_dev, ABS_MT_POSITION_Y,
+				 z2->y_size - le16_to_cpu(fingers[i].abs_y));
+		input_report_abs(z2->input_dev, ABS_MT_WIDTH_MAJOR,
+				 le16_to_cpu(fingers[i].tool_major));
+		input_report_abs(z2->input_dev, ABS_MT_WIDTH_MINOR,
+				 le16_to_cpu(fingers[i].tool_minor));
+		input_report_abs(z2->input_dev, ABS_MT_ORIENTATION,
+				 le16_to_cpu(fingers[i].orientation));
+		input_report_abs(z2->input_dev, ABS_MT_TOUCH_MAJOR,
+				 le16_to_cpu(fingers[i].touch_major));
+		input_report_abs(z2->input_dev, ABS_MT_TOUCH_MINOR,
+				 le16_to_cpu(fingers[i].touch_minor));
+	}
+	input_mt_sync_frame(z2->input_dev);
+	input_sync(z2->input_dev);
+}
+
+static int apple_z2_spi_sync(struct apple_z2 *z2, struct spi_message *msg)
+{
+	int err;
+	gpiod_direction_output(z2->cs_gpio, 0);
+	usleep_range(1000, 2000);
+	err = spi_sync(z2->spidev, msg);
+	usleep_range(1000, 2000);
+	gpiod_direction_output(z2->cs_gpio, 1);
+	return err;
+}
+
+static int apple_z2_read_packet(struct apple_z2 *z2)
+{
+	struct spi_message msg;
+	struct spi_transfer xfer;
+	struct z2_read_interrupt_cmd len_cmd;
+	char len_rx[16];
+	size_t pkt_len;
+	char *pkt_rx;
+	int err = 0;
+
+	spi_message_init(&msg);
+	memset(&xfer, 0, sizeof(xfer));
+	memset(&len_cmd, 0, sizeof(len_cmd));
+	len_cmd.cmd = Z2_CMD_READ_INTERRUPT_DATA;
+	len_cmd.counter = z2->counter + 1;
+	len_cmd.checksum = cpu_to_le16(Z2_CMD_READ_INTERRUPT_DATA + 1 + z2->counter);
+	z2->counter = 1 - z2->counter;
+	xfer.tx_buf = &len_cmd;
+	xfer.rx_buf = len_rx;
+	xfer.len = sizeof(len_cmd);
+	spi_message_add_tail(&xfer, &msg);
+	err = apple_z2_spi_sync(z2, &msg);
+	if (err)
+		return err;
+
+	pkt_len = ((len_rx[1] | len_rx[2] << 8) + 8) & (-4);
+	pkt_rx = kzalloc(pkt_len, GFP_KERNEL);
+	if (!pkt_rx)
+		return -ENOMEM;
+
+	spi_message_init(&msg);
+	xfer.rx_buf = pkt_rx;
+	xfer.len = pkt_len;
+	spi_message_add_tail(&xfer, &msg);
+	err = apple_z2_spi_sync(z2, &msg);
+
+	if (!err)
+		apple_z2_parse_touches(z2, pkt_rx + 5, pkt_len - 5);
+
+	kfree(pkt_rx);
+	return err;
+}
+
+static irqreturn_t apple_z2_irq(int irq, void *data)
+{
+	struct spi_device *spi = data;
+	struct apple_z2 *z2 = spi_get_drvdata(spi);
+
+	if (!z2->booted)
+		complete(&z2->boot_irq);
+	else
+		apple_z2_read_packet(z2);
+
+	return IRQ_HANDLED;
+}
+
+
+// Return value must be freed with kfree
+static char *apple_z2_build_cal_blob(struct apple_z2 *z2, u32 address, u32 *size)
+{
+	u16 len_words = (z2->cal_size + 3) / 4;
+	u32 checksum = 0;
+	u16 checksum_hdr = 0;
+	char *data;
+	int i;
+	struct z2_hbpp_blob_hdr *hdr;
+
+	*size = z2->cal_size + sizeof(struct z2_hbpp_blob_hdr) + 4;
+
+	data = kzalloc(*size, GFP_KERNEL);
+	hdr = (struct z2_hbpp_blob_hdr *)data;
+	hdr->cmd = Z2_HBPP_CMD_BLOB;
+	hdr->len = len_words;
+	hdr->addr = address;
+
+	for (i = 2; i < 8; i++)
+		checksum_hdr += data[i];
+
+	hdr->checksum = checksum_hdr;
+	memcpy(data + 10, z2->cal_blob, z2->cal_size);
+
+	for (i = 0; i < z2->cal_size; i++)
+		checksum += z2->cal_blob[i];
+
+	*(u32 *)(data + z2->cal_size + 10) = checksum;
+
+	return data;
+}
+
+static int apple_z2_send_firmware_blob(struct apple_z2 *z2, const char *data, u32 size, u8 bpw)
+{
+	struct spi_message msg;
+	struct spi_transfer blob_xfer, ack_xfer;
+	char int_ack[] = {0x1a, 0xa1};
+	char ack_rsp[] = {0, 0};
+	int err;
+
+	spi_message_init(&msg);
+	memset(&blob_xfer, 0, sizeof(blob_xfer));
+	memset(&ack_xfer, 0, sizeof(ack_xfer));
+	blob_xfer.tx_buf = data;
+	blob_xfer.len = size;
+	blob_xfer.bits_per_word = bpw;
+	spi_message_add_tail(&blob_xfer, &msg);
+	ack_xfer.tx_buf = int_ack;
+	ack_xfer.rx_buf = ack_rsp;
+	ack_xfer.len = 2;
+	spi_message_add_tail(&ack_xfer, &msg);
+	reinit_completion(&z2->boot_irq);
+	err = apple_z2_spi_sync(z2, &msg);
+	if (err)
+		return err;
+	wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20));
+	return 0;
+}
+
+static int apple_z2_upload_firmware(struct apple_z2 *z2)
+{
+	const struct firmware *fw;
+	struct z2_fw_hdr *fw_hdr;
+	size_t fw_idx = sizeof(struct z2_fw_hdr);
+	int err;
+	u32 load_cmd;
+	u32 size;
+	u32 address;
+	char *data;
+
+	err = request_firmware(&fw, z2->fw_name, &z2->spidev->dev);
+	if (err)
+		return dev_err_probe(&z2->spidev->dev, err, "unable to load firmware");
+
+	fw_hdr = (struct z2_fw_hdr *)fw->data;
+	if (fw_hdr->magic != Z2_FW_MAGIC || fw_hdr->version != 1)
+		return dev_err_probe(&z2->spidev->dev, -EINVAL, "invalid firmware header");
+
+	while (fw_idx < fw->size) {
+		if (fw->size - fw_idx < 8) {
+			err = dev_err_probe(&z2->spidev->dev, -EINVAL, "firmware malformed");
+			goto error;
+		}
+
+		load_cmd = *(u32 *)(fw->data + fw_idx);
+		fw_idx += 4;
+		if (load_cmd == LOAD_COMMAND_INIT_PAYLOAD || load_cmd == LOAD_COMMAND_SEND_BLOB) {
+			size = *(u32 *)(fw->data + fw_idx);
+			fw_idx += 4;
+			if (fw->size - fw_idx < size) {
+				err = dev_err_probe(&z2->spidev->dev,
+						     -EINVAL, "firmware malformed");
+				goto error;
+			}
+			err = apple_z2_send_firmware_blob(z2, fw->data + fw_idx,
+							  size, load_cmd == LOAD_COMMAND_SEND_BLOB ? 16 : 8);
+			if (err)
+				goto error;
+			fw_idx += size;
+		} else if (load_cmd == 2) {
+			address = *(u32 *)(fw->data + fw_idx);
+			fw_idx += 4;
+			data = apple_z2_build_cal_blob(z2, address, &size);
+			err = apple_z2_send_firmware_blob(z2, data, size, 16);
+			kfree(data);
+			if (err)
+				goto error;
+		} else {
+			err = dev_err_probe(&z2->spidev->dev, -EINVAL, "firmware malformed");
+			goto error;
+		}
+		if (fw_idx % 4 != 0)
+			fw_idx += 4 - (fw_idx % 4);
+	}
+
+
+	z2->booted = 1;
+	apple_z2_read_packet(z2);
+ error:
+	release_firmware(fw);
+	return err;
+}
+
+static int apple_z2_boot(struct apple_z2 *z2)
+{
+	int err;
+	enable_irq(z2->spidev->irq);
+	gpiod_direction_output(z2->reset_gpio, 1);
+	wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20));
+	err = apple_z2_upload_firmware(z2);
+	if (err) { // Boot failed, let's put the device into reset just in case.
+		gpiod_direction_output(z2->reset_gpio, 0);
+		disable_irq(z2->spidev->irq);
+	}
+	return err;
+}
+
+static int apple_z2_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct apple_z2 *z2;
+	int err;
+	int x_size;
+	const char *device_name;
+
+	z2 = devm_kzalloc(dev, sizeof(*z2), GFP_KERNEL);
+	if (!z2)
+		return -ENOMEM;
+
+	z2->spidev = spi;
+	init_completion(&z2->boot_irq);
+	spi_set_drvdata(spi, z2);
+
+	z2->cs_gpio = devm_gpiod_get_index(dev, "cs", 0, 0);
+	if (IS_ERR(z2->cs_gpio))
+		return dev_err_probe(dev, PTR_ERR(z2->cs_gpio), "unable to get cs");
+
+	z2->reset_gpio = devm_gpiod_get_index(dev, "reset", 0, 0);
+	if (IS_ERR(z2->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(z2->reset_gpio), "unable to get reset");
+
+	err = devm_request_threaded_irq(dev, z2->spidev->irq, NULL,
+					apple_z2_irq, IRQF_ONESHOT | IRQF_NO_AUTOEN,
+					"apple-z2-irq", spi);
+	if (err < 0)
+		return dev_err_probe(dev, z2->spidev->irq, "unable to request irq");
+
+	err = device_property_read_u32(dev, "touchscreen-size-x", &x_size);
+	if (err)
+		return dev_err_probe(dev, err, "unable to get touchscreen size");
+
+	err = device_property_read_u32(dev, "touchscreen-size-y", &z2->y_size);
+	if (err)
+		return dev_err_probe(dev, err, "unable to get touchscreen size");
+
+	err = device_property_read_string(dev, "apple,z2-device-name", &device_name);
+	if (err)
+		return dev_err_probe(dev, err, "unable to get device name");
+
+	err = device_property_read_string(dev, "firmware-name", &z2->fw_name);
+	if (err)
+		return dev_err_probe(dev, err, "unable to get firmware name");
+
+	z2->cal_blob = of_get_property(dev->of_node, "apple,z2-cal-blob", &z2->cal_size);
+	if (!z2->cal_blob)
+		return dev_err_probe(dev, -EINVAL, "unable to get calibration");
+
+	z2->input_dev = devm_input_allocate_device(dev);
+	if (!z2->input_dev)
+		return -ENOMEM;
+	z2->input_dev->name = device_name;
+	z2->input_dev->phys = "apple_z2";
+	z2->input_dev->dev.parent = dev;
+	z2->input_dev->id.bustype = BUS_SPI;
+	input_set_abs_params(z2->input_dev, ABS_MT_POSITION_X, 0, x_size, 0, 0);
+	input_abs_set_res(z2->input_dev, ABS_MT_POSITION_X, 1);
+	input_set_abs_params(z2->input_dev, ABS_MT_POSITION_Y, 0, z2->y_size, 0, 0);
+	input_abs_set_res(z2->input_dev, ABS_MT_POSITION_Y, 1);
+	input_set_abs_params(z2->input_dev, ABS_MT_WIDTH_MAJOR, 0, 65535, 0, 0);
+	input_set_abs_params(z2->input_dev, ABS_MT_WIDTH_MINOR, 0, 65535, 0, 0);
+	input_set_abs_params(z2->input_dev, ABS_MT_TOUCH_MAJOR, 0, 65535, 0, 0);
+	input_set_abs_params(z2->input_dev, ABS_MT_TOUCH_MINOR, 0, 65535, 0, 0);
+	input_set_abs_params(z2->input_dev, ABS_MT_ORIENTATION, -32768, 32767, 0, 0);
+	input_mt_init_slots(z2->input_dev, 256, INPUT_MT_DIRECT);
+
+	err = input_register_device(z2->input_dev);
+	if (err < 0)
+		return dev_err_probe(dev, err, "unable to register input device");
+
+
+	// Reset the device on probe
+	gpiod_direction_output(z2->reset_gpio, 0);
+	usleep_range(5000, 10000);
+	return apple_z2_boot(z2);
+}
+
+static void apple_z2_remove(struct spi_device *spi)
+{
+	struct apple_z2 *z2 = spi_get_drvdata(spi);
+
+	disable_irq(z2->spidev->irq);
+	gpiod_direction_output(z2->reset_gpio, 0);
+}
+
+static void apple_z2_shutdown(struct spi_device *spi)
+{
+	struct apple_z2 *z2 = spi_get_drvdata(spi);
+
+	disable_irq(z2->spidev->irq);
+	gpiod_direction_output(z2->reset_gpio, 0);
+}
+
+static int apple_z2_suspend(struct device *dev)
+{
+	apple_z2_shutdown(to_spi_device(dev));
+	return 0;
+}
+
+static int apple_z2_resume(struct device *dev)
+{
+	struct apple_z2 *z2 = spi_get_drvdata(to_spi_device(dev));
+
+	return apple_z2_boot(z2);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(apple_z2_pm, apple_z2_suspend, apple_z2_resume);
+
+static const struct of_device_id apple_z2_of_match[] = {
+	{ .compatible = "apple,z2-touchscreen" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, apple_z2_of_match);
+
+static struct spi_device_id apple_z2_of_id[] = {
+	{ .name = "z2-touchscreen" },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, apple_z2_of_id);
+
+static struct spi_driver apple_z2_driver = {
+	.driver = {
+		.name	= "apple-z2",
+		.pm	= pm_sleep_ptr(&apple_z2_pm),
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(apple_z2_of_match),
+	},
+
+	.id_table       = apple_z2_of_id,
+	.probe		= apple_z2_probe,
+	.remove		= apple_z2_remove,
+	.shutdown	= apple_z2_shutdown,
+};
+
+module_spi_driver(apple_z2_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_FIRMWARE("apple/mtfw-*.bin");

-- 
Git-137.1)

Re: [PATCH RFC 2/4] input: apple_z2: Add a driver for Apple Z2 touchscreens

[Jeff LaBundy]

Hi Sasha,

Great work; this is an interesting driver. I have left some comments
throughout.

On Fri, Feb 24, 2023 at 11:20:07AM +0100, Sasha Finkelstein via B4 Relay wrote:
> From: Sasha Finkelstein <fnkl.kernel@gmail.com>
> 
> Adds a driver for Apple touchscreens using the Z2 protocol.
> 
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
>  drivers/input/touchscreen/Kconfig    |  13 +
>  drivers/input/touchscreen/Makefile   |   1 +
>  drivers/input/touchscreen/apple_z2.c | 465 +++++++++++++++++++++++++++++++++++
>  3 files changed, 479 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 68d99a112e14..76aeea837ab9 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -103,6 +103,19 @@ config TOUCHSCREEN_ADC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called resistive-adc-touch.ko.
>  
> +config TOUCHSCREEN_APPLE_Z2
> +	tristate "Apple Z2 touchscreens"
> +	default ARCH_APPLE
> +	depends on SPI && INPUT && OF

There is no need to depend on INPUT; the build will not enter TOUCHSCREEN_*
without it.

I also do not see any need to depend on OF as you are (correctly) using the
device property API. The of_device_id struct toward the end is not gated by
OF either.

> +	help
> +	  Say Y here if you have an Apple device with
> +	  a touchscreen or a touchbar.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called apple_z2.
> +
>  config TOUCHSCREEN_AR1021_I2C
>  	tristate "Microchip AR1020/1021 i2c touchscreen"
>  	depends on I2C && OF
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 4968c370479a..d885b2c4e3a6 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_TOUCHSCREEN_AD7879_I2C)	+= ad7879-i2c.o
>  obj-$(CONFIG_TOUCHSCREEN_AD7879_SPI)	+= ad7879-spi.o
>  obj-$(CONFIG_TOUCHSCREEN_ADC)		+= resistive-adc-touch.o
>  obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
> +obj-$(CONFIG_TOUCHSCREEN_APPLE_Z2)	+= apple_z2.o
>  obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C)	+= ar1021_i2c.o
>  obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT)	+= atmel_mxt_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR)	+= auo-pixcir-ts.o
> diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
> new file mode 100644
> index 000000000000..06f1d864a137
> --- /dev/null
> +++ b/drivers/input/touchscreen/apple_z2.c
> @@ -0,0 +1,465 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Z2 touchscreen driver
> + *
> + * Copyright (C) The Asahi Linux Contributors
> + */
> +
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>

Please sort these #includes alphabetically.

> +
> +#define Z2_NUM_FINGERS_OFFSET 16
> +#define Z2_FINGERS_OFFSET 24
> +#define Z2_TOUCH_STARTED 3
> +#define Z2_TOUCH_MOVED 4
> +#define Z2_CMD_READ_INTERRUPT_DATA 0xEB
> +#define Z2_HBPP_CMD_BLOB 0x3001
> +#define Z2_FW_MAGIC 0x5746325A
> +#define LOAD_COMMAND_INIT_PAYLOAD 0
> +#define LOAD_COMMAND_SEND_BLOB 1
> +#define LOAD_COMMAND_SEND_CALIBRATION 2

In my opinion, #defines are easier to read if the values are aligned vertically.

> +
> +struct apple_z2 {
> +	struct spi_device *spidev;
> +	struct gpio_desc *cs_gpio;
> +	struct gpio_desc *reset_gpio;
> +	struct input_dev *input_dev;
> +	struct completion boot_irq;
> +	int booted;
> +	int counter;
> +	int y_size;
> +	const char *fw_name;
> +	const char *cal_blob;
> +	int cal_size;
> +};
> +
> +struct z2_finger {
> +	u8 finger;
> +	u8 state;
> +	__le16 unknown2;
> +	__le16 abs_x;
> +	__le16 abs_y;
> +	__le16 rel_x;
> +	__le16 rel_y;
> +	__le16 tool_major;
> +	__le16 tool_minor;
> +	__le16 orientation;
> +	__le16 touch_major;
> +	__le16 touch_minor;
> +	__le16 unused[2];
> +	__le16 pressure;
> +	__le16 multi;
> +} __packed;

Please pick one namespace (i.e. apple_z2 or z2) and use it consistently throughout.

> +
> +struct z2_hbpp_blob_hdr {
> +	u16 cmd;
> +	u16 len;
> +	u32 addr;
> +	u16 checksum;
> +} __packed;
> +
> +struct z2_fw_hdr {
> +	u32 magic;
> +	u32 version;
> +} __packed;
> +
> +struct z2_read_interrupt_cmd {
> +	u8 cmd;
> +	u8 counter;
> +	u8 unused[12];
> +	__le16 checksum;
> +} __packed;
> +
> +static void apple_z2_parse_touches(struct apple_z2 *z2, char *msg, size_t msg_len)
> +{
> +	int i;
> +	int nfingers;
> +	int slot;
> +	int slot_valid;
> +	struct z2_finger *fingers;
> +
> +	if (msg_len <= Z2_NUM_FINGERS_OFFSET)
> +		return;
> +	nfingers = msg[Z2_NUM_FINGERS_OFFSET];
> +	fingers = (struct z2_finger *)(msg + Z2_FINGERS_OFFSET);
> +	for (i = 0; i < nfingers; i++) {
> +		slot = input_mt_get_slot_by_key(z2->input_dev, fingers[i].finger);
> +		if (slot < 0) {
> +			dev_warn(&z2->spidev->dev, "unable to get slot for finger");

We need an '\n' at the end of this string.

> +			continue;
> +		}
> +		slot_valid = fingers[i].state == Z2_TOUCH_STARTED ||
> +			fingers[i].state == Z2_TOUCH_MOVED;

This logic was initially a bit hard to parse; I think it would be easier
to read if aligned as follows:

        slot_valid = fingers[i].state == Z2_TOUCH_STARTED ||
                     fingers[i].state == Z2_TOUCH_MOVED;

> +		input_mt_slot(z2->input_dev, slot);
> +		input_mt_report_slot_state(z2->input_dev, MT_TOOL_FINGER, slot_valid);
> +		if (!slot_valid)
> +			continue;
> +		input_report_abs(z2->input_dev, ABS_MT_POSITION_X,
> +				 le16_to_cpu(fingers[i].abs_x));
> +		input_report_abs(z2->input_dev, ABS_MT_POSITION_Y,
> +				 z2->y_size - le16_to_cpu(fingers[i].abs_y));
> +		input_report_abs(z2->input_dev, ABS_MT_WIDTH_MAJOR,
> +				 le16_to_cpu(fingers[i].tool_major));
> +		input_report_abs(z2->input_dev, ABS_MT_WIDTH_MINOR,
> +				 le16_to_cpu(fingers[i].tool_minor));
> +		input_report_abs(z2->input_dev, ABS_MT_ORIENTATION,
> +				 le16_to_cpu(fingers[i].orientation));
> +		input_report_abs(z2->input_dev, ABS_MT_TOUCH_MAJOR,
> +				 le16_to_cpu(fingers[i].touch_major));
> +		input_report_abs(z2->input_dev, ABS_MT_TOUCH_MINOR,
> +				 le16_to_cpu(fingers[i].touch_minor));
> +	}
> +	input_mt_sync_frame(z2->input_dev);
> +	input_sync(z2->input_dev);
> +}
> +
> +static int apple_z2_spi_sync(struct apple_z2 *z2, struct spi_message *msg)
> +{
> +	int err;

Did you not get a compiler warning about there not being a newline between
the declaration and subsequent code?

At any rate, please consider spacing this function out as follows:

        int err;

        gpiod_direction_output(...);
        usleep_range(....);

        err = ...;
        usleep_range(...);

        gpiod_direction_output(...);

        return err;

As it stands, it's difficult to read.

> +	gpiod_direction_output(z2->cs_gpio, 0);
> +	usleep_range(1000, 2000);
> +	err = spi_sync(z2->spidev, msg);
> +	usleep_range(1000, 2000);
> +	gpiod_direction_output(z2->cs_gpio, 1);
> +	return err;
> +}
> +

Thinking about this more, why is the touchscreen driver responsible for
managing its own chip select? Shouldn't the SPI controller driver figure
out on its own if the chosen GPIO can be toggled by the native hardware
peripheral, or bit banged?

It seems the touchscreen driver is absorbing work that is missing in the
SPI controller driver. Please let me know in case I have misunderstood.

> +static int apple_z2_read_packet(struct apple_z2 *z2)
> +{
> +	struct spi_message msg;
> +	struct spi_transfer xfer;
> +	struct z2_read_interrupt_cmd len_cmd;
> +	char len_rx[16];
> +	size_t pkt_len;
> +	char *pkt_rx;
> +	int err = 0;

No need to initialize this to zero.

> +
> +	spi_message_init(&msg);
> +	memset(&xfer, 0, sizeof(xfer));
> +	memset(&len_cmd, 0, sizeof(len_cmd));
> +	len_cmd.cmd = Z2_CMD_READ_INTERRUPT_DATA;
> +	len_cmd.counter = z2->counter + 1;
> +	len_cmd.checksum = cpu_to_le16(Z2_CMD_READ_INTERRUPT_DATA + 1 + z2->counter);
> +	z2->counter = 1 - z2->counter;
> +	xfer.tx_buf = &len_cmd;
> +	xfer.rx_buf = len_rx;
> +	xfer.len = sizeof(len_cmd);
> +	spi_message_add_tail(&xfer, &msg);
> +	err = apple_z2_spi_sync(z2, &msg);
> +	if (err)
> +		return err;

Again, please consider adding some newlines to spread out functionally
separate segments of code.

> +
> +	pkt_len = ((len_rx[1] | len_rx[2] << 8) + 8) & (-4);

We can reduce some of this math with get_unaligned_le16().

> +	pkt_rx = kzalloc(pkt_len, GFP_KERNEL);
> +	if (!pkt_rx)
> +		return -ENOMEM;
> +
> +	spi_message_init(&msg);
> +	xfer.rx_buf = pkt_rx;
> +	xfer.len = pkt_len;
> +	spi_message_add_tail(&xfer, &msg);
> +	err = apple_z2_spi_sync(z2, &msg);
> +
> +	if (!err)
> +		apple_z2_parse_touches(z2, pkt_rx + 5, pkt_len - 5);
> +
> +	kfree(pkt_rx);
> +	return err;
> +}
> +
> +static irqreturn_t apple_z2_irq(int irq, void *data)
> +{
> +	struct spi_device *spi = data;
> +	struct apple_z2 *z2 = spi_get_drvdata(spi);
> +
> +	if (!z2->booted)
> +		complete(&z2->boot_irq);
> +	else
> +		apple_z2_read_packet(z2);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +
> +// Return value must be freed with kfree

If not already, please run checkpatch; it should have barked about
C99-style comments.

That being said, I think it is dangerous to expect the caller to free
the return value; it is ripe for bugs being introduced later in case
others contribute to this driver.

Stated another way, a better method of encapsulation is to limit this
function to working on the data, with the caller being responsible to
manage the memory which holds the data.

> +static char *apple_z2_build_cal_blob(struct apple_z2 *z2, u32 address, u32 *size)
> +{
> +	u16 len_words = (z2->cal_size + 3) / 4;
> +	u32 checksum = 0;
> +	u16 checksum_hdr = 0;
> +	char *data;
> +	int i;
> +	struct z2_hbpp_blob_hdr *hdr;
> +
> +	*size = z2->cal_size + sizeof(struct z2_hbpp_blob_hdr) + 4;
> +
> +	data = kzalloc(*size, GFP_KERNEL);
> +	hdr = (struct z2_hbpp_blob_hdr *)data;
> +	hdr->cmd = Z2_HBPP_CMD_BLOB;
> +	hdr->len = len_words;
> +	hdr->addr = address;
> +
> +	for (i = 2; i < 8; i++)
> +		checksum_hdr += data[i];
> +
> +	hdr->checksum = checksum_hdr;
> +	memcpy(data + 10, z2->cal_blob, z2->cal_size);
> +
> +	for (i = 0; i < z2->cal_size; i++)
> +		checksum += z2->cal_blob[i];
> +
> +	*(u32 *)(data + z2->cal_size + 10) = checksum;
> +
> +	return data;
> +}
> +
> +static int apple_z2_send_firmware_blob(struct apple_z2 *z2, const char *data, u32 size, u8 bpw)
> +{
> +	struct spi_message msg;
> +	struct spi_transfer blob_xfer, ack_xfer;
> +	char int_ack[] = {0x1a, 0xa1};
> +	char ack_rsp[] = {0, 0};
> +	int err;
> +
> +	spi_message_init(&msg);
> +	memset(&blob_xfer, 0, sizeof(blob_xfer));
> +	memset(&ack_xfer, 0, sizeof(ack_xfer));
> +	blob_xfer.tx_buf = data;
> +	blob_xfer.len = size;
> +	blob_xfer.bits_per_word = bpw;
> +	spi_message_add_tail(&blob_xfer, &msg);
> +	ack_xfer.tx_buf = int_ack;
> +	ack_xfer.rx_buf = ack_rsp;
> +	ack_xfer.len = 2;
> +	spi_message_add_tail(&ack_xfer, &msg);
> +	reinit_completion(&z2->boot_irq);
> +	err = apple_z2_spi_sync(z2, &msg);
> +	if (err)
> +		return err;
> +	wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20));
> +	return 0;
> +}

Same comments with regard to whitespace here; this function is difficult
to read and hence review.

> +
> +static int apple_z2_upload_firmware(struct apple_z2 *z2)
> +{
> +	const struct firmware *fw;
> +	struct z2_fw_hdr *fw_hdr;
> +	size_t fw_idx = sizeof(struct z2_fw_hdr);
> +	int err;
> +	u32 load_cmd;
> +	u32 size;
> +	u32 address;
> +	char *data;
> +
> +	err = request_firmware(&fw, z2->fw_name, &z2->spidev->dev);
> +	if (err)
> +		return dev_err_probe(&z2->spidev->dev, err, "unable to load firmware");

In my experience, calling request_firmware() from probe is dangerous
especially if the driver is built-in and not a module. The reason is
the filesystem may not be ready; depending on the platform, you may
get -ENOENT or a deadlock.

If the device has no nonvolatile memory and firmware loading _must_
be kicked off from probe, request_firmware_nowait() with the rest
of the work offloaded to its callback is safer.

> +
> +	fw_hdr = (struct z2_fw_hdr *)fw->data;
> +	if (fw_hdr->magic != Z2_FW_MAGIC || fw_hdr->version != 1)
> +		return dev_err_probe(&z2->spidev->dev, -EINVAL, "invalid firmware header");
> +
> +	while (fw_idx < fw->size) {
> +		if (fw->size - fw_idx < 8) {
> +			err = dev_err_probe(&z2->spidev->dev, -EINVAL, "firmware malformed");
> +			goto error;
> +		}
> +
> +		load_cmd = *(u32 *)(fw->data + fw_idx);
> +		fw_idx += 4;
> +		if (load_cmd == LOAD_COMMAND_INIT_PAYLOAD || load_cmd == LOAD_COMMAND_SEND_BLOB) {
> +			size = *(u32 *)(fw->data + fw_idx);
> +			fw_idx += 4;
> +			if (fw->size - fw_idx < size) {
> +				err = dev_err_probe(&z2->spidev->dev,
> +						     -EINVAL, "firmware malformed");
> +				goto error;
> +			}
> +			err = apple_z2_send_firmware_blob(z2, fw->data + fw_idx,
> +							  size, load_cmd == LOAD_COMMAND_SEND_BLOB ? 16 : 8);
> +			if (err)
> +				goto error;
> +			fw_idx += size;
> +		} else if (load_cmd == 2) {
> +			address = *(u32 *)(fw->data + fw_idx);
> +			fw_idx += 4;
> +			data = apple_z2_build_cal_blob(z2, address, &size);
> +			err = apple_z2_send_firmware_blob(z2, data, size, 16);
> +			kfree(data);
> +			if (err)
> +				goto error;
> +		} else {
> +			err = dev_err_probe(&z2->spidev->dev, -EINVAL, "firmware malformed");
> +			goto error;
> +		}
> +		if (fw_idx % 4 != 0)
> +			fw_idx += 4 - (fw_idx % 4);
> +	}
> +
> +
> +	z2->booted = 1;
> +	apple_z2_read_packet(z2);
> + error:
> +	release_firmware(fw);
> +	return err;
> +}
> +
> +static int apple_z2_boot(struct apple_z2 *z2)
> +{
> +	int err;
> +	enable_irq(z2->spidev->irq);
> +	gpiod_direction_output(z2->reset_gpio, 1);
> +	wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20));
> +	err = apple_z2_upload_firmware(z2);
> +	if (err) { // Boot failed, let's put the device into reset just in case.
> +		gpiod_direction_output(z2->reset_gpio, 0);
> +		disable_irq(z2->spidev->irq);
> +	}
> +	return err;
> +}
> +
> +static int apple_z2_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct apple_z2 *z2;
> +	int err;

In input, return values for functions that only return zero on success tend to
be named 'error'.

> +	int x_size;
> +	const char *device_name;
> +
> +	z2 = devm_kzalloc(dev, sizeof(*z2), GFP_KERNEL);
> +	if (!z2)
> +		return -ENOMEM;
> +
> +	z2->spidev = spi;
> +	init_completion(&z2->boot_irq);
> +	spi_set_drvdata(spi, z2);
> +
> +	z2->cs_gpio = devm_gpiod_get_index(dev, "cs", 0, 0);
> +	if (IS_ERR(z2->cs_gpio))
> +		return dev_err_probe(dev, PTR_ERR(z2->cs_gpio), "unable to get cs");

dev_err_probe() tends not to be accepted in input, the argument being
that the callers who can return -EPROBE_DEFER be responsible for setting
the reason as opposed to every driver calling a separate function that
does so.

> +
> +	z2->reset_gpio = devm_gpiod_get_index(dev, "reset", 0, 0);
> +	if (IS_ERR(z2->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(z2->reset_gpio), "unable to get reset");
> +
> +	err = devm_request_threaded_irq(dev, z2->spidev->irq, NULL,
> +					apple_z2_irq, IRQF_ONESHOT | IRQF_NO_AUTOEN,
> +					"apple-z2-irq", spi);
> +	if (err < 0)
> +		return dev_err_probe(dev, z2->spidev->irq, "unable to request irq");
> +
> +	err = device_property_read_u32(dev, "touchscreen-size-x", &x_size);
> +	if (err)
> +		return dev_err_probe(dev, err, "unable to get touchscreen size");
> +
> +	err = device_property_read_u32(dev, "touchscreen-size-y", &z2->y_size);
> +	if (err)
> +		return dev_err_probe(dev, err, "unable to get touchscreen size");

Please use touchscreen_parse_properties() instead of hard-coding these.

> +
> +	err = device_property_read_string(dev, "apple,z2-device-name", &device_name);
> +	if (err)
> +		return dev_err_probe(dev, err, "unable to get device name");
> +
> +	err = device_property_read_string(dev, "firmware-name", &z2->fw_name);
> +	if (err)
> +		return dev_err_probe(dev, err, "unable to get firmware name");
> +
> +	z2->cal_blob = of_get_property(dev->of_node, "apple,z2-cal-blob", &z2->cal_size);
> +	if (!z2->cal_blob)
> +		return dev_err_probe(dev, -EINVAL, "unable to get calibration");
> +
> +	z2->input_dev = devm_input_allocate_device(dev);
> +	if (!z2->input_dev)
> +		return -ENOMEM;
> +	z2->input_dev->name = device_name;
> +	z2->input_dev->phys = "apple_z2";
> +	z2->input_dev->dev.parent = dev;
> +	z2->input_dev->id.bustype = BUS_SPI;
> +	input_set_abs_params(z2->input_dev, ABS_MT_POSITION_X, 0, x_size, 0, 0);
> +	input_abs_set_res(z2->input_dev, ABS_MT_POSITION_X, 1);
> +	input_set_abs_params(z2->input_dev, ABS_MT_POSITION_Y, 0, z2->y_size, 0, 0);
> +	input_abs_set_res(z2->input_dev, ABS_MT_POSITION_Y, 1);
> +	input_set_abs_params(z2->input_dev, ABS_MT_WIDTH_MAJOR, 0, 65535, 0, 0);
> +	input_set_abs_params(z2->input_dev, ABS_MT_WIDTH_MINOR, 0, 65535, 0, 0);
> +	input_set_abs_params(z2->input_dev, ABS_MT_TOUCH_MAJOR, 0, 65535, 0, 0);
> +	input_set_abs_params(z2->input_dev, ABS_MT_TOUCH_MINOR, 0, 65535, 0, 0);
> +	input_set_abs_params(z2->input_dev, ABS_MT_ORIENTATION, -32768, 32767, 0, 0);
> +	input_mt_init_slots(z2->input_dev, 256, INPUT_MT_DIRECT);

Please check the return value of input_mt_init_slots().

> +
> +	err = input_register_device(z2->input_dev);
> +	if (err < 0)
> +		return dev_err_probe(dev, err, "unable to register input device");
> +
> +
> +	// Reset the device on probe
> +	gpiod_direction_output(z2->reset_gpio, 0);

Technically you are de-asserting reset here, because gpiod is a logical API. A
'0' means take _out_ of reset. This means the reset GPIO's polarity in the dts
is backwards as well.

That being said, this specific call doesn't seem to be necessary since the value
matches the initial value in devm_gpiod_get_index(). So this call can be dropped,
and the polarity needs to be fixed in the call to devm_gpiod_get_index().

> +	usleep_range(5000, 10000);
> +	return apple_z2_boot(z2);
> +}
> +
> +static void apple_z2_remove(struct spi_device *spi)
> +{
> +	struct apple_z2 *z2 = spi_get_drvdata(spi);
> +
> +	disable_irq(z2->spidev->irq);
> +	gpiod_direction_output(z2->reset_gpio, 0);

Same here; the polarity is wrong here. This should be set to 1, with the polarity
corrected in dts.

> +}
> +
> +static void apple_z2_shutdown(struct spi_device *spi)
> +{
> +	struct apple_z2 *z2 = spi_get_drvdata(spi);
> +
> +	disable_irq(z2->spidev->irq);
> +	gpiod_direction_output(z2->reset_gpio, 0);

And here.

> +}
> +
> +static int apple_z2_suspend(struct device *dev)
> +{
> +	apple_z2_shutdown(to_spi_device(dev));
> +	return 0;
> +}
> +
> +static int apple_z2_resume(struct device *dev)
> +{
> +	struct apple_z2 *z2 = spi_get_drvdata(to_spi_device(dev));
> +
> +	return apple_z2_boot(z2);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(apple_z2_pm, apple_z2_suspend, apple_z2_resume);
> +
> +static const struct of_device_id apple_z2_of_match[] = {
> +	{ .compatible = "apple,z2-touchscreen" },
> +	{},

No need for a comma after the sentinel because we would never add a line
below it.

> +};
> +MODULE_DEVICE_TABLE(of, apple_z2_of_match);
> +
> +static struct spi_device_id apple_z2_of_id[] = {
> +	{ .name = "z2-touchscreen" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, apple_z2_of_id);
> +
> +static struct spi_driver apple_z2_driver = {
> +	.driver = {
> +		.name	= "apple-z2",
> +		.pm	= pm_sleep_ptr(&apple_z2_pm),
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(apple_z2_of_match),
> +	},
> +

Nit: extraneous newline.

> +	.id_table       = apple_z2_of_id,
> +	.probe		= apple_z2_probe,
> +	.remove		= apple_z2_remove,
> +	.shutdown	= apple_z2_shutdown,
> +};
> +
> +module_spi_driver(apple_z2_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_FIRMWARE("apple/mtfw-*.bin");
> 
> -- 
> Git-137.1)
> 

Kind regards,
Jeff LaBundy

[PATCH RFC 3/4] arm64: dts: apple: t8103: Add touchbar bindings

[Sasha Finkelstein via B4 Relay]

From: Sasha Finkelstein <fnkl.kernel@gmail.com>

Adds device tree entries for the touchbar digitizer

Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 arch/arm64/boot/dts/apple/t8103-j293.dts | 20 ++++++++++++++++++++
 arch/arm64/boot/dts/apple/t8103.dtsi     | 12 ++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/arch/arm64/boot/dts/apple/t8103-j293.dts b/arch/arm64/boot/dts/apple/t8103-j293.dts
index 151074109a11..c369493b1c06 100644
--- a/arch/arm64/boot/dts/apple/t8103-j293.dts
+++ b/arch/arm64/boot/dts/apple/t8103-j293.dts
@@ -15,6 +15,9 @@
 / {
 	compatible = "apple,j293", "apple,t8103", "apple,arm-platform";
 	model = "Apple MacBook Pro (13-inch, M1, 2020)";
+	aliases {
+		touchbar0 = &touchbar0;
+	};
 };
 
 &bluetooth0 {
@@ -25,6 +28,23 @@ &wifi0 {
 	brcm,board-type = "apple,honshu";
 };
 
+&spi0 {
+	status = "okay";
+
+	touchbar0: touchbar@0 {
+		compatible = "apple,z2-touchscreen";
+		reg = <0>;
+		spi-max-frequency = <11500000>;
+		reset-gpios = <&pinctrl_ap 139 0>;
+		cs-gpios = <&pinctrl_ap 109 0>;
+		interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
+		firmware-name = "apple/dfrmtfw-j293.bin";
+		touchscreen-size-x = <23045>;
+		touchscreen-size-y = <640>;
+		apple,z2-device-name = "MacBookPro17,1 Touch Bar";
+	};
+};
+
 /*
  * Remove unused PCIe ports and disable the associated DARTs.
  */
diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index 9859219699f4..70d3183f72bf 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -432,6 +432,18 @@ i2c4: i2c@235020000 {
 			status = "disabled"; /* only used in J293 */
 		};
 
+		spi0: spi@235100000 {
+			compatible = "apple,t8103-spi", "apple,spi";
+			reg = <0x2 0x35100000 0x0 0x4000>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 614 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clk_200m>;
+			power-domains = <&ps_spi0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled"; /* only used in J293 */
+		};
+
 		serial0: serial@235200000 {
 			compatible = "apple,s5l-uart";
 			reg = <0x2 0x35200000 0x0 0x1000>;

-- 
Git-137.1)

[PATCH RFC 4/4] MAINTAINERS: Add entries for Apple Z2 touchscreen driver

[Sasha Finkelstein via B4 Relay]

From: Sasha Finkelstein <fnkl.kernel@gmail.com>

Add the MAINTAINERS entries for the driver

Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 135d93368d36..12811aa1bd01 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1958,6 +1958,7 @@ F:	Documentation/devicetree/bindings/clock/apple,nco.yaml
 F:	Documentation/devicetree/bindings/cpufreq/apple,cluster-cpufreq.yaml
 F:	Documentation/devicetree/bindings/dma/apple,admac.yaml
 F:	Documentation/devicetree/bindings/i2c/apple,i2c.yaml
+F:	Documentation/devicetree/bindings/input/touchscreen/apple,z2-touchscreen.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,*
 F:	Documentation/devicetree/bindings/iommu/apple,dart.yaml
 F:	Documentation/devicetree/bindings/iommu/apple,sart.yaml
@@ -1976,6 +1977,7 @@ F:	drivers/cpufreq/apple-soc-cpufreq.c
 F:	drivers/dma/apple-admac.c
 F:	drivers/i2c/busses/i2c-pasemi-core.c
 F:	drivers/i2c/busses/i2c-pasemi-platform.c
+F:	drivers/input/touchscreen/apple_z2.c
 F:	drivers/iommu/apple-dart.c
 F:	drivers/iommu/io-pgtable-dart.c
 F:	drivers/irqchip/irq-apple-aic.c

-- 
Git-137.1)

Re: [PATCH RFC 0/4] Driver for Apple Z2 touchscreens.

[Hector Martin]

On 24/02/2023 19.20, Sasha Finkelstein via B4 Relay wrote:
> Hi.
> 
> This series adds support for Apple touchscreens using the Z2 protocol.
> Those are used as the primary touchscreen on mobile Apple devices, and for the
> touchbar on laptops using the M-series chips. (T1/T2 laptops have a coprocessor
> in charge of speaking Z2 to the touchbar).
> 
> Sending this as a RFC for now, since this series requires the SPI controller
> support which is not upstream yet:
> https://lore.kernel.org/all/20211212034726.26306-1-marcan@marcan.st/
> 
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>

Just FWIW, I'm happy to see this get RFCed early (early review is always
good), but I don't think we should submit it until it actually gets some
testing in real-world scenarios. That is naturally blocked on having a
MIPI display driver, since a touchscreen above a dead screen isn't
terribly useful. Otherwise, we might realize that we have some
binding/API issues we missed that are harder to fix after the fact.

So let's hold off on submission proper until we have the screen working
and some basic userspace tooling to go with all this shipping downstream
and it gets some real-world testing. Hopefully by then I'll have sent
out the SPI controller driver too :)

- Hector