| Message ID | 20190109105245.25954-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFT,v2] gpiolib: acpi: Introduce ACPI_GPIO_QUIRK_ONLY_GPIOIO | expand |
On Wed, Jan 09, 2019 at 12:52:45PM +0200, 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>
On Wed, Jan 9, 2019 at 11:52 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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> > --- > > - it was sent few weeks ago to Hans for testing, but better to re-test > - 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) I tried to apply this but it doesn't apply on the GPIO devel branch. I suppose because of Hans de Goede's commit 72893f0c6bd399ce84e3c1c9fc69d234fe37d098 Author: Hans de Goede <hdegoede@redhat.com> Date: Mon Dec 31 21:55:21 2018 +0100 gpiolib-acpi: Preserve non direction flags when updating gpiod_flags Could you rebase and resend? (Also pick up Mika's ACK.) PS I'm a bit split about pure ACPI patches, part of me see it as an "intel thing" so that you could very well collect it in the Intel GPIO tree and send me pull requests, on the other hand it is ACPI and sometimes applied in Rafael's tree. Maybe just as good that I keep picking them separately like this? Yours, Linus Walleij
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); >
On Thu, Jan 24, 2019 at 2:20 PM Hans de Goede <hdegoede@redhat.com> wrote: > 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> Thanks, Hans, I will resend it rebased and with tags tomorow.
On Mon, Jan 21, 2019 at 02:03:09PM +0100, Linus Walleij wrote: > On Wed, Jan 9, 2019 at 11:52 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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> > > --- > > > > - it was sent few weeks ago to Hans for testing, but better to re-test > > - 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) > > I tried to apply this but it doesn't apply on the GPIO devel > branch. > > I suppose because of Hans de Goede's > commit 72893f0c6bd399ce84e3c1c9fc69d234fe37d098 > Author: Hans de Goede <hdegoede@redhat.com> > Date: Mon Dec 31 21:55:21 2018 +0100 > > gpiolib-acpi: Preserve non direction flags when updating gpiod_flags > > Could you rebase and resend? Linus, as I mentioned in comments above this should go via ASoC tree, there is no conflicts with gpio/devel for GPIO matters. So, please, give your Ack and hopefully Mark will apply this soon. > > (Also pick up Mika's ACK.) > > PS I'm a bit split about pure ACPI patches, part of me see it as an > "intel thing" so that you could very well collect it in the Intel > GPIO tree and send me pull requests, on the other hand it is > ACPI and sometimes applied in Rafael's tree. Maybe just as good > that I keep picking them separately like this? > > Yours, > Linus Walleij
On Fri, Jan 25, 2019 at 07:44:42PM +0200, Andy Shevchenko wrote: > On Mon, Jan 21, 2019 at 02:03:09PM +0100, Linus Walleij wrote: > > On Wed, Jan 9, 2019 at 11:52 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> 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> > > > --- > > > > > > - it was sent few weeks ago to Hans for testing, but better to re-test > > > - 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) > > > > I tried to apply this but it doesn't apply on the GPIO devel > > branch. > > > > I suppose because of Hans de Goede's > > commit 72893f0c6bd399ce84e3c1c9fc69d234fe37d098 > > Author: Hans de Goede <hdegoede@redhat.com> > > Date: Mon Dec 31 21:55:21 2018 +0100 > > > > gpiolib-acpi: Preserve non direction flags when updating gpiod_flags > > > > Could you rebase and resend? > > Linus, as I mentioned in comments above this should go via ASoC tree, there is > no conflicts with gpio/devel for GPIO matters. > > So, please, give your Ack and hopefully Mark will apply this soon. Perhaps Mark can provide you an immutable branch to be merged. > > (Also pick up Mika's ACK.) > > > > PS I'm a bit split about pure ACPI patches, part of me see it as an > > "intel thing" so that you could very well collect it in the Intel > > GPIO tree and send me pull requests, on the other hand it is > > ACPI and sometimes applied in Rafael's tree. Maybe just as good > > that I keep picking them separately like this? I dunno. Let Mika speak up.
On Fri, Jan 25, 2019 at 6:44 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, Jan 21, 2019 at 02:03:09PM +0100, Linus Walleij wrote: > > On Wed, Jan 9, 2019 at 11:52 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> 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> > > > --- > > > > > > - it was sent few weeks ago to Hans for testing, but better to re-test > > > - 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) > > > > I tried to apply this but it doesn't apply on the GPIO devel > > branch. > > > > I suppose because of Hans de Goede's > > commit 72893f0c6bd399ce84e3c1c9fc69d234fe37d098 > > Author: Hans de Goede <hdegoede@redhat.com> > > Date: Mon Dec 31 21:55:21 2018 +0100 > > > > gpiolib-acpi: Preserve non direction flags when updating gpiod_flags > > > > Could you rebase and resend? > > Linus, as I mentioned in comments above this should go via ASoC tree, there is > no conflicts with gpio/devel for GPIO matters. > > So, please, give your Ack and hopefully Mark will apply this soon. OK with me: Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
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);
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> --- - it was sent few weeks ago to Hans for testing, but better to re-test - 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(-)