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

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

Originally sent as a RFC at https://lore.kernel.org/all/20230223-z2-for-ml-v1-0-028f2b85dc15@gmail.com/
The changes since then mostly address the review feedback, but also
add another machine that has this specific controller.

The extra gpio needed to be toggled turned out to be a quirk of the
j293, normal CS is unusable on it, and a gpio has to be used instead.
(j493 does not have this quirk)

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

 .../input/touchscreen/apple,z2-multitouch.yaml     |  83 ++++
 MAINTAINERS                                        |   2 +
 arch/arm64/boot/dts/apple/t8103-j293.dts           |  24 +
 arch/arm64/boot/dts/apple/t8103.dtsi               |  19 +
 arch/arm64/boot/dts/apple/t8112-j493.dts           |  20 +
 arch/arm64/boot/dts/apple/t8112.dtsi               |  14 +
 drivers/input/touchscreen/Kconfig                  |  13 +
 drivers/input/touchscreen/Makefile                 |   1 +
 drivers/input/touchscreen/apple_z2.c               | 495 +++++++++++++++++++++
 9 files changed, 671 insertions(+)
---
base-commit: 9f16d5e6f220661f73b36a4be1b21575651d8833
change-id: 20241124-z2-c012b528ea0d

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

[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-multitouch.yaml     | 83 ++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/apple,z2-multitouch.yaml b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-multitouch.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..0387e2178b91d5658dfd60cb44099a8048dc97df
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/apple,z2-multitouch.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/apple,z2-multitouch.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple touchscreens attached using the Z2 protocol
+
+maintainers:
+  - 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:
+    items:
+      - enum:
+          - apple,j293-touchbar
+          - apple,j493-touchbar
+      - const: apple,z2-touchbar
+      - const: apple,z2-multitouch
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  cs-gpios:
+    maxItems: 1
+    description: |
+      J293 has a hardware quirk where the CS line is unusable and has
+      to the be driven by a GPIO pin instead
+
+  firmware-name:
+    maxItems: 1
+
+  label:
+    maxItems: 1
+
+  touchscreen-size-x: true
+  touchscreen-size-y: true
+
+required:
+  - compatible
+  - interrupts
+  - reset-gpios
+  - firmware-name
+  - label
+  - touchscreen-size-x
+  - touchscreen-size-y
+
+unevaluatedProperties: 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,j293-touchbar", "apple,z2-touchbar",
+                         "apple,z2-multitouch";
+            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>;
+            label = "MacBookPro17,1 Touch Bar";
+        };
+    };
+
+...

-- 
2.47.1

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

[Krzysztof Kozlowski]

On Tue, Nov 26, 2024 at 09:47:59PM +0100, Sasha Finkelstein wrote:
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,j293-touchbar
> +          - apple,j493-touchbar
> +      - const: apple,z2-touchbar
> +      - const: apple,z2-multitouch

What is the meaning of these two last compatibles in the list? What are
these devices?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  cs-gpios:
> +    maxItems: 1
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      J293 has a hardware quirk where the CS line is unusable and has
> +      to the be driven by a GPIO pin instead
> +
> +  firmware-name:
> +    maxItems: 1
> +
> +  label:
> +    maxItems: 1

Why is this needed? I think it is not part of common touchscreen schema.
Drop, devices do not need labels - node name and unit address identify
it. If this is needed for something else, then come with generic
property matching all touchscreens.

> +
> +  touchscreen-size-x: true
> +  touchscreen-size-y: true

Drop these two

> +
> +required:
> +  - compatible
> +  - interrupts
> +  - reset-gpios
> +  - firmware-name
> +  - label
> +  - touchscreen-size-x
> +  - touchscreen-size-y
> +
> +unevaluatedProperties: 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,j293-touchbar", "apple,z2-touchbar",
> +                         "apple,z2-multitouch";
> +            reg = <0>;
> +            spi-max-frequency = <11500000>;
> +            reset-gpios = <&pinctrl_ap 139 0>;
> +            cs-gpios = <&pinctrl_ap 109 0>;

Use proper GPIO bindings constants. You included header for this, I
guess.

> +            interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
> +            firmware-name = "apple/dfrmtfw-j293.bin";
> +            touchscreen-size-x = <23045>;
> +            touchscreen-size-y = <640>;
> +            label = "MacBookPro17,1 Touch Bar";
> +        };
> +    };
> +
> +...
> 
> -- 
> 2.47.1
> 

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

[Krzysztof Kozlowski]

On 27/11/2024 09:46, Krzysztof Kozlowski wrote:
> On Tue, Nov 26, 2024 at 09:47:59PM +0100, Sasha Finkelstein wrote:
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - apple,j293-touchbar
>> +          - apple,j493-touchbar
>> +      - const: apple,z2-touchbar
>> +      - const: apple,z2-multitouch
> 
> What is the meaning of these two last compatibles in the list? What are
> these devices?


Previous Rob's comment apply here as well. If z2 is protocol, then
multitouch and touchbar do not feel appropriate, unless these are some
subsets of the protocol. But as in other cases no one knows here what's
there inside, so avoid making generic compatibles. Just
apple,j293-touchbar and 493+293. That's the recommendation we keep
insisting on almost always.

As Rob explained: protocol does not matter in terms of compatible. We do
not have devices like "analog,j293-spi" (and there is clear NAK when
people post them, with one or two exceptions).

> 
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +
>> +  cs-gpios:
>> +    maxItems: 1
>> +    description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
>> +      J293 has a hardware quirk where the CS line is unusable and has
>> +      to the be driven by a GPIO pin instead
>> +
>> +  firmware-name:
>> +    maxItems: 1
>> +
>> +  label:
>> +    maxItems: 1
> 
> Why is this needed? I think it is not part of common touchscreen schema.
> Drop, devices do not need labels - node name and unit address identify
> it. If this is needed for something else, then come with generic
> property matching all touchscreens.

This is v1 so I did not expect previous talks, but now I dig them out
and there was conclusion: your compatible defines label. You do not have
two of same devices in the DTS to justify it. Drop the property.

Best regards,
Krzysztof

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

[Sasha Finkelstein]

On Wed, 27 Nov 2024 at 09:47, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> What is the meaning of these two last compatibles in the list? What are
> these devices?

Those are generic compatibles for everything that speaks the Z2 protocol
multitouch is used for everything, as this is currently enough for the driver,
while touchbar is for userspace, as touchscreens and touchbars
need different handling. This specific schema was suggested
on the previous version.


> > +  label:
> > +    maxItems: 1
>
> Why is this needed? I think it is not part of common touchscreen schema.
> Drop, devices do not need labels - node name and unit address identify
> it. If this is needed for something else, then come with generic
> property matching all touchscreens.

I want some sort of a property to contain a human readable (ish)
name of this device.

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

[Sasha Finkelstein]

On Wed, 27 Nov 2024 at 11:49, Sasha Finkelstein <fnkl.kernel@gmail.com> wrote:

> > Why is this needed? I think it is not part of common touchscreen schema.
> > Drop, devices do not need labels - node name and unit address identify
> > it. If this is needed for something else, then come with generic
> > property matching all touchscreens.
>
> I want some sort of a property to contain a human readable (ish)
> name of this device.

Actually, nvm, i think i understand it now, you want the labels stored
in the driver and set based on device compatible, right?

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

[Krzysztof Kozlowski]

On 27/11/2024 12:23, Sasha Finkelstein wrote:
> On Wed, 27 Nov 2024 at 11:49, Sasha Finkelstein <fnkl.kernel@gmail.com> wrote:
> 
>>> Why is this needed? I think it is not part of common touchscreen schema.
>>> Drop, devices do not need labels - node name and unit address identify
>>> it. If this is needed for something else, then come with generic
>>> property matching all touchscreens.
>>
>> I want some sort of a property to contain a human readable (ish)
>> name of this device.
> 
> Actually, nvm, i think i understand it now, you want the labels stored
> in the driver and set based on device compatible, right?


Yes, I think this was also suggested last time.

Best regards,
Krzysztof

