Skip to content

Fix sof_8336 gpio problems #4066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yangxiaohua2009 wants to merge 1 commit into from
Closed

Fix sof_8336 gpio problems #4066

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 17 additions & 56 deletions sound/soc/intel/boards/sof_es8336.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
#define SOF_ES8336_SSP_CODEC(quirk) ((quirk) & GENMASK(3, 0))
#define SOF_ES8336_SSP_CODEC_MASK (GENMASK(3, 0))

#define SOF_ES8336_SPEAKERS_EN_GPIO1_QUIRK BIT(4)

/* HDMI capture*/
#define SOF_SSP_HDMI_CAPTURE_PRESENT BIT(14)
#define SOF_NO_OF_HDMI_CAPTURE_SSP_SHIFT 15
Expand All @@ -48,7 +46,6 @@

#define SOF_ES8336_ENABLE_DMIC BIT(5)
#define SOF_ES8336_JD_INVERTED BIT(6)
#define SOF_ES8336_HEADPHONE_GPIO BIT(7)
#define SOC_ES8336_HEADSET_MIC1 BIT(8)

static unsigned long quirk;
Expand All @@ -72,40 +69,21 @@ struct sof_hdmi_pcm {
int device;
};

static const struct acpi_gpio_params enable_gpio0 = { 0, 0, true };
static const struct acpi_gpio_params enable_gpio1 = { 1, 0, true };

