On Wed, 27 Nov 2024 at 03:22, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> 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.