[PATCH 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: Janne Grunau <j@jannau.net>
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 | 495 +++++++++++++++++++++++++++++++++++
 3 files changed, 509 insertions(+)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 1ac26fc2e3eb94a4f62a49962c25ec853b567a43..4ad5002191f77a17414f9e1494b0afd6447355c0 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
+	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 82bc837ca01e2ee18c5e9c578765d55ef9fab6d4..97a025c6a3770fb80255246eb63c11688ebd79eb 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 0000000000000000000000000000000000000000..9f57708b3d1749bf23db6132d25cbb47fd622a9f
--- /dev/null
+++ b/drivers/input/touchscreen/apple_z2.c
@@ -0,0 +1,495 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Apple Z2 touchscreen driver
+ *
+ * Copyright (C) The Asahi Linux Contributors
+ */
+
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/spi/spi.h>
+#include <linux/unaligned.h>
+
+#define APPLE_Z2_NUM_FINGERS_OFFSET      16
+#define APPLE_Z2_FINGERS_OFFSET          24
+#define APPLE_Z2_TOUCH_STARTED           3
+#define APPLE_Z2_TOUCH_MOVED             4
+#define APPLE_Z2_CMD_READ_INTERRUPT_DATA 0xEB
+#define APPLE_Z2_HBPP_CMD_BLOB           0x3001
+#define APPLE_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 open;
+	int counter;
+	int y_size;
+	const char *fw_name;
+	const char *cal_blob;
+	int cal_size;
+};
+
+struct apple_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 apple_z2_hbpp_blob_hdr {
+	u16 cmd;
+	u16 len;
+	u32 addr;
+	u16 checksum;
+} __packed;
+
+struct apple_z2_fw_hdr {
+	u32 magic;
+	u32 version;
+} __packed;
+
+struct apple_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 apple_z2_finger *fingers;
+
+	if (!z2->open)
+		return;
+	if (msg_len <= APPLE_Z2_NUM_FINGERS_OFFSET)
+		return;
+	nfingers = msg[APPLE_Z2_NUM_FINGERS_OFFSET];
+	fingers = (struct apple_z2_finger *)(msg + APPLE_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\n");
+			continue;
+		}
+		slot_valid = fingers[i].state == APPLE_Z2_TOUCH_STARTED ||
+			     fingers[i].state == APPLE_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 error;
+
+	if (z2->cs_gpio)
+		gpiod_direction_output(z2->cs_gpio, 0);
+
+	error = spi_sync(z2->spidev, msg);
+
+	if (z2->cs_gpio)
+		gpiod_direction_output(z2->cs_gpio, 1);
+
+	return error;
+}
+
+static int apple_z2_read_packet(struct apple_z2 *z2)
+{
+	struct spi_message msg;
+	struct spi_transfer xfer;
+	struct apple_z2_read_interrupt_cmd len_cmd;
+	char len_rx[16];
+	size_t pkt_len;
+	char *pkt_rx;
+	int error;
+
+	spi_message_init(&msg);
+	memset(&xfer, 0, sizeof(xfer));
+	memset(&len_cmd, 0, sizeof(len_cmd));
+
+	len_cmd.cmd = APPLE_Z2_CMD_READ_INTERRUPT_DATA;
+	len_cmd.counter = z2->counter + 1;
+	len_cmd.checksum = cpu_to_le16(APPLE_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);
+	error = apple_z2_spi_sync(z2, &msg);
+	if (error)
+		return error;
+
+	pkt_len = (get_unaligned_le16(len_rx + 1) + 8) & (-4);
+	pkt_rx = kzalloc(pkt_len, GFP_KERNEL);
+	if (!pkt_rx)
+		return -ENOMEM;
+
+	spi_message_init(&msg);
+	memset(&xfer, 0, sizeof(xfer));
+	xfer.rx_buf = pkt_rx;
+	xfer.len = pkt_len;
+
+	spi_message_add_tail(&xfer, &msg);
+	error = apple_z2_spi_sync(z2, &msg);
+
+	if (!error)
+		apple_z2_parse_touches(z2, pkt_rx + 5, pkt_len - 5);
+
+	kfree(pkt_rx);
+	return error;
+}
+
+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;
+}
+
+static void apple_z2_build_cal_blob(struct apple_z2 *z2, u32 address, char *data)
+{
+	u16 len_words = (z2->cal_size + 3) / 4;
+	u32 checksum = 0;
+	u16 checksum_hdr = 0;
+	int i;
+	struct apple_z2_hbpp_blob_hdr *hdr;
+
+	hdr = (struct apple_z2_hbpp_blob_hdr *)data;
+	hdr->cmd = APPLE_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;
+}
+
+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 error;
+
+	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);
+	error = apple_z2_spi_sync(z2, &msg);
+	if (error)
+		return error;
+	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 apple_z2_fw_hdr *fw_hdr;
+	size_t fw_idx = sizeof(struct apple_z2_fw_hdr);
+	int error;
+	u32 load_cmd;
+	u32 size;
+	u32 address;
+	char *data;
+	u8 bits_per_word;
+
+	error = request_firmware(&fw, z2->fw_name, &z2->spidev->dev);
+	if (error) {
+		dev_err(&z2->spidev->dev, "unable to load firmware");
+		return error;
+	}
+
+	fw_hdr = (struct apple_z2_fw_hdr *)fw->data;
+	if (fw_hdr->magic != APPLE_Z2_FW_MAGIC || fw_hdr->version != 1) {
+		dev_err(&z2->spidev->dev, "invalid firmware header");
+		return -EINVAL;
+	}
+
+	while (fw_idx < fw->size) {
+		if (fw->size - fw_idx < 8) {
+			dev_err(&z2->spidev->dev, "firmware malformed");
+			error = -EINVAL;
+			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) {
+				dev_err(&z2->spidev->dev, "firmware malformed");
+				error = -EINVAL;
+				goto error;
+			}
+			bits_per_word = load_cmd == LOAD_COMMAND_SEND_BLOB ? 16 : 8;
+			error = apple_z2_send_firmware_blob(z2, fw->data + fw_idx,
+							    size, bits_per_word);
+			if (error)
+				goto error;
+			fw_idx += size;
+		} else if (load_cmd == 2) {
+			address = *(u32 *)(fw->data + fw_idx);
+			fw_idx += 4;
+			if (z2->cal_size != 0) {
+				size = z2->cal_size + sizeof(struct apple_z2_hbpp_blob_hdr) + 4;
+				data = kzalloc(size, GFP_KERNEL);
+				apple_z2_build_cal_blob(z2, address, data);
+				error = apple_z2_send_firmware_blob(z2, data, size, 16);
+				kfree(data);
+				if (error)
+					goto error;
+			}
+		} else {
+			dev_err(&z2->spidev->dev, "firmware malformed");
+			error = -EINVAL;
+			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 error;
+}
+
+static int apple_z2_boot(struct apple_z2 *z2)
+{
+	int timeout;
+
+	enable_irq(z2->spidev->irq);
+	gpiod_direction_output(z2->reset_gpio, 0);
+	timeout = wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20));
+	if (timeout == 0)
+		return -ETIMEDOUT;
+	return apple_z2_upload_firmware(z2);
+}
+
+static int apple_z2_open(struct input_dev *dev)
+{
+	struct apple_z2 *z2 = input_get_drvdata(dev);
+	int error;
+
+	/* Reset the device on boot */
+	gpiod_direction_output(z2->reset_gpio, 1);
+	usleep_range(5000, 10000);
+	error = apple_z2_boot(z2);
+	if (error) {
+		gpiod_direction_output(z2->reset_gpio, 1);
+		disable_irq(z2->spidev->irq);
+	} else
+		z2->open = 1;
+	return error;
+}
+
+static void apple_z2_close(struct input_dev *dev)
+{
+	struct apple_z2 *z2 = input_get_drvdata(dev);
+
+	disable_irq(z2->spidev->irq);
+	gpiod_direction_output(z2->reset_gpio, 1);
+	z2->open = 0;
+	z2->booted = 0;
+}
+
+static int apple_z2_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct apple_z2 *z2;
+	int error;
+	const char *label;
+	struct touchscreen_properties props;
+
+	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)) {
+		if (PTR_ERR(z2->cs_gpio) != -ENOENT) {
+			dev_err(dev, "unable to get cs");
+			return PTR_ERR(z2->cs_gpio);
+		}
+		z2->cs_gpio = NULL;
+	}
+
+	z2->reset_gpio = devm_gpiod_get_index(dev, "reset", 0, 0);
+	if (IS_ERR(z2->reset_gpio)) {
+		dev_err(dev, "unable to get reset");
+		return PTR_ERR(z2->reset_gpio);
+	}
+
+	error = devm_request_threaded_irq(dev, z2->spidev->irq, NULL,
+					apple_z2_irq, IRQF_ONESHOT | IRQF_NO_AUTOEN,
+					"apple-z2-irq", spi);
+	if (error < 0) {
+		dev_err(dev, "unable to request irq");
+		return z2->spidev->irq;
+	}
+
+	error = device_property_read_string(dev, "label", &label);
+	if (error) {
+		dev_err(dev, "unable to get device name");
+		return error;
+	}
+
+	error = device_property_read_string(dev, "firmware-name", &z2->fw_name);
+	if (error) {
+		dev_err(dev, "unable to get firmware name");
+		return error;
+	}
+
+	z2->cal_blob = of_get_property(dev->of_node, "apple,z2-cal-blob", &z2->cal_size);
+	if (!z2->cal_blob) {
+		dev_warn(dev, "unable to get calibration, precision may be degraded");
+		z2->cal_size = 0;
+	}
+
+	z2->input_dev = devm_input_allocate_device(dev);
+	if (!z2->input_dev)
+		return -ENOMEM;
+	z2->input_dev->name = label;
+	z2->input_dev->phys = "apple_z2";
+	z2->input_dev->dev.parent = dev;
+	z2->input_dev->id.bustype = BUS_SPI;
+	z2->input_dev->open = apple_z2_open;
+	z2->input_dev->close = apple_z2_close;
+
+	/* Allocate the axes before setting from DT */
+	input_set_abs_params(z2->input_dev, ABS_MT_POSITION_X, 0, 0, 0, 0);
+	input_set_abs_params(z2->input_dev, ABS_MT_POSITION_Y, 0, 0, 0, 0);
+	touchscreen_parse_properties(z2->input_dev, true, &props);
+	z2->y_size = props.max_y;
+	input_abs_set_res(z2->input_dev, ABS_MT_POSITION_X, 100);
+	input_abs_set_res(z2->input_dev, ABS_MT_POSITION_Y, 100);
+	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_set_drvdata(z2->input_dev, z2);
+
+	error = input_mt_init_slots(z2->input_dev, 256, INPUT_MT_DIRECT);
+	if (error < 0) {
+		dev_err(dev, "unable to initialize multitouch slots");
+		return error;
+	}
+
+	error = input_register_device(z2->input_dev);
+	if (error < 0)
+		dev_err(dev, "unable to register input device");
+
+	return error;
+}
+
+static const struct of_device_id apple_z2_of_match[] = {
+	{ .compatible = "apple,z2-multitouch" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, apple_z2_of_match);
+
+static struct spi_device_id apple_z2_of_id[] = {
+	{ .name = "j293-touchbar" },
+	{ .name = "j493-touchbar" },
+	{ .name = "z2-touchbar" },
+	{ .name = "z2-multitouch" },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, apple_z2_of_id);
+
+static struct spi_driver apple_z2_driver = {
+	.driver = {
+		.name	= "apple-z2",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(apple_z2_of_match),
+	},
+	.id_table       = apple_z2_of_id,
+	.probe		= apple_z2_probe,
+};
+
+module_spi_driver(apple_z2_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_FIRMWARE("apple/dfrmtfw-*.bin");

-- 
2.47.1

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

[Dmitry Torokhov]

Hi Sasha,

On Tue, Nov 26, 2024 at 09:48:00PM +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: Janne Grunau <j@jannau.net>
> 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 | 495 +++++++++++++++++++++++++++++++++++
>  3 files changed, 509 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1ac26fc2e3eb94a4f62a49962c25ec853b567a43..4ad5002191f77a17414f9e1494b0afd6447355c0 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
> +	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 82bc837ca01e2ee18c5e9c578765d55ef9fab6d4..97a025c6a3770fb80255246eb63c11688ebd79eb 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 0000000000000000000000000000000000000000..9f57708b3d1749bf23db6132d25cbb47fd622a9f
> --- /dev/null
> +++ b/drivers/input/touchscreen/apple_z2.c
> @@ -0,0 +1,495 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple Z2 touchscreen driver
> + *
> + * Copyright (C) The Asahi Linux Contributors
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +
> +#define APPLE_Z2_NUM_FINGERS_OFFSET      16
> +#define APPLE_Z2_FINGERS_OFFSET          24
> +#define APPLE_Z2_TOUCH_STARTED           3
> +#define APPLE_Z2_TOUCH_MOVED             4
> +#define APPLE_Z2_CMD_READ_INTERRUPT_DATA 0xEB
> +#define APPLE_Z2_HBPP_CMD_BLOB           0x3001
> +#define APPLE_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 open;
> +	int counter;
> +	int y_size;
> +	const char *fw_name;
> +	const char *cal_blob;
> +	int cal_size;
> +};
> +
> +struct apple_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 apple_z2_hbpp_blob_hdr {
> +	u16 cmd;
> +	u16 len;
> +	u32 addr;
> +	u16 checksum;

Does this need endianness annotation? It is being sent to the device...

> +} __packed;
> +
> +struct apple_z2_fw_hdr {
> +	u32 magic;
> +	u32 version;
> +} __packed;
> +
> +struct apple_z2_read_interrupt_cmd {
> +	u8 cmd;
> +	u8 counter;
> +	u8 unused[12];
> +	__le16 checksum;
> +} __packed;

Do these need to be packed? They seem to be naturally aligned.

> +
> +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 apple_z2_finger *fingers;
> +
> +	if (!z2->open)
> +		return;

I do not think you need to check this condition (and need to track
whether the device is open or closed). The input core will not be
delivering events for a closed device even if you try sending them.

> +	if (msg_len <= APPLE_Z2_NUM_FINGERS_OFFSET)
> +		return;
> +	nfingers = msg[APPLE_Z2_NUM_FINGERS_OFFSET];
> +	fingers = (struct apple_z2_finger *)(msg + APPLE_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\n");
> +			continue;
> +		}
> +		slot_valid = fingers[i].state == APPLE_Z2_TOUCH_STARTED ||
> +			     fingers[i].state == APPLE_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;

Shorter form:

		if (!input_mt_report_slot_state(...))
			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 error;
> +
> +	if (z2->cs_gpio)
> +		gpiod_direction_output(z2->cs_gpio, 0);
> +
> +	error = spi_sync(z2->spidev, msg);
> +
> +	if (z2->cs_gpio)
> +		gpiod_direction_output(z2->cs_gpio, 1);

As I mentioned the CS quirk needs to be at the SPI controller level.

> +
> +	return error;
> +}
> +
> +static int apple_z2_read_packet(struct apple_z2 *z2)
> +{
> +	struct spi_message msg;
> +	struct spi_transfer xfer;
> +	struct apple_z2_read_interrupt_cmd len_cmd;
> +	char len_rx[16];
> +	size_t pkt_len;
> +	char *pkt_rx;
> +	int error;
> +
> +	spi_message_init(&msg);
> +	memset(&xfer, 0, sizeof(xfer));
> +	memset(&len_cmd, 0, sizeof(len_cmd));
> +
> +	len_cmd.cmd = APPLE_Z2_CMD_READ_INTERRUPT_DATA;
> +	len_cmd.counter = z2->counter + 1;
> +	len_cmd.checksum = cpu_to_le16(APPLE_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);
> +	error = apple_z2_spi_sync(z2, &msg);
> +	if (error)
> +		return error;
> +
> +	pkt_len = (get_unaligned_le16(len_rx + 1) + 8) & (-4);
> +	pkt_rx = kzalloc(pkt_len, GFP_KERNEL);

Please use __free(kfree) cleanup for temp allocations.

> +	if (!pkt_rx)
> +		return -ENOMEM;
> +
> +	spi_message_init(&msg);
> +	memset(&xfer, 0, sizeof(xfer));
> +	xfer.rx_buf = pkt_rx;
> +	xfer.len = pkt_len;
> +
> +	spi_message_add_tail(&xfer, &msg);
> +	error = apple_z2_spi_sync(z2, &msg);
> +
> +	if (!error)
> +		apple_z2_parse_touches(z2, pkt_rx + 5, pkt_len - 5);
> +
> +	kfree(pkt_rx);
> +	return error;
> +}
> +
> +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)