static const struct acpi_gpio_mapping acpi_speakers_enable_gpio0[] = {
{ "speakers-enable-gpios", &enable_gpio0, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
{ }
};

static const struct acpi_gpio_mapping acpi_speakers_enable_gpio1[] = {
{ "speakers-enable-gpios", &enable_gpio1, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
};
static const struct acpi_gpio_params enable_gpio0 = { 0, 0, false };
static const struct acpi_gpio_params enable_gpio1 = { 1, 0, false };

static const struct acpi_gpio_mapping acpi_enable_both_gpios[] = {
{ "speakers-enable-gpios", &enable_gpio0, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
{ "headphone-enable-gpios", &enable_gpio1, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
{ }
};

static const struct acpi_gpio_mapping acpi_enable_both_gpios_rev_order[] = {
{ "speakers-enable-gpios", &enable_gpio1, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
{ "headphone-enable-gpios", &enable_gpio0, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
{ }
};

Copy link
Member

@plbossart plbossart Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this removed? this was added for a reason...

Copy link
Author

@yangxiaohua2009 yangxiaohua2009 Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When two GPIOs are used, Speaker GPIO is always the first and HP GPIO the second. The user may need a quirk to invert the GPIO level.

static void log_quirks(struct device *dev)
{
dev_info(dev, "quirk mask %#lx\n", quirk);
dev_info(dev, "quirk SSP%ld\n", SOF_ES8336_SSP_CODEC(quirk));
if (quirk & SOF_ES8336_ENABLE_DMIC)
dev_info(dev, "quirk DMIC enabled\n");
if (quirk & SOF_ES8336_SPEAKERS_EN_GPIO1_QUIRK)
dev_info(dev, "Speakers GPIO1 quirk enabled\n");
if (quirk & SOF_ES8336_HEADPHONE_GPIO)
dev_info(dev, "quirk headphone GPIO enabled\n");
if (quirk & SOF_ES8336_JD_INVERTED)
dev_info(dev, "quirk JD inverted enabled\n");
if (quirk & SOC_ES8336_HEADSET_MIC1)
Expand All @@ -119,8 +97,7 @@ static void pcm_pop_work_events(struct work_struct *work)

gpiod_set_value_cansleep(priv->gpio_speakers, priv->speaker_en);

if (quirk & SOF_ES8336_HEADPHONE_GPIO)
gpiod_set_value_cansleep(priv->gpio_headphone, priv->speaker_en);
gpiod_set_value_cansleep(priv->gpio_headphone, true);
Copy link
Member

@plbossart plbossart Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the setting for headphone independent from the speaker enablement? Shouldn't the value be !priv->speaker_en?

Copy link
Author

@yangxiaohua2009 yangxiaohua2009 Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaker GPIO Headphone GPIO
Playing music Speaker on high low
Playing music Headphone on low high
Suspend low low

Copy link
Author

@yangxiaohua2009 yangxiaohua2009 Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When only one GPIO is used things are simple. The GPIO is high by default. Before playing music, the SPEAKER_ON dapm event is called and the GPIO set to high. When music stop, the GPIO is set to low so that there is no pop noise.

When HP GPIO is involved things become complicated. I want to do the same for HP GPIO but I don't know how. Maybe another SND_SOC_DAPM_SUPPLY for headphone should be invloved or maybe the headphone status can be read by snd_soc_dapm_get_pin_status(&card->dapm, "Headphone"); and the headphone can be turned on and off in sof_8336_trigger


}

Expand All @@ -139,10 +116,10 @@ static int sof_8336_trigger(struct snd_pcm_substream *substream, int cmd)
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
if (priv->speaker_en == false)
if (priv->speaker_en == true)
if (substream->stream == 0) {
cancel_delayed_work(&priv->pcm_pop_work);
gpiod_set_value_cansleep(priv->gpio_speakers, true);
gpiod_set_value_cansleep(priv->gpio_speakers, false);
}
break;
default:
Expand All @@ -158,10 +135,10 @@ static int sof_es8316_speaker_power_event(struct snd_soc_dapm_widget *w,
struct snd_soc_card *card = w->dapm->card;
struct sof_es8336_private *priv = snd_soc_card_get_drvdata(card);

if (priv->speaker_en == !SND_SOC_DAPM_EVENT_ON(event))
return 0;
dev_info(card->dev, "sof_es8316_speaker_power_event %d\n",SND_SOC_DAPM_EVENT_ON(event));
Copy link
Member

@plbossart plbossart Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dev_info is way too verbose.

/* Redundant check, removed */
Copy link
Member

@plbossart plbossart Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it redundant? and if yes, it should be a separate patch to avoid confusing reviewers between different functionality and code cleanups.

Copy link
Author

@yangxiaohua2009 yangxiaohua2009 Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check will return 0 when first time the event is called. So the gpios are always the default value the first time sound is played. Should be a seperate patch.


priv->speaker_en = !SND_SOC_DAPM_EVENT_ON(event);
priv->speaker_en = SND_SOC_DAPM_EVENT_ON(event);

queue_delayed_work(system_wq, &priv->pcm_pop_work, msecs_to_jiffies(70));
return 0;
Expand Down Expand Up @@ -302,6 +279,8 @@ static int sof_es8316_init(struct snd_soc_pcm_runtime *runtime)
snd_jack_set_key(priv->jack.jack, SND_JACK_BTN_0, KEY_PLAYPAUSE);

snd_soc_component_set_jack(codec, &priv->jack, NULL);
ret = snd_soc_dapm_get_pin_status(&card->dapm, "Speaker");
dev_info(card->dev, "Speaker status %d", ret);
Copy link
Member

@plbossart plbossart Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?


return 0;
}
Expand Down Expand Up @@ -332,22 +311,13 @@ static int sof_es8336_quirk_cb(const struct dmi_system_id *id)
* if the topology file is modified as well.
*/
static const struct dmi_system_id sof_es8336_quirk_table[] = {
{
.callback = sof_es8336_quirk_cb,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "IP3 tech"),
DMI_MATCH(DMI_BOARD_NAME, "WN1"),
},
.driver_data = (void *)(SOF_ES8336_SPEAKERS_EN_GPIO1_QUIRK)
},
{
.callback = sof_es8336_quirk_cb,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "HUAWEI"),
DMI_MATCH(DMI_BOARD_NAME, "BOHB-WAX9-PCB-B2"),
},
.driver_data = (void *)(SOF_ES8336_HEADPHONE_GPIO |
SOC_ES8336_HEADSET_MIC1)
.driver_data = (void *)(SOC_ES8336_HEADSET_MIC1)
},
{}
};
Expand Down Expand Up @@ -722,36 +692,28 @@ static int sof_es8336_probe(struct platform_device *pdev)
}
}

