| Message ID | 20190702003740.75970-3-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support for buttons on newer MS Surface devices | expand |
On 7/2/19 2:37 AM, Maximilian Luz wrote: > +static int soc_device_check_MSHW0040(struct device *dev) > +{ > + acpi_handle handle = ACPI_HANDLE(dev); > + union acpi_object *result; > + u64 oem_platform_rev = 0; > + 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 (oem_platform_rev == 0) > + return -ENODEV; > + > + dev_dbg(dev, "OEM Platform Revision %llu\n", oem_platform_rev); > + > + /* > + * We are _really_ expecting GPIOs here. If we do not get any, this > + * means the GPIO driver has not been loaded yet (which can happen). > + * Try again later. > + */ > + gpios = gpiod_count(dev, NULL); > + if (gpios < 0) > + return -EAGAIN; > + > + return 0; > +} Just had another look at this: Shouldn't the EAGAIN here be EPROBE_DEFER? The reasoning is that we would want to defer probing of the driver if we can't get any GPIO pins, since we know that MSHW0040 should have some. It has in the past been reported that the driver didn't load properly (without this check), since it could happen that the GPIO subsystem wasn't quite ready yet when probing. Best, Maximilian
Hi Maximilian, On Tue, Jul 02, 2019 at 02:37:40AM +0200, Maximilian Luz wrote: > Power and volume button support for 5th and 6th genration 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. When you are saying that Pro 4 and later models use different notifications, does this mean that Pro 4 does not define any GPIOs? If so can we use their presence as indicator whether we should be using this driver or not. I would like to avoid repeating the ACPI parsing code that you have in the platform driver. > +static int soc_device_check_MSHW0040(struct device *dev) > +{ > + acpi_handle handle = ACPI_HANDLE(dev); > + union acpi_object *result; > + u64 oem_platform_rev = 0; > + 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 (oem_platform_rev == 0) > + return -ENODEV; > + > + dev_dbg(dev, "OEM Platform Revision %llu\n", oem_platform_rev); > + > + /* > + * We are _really_ expecting GPIOs here. If we do not get any, this > + * means the GPIO driver has not been loaded yet (which can happen). > + * Try again later. > + */ > + gpios = gpiod_count(dev, NULL); > + if (gpios < 0) > + return -EAGAIN; I do not believe -EAGAIN has any special meaning in the driver core; also when the GPIO controller is not ready gpiod_get() will return -EPROBE_DEFER, which is the prober way if signalling that some resource is not yet available and probe should be retries at a later time. Moreover, I do not believe that gpiod_count() needs GPIO controller to be ready, the count is taken from board firmware or static board file definition, so if gpiod_count() returns 0 it should be clear indication that the driver should not be used with the device. Thanks.
Hi, On 7/16/19 9:21 AM, Dmitry Torokhov wrote: > When you are saying that Pro 4 and later models use different > notifications, does this mean that Pro 4 does not define any GPIOs? Unfortunately, at least the Surface Book (first generation, buttons are handled the same way as on the Pro 4) has GPIOs defined in MSHW0040, but they are for different use. Specifically: They can detect if the clipboard (screen part of the device, the device basically has two parts that can be separated: clipboard and base) is being removed. Relying on the GPIOs was my first idea too, but that has been reported to shut down the Book 1 when the clipboard is detached. As far as I know, the OEM platform revision check is the easiest way to differentiate between those devices. > I do not believe -EAGAIN has any special meaning in the driver core; I think I got the -EAGAIN from an outdated LWN article when I first started working on this, thanks for confirming. > also when the GPIO controller is not ready gpiod_get() will return > -EPROBE_DEFER, which is the prober way if signalling that some resource > is not yet available and probe should be retries at a later time. > > Moreover, I do not believe that gpiod_count() needs GPIO controller to > be ready, the count is taken from board firmware or static board file > definition, so if gpiod_count() returns 0 it should be clear indication > that the driver should not be used with the device. Thank you for this insight, I will update the patch accordingly. Maximilian
On Tue, Jul 16, 2019 at 08:19:04PM +0200, Maximilian Luz wrote: > > Hi, > > On 7/16/19 9:21 AM, Dmitry Torokhov wrote: > > When you are saying that Pro 4 and later models use different > > notifications, does this mean that Pro 4 does not define any GPIOs? > > Unfortunately, at least the Surface Book (first generation, buttons are > handled the same way as on the Pro 4) has GPIOs defined in MSHW0040, but > they are for different use. Specifically: They can detect if the > clipboard (screen part of the device, the device basically has two parts > that can be separated: clipboard and base) is being removed. Relying on > the GPIOs was my first idea too, but that has been reported to shut down > the Book 1 when the clipboard is detached. > > As far as I know, the OEM platform revision check is the easiest way to > differentiate between those devices. OK, fair enough. By the way, I see you are adding some #ifdef CONFIG_ACPI and stubbing out new functions, but the driver does not really work without ACPI (acpi_match_device() will fail in this case I would think and that will cause probe() to abort). So maybe we just add depends on ACPI to the driver's Kconfig entry? Thanks.
On 7/16/19 10:18 PM, Dmitry Torokhov wrote: > OK, fair enough. By the way, I see you are adding some #ifdef > CONFIG_ACPI and stubbing out new functions, but the driver does not > really work without ACPI (acpi_match_device() will fail in this case I > would think and that will cause probe() to abort). So maybe we just add > depends on ACPI to the driver's Kconfig entry? Makes sense, I can change that too. Maximilian
diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c index 5e59f8e57f8e..8bd6909f8d1f 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 @@ -310,6 +321,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 +332,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 +371,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 +411,96 @@ 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); + +#ifdef CONFIG_ACPI + +static int soc_device_check_MSHW0040(struct device *dev) +{ + acpi_handle handle = ACPI_HANDLE(dev); + union acpi_object *result; + u64 oem_platform_rev = 0; + 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 (oem_platform_rev == 0) + return -ENODEV; + + dev_dbg(dev, "OEM Platform Revision %llu\n", oem_platform_rev); + + /* + * We are _really_ expecting GPIOs here. If we do not get any, this + * means the GPIO driver has not been loaded yet (which can happen). + * Try again later. + */ + gpios = gpiod_count(dev, NULL); + if (gpios < 0) + return -EAGAIN; + + return 0; +} + +#else /* CONFIG_ACPI */ + +static int soc_device_check_MSHW0040(struct device *dev) +{ + return -ENODEV; +} + +#endif /* CONFIG_ACPI */ + +/* + * 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 genration 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/soc_button_array.c | 145 +++++++++++++++++++++++--- 1 file changed, 133 insertions(+), 12 deletions(-)