unlikely()?

> +		complete(&z2->boot_irq);
> +	else
> +		apple_z2_read_packet(z2);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void apple_z2_build_cal_blob(struct apple_z2 *z2, u32 address, char *data)
> +{
> +	u16 len_words = (z2->cal_size + 3) / 4;
> +	u32 checksum = 0;
> +	u16 checksum_hdr = 0;
> +	int i;
> +	struct apple_z2_hbpp_blob_hdr *hdr;
> +
> +	hdr = (struct apple_z2_hbpp_blob_hdr *)data;
> +	hdr->cmd = APPLE_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;

Endianness? cpu_to_Xe32() or put_unaligned_Xe32().

> +}
> +
> +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 error;
> +
> +	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;

I think these buffers need to be DMA-safe.

> +	ack_xfer.len = 2;
> +	spi_message_add_tail(&ack_xfer, &msg);
> +
> +	reinit_completion(&z2->boot_irq);
> +	error = apple_z2_spi_sync(z2, &msg);
> +	if (error)
> +		return error;
> +	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 apple_z2_fw_hdr *fw_hdr;
> +	size_t fw_idx = sizeof(struct apple_z2_fw_hdr);
> +	int error;
> +	u32 load_cmd;
> +	u32 size;
> +	u32 address;
> +	char *data;
> +	u8 bits_per_word;
> +
> +	error = request_firmware(&fw, z2->fw_name, &z2->spidev->dev);

