Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
On Mon, Feb 18, 2019 at 01:08:51AM -0800, Life is hard, and then you die wrote:
> On Sat, Feb 16, 2019 at 02:44:26AM +0200, Andy Shevchenko wrote:
> > On Sun, Feb 10, 2019 at 03:18:34AM -0800, Life is hard, and then you die
> > wrote:
> > > On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:
> > Hmm... Usually you put either module_name.dyndbg into kernel command line to
> > enable Dynamic Debug on built-in modules, or modprobe module_name dyndbg if
> > it's built as a module.
>
> Right, but while that works with
>
> applespi.dyndbg=+p
>
> it does not appear to work with
>
> applespi.dyndbg="func applespi_get_spi_settings +p"
>
> (it is parsed correctly, but it just doesn't get applied when the
> module is later loaded - I've not tried to chase this down any further
> than that)
Of course it wouldn't work that way. If you have compiled driver as a module,
you need to supply the parameters during modprobe. Actually it's kinda luck
that +p works for you.
> > > > > + message->counter = applespi->cmd_msg_cntr++ & 0xff;
> > > >
> > > > Usual pattern for this kind of entries is
> > > >
> > > > x = ... % 256;
> > > >
> > > > Btw, isn't 256 is defined somewhere?
> > >
> > > Many things are defined as have a value of 256 :-) But I didn't find any
> > > with the intended semantics; could use (U8_MAX+1), though.
> >
> > What I meant is that I saw in the same driver definition with value of 256.
> > Isn't it about messages?
>
> Ah, right: the packet length is 256 bytes. But the semantics are quite
> different, so IMHO it wouldn't be good to use that here. I.e. I think
> (U8_MAX+1) is still semantically the best.
OK.
> > > > > +/* lifted from the BCM5974 driver */
> > > > > +/* convert 16-bit little endian to signed integer */
> > > > > +static inline int raw2int(__le16 x)
> > > > > +{
> > > > > + return (signed short)le16_to_cpu(x);
> > > > > +}
> > > >
> > > > Perhaps it's time to introduce it inside include/linux/input.h ?
> > >
> > > Perhaps as
> > >
> > > static inline int le16_to_signed_int(__le16 x)
> > >
> > > ? (raw2int seems to ambiguous for a global function)
> >
> > I'm fine. It would need a description though.
>
> After looking at this in more detail I don't think that
> include/linux/input.h is the proper place for this, because input.h
> basically represents the interface to the input core, and as such it
> is devoid of helpers like above or any driver specific definitions.
> Instead I'd like to suggest a new include file specific to the bcm5974
> driver, as follows:
>
> --- /dev/null
> +++ b/include/linux/input/bcm5974.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (C) 2008 Henrik Rydberg (rydb...@euromail.se)
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef _BCM5974_H
> +#define _BCM5974_H
> +
> +#include
> +
> +/**
> + * raw2int - convert a 16-bit little endian value as received from the device
> + * to a signed integer in cpu endianness.
> + * @x: the received value
> + *
> + * Returns the converted value.
> + */
> +static inline int raw2int(__le16 x)
> +{
> + return (signed short)le16_to_cpu(x);
> +}
> +
> +#endif
>
> Thoughts?
I see. Since there is no comment from input maintainers, let's stick with the
initial approach (copy'n'paste to your code with comment from where). Only one
suggestion is to name function for further use without changing it. So, in the
future we may move it to generic header.
> I'll send out v2 of the patches with all the changes soon.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
On Sat, Feb 16, 2019 at 02:44:26AM +0200, Andy Shevchenko wrote:
> On Sun, Feb 10, 2019 at 03:18:34AM -0800, Life is hard, and then you die
> wrote:
> > On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> > > On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:
>
> > > > +#definedebug_print(mask, fmt, ...) \
> > > > + do { \
> > > > + if (debug & mask) \
> > > > + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> > > > + } while (0)
> > > > +
> > > > +#definedebug_print_header(mask) \
> > > > + debug_print(mask, "--- %s ---\n", \
> > > > + applespi_debug_facility(mask))
> > > > +
> > > > +#definedebug_print_buffer(mask, fmt, ...) \
> > > > + do { \
> > > > + if (debug & mask) \
> > > > + print_hex_dump(KERN_DEBUG, pr_fmt(fmt), \
> > > > + DUMP_PREFIX_NONE, 32, 1,
> > > > ##__VA_ARGS__, \
> > > > + false); \
> > > > + } while (0)
> > >
> > > I'm not sure we need all of these... But okay, the driver is
> > > reverse-engineered, perhaps we can drop it later on.
> >
> > These have been extremely useful for debugging; but yes, they are for
> > debugging only. They also explicitly do not use the dynamic-debug
> > facilities because
> > 1. those can't be enabled at a function or line level on the kernel
> > command line (the docs indicate this should work, but it doesn't,
> > or at the very least I've been unable to figure out how, at least
> > for those drivers built as modules)
>
> Hmm... Usually you put either module_name.dyndbg into kernel command line to
> enable Dynamic Debug on built-in modules, or modprobe module_name dyndbg if
> it's built as a module.
Right, but while that works with
applespi.dyndbg=+p
it does not appear to work with
applespi.dyndbg="func applespi_get_spi_settings +p"
(it is parsed correctly, but it just doesn't get applied when the
module is later loaded - I've not tried to chase this down any further
than that)
[snip]
> > > > +static int touchpad_dimensions[4];
> > > > +module_param_array(touchpad_dimensions, int, NULL, 0444);
> > > > +MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the
> > > > touchpad, as x_min,x_max,y_min,y_max .");
> > >
> > > We have some parsers for similar data as in format
> > >
> > > %dx%d%+d%+d ?
> > >
> > > For example, 200x100+20+10.
> >
> > Maybe it's just my grep-foo that is lacking, but I couldn't find
> > anything useful. There is some stuff for framebuffer video modes, but
> > that is too different and not really amenable for use here. OTOH, a
> > simple sscanf(buf, "%dx%d+%d+%d", ...) would parse this just fine
> > without the need for any fancier parser.
> >
> > OTOH, I'm not sure this format is any better: internally we need
> > x/y min/max, not x/y/w/h (though obviously the two are trivially
> > convertable); and for the user it doesn't really matter since they would
> > basically be getting the dimensions from running with debug set to
> > 0x1 and using that output to set this param, i.e. I don't see any
> > inherent advantage of using x/y/w/h.
>
> I would stick with some more or less standard notation. The XxY+W+H is widely
> used in X11 world.
>
> If you have better examples for input devices, choose them. My point that it
> would be better to be a standard to some extent.
I couldn't find anything useful (looked in the kernel and libinput),
so going with XxY+W+H.
[snip]
> > > > + message->counter = applespi->cmd_msg_cntr++ & 0xff;
> > >
> > > Usual pattern for this kind of entries is
> > >
> > > x = ... % 256;
> > >
> > > Btw, isn't 256 is defined somewhere?
> >
> > Many things are defined as have a value of 256 :-) But I didn't find any
> > with the intended semantics; could use (U8_MAX+1), though.
>
> What I meant is that I saw in the same driver definition with value of 256.
> Isn't it about messages?
Ah, right: the packet length is 256 bytes. But the semantics are quite
different, so IMHO it wouldn't be good to use that here. I.e. I think
(U8_MAX+1) is still semantically the best.
[snip]
> > > > +/* lifted from the BCM5974 driver */
> > > > +/* convert 16-bit little endian to signed integer */
> > > > +static inline int raw2int(__le16 x)
> > > > +{
> > > > + return (signed short)le16_to_cpu(x);
> > > > +}
> > >
> > > Perhaps it's time to introduce it inside include/linux/input.h ?
> >
> > Perhaps as
> >
> > static inline int le16_to_signed_int(__le16 x)
> >
> > ? (raw2int seems to ambiguous for a global function)
>
> I'm fine. It would need a description though.
After looking at this in more detail I don't think that
include/linux/input.h is the proper place for this, because input.h
basically represents the interface to the input core, and as such it
is devoid of helpers li
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
On Sun, Feb 10, 2019 at 03:18:34AM -0800, Life is hard, and then you die wrote:
> On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> > On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:
> > > +#define debug_print(mask, fmt, ...) \
> > > + do { \
> > > + if (debug & mask) \
> > > + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> > > + } while (0)
> > > +
> > > +#define debug_print_header(mask) \
> > > + debug_print(mask, "--- %s ---\n", \
> > > + applespi_debug_facility(mask))
> > > +
> > > +#define debug_print_buffer(mask, fmt, ...) \
> > > + do { \
> > > + if (debug & mask) \
> > > + print_hex_dump(KERN_DEBUG, pr_fmt(fmt), \
> > > +DUMP_PREFIX_NONE, 32, 1, ##__VA_ARGS__, \
> > > +false); \
> > > + } while (0)
> >
> > I'm not sure we need all of these... But okay, the driver is
> > reverse-engineered, perhaps we can drop it later on.
>
> These have been extremely useful for debugging; but yes, they are for
> debugging only. They also explicitly do not use the dynamic-debug
> facilities because
> 1. those can't be enabled at a function or line level on the kernel
> command line (the docs indicate this should work, but it doesn't,
> or at the very least I've been unable to figure out how, at least
> for those drivers built as modules)
Hmm... Usually you put either module_name.dyndbg into kernel command line to
enable Dynamic Debug on built-in modules, or modprobe module_name dyndbg if
it's built as a module.
> 2. the expressions to enable them quite brittle (in particular the
> line-based ones) since they (may) change any time the code is
> changed - having stable commands to give to users and put in
> documentation (e.g.
> "echo 0x200 > /sys/module/applespi/parameters/debug") is
> quite valuable.
> 3. In at least two places we log different types of packets in the
> same lines of code - dynamic-debug would only allow enabling or
> disabling logging of all packets, unless we do something like
> create separate functions for logging each type of packet.
> > > +static unsigned int fnmode = 1;
> > > +module_param(fnmode, uint, 0644);
> > > +MODULE_PARM_DESC(fnmode, "Mode of fn key on Apple keyboards (0 =
> > > disabled, [1] = fkeyslast, 2 = fkeysfirst)");
> >
> > fn -> Fn ?
>
> The physical key is actually labelled "fn" (no uppercase). But will
> certainly change it if you still wish.
I think Fn would be suitable (on the computer I'm typing this message the
special keys are all marked in small case, but we don't change HP driver to
state 'ctrl' instead 'Ctrl', for example, do we?).
> > Btw, do we need all these parameters? Can't we do modify behaviour run-time
> > using some Input framework facilities?
>
> I'm not aware of any way to do so. I suppose the event callback could
> be used/extended to send "configuration" data, but that would require
> changes to the input core.
>
> Also, for what it's worth, current drivers such as hid-apple (from
> which 'fnmode' and 'iso_layout' were copied and intentionally kept the
> same) use this approach too.
Hmm... Okay, I leave this to Input maintainers.
> > > +static int touchpad_dimensions[4];
> > > +module_param_array(touchpad_dimensions, int, NULL, 0444);
> > > +MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the
> > > touchpad, as x_min,x_max,y_min,y_max .");
> >
> > We have some parsers for similar data as in format
> >
> > %dx%d%+d%+d ?
> >
> > For example, 200x100+20+10.
>
> Maybe it's just my grep-foo that is lacking, but I couldn't find
> anything useful. There is some stuff for framebuffer video modes, but
> that is too different and not really amenable for use here. OTOH, a
> simple sscanf(buf, "%dx%d+%d+%d", ...) would parse this just fine
> without the need for any fancier parser.
>
> OTOH, I'm not sure this format is any better: internally we need
> x/y min/max, not x/y/w/h (though obviously the two are trivially
> convertable); and for the user it doesn't really matter since they would
> basically be getting the dimensions from running with debug set to
> 0x1 and using that output to set this param, i.e. I don't see any
> inherent advantage of using x/y/w/h.
I would stick with some more or less standard notation. The XxY+W+H is widely
used in X11 world.
If you have better examples for input devices, choose them. My point that it
would be better to be a standard to some extent.
> > > +/**
> > > + * struct keyboard_protocol - keyboard message.
> > > + * message.type = 0x0110, message.length = 0x000a
> > > + *
> > > + * @unknown1:unknown
> > > + * @modifiers: bit-set of modifier/control keys pressed
> > > + * @unknown2:unknown
> > > + * @keys_pressed:the (non-modifier) keys currently pressed
> > > + * @fn_pressed: whether the f
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
Hi Andy,
On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 04, 2019 at 12:19:47AM -0800, 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.
>
> Thanks for doing this. My review below.
Many thanks for your extensive review. Sorry for the delay, managed to
mess up my machine...
I've made most of the changes you suggested - below are my comments
and answers where I have questsions or I think there are good reasons
for the current approach.
> > +/**
>
> I'm not sure it's kernel doc format comment.
Sorry, you mean you're not sure whether this should be a kernel doc vs
a regular comment? Ok, making it a regular comment.
[snip]
> > +#definedebug_print(mask, fmt, ...) \
> > + do { \
> > + if (debug & mask) \
> > + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> > + } while (0)
> > +
> > +#definedebug_print_header(mask) \
> > + debug_print(mask, "--- %s ---\n", \
> > + applespi_debug_facility(mask))
> > +
> > +#definedebug_print_buffer(mask, fmt, ...) \
> > + do { \
> > + if (debug & mask) \
> > + print_hex_dump(KERN_DEBUG, pr_fmt(fmt), \
> > + DUMP_PREFIX_NONE, 32, 1, ##__VA_ARGS__, \
> > + false); \
> > + } while (0)
>
> I'm not sure we need all of these... But okay, the driver is
> reverse-engineered, perhaps we can drop it later on.
These have been extremely useful for debugging; but yes, they are for
debugging only. They also explicitly do not use the dynamic-debug
facilities because
1. those can't be enabled at a function or line level on the kernel
command line (the docs indicate this should work, but it doesn't,
or at the very least I've been unable to figure out how, at least
for those drivers built as modules)
2. the expressions to enable them quite brittle (in particular the
line-based ones) since they (may) change any time the code is
changed - having stable commands to give to users and put in
documentation (e.g.
"echo 0x200 > /sys/module/applespi/parameters/debug") is
quite valuable.
3. In at least two places we log different types of packets in the
same lines of code - dynamic-debug would only allow enabling or
disabling logging of all packets, unless we do something like
create separate functions for logging each type of packet.
[snip]
> > +static unsigned int fnmode = 1;
> > +module_param(fnmode, uint, 0644);
> > +MODULE_PARM_DESC(fnmode, "Mode of fn key on Apple keyboards (0 = disabled,
> > [1] = fkeyslast, 2 = fkeysfirst)");
>
> fn -> Fn ?
The physical key is actually labelled "fn" (no uppercase). But will
certainly change it if you still wish.
> Btw, do we need all these parameters? Can't we do modify behaviour run-time
> using some Input framework facilities?
I'm not aware of any way to do so. I suppose the event callback could
be used/extended to send "configuration" data, but that would require
changes to the input core.
Also, for what it's worth, current drivers such as hid-apple (from
which 'fnmode' and 'iso_layout' were copied and intentionally kept the
same) use this approach too.
[snip]
> > +static int touchpad_dimensions[4];
> > +module_param_array(touchpad_dimensions, int, NULL, 0444);
> > +MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the
> > touchpad, as x_min,x_max,y_min,y_max .");
>
> We have some parsers for similar data as in format
>
> %dx%d%+d%+d ?
>
> For example, 200x100+20+10.
Maybe it's just my grep-foo that is lacking, but I couldn't find
anything useful. There is some stuff for framebuffer video modes, but
that is too different and not really amenable for use here. OTOH, a
simple sscanf(buf, "%dx%d+%d+%d", ...) would parse this just fine
without the need for any fancier parser.
OTOH, I'm not sure this format is any better: internally we need
x/y min/max, not x/y/w/h (though obviously the two are trivially
convertable); and for the user it doesn't really matter since they would
basically be getting the dimensions from running with debug set to
0x1 and using that output to set this param, i.e. I don't see any
inherent advantage of using x/y/w/h.
>
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
On Mon, Feb 04, 2019 at 12:19:47AM -0800, 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.
Thanks for doing this. My review below.
> +/**
I'm not sure it's kernel doc format comment.
> + * The keyboard and touchpad controller on the MacBookAir6, MacBookPro12,
> + * MacBook8 and newer can be driven either by USB or SPI. However the USB
> + * pins are only connected on the MacBookAir6 and 7 and the MacBookPro12.
> + * All others need this driver. The interface is selected using ACPI methods:
> + *
> + * * UIEN ("USB Interface Enable"): If invoked with argument 1, disables SPI
> + * and enables USB. If invoked with argument 0, disables USB.
> + * * UIST ("USB Interface Status"): Returns 1 if USB is enabled, 0 otherwise.
> + * * SIEN ("SPI Interface Enable"): If invoked with argument 1, disables USB
> + * and enables SPI. If invoked with argument 0, disables SPI.
> + * * SIST ("SPI Interface Status"): Returns 1 if SPI is enabled, 0 otherwise.
> + * * ISOL: Resets the four GPIO pins used for SPI. Intended to be invoked
> with
> + * argument 1, then once more with argument 0.
> + *
> + * UIEN and UIST are only provided on models where the USB pins are
> connected.
> + *
> + * SPI-based Protocol
> + * --
> + *
> + * The device and driver exchange messages (struct message); each message is
> + * encapsulated in one or more packets (struct spi_packet). There are two
> types
> + * of exchanges: reads, and writes. A read is signaled by a GPE, upon which
> one
> + * message can be read from the device. A write exchange consists of writing
> a
> + * command message, immediately reading a short status packet, and then, upon
> + * receiving a GPE, reading the response message. Write exchanges cannot be
> + * interleaved, i.e. a new write exchange must not be started till the
> previous
> + * write exchange is complete. Whether a received message is part of a read
> or
> + * write exchange is indicated in the encapsulating packet's flags field.
> + *
> + * A single message may be too large to fit in a single packet (which has a
> + * fixed, 256-byte size). In that case it will be split over multiple,
> + * consecutive packets.
> + */
> +
> +#define pr_fmt(fmt) "applespi: " fmt
Better to use usual pattern.
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Can we keep them sorted? Do you need, btw, all of them?
+ blank line.
> +#include
> +#define MIN_KBD_BL_LEVEL 32
> +#define MAX_KBD_BL_LEVEL 255
I would rather use similar pattern as below, i.e. KBD_..._MIN/_MAX.
> +#define KBD_BL_LEVEL_SCALE 100
> +#define KBD_BL_LEVEL_ADJ \
> + ((MAX_KBD_BL_LEVEL - MIN_KBD_BL_LEVEL) * KBD_BL_LEVEL_SCALE / 255)
> +#define debug_print(mask, fmt, ...) \
> + do { \
> + if (debug & mask) \
> + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> + } while (0)
> +
> +#define debug_print_header(mask) \
> + debug_print(mask, "--- %s ---\n", \
> + applespi_debug_facility(mask))
> +
> +#define debug_print_buffer(mask, fmt, ...) \
> + do { \
> + if (debug & mask) \
> + print_hex_dump(KERN_DEBUG, pr_fmt(fmt), \
> +DUMP_PREFIX_NONE, 32, 1, ##__VA_ARGS__, \
> +false); \
> + } while (0)
I'm not sure we need all of these... But okay, the driver is
reverse-engineered, perhaps we can drop it later on.
> +#define SPI_RW_CHG_DLY 100 /* from experimentation, in us
> */
In _US would be good to have in a constant name, i.e.
SPI_RW_CHG_DELAY_US
> +static unsigned int fnmode = 1;
> +module_param(fnmode, uint, 0644);
> +MODULE_PARM_DESC(fnmode, "Mode of fn key on Apple keyboards (0 = disabled,
> [1] = fkeyslast, 2 = fkeysfirst)");
fn -> Fn ?
Ditto for the rest.
Btw, do we need all these parameters? Can't we do modify behaviour run-time
using some Input framework facilities?
> +static unsigned int iso_layout;
> +module_param(iso_layout, uint, 0644);
> +MODULE_PARM_D
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
On Tue, Feb 5, 2019 at 4:21 PM Life is hard, and then you die wrote: > > > +config KEYBOARD_APPLESPI > > > + tristate "Apple SPI keyboard and trackpad" > > > > > + depends on (X86 && ACPI && SPI) || COMPILE_TEST > > > > COMPILE_TEST more or less makes sense in conjunction with architecture > > selection. > > It means, your code always dependant to ACPI and SPI frameworks. > > That's why 0day complained. > > Thanks. Yes, looking at this again I realized I somewhat misunderstood > the uses of COMPILE_TEST. I've changed this now to > > depends on ACPI && SPI && (X86 || COMPILE_TEST) Better to split like depends on SPI && ACPI depends on X86 || COMPILE_TEST -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
Hi Andy, On Tue, Feb 05, 2019 at 01:45:22PM +0200, Andy Shevchenko wrote: > On Mon, Feb 04, 2019 at 12:19:47AM -0800, 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. > > > +config KEYBOARD_APPLESPI > > + tristate "Apple SPI keyboard and trackpad" > > > + depends on (X86 && ACPI && SPI) || COMPILE_TEST > > COMPILE_TEST more or less makes sense in conjunction with architecture > selection. > It means, your code always dependant to ACPI and SPI frameworks. > That's why 0day complained. Thanks. Yes, looking at this again I realized I somewhat misunderstood the uses of COMPILE_TEST. I've changed this now to depends on ACPI && SPI && (X86 || COMPILE_TEST) Cheers, Ronald
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
Hi Dan, On Tue, Feb 05, 2019 at 01:21:10PM +0300, Dan Carpenter wrote: > Hi Ronald, > > Thank you for the patch! Perhaps something to improve: > > url: > https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/drm-bridge-sil_sii8620-depend-on-INPUT-instead-of-selecting-it/20190205-003319 > base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next > > smatch warnings: > drivers/input/keyboard/applespi.c:1551 applespi_get_saved_bl_level() warn: > returning -1 instead of -ENOMEM is sloppy [snip] > cfa9f3705 Ronald Tschalär 2019-02-04 1549efivar_entry = > kmalloc(sizeof(*efivar_entry), GFP_KERNEL); > cfa9f3705 Ronald Tschalär 2019-02-04 1550if (!efivar_entry) > cfa9f3705 Ronald Tschalär 2019-02-04 @1551return -1; Agreed, that is sloppy. Fixed in for next version. Thanks! Cheers, Ronald
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
On Mon, Feb 04, 2019 at 12:19:47AM -0800, 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. > +config KEYBOARD_APPLESPI > + tristate "Apple SPI keyboard and trackpad" > + depends on (X86 && ACPI && SPI) || COMPILE_TEST COMPILE_TEST more or less makes sense in conjunction with architecture selection. It means, your code always dependant to ACPI and SPI frameworks. That's why 0day complained. > + imply SPI_PXA2XX > + imply SPI_PXA2XX_PCI > + imply MFD_INTEL_LPSS_PCI > + help > + Say Y here if you are running Linux on any Apple MacBook8,1 or later, > + or any MacBookPro13,* or MacBookPro14,*. > + > + To compile this driver as a module, choose M here: the > + module will be called applespi. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
Hi Ronald,
Thank you for the patch! Perhaps something to improve:
url:
https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/drm-bridge-sil_sii8620-depend-on-INPUT-instead-of-selecting-it/20190205-003319
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
smatch warnings:
drivers/input/keyboard/applespi.c:1551 applespi_get_saved_bl_level() warn:
returning -1 instead of -ENOMEM is sloppy
#
https://github.com/0day-ci/linux/commit/cfa9f37054a5b21113aa8b00c455275145114f8e
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout cfa9f37054a5b21113aa8b00c455275145114f8e
vim +1551 drivers/input/keyboard/applespi.c
cfa9f3705 Ronald Tschalär 2019-02-04 1541
cfa9f3705 Ronald Tschalär 2019-02-04 1542 static int
applespi_get_saved_bl_level(void)
cfa9f3705 Ronald Tschalär 2019-02-04 1543 {
cfa9f3705 Ronald Tschalär 2019-02-04 1544 struct efivar_entry
*efivar_entry;
cfa9f3705 Ronald Tschalär 2019-02-04 1545 u16 efi_data = 0;
cfa9f3705 Ronald Tschalär 2019-02-04 1546 unsigned long efi_data_len;
cfa9f3705 Ronald Tschalär 2019-02-04 1547 int sts;
cfa9f3705 Ronald Tschalär 2019-02-04 1548
cfa9f3705 Ronald Tschalär 2019-02-04 1549 efivar_entry =
kmalloc(sizeof(*efivar_entry), GFP_KERNEL);
cfa9f3705 Ronald Tschalär 2019-02-04 1550 if (!efivar_entry)
cfa9f3705 Ronald Tschalär 2019-02-04 @1551 return -1;
cfa9f3705 Ronald Tschalär 2019-02-04 1552
cfa9f3705 Ronald Tschalär 2019-02-04 1553
memcpy(efivar_entry->var.VariableName, EFI_BL_LEVEL_NAME,
cfa9f3705 Ronald Tschalär 2019-02-04 1554
sizeof(EFI_BL_LEVEL_NAME));
cfa9f3705 Ronald Tschalär 2019-02-04 1555 efivar_entry->var.VendorGuid =
EFI_BL_LEVEL_GUID;
cfa9f3705 Ronald Tschalär 2019-02-04 1556 efi_data_len = sizeof(efi_data);
cfa9f3705 Ronald Tschalär 2019-02-04 1557
cfa9f3705 Ronald Tschalär 2019-02-04 1558 sts =
efivar_entry_get(efivar_entry, NULL, &efi_data_len, &efi_data);
cfa9f3705 Ronald Tschalär 2019-02-04 1559 if (sts && sts != -ENOENT)
cfa9f3705 Ronald Tschalär 2019-02-04 1560 pr_warn("Error getting
backlight level from EFI vars: %d\n",
cfa9f3705 Ronald Tschalär 2019-02-04 1561 sts);
cfa9f3705 Ronald Tschalär 2019-02-04 1562
cfa9f3705 Ronald Tschalär 2019-02-04 1563 kfree(efivar_entry);
cfa9f3705 Ronald Tschalär 2019-02-04 1564
cfa9f3705 Ronald Tschalär 2019-02-04 1565 return efi_data;
cfa9f3705 Ronald Tschalär 2019-02-04 1566 }
cfa9f3705 Ronald Tschalär 2019-02-04 1567
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
Hi Ronald,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on input/next]
[also build test ERROR on v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/drm-bridge-sil_sii8620-depend-on-INPUT-instead-of-selecting-it/20190205-003319
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=sh
All errors (new ones prefixed by >>):
drivers//spi/spi-pxa2xx-pci.c: In function 'pxa2xx_spi_pci_probe':
drivers//spi/spi-pxa2xx-pci.c:205:8: error: implicit declaration of function
'pcim_enable_device'; did you mean 'pci_enable_device'?
[-Werror=implicit-function-declaration]
ret = pcim_enable_device(dev);
^~
pci_enable_device
drivers//spi/spi-pxa2xx-pci.c:235:8: error: implicit declaration of function
'pci_alloc_irq_vectors'; did you mean 'pci_alloc_consistent'?
[-Werror=implicit-function-declaration]
ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
^
pci_alloc_consistent
>> drivers//spi/spi-pxa2xx-pci.c:235:41: error: 'PCI_IRQ_ALL_TYPES' undeclared
>> (first use in this function); did you mean 'PCI_PRI_ALLOC_REQ'?
ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
^
PCI_PRI_ALLOC_REQ
drivers//spi/spi-pxa2xx-pci.c:235:41: note: each undeclared identifier is
reported only once for each function it appears in
drivers//spi/spi-pxa2xx-pci.c:238:13: error: implicit declaration of
function 'pci_irq_vector'; did you mean 'rcu_irq_enter'?
[-Werror=implicit-function-declaration]
ssp->irq = pci_irq_vector(dev, 0);
^~
rcu_irq_enter
drivers//spi/spi-pxa2xx-pci.c: At top level:
drivers//spi/spi-pxa2xx-pci.c:296:1: warning: data definition has no type or
storage class
module_pci_driver(pxa2xx_spi_pci_driver);
^
drivers//spi/spi-pxa2xx-pci.c:296:1: error: type defaults to 'int' in
declaration of 'module_pci_driver' [-Werror=implicit-int]
drivers//spi/spi-pxa2xx-pci.c:296:1: warning: parameter names (without
types) in function declaration
drivers//spi/spi-pxa2xx-pci.c:289:26: warning: 'pxa2xx_spi_pci_driver'
defined but not used [-Wunused-variable]
static struct pci_driver pxa2xx_spi_pci_driver = {
^
cc1: some warnings being treated as errors
vim +235 drivers//spi/spi-pxa2xx-pci.c
d6ba32d5c drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee2014-04-18
193
d6ba32d5c drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee2014-04-18
194 static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24
195const struct pci_device_id *ent)
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24
196 {
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg 2013-01-07
197struct platform_device_info pi;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24
198int ret;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24
199struct platform_device *pdev;
0f3e1d27a drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2011-02-03
200struct pxa2xx_spi_master spi_pdata;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24
201struct ssp_device *ssp;
d6ba32d5c drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee2014-04-18
202struct pxa_spi_info *c;
afa93c901 drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee2014-07-25
203char buf[40];
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24
204
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg 2013-01-07
@205ret = pcim_enable_device(dev);
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24
206if (ret)
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24
207return ret;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24
208
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg 2013-01-07
209ret = pcim_iomap_regions(dev, 1 << 0, "PXA2xx SPI");
c13463407 drivers/spi/spi-pxa2xx-pci.c Mika Westerberg 2013-03-05
210if (ret)
d6ea3df0d drivers/spi/pxa2xx_spi_p
Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
Hi Ronald,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on input/next]
[also build test ERROR on v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/drm-bridge-sil_sii8620-depend-on-INPUT-instead-of-selecting-it/20190205-003319
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=sh
All error/warnings (new ones prefixed by >>):
drivers/input/keyboard/applespi.c: In function 'applespi_enable_spi':
>> drivers/input/keyboard/applespi.c:692:11: error: implicit declaration of
>> function 'acpi_evaluate_integer'; did you mean 'acpi_evaluate_object'?
>> [-Werror=implicit-function-declaration]
result = acpi_evaluate_integer(applespi->sist, NULL, NULL, &spi_status);
^
acpi_evaluate_object
>> drivers/input/keyboard/applespi.c:697:11: error: implicit declaration of
>> function 'acpi_execute_simple_method'; did you mean 'acpi_install_method'?
>> [-Werror=implicit-function-declaration]
result = acpi_execute_simple_method(applespi->sien, NULL, 1);
^~
acpi_install_method
At top level:
drivers/input/keyboard/applespi.c:1851:12: warning: 'applespi_resume'
defined but not used [-Wunused-function]
static int applespi_resume(struct device *dev)
^~~
drivers/input/keyboard/applespi.c:1808:12: warning: 'applespi_suspend'
defined but not used [-Wunused-function]
static int applespi_suspend(struct device *dev)
^~~~
cc1: some warnings being treated as errors
--
drivers/spi/spi-pxa2xx-pci.c: In function 'pxa2xx_spi_pci_probe':
>> drivers/spi/spi-pxa2xx-pci.c:205:8: error: implicit declaration of function
>> 'pcim_enable_device'; did you mean 'pci_enable_device'?
>> [-Werror=implicit-function-declaration]
ret = pcim_enable_device(dev);
^~
pci_enable_device
>> drivers/spi/spi-pxa2xx-pci.c:235:8: error: implicit declaration of function
>> 'pci_alloc_irq_vectors'; did you mean 'pci_alloc_consistent'?
>> [-Werror=implicit-function-declaration]
ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
^
pci_alloc_consistent
drivers/spi/spi-pxa2xx-pci.c:235:41: error: 'PCI_IRQ_ALL_TYPES' undeclared
(first use in this function); did you mean 'PCI_PRI_ALLOC_REQ'?
ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
^
PCI_PRI_ALLOC_REQ
drivers/spi/spi-pxa2xx-pci.c:235:41: note: each undeclared identifier is
reported only once for each function it appears in
>> drivers/spi/spi-pxa2xx-pci.c:238:13: error: implicit declaration of function
>> 'pci_irq_vector'; did you mean 'rcu_irq_enter'?
>> [-Werror=implicit-function-declaration]
ssp->irq = pci_irq_vector(dev, 0);
^~
rcu_irq_enter
drivers/spi/spi-pxa2xx-pci.c: At top level:
>> drivers/spi/spi-pxa2xx-pci.c:296:1: warning: data definition has no type or
>> storage class
module_pci_driver(pxa2xx_spi_pci_driver);
^
>> drivers/spi/spi-pxa2xx-pci.c:296:1: error: type defaults to 'int' in
>> declaration of 'module_pci_driver' [-Werror=implicit-int]
>> drivers/spi/spi-pxa2xx-pci.c:296:1: warning: parameter names (without types)
>> in function declaration
drivers/spi/spi-pxa2xx-pci.c:289:26: warning: 'pxa2xx_spi_pci_driver'
defined but not used [-Wunused-variable]
static struct pci_driver pxa2xx_spi_pci_driver = {
^
cc1: some warnings being treated as errors
vim +692 drivers/input/keyboard/applespi.c
685
686 static int applespi_enable_spi(struct applespi_data *applespi)
687 {
688 int result;
689 unsigned long long spi_status;
690
691 /* check if SPI is already enabled, so we can skip the delay
below */
> 692 result = acpi_evaluate_integer(applespi->sist, NULL, NULL,
> &spi_status);
693 if (ACPI_SUCCESS(result) && spi_status)
694 return 0;
695
696 /* SIEN(1) will enable SPI communication */
> 697 result = acpi_execute_simple_method(applespi->sien, NULL, 1);
698 if (ACPI_FAILURE(result)) {
699 pr_err("SIEN failed: %s\n",
acpi_format_exception(result));
700 return
