lkml.org 
[lkml]   [2019]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 2/2] Input: add Apple SPI keyboard and trackpad driver.

Hi Andy,

On Mon, Apr 15, 2019 at 12:03:46PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 15, 2019 at 01:13:00AM -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.
>
> Thank you for an update.
> I suddenly realized couple of places where something maybe optimized.
>
> Nevertheless, FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Many thanks for all your reviews!

[snip]
> > +static void
> > +applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
> > +{
> > + unsigned char tmp;
>
> > + u8 bit = BIT(fnremap - 1);
>
> The above is UB and I'm sorry I didn't find this earlier.
>
> So, something like this would work
>
> u8 bit = BIT((fnremap - 1) & 0x07);

fnremap is already constrained by the following:

> > +
> > + if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
> > + !applespi_controlcodes[fnremap - 1])
> > + return;

and the array-size of applespi_controlcodes is constrained to the
number of bits in u8 according to this assertion

> > + compiletime_assert(ARRAY_SIZE(applespi_controlcodes) ==
> > + sizeof_field(struct keyboard_protocol, modifiers) * 8,
> > + "applespi_controlcodes has wrong number of entries");

So I don't see that the masking buys anything new.

[snip]
> > + /* check for rollover overflow, which is signalled by all keys == 1 */
> > + for (i = 0; i < MAX_ROLLOVER; i++) {
> > + if (keyboard_protocol->keys_pressed[i] != 1)
> > + break;
> > + }
> > +
> > + if (i == MAX_ROLLOVER) /* all keys were 1 */
> > + return;
>
> Since keys_pressed is an array of byte values, it may be replaced with
> memchr_inv().
>
> > +
> > + /* remap fn key if desired */
> > + applespi_remap_fn_key(keyboard_protocol);
> > +
> > + /* check released keys */
> > + for (i = 0; i < MAX_ROLLOVER; i++) {
> > + for (j = 0; j < MAX_ROLLOVER; j++) {
> > + if (applespi->last_keys_pressed[i] ==
> > + keyboard_protocol->keys_pressed[j])
> > + break;
> > + }
> > +
> > + if (j < MAX_ROLLOVER) /* key is still pressed */
> > + continue;
>
> And memchr() here.

Ah, yes, excellent suggestion. Thanks.


Cheers,

Ronald

\
 
 \ /
  Last update: 2019-04-16 01:10    [W:0.040 / U:0.772 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site