Please use __free(firmware) annotation.

> +	if (error) {
> +		dev_err(&z2->spidev->dev, "unable to load firmware");
> +		return error;
> +	}
> +
> +	fw_hdr = (struct apple_z2_fw_hdr *)fw->data;
> +	if (fw_hdr->magic != APPLE_Z2_FW_MAGIC || fw_hdr->version != 1) {
> +		dev_err(&z2->spidev->dev, "invalid firmware header");
> +		return -EINVAL;
> +	}
> +
> +	while (fw_idx < fw->size) {
> +		if (fw->size - fw_idx < 8) {
> +			dev_err(&z2->spidev->dev, "firmware malformed");

Maybe check this before uploading half of it?

> +			error = -EINVAL;
> +			goto error;
> +		}
> +
> +		load_cmd = *(u32 *)(fw->data + fw_idx);

Please review endianness handling of firmware upload.

> +		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) {
> +				dev_err(&z2->spidev->dev, "firmware malformed");
> +				error = -EINVAL;
> +				goto error;
> +			}
> +			bits_per_word = load_cmd == LOAD_COMMAND_SEND_BLOB ? 16 : 8;
> +			error = apple_z2_send_firmware_blob(z2, fw->data + fw_idx,
> +							    size, bits_per_word);
> +			if (error)
> +				goto error;
> +			fw_idx += size;
> +		} else if (load_cmd == 2) {
> +			address = *(u32 *)(fw->data + fw_idx);
> +			fw_idx += 4;
> +			if (z2->cal_size != 0) {
> +				size = z2->cal_size + sizeof(struct apple_z2_hbpp_blob_hdr) + 4;
> +				data = kzalloc(size, GFP_KERNEL);
> +				apple_z2_build_cal_blob(z2, address, data);
> +				error = apple_z2_send_firmware_blob(z2, data, size, 16);
> +				kfree(data);
> +				if (error)
> +					goto error;
> +			}
> +		} else {
> +			dev_err(&z2->spidev->dev, "firmware malformed");
> +			error = -EINVAL;
> +			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 error;
> +}
> +
> +static int apple_z2_boot(struct apple_z2 *z2)
> +{
> +	int timeout;
> +
> +	enable_irq(z2->spidev->irq);
> +	gpiod_direction_output(z2->reset_gpio, 0);
> +	timeout = wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20));
> +	if (timeout == 0)
> +		return -ETIMEDOUT;
> +	return apple_z2_upload_firmware(z2);
> +}
> +
> +static int apple_z2_open(struct input_dev *dev)
> +{
> +	struct apple_z2 *z2 = input_get_drvdata(dev);
> +	int error;
> +
> +	/* Reset the device on boot */
> +	gpiod_direction_output(z2->reset_gpio, 1);
> +	usleep_range(5000, 10000);
> +	error = apple_z2_boot(z2);

Why can't we wait for the boot in probe()? We can mark the driver as
preferring asynchronous probe to not delay the overall boot process.

> +	if (error) {
> +		gpiod_direction_output(z2->reset_gpio, 1);
> +		disable_irq(z2->spidev->irq);
> +	} else
> +		z2->open = 1;
> +	return error;
> +}
> +
> +static void apple_z2_close(struct input_dev *dev)
> +{
> +	struct apple_z2 *z2 = input_get_drvdata(dev);
> +
> +	disable_irq(z2->spidev->irq);
> +	gpiod_direction_output(z2->reset_gpio, 1);
> +	z2->open = 0;
> +	z2->booted = 0;
> +}
> +
> +static int apple_z2_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct apple_z2 *z2;
> +	int error;
> +	const char *label;
> +	struct touchscreen_properties props;
> +
> +	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)) {
> +		if (PTR_ERR(z2->cs_gpio) != -ENOENT) {
> +			dev_err(dev, "unable to get cs");
> +			return PTR_ERR(z2->cs_gpio);
> +		}
> +		z2->cs_gpio = NULL;
> +	}
> +
> +	z2->reset_gpio = devm_gpiod_get_index(dev, "reset", 0, 0);
> +	if (IS_ERR(z2->reset_gpio)) {
> +		dev_err(dev, "unable to get reset");
> +		return PTR_ERR(z2->reset_gpio);
> +	}
> +
> +	error = devm_request_threaded_irq(dev, z2->spidev->irq, NULL,
> +					apple_z2_irq, IRQF_ONESHOT | IRQF_NO_AUTOEN,
> +					"apple-z2-irq", spi);
> +	if (error < 0) {
> +		dev_err(dev, "unable to request irq");
> +		return z2->spidev->irq;
> +	}
> +
> +	error = device_property_read_string(dev, "label", &label);
> +	if (error) {
> +		dev_err(dev, "unable to get device name");
> +		return error;
> +	}
> +
> +	error = device_property_read_string(dev, "firmware-name", &z2->fw_name);
> +	if (error) {
> +		dev_err(dev, "unable to get firmware name");
> +		return error;
> +	}
> +
> +	z2->cal_blob = of_get_property(dev->of_node, "apple,z2-cal-blob", &z2->cal_size);

I would prefer if we used generic device property API instead of
OF-specific one. I know we do not have direct replacement of
of_get_property(), but we need to make a copy anyway, so we can use
device_property_read_u8_array() when we build calibration blob instead
of doing it at probe time.

> +	if (!z2->cal_blob) {
> +		dev_warn(dev, "unable to get calibration, precision may be degraded");
> +		z2->cal_size = 0;
> +	}
> +
> +	z2->input_dev = devm_input_allocate_device(dev);
> +	if (!z2->input_dev)
> +		return -ENOMEM;
> +	z2->input_dev->name = label;
> +	z2->input_dev->phys = "apple_z2";
> +	z2->input_dev->dev.parent = dev;

Not needed because of devm_input_allocate_device(), please drop.

> +	z2->input_dev->id.bustype = BUS_SPI;
> +	z2->input_dev->open = apple_z2_open;
> +	z2->input_dev->close = apple_z2_close;
> +
> +	/* Allocate the axes before setting from DT */
> +	input_set_abs_params(z2->input_dev, ABS_MT_POSITION_X, 0, 0, 0, 0);
> +	input_set_abs_params(z2->input_dev, ABS_MT_POSITION_Y, 0, 0, 0, 0);
> +	touchscreen_parse_properties(z2->input_dev, true, &props);
> +	z2->y_size = props.max_y;

You can use input_abs_get_max() to get max coordinate, no need to store
it separately. OTOH you should use touchscreen_report_pos() and use
device property to indicate that the axis is inverted.

