Re: [PATCH 2/2] Input: soc_button_array: Add support for ACPI 6.0 Generic Button Device

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Hans,

On Tue, Feb 14, 2017 at 03:10:06PM +0100, Hans de Goede wrote:
> Windows 10 tablets with gpio buttons will typically use the ACPI 6.0
> Generic Button Device with a HID of ACPI0011 for these buttons.
> 
> The ACPI description for these in the ACPI0011 devices _DSD object uses
> something resembling HID descriptors, except that instead of indicating
> a bit index into a HID input report, the index indicates the _CRS index
> for the GPIO.
> 
> The use of 1 interrupt per button, some of which need to be wakeup
> sources, instead of using input reports makes it impossible to use the
> HID subsystem for this.
> 
> This really is just another gpio-keys input device with the platform
> data described in ACPI, so this commit adds parsing for this new way
> to describe gpio-keys to the soc_button_array driver.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/input/misc/soc_button_array.c | 158 +++++++++++++++++++++++++++++++++-
>  1 file changed, 157 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
> index eb1ba4e..a52eb12 100644
> --- a/drivers/input/misc/soc_button_array.c
> +++ b/drivers/input/misc/soc_button_array.c
> @@ -138,6 +138,152 @@ soc_button_device_create(struct platform_device *pdev,
>  	return ERR_PTR(error);
>  }
>  
> +static int soc_button_get_acpi_object_int(const union acpi_object *obj)
> +{
> +	if (obj->type != ACPI_TYPE_INTEGER)
> +		return -1;
> +
> +	return obj->integer.value;
> +}
> +
> +/* Parse a single ACPI0011 _DSD button descriptor */
> +static int soc_button_parse_btn_desc(struct device *dev,
> +				     const union acpi_object *desc,
> +				     int collection_uid,
> +				     struct soc_button_info *info)
> +{
> +	int upage, usage;
> +
> +	if (desc->type != ACPI_TYPE_PACKAGE ||
> +	    desc->package.count != 5 ||
> +	    /* First byte should be 1 (control) */
> +	    soc_button_get_acpi_object_int(&desc->package.elements[0]) != 1 ||
> +	    /* Third byte should be collection uid */
> +	    soc_button_get_acpi_object_int(&desc->package.elements[2]) !=
> +							    collection_uid) {
> +		dev_err(dev, "Invalid ACPI Button Descriptor\n");
> +		return -ENODEV;
> +	}
> +
> +	info->event_type = EV_KEY;
> +	info->acpi_index =
> +		soc_button_get_acpi_object_int(&desc->package.elements[1]);
> +	upage = soc_button_get_acpi_object_int(&desc->package.elements[3]);
> +	usage = soc_button_get_acpi_object_int(&desc->package.elements[4]);
> +
> +	/*
> +	 * The UUID: fa6bd625-9ce8-470d-a2c7-b3ca36c4282e descriptors use HID
> +	 * usage page and usage codes, but otherwise the device is not HID
> +	 * compliant: it uses one irq per button instead of generating HID
> +	 * input reports and some buttons should generate wakeups where as
> +	 * others should not, so we cannot use the HID subsystem.
> +	 *
> +	 * Luckily all devices only use a few usage page + usage combinations,
> +	 * so we can simply check for the known combinations here.
> +	 */
> +	if (upage == 0x01 && usage == 0x81) {
> +		info->name = "power";
> +		info->event_code = KEY_POWER;
> +		info->wakeup = true;
> +	} else if (upage == 0x07 && usage == 0xe3) {
> +		info->name = "home";
> +		info->event_code = KEY_HOMEPAGE;
> +		info->wakeup = true;
> +	} else if (upage == 0x0c && usage == 0xe9) {
> +		info->name = "volume_up";
> +		info->event_code = KEY_VOLUMEUP;
> +		info->autorepeat = true;
> +	} else if (upage == 0x0c && usage == 0xea) {
> +		info->name = "volume_down";
> +		info->event_code = KEY_VOLUMEDOWN;
> +		info->autorepeat = true;
> +	} else {
> +		dev_warn(dev, "Unknown button index %d upage %02x usage %02x, ignoring\n",
> +			 info->acpi_index, upage, usage);
> +		info->name = "unkown";
> +		info->event_code = KEY_RESERVED;
> +	}
> +
> +	return 0;
> +}
> +
> +/* ACPI0011 _DSD btns descriptors UUID: fa6bd625-9ce8-470d-a2c7-b3ca36c4282e */
> +static const u8 btns_desc_uuid[16] = {
> +	0x25, 0xd6, 0x6b, 0xfa, 0xe8, 0x9c, 0x0d, 0x47,
> +	0xa2, 0xc7, 0xb3, 0xca, 0x36, 0xc4, 0x28, 0x2e
> +};
> +
> +/* Parse ACPI0011 _DSD button descriptors */
> +static struct soc_button_info *soc_button_get_button_info(struct device *dev)
> +{
> +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
> +	const union acpi_object *desc, *uuid, *btns_desc = NULL;
> +	struct soc_button_info *button_info;
> +	acpi_status status;
> +	int i, btn, collection_uid = -1;
> +
> +	status = acpi_evaluate_object_typed(ACPI_HANDLE(dev), "_DSD", NULL,
> +					    &buf, ACPI_TYPE_PACKAGE);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(dev, "ACPI _DSD object not found\n");
> +		return NULL;
> +	}
> +
> +	/* Look for the Button Descriptors UUID */
> +	desc = buf.pointer;
> +	for (i = 0; (i + 1) < desc->package.count; i += 2) {
> +		uuid = &desc->package.elements[i];
> +
> +		if (uuid->type != ACPI_TYPE_BUFFER || uuid->buffer.length != 16
> +		    || desc->package.elements[i + 1].type != ACPI_TYPE_PACKAGE)
> +			break;;

Are you sure you want to break and not continue? Maybe the whole thing
should be:

		const union acpi_object *uuid = &desc->package.elements[i];
		const union acpi_object *bdsc = &desc->package.elements[i + 1];


		if (uuid->type == ACPI_TYPE_BUFFER &&
		    uuid->buffer.length == 16 &&
		    memcmp(uuid->buffer.pointer, btns_desc_uuid, 16) == 0 &&
		    bdsc->type == ACPI_TYPE_PACKAGE) {
			btns_desc = bdsc;
			break;
		}

> +
> +		if (memcmp(uuid->buffer.pointer, btns_desc_uuid, 16) == 0) {
> +			btns_desc = &desc->package.elements[i + 1];
> +			break;
> +		}
> +	}
> +
> +	if (!btns_desc) {
> +		dev_err(dev, "ACPI Button Descriptors not found\n");
> +		return NULL;
> +	}
> +
> +	/* There are package.count - 1 buttons + 1 terminating empty entry */
> +	button_info = devm_kcalloc(dev, btns_desc->package.count,
> +				   sizeof(*button_info), GFP_KERNEL);
> +	if (!button_info)
> +		return NULL;
> +
> +	/* The first package describes the collection */
> +	if (btns_desc->package.elements[0].type == ACPI_TYPE_PACKAGE &&
> +	    btns_desc->package.elements[0].package.count == 5 &&
> +	    /* First byte should be 0 (collection) */
> +	    soc_button_get_acpi_object_int(
> +		    &btns_desc->package.elements[0].package.elements[0]) == 0 &&
> +	    /* Third byte should be 0 (top level collection) */
> +	    soc_button_get_acpi_object_int(
> +		    &btns_desc->package.elements[0].package.elements[2]) == 0) {
> +		collection_uid = soc_button_get_acpi_object_int(
> +			&btns_desc->package.elements[0].package.elements[1]);
> +	}
> +	if (collection_uid == -1) {
> +		dev_err(dev, "Invalid Button Collection Descriptor\n");
> +		return NULL;
> +	}

Should we do it before trying to allocate memory? Also maybe have a
temporary, like el0 for btns_desc->package.elements[0]?

> +
> +	/* Parse the button descriptors */
> +	for (i = 1, btn = 0; i < btns_desc->package.count; i++, btn++) {
> +		if (soc_button_parse_btn_desc(dev,
> +					      &btns_desc->package.elements[i],
> +					      collection_uid,
> +					      &button_info[btn]))
> +			return NULL;
> +	}
> +
> +	return button_info;
> +}
> +
>  static int soc_button_remove(struct platform_device *pdev)
>  {
>  	struct soc_button_data *priv = platform_get_drvdata(pdev);
> @@ -165,7 +311,13 @@ static int soc_button_probe(struct platform_device *pdev)
>  	if (!id)
>  		return -ENODEV;
>  
> -	button_info = (struct soc_button_info *)id->driver_data;
> +	if (!id->driver_data) {
> +		button_info = soc_button_get_button_info(dev);
> +		if (!button_info)
> +			return -ENODEV;

Better have soc_button_get_button_info() return ERR_PTR-encoded errors
and propagate them to the callers.

> +	} else {
> +		button_info = (struct soc_button_info *)id->driver_data;
> +	}
>  
>  	if (gpiod_count(dev, KBUILD_MODNAME) <= 0) {
>  		dev_dbg(dev, "no GPIO attached, ignoring...\n");
> @@ -195,6 +347,9 @@ static int soc_button_probe(struct platform_device *pdev)
>  	if (!priv->children[0] && !priv->children[1])
>  		return -ENODEV;
>  
> +	if (!id->driver_data)
> +		devm_kfree(dev, button_info);
> +
>  	return 0;
>  }
>  
> @@ -214,6 +369,7 @@ static struct soc_button_info soc_button_PNP0C40[] = {
>  
>  static const struct acpi_device_id soc_button_acpi_match[] = {
>  	{ "PNP0C40", (unsigned long)soc_button_PNP0C40 },
> +	{ "ACPI0011", 0 },
>  	{ }
>  };
>  
> -- 
> 2.9.3
> 

Thanks.

-- 
Dmitry