Re: [PATCH 0/2] Add Apple SPI keyboard and trackpad driver

[Life is hard, and then you die]

  Hi Henrik,

On Mon, Feb 04, 2019 at 09:47:55PM +0100, Henrik Rydberg wrote:
> Hi Ronald,
> 
> > This changeset adds a driver for the SPI keyboard and trackpad on recent
> > MacBook's and MacBook Pro's. The driver has seen a fair amount of use
> > over the last 2 years (basically anybody running linux on these
> > machines), with only relatively small changes in the last year or so.
> > For those interested, the driver development has been hosted at
> > https://github.com/cb22/macbook12-spi-driver/  (as well as my clone at
> > https://github.com/roadrunner2/macbook12-spi-driver/).
> > 
> > The first patch is just a placeholder for now and is provided in case
> > somebody wants to compile the driver while it's being reviewed here; the
> > real patch has been submitted to dri-devel and is being discussed there,
> > with the intent/hope that I can get an Ack and permission to merge it
> > through the input subsystem tree here as part of this patch series.
> 
> Great to see this upstream. The patch obviously has a lot in common with the
> previous keyboard and touchpad drivers; foremost this is a change of
> transport protocol and not functionality. That said, the code is compact and
> clear enough to make it hard to motivate any major effort to share more of
> existing code, at least initially.

Yes, some pieces have been copy-pasted from the existing drivers.
However, when I last reviewed those pieces they seemed a bit small and
I had a hard time seeing how to share them usefully at least for some
of it.

The pieces in question on the keyboard side (from the hid-apple
driver) are really the 'applespi_fn_codes' and 'apple_iso_keyboard'
tables, the corresponding 'applespi_find_translation()' function, and
some bits in the of the 'applespi_code_to_key()' function. Pulling out
the tables and maybe the applespi_find_translation() function into a
common include might be a simple change and take care of most of the
shared stuff.

A few lines were also lifted from the bcm5974 driver, basically the
'struct tp_finger' and the 'report_tp_state()' function. Though here
it's even harder to see how to share, as there are various small
differences scattered throughout the implemenation of that function.

> Barring detailed comments that are likely
> to produce new revisions, this looks like really good work.

Thank you for looking at this.


  Cheers,

  Ronald