> +	input_abs_set_res(z2->input_dev, ABS_MT_POSITION_X, 100);
> +	input_abs_set_res(z2->input_dev, ABS_MT_POSITION_Y, 100);
> +	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_set_drvdata(z2->input_dev, z2);
> +
> +	error = input_mt_init_slots(z2->input_dev, 256, INPUT_MT_DIRECT);
> +	if (error < 0) {

	if (error) {

> +		dev_err(dev, "unable to initialize multitouch slots");
> +		return error;
> +	}
> +
> +	error = input_register_device(z2->input_dev);
> +	if (error < 0)

	if (error) {

> +		dev_err(dev, "unable to register input device");

		return error;
	}

> +
> +	return error;

	return 0;

> +}
> +
> +static const struct of_device_id apple_z2_of_match[] = {
> +	{ .compatible = "apple,z2-multitouch" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, apple_z2_of_match);
> +
> +static struct spi_device_id apple_z2_of_id[] = {
> +	{ .name = "j293-touchbar" },
> +	{ .name = "j493-touchbar" },
> +	{ .name = "z2-touchbar" },

Why do you need these extra IDs?

> +	{ .name = "z2-multitouch" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, apple_z2_of_id);
> +
> +static struct spi_driver apple_z2_driver = {
> +	.driver = {
> +		.name	= "apple-z2",
> +		.owner = THIS_MODULE,

This is no longer needed, module_spi_driver() will take care of setting
this.

> +		.of_match_table = of_match_ptr(apple_z2_of_match),
> +	},
> +	.id_table       = apple_z2_of_id,
> +	.probe		= apple_z2_probe,
> +};
> +
> +module_spi_driver(apple_z2_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_FIRMWARE("apple/dfrmtfw-*.bin");
> 
> -- 
> 2.47.1
> 
> 

Thanks.

-- 
Dmitry

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

[Sasha Finkelstein]

On Wed, 27 Nov 2024 at 03:22, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > +     u16 checksum;
>
> Does this need endianness annotation? It is being sent to the device...

Both host and device are always little endian, and this whole thing is
using a bespoke Apple protocol, so is unlikely to ever be seen on a BE
machine. But i am not opposed to adding endianness handling.

> > +             slot_valid = fingers[i].state == APPLE_Z2_TOUCH_STARTED ||
> > +                          fingers[i].state == APPLE_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;
>
> Shorter form:
>
>                 if (!input_mt_report_slot_state(...))
>                         continue;

Sorry, but i fail to see how that is shorter, i am setting the slot state to
slot_valid, which is being computed above, so, why not just reuse
that instead of fetching it from input's slot state?

> > +     ack_xfer.tx_buf = int_ack;
> > +     ack_xfer.rx_buf = ack_rsp;
>
> I think these buffers need to be DMA-safe.

Do they? Our spi controller is not capable of doing DMA (yet?)
and instead copies everything into a fifo. But even if it was capable,
wouldn't that be the controller driver's responsibility to dma-map them?

> > +             if (fw->size - fw_idx < 8) {
> > +                     dev_err(&z2->spidev->dev, "firmware malformed");
>
> Maybe check this before uploading half of it?

That would be an extra pass though the firmware file, and the device
is okay with getting reset after a partial firmware upload, there is no
onboard storage that can be corrupted, and we fully reset it on each
boot (or even more often) anyway.

> > +     error = apple_z2_boot(z2);
>
> Why can't we wait for the boot in probe()? We can mark the driver as
> preferring asynchronous probe to not delay the overall boot process.

A comment on previous version of this submission asked not to load
firmware in probe callback, since the fs may be unavailable at that point.

Ack on all other comments, will be fixed for v2.

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

[Dmitry Torokhov]

On Wed, Nov 27, 2024 at 09:24:16AM +0100, Sasha Finkelstein wrote:
> On Wed, 27 Nov 2024 at 03:22, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > +     u16 checksum;
> >
> > Does this need endianness annotation? It is being sent to the device...
> 
> Both host and device are always little endian, and this whole thing is
> using a bespoke Apple protocol, so is unlikely to ever be seen on a BE
> machine. But i am not opposed to adding endianness handling.

In this case the endianness handling will be "free", but will still show
good code hygiene.

> 
> > > +             slot_valid = fingers[i].state == APPLE_Z2_TOUCH_STARTED ||
> > > +                          fingers[i].state == APPLE_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;
> >
> > Shorter form:
> >
> >                 if (!input_mt_report_slot_state(...))
> >                         continue;
> 
> Sorry, but i fail to see how that is shorter, i am setting the slot state to
> slot_valid, which is being computed above, so, why not just reuse
> that instead of fetching it from input's slot state?

You are not fetching anything, input_mt_report_slot_state() simply
returns "true" for active slots. You are saving a line. You can also do

		if (!input_mt_report_slot_state(z2->input_dev, MT_TOOL_FINGER,
					fingers[i].state == APPLE_Z2_TOUCH_STARTED ||
					fingers[i].state == APPLE_Z2_TOUCH_MOVED))
			continue;

> 
> > > +     ack_xfer.tx_buf = int_ack;
> > > +     ack_xfer.rx_buf = ack_rsp;
> >
> > I think these buffers need to be DMA-safe.
> 
> Do they? Our spi controller is not capable of doing DMA (yet?)
> and instead copies everything into a fifo. But even if it was capable,
> wouldn't that be the controller driver's responsibility to dma-map them?

Yes, they do. From include/linux/spi/spi.h:

/**
 * struct spi_transfer - a read/write buffer pair
 * @tx_buf: data to be written (DMA-safe memory), or NULL
 * @rx_buf: data to be read (DMA-safe memory), or NULL

> 
> > > +             if (fw->size - fw_idx < 8) {
> > > +                     dev_err(&z2->spidev->dev, "firmware malformed");
> >
> > Maybe check this before uploading half of it?
> 
> That would be an extra pass though the firmware file, and the device
> is okay with getting reset after a partial firmware upload, there is no
> onboard storage that can be corrupted, and we fully reset it on each
> boot (or even more often) anyway.

OK, please add a comment to that effect.

> 
> > > +     error = apple_z2_boot(z2);
> >
> > Why can't we wait for the boot in probe()? We can mark the driver as
> > preferring asynchronous probe to not delay the overall boot process.
> 
> A comment on previous version of this submission asked not to load
> firmware in probe callback, since the fs may be unavailable at that point.

But why do you assume that the fs will be available at open time? There
is a number of input handlers that serve internal kernel purposes and we
could have more in the future. They will open the device as soon as it
is registered with the input core.

It is up to the system distributor to configure the kernel properly,
including adding needed firmware to the kernel image if they want the
driver to be built-in.

Thanks.

-- 
Dmitry

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

[Krzysztof Kozlowski]

On Tue, Nov 26, 2024 at 09:48:00PM +0100, Sasha Finkelstein wrote:
> +static int apple_z2_boot(struct apple_z2 *z2)
> +{
> +	int timeout;
> +
> +	enable_irq(z2->spidev->irq);
> +	gpiod_direction_output(z2->reset_gpio, 0);
> +	timeout = wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20));
> +	if (timeout == 0)
> +		return -ETIMEDOUT;
> +	return apple_z2_upload_firmware(z2);
> +}
> +
> +static int apple_z2_open(struct input_dev *dev)
> +{
> +	struct apple_z2 *z2 = input_get_drvdata(dev);
> +	int error;
> +
> +	/* Reset the device on boot */
> +	gpiod_direction_output(z2->reset_gpio, 1);
> +	usleep_range(5000, 10000);
> +	error = apple_z2_boot(z2);
> +	if (error) {
> +		gpiod_direction_output(z2->reset_gpio, 1);

This is less readable code. Each function should clean up its own stuff,
so if z2_boot() de-asserted the reset, then z2_boot() should clean up by
asserting again, not expecting the caller to do this.

> +		disable_irq(z2->spidev->irq);
> +	} else
> +		z2->open = 1;
> +	return error;
> +}
> +
> +static void apple_z2_close(struct input_dev *dev)
> +{
> +	struct apple_z2 *z2 = input_get_drvdata(dev);
> +
> +	disable_irq(z2->spidev->irq);
> +	gpiod_direction_output(z2->reset_gpio, 1);
> +	z2->open = 0;
> +	z2->booted = 0;
> +}
> +
> +static int apple_z2_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct apple_z2 *z2;
> +	int error;
> +	const char *label;
> +	struct touchscreen_properties props;
> +
> +	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)) {
> +		if (PTR_ERR(z2->cs_gpio) != -ENOENT) {
> +			dev_err(dev, "unable to get cs");
> +			return PTR_ERR(z2->cs_gpio);
> +		}
> +		z2->cs_gpio = NULL;
> +	}
> +
> +	z2->reset_gpio = devm_gpiod_get_index(dev, "reset", 0, 0);
> +	if (IS_ERR(z2->reset_gpio)) {
> +		dev_err(dev, "unable to get reset");

Syntax is: return dev_err_probe, almost everywhere here.

> +		return PTR_ERR(z2->reset_gpio);
> +	}
> +
> +	error = devm_request_threaded_irq(dev, z2->spidev->irq, NULL,
> +					apple_z2_irq, IRQF_ONESHOT | IRQF_NO_AUTOEN,
> +					"apple-z2-irq", spi);
> +	if (error < 0) {
> +		dev_err(dev, "unable to request irq");
> +		return z2->spidev->irq;
> +	}
> +
> +	error = device_property_read_string(dev, "label", &label);
> +	if (error) {
> +		dev_err(dev, "unable to get device name");
> +		return error;
> +	}
> +
> +	error = device_property_read_string(dev, "firmware-name", &z2->fw_name);
> +	if (error) {
> +		dev_err(dev, "unable to get firmware name");
> +		return error;
> +	}
> +
> +	z2->cal_blob = of_get_property(dev->of_node, "apple,z2-cal-blob", &z2->cal_size);

There is no such property.

You cannot sneak undocumented properties.

> +	if (!z2->cal_blob) {
> +		dev_warn(dev, "unable to get calibration, precision may be degraded");
> +		z2->cal_size = 0;
> +	}
> +
> +	z2->input_dev = devm_input_allocate_device(dev);
> +	if (!z2->input_dev)
> +		return -ENOMEM;
> +	z2->input_dev->name = label;
> +	z2->input_dev->phys = "apple_z2";
> +	z2->input_dev->dev.parent = dev;
> +	z2->input_dev->id.bustype = BUS_SPI;
> +	z2->input_dev->open = apple_z2_open;
> +	z2->input_dev->close = apple_z2_close;
> +
> +	/* Allocate the axes before setting from DT */
> +	input_set_abs_params(z2->input_dev, ABS_MT_POSITION_X, 0, 0, 0, 0);
> +	input_set_abs_params(z2->input_dev, ABS_MT_POSITION_Y, 0, 0, 0, 0);
> +	touchscreen_parse_properties(z2->input_dev, true, &props);
> +	z2->y_size = props.max_y;
> +	input_abs_set_res(z2->input_dev, ABS_MT_POSITION_X, 100);
> +	input_abs_set_res(z2->input_dev, ABS_MT_POSITION_Y, 100);
> +	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_set_drvdata(z2->input_dev, z2);
> +
> +	error = input_mt_init_slots(z2->input_dev, 256, INPUT_MT_DIRECT);
> +	if (error < 0) {
> +		dev_err(dev, "unable to initialize multitouch slots");
> +		return error;
> +	}
> +
> +	error = input_register_device(z2->input_dev);
> +	if (error < 0)
> +		dev_err(dev, "unable to register input device");
> +
> +	return error;
> +}
> +
> +static const struct of_device_id apple_z2_of_match[] = {
> +	{ .compatible = "apple,z2-multitouch" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, apple_z2_of_match);
> +
> +static struct spi_device_id apple_z2_of_id[] = {
> +	{ .name = "j293-touchbar" },
> +	{ .name = "j493-touchbar" },
> +	{ .name = "z2-touchbar" },

You should not need all these above.

> +	{ .name = "z2-multitouch" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, apple_z2_of_id);
> +
> +static struct spi_driver apple_z2_driver = {
> +	.driver = {
> +		.name	= "apple-z2",
> +		.owner = THIS_MODULE,

Drop, this is some very old code. All owners were removed ~10 or more
years ago. This suggests you took some old or poorly maintained driver
as a template, thus you duplicate all the issues we already fixed.

> +		.of_match_table = of_match_ptr(apple_z2_of_match),

Drop of_match_ptr(), you have a warning here.

> +	},
> +	.id_table       = apple_z2_of_id,
> +	.probe		= apple_z2_probe,

Best regards,
Krzysztof

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

[Sasha Finkelstein]

On Wed, 27 Nov 2024 at 10:00, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > +             dev_err(dev, "unable to get reset");
>
> Syntax is: return dev_err_probe, almost everywhere here.

Per discussion on previous version of this series, input asks
dev_err_probe to not be used.

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

[Dmitry Torokhov]

On November 27, 2024 2:19:38 AM PST, Sasha Finkelstein <fnkl.kernel@gmail.com> wrote:
>On Wed, 27 Nov 2024 at 10:00, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> > +             dev_err(dev, "unable to get reset");
>>
>> Syntax is: return dev_err_probe, almost everywhere here.
>
>Per discussion on previous version of this series, input asks
>dev_err_probe to not be used.

They twisted my arm. It's ok to use now. 

Thanks.


-- 
Dmitry

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

[Krzysztof Kozlowski]

On 27/11/2024 11:22, Dmitry Torokhov wrote:
> On November 27, 2024 2:19:38 AM PST, Sasha Finkelstein <fnkl.kernel@gmail.com> wrote:
>> On Wed, 27 Nov 2024 at 10:00, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>> +             dev_err(dev, "unable to get reset");
>>>
>>> Syntax is: return dev_err_probe, almost everywhere here.
>>
>> Per discussion on previous version of this series, input asks
>> dev_err_probe to not be used.
> 
> They twisted my arm. It's ok to use now. 

I am fine if that's the preference for input, although this particular
case without dev_err_probe would be just wrong: possible dmesg flood on
deferral.

Best regards,
Krzysztof

[PATCH 3/4] arm64: dts: apple: Add touchbar digitizer nodes

[Sasha Finkelstein via B4 Relay]

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

Adds device tree entries for the touchbar digitizer

Co-developed-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 arch/arm64/boot/dts/apple/t8103-j293.dts | 24 ++++++++++++++++++++++++
 arch/arm64/boot/dts/apple/t8103.dtsi     | 19 +++++++++++++++++++
 arch/arm64/boot/dts/apple/t8112-j493.dts | 20 ++++++++++++++++++++
 arch/arm64/boot/dts/apple/t8112.dtsi     | 14 ++++++++++++++
 4 files changed, 77 insertions(+)

diff --git a/arch/arm64/boot/dts/apple/t8103-j293.dts b/arch/arm64/boot/dts/apple/t8103-j293.dts
index 56b0c67bfcda321b60c621de092643017693ff91..a1c4e5731f2147121a9845bc9f34d224025fb145 100644
--- a/arch/arm64/boot/dts/apple/t8103-j293.dts
+++ b/arch/arm64/boot/dts/apple/t8103-j293.dts
@@ -28,6 +28,10 @@ led-0 {
 			default-state = "keep";
 		};
 	};
+
+	aliases {
+		touchbar0 = &touchbar0;
+	};
 };
 
 &bluetooth0 {
@@ -38,6 +42,26 @@ &wifi0 {
 	brcm,board-type = "apple,honshu";
 };
 
+&spi0 {
+	status = "okay";
+
+	touchbar0: touchbar@0 {
+		compatible = "apple,j293-touchbar",
+			"apple,z2-touchbar", "apple,z2-multitouch";
+		reg = <0>;
+		spi-max-frequency = <11500000>;
+		spi-cs-setup-delay-ns = <2000>;
+		spi-cs-hold-delay-ns = <2000>;
+		reset-gpios = <&pinctrl_ap 139 GPIO_ACTIVE_LOW>;
+		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>;
+		label = "MacBookPro17,1 Touch Bar";
+	};
+};
+
 &i2c2 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index 9b0dad6b618444ac6b1c9735c50cccfc3965f947..dc72aae3844bf33579f623f0b01abc7de4033af4 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -326,6 +326,13 @@ clkref: clock-ref {
 		clock-output-names = "clkref";
 	};
 
+	clk_200m: clock-200m {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <200000000>;
+		clock-output-names = "clk_200m";
+	};
+
 	/*
 	 * This is a fabulated representation of the input clock
 	 * to NCO since we don't know the true clock tree.
@@ -441,6 +448,18 @@ fpwm1: pwm@235044000 {
 			status = "disabled";
 		};
 
+		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>;
diff --git a/arch/arm64/boot/dts/apple/t8112-j493.dts b/arch/arm64/boot/dts/apple/t8112-j493.dts
index 0ad908349f55406783942735a2e9dad54cda00ec..03fb711b3a1fa767ba70807a6d3404e4d52eb783 100644
--- a/arch/arm64/boot/dts/apple/t8112-j493.dts
+++ b/arch/arm64/boot/dts/apple/t8112-j493.dts
@@ -20,6 +20,7 @@ / {
 	aliases {
 		bluetooth0 = &bluetooth0;
 		wifi0 = &wifi0;
+		touchbar0 = &touchbar0;
 	};
 
 	led-controller {
@@ -67,3 +68,22 @@ &i2c4 {
 &fpwm1 {
 	status = "okay";
 };
+
+&spi3 {
+	status = "okay";
+
+	touchbar0: touchbar@0 {
+		compatible = "apple,j493-touchbar", "apple,z2-touchbar", "apple,z2-multitouch";
+		reg = <0>;
+		label = "Mac14,7 Touch Bar";
+		spi-max-frequency = <8000000>;
+		spi-cs-setup-delay-ns = <2000>;
+		spi-cs-hold-delay-ns = <2000>;
+
+		reset-gpios = <&pinctrl_ap 170 GPIO_ACTIVE_LOW>;
+		interrupts-extended = <&pinctrl_ap 174 IRQ_TYPE_EDGE_FALLING>;
+		firmware-name = "apple/dfrmtfw-j493.bin";
+		touchscreen-size-x = <23045>;
+		touchscreen-size-y = <640>;
+	};
+};
diff --git a/arch/arm64/boot/dts/apple/t8112.dtsi b/arch/arm64/boot/dts/apple/t8112.dtsi
index 1666e6ab250bc0be9b8318e3c8fc903ccd3f3760..977c1ca5e8c1b566bb3876b6619ea8812b98e072 100644
--- a/arch/arm64/boot/dts/apple/t8112.dtsi
+++ b/arch/arm64/boot/dts/apple/t8112.dtsi
@@ -467,6 +467,20 @@ fpwm1: pwm@235044000 {
 			status = "disabled";
 		};
 
+		spi3: spi@23510c000 {
+			compatible = "apple,t8112-spi", "apple,spi";
+			reg = <0x2 0x3510c000 0x0 0x4000>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 751 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clkref>;
+			pinctrl-0 = <&spi3_pins>;
+			pinctrl-names = "default";
+			power-domains = <&ps_spi3>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
 		serial0: serial@235200000 {
 			compatible = "apple,s5l-uart";
 			reg = <0x2 0x35200000 0x0 0x1000>;

-- 
2.47.1

Re: [PATCH 3/4] arm64: dts: apple: Add touchbar digitizer nodes

[Janne Grunau]

On Tue, Nov 26, 2024 at 09:48:01PM +0100, Sasha Finkelstein via B4 Relay wrote:
> From: Sasha Finkelstein <fnkl.kernel@gmail.com>
> 
> Adds device tree entries for the touchbar digitizer
> 
> Co-developed-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
>  arch/arm64/boot/dts/apple/t8103-j293.dts | 24 ++++++++++++++++++++++++
>  arch/arm64/boot/dts/apple/t8103.dtsi     | 19 +++++++++++++++++++
>  arch/arm64/boot/dts/apple/t8112-j493.dts | 20 ++++++++++++++++++++
>  arch/arm64/boot/dts/apple/t8112.dtsi     | 14 ++++++++++++++

The changes in t8103.dtsi and t8112.dtsi conflict with my "Add Apple SPI
controller and spi-nor dt nodes" in
https://lore.kernel.org/asahi/20241102-asahi-spi-dt-v1-0-7ac44c0a88f9@jannau.net/

I think it makes more sense to add the spi controller nodes in one go
instead of piece by piece based on device support.

Janne

Re: [PATCH 3/4] arm64: dts: apple: Add touchbar digitizer nodes

[Krzysztof Kozlowski]

On Tue, Nov 26, 2024 at 09:48:01PM +0100, Sasha Finkelstein wrote:
> Adds device tree entries for the touchbar digitizer
> 
> Co-developed-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
>  arch/arm64/boot/dts/apple/t8103-j293.dts | 24 ++++++++++++++++++++++++
>  arch/arm64/boot/dts/apple/t8103.dtsi     | 19 +++++++++++++++++++
>  arch/arm64/boot/dts/apple/t8112-j493.dts | 20 ++++++++++++++++++++
>  arch/arm64/boot/dts/apple/t8112.dtsi     | 14 ++++++++++++++
>  4 files changed, 77 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/apple/t8103-j293.dts b/arch/arm64/boot/dts/apple/t8103-j293.dts
> index 56b0c67bfcda321b60c621de092643017693ff91..a1c4e5731f2147121a9845bc9f34d224025fb145 100644
> --- a/arch/arm64/boot/dts/apple/t8103-j293.dts
> +++ b/arch/arm64/boot/dts/apple/t8103-j293.dts
> @@ -28,6 +28,10 @@ led-0 {
>  			default-state = "keep";
>  		};
>  	};
> +
> +	aliases {

Do not add nodes to the end, but in appropriate place. Either ordered by
name, as DTS coding style asks, or in logical place matching existing
convention (convention: aliases are always the first node).

> +		touchbar0 = &touchbar0;

Not used, drop.

> +	};
>  };
>  
>  &bluetooth0 {
> @@ -38,6 +42,26 @@ &wifi0 {
>  	brcm,board-type = "apple,honshu";
>  };
>  
> +&spi0 {

Also unusual placement - between 'w' and 'i'... unless you keep here
the second style of sorting (matching DTSI)?

> +	status = "okay";
> +
> +	touchbar0: touchbar@0 {
> +		compatible = "apple,j293-touchbar",
> +			"apple,z2-touchbar", "apple,z2-multitouch";
> +		reg = <0>;
> +		spi-max-frequency = <11500000>;
> +		spi-cs-setup-delay-ns = <2000>;
> +		spi-cs-hold-delay-ns = <2000>;
> +		reset-gpios = <&pinctrl_ap 139 GPIO_ACTIVE_LOW>;
> +		cs-gpios = <&pinctrl_ap 109 0>;

Use proper GPIO flag define.

> +		interrupts-extended = <&pinctrl_ap 194 IRQ_TYPE_EDGE_FALLING>;
> +		firmware-name = "apple/dfrmtfw-j293.bin";
> +		touchscreen-size-x = <23045>;
> +		touchscreen-size-y = <640>;
> +		label = "MacBookPro17,1 Touch Bar";
> +	};
> +};
> +
>  &i2c2 {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
> index 9b0dad6b618444ac6b1c9735c50cccfc3965f947..dc72aae3844bf33579f623f0b01abc7de4033af4 100644
> --- a/arch/arm64/boot/dts/apple/t8103.dtsi
> +++ b/arch/arm64/boot/dts/apple/t8103.dtsi
> @@ -326,6 +326,13 @@ clkref: clock-ref {
>  		clock-output-names = "clkref";
>  	};
>  
> +	clk_200m: clock-200m {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <200000000>;
> +		clock-output-names = "clk_200m";
> +	};
> +
>  	/*
>  	 * This is a fabulated representation of the input clock
>  	 * to NCO since we don't know the true clock tree.
> @@ -441,6 +448,18 @@ fpwm1: pwm@235044000 {
>  			status = "disabled";
>  		};
>  
> +		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>;
> diff --git a/arch/arm64/boot/dts/apple/t8112-j493.dts b/arch/arm64/boot/dts/apple/t8112-j493.dts
> index 0ad908349f55406783942735a2e9dad54cda00ec..03fb711b3a1fa767ba70807a6d3404e4d52eb783 100644
> --- a/arch/arm64/boot/dts/apple/t8112-j493.dts
> +++ b/arch/arm64/boot/dts/apple/t8112-j493.dts
> @@ -20,6 +20,7 @@ / {
>  	aliases {
>  		bluetooth0 = &bluetooth0;
>  		wifi0 = &wifi0;
> +		touchbar0 = &touchbar0;

Do not add to the end of lists/properties/nodes etc, but keep order,
usually alphabetical.  This avoids conflicts or allows conflicting
series to be still merged.  That's a general rule for most of
development (Makefiles, DTS, Kconfigs, lists in DT bindings).

Best regards,
Krzysztof

Re: [PATCH 3/4] arm64: dts: apple: Add touchbar digitizer nodes

[Sasha Finkelstein]

On Wed, 27 Nov 2024 at 09:55, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > +             touchbar0 = &touchbar0;
>
> Not used, drop.

Used by the bootloader to forward calibration data to the correct node.

Re: [PATCH 3/4] arm64: dts: apple: Add touchbar digitizer nodes

[Krzysztof Kozlowski]

On 27/11/2024 11:31, Sasha Finkelstein wrote:
> On Wed, 27 Nov 2024 at 09:55, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> +             touchbar0 = &touchbar0;
>>
>> Not used, drop.
> 
> Used by the bootloader to forward calibration data to the correct node.

I suggest document it with a comment, otherwise each undocumented alias
without in-kernel user is a subject of removal later during cleanup.

Best regards,
Krzysztof

[PATCH 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 e7f0170977013889ca7c39b17727ba36d32e92dc..9f75fff12fa1912b70251cade845d6b3d5e28c48 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2166,6 +2166,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-multitouch.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,*
 F:	Documentation/devicetree/bindings/iommu/apple,dart.yaml
 F:	Documentation/devicetree/bindings/iommu/apple,sart.yaml
@@ -2186,6 +2187,7 @@ F:	drivers/dma/apple-admac.c
 F:	drivers/pmdomain/apple/
 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

-- 
2.47.1

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

[Dmitry Torokhov]

Hi Sasha,

On Tue, Nov 26, 2024 at 09:47:58PM +0100, 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).
> 
> Originally sent as a RFC at https://lore.kernel.org/all/20230223-z2-for-ml-v1-0-028f2b85dc15@gmail.com/
> The changes since then mostly address the review feedback, but also
> add another machine that has this specific controller.
> 
> The extra gpio needed to be toggled turned out to be a quirk of the
> j293, normal CS is unusable on it, and a gpio has to be used instead.
> (j493 does not have this quirk)

I believe this needs to be done at the SPI controller level. See
"cs-gpiods" property in
Documentation/devicetree/bindings/spi/spi-controller.yaml that, as far
as I understand, allows overriding controller's native CS handling with
a GPIO when needed.

Cc-ing Mark Brown.

Thanks.

-- 
Dmitry

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

[Sasha Finkelstein]

On Wed, 27 Nov 2024 at 02:51, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> I believe this needs to be done at the SPI controller level. See
> "cs-gpiods" property in
> Documentation/devicetree/bindings/spi/spi-controller.yaml that, as far
> as I understand, allows overriding controller's native CS handling with
> a GPIO when needed.

I have already tried doing that (adding the relevant gpio as cs-gpios
on the controller)
and for some reason none of my attempts worked. Since there is no hardware
documentation, I can't really tell why, could be possible that we need both
native CS and that gpio, could be memory barrier issues somewhere in
the driver core,
but the method above is the only one i could get to work.

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

[Hector Martin]

On 2024/11/27 17:29, Sasha Finkelstein wrote:
> On Wed, 27 Nov 2024 at 02:51, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> I believe this needs to be done at the SPI controller level. See
>> "cs-gpiods" property in
>> Documentation/devicetree/bindings/spi/spi-controller.yaml that, as far
>> as I understand, allows overriding controller's native CS handling with
>> a GPIO when needed.
> 
> I have already tried doing that (adding the relevant gpio as cs-gpios
> on the controller)
> and for some reason none of my attempts worked. Since there is no hardware
> documentation, I can't really tell why, could be possible that we need both
> native CS and that gpio, could be memory barrier issues somewhere in
> the driver core,
> but the method above is the only one i could get to work.

Are you sure this isn't just a pinmux problem, i.e. the bootloader
initializes the pinmux for hardware CS only on one device and not the other?

See spi3_pins in the DTS in our downstream tree (and the reference from
the SPI controller). If the rest of the SPI pins are already working
properly you can just try with the CS pin (same index as on the gpio
spec). Ideally we'd list the 4 pins, but someone needs to reverse
engineer the mapping with m1n1 gpiola since we don't know what it is.

If it really doesn't work with native CS and proper pinmux then cs-gpios
on the controller should work. If it doesn't something weird is going on
elsewhere. There's only one CS line, needing both makes no sense.

- Hector

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

[Hector Martin]

On 2024/11/28 0:29, Hector Martin wrote:
> 
> 
> On 2024/11/27 17:29, Sasha Finkelstein wrote:
>> On Wed, 27 Nov 2024 at 02:51, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>> I believe this needs to be done at the SPI controller level. See
>>> "cs-gpiods" property in
>>> Documentation/devicetree/bindings/spi/spi-controller.yaml that, as far
>>> as I understand, allows overriding controller's native CS handling with
>>> a GPIO when needed.
>>
>> I have already tried doing that (adding the relevant gpio as cs-gpios
>> on the controller)
>> and for some reason none of my attempts worked. Since there is no hardware
>> documentation, I can't really tell why, could be possible that we need both
>> native CS and that gpio, could be memory barrier issues somewhere in
>> the driver core,
>> but the method above is the only one i could get to work.
> 
> Are you sure this isn't just a pinmux problem, i.e. the bootloader
> initializes the pinmux for hardware CS only on one device and not the other?
> 
> See spi3_pins in the DTS in our downstream tree (and the reference from
> the SPI controller). If the rest of the SPI pins are already working
> properly you can just try with the CS pin (same index as on the gpio
> spec). Ideally we'd list the 4 pins, but someone needs to reverse
> engineer the mapping with m1n1 gpiola since we don't know what it is.
> 
> If it really doesn't work with native CS and proper pinmux then cs-gpios
> on the controller should work. If it doesn't something weird is going on
> elsewhere. There's only one CS line, needing both makes no sense.
> 

Looked into this. The pins are 67=CLK, 68=MOSI, 69=MISO for spi0 on
t8103 (they should be added to pinctrl even though they are already
configured by iBoot, ping Janne).

It does sound like there's no hardware CS for SPI0, so the referenced
GPIO 109 is probably just a GPIO. In that case it should work when
declared as a cs for the SPI controller node. If that doesn't work we
need to figure out why and fix it, not add it into the z2 driver.

- Hector

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

[Janne Grunau]

On Thu, Nov 28, 2024 at 01:20:09AM +0900, Hector Martin wrote:
> 
> 
> On 2024/11/28 0:29, Hector Martin wrote:
> > 
> > 
> > On 2024/11/27 17:29, Sasha Finkelstein wrote:
> >> On Wed, 27 Nov 2024 at 02:51, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >>> I believe this needs to be done at the SPI controller level. See
> >>> "cs-gpiods" property in
> >>> Documentation/devicetree/bindings/spi/spi-controller.yaml that, as far
> >>> as I understand, allows overriding controller's native CS handling with
> >>> a GPIO when needed.
> >>
> >> I have already tried doing that (adding the relevant gpio as cs-gpios
> >> on the controller)
> >> and for some reason none of my attempts worked. Since there is no hardware
> >> documentation, I can't really tell why, could be possible that we need both
> >> native CS and that gpio, could be memory barrier issues somewhere in
> >> the driver core,
> >> but the method above is the only one i could get to work.
> > 
> > Are you sure this isn't just a pinmux problem, i.e. the bootloader
> > initializes the pinmux for hardware CS only on one device and not the other?
> > 
> > See spi3_pins in the DTS in our downstream tree (and the reference from
> > the SPI controller). If the rest of the SPI pins are already working
> > properly you can just try with the CS pin (same index as on the gpio
> > spec). Ideally we'd list the 4 pins, but someone needs to reverse
> > engineer the mapping with m1n1 gpiola since we don't know what it is.
> > 
> > If it really doesn't work with native CS and proper pinmux then cs-gpios
> > on the controller should work. If it doesn't something weird is going on
> > elsewhere. There's only one CS line, needing both makes no sense.
> > 
> 
> Looked into this. The pins are 67=CLK, 68=MOSI, 69=MISO for spi0 on
> t8103 (they should be added to pinctrl even though they are already
> configured by iBoot, ping Janne).

queued for the next revision of my "Add Apple SPI controller and spi-nor
dt nodes" series [0] and imported in the downstream kernel.

thanks
Janne

[0] https://lore.kernel.org/asahi/20241127-asahi-spi-dt-v1-0-907c9447f623@jannau.net/

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

[Krzysztof Kozlowski]

On 26/11/2024 21:47, 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).
> 
> Originally sent as a RFC at https://lore.kernel.org/all/20230223-z2-for-ml-v1-0-028f2b85dc15@gmail.com/
> The changes since then mostly address the review feedback, but also
> add another machine that has this specific controller.
Then this is v2, not v1. Look:

b4 diff '<20241126-z2-v1-0-c43c4cc6200d@gmail.com>'
Grabbing thread from
lore.kernel.org/all/20241126-z2-v1-0-c43c4cc6200d@gmail.com/t.mbox.gz
---
Analyzing 16 messages in the thread
Could not find lower series to compare against.

Also we expect changelog in cover letter or individual patches.

Best regards,
Krzysztof