| Message ID | 20190720150511.95076-3-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support for buttons on newer MS Surface devices | expand |
On 20.07.19 17:05, Maximilian Luz wrote: > Power and volume button support for 5th and 6th generation Microsoft > Surface devices via soc_button_array. > > Note that these devices use the same MSHW0040 device as on the Surface > Pro 4, however the implementation is different (GPIOs vs. ACPI > notifications). Thus some checking is required to ensure we only load > this driver on the correct devices. Could this also used on the older (pre pro4) devices (also using the gpios directly, and leave off acpi notifications) ? --mtx
On 7/22/19 10:00 AM, Enrico Weigelt, metux IT consult wrote: > On 20.07.19 17:05, Maximilian Luz wrote: >> Power and volume button support for 5th and 6th generation Microsoft >> Surface devices via soc_button_array. >> >> Note that these devices use the same MSHW0040 device as on the Surface >> Pro 4, however the implementation is different (GPIOs vs. ACPI >> notifications). Thus some checking is required to ensure we only load >> this driver on the correct devices. > > Could this also used on the older (pre pro4) devices (also using the > gpios directly, and leave off acpi notifications) ? As far as I can tell, no. The Pro 4 and Pro 3 don't have any GPIOs on MSHW0028/MSHW0040. Book 1 has GPIOs but for a different purpose. The Pro 2 has a standard PNP0C0C power button (no idea how the volume buttons are handled there, but also seems to be different from what I can gather from DSDT). I can't say anything for the Pro 1 and non-Pro devices. Maximilian
Hi Maximilian, On Sat, Jul 20, 2019 at 05:05:11PM +0200, Maximilian Luz wrote: > - > - error = gpiod_count(dev, NULL); > - if (error < 0) { > - dev_dbg(dev, "no GPIO attached, ignoring...\n"); > - return -ENODEV; I do not think we need to move this into individual "check" functions. It is needed in all cases so we should keep it here. How about version below? Input: soc_button_array - add support for newer surface devices From: Maximilian Luz <luzmaximilian@gmail.com> Power and volume button support for 5th and 6th generation Microsoft Surface devices via soc_button_array. Note that these devices use the same MSHW0040 device as on the Surface Pro 4, however the implementation is different (GPIOs vs. ACPI notifications). Thus some checking is required to ensure we only load this driver on the correct devices. Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/misc/Kconfig | 6 +- drivers/input/misc/soc_button_array.c | 105 +++++++++++++++++++++++++++++---- 2 files changed, 96 insertions(+), 15 deletions(-) diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig index d07c1eb15aa6..7d9ae394e597 100644 --- a/drivers/input/misc/Kconfig +++ b/drivers/input/misc/Kconfig @@ -813,10 +813,10 @@ config INPUT_IDEAPAD_SLIDEBAR config INPUT_SOC_BUTTON_ARRAY tristate "Windows-compatible SoC Button Array" - depends on KEYBOARD_GPIO + depends on KEYBOARD_GPIO && ACPI help - Say Y here if you have a SoC-based tablet that originally - runs Windows 8. + Say Y here if you have a SoC-based tablet that originally runs + Windows 8 or a Microsoft Surface Book 2, Pro 5, Laptop 1 or later. To compile this driver as a module, choose M here: the module will be called soc_button_array. diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c index 5e59f8e57f8e..6f0133fe1546 100644 --- a/drivers/input/misc/soc_button_array.c +++ b/drivers/input/misc/soc_button_array.c @@ -25,6 +25,11 @@ struct soc_button_info { bool wakeup; }; +struct soc_device_data { + const struct soc_button_info *button_info; + int (*check)(struct device *dev); +}; + /* * Some of the buttons like volume up/down are auto repeat, while others * are not. To support both, we register two platform devices, and put @@ -87,8 +92,13 @@ soc_button_device_create(struct platform_device *pdev, continue; gpio = soc_button_lookup_gpio(&pdev->dev, info->acpi_index); - if (!gpio_is_valid(gpio)) + if (gpio < 0 && gpio != -ENOENT) { + error = gpio; + goto err_free_mem; + } else if (!gpio_is_valid(gpio)) { + /* Skip GPIO if not present */ continue; + } gpio_keys[n_buttons].type = info->event_type; gpio_keys[n_buttons].code = info->event_code; @@ -309,23 +319,26 @@ static int soc_button_remove(struct platform_device *pdev) static int soc_button_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - const struct acpi_device_id *id; - struct soc_button_info *button_info; + const struct soc_device_data *device_data; + const struct soc_button_info *button_info; struct soc_button_data *priv; struct platform_device *pd; int i; int error; - id = acpi_match_device(dev->driver->acpi_match_table, dev); - if (!id) - return -ENODEV; + device_data = acpi_device_get_match_data(dev); + if (device_data && device_data->check) { + error = device_data->check(dev); + if (error) + return error; + } - if (!id->driver_data) { + if (device_data && device_data->button_info) { + button_info = device_data->button_info; + } else { button_info = soc_button_get_button_info(dev); if (IS_ERR(button_info)) return PTR_ERR(button_info); - } else { - button_info = (struct soc_button_info *)id->driver_data; } error = gpiod_count(dev, NULL); @@ -357,7 +370,7 @@ static int soc_button_probe(struct platform_device *pdev) if (!priv->children[0] && !priv->children[1]) return -ENODEV; - if (!id->driver_data) + if (!device_data || !device_data->button_info) devm_kfree(dev, button_info); return 0; @@ -368,7 +381,7 @@ static int soc_button_probe(struct platform_device *pdev) * is defined in section 2.8.7.2 of "Windows ACPI Design Guide for SoC * Platforms" */ -static struct soc_button_info soc_button_PNP0C40[] = { +static const struct soc_button_info soc_button_PNP0C40[] = { { "power", 0, EV_KEY, KEY_POWER, false, true }, { "home", 1, EV_KEY, KEY_LEFTMETA, false, true }, { "volume_up", 2, EV_KEY, KEY_VOLUMEUP, true, false }, @@ -377,9 +390,77 @@ static struct soc_button_info soc_button_PNP0C40[] = { { } }; +static const struct soc_device_data soc_device_PNP0C40 = { + .button_info = soc_button_PNP0C40, +}; + +/* + * Special device check for Surface Book 2 and Surface Pro (2017). + * Both, the Surface Pro 4 (surfacepro3_button.c) and the above mentioned + * devices use MSHW0040 for power and volume buttons, however the way they + * have to be addressed differs. Make sure that we only load this drivers + * for the correct devices by checking the OEM Platform Revision provided by + * the _DSM method. + */ +#define MSHW0040_DSM_REVISION 0x01 +#define MSHW0040_DSM_GET_OMPR 0x02 // get OEM Platform Revision +static const guid_t MSHW0040_DSM_UUID = + GUID_INIT(0x6fd05c69, 0xcde3, 0x49f4, 0x95, 0xed, 0xab, 0x16, 0x65, + 0x49, 0x80, 0x35); + +static int soc_device_check_MSHW0040(struct device *dev) +{ + acpi_handle handle = ACPI_HANDLE(dev); + union acpi_object *result; + u64 oem_platform_rev = 0; // valid revisions are nonzero + + // get OEM platform revision + result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID, + MSHW0040_DSM_REVISION, + MSHW0040_DSM_GET_OMPR, NULL, + ACPI_TYPE_INTEGER); + + if (result) { + oem_platform_rev = result->integer.value; + ACPI_FREE(result); + } + + /* + * If the revision is zero here, the _DSM evaluation has failed. This + * indicates that we have a Pro 4 or Book 1 and this driver should not + * be used. + */ + if (oem_platform_rev == 0) + return -ENODEV; + + dev_dbg(dev, "OEM Platform Revision %llu\n", oem_platform_rev); + + return 0; +} + +/* + * Button infos for Microsoft Surface Book 2 and Surface Pro (2017). + * Obtained from DSDT/testing. + */ +static const struct soc_button_info soc_button_MSHW0040[] = { + { "power", 0, EV_KEY, KEY_POWER, false, true }, + { "volume_up", 2, EV_KEY, KEY_VOLUMEUP, true, false }, + { "volume_down", 4, EV_KEY, KEY_VOLUMEDOWN, true, false }, + { } +}; + +static const struct soc_device_data soc_device_MSHW0040 = { + .button_info = soc_button_MSHW0040, + .check = soc_device_check_MSHW0040, +}; + static const struct acpi_device_id soc_button_acpi_match[] = { - { "PNP0C40", (unsigned long)soc_button_PNP0C40 }, + { "PNP0C40", (unsigned long)&soc_device_PNP0C40 }, { "ACPI0011", 0 }, + + /* Microsoft Surface Devices (5th and 6th generation) */ + { "MSHW0040", (unsigned long)&soc_device_MSHW0040 }, + { } };
On 7/27/19 11:14 AM, Dmitry Torokhov wrote: > On Sat, Jul 20, 2019 at 05:05:11PM +0200, Maximilian Luz wrote: >> - >> - error = gpiod_count(dev, NULL); >> - if (error < 0) { >> - dev_dbg(dev, "no GPIO attached, ignoring...\n"); >> - return -ENODEV; > > I do not think we need to move this into individual "check" functions. > It is needed in all cases so we should keep it here. > > How about version below? Makes sense, looks good to me! Maximilian
On Sat, Jul 27, 2019 at 02:01:26PM +0200, Maximilian Luz wrote: > On 7/27/19 11:14 AM, Dmitry Torokhov wrote: > > On Sat, Jul 20, 2019 at 05:05:11PM +0200, Maximilian Luz wrote: > > > - > > > - error = gpiod_count(dev, NULL); > > > - if (error < 0) { > > > - dev_dbg(dev, "no GPIO attached, ignoring...\n"); > > > - return -ENODEV; > > > > I do not think we need to move this into individual "check" functions. > > It is needed in all cases so we should keep it here. > > > > How about version below? > > Makes sense, looks good to me! OK, great, applied.
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig index d07c1eb15aa6..7d9ae394e597 100644 --- a/drivers/input/misc/Kconfig +++ b/drivers/input/misc/Kconfig @@ -813,10 +813,10 @@ config INPUT_IDEAPAD_SLIDEBAR config INPUT_SOC_BUTTON_ARRAY tristate "Windows-compatible SoC Button Array" - depends on KEYBOARD_GPIO + depends on KEYBOARD_GPIO && ACPI help - Say Y here if you have a SoC-based tablet that originally - runs Windows 8. + Say Y here if you have a SoC-based tablet that originally runs + Windows 8 or a Microsoft Surface Book 2, Pro 5, Laptop 1 or later. To compile this driver as a module, choose M here: the module will be called soc_button_array. diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c index 5e59f8e57f8e..4d5150bebeef 100644 --- a/drivers/input/misc/soc_button_array.c +++ b/drivers/input/misc/soc_button_array.c @@ -25,6 +25,17 @@ struct soc_button_info { bool wakeup; }; +/** + * struct soc_device_data - driver data for different device types + * @button_info: specifications of buttons, if NULL specification is assumed to + * be present in _DSD + * @check: device-specific check (NULL means all will be accepted) + */ +struct soc_device_data { + struct soc_button_info *button_info; + int (*check)(struct device *dev); +}; + /* * Some of the buttons like volume up/down are auto repeat, while others * are not. To support both, we register two platform devices, and put @@ -87,8 +98,13 @@ soc_button_device_create(struct platform_device *pdev, continue; gpio = soc_button_lookup_gpio(&pdev->dev, info->acpi_index); - if (!gpio_is_valid(gpio)) + if (gpio < 0 && gpio != -ENOENT) { + error = gpio; + goto err_free_mem; + } else if (!gpio_is_valid(gpio)) { + /* Skip GPIO if not present */ continue; + } gpio_keys[n_buttons].type = info->event_type; gpio_keys[n_buttons].code = info->event_code; @@ -310,6 +326,7 @@ static int soc_button_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; const struct acpi_device_id *id; + struct soc_device_data *device_data; struct soc_button_info *button_info; struct soc_button_data *priv; struct platform_device *pd; @@ -320,18 +337,20 @@ static int soc_button_probe(struct platform_device *pdev) if (!id) return -ENODEV; - if (!id->driver_data) { + device_data = (struct soc_device_data *)id->driver_data; + if (device_data && device_data->check) { + error = device_data->check(dev); + if (error) + return error; + } + + if (device_data && device_data->button_info) { + button_info = (struct soc_button_info *) + device_data->button_info; + } else { button_info = soc_button_get_button_info(dev); if (IS_ERR(button_info)) return PTR_ERR(button_info); - } else { - button_info = (struct soc_button_info *)id->driver_data; - } - - error = gpiod_count(dev, NULL); - if (error < 0) { - dev_dbg(dev, "no GPIO attached, ignoring...\n"); - return -ENODEV; } priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); @@ -357,12 +376,32 @@ static int soc_button_probe(struct platform_device *pdev) if (!priv->children[0] && !priv->children[1]) return -ENODEV; - if (!id->driver_data) + if (!device_data || !device_data->button_info) devm_kfree(dev, button_info); return 0; } + +static int soc_device_check_generic(struct device *dev) +{ + int gpios; + + gpios = gpiod_count(dev, NULL); + if (gpios < 0) { + dev_dbg(dev, "no GPIO attached, ignoring...\n"); + return -ENODEV; + } + + return 0; +} + +static struct soc_device_data soc_device_ACPI0011 = { + .button_info = NULL, + .check = soc_device_check_generic, +}; + + /* * Definition of buttons on the tablet. The ACPI index of each button * is defined in section 2.8.7.2 of "Windows ACPI Design Guide for SoC @@ -377,9 +416,85 @@ static struct soc_button_info soc_button_PNP0C40[] = { { } }; +static struct soc_device_data soc_device_PNP0C40 = { + .button_info = soc_button_PNP0C40, + .check = soc_device_check_generic, +}; + + +/* + * Special device check for Surface Book 2 and Surface Pro (2017). + * Both, the Surface Pro 4 (surfacepro3_button.c) and the above mentioned + * devices use MSHW0040 for power and volume buttons, however the way they + * have to be addressed differs. Make sure that we only load this drivers + * for the correct devices by checking the OEM Platform Revision provided by + * the _DSM method. + */ +#define MSHW0040_DSM_REVISION 0x01 +#define MSHW0040_DSM_GET_OMPR 0x02 // get OEM Platform Revision +static const guid_t MSHW0040_DSM_UUID = + GUID_INIT(0x6fd05c69, 0xcde3, 0x49f4, 0x95, 0xed, 0xab, 0x16, 0x65, + 0x49, 0x80, 0x35); + +static int soc_device_check_MSHW0040(struct device *dev) +{ + acpi_handle handle = ACPI_HANDLE(dev); + union acpi_object *result; + u64 oem_platform_rev = 0; // valid revisions are nonzero + int gpios; + + // get OEM platform revision + result = acpi_evaluate_dsm_typed(handle, &MSHW0040_DSM_UUID, + MSHW0040_DSM_REVISION, + MSHW0040_DSM_GET_OMPR, NULL, + ACPI_TYPE_INTEGER); + + if (result) { + oem_platform_rev = result->integer.value; + ACPI_FREE(result); + } + + /* + * If the revision is zero here, the _DSM evaluation has failed. This + * indicates that we have a Pro 4 or Book 1 and this driver should not + * be used. + */ + if (oem_platform_rev == 0) + return -ENODEV; + + dev_dbg(dev, "OEM Platform Revision %llu\n", oem_platform_rev); + + gpios = gpiod_count(dev, NULL); + if (gpios < 0) + return -ENODEV; + + return 0; +} + +/* + * Button infos for Microsoft Surface Book 2 and Surface Pro (2017). + * Obtained from DSDT/testing. + */ +static struct soc_button_info soc_button_MSHW0040[] = { + { "power", 0, EV_KEY, KEY_POWER, false, true }, + { "volume_up", 2, EV_KEY, KEY_VOLUMEUP, true, false }, + { "volume_down", 4, EV_KEY, KEY_VOLUMEDOWN, true, false }, + { } +}; + +static struct soc_device_data soc_device_MSHW0040 = { + .button_info = soc_button_MSHW0040, + .check = soc_device_check_MSHW0040, +}; + + static const struct acpi_device_id soc_button_acpi_match[] = { - { "PNP0C40", (unsigned long)soc_button_PNP0C40 }, - { "ACPI0011", 0 }, + { "PNP0C40", (unsigned long)&soc_device_PNP0C40 }, + { "ACPI0011", (unsigned long)&soc_device_ACPI0011 }, + + /* Microsoft Surface Devices (5th and 6th generation) */ + { "MSHW0040", (unsigned long)&soc_device_MSHW0040 }, + { } };
Power and volume button support for 5th and 6th generation Microsoft Surface devices via soc_button_array. Note that these devices use the same MSHW0040 device as on the Surface Pro 4, however the implementation is different (GPIOs vs. ACPI notifications). Thus some checking is required to ensure we only load this driver on the correct devices. Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> --- drivers/input/misc/Kconfig | 6 +- drivers/input/misc/soc_button_array.c | 141 +++++++++++++++++++++++--- 2 files changed, 131 insertions(+), 16 deletions(-)