Skip to content

Conversation

@plbossart
Copy link
Member

we no longer need a 'depends on GPIOLIB' for the DMIC stuff, but conversely we are missing dependencies for 8 machine drivers.

kv2019i
kv2019i previously approved these changes Feb 3, 2022
@fredoh9
Copy link
Collaborator

fredoh9 commented Feb 3, 2022

Might be dull question. I was curious about last commit which is about missing gpiolib dependency on 8 machine drivers. I checked on one of machine driver, sof-pcm512x.c. But I don't see explicit gpio access. Even without last commit I'm able to build the kernel. Can you educate me about last commit?

@plbossart
Copy link
Member Author

Might be dull question. I was curious about last commit which is about missing gpiolib dependency on 8 machine drivers. I checked on one of machine driver, sof-pcm512x.c. But I don't see explicit gpio access. Even without last commit I'm able to build the kernel. Can you educate me about last commit?

Correct @fredoh9 in the upstream version there's no GPIOLIB in use. This would be needed with additional code to deal with the 'codec producer' mode in #1945 (abandoned).

@plbossart
Copy link
Member Author

reworded the last commit message based on reviews from @fredoh9 and @ujfalusi. Thanks!

@plbossart plbossart requested review from kv2019i and ujfalusi February 4, 2022 14:55
@plbossart
Copy link
Member Author

@ujfalusi @kv2019i @bardliao can you take a look?

ujfalusi
ujfalusi previously approved these changes Feb 14, 2022
@bardliao
Copy link
Collaborator

@plbossart How do we judge is GPIOLIB is needed or not? For example, devm_acpi_dev_add_driver_gpios is used, and linux/gpio/consumer.h is included in bytcht_cx2072x.c, but we don't add depends on GPIOLIB || COMPILE_TEST for config SND_SOC_INTEL_BYT_CHT_CX2072X_MACH

@plbossart
Copy link
Member Author

plbossart commented Feb 14, 2022

Good point @bardliao. I think we need GPIOLIB support for all of those drivers:

bdw-rt5677.c:   gpiod_set_value_cansleep(bdw_rt5677->gpio_hp_en,
bdw-rt5677.c:   bdw_rt5677->gpio_hp_en = gpiod_get(component->dev, "headphone-enable",
bdw-rt5677.c:           headphone_jack_gpio.gpiod_dev = component->dev;
bdw-rt5677.c:           mic_jack_gpio.gpiod_dev = component->dev;
bdw-rt5677.c:    * so explicitly test if the gpiod is valid
bdw-rt5677.c:           gpiod_put(bdw_rt5677->gpio_hp_en);
bdw-rt5677.c:           if (snd_soc_jack_add_gpios(&headphone_jack, 1,
bdw-rt5677.c:           if (snd_soc_jack_add_gpios(&mic_jack, 1, &mic_jack_gpio))
bdw-rt5677.c:   ret = devm_acpi_dev_add_driver_gpios(component->dev, bdw_rt5677_gpios);
bytcht_cx2072x.c:       if (devm_acpi_dev_add_driver_gpios(codec->dev,
bytcht_es8316.c:        gpiod_set_value_cansleep(priv->speaker_en_gpio, priv->speaker_en);
bytcht_es8316.c:                gpiod_get_optional(codec_dev, "speaker-enable",
bytcht_es8316.c:                gpiod_put(priv->speaker_en_gpio);
bytcht_es8316.c:        gpiod_put(priv->speaker_en_gpio);
bytcht_es8316.c:        devm_acpi_dev_add_driver_gpios(codec_dev, byt_cht_es8316_gpios);
bytcr_rt5640.c: jack_status = gpiod_get_value_cansleep(rt5640_jack_gpio.desc);
bytcr_rt5640.c: mic_status = gpiod_get_value_cansleep(priv->hsmic_detect);
bytcr_rt5640.c: jack_status = gpiod_get_value_cansleep(rt5640_jack2_gpio.desc);
bytcr_rt5640.c:         data->jd_gpio = devm_fwnode_gpiod_get(card->dev, acpi_fwnode_handle(adev),
bytcr_rt5640.c:         rt5640_jack_gpio.gpiod_dev = priv->codec_dev;
bytcr_rt5640.c:         rt5640_jack2_gpio.gpiod_dev = priv->codec_dev;
bytcr_rt5640.c:         priv->hsmic_detect = devm_fwnode_gpiod_get(dev, codec_dev->fwnode,
bytcr_rt5640.c:         ret = snd_soc_jack_add_gpios(&priv->jack, 1, &rt5640_jack_gpio);
bytcr_rt5640.c:         ret = snd_soc_jack_add_gpios(&priv->jack2, 1, &rt5640_jack2_gpio);
bytcr_rt5640.c:         acpi_dev_add_driver_gpios(adev, amcr0f28_gpios);
bytcr_rt5640.c:         acpi_dev_add_driver_gpios(ACPI_COMPANION(priv->codec_dev),
bytcr_rt5651.c:         gpiod_set_value_cansleep(priv->ext_amp_gpio, 1);
bytcr_rt5651.c:         gpiod_set_value_cansleep(priv->ext_amp_gpio, 0);
bytcr_rt5651.c:         priv->ext_amp_gpio = devm_fwnode_gpiod_get(dev, codec_dev->fwnode,
bytcr_rt5651.c:         priv->hp_detect = devm_fwnode_gpiod_get(dev, codec_dev->fwnode,
bytcr_rt5651.c:         devm_acpi_dev_add_driver_gpios(codec_dev, byt_rt5651_gpios);
bytcr_wm5102.c: gpiod_set_value_cansleep(priv->spkvdd_en_gpio,
bytcr_wm5102.c: /* Note no devm_ here since we call gpiod_get on codec_dev rather then dev */
bytcr_wm5102.c: priv->spkvdd_en_gpio = gpiod_get(codec_dev, "wlf,spkvdd-ena", GPIOD_OUT_LOW);
bytcr_wm5102.c: gpiod_put(priv->spkvdd_en_gpio);
bytcr_wm5102.c: gpiod_put(priv->spkvdd_en_gpio);
cht_bsw_max98090_ti.c:  ret = snd_soc_jack_add_gpiods(runtime->card->dev->parent, jack,
cht_bsw_max98090_ti.c:          ret_val = devm_acpi_dev_add_driver_gpios(dev->parent,
cht_bsw_rt5672.c:       if (devm_acpi_dev_add_driver_gpios(component->dev, cht_rt5672_gpios))
kbl_rt5660.c:   gpiod_set_value_cansleep(priv->gpio_lo_mute,
kbl_rt5660.c:   ctx->gpio_lo_mute = gpiod_get(component->dev, "lineout-mute",
kbl_rt5660.c:           lineout_jack_gpio.gpiod_dev = component->dev;
kbl_rt5660.c:           mic_jack_gpio.gpiod_dev = component->dev;
kbl_rt5660.c:    * so explicitly test if the gpiod is valid
kbl_rt5660.c:           gpiod_put(ctx->gpio_lo_mute);
kbl_rt5660.c:           ret = snd_soc_jack_add_gpios(&lineout_jack, 1,
kbl_rt5660.c:           ret = snd_soc_jack_add_gpios(&mic_jack, 1, &mic_jack_gpio);
kbl_rt5660.c:   ret = devm_acpi_dev_add_driver_gpios(component->dev, acpi_rt5660_gpios);
sof_es8336.c:   gpiod_set_value_cansleep(priv->gpio_pa, priv->speaker_en);
sof_es8336.c:   priv->gpio_pa = gpiod_get(codec_dev, "pa-enable", GPIOD_OUT_LOW);
sof_es8336.c:           gpiod_put(priv->gpio_pa);
sof_es8336.c:   gpiod_put(priv->gpio_pa);
sof_es8336.c:   ret = devm_acpi_dev_add_driver_gpios(codec_dev, gpio_mapping);
sof_wm8804.c:                   gpiod_set_value_cansleep(ctx->gpio_48, !clk_44);
sof_wm8804.c:                   gpiod_set_value_cansleep(ctx->gpio_44, clk_44);
sof_wm8804.c:                   gpiod_set_value_cansleep(ctx->gpio_44, clk_44);
sof_wm8804.c:                   gpiod_set_value_cansleep(ctx->gpio_48, !clk_44);
sof_wm8804.c:static struct gpiod_lookup_table up2_gpios_table = {
sof_wm8804.c:           gpiod_add_lookup_table(&up2_gpios_table);
sof_wm8804.c:           ctx->gpio_44 = devm_gpiod_get(&pdev->dev, "BCM-GPIO5",
sof_wm8804.c:           ctx->gpio_48 = devm_gpiod_get(&pdev->dev, "BCM-GPIO6",
sof_wm8804.c:           gpiod_remove_lookup_table(&up2_gpios_table);

bardliao
bardliao previously approved these changes Feb 15, 2022
…is used"

This patch reverts commit 4262ddc ("ASoC: Intel: boards: add
explicit dependency on GPIOLIB when DMIC is used") and all follow-up
additions of this dependency.

Now that the DMIC does not depend on GPIOLIB we can simplify again.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We have eleven machine drivers who make explicit references to
gpios. Let's add the dependency.

The use of 'depends on' instead of 'select' is intentional. On one
side it could be argued that the GPIOs are required, but on the other
it might create more issues with randconfig builds. This patch sticks
with the existing direction of using 'depends' on high-level non-audio
dependencies

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart requested a review from bardliao March 1, 2022 20:04
@plbossart plbossart merged commit 23824cb into thesofproject:topic/sof-dev Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants