Re: [alsa-devel] [RFT][PATCH v2] gpiolib: acpi: Introduce ACPI_GPIO_QUIRK_ONLY_GPIOIO

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 09-01-19 11:52, Andy Shevchenko wrote:
> New quirk enforces search for GPIO based on its type,
> i.e. iterate over GpioIo resources only.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> 
> - it was sent few weeks ago to Hans for testing, but better to re-test

I've just re-tested this on a device which uses the speaker-enable
GPIO with a rt5651 codec, still works as advertised:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> - it's supposed to go via ASoC subsystem due to recent changes made for
>    sound driver
> 
> v2:
> - Expand explanation why this quirk might be needed (Mika)
> 
>   drivers/gpio/gpiolib-acpi.c           | 15 ++++--
>   include/linux/acpi.h                  |  7 +++
>   sound/soc/intel/boards/bytcr_rt5651.c | 74 ++++-----------------------
>   3 files changed, 27 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 259cf6ab969b..4d291b75cb9f 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -530,17 +530,24 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
>   	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
>   		return 1;
>   
> -	if (lookup->n++ == lookup->index && !lookup->desc) {
> +	if (!lookup->desc) {
>   		const struct acpi_resource_gpio *agpio = &ares->data.gpio;
> -		int pin_index = lookup->pin_index;
> +		bool gpioint = agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
> +		int pin_index;
>   
> +		if (lookup->info.quirks & ACPI_GPIO_QUIRK_ONLY_GPIOIO && gpioint)
> +			lookup->index++;
> +
> +		if (lookup->n++ != lookup->index)
> +			return 1;
> +
> +		pin_index = lookup->pin_index;
>   		if (pin_index >= agpio->pin_table_length)
>   			return 1;
>   
>   		lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
>   					      agpio->pin_table[pin_index]);
> -		lookup->info.gpioint =
> -			agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
> +		lookup->info.gpioint = gpioint;
>   
>   		/*
>   		 * Polarity and triggering are only specified for GpioInt
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 1e89c688139f..430fd0b6df7b 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1020,6 +1020,13 @@ struct acpi_gpio_mapping {
>   
>   /* Ignore IoRestriction field */
>   #define ACPI_GPIO_QUIRK_NO_IO_RESTRICTION	BIT(0)
> +/*
> + * When ACPI GPIO mapping table is in use the index parameter inside it
> + * refers to the GPIO resource in _CRS method. That index has no
> + * distinction of actual type of the resource. When consumer wants to
> + * get GpioIo type explicitly, this quirk may be used.
> + */
> +#define ACPI_GPIO_QUIRK_ONLY_GPIOIO		BIT(1)
>   
>   	unsigned int quirks;
>   };
> diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
> index b618d984e2d5..99d0c01a1bec 100644
> --- a/sound/soc/intel/boards/bytcr_rt5651.c
> +++ b/sound/soc/intel/boards/bytcr_rt5651.c
> @@ -844,74 +844,18 @@ static const struct x86_cpu_id cherrytrail_cpu_ids[] = {
>   	{}
>   };
>   
> -static const struct acpi_gpio_params first_gpio = { 0, 0, false };
> -static const struct acpi_gpio_params second_gpio = { 1, 0, false };
> +static const struct acpi_gpio_params ext_amp_enable_gpios = { 0, 0, false };
>   
> -static const struct acpi_gpio_mapping byt_rt5651_amp_en_first[] = {
> -	{ "ext-amp-enable-gpios", &first_gpio, 1 },
> -	{ },
> -};
> -
> -static const struct acpi_gpio_mapping byt_rt5651_amp_en_second[] = {
> -	{ "ext-amp-enable-gpios", &second_gpio, 1 },
> +static const struct acpi_gpio_mapping cht_rt5651_gpios[] = {
> +	/*
> +	 * Some boards have I2cSerialBusV2, GpioIo, GpioInt as ACPI resources,
> +	 * other boards may  have I2cSerialBusV2, GpioInt, GpioIo instead.
> +	 * We want the GpioIo one for the ext-amp-enable-gpio.
> +	 */
> +	{ "ext-amp-enable-gpios", &ext_amp_enable_gpios, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
>   	{ },
>   };
>   
> -/*
> - * Some boards have I2cSerialBusV2, GpioIo, GpioInt as ACPI resources, other
> - * boards may  have I2cSerialBusV2, GpioInt, GpioIo instead. We want the
> - * GpioIo one for the ext-amp-enable-gpio and both count for the index in
> - * acpi_gpio_params index.  So we have 2 different mappings and the code
> - * below figures out which one to use.
> - */
> -struct byt_rt5651_acpi_resource_data {
> -	int gpio_count;
> -	int gpio_int_idx;
> -};
> -
> -static int snd_byt_rt5651_acpi_resource(struct acpi_resource *ares, void *arg)
> -{
> -	struct byt_rt5651_acpi_resource_data *data = arg;
> -
> -	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
> -		return 0;
> -
> -	if (ares->data.gpio.connection_type == ACPI_RESOURCE_GPIO_TYPE_INT)
> -		data->gpio_int_idx = data->gpio_count;
> -
> -	data->gpio_count++;
> -	return 0;
> -}
> -
> -static void snd_byt_rt5651_mc_pick_amp_en_gpio_mapping(struct device *codec)
> -{
> -	struct byt_rt5651_acpi_resource_data data = { 0, -1 };
> -	LIST_HEAD(resources);
> -	int ret;
> -
> -	ret = acpi_dev_get_resources(ACPI_COMPANION(codec), &resources,
> -				     snd_byt_rt5651_acpi_resource, &data);
> -	if (ret < 0) {
> -		dev_warn(codec, "Failed to get ACPI resources, not adding external amplifier GPIO mapping\n");
> -		return;
> -	}
> -
> -	/* All info we need is gathered during the walk */
> -	acpi_dev_free_resource_list(&resources);
> -
> -	switch (data.gpio_int_idx) {
> -	case 0:
> -		byt_rt5651_gpios = byt_rt5651_amp_en_second;
> -		break;
> -	case 1:
> -		byt_rt5651_gpios = byt_rt5651_amp_en_first;
> -		break;
> -	default:
> -		dev_warn(codec, "Unknown GpioInt index %d, not adding external amplifier GPIO mapping\n",
> -			 data.gpio_int_idx);
> -	}
> -}
> -
>   struct acpi_chan_package {   /* ACPICA seems to require 64 bit integers */
>   	u64 aif_value;       /* 1: AIF1, 2: AIF2 */
>   	u64 mclock_value;    /* usually 25MHz (0x17d7940), ignored */
> @@ -1037,7 +981,7 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
>   
>   	/* Cherry Trail devices use an external amplifier enable gpio */
>   	if (x86_match_cpu(cherrytrail_cpu_ids) && !byt_rt5651_gpios)
> -		snd_byt_rt5651_mc_pick_amp_en_gpio_mapping(codec_dev);
> +		byt_rt5651_gpios = cht_rt5651_gpios;
>   
>   	if (byt_rt5651_gpios) {
>   		devm_acpi_dev_add_driver_gpios(codec_dev, byt_rt5651_gpios);
>