| Message ID | 20210801210431.161775-5-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel: bytcr_rt5640: Fix HP ElitePad 1000 G2 audio routing | expand |
On 8/1/21 4:04 PM, Hans de Goede wrote: > Some devices (HP Elitepad 1000 G2) have a second headphones output > (1 on the dock, 2nd on the tablet itself) which is implemented through > the line-out output of the codec combined with an external hp-amp > which gets enabled through the codec's GPIO1 pin. > > Add support for this through a new BYT_RT5640_LINEOUT_AS_HP2 quirk. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > sound/soc/intel/boards/bytcr_rt5640.c | 36 +++++++++++++++++++++++++-- > 1 file changed, 34 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c > index 70faba13450c..51fb44ad9b4b 100644 > --- a/sound/soc/intel/boards/bytcr_rt5640.c > +++ b/sound/soc/intel/boards/bytcr_rt5640.c > @@ -74,6 +74,7 @@ enum { > #define BYT_RT5640_MCLK_25MHZ BIT(23) > #define BYT_RT5640_NO_SPEAKERS BIT(24) > #define BYT_RT5640_LINEOUT BIT(25) > +#define BYT_RT5640_LINEOUT_AS_HP2 BIT(26) The definitions aren't fully clear to me. It seems that the two quirks above are mutually exclusive if I read the code below + if (byt_rt5640_quirk & BYT_RT5640_LINEOUT_AS_HP2) + lineout_string = " cfg-hp2:lineout"; + else if (byt_rt5640_quirk & BYT_RT5640_LINEOUT) lineout_string = " cfg-lineout:1"; But in the following patch the two are mixed: + .driver_data = (void *)(BYT_RT5640_DMIC2_MAP | + BYT_RT5640_MCLK_EN | + BYT_RT5640_LINEOUT | + BYT_RT5640_LINEOUT_AS_HP2 | + BYT_RT5640_HSMIC2_ON_IN1), so maybe the test above should be if (byt_rt5640_quirk & BYT_RT5640_LINEOUT) { if (byt_rt5640_quirk & BYT_RT5640_LINEOUT_AS_HP2) lineout_string = " cfg-hp2:lineout"; else lineout_string = " cfg-lineout:1"; } I am also not very clear on the hp2 support, is there any sort of jack detection or will this require always an explicit user choice?
Hi Pierre-Louis, On 8/2/21 3:45 PM, Pierre-Louis Bossart wrote: > > > On 8/1/21 4:04 PM, Hans de Goede wrote: >> Some devices (HP Elitepad 1000 G2) have a second headphones output >> (1 on the dock, 2nd on the tablet itself) which is implemented through >> the line-out output of the codec combined with an external hp-amp >> which gets enabled through the codec's GPIO1 pin. >> >> Add support for this through a new BYT_RT5640_LINEOUT_AS_HP2 quirk. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> sound/soc/intel/boards/bytcr_rt5640.c | 36 +++++++++++++++++++++++++-- >> 1 file changed, 34 insertions(+), 2 deletions(-) >> >> diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c >> index 70faba13450c..51fb44ad9b4b 100644 >> --- a/sound/soc/intel/boards/bytcr_rt5640.c >> +++ b/sound/soc/intel/boards/bytcr_rt5640.c >> @@ -74,6 +74,7 @@ enum { >> #define BYT_RT5640_MCLK_25MHZ BIT(23) >> #define BYT_RT5640_NO_SPEAKERS BIT(24) >> #define BYT_RT5640_LINEOUT BIT(25) >> +#define BYT_RT5640_LINEOUT_AS_HP2 BIT(26) > > The definitions aren't fully clear to me. It seems that the two quirks > above are mutually exclusive if I read the code below > > + if (byt_rt5640_quirk & BYT_RT5640_LINEOUT_AS_HP2) > + lineout_string = " cfg-hp2:lineout"; > + else if (byt_rt5640_quirk & BYT_RT5640_LINEOUT) > lineout_string = " cfg-lineout:1"; > > But in the following patch the two are mixed: > > + .driver_data = (void *)(BYT_RT5640_DMIC2_MAP | > + BYT_RT5640_MCLK_EN | > + BYT_RT5640_LINEOUT | > + BYT_RT5640_LINEOUT_AS_HP2 | > + BYT_RT5640_HSMIC2_ON_IN1), > > so maybe the test above should be > > if (byt_rt5640_quirk & BYT_RT5640_LINEOUT) { > if (byt_rt5640_quirk & BYT_RT5640_LINEOUT_AS_HP2) > lineout_string = " cfg-hp2:lineout"; > else > lineout_string = " cfg-lineout:1"; > } Right, I decided to first add plain line-out support (which may be useful elsewhere) and then to build the hp2-out using external-amp connected to lineout on top of that. So your suggestion to change the test to set the lineout_string makes sense. I'll fix that for v2 of the patch-series. > I am also not very clear on the hp2 support, is there any sort of jack > detection or will this require always an explicit user choice? Quoting from: """ I've also figured out how jack-detect works, since the codec's GPIO1 is used for the external-hp-amp enable, the jack-detect signals are directly connected to the Bay Trail SoC's GPIOs: -gpioget 'INT33FC:02' 14 && gpioget 'INT33FC:00' 0 && gpioget 'INT33FC:00' 3 Nothing inserted: 1 1 0 Headset in dock: 0 1 0 Headphon in dock: 0 1 1 Headset in tabl: 1 0 0 Headphon in tabl: 1 0 0 Conclusion: GPO2 pin 14: !jack in dock GPO0 pin 0: !jack in tablet GPO0 pin 3: 1 when jack in dock with no mic And I believe that the mic on the tablet jack can be detected using the normal micBIAS over current detection which is normally also used. This will require some special jack-detect handling inside the machine driver for this model. I plan to also add support for this when I have some time to work on this. """ So ATM this requires an explicit user-choice, but I plan to add support for jack-detect on both jacks when I've some more time to work on this. Regards, Hans
> I've also figured out how jack-detect works, since the codec's GPIO1 is used for the external-hp-amp enable, the jack-detect signals are directly connected to the Bay Trail SoC's GPIOs: > > -gpioget 'INT33FC:02' 14 && gpioget 'INT33FC:00' 0 && gpioget 'INT33FC:00' 3 > Nothing inserted: 1 1 0 > Headset in dock: 0 1 0 > Headphon in dock: 0 1 1 > Headset in tabl: 1 0 0 > Headphon in tabl: 1 0 0 > Conclusion: > GPO2 pin 14: !jack in dock > GPO0 pin 0: !jack in tablet > GPO0 pin 3: 1 when jack in dock with no mic I am a bit confused about the logic. Could you have a case with 1 1 1 (separate headphones in tablet and dock jacks)? > > And I believe that the mic on the tablet jack can be detected using the normal micBIAS over current detection which is normally also used. > > This will require some special jack-detect handling inside the machine driver for this model. I plan to also add support for this when I have some time to work on this. > """ > > So ATM this requires an explicit user-choice, but I plan to add support for > jack-detect on both jacks when I've some more time to work on this. Thanks!
Hi, On 8/2/21 4:35 PM, Pierre-Louis Bossart wrote: > > >> I've also figured out how jack-detect works, since the codec's GPIO1 is used for the external-hp-amp enable, the jack-detect signals are directly connected to the Bay Trail SoC's GPIOs: >> >> -gpioget 'INT33FC:02' 14 && gpioget 'INT33FC:00' 0 && gpioget 'INT33FC:00' 3 >> Nothing inserted: 1 1 0 >> Headset in dock: 0 1 0 >> Headphon in dock: 0 1 1 >> Headset in tabl: 1 0 0 >> Headphon in tabl: 1 0 0 >> Conclusion: >> GPO2 pin 14: !jack in dock >> GPO0 pin 0: !jack in tablet >> GPO0 pin 3: 1 when jack in dock with no mic > > I am a bit confused about the logic. Could you have a case with 1 1 1 > (separate headphones in tablet and dock jacks)? 1 1 1 is not possible since 1 in column 0 + 1 means nothing inserted, iow the jack-detect is active-low; and when nothing is inserted then the last column is always 0. With that said yes headphones in the dock + something in the tablet-jack should be detectable as a new combination not in the table, this should give us "0 0 x" (I did not test that yet, but the 2 jacks should be fully independent). IOW we will end up with 2 separate snd_soc_jack-s which together register 4 pins. E.g. the declaration for the pins will look like this: static struct snd_soc_jack_pin rt5640_pins[] = { { .pin = "Headphone", .mask = SND_JACK_HEADPHONE, }, { .pin = "Headset Mic", .mask = SND_JACK_MICROPHONE, }, }; static struct snd_soc_jack_pin rt5640_pins2[] = { { .pin = "Headphone 2", .mask = SND_JACK_HEADPHONE, }, { .pin = "Headset Mic 2", .mask = SND_JACK_MICROPHONE, }, }; And there will be 2 snd_soc_jack_add_gpio calls each using 1 of the set of pins (assuming I can use bias over-current detect to differentiate between headphones/headset on the jack on the tablet). As you said in your other email "this is an interesting hardware setup" I hope I won't hit any userspace issues when I have the kernel code ready to register 2 jacks. Regards, Hans
diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index 70faba13450c..51fb44ad9b4b 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -74,6 +74,7 @@ enum { #define BYT_RT5640_MCLK_25MHZ BIT(23) #define BYT_RT5640_NO_SPEAKERS BIT(24) #define BYT_RT5640_LINEOUT BIT(25) +#define BYT_RT5640_LINEOUT_AS_HP2 BIT(26) #define BYTCR_INPUT_DEFAULTS \ (BYT_RT5640_IN3_MAP | \ @@ -142,6 +143,8 @@ static void log_quirks(struct device *dev) dev_info(dev, "quirk NO_SPEAKERS enabled\n"); if (byt_rt5640_quirk & BYT_RT5640_LINEOUT) dev_info(dev, "quirk LINEOUT enabled\n"); + if (byt_rt5640_quirk & BYT_RT5640_LINEOUT_AS_HP2) + dev_info(dev, "quirk LINEOUT_AS_HP2 enabled\n"); if (byt_rt5640_quirk & BYT_RT5640_DIFF_MIC) dev_info(dev, "quirk DIFF_MIC enabled\n"); if (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF1) { @@ -287,12 +290,39 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w, return 0; } +static int byt_rt5640_event_lineout(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) +{ + unsigned int gpio_ctrl3_val = RT5640_GP1_PF_OUT; + struct snd_soc_dai *codec_dai; + + if (!(byt_rt5640_quirk & BYT_RT5640_LINEOUT_AS_HP2)) + return 0; + + /* + * On devices which use line-out as a second headphones output, + * the codec's GPIO1 pin is used to enable an external HP-amp. + */ + + codec_dai = byt_rt5640_get_codec_dai(w->dapm); + if (!codec_dai) + return -EIO; + + if (SND_SOC_DAPM_EVENT_ON(event)) + gpio_ctrl3_val |= RT5640_GP1_OUT_HI; + + snd_soc_component_update_bits(codec_dai->component, RT5640_GPIO_CTRL3, + RT5640_GP1_PF_MASK | RT5640_GP1_OUT_MASK, gpio_ctrl3_val); + + return 0; +} + static const struct snd_soc_dapm_widget byt_rt5640_widgets[] = { SND_SOC_DAPM_HP("Headphone", NULL), SND_SOC_DAPM_MIC("Headset Mic", NULL), SND_SOC_DAPM_MIC("Internal Mic", NULL), SND_SOC_DAPM_SPK("Speaker", NULL), - SND_SOC_DAPM_LINE("Line Out", NULL), + SND_SOC_DAPM_LINE("Line Out", byt_rt5640_event_lineout), SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0, platform_clock_control, SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD), @@ -1480,7 +1510,9 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) spk_type = "stereo"; } - if (byt_rt5640_quirk & BYT_RT5640_LINEOUT) + if (byt_rt5640_quirk & BYT_RT5640_LINEOUT_AS_HP2) + lineout_string = " cfg-hp2:lineout"; + else if (byt_rt5640_quirk & BYT_RT5640_LINEOUT) lineout_string = " cfg-lineout:1"; snprintf(byt_rt5640_components, sizeof(byt_rt5640_components),
Some devices (HP Elitepad 1000 G2) have a second headphones output (1 on the dock, 2nd on the tablet itself) which is implemented through the line-out output of the codec combined with an external hp-amp which gets enabled through the codec's GPIO1 pin. Add support for this through a new BYT_RT5640_LINEOUT_AS_HP2 quirk. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- sound/soc/intel/boards/bytcr_rt5640.c | 36 +++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-)