Re: [PATCH v7 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Life is hard, and then you die]

  Hi Dmitry,

thanks for taking a look at this!

On Tue, Jul 16, 2019 at 08:47:44PM +0200, Dmitry Torokhov wrote:
> Hi Ronald,
> 
> On Fri, Apr 19, 2019 at 01:19:26AM -0700, Ronald Tschalär wrote:
> > The keyboard and trackpad on recent MacBook's (since 8,1) and
> > MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
> > of USB, as previously. The higher level protocol is not publicly
> > documented and hence has been reverse engineered. As a consequence there
> > are still a number of unknown fields and commands. However, the known
> > parts have been working well and received extensive testing and use.
> > 
> > In order for this driver to work, the proper SPI drivers need to be
> > loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
> > for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
> > reason enabling this driver in the config implies enabling the above
> > drivers.
> 
> I applied the patch (but changed __u8 to u8 as that's preferred form for
> inside the kernel, and added error handling for input_mt_init_slots) but
> we need to do some more work on the driver.

Looks good.

> My main issue is with registering touchpad device asynchronously,
> independent from the probe() function. This means (as far as I can tell)
> that any error is not really appropriately handled (as by that time it
> is too late to signal errors from probe()) and devm functions are not
> going to be called, leaving remnants of the resources in memory on
> driver unload. It also brings in issues with suspend/resume (what
> happens if you suspend really quickly while device is not registered
> yet?), etc, etc.

Yes, the lack of error propagation also bothered me a bit when I
introduced the dev-info command, but I thought doing synchronous
I/O operations in a probe function wasn't kosher. Happy to rectify
that though.

> Can we switch to calling DEV_INFO command synchronously from probe()? If
> we are concerned about it taking relatively long time we can always
> annotate the driver as having probe_type = PROBE_PREFER_ASYNCHRONOUS so
> that other devices can be probed simultaneously with applespi.

Normally the dev-info retrieval takes about 15ms - I presume that's
fast enough?

Attached is a patch that does this now (on top of your changes above).
The cancelling of outstanding spi requests in the error case is a bit
ugly (I wish there were an exported spi-flush/spi-wait-for-queue-empty
function or similar), but otherwise it's fairly straightforward.


  Cheers,

  Ronald