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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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