/* get speaker enable GPIO */
if (quirk & SOF_ES8336_HEADPHONE_GPIO) {
if (quirk & SOF_ES8336_SPEAKERS_EN_GPIO1_QUIRK)
gpio_mapping = acpi_enable_both_gpios;
else
gpio_mapping = acpi_enable_both_gpios_rev_order;
} else if (quirk & SOF_ES8336_SPEAKERS_EN_GPIO1_QUIRK) {
gpio_mapping = acpi_speakers_enable_gpio1;
} else {
gpio_mapping = acpi_speakers_enable_gpio0;
}
/* Always enable both GPIOs */
gpio_mapping = acpi_enable_both_gpios;

ret = devm_acpi_dev_add_driver_gpios(codec_dev, gpio_mapping);
if (ret)
dev_warn(codec_dev, "unable to add GPIO mapping table\n");

priv->gpio_speakers = gpiod_get_optional(codec_dev, "speakers-enable", GPIOD_OUT_LOW);
priv->gpio_speakers = gpiod_get_optional(codec_dev, "speakers-enable", GPIOD_OUT_HIGH);
if (IS_ERR(priv->gpio_speakers)) {
ret = dev_err_probe(dev, PTR_ERR(priv->gpio_speakers),
"could not get speakers-enable GPIO\n");
goto err_put_codec;
}
gpiod_set_value_cansleep(priv->gpio_speakers, false);

priv->gpio_headphone = gpiod_get_optional(codec_dev, "headphone-enable", GPIOD_OUT_LOW);
priv->gpio_headphone = gpiod_get_optional(codec_dev, "headphone-enable", GPIOD_OUT_HIGH);
Copy link
Member

@plbossart plbossart Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this intentional? why would the GPIO level change?

Copy link
Author

@yangxiaohua2009 yangxiaohua2009 Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the past speaker_en false means on (high) and true means off (low) because the code is taken from bytcht driver. The patch inverts this.

Copy link
Member

@plbossart plbossart Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this is going to break existing solutions, I really don't understand this change.

if (IS_ERR(priv->gpio_headphone)) {
ret = dev_err_probe(dev, PTR_ERR(priv->gpio_headphone),
"could not get headphone-enable GPIO\n");
goto err_put_codec;
}

Copy link
Member

@plbossart plbossart Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spurious line change

INIT_LIST_HEAD(&priv->hdmi_pcm_list);
INIT_DELAYED_WORK(&priv->pcm_pop_work,
pcm_pop_work_events);
Expand Down Expand Up @@ -802,7 +764,6 @@ static const struct platform_device_id board_ids[] = {
SOF_HDMI_CAPTURE_1_SSP(0) |
SOF_HDMI_CAPTURE_2_SSP(2) |
SOF_SSP_HDMI_CAPTURE_PRESENT |
SOF_ES8336_SPEAKERS_EN_GPIO1_QUIRK |
Copy link
Member

@plbossart plbossart Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if gpio1 was used, why did you remove this?

Copy link
Author

@yangxiaohua2009 yangxiaohua2009 Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows GPIO0 is always the speaker GPIO. The quirk may be reused to invert the GPIO level. (The results are the same: either invert the level or invert the order.)

Copy link
Member

@plbossart plbossart Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't follow this "either invert the level or invert the order". How is this equivalent?

Copy link
Author

@yangxiaohua2009 yangxiaohua2009 Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPIO1 GPIO2
Speaker on high low
Headphone on low high

Inverting the high/low level or changing the GPIO order are basically the same.

SOF_ES8336_JD_INVERTED),
},
{ }
Expand Down