lkml.org 
[lkml]   [2019]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 2/2] Input: add Apple SPI keyboard and trackpad driver.
On Sat, Apr 06, 2019 at 10:03:58PM -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, my comments below.

> +static void applespi_debug_update_dimensions(struct applespi_data *applespi,
> + const struct tp_finger *f)
> +{
> + #define UPDATE_DIMENSIONS(val, op, last) \
> + do { \
> + if (le16_to_int(val) op last) \
> + last = le16_to_int(val); \
> + } while (0)

I do not see how this is better than min()/max()/min_t()/max_t().

> +
> + UPDATE_DIMENSIONS(f->abs_x, <, applespi->tp_dim_min_x);
> + UPDATE_DIMENSIONS(f->abs_x, >, applespi->tp_dim_max_x);
> + UPDATE_DIMENSIONS(f->abs_y, <, applespi->tp_dim_min_y);
> + UPDATE_DIMENSIONS(f->abs_y, >, applespi->tp_dim_max_y);
> +
> + #undef UPDATE_DIMENSIONS
> +}

> +static void
> +applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
> +{
> + unsigned char tmp;

What about

u8 bit = BIT(fnremap - 1);

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

> + return;
> +
> + tmp = keyboard_protocol->fn_pressed;
> + keyboard_protocol->fn_pressed =
> + !!(keyboard_protocol->modifiers & BIT(fnremap - 1));

... & bit);

and so on?

> + if (tmp)
> + keyboard_protocol->modifiers |= BIT(fnremap - 1);
> + else
> + keyboard_protocol->modifiers &= ~BIT(fnremap - 1);
> +}

> +static void
> +applespi_handle_keyboard_event(struct applespi_data *applespi,
> + struct keyboard_protocol *keyboard_protocol)
> +{
> + int i, j;
> + unsigned int key;
> + bool still_pressed;
> + bool is_overflow;

Perhaps reversed xmas tree order?

> +
> + compiletime_assert(ARRAY_SIZE(applespi_controlcodes) ==
> + sizeof_field(struct keyboard_protocol, modifiers) * 8,
> + "applespi_controlcodes has wrong number of entries");
> +
> + /* check for rollover overflow, which is signalled by all keys == 1 */
> + is_overflow = true;
> +
> + for (i = 0; i < MAX_ROLLOVER; i++) {
> + if (keyboard_protocol->keys_pressed[i] != 1) {
> + is_overflow = false;
> + break;
> + }
> + }
> +

> + if (is_overflow)
> + return;


Isn't it simple

if (i == MAX_ROLLOVER)

w/o need of additional variable?

> + /* remap fn key if desired */
> + applespi_remap_fn_key(keyboard_protocol);
> +
> + /* check released keys */
> + for (i = 0; i < MAX_ROLLOVER; i++) {
> + still_pressed = false;
> + for (j = 0; j < MAX_ROLLOVER; j++) {
> + if (applespi->last_keys_pressed[i] ==
> + keyboard_protocol->keys_pressed[j]) {
> + still_pressed = true;
> + break;
> + }
> + }
> +
> + if (still_pressed)
> + continue;

Similar idea here.

> +
> + key = applespi_code_to_key(applespi->last_keys_pressed[i],
> + applespi->last_keys_fn_pressed[i]);
> + input_report_key(applespi->keyboard_input_dev, key, 0);
> + applespi->last_keys_fn_pressed[i] = 0;
> + }

> + /* check pressed keys */
> + for (i = 0; i < MAX_ROLLOVER; i++) {
> + if (keyboard_protocol->keys_pressed[i] <
> + ARRAY_SIZE(applespi_scancodes) &&
> + keyboard_protocol->keys_pressed[i] > 0) {
> + key = applespi_code_to_key(
> + keyboard_protocol->keys_pressed[i],
> + keyboard_protocol->fn_pressed);
> + input_report_key(applespi->keyboard_input_dev, key, 1);
> + applespi->last_keys_fn_pressed[i] =
> + keyboard_protocol->fn_pressed;
> + }
> + }
> +
> + /* check control keys */
> + for (i = 0; i < ARRAY_SIZE(applespi_controlcodes); i++) {
> + if (keyboard_protocol->modifiers & BIT(i))
> + input_report_key(applespi->keyboard_input_dev,
> + applespi_controlcodes[i], 1);
> + else
> + input_report_key(applespi->keyboard_input_dev,
> + applespi_controlcodes[i], 0);
> + }
> +
> + /* check function key */
> + if (keyboard_protocol->fn_pressed && !applespi->last_fn_pressed)
> + input_report_key(applespi->keyboard_input_dev, KEY_FN, 1);
> + else if (!keyboard_protocol->fn_pressed && applespi->last_fn_pressed)
> + input_report_key(applespi->keyboard_input_dev, KEY_FN, 0);
> + applespi->last_fn_pressed = keyboard_protocol->fn_pressed;
> +
> + /* done */
> + input_sync(applespi->keyboard_input_dev);
> + memcpy(&applespi->last_keys_pressed, keyboard_protocol->keys_pressed,
> + sizeof(applespi->last_keys_pressed));
> +}

> +static int
> +applespi_register_touchpad_device(struct applespi_data *applespi,
> + struct touchpad_info_protocol *rcvd_tp_info)
> +{
> + const struct applespi_tp_info *tp_info;
> + struct input_dev *touchpad_input_dev;
> + int sts;
> +
> + /* set up touchpad dimensions */
> + tp_info = applespi_find_touchpad_info(rcvd_tp_info->model_no);
> + if (!tp_info) {

> + dev_warn(&(applespi)->spi->dev,

I'm not sure what parens are helping with (here and everywhere with device parameter)?

> + "Unknown touchpad model %x - falling back to MB8 touchpad\n",
> + rcvd_tp_info->model_no);
> + tp_info = &applespi_tp_models[0].tp_info;
> + }

> + return 0;
> +}

> +static int applespi_probe(struct spi_device *spi)
> +{
> + struct applespi_data *applespi;
> + acpi_status acpi_sts;
> + int sts, i;
> + unsigned long long gpe, usb_status;
> +
> + /* check if the USB interface is present and enabled already */

> + acpi_sts = acpi_evaluate_integer(ACPI_HANDLE(&spi->dev), "UIST", NULL,
> + &usb_status);

... see below ...

> + if (ACPI_SUCCESS(acpi_sts) && usb_status) {
> + /* let the USB driver take over instead */
> + dev_info(&spi->dev, "USB interface already enabled\n");
> + return -ENODEV;
> + }
> +
> + /* allocate driver data */
> + applespi = devm_kzalloc(&spi->dev, sizeof(*applespi), GFP_KERNEL);
> + if (!applespi)
> + return -ENOMEM;
> +
> + applespi->spi = spi;

> + applespi->handle = ACPI_HANDLE(&spi->dev);

AFAICS this is local variable, why put it in the data structure?
And above you are not using this, btw.

> + } else {
> + struct dentry *ret;
> +
> + ret = debugfs_create_bool("enable_tp_dim", 0600,
> + applespi->debugfs_root,
> + &applespi->debug_tp_dim);
> + if (IS_ERR(ret))

> + dev_warn(&(applespi)->spi->dev,
> + "Error creating debugfs entry enable_tp_dim (%ld)\n",
> + PTR_ERR(ret));

Can ret be NULL?

dev_dbg() looks more appropriate.

> +
> + ret = debugfs_create_file("tp_dim", 0400,
> + applespi->debugfs_root, applespi,
> + &applespi_tp_dim_fops);
> + if (IS_ERR(ret))

> + dev_warn(&(applespi)->spi->dev,
> + "Error creating debugfs entry tp_dim (%ld)\n",
> + PTR_ERR(ret));

Ditto.

> + }
> +
> + return 0;
> +}

--
With Best Regards,
Andy Shevchenko


\
 
 \ /
  Last update: 2019-04-08 14:34    [W:0.089 / U:4.808 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site