| Message ID | 20241126-z2-v1-0-c43c4cc6200d@gmail.com |
|---|---|
| Headers | show |
| Series | Driver for Apple Z2 touchscreens. | expand |
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.
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.
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.
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.
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 >
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
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.
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.
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
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
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
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.
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?
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
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
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
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.
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/
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