[alsa-devel] [PATCH 01/11] ASoC: rt5651: Avoid duplicating DMI quirks between codec and machine driver
Instead of duplicating the DMI quirks between the codec and machine driver, add a rt5651_set_pdata() codec private function which the machine driver can use to pass in pdata.
This means we need to delay setting up the jack-detect registers, so this commits moves this to rt5651_set_jack() code. Note this is actually a good thing as before we would register the irq handler before rt5651->hp_jack was assigned, leading to a potential NULL deref if the jack_detect work runs before the machine driver has called set_jack.
While at it also move over to the standard snd_soc_codec_set_jack() function instead of using a codec private function for this.
Note this commit causes the machine-driver to now actually honor the BYT_RT5651_DMIC_EN quirk, which was ignored before. To avoid this causing a functional change in what is purely a refactor commit, this commit removes the quirk from the defaults.
If we really want the BYT_RT5651_DMIC_EN behavior anywhere it should be specifically enabled by follow up commits.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/codecs/rt5651.c | 232 +++++++++++++++------------------- sound/soc/codecs/rt5651.h | 6 +- sound/soc/intel/boards/bytcr_rt5651.c | 43 +++++-- 3 files changed, 139 insertions(+), 142 deletions(-)
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index 45a73049cf64..c5d19a80506b 100644 --- a/sound/soc/codecs/rt5651.c +++ b/sound/soc/codecs/rt5651.c @@ -19,7 +19,6 @@ #include <linux/platform_device.h> #include <linux/spi/spi.h> #include <linux/acpi.h> -#include <linux/dmi.h> #include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> @@ -32,10 +31,6 @@ #include "rl6231.h" #include "rt5651.h"
-#define RT5651_JD_MAP(quirk) ((quirk) & GENMASK(7, 0)) -#define RT5651_IN2_DIFF BIT(16) -#define RT5651_DMIC_EN BIT(17) - #define RT5651_DEVICE_ID_VALUE 0x6281
#define RT5651_PR_RANGE_BASE (0xff + 1) @@ -43,8 +38,6 @@
#define RT5651_PR_BASE (RT5651_PR_RANGE_BASE + (0 * RT5651_PR_SPACING))
-static unsigned long rt5651_quirk; - static const struct regmap_range_cfg rt5651_ranges[] = { { .name = "PR", .range_min = RT5651_PR_BASE, .range_max = RT5651_PR_BASE + 0xb4, @@ -1585,10 +1578,88 @@ static int rt5651_set_bias_level(struct snd_soc_codec *codec, return 0; }
+static irqreturn_t rt5651_irq(int irq, void *data) +{ + struct rt5651_priv *rt5651 = data; + + queue_delayed_work(system_power_efficient_wq, + &rt5651->jack_detect_work, msecs_to_jiffies(250)); + + return IRQ_HANDLED; +} + +static int rt5651_set_jack(struct snd_soc_codec *codec, + struct snd_soc_jack *hp_jack, void *data) +{ + struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); + struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); + int ret; + + if (!rt5651->pdata.jd_src || !rt5651->irq) + return -EINVAL; + + /* IRQ output on GPIO1 */ + regmap_update_bits(rt5651->regmap, RT5651_GPIO_CTRL1, + RT5651_GP1_PIN_MASK, RT5651_GP1_PIN_IRQ); + + switch (rt5651->pdata.jd_src) { + case RT5651_JD1_1: + regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, + RT5651_JD_TRG_SEL_MASK, + RT5651_JD_TRG_SEL_JD1_1); + regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, + RT5651_JD1_1_IRQ_EN, + RT5651_JD1_1_IRQ_EN); + break; + case RT5651_JD1_2: + regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, + RT5651_JD_TRG_SEL_MASK, + RT5651_JD_TRG_SEL_JD1_2); + regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, + RT5651_JD1_2_IRQ_EN, + RT5651_JD1_2_IRQ_EN); + break; + case RT5651_JD2: + regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, + RT5651_JD_TRG_SEL_MASK, + RT5651_JD_TRG_SEL_JD2); + regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, + RT5651_JD2_IRQ_EN, + RT5651_JD2_IRQ_EN); + break; + case RT5651_JD_NULL: + break; + default: + dev_warn(codec->dev, "Currently only JD1_1 / JD1_2 / JD2 are supported\n"); + break; + } + + snd_soc_dapm_force_enable_pin(dapm, "JD Power"); + snd_soc_dapm_force_enable_pin(dapm, "LDO"); + snd_soc_dapm_sync(dapm); + + regmap_update_bits(rt5651->regmap, RT5651_MICBIAS, + 0x38, 0x38); + + ret = devm_request_threaded_irq(codec->dev, rt5651->irq, NULL, + rt5651_irq, + IRQF_TRIGGER_RISING | + IRQF_TRIGGER_FALLING | + IRQF_ONESHOT, "rt5651", rt5651); + if (ret) { + dev_err(codec->dev, "Failed to reguest IRQ: %d\n", ret); + return ret; + } + + rt5651->hp_jack = hp_jack; + rt5651_irq(0, rt5651); + + return 0; +} + static int rt5651_probe(struct snd_soc_codec *codec) { struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); - struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec);
rt5651->codec = codec;
@@ -1604,15 +1675,6 @@ static int rt5651_probe(struct snd_soc_codec *codec)
snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF);
- if (rt5651->pdata.jd_src) { - snd_soc_dapm_force_enable_pin(dapm, "JD Power"); - snd_soc_dapm_force_enable_pin(dapm, "LDO"); - snd_soc_dapm_sync(dapm); - - regmap_update_bits(rt5651->regmap, RT5651_MICBIAS, - 0x38, 0x38); - } - return 0; }
@@ -1698,6 +1760,7 @@ static const struct snd_soc_codec_driver soc_codec_dev_rt5651 = { .resume = rt5651_resume, .set_bias_level = rt5651_set_bias_level, .idle_bias_off = true, + .set_jack = rt5651_set_jack, .component_driver = { .controls = rt5651_snd_controls, .num_controls = ARRAY_SIZE(rt5651_snd_controls), @@ -1747,54 +1810,16 @@ static const struct i2c_device_id rt5651_i2c_id[] = { }; MODULE_DEVICE_TABLE(i2c, rt5651_i2c_id);
-static int rt5651_quirk_cb(const struct dmi_system_id *id) -{ - rt5651_quirk = (unsigned long) id->driver_data; - return 1; -} - -static const struct dmi_system_id rt5651_quirk_table[] = { - { - .callback = rt5651_quirk_cb, - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "KIANO"), - DMI_MATCH(DMI_PRODUCT_NAME, "KIANO SlimNote 14.2"), - }, - .driver_data = (unsigned long *) RT5651_JD1_1, - }, - {} -}; - static int rt5651_parse_dt(struct rt5651_priv *rt5651, struct device_node *np) { - if (of_property_read_bool(np, "realtek,in2-differential")) - rt5651_quirk |= RT5651_IN2_DIFF; - if (of_property_read_bool(np, "realtek,dmic-en")) - rt5651_quirk |= RT5651_DMIC_EN; + rt5651->pdata.in2_diff = + of_property_read_bool(np, "realtek,in2-differential"); + rt5651->pdata.dmic_en = + of_property_read_bool(np, "realtek,dmic-en");
return 0; }
-static void rt5651_set_pdata(struct rt5651_priv *rt5651) -{ - if (rt5651_quirk & RT5651_IN2_DIFF) - rt5651->pdata.in2_diff = true; - if (rt5651_quirk & RT5651_DMIC_EN) - rt5651->pdata.dmic_en = true; - if (RT5651_JD_MAP(rt5651_quirk)) - rt5651->pdata.jd_src = RT5651_JD_MAP(rt5651_quirk); -} - -static irqreturn_t rt5651_irq(int irq, void *data) -{ - struct rt5651_priv *rt5651 = data; - - queue_delayed_work(system_power_efficient_wq, - &rt5651->jack_detect_work, msecs_to_jiffies(250)); - - return IRQ_HANDLED; -} - static int rt5651_jack_detect(struct snd_soc_codec *codec, int jack_insert) { struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); @@ -1860,17 +1885,26 @@ static void rt5651_jack_detect_work(struct work_struct *work) snd_soc_jack_report(rt5651->hp_jack, report, SND_JACK_HEADSET); }
-int rt5651_set_jack_detect(struct snd_soc_codec *codec, - struct snd_soc_jack *hp_jack) +static void rt5651_apply_pdata(struct rt5651_priv *rt5651) { - struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); + if (rt5651->pdata.in2_diff) + regmap_update_bits(rt5651->regmap, RT5651_IN1_IN2, + RT5651_IN_DF2, RT5651_IN_DF2);
- rt5651->hp_jack = hp_jack; - rt5651_irq(0, rt5651); + if (rt5651->pdata.dmic_en) + regmap_update_bits(rt5651->regmap, RT5651_GPIO_CTRL1, + RT5651_GP2_PIN_MASK, RT5651_GP2_PIN_DMIC1_SCL); +}
- return 0; +void rt5651_set_pdata(struct snd_soc_codec *codec, + struct rt5651_platform_data *pdata) +{ + struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); + + rt5651->pdata = *pdata; + rt5651_apply_pdata(rt5651); } -EXPORT_SYMBOL_GPL(rt5651_set_jack_detect); +EXPORT_SYMBOL_GPL(rt5651_set_pdata);
static int rt5651_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) @@ -1890,10 +1924,6 @@ static int rt5651_i2c_probe(struct i2c_client *i2c, rt5651->pdata = *pdata; else if (i2c->dev.of_node) rt5651_parse_dt(rt5651, i2c->dev.of_node); - else - dmi_check_system(rt5651_quirk_table); - - rt5651_set_pdata(rt5651);
rt5651->regmap = devm_regmap_init_i2c(i2c, &rt5651_regmap); if (IS_ERR(rt5651->regmap)) { @@ -1917,69 +1947,13 @@ static int rt5651_i2c_probe(struct i2c_client *i2c, if (ret != 0) dev_warn(&i2c->dev, "Failed to apply regmap patch: %d\n", ret);
- if (rt5651->pdata.in2_diff) - regmap_update_bits(rt5651->regmap, RT5651_IN1_IN2, - RT5651_IN_DF2, RT5651_IN_DF2); - - if (rt5651->pdata.dmic_en) - regmap_update_bits(rt5651->regmap, RT5651_GPIO_CTRL1, - RT5651_GP2_PIN_MASK, RT5651_GP2_PIN_DMIC1_SCL); + if (pdata || i2c->dev.of_node) + rt5651_apply_pdata(rt5651);
+ rt5651->irq = i2c->irq; rt5651->hp_mute = 1; - - if (rt5651->pdata.jd_src) { - - /* IRQ output on GPIO1 */ - regmap_update_bits(rt5651->regmap, RT5651_GPIO_CTRL1, - RT5651_GP1_PIN_MASK, RT5651_GP1_PIN_IRQ); - - switch (rt5651->pdata.jd_src) { - case RT5651_JD1_1: - regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, - RT5651_JD_TRG_SEL_MASK, - RT5651_JD_TRG_SEL_JD1_1); - regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, - RT5651_JD1_1_IRQ_EN, - RT5651_JD1_1_IRQ_EN); - break; - case RT5651_JD1_2: - regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, - RT5651_JD_TRG_SEL_MASK, - RT5651_JD_TRG_SEL_JD1_2); - regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, - RT5651_JD1_2_IRQ_EN, - RT5651_JD1_2_IRQ_EN); - break; - case RT5651_JD2: - regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, - RT5651_JD_TRG_SEL_MASK, - RT5651_JD_TRG_SEL_JD2); - regmap_update_bits(rt5651->regmap, RT5651_IRQ_CTRL1, - RT5651_JD2_IRQ_EN, - RT5651_JD2_IRQ_EN); - break; - case RT5651_JD_NULL: - break; - default: - dev_warn(&i2c->dev, "Currently only JD1_1 / JD1_2 / JD2 are supported\n"); - break; - } - } - INIT_DELAYED_WORK(&rt5651->jack_detect_work, rt5651_jack_detect_work);
- if (i2c->irq) { - ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL, - rt5651_irq, - IRQF_TRIGGER_RISING | - IRQF_TRIGGER_FALLING | - IRQF_ONESHOT, "rt5651", rt5651); - if (ret) { - dev_err(&i2c->dev, "Failed to reguest IRQ: %d\n", ret); - return ret; - } - } - ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt5651, rt5651_dai, ARRAY_SIZE(rt5651_dai));
diff --git a/sound/soc/codecs/rt5651.h b/sound/soc/codecs/rt5651.h index 4f8b202121d7..151ac92f6bad 100644 --- a/sound/soc/codecs/rt5651.h +++ b/sound/soc/codecs/rt5651.h @@ -2065,6 +2065,7 @@ struct rt5651_priv { struct snd_soc_jack *hp_jack; struct delayed_work jack_detect_work;
+ int irq; int sysclk; int sysclk_src; int lrck[RT5651_AIFS]; @@ -2079,6 +2080,7 @@ struct rt5651_priv { bool hp_mute; };
-int rt5651_set_jack_detect(struct snd_soc_codec *codec, - struct snd_soc_jack *hp_jack); +void rt5651_set_pdata(struct snd_soc_codec *codec, + struct rt5651_platform_data *pdata); + #endif /* __RT5651_H__ */ diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 49c538f2770a..8ef5b5500fb7 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -42,7 +42,15 @@ enum { BYT_RT5651_IN3_MAP, };
-#define BYT_RT5651_MAP(quirk) ((quirk) & GENMASK(7, 0)) +enum { + BYT_RT5651_JD_NULL = (RT5651_JD_NULL << 4), + BYT_RT5651_JD1_1 = (RT5651_JD1_1 << 4), + BYT_RT5651_JD1_2 = (RT5651_JD1_2 << 4), + BYT_RT5651_JD2 = (RT5651_JD2 << 4), +}; + +#define BYT_RT5651_MAP(quirk) ((quirk) & GENMASK(3, 0)) +#define BYT_RT5651_JDSRC(quirk) (((quirk) & GENMASK(7, 4)) >> 4) #define BYT_RT5651_DMIC_EN BIT(16) #define BYT_RT5651_MCLK_EN BIT(17) #define BYT_RT5651_MCLK_25MHZ BIT(18) @@ -53,7 +61,6 @@ struct byt_rt5651_private { };
static unsigned long byt_rt5651_quirk = BYT_RT5651_DMIC_MAP | - BYT_RT5651_DMIC_EN | BYT_RT5651_MCLK_EN;
static void log_quirks(struct device *dev) @@ -66,6 +73,9 @@ static void log_quirks(struct device *dev) dev_info(dev, "quirk IN2_MAP enabled"); if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN3_MAP) dev_info(dev, "quirk IN3_MAP enabled"); + if (BYT_RT5651_JDSRC(byt_rt5651_quirk)) + dev_info(dev, "quirk jack-detect src %ld\n", + BYT_RT5651_JDSRC(byt_rt5651_quirk)); if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN) dev_info(dev, "quirk DMIC enabled"); if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) @@ -288,6 +298,7 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = { DMI_MATCH(DMI_PRODUCT_NAME, "KIANO SlimNote 14.2"), }, .driver_data = (void *)(BYT_RT5651_MCLK_EN | + BYT_RT5651_JD1_1 | BYT_RT5651_IN1_IN2_MAP), }, {} @@ -299,6 +310,7 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) struct snd_soc_codec *codec = runtime->codec; struct byt_rt5651_private *priv = snd_soc_card_get_drvdata(card); const struct snd_soc_dapm_route *custom_map; + struct rt5651_platform_data pdata = {}; int num_routes; int ret;
@@ -360,17 +372,26 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) dev_err(card->dev, "unable to set MCLK rate\n"); }
- ret = snd_soc_card_jack_new(runtime->card, "Headset", - SND_JACK_HEADSET, &priv->jack, - bytcr_jack_pins, ARRAY_SIZE(bytcr_jack_pins)); - if (ret) { - dev_err(runtime->dev, "Headset jack creation failed %d\n", ret); - return ret; - } + pdata.jd_src = BYT_RT5651_JDSRC(byt_rt5651_quirk); + if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN) + pdata.dmic_en = true; + rt5651_set_pdata(codec, &pdata); + + if (BYT_RT5651_JDSRC(byt_rt5651_quirk)) { + ret = snd_soc_card_jack_new(runtime->card, "Headset", + SND_JACK_HEADSET, &priv->jack, + bytcr_jack_pins, ARRAY_SIZE(bytcr_jack_pins)); + if (ret) { + dev_err(runtime->dev, "Jack creation failed %d\n", ret); + return ret; + }
- rt5651_set_jack_detect(codec, &priv->jack); + ret = snd_soc_codec_set_jack(codec, &priv->jack, NULL); + if (ret) + return ret; + }
- return ret; + return 0; }
static const struct snd_soc_pcm_stream byt_rt5651_dai_params = {
dapm_power_widgets() first builds a list of which widgets to power-up before actually powering any of them up. For dapm-supply widgets their connected method, in our case is_sys_clk_from_pll() get called at this point.
Before this commit is_sys_clk_from_pll() was looking at the actually selected clock in the RT5651_GBL_CLK register. But the sysclk itself is selected by another dapm-supply (the "Platform Clock" supply in the machine driver) and any changes to that supply part of the same power- transition get executed after building the list, causing is_sys_clk_from_pll() to return the wrong value.
This sometimes leads to the PWR_PLL bit not getting set even though we have configured an active audio chain and RT5651_GBL_CLK does point to PLL1 as the sysclk-source.
Note that even if we do not have an active audio chain we should still keep the PWR_PLL bit set if PLL1 is our sysclk-source, because we must always have a working sysclk-source, otherwise e.g. jack-detection will not work.
If we don't have an active audio-chain then the machine-driver should switch the sysclk-source to the RCCLK and if does not then that is a machine-driver (or UCM config) problem and not something we should try to work around by disabling our sysclk-source and breaking jack-detect.
TL;DR: The PLL1-supply must always be powered up when PLL1 is the sysclk source and we can simply set the RT5651_PWR_PLL bit when selecting PLL1.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- Note that many of the other rt56?? drivers have the same is_sys_clk_from_pll() stuff and may need similar fixes. --- sound/soc/codecs/rt5651.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-)
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index c5d19a80506b..a48278f6205d 100644 --- a/sound/soc/codecs/rt5651.c +++ b/sound/soc/codecs/rt5651.c @@ -393,20 +393,6 @@ static int set_dmic_clk(struct snd_soc_dapm_widget *w, return idx; }
-static int is_sysclk_from_pll(struct snd_soc_dapm_widget *source, - struct snd_soc_dapm_widget *sink) -{ - struct snd_soc_codec *codec = snd_soc_dapm_to_codec(source->dapm); - unsigned int val; - - val = snd_soc_read(codec, RT5651_GLB_CLK); - val &= RT5651_SCLK_SRC_MASK; - if (val == RT5651_SCLK_SRC_PLL1) - return 1; - else - return 0; -} - /* Digital Mixer */ static const struct snd_kcontrol_new rt5651_sto1_adc_l_mix[] = { SOC_DAPM_SINGLE("ADC1 Switch", RT5651_STO1_ADC_MIXER, @@ -878,8 +864,6 @@ static const struct snd_soc_dapm_widget rt5651_dapm_widgets[] = { SND_SOC_DAPM_SUPPLY_S("ADC ASRC", 1, RT5651_PLL_MODE_2, 11, 0, NULL, 0),
- SND_SOC_DAPM_SUPPLY("PLL1", RT5651_PWR_ANLG2, - RT5651_PWR_PLL_BIT, 0, NULL, 0), /* Input Side */ SND_SOC_DAPM_SUPPLY("JD Power", RT5651_PWR_ANLG2, RT5651_PWM_JD_M_BIT, 0, NULL, 0), @@ -1162,7 +1146,6 @@ static const struct snd_soc_dapm_route rt5651_dapm_routes[] = { {"Stereo1 ADC MIXL", "ADC1 Switch", "Stereo1 ADC L1 Mux"}, {"Stereo1 ADC MIXL", "ADC2 Switch", "Stereo1 ADC L2 Mux"}, {"Stereo1 ADC MIXL", NULL, "Stereo1 Filter"}, - {"Stereo1 Filter", NULL, "PLL1", is_sysclk_from_pll}, {"Stereo1 Filter", NULL, "ADC ASRC"},
{"Stereo1 ADC MIXR", "ADC1 Switch", "Stereo1 ADC R1 Mux"}, @@ -1172,7 +1155,6 @@ static const struct snd_soc_dapm_route rt5651_dapm_routes[] = { {"Stereo2 ADC MIXL", "ADC1 Switch", "Stereo2 ADC L1 Mux"}, {"Stereo2 ADC MIXL", "ADC2 Switch", "Stereo2 ADC L2 Mux"}, {"Stereo2 ADC MIXL", NULL, "Stereo2 Filter"}, - {"Stereo2 Filter", NULL, "PLL1", is_sysclk_from_pll}, {"Stereo2 Filter", NULL, "ADC ASRC"},
{"Stereo2 ADC MIXR", "ADC1 Switch", "Stereo2 ADC R1 Mux"}, @@ -1239,10 +1221,8 @@ static const struct snd_soc_dapm_route rt5651_dapm_routes[] = { {"PDM R Mux", "DD MIX", "DAC MIXR"},
{"DAC L1", NULL, "Stereo DAC MIXL"}, - {"DAC L1", NULL, "PLL1", is_sysclk_from_pll}, {"DAC L1", NULL, "DAC L1 Power"}, {"DAC R1", NULL, "Stereo DAC MIXR"}, - {"DAC R1", NULL, "PLL1", is_sysclk_from_pll}, {"DAC R1", NULL, "DAC R1 Power"},
{"DD MIXL", "DAC L1 Switch", "DAC MIXL"}, @@ -1438,6 +1418,7 @@ static int rt5651_set_dai_sysclk(struct snd_soc_dai *dai, struct snd_soc_codec *codec = dai->codec; struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); unsigned int reg_val = 0; + unsigned int pll_bit = 0;
if (freq == rt5651->sysclk && clk_id == rt5651->sysclk_src) return 0; @@ -1448,6 +1429,7 @@ static int rt5651_set_dai_sysclk(struct snd_soc_dai *dai, break; case RT5651_SCLK_S_PLL1: reg_val |= RT5651_SCLK_SRC_PLL1; + pll_bit |= RT5651_PWR_PLL; break; case RT5651_SCLK_S_RCCLK: reg_val |= RT5651_SCLK_SRC_RCCLK; @@ -1456,6 +1438,9 @@ static int rt5651_set_dai_sysclk(struct snd_soc_dai *dai, dev_err(codec->dev, "Invalid clock id (%d)\n", clk_id); return -EINVAL; } + + snd_soc_update_bits(codec, RT5651_PWR_ANLG2, + RT5651_PWR_PLL, pll_bit); snd_soc_update_bits(codec, RT5651_GLB_CLK, RT5651_SCLK_SRC_MASK, reg_val); rt5651->sysclk = freq;
On Tue, Feb 20, 2018 at 11:15:02PM +0100, Hans de Goede wrote:
TL;DR: The PLL1-supply must always be powered up when PLL1 is the sysclk source and we can simply set the RT5651_PWR_PLL bit when selecting PLL1.
Only if jack detection is enabled, if jack detection is not in use for some reason then the PLL isn't required and should be powered off - this is normally handled by having the jack detection code force enable things.
Hi,
On 21-02-18 12:18, Mark Brown wrote:
On Tue, Feb 20, 2018 at 11:15:02PM +0100, Hans de Goede wrote:
TL;DR: The PLL1-supply must always be powered up when PLL1 is the sysclk source and we can simply set the RT5651_PWR_PLL bit when selecting PLL1.
Only if jack detection is enabled, if jack detection is not in use for some reason then the PLL isn't required and should be powered off - this is normally handled by having the jack detection code force enable things.
As the commit message tries to explain, the code this removes is fundamentally broken and this is not jack-detection related:
"dapm_power_widgets() first builds a list of which widgets to power-up before actually powering any of them up. For dapm-supply widgets their connected method, in our case is_sys_clk_from_pll() get called at this point.
Before this commit is_sys_clk_from_pll() was looking at the actually selected clock in the RT5651_GBL_CLK register. But the sysclk itself is selected by another dapm-supply (the "Platform Clock" supply in the machine driver) and any changes to that supply part of the same power- transition get executed after building the list, causing is_sys_clk_from_pll() to return the wrong value."
I actually wrote this patch for the rt5640 driver first (but my rt5640 work / patch-series is not finished yet). I saw the following problem on rt5640 boards before even introducing jack-detect:
1) Have a generic UCM file 2) Start recording 3) Select an input which is not part of the input-map quirk in the machine driver 4) Switch to an input which is part of the input-map 5) Notice how only silence is recorded 6) Use i2c-get to get the PWR_ANLG2 register, observe that the PLL-bit is OFF at this point despite a stream being recorded due to the ordering issues 7) Use i2c-get to get the GLB_CLK register notice that it does point to PLL1, so is_sys_clk_from_pll() will return 1 if called *NOW*, but clearly it returned 0 when called as proven by 6) which shows the ordering issue is real 8) Use i2c-set to enable the PLL-bit in PWR_ANLG2 and the recording starts working
As I tried to explain in the commit message the is_sys_clk_from_pll() thingie simply can NOT work because it should check for what the GLB_CLK register will be *after* the dapm transaction / transition but in reality it checks for what it was *before* the transition, which means that it will only return the right thing if 2 transitions are made in a row where the second transition preserves the GLB_CLK value of the first.
As for that the PLL should be powered-off when not needed, I agree but that is controlled by the machine-driver through the "Platform Clock" dapm supply and if that leaves things on for some reason then that is the problem which we actually need to fix, e.g. this will also leave the MCLK input clk enabled needlessly.
I really don't see how configuring a broken sysclk (GBL_CLK points to PLL1, but PLL-bit in PWR_ANLG2 is OFF) is ever a good thing, instead we should make sure we always switch to the RCCLK when we don't need the SYSCLK.
Last force-enabling the PLL bit on all machines where we can do jack-detection (not only over-current, but the JD irq in general needs a valid sysclk) would in practice mean always force-enabling the PLL bit since almost all boards have jack-detection once we add the necessary quirks, so this really is not a solution.
Regards,
Hans
On Wed, Feb 21, 2018 at 08:38:01PM +0100, Hans de Goede wrote:
On 21-02-18 12:18, Mark Brown wrote:
On Tue, Feb 20, 2018 at 11:15:02PM +0100, Hans de Goede wrote:
Only if jack detection is enabled, if jack detection is not in use for some reason then the PLL isn't required and should be powered off - this is normally handled by having the jack detection code force enable things.
As the commit message tries to explain, the code this removes is fundamentally broken and this is not jack-detection related:
"dapm_power_widgets() first builds a list of which widgets to power-up before actually powering any of them up. For dapm-supply widgets their connected method, in our case is_sys_clk_from_pll() get called at this point.
Before this commit is_sys_clk_from_pll() was looking at the actually selected clock in the RT5651_GBL_CLK register. But the sysclk itself is selected by another dapm-supply (the "Platform Clock" supply in the machine driver) and any changes to that supply part of the same power- transition get executed after building the list, causing is_sys_clk_from_pll() to return the wrong value."
Right, but there's a couple of jumps in your reasoning with the actual solution.
As for that the PLL should be powered-off when not needed, I agree but that is controlled by the machine-driver through the "Platform Clock" dapm supply and if that leaves things on for some reason then that is the problem which we actually need to fix, e.g. this will also leave the MCLK input clk enabled needlessly.
I don't follow this bit, sorry. If there's a DAPM supply that's being enabled when it's not needed then we should just disable it.
I really don't see how configuring a broken sysclk (GBL_CLK points to PLL1, but PLL-bit in PWR_ANLG2 is OFF) is ever a good thing, instead we should make sure we always switch to the RCCLK when we don't need the SYSCLK.
Last force-enabling the PLL bit on all machines where we can do jack-detection (not only over-current, but the JD irq in general needs a valid sysclk) would in practice mean always force-enabling the PLL bit since almost all boards have jack-detection once we add the necessary quirks, so this really is not a solution.
A bit confused here as well, sorry - I think you're assuming I'm suggesting some particular solution but I'm not sure what that is.
Hi,
On 22-02-18 11:48, Mark Brown wrote:
On Wed, Feb 21, 2018 at 08:38:01PM +0100, Hans de Goede wrote:
On 21-02-18 12:18, Mark Brown wrote:
On Tue, Feb 20, 2018 at 11:15:02PM +0100, Hans de Goede wrote:
Only if jack detection is enabled, if jack detection is not in use for some reason then the PLL isn't required and should be powered off - this is normally handled by having the jack detection code force enable things.
As the commit message tries to explain, the code this removes is fundamentally broken and this is not jack-detection related:
"dapm_power_widgets() first builds a list of which widgets to power-up before actually powering any of them up. For dapm-supply widgets their connected method, in our case is_sys_clk_from_pll() get called at this point.Before this commit is_sys_clk_from_pll() was looking at the actually selected clock in the RT5651_GBL_CLK register. But the sysclk itself is selected by another dapm-supply (the "Platform Clock" supply in the machine driver) and any changes to that supply part of the same power- transition get executed after building the list, causing is_sys_clk_from_pll() to return the wrong value."
Right, but there's a couple of jumps in your reasoning with the actual solution.
As for that the PLL should be powered-off when not needed, I agree but that is controlled by the machine-driver through the "Platform Clock" dapm supply and if that leaves things on for some reason then that is the problem which we actually need to fix, e.g. this will also leave the MCLK input clk enabled needlessly.
I don't follow this bit, sorry. If there's a DAPM supply that's being enabled when it's not needed then we should just disable it.
Right, that is exactly what I'm saying too.
Whether we use PLL1 as sys-clk or the RCCLK is controlled by the "Platform Clock" dapm supply. If we proper disable that supply when we don't need PLL1, then it will switch to RCCLK and we can simply have rt5651_set_dai_sysclk() enable / disable the PLL pwr bit in PWR_ANLG2 when we switch clock rather then using the is_sys_clk_from_pll() check.
I hope this helps clear up the confusion surrounding this patch. And I guess I need to improve the commit message...
Regards,
Hans
On Thu, Feb 22, 2018 at 12:00:35PM +0100, Hans de Goede wrote:
I hope this helps clear up the confusion surrounding this patch. And I guess I need to improve the commit message...
Yeah, that's *probably* the main issue here.
Overcurrent detection (ovcd) requires the following to be on: 1) The LDO supply 2) The micbias1 supply 3) General analog voltages such as vref aka a bias_level of standby
Before this commit deps 2. and 3. were not met (unless a stream recording from the mic was active).
3. Is not met because rt5651_set_bias_level() was only enabling this when reaching a bias level of prepared instead of doing this in the normal standby bias level, which the dapm core will select as soon as any pins / supplies are on. This commit fixes by making rt5651_set_bias_level() behave as a normal set_bias function for other codecs and already enabling these things at standby level.
This change to rt5651_set_bias_level() causes a problem because when jack- detect is used the bias-level was always set to standby because of the "JD Power" supply being force-enabled.
As the set_bias_level code already leaves the RT5651_PWR_JD_M bit on when entering standby (now off) when jd is in use, we can simply drop the "JD Power" supply so that the bias-level properly becomes off when nothing is happening. For the same reason we should also not enable the LDO supply until we actually want to do ovcd detection.
2. is fixed by simply force-enabling "micbias1" when doing ovcd, this commit also adds code to turn both the micbias1 and the LDO supplies of again when we're done, note they will only really get turned off if the ovcd was the only user.
The snd_soc_codec_force_bias_level(BIAS_OFF) call done in rt5651_probe() will now turn off PWR_ANLG1, so the programming of PWR_ANLG1 before the snd_soc_codec_force_bias_level() now is a no-op and can be removed.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/codecs/rt5651.c | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index a48278f6205d..52fb835ea584 100644 --- a/sound/soc/codecs/rt5651.c +++ b/sound/soc/codecs/rt5651.c @@ -864,10 +864,6 @@ static const struct snd_soc_dapm_widget rt5651_dapm_widgets[] = { SND_SOC_DAPM_SUPPLY_S("ADC ASRC", 1, RT5651_PLL_MODE_2, 11, 0, NULL, 0),
- /* Input Side */ - SND_SOC_DAPM_SUPPLY("JD Power", RT5651_PWR_ANLG2, - RT5651_PWM_JD_M_BIT, 0, NULL, 0), - /* micbias */ SND_SOC_DAPM_SUPPLY("LDO", RT5651_PWR_ANLG1, RT5651_PWR_LDO_BIT, 0, NULL, 0), @@ -1520,8 +1516,8 @@ static int rt5651_set_bias_level(struct snd_soc_codec *codec, struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec);
switch (level) { - case SND_SOC_BIAS_PREPARE: - if (SND_SOC_BIAS_STANDBY == snd_soc_codec_get_bias_level(codec)) { + case SND_SOC_BIAS_STANDBY: + if (snd_soc_codec_get_bias_level(codec) == SND_SOC_BIAS_OFF) { snd_soc_update_bits(codec, RT5651_PWR_ANLG1, RT5651_PWR_VREF1 | RT5651_PWR_MB | RT5651_PWR_BG | RT5651_PWR_VREF2, @@ -1541,7 +1537,7 @@ static int rt5651_set_bias_level(struct snd_soc_codec *codec, } break;
- case SND_SOC_BIAS_STANDBY: + case SND_SOC_BIAS_OFF: snd_soc_write(codec, RT5651_D_MISC, 0x0010); snd_soc_write(codec, RT5651_PWR_DIG1, 0x0000); snd_soc_write(codec, RT5651_PWR_DIG2, 0x0000); @@ -1576,7 +1572,6 @@ static irqreturn_t rt5651_irq(int irq, void *data) static int rt5651_set_jack(struct snd_soc_codec *codec, struct snd_soc_jack *hp_jack, void *data) { - struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); int ret;
@@ -1619,9 +1614,8 @@ static int rt5651_set_jack(struct snd_soc_codec *codec, break; }
- snd_soc_dapm_force_enable_pin(dapm, "JD Power"); - snd_soc_dapm_force_enable_pin(dapm, "LDO"); - snd_soc_dapm_sync(dapm); + regmap_update_bits(rt5651->regmap, RT5651_PWR_ANLG2, RT5651_PWR_JD_M, + RT5651_PWR_JD_M);
regmap_update_bits(rt5651->regmap, RT5651_MICBIAS, 0x38, 0x38); @@ -1648,16 +1642,6 @@ static int rt5651_probe(struct snd_soc_codec *codec)
rt5651->codec = codec;
- snd_soc_update_bits(codec, RT5651_PWR_ANLG1, - RT5651_PWR_VREF1 | RT5651_PWR_MB | - RT5651_PWR_BG | RT5651_PWR_VREF2, - RT5651_PWR_VREF1 | RT5651_PWR_MB | - RT5651_PWR_BG | RT5651_PWR_VREF2); - usleep_range(10000, 15000); - snd_soc_update_bits(codec, RT5651_PWR_ANLG1, - RT5651_PWR_FV1 | RT5651_PWR_FV2, - RT5651_PWR_FV1 | RT5651_PWR_FV2); - snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF);
return 0; @@ -1811,8 +1795,11 @@ static int rt5651_jack_detect(struct snd_soc_codec *codec, int jack_insert) int jack_type;
if (jack_insert) { - snd_soc_dapm_force_enable_pin(dapm, "LDO"); - snd_soc_dapm_sync(dapm); + snd_soc_dapm_mutex_lock(dapm); + snd_soc_dapm_force_enable_pin_unlocked(dapm, "LDO"); + snd_soc_dapm_force_enable_pin_unlocked(dapm, "micbias1"); + snd_soc_dapm_sync_unlocked(dapm); + snd_soc_dapm_mutex_unlock(dapm);
snd_soc_update_bits(codec, RT5651_MICBIAS, RT5651_MIC1_OVCD_MASK | @@ -1830,6 +1817,12 @@ static int rt5651_jack_detect(struct snd_soc_codec *codec, int jack_insert) jack_type = SND_JACK_HEADSET; snd_soc_update_bits(codec, RT5651_IRQ_CTRL2, RT5651_MB1_OC_CLR, 0); + + snd_soc_dapm_mutex_lock(dapm); + snd_soc_dapm_disable_pin_unlocked(dapm, "micbias1"); + snd_soc_dapm_disable_pin_unlocked(dapm, "LDO"); + snd_soc_dapm_sync_unlocked(dapm); + snd_soc_dapm_mutex_unlock(dapm); } else { /* jack out */ jack_type = 0;
On Tue, Feb 20, 2018 at 11:15:03PM +0100, Hans de Goede wrote:
Overcurrent detection (ovcd) requires the following to be on:
- The LDO supply
- The micbias1 supply
- General analog voltages such as vref aka a bias_level of standby
Before this commit deps 2. and 3. were not met (unless a stream recording from the mic was active).
What does the overcurrent detection detect overcurrent on?
- Is not met because rt5651_set_bias_level() was only enabling this when
reaching a bias level of prepared instead of doing this in the normal standby bias level, which the dapm core will select as soon as any pins / supplies are on. This commit fixes by making rt5651_set_bias_level() behave as a normal set_bias function for other codecs and already enabling these things at standby level.
...
- is fixed by simply force-enabling "micbias1" when doing ovcd, this
commit also adds code to turn both the micbias1 and the LDO supplies of again when we're done, note they will only really get turned off if the ovcd was the only user.
The snd_soc_codec_force_bias_level(BIAS_OFF) call done in rt5651_probe() will now turn off PWR_ANLG1, so the programming of PWR_ANLG1 before the snd_soc_codec_force_bias_level() now is a no-op and can be removed.
This is two or three changes combined into a single commit which makes it harder to review :(
@@ -1811,8 +1795,11 @@ static int rt5651_jack_detect(struct snd_soc_codec *codec, int jack_insert) int jack_type;
if (jack_insert) {
snd_soc_dapm_force_enable_pin(dapm, "LDO");snd_soc_dapm_sync(dapm);
snd_soc_dapm_mutex_lock(dapm);snd_soc_dapm_force_enable_pin_unlocked(dapm, "LDO");snd_soc_dapm_force_enable_pin_unlocked(dapm, "micbias1");snd_soc_dapm_sync_unlocked(dapm);snd_soc_dapm_mutex_unlock(dapm);snd_soc_update_bits(codec, RT5651_MICBIAS, RT5651_MIC1_OVCD_MASK |
This is now only enabling the LDO and bias when the jack is inserted - is that enough? The changelog isn't very clear about what is going on here.
Hi,
On 21-02-18 12:40, Mark Brown wrote:
On Tue, Feb 20, 2018 at 11:15:03PM +0100, Hans de Goede wrote:
Overcurrent detection (ovcd) requires the following to be on:
- The LDO supply
- The micbias1 supply
- General analog voltages such as vref aka a bias_level of standby
Before this commit deps 2. and 3. were not met (unless a stream recording from the mic was active).
What does the overcurrent detection detect overcurrent on?
On the micbias current, which is why we need the micbias1 supply on to do OVCD, which we use to determine if we've a headset (speakers + microphone) or headphones (speakers only, mic contact shorted to ground) plugged in.
- Is not met because rt5651_set_bias_level() was only enabling this when
reaching a bias level of prepared instead of doing this in the normal standby bias level, which the dapm core will select as soon as any pins / supplies are on. This commit fixes by making rt5651_set_bias_level() behave as a normal set_bias function for other codecs and already enabling these things at standby level.
...
- is fixed by simply force-enabling "micbias1" when doing ovcd, this
commit also adds code to turn both the micbias1 and the LDO supplies of again when we're done, note they will only really get turned off if the ovcd was the only user.
The snd_soc_codec_force_bias_level(BIAS_OFF) call done in rt5651_probe() will now turn off PWR_ANLG1, so the programming of PWR_ANLG1 before the snd_soc_codec_force_bias_level() now is a no-op and can be removed.
This is two or three changes combined into a single commit which makes it harder to review :(
I can split this up, but I cannot guarantee then that there will not be a state between 1 of the commits where OVCD is not even more broken then before.
@@ -1811,8 +1795,11 @@ static int rt5651_jack_detect(struct snd_soc_codec *codec, int jack_insert) int jack_type;
if (jack_insert) {
snd_soc_dapm_force_enable_pin(dapm, "LDO");snd_soc_dapm_sync(dapm);
snd_soc_dapm_mutex_lock(dapm);snd_soc_dapm_force_enable_pin_unlocked(dapm, "LDO");snd_soc_dapm_force_enable_pin_unlocked(dapm, "micbias1");snd_soc_dapm_sync_unlocked(dapm);snd_soc_dapm_mutex_unlock(dapm);snd_soc_update_bits(codec, RT5651_MICBIAS, RT5651_MIC1_OVCD_MASK |
This is now only enabling the LDO and bias when the jack is inserted - is that enough?
Yes, we only need to do OVCD on an insertion event to differ between headset vs headphones.
Regards,
Hans
On Wed, Feb 21, 2018 at 08:43:47PM +0100, Hans de Goede wrote:
On 21-02-18 12:40, Mark Brown wrote:
This is now only enabling the LDO and bias when the jack is inserted - is that enough?
Yes, we only need to do OVCD on an insertion event to differ between headset vs headphones.
Is there not a fault protection aspect to this? The naming and the separate control makes this seem like it's got some wider purpose.
Hi,
On 22-02-18 11:52, Mark Brown wrote:
On Wed, Feb 21, 2018 at 08:43:47PM +0100, Hans de Goede wrote:
On 21-02-18 12:40, Mark Brown wrote:
This is now only enabling the LDO and bias when the jack is inserted - is that enough?
Yes, we only need to do OVCD on an insertion event to differ between headset vs headphones.
Is there not a fault protection aspect to this? The naming and the separate control makes this seem like it's got some wider purpose.
That is a good question, my patch series ends up always enabling this because my testing has shown that it works better (*) when enabled before the supplies feeding it are enabled, and there is no harm in leaving it enabled while the supplies are disabled.
This will automatically also give us the fault protection when micbias1 is on because we're actually recording from the mic, rather then that it is forced on for a short while for the headset vs headphones detection.
So after this series we are good wrt its function for fault protection too. Note that the datasheet is not clear on if it is really necessary for this, but I think it makes sense to have this enabled at all times.
Regards,
Hans
There is no need to set the LDO voltage to 1.2 volt each time we enter standby, instead always leave it 1.2 volt on BIAS_OFF. Note we do a snd_soc_codec_force_bias_level(BIAS_OFF) on probe, so this will configure it correctly right from the start.
For PWR_ANLG2 leave the RT5651_PWR_JD_M and PLL bits as is instead of having different code-paths for when we've jack-detect vs when we don't.
Note that this also stops enabling the PLL bit when we've a jd_src and we are running of the RCCLK, this is intentional.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/codecs/rt5651.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index 52fb835ea584..4b0509f7e001 100644 --- a/sound/soc/codecs/rt5651.c +++ b/sound/soc/codecs/rt5651.c @@ -1513,8 +1513,6 @@ static int rt5651_set_dai_pll(struct snd_soc_dai *dai, int pll_id, int source, static int rt5651_set_bias_level(struct snd_soc_codec *codec, enum snd_soc_bias_level level) { - struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); - switch (level) { case SND_SOC_BIAS_STANDBY: if (snd_soc_codec_get_bias_level(codec) == SND_SOC_BIAS_OFF) { @@ -1527,9 +1525,6 @@ static int rt5651_set_bias_level(struct snd_soc_codec *codec, snd_soc_update_bits(codec, RT5651_PWR_ANLG1, RT5651_PWR_FV1 | RT5651_PWR_FV2, RT5651_PWR_FV1 | RT5651_PWR_FV2); - snd_soc_update_bits(codec, RT5651_PWR_ANLG1, - RT5651_PWR_LDO_DVO_MASK, - RT5651_PWR_LDO_DVO_1_2V); snd_soc_update_bits(codec, RT5651_D_MISC, 0x1, 0x1); if (snd_soc_read(codec, RT5651_PLL_MODE_1) & 0x9200) snd_soc_update_bits(codec, RT5651_D_MISC, @@ -1543,13 +1538,11 @@ static int rt5651_set_bias_level(struct snd_soc_codec *codec, snd_soc_write(codec, RT5651_PWR_DIG2, 0x0000); snd_soc_write(codec, RT5651_PWR_VOL, 0x0000); snd_soc_write(codec, RT5651_PWR_MIXER, 0x0000); - if (rt5651->pdata.jd_src) { - snd_soc_write(codec, RT5651_PWR_ANLG2, 0x0204); - snd_soc_write(codec, RT5651_PWR_ANLG1, 0x0002); - } else { - snd_soc_write(codec, RT5651_PWR_ANLG1, 0x0000); - snd_soc_write(codec, RT5651_PWR_ANLG2, 0x0000); - } + snd_soc_write(codec, RT5651_PWR_ANLG1, RT5651_PWR_LDO_DVO_1_2V); + /* Leave PLL1 and jack-detect power as is, all others off */ + snd_soc_update_bits(codec, RT5651_PWR_ANLG2, + ~(RT5651_PWR_PLL | RT5651_PWR_JD_M), + 0x0000); break;
default:
On Tue, Feb 20, 2018 at 11:15:04PM +0100, Hans de Goede wrote:
There is no need to set the LDO voltage to 1.2 volt each time we enter standby, instead always leave it 1.2 volt on BIAS_OFF. Note we do a snd_soc_codec_force_bias_level(BIAS_OFF) on probe, so this will configure it correctly right from the start.
That force on probe sounds like a problem... if this is being done once at startup it should be done in the probe function, not in the bias level configuration.
Also, are you sure this is a good fix? If the bias voltage is being configured all the time does that perhaps indicate that for better performance or something it should have been being set to some other voltage when the device is in standby?
Hi,
On 21-02-18 12:43, Mark Brown wrote:
On Tue, Feb 20, 2018 at 11:15:04PM +0100, Hans de Goede wrote:
There is no need to set the LDO voltage to 1.2 volt each time we enter standby, instead always leave it 1.2 volt on BIAS_OFF. Note we do a snd_soc_codec_force_bias_level(BIAS_OFF) on probe, so this will configure it correctly right from the start.
That force on probe sounds like a problem... if this is being done once at startup it should be done in the probe function, not in the bias level configuration.
This is more like "we do a snd_soc_codec_force_bias_level(BIAS_OFF) on probe anyways and that already sets the level to 1.2 volt, so we don't need to do this explicitly at probe time".
Also, are you sure this is a good fix? If the bias voltage is being configured all the time does that perhaps indicate that for better performance or something it should have been being set to some other voltage when the device is in standby?
We switch the LDO off when in the bias level is BIAS_OFF and on at 1.2 volt when in standby or higher. Before this commit we would unconditionally write 0 to PWR_ANLG1 not only turning off all supplies, but additionally changing the LDO voltage control bits to 00, which requires reprogramming them when we go back to standby. The value of the LDO voltage controls bits when in BIAS_OFF does not matter as the LDO on/off bit is set to off, so the purpose of this commit is to replace the blindly setting of PWR_ANLG1 to 0 with setting all the bits in PWR_ANLG1 to 0, while leaving the 2 LDO voltage-control bits as-is, so that we don't need to reprogram these 2 bits when entering standby.
Regards,
Hans
On Wed, Feb 21, 2018 at 09:11:34PM +0100, Hans de Goede wrote:
On 21-02-18 12:43, Mark Brown wrote:
On Tue, Feb 20, 2018 at 11:15:04PM +0100, Hans de Goede wrote:
There is no need to set the LDO voltage to 1.2 volt each time we enter standby, instead always leave it 1.2 volt on BIAS_OFF. Note we do a snd_soc_codec_force_bias_level(BIAS_OFF) on probe, so this will configure it correctly right from the start.
That force on probe sounds like a problem... if this is being done once at startup it should be done in the probe function, not in the bias level configuration.
This is more like "we do a snd_soc_codec_force_bias_level(BIAS_OFF) on probe anyways and that already sets the level to 1.2 volt, so we don't need to do this explicitly at probe time".
What I'm saying is that if the code is written such that you need to do that _force_bias_level() then it's not very idiomatic which is a bit worrying - it might be safer to reorganize the code so that this isn't required any more.
Also, are you sure this is a good fix? If the bias voltage is being configured all the time does that perhaps indicate that for better performance or something it should have been being set to some other voltage when the device is in standby?
We switch the LDO off when in the bias level is BIAS_OFF and on at 1.2 volt when in standby or higher. Before this commit we would
OK.
OVer-Current-Detection (OVCD) for the micbias current is used to detect if an inserted jack is a headset or headphones (mic shorted to ground).
Some boards may need different values for the OVCD threshold because of a resistor on the board in serial with or parallel to the jack mic contact.
This commit adds support for the sofar unset OVCD scale-factor register values and adds support for specifying both the current threshold and the scale-factor to pdata and this commets sets these values only once from rt5651_set_jack() instead of setting them every time we do jack-detection.
This commit sets the new pdata values for this to 2000uA with a scale-factor of 0.75 for the KIANO SlimNote 14.2 device, which is the only rt5652 using device on which jack-detection is currently enabled.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- include/sound/rt5651.h | 29 +++++++++++++++++++++++++++- sound/soc/codecs/rt5651.c | 27 +++++++++++++++++--------- sound/soc/codecs/rt5651.h | 10 ++++++++++ sound/soc/intel/boards/bytcr_rt5651.c | 36 +++++++++++++++++++++++++++++------ 4 files changed, 86 insertions(+), 16 deletions(-)
diff --git a/include/sound/rt5651.h b/include/sound/rt5651.h index 18b79a761f10..7b000406589c 100644 --- a/include/sound/rt5651.h +++ b/include/sound/rt5651.h @@ -18,12 +18,39 @@ enum rt5651_jd_src { RT5651_JD2, };
+/* These mirror the RT5651_MIC1_OVTH_*UA consts and MUST be in the same order */ +enum rt5651_ovth_curr { + RT5651_OVTH_600UA, + RT5651_OVTH_1500UA, + RT5651_OVTH_2000UA, +}; + +/* These mirror the RT5651_MIC_OVCD_SF* consts and MUST be in the same order */ +enum rt5651_ovcd_sf { + RT5651_OVCD_SF_0P5, + RT5651_OVCD_SF_0P75, + RT5651_OVCD_SF_1P0, + RT5651_OVCD_SF_1P5, +}; + +/* + * Note testing on various boards has shown that good defaults for ovth_curr + * and ovth_sf are 2000UA and 0.75. For an effective threshold of 1500UA, + * this seems to be more reliable then 1500UA and 1.0. Some boards may need + * different values because of a resistor on the board in serial with or + * parallel to the jack mic contact. + */ struct rt5651_platform_data { /* IN2 can optionally be differential */ bool in2_diff; - + /* Configure GPIO2 as DMIC1 SCL */ bool dmic_en; + /* Jack detect source or JD_NULL to disable jack-detect */ enum rt5651_jd_src jd_src; + /* Jack micbias overcurrent detect current threshold */ + enum rt5651_ovth_curr ovth_curr; + /* Jack micbias overcurrent detect current scale-factor */ + enum rt5651_ovcd_sf ovth_sf; };
#endif diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index 4b0509f7e001..1e20cdb8b569 100644 --- a/sound/soc/codecs/rt5651.c +++ b/sound/soc/codecs/rt5651.c @@ -1575,6 +1575,7 @@ static int rt5651_set_jack(struct snd_soc_codec *codec, regmap_update_bits(rt5651->regmap, RT5651_GPIO_CTRL1, RT5651_GP1_PIN_MASK, RT5651_GP1_PIN_IRQ);
+ /* Select jack detect source */ switch (rt5651->pdata.jd_src) { case RT5651_JD1_1: regmap_update_bits(rt5651->regmap, RT5651_JD_CTRL2, @@ -1607,11 +1608,25 @@ static int rt5651_set_jack(struct snd_soc_codec *codec, break; }
+ /* Enable jack detect power */ regmap_update_bits(rt5651->regmap, RT5651_PWR_ANLG2, RT5651_PWR_JD_M, RT5651_PWR_JD_M);
+ /* + * Set OVCD threshold current and scale-factor from pdata. + */ + regmap_write(rt5651->regmap, RT5651_PR_BASE + RT5651_BIAS_CUR4, 0xa800 | + (rt5651->pdata.ovth_sf << RT5651_MIC_OVCD_SF_SFT)); + regmap_update_bits(rt5651->regmap, RT5651_MICBIAS, - 0x38, 0x38); + RT5651_MIC1_OVCD_MASK | + RT5651_MIC1_OVTH_MASK | + RT5651_PWR_CLK12M_MASK | + RT5651_PWR_MB_MASK, + RT5651_MIC1_OVCD_DIS | + (rt5651->pdata.ovth_curr << RT5651_MIC1_OVTH_SFT) | + RT5651_PWR_MB_PU | + RT5651_PWR_CLK12M_PU);
ret = devm_request_threaded_irq(codec->dev, rt5651->irq, NULL, rt5651_irq, @@ -1795,14 +1810,8 @@ static int rt5651_jack_detect(struct snd_soc_codec *codec, int jack_insert) snd_soc_dapm_mutex_unlock(dapm);
snd_soc_update_bits(codec, RT5651_MICBIAS, - RT5651_MIC1_OVCD_MASK | - RT5651_MIC1_OVTH_MASK | - RT5651_PWR_CLK12M_MASK | - RT5651_PWR_MB_MASK, - RT5651_MIC1_OVCD_EN | - RT5651_MIC1_OVTH_600UA | - RT5651_PWR_MB_PU | - RT5651_PWR_CLK12M_PU); + RT5651_MIC1_OVCD_MASK, + RT5651_MIC1_OVCD_EN); msleep(100); if (snd_soc_read(codec, RT5651_IRQ_CTRL2) & RT5651_MB1_OC_CLR) jack_type = SND_JACK_HEADPHONE; diff --git a/sound/soc/codecs/rt5651.h b/sound/soc/codecs/rt5651.h index 151ac92f6bad..96168a1e87c1 100644 --- a/sound/soc/codecs/rt5651.h +++ b/sound/soc/codecs/rt5651.h @@ -138,6 +138,7 @@ /* Index of Codec Private Register definition */ #define RT5651_BIAS_CUR1 0x12 #define RT5651_BIAS_CUR3 0x14 +#define RT5651_BIAS_CUR4 0x15 #define RT5651_CLSD_INT_REG1 0x1c #define RT5651_CHPUMP_INT_REG1 0x24 #define RT5651_MAMP_INT_REG2 0x37 @@ -1966,6 +1967,15 @@ #define RT5651_D_GATE_EN_SFT 0
/* Codec Private Register definition */ + +/* MIC Over current threshold scale factor (0x15) */ +#define RT5651_MIC_OVCD_SF_MASK (0x3 << 8) +#define RT5651_MIC_OVCD_SF_SFT 8 +#define RT5651_MIC_OVCD_SF_0P5 (0x0 << 8) +#define RT5651_MIC_OVCD_SF_0P75 (0x1 << 8) +#define RT5651_MIC_OVCD_SF_1P0 (0x2 << 8) +#define RT5651_MIC_OVCD_SF_1P5 (0x3 << 8) + /* 3D Speaker Control (0x63) */ #define RT5651_3D_SPK_MASK (0x1 << 15) #define RT5651_3D_SPK_SFT 15 diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 8ef5b5500fb7..a6cc0bc85db8 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -49,11 +49,26 @@ enum { BYT_RT5651_JD2 = (RT5651_JD2 << 4), };
-#define BYT_RT5651_MAP(quirk) ((quirk) & GENMASK(3, 0)) -#define BYT_RT5651_JDSRC(quirk) (((quirk) & GENMASK(7, 4)) >> 4) -#define BYT_RT5651_DMIC_EN BIT(16) -#define BYT_RT5651_MCLK_EN BIT(17) -#define BYT_RT5651_MCLK_25MHZ BIT(18) +enum { + BYT_RT5651_OVTH_600UA = (RT5651_OVTH_600UA << 8), + BYT_RT5651_OVTH_1500UA = (RT5651_OVTH_1500UA << 8), + BYT_RT5651_OVTH_2000UA = (RT5651_OVTH_2000UA << 8), +}; + +enum { + BYT_RT5651_OVCD_SF_0P5 = (RT5651_OVCD_SF_0P5 << 12), + BYT_RT5651_OVCD_SF_0P75 = (RT5651_OVCD_SF_0P75 << 12), + BYT_RT5651_OVCD_SF_1P0 = (RT5651_OVCD_SF_1P0 << 12), + BYT_RT5651_OVCD_SF_1P5 = (RT5651_OVCD_SF_1P5 << 12), +}; + +#define BYT_RT5651_MAP(quirk) ((quirk) & GENMASK(3, 0)) +#define BYT_RT5651_JDSRC(quirk) (((quirk) & GENMASK(7, 4)) >> 4) +#define BYT_RT5651_OVTH(quirk) (((quirk) & GENMASK(11, 8)) >> 8) +#define BYT_RT5651_OVCD_SF(quirk) (((quirk) & GENMASK(15, 12)) >> 12) +#define BYT_RT5651_DMIC_EN BIT(16) +#define BYT_RT5651_MCLK_EN BIT(17) +#define BYT_RT5651_MCLK_25MHZ BIT(18)
struct byt_rt5651_private { struct clk *mclk; @@ -73,9 +88,14 @@ static void log_quirks(struct device *dev) dev_info(dev, "quirk IN2_MAP enabled"); if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN3_MAP) dev_info(dev, "quirk IN3_MAP enabled"); - if (BYT_RT5651_JDSRC(byt_rt5651_quirk)) + if (BYT_RT5651_JDSRC(byt_rt5651_quirk)) { dev_info(dev, "quirk jack-detect src %ld\n", BYT_RT5651_JDSRC(byt_rt5651_quirk)); + dev_info(dev, "quirk ovth_curr %ld\n", + BYT_RT5651_OVTH(byt_rt5651_quirk)); + dev_info(dev, "quirk ovth_sf %ld\n", + BYT_RT5651_OVCD_SF(byt_rt5651_quirk)); + } if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN) dev_info(dev, "quirk DMIC enabled"); if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) @@ -299,6 +319,8 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = { }, .driver_data = (void *)(BYT_RT5651_MCLK_EN | BYT_RT5651_JD1_1 | + BYT_RT5651_OVTH_2000UA | + BYT_RT5651_OVCD_SF_0P75 | BYT_RT5651_IN1_IN2_MAP), }, {} @@ -373,6 +395,8 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) }
pdata.jd_src = BYT_RT5651_JDSRC(byt_rt5651_quirk); + pdata.ovth_curr = BYT_RT5651_OVTH(byt_rt5651_quirk); + pdata.ovth_sf = BYT_RT5651_OVCD_SF(byt_rt5651_quirk); if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN) pdata.dmic_en = true; rt5651_set_pdata(codec, &pdata);
Headphone vs headset detection relies on OVCD and testing on the rt5640 and rt5651 (which seem to have the same OVCD detection) has shown that getting OVCD detection to work reliabe on these codecs is somewhat finicky.
This commit ports my work to make this reliable on the rt5640 over to the rt5651, making the following OVCD changes, each of which has been tested individually and each of which has shown to be necessary on the rt5651:
1) When the mic-gnd contacts are short-circuited by a headphones plug, the hardware periodically retries if it can apply the bias-current leading to the OVCD status flip-flopping 1-0-1 with it being 0 about 10% of the time. This commit enables the sticky bit for the OVCD status to deal with this.
2) When using RCCLK instead of MCLK / PLL1 the OVCD status often gets stuck at its last value, force-enable the platform clock to avoid this.
3) Change the detection algorithm to require 5 identical OVCD values in a row, as OVCD may bounce a bit after jack insertion.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- Note this is based on the jack-detection work I've been doing for the rt5640, I took a detour and ended up fixing this for rt5651 first, the rt5640 work is almost ready too and I hope to post a version of that upstream soon. --- include/sound/rt5651.h | 4 + sound/soc/codecs/rt5651.c | 237 +++++++++++++++++++++++----------- sound/soc/codecs/rt5651.h | 6 +- sound/soc/intel/boards/bytcr_rt5651.c | 2 + 4 files changed, 171 insertions(+), 78 deletions(-)
diff --git a/include/sound/rt5651.h b/include/sound/rt5651.h index 7b000406589c..7122236aec60 100644 --- a/include/sound/rt5651.h +++ b/include/sound/rt5651.h @@ -45,12 +45,16 @@ struct rt5651_platform_data { bool in2_diff; /* Configure GPIO2 as DMIC1 SCL */ bool dmic_en; + /* Jack detect is inverted */ + bool jd_inverted; /* Jack detect source or JD_NULL to disable jack-detect */ enum rt5651_jd_src jd_src; /* Jack micbias overcurrent detect current threshold */ enum rt5651_ovth_curr ovth_curr; /* Jack micbias overcurrent detect current scale-factor */ enum rt5651_ovcd_sf ovth_sf; + /* Platform clock dapm supply name */ + const char *clk; };
#endif diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c index 1e20cdb8b569..2a2ce0a7d870 100644 --- a/sound/soc/codecs/rt5651.c +++ b/sound/soc/codecs/rt5651.c @@ -1552,13 +1552,153 @@ static int rt5651_set_bias_level(struct snd_soc_codec *codec, return 0; }
+/* + * Note we do not toggle the RT5651_MIC1_OVCD_EN bit in these 2 functions, + * to ensure reliable OVCD operation it *must* be enabled before enabling + * the supplies, which may already be enabled. + */ +static void rt5651_enable_micbias1_ovcd(struct snd_soc_codec *codec) +{ + struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); + struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); + + snd_soc_dapm_mutex_lock(dapm); + snd_soc_dapm_force_enable_pin_unlocked(dapm, "LDO"); + snd_soc_dapm_force_enable_pin_unlocked(dapm, "micbias1"); + /* OVCD is unreliable when used with RCCLK as sysclk-source */ + if (rt5651->pdata.clk) + snd_soc_dapm_force_enable_pin_unlocked(dapm, rt5651->pdata.clk); + snd_soc_dapm_sync_unlocked(dapm); + snd_soc_dapm_mutex_unlock(dapm); +} + +static void rt5651_disable_micbias1_ovcd(struct snd_soc_codec *codec) +{ + struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); + struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); + + snd_soc_dapm_mutex_lock(dapm); + if (rt5651->pdata.clk) + snd_soc_dapm_disable_pin_unlocked(dapm, rt5651->pdata.clk); + snd_soc_dapm_disable_pin_unlocked(dapm, "micbias1"); + snd_soc_dapm_disable_pin_unlocked(dapm, "LDO"); + snd_soc_dapm_sync_unlocked(dapm); + snd_soc_dapm_mutex_unlock(dapm); +} + +static void rt5651_clear_micbias1_ovcd(struct snd_soc_codec *codec) +{ + snd_soc_update_bits(codec, RT5651_IRQ_CTRL2, RT5651_MB1_OC_STATUS, 0); +} + +static bool rt5651_micbias1_ovcd(struct snd_soc_codec *codec) +{ + int val; + + val = snd_soc_read(codec, RT5651_IRQ_CTRL2); + dev_dbg(codec->dev, "irq ctrl2 %#04x\n", val); + + return (val & RT5651_MB1_OC_STATUS); +} + +static bool rt5651_jack_inserted(struct snd_soc_codec *codec) +{ + struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec); + int val; + + val = snd_soc_read(codec, RT5651_INT_IRQ_ST); + dev_dbg(codec->dev, "irq status %#04x\n", val); + + switch (rt5651->pdata.jd_src) { + case RT5651_JD1_1: + val &= 0x1000; + break; + case RT5651_JD1_2: + val &= 0x2000; + break; + case RT5651_JD2: + val &= 0x4000; + break; + default: + break; + } + + if (rt5651->pdata.jd_inverted) + return val == 0; + else + return val != 0; +} + +/* Jack detect and button-press timings */ +#define JACK_SETTLE_TIME 100 /* milli seconds */ +#define JACK_DETECT_COUNT 5 +#define JACK_DETECT_MAXCOUNT 20 /* Aprox. 2 seconds worth of tries */ + +static int rt5651_detect_headset(struct snd_soc_codec *codec) +{ + int i, headset_count = 0, headphone_count = 0; + + /* + * We get the insertion event before the jack is fully inserted at which + * point the second ring on a TRRS connector may short the 2nd ring and + * sleeve contacts, also the overcurrent detection is not entirely + * reliable. So we try several times with a wait in between until we + * detect the same type JACK_DETECT_COUNT times in a row. + */ + for (i = 0; i < JACK_DETECT_MAXCOUNT; i++) { + /* Clear any previous over-current status flag */ + rt5651_clear_micbias1_ovcd(codec); + + msleep(JACK_SETTLE_TIME); + + /* Check the jack is still connected before checking ovcd */ + if (!rt5651_jack_inserted(codec)) + return 0; + + if (rt5651_micbias1_ovcd(codec)) { + /* + * Over current detected, there is a short between the + * 2nd ring contact and the ground, so a TRS connector + * without a mic contact and thus plain headphones. + */ + dev_dbg(codec->dev, "%s: mic-gnd shorted\n", __func__); + headset_count = 0; + headphone_count++; + if (headphone_count == JACK_DETECT_COUNT) + return SND_JACK_HEADPHONE; + } else { + dev_dbg(codec->dev, "%s: mic-gnd open\n", __func__); + headphone_count = 0; + headset_count++; + if (headset_count == JACK_DETECT_COUNT) + return SND_JACK_HEADSET; + } + } + + dev_err(codec->dev, "Error detecting headset vs headphones, bad contact?, assuming headphones\n"); + return SND_JACK_HEADPHONE; +} + +static void rt5651_jack_detect_work(struct work_struct *work) +{ + struct rt5651_priv *rt5651 = + container_of(work, struct rt5651_priv, jack_detect_work); + int report = 0; + + if (rt5651_jack_inserted(rt5651->codec)) { + rt5651_enable_micbias1_ovcd(rt5651->codec); + report = rt5651_detect_headset(rt5651->codec); + rt5651_disable_micbias1_ovcd(rt5651->codec); + } + + snd_soc_jack_report(rt5651->hp_jack, report, SND_JACK_HEADSET); +} + static irqreturn_t rt5651_irq(int irq, void *data) { struct rt5651_priv *rt5651 = data;
- queue_delayed_work(system_power_efficient_wq, - &rt5651->jack_detect_work, msecs_to_jiffies(250)); - + queue_work(system_power_efficient_wq, &rt5651->jack_detect_work); return IRQ_HANDLED; }
@@ -1614,6 +1754,8 @@ static int rt5651_set_jack(struct snd_soc_codec *codec,
/* * Set OVCD threshold current and scale-factor from pdata. + * OVCD seems to be more reliable when enabled before enabling the + * LDO2 / MICBIAS1 supplies, so we enable it here once. */ regmap_write(rt5651->regmap, RT5651_PR_BASE + RT5651_BIAS_CUR4, 0xa800 | (rt5651->pdata.ovth_sf << RT5651_MIC_OVCD_SF_SFT)); @@ -1623,11 +1765,23 @@ static int rt5651_set_jack(struct snd_soc_codec *codec, RT5651_MIC1_OVTH_MASK | RT5651_PWR_CLK12M_MASK | RT5651_PWR_MB_MASK, - RT5651_MIC1_OVCD_DIS | + RT5651_MIC1_OVCD_EN | (rt5651->pdata.ovth_curr << RT5651_MIC1_OVTH_SFT) | RT5651_PWR_MB_PU | RT5651_PWR_CLK12M_PU);
+ /* + * The over-current-detect is only reliable in detecting the absence + * of over-current, when the mic-contact in the jack is short-circuited, + * the hardware periodically retries if it can apply the bias-current + * leading to the ovcd status flip-flopping 1-0-1 with it being 0 about + * 10% of the time, as we poll the ovcd status bit we might hit that + * 10%, so we enable sticky mode and when checking OVCD we clear the + * status, msleep() a bit and then check to get a reliable reading. + */ + snd_soc_update_bits(codec, RT5651_IRQ_CTRL2, RT5651_MB1_OC_STKY_MASK, + RT5651_MB1_OC_STKY_EN); + ret = devm_request_threaded_irq(codec->dev, rt5651->irq, NULL, rt5651_irq, IRQF_TRIGGER_RISING | @@ -1639,7 +1793,8 @@ static int rt5651_set_jack(struct snd_soc_codec *codec, }
rt5651->hp_jack = hp_jack; - rt5651_irq(0, rt5651); + /* Get initial jack status */ + queue_work(system_power_efficient_wq, &rt5651->jack_detect_work);
return 0; } @@ -1797,74 +1952,6 @@ static int rt5651_parse_dt(struct rt5651_priv *rt5651, struct device_node *np) return 0; }
-static int rt5651_jack_detect(struct snd_soc_codec *codec, int jack_insert) -{ - struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); - int jack_type; - - if (jack_insert) { - snd_soc_dapm_mutex_lock(dapm); - snd_soc_dapm_force_enable_pin_unlocked(dapm, "LDO"); - snd_soc_dapm_force_enable_pin_unlocked(dapm, "micbias1"); - snd_soc_dapm_sync_unlocked(dapm); - snd_soc_dapm_mutex_unlock(dapm); - - snd_soc_update_bits(codec, RT5651_MICBIAS, - RT5651_MIC1_OVCD_MASK, - RT5651_MIC1_OVCD_EN); - msleep(100); - if (snd_soc_read(codec, RT5651_IRQ_CTRL2) & RT5651_MB1_OC_CLR) - jack_type = SND_JACK_HEADPHONE; - else - jack_type = SND_JACK_HEADSET; - snd_soc_update_bits(codec, RT5651_IRQ_CTRL2, - RT5651_MB1_OC_CLR, 0); - - snd_soc_dapm_mutex_lock(dapm); - snd_soc_dapm_disable_pin_unlocked(dapm, "micbias1"); - snd_soc_dapm_disable_pin_unlocked(dapm, "LDO"); - snd_soc_dapm_sync_unlocked(dapm); - snd_soc_dapm_mutex_unlock(dapm); - } else { /* jack out */ - jack_type = 0; - - snd_soc_update_bits(codec, RT5651_MICBIAS, - RT5651_MIC1_OVCD_MASK, - RT5651_MIC1_OVCD_DIS); - } - - return jack_type; -} - -static void rt5651_jack_detect_work(struct work_struct *work) -{ - struct rt5651_priv *rt5651 = - container_of(work, struct rt5651_priv, jack_detect_work.work); - - int report, val = 0; - - if (!rt5651->codec) - return; - - switch (rt5651->pdata.jd_src) { - case RT5651_JD1_1: - val = snd_soc_read(rt5651->codec, RT5651_INT_IRQ_ST) & 0x1000; - break; - case RT5651_JD1_2: - val = snd_soc_read(rt5651->codec, RT5651_INT_IRQ_ST) & 0x2000; - break; - case RT5651_JD2: - val = snd_soc_read(rt5651->codec, RT5651_INT_IRQ_ST) & 0x4000; - break; - default: - break; - } - - report = rt5651_jack_detect(rt5651->codec, !val); - - snd_soc_jack_report(rt5651->hp_jack, report, SND_JACK_HEADSET); -} - static void rt5651_apply_pdata(struct rt5651_priv *rt5651) { if (rt5651->pdata.in2_diff) @@ -1932,7 +2019,7 @@ static int rt5651_i2c_probe(struct i2c_client *i2c,
rt5651->irq = i2c->irq; rt5651->hp_mute = 1; - INIT_DELAYED_WORK(&rt5651->jack_detect_work, rt5651_jack_detect_work); + INIT_WORK(&rt5651->jack_detect_work, rt5651_jack_detect_work);
ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt5651, rt5651_dai, ARRAY_SIZE(rt5651_dai)); @@ -1944,7 +2031,7 @@ static int rt5651_i2c_remove(struct i2c_client *i2c) { struct rt5651_priv *rt5651 = i2c_get_clientdata(i2c);
- cancel_delayed_work_sync(&rt5651->jack_detect_work); + cancel_work_sync(&rt5651->jack_detect_work); snd_soc_unregister_codec(&i2c->dev);
return 0; diff --git a/sound/soc/codecs/rt5651.h b/sound/soc/codecs/rt5651.h index 96168a1e87c1..9d5f764ba467 100644 --- a/sound/soc/codecs/rt5651.h +++ b/sound/soc/codecs/rt5651.h @@ -1598,8 +1598,8 @@ #define RT5651_MB1_OC_P_NOR (0x0 << 7) #define RT5651_MB1_OC_P_INV (0x1 << 7) #define RT5651_MB2_OC_P_MASK (0x1 << 6) -#define RT5651_MB1_OC_CLR (0x1 << 3) -#define RT5651_MB1_OC_CLR_SFT 3 +#define RT5651_MB1_OC_STATUS (0x1 << 3) +#define RT5651_MB1_OC_STATUS_SFT 3 #define RT5651_STA_GPIO8 (0x1) #define RT5651_STA_GPIO8_BIT 0
@@ -2073,7 +2073,7 @@ struct rt5651_priv { struct rt5651_platform_data pdata; struct regmap *regmap; struct snd_soc_jack *hp_jack; - struct delayed_work jack_detect_work; + struct work_struct jack_detect_work;
int irq; int sysclk; diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index a6cc0bc85db8..dfe5bd3dbb6c 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -394,6 +394,8 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) dev_err(card->dev, "unable to set MCLK rate\n"); }
+ pdata.clk = "Platform Clock"; + pdata.jd_inverted = true; pdata.jd_src = BYT_RT5651_JDSRC(byt_rt5651_quirk); pdata.ovth_curr = BYT_RT5651_OVTH(byt_rt5651_quirk); pdata.ovth_sf = BYT_RT5651_OVCD_SF(byt_rt5651_quirk);
On Tue, Feb 20, 2018 at 11:15:06PM +0100, Hans de Goede wrote:
This commit ports my work to make this reliable on the rt5640 over to the rt5651, making the following OVCD changes, each of which has been tested individually and each of which has shown to be necessary on the rt5651:
It's a big rewrite everything commit :(
+/*
- Note we do not toggle the RT5651_MIC1_OVCD_EN bit in these 2 functions,
- to ensure reliable OVCD operation it *must* be enabled before enabling
- the supplies, which may already be enabled.
- */
+static void rt5651_enable_micbias1_ovcd(struct snd_soc_codec *codec) +{
I'm finding this comment pretty confusing - it would probably be clearer if it said something like "since the supplies may also be enabled by other DAPM operations we toggle the bit in $PLACE instead and only make sure the supplies are enabled here".
+static bool rt5651_jack_inserted(struct snd_soc_codec *codec) +{
BTW all these changes are in terms of snd_soc_codec not snd_soc_component, we're trying to move everytihng up a level so that all devices are components. A bunch of refactoring went in just after the merge window doing that.
Hi,
On 21-02-18 13:05, Mark Brown wrote:
On Tue, Feb 20, 2018 at 11:15:06PM +0100, Hans de Goede wrote:
This commit ports my work to make this reliable on the rt5640 over to the rt5651, making the following OVCD changes, each of which has been tested individually and each of which has shown to be necessary on the rt5651:
It's a big rewrite everything commit :(
Yes your right, given that the headphone vs headset detection was pretty broken before this commit I just ported my pending rt5640 work over in one go. I will split this up better for the next version.
+/*
- Note we do not toggle the RT5651_MIC1_OVCD_EN bit in these 2 functions,
- to ensure reliable OVCD operation it *must* be enabled before enabling
- the supplies, which may already be enabled.
- */
+static void rt5651_enable_micbias1_ovcd(struct snd_soc_codec *codec) +{
I'm finding this comment pretty confusing - it would probably be clearer if it said something like "since the supplies may also be enabled by other DAPM operations we toggle the bit in $PLACE instead and only make sure the supplies are enabled here".
Ok I will update the comment.
+static bool rt5651_jack_inserted(struct snd_soc_codec *codec) +{
BTW all these changes are in terms of snd_soc_codec not snd_soc_component, we're trying to move everytihng up a level so that all devices are components. A bunch of refactoring went in just after the merge window doing that.
Ugh, just my luck to hit a refactoring like that with a big series. Ah well I will rebase everything on top of your -next and try to use component instead of codec everywhere.
Regards,
Hans
When platform_clock_control() first selects PLL1 as sysclk the PLL_CTRL registers have not been setup yet and we effectively have an invalid clock configuration until byt_rt5651_aif1_hw_params() gets called.
Add a new byt_rt5651_prepare_and_enable_pll1() helper and use that from both platform_clock_control() and byt_rt5651_aif1_hw_params() to fix this.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/intel/boards/bytcr_rt5651.c | 79 ++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 38 deletions(-)
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index dfe5bd3dbb6c..15df49697e97 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -106,6 +106,44 @@ static void log_quirks(struct device *dev)
#define BYT_CODEC_DAI1 "rt5651-aif1"
+static int byt_rt5651_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai, + int rate) +{ + int ret; + + /* Configure the PLL before selecting it */ + if (!(byt_rt5651_quirk & BYT_RT5651_MCLK_EN)) { + /* 2x25 bit slots on SSP2 */ + ret = snd_soc_dai_set_pll(codec_dai, 0, + RT5651_PLL1_S_BCLK1, + rate * 50, rate * 512); + } else { + if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ) { + ret = snd_soc_dai_set_pll(codec_dai, 0, + RT5651_PLL1_S_MCLK, + 25000000, rate * 512); + } else { + ret = snd_soc_dai_set_pll(codec_dai, 0, + RT5651_PLL1_S_MCLK, + 19200000, rate * 512); + } + } + + if (ret < 0) { + dev_err(codec_dai->codec->dev, "can't set pll: %d\n", ret); + return ret; + } + + ret = snd_soc_dai_set_sysclk(codec_dai, RT5651_SCLK_S_PLL1, + rate * 512, SND_SOC_CLOCK_IN); + if (ret < 0) { + dev_err(codec_dai->codec->dev, "can't set clock %d\n", ret); + return ret; + } + + return 0; +} + static int platform_clock_control(struct snd_soc_dapm_widget *w, struct snd_kcontrol *k, int event) { @@ -131,9 +169,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w, return ret; } } - ret = snd_soc_dai_set_sysclk(codec_dai, RT5651_SCLK_S_PLL1, - 48000 * 512, - SND_SOC_CLOCK_IN); + ret = byt_rt5651_prepare_and_enable_pll1(codec_dai, 48000); } else { /* * Set codec clock source to internal clock before @@ -247,44 +283,11 @@ static int byt_rt5651_aif1_hw_params(struct snd_pcm_substream *substream, { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_dai *codec_dai = rtd->codec_dai; - int ret; + int rate = params_rate(params);
snd_soc_dai_set_bclk_ratio(codec_dai, 50);
- ret = snd_soc_dai_set_sysclk(codec_dai, RT5651_SCLK_S_PLL1, - params_rate(params) * 512, - SND_SOC_CLOCK_IN); - if (ret < 0) { - dev_err(rtd->dev, "can't set codec clock %d\n", ret); - return ret; - } - - if (!(byt_rt5651_quirk & BYT_RT5651_MCLK_EN)) { - /* 2x25 bit slots on SSP2 */ - ret = snd_soc_dai_set_pll(codec_dai, 0, - RT5651_PLL1_S_BCLK1, - params_rate(params) * 50, - params_rate(params) * 512); - } else { - if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ) { - ret = snd_soc_dai_set_pll(codec_dai, 0, - RT5651_PLL1_S_MCLK, - 25000000, - params_rate(params) * 512); - } else { - ret = snd_soc_dai_set_pll(codec_dai, 0, - RT5651_PLL1_S_MCLK, - 19200000, - params_rate(params) * 512); - } - } - - if (ret < 0) { - dev_err(rtd->dev, "can't set codec pll: %d\n", ret); - return ret; - } - - return 0; + return byt_rt5651_prepare_and_enable_pll1(codec_dai, rate); }
static int byt_rt5651_quirk_cb(const struct dmi_system_id *id)
All the mappings are named for where the internal mic is routed and in that sense the newly added in3_map really is the same as in1_map, what makes it different is that it maps the headset mic at IN3 rather then at IN2.
Rename in3_map to in1_hs_in3_map to better reflect what it actually does.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/intel/boards/bytcr_rt5651.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 15df49697e97..608a3339d523 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -39,7 +39,7 @@ enum { BYT_RT5651_IN1_MAP, BYT_RT5651_IN2_MAP, BYT_RT5651_IN1_IN2_MAP, - BYT_RT5651_IN3_MAP, + BYT_RT5651_IN1_HS_IN3_MAP, };
enum { @@ -86,8 +86,8 @@ static void log_quirks(struct device *dev) dev_info(dev, "quirk IN1_MAP enabled"); if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN2_MAP) dev_info(dev, "quirk IN2_MAP enabled"); - if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN3_MAP) - dev_info(dev, "quirk IN3_MAP enabled"); + if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN1_HS_IN3_MAP) + dev_info(dev, "quirk IN1_HS_IN3_MAP enabled"); if (BYT_RT5651_JDSRC(byt_rt5651_quirk)) { dev_info(dev, "quirk jack-detect src %ld\n", BYT_RT5651_JDSRC(byt_rt5651_quirk)); @@ -236,8 +236,8 @@ static const struct snd_soc_dapm_route byt_rt5651_intmic_dmic_map[] = {
static const struct snd_soc_dapm_route byt_rt5651_intmic_in1_map[] = { {"Internal Mic", NULL, "micbias1"}, - {"IN2P", NULL, "Headset Mic"}, {"IN1P", NULL, "Internal Mic"}, + {"IN2P", NULL, "Headset Mic"}, };
static const struct snd_soc_dapm_route byt_rt5651_intmic_in2_map[] = { @@ -253,10 +253,10 @@ static const struct snd_soc_dapm_route byt_rt5651_intmic_in1_in2_map[] = { {"IN3P", NULL, "Headset Mic"}, };
-static const struct snd_soc_dapm_route byt_rt5651_intmic_in3_map[] = { +static const struct snd_soc_dapm_route byt_rt5651_intmic_in1_hs_in3_map[] = { {"Internal Mic", NULL, "micbias1"}, - {"IN3P", NULL, "Headset Mic"}, {"IN1P", NULL, "Internal Mic"}, + {"IN3P", NULL, "Headset Mic"}, };
static const struct snd_kcontrol_new byt_rt5651_controls[] = { @@ -303,7 +303,7 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = { DMI_MATCH(DMI_SYS_VENDOR, "Circuitco"), DMI_MATCH(DMI_PRODUCT_NAME, "Minnowboard Max B3 PLATFORM"), }, - .driver_data = (void *)(BYT_RT5651_IN3_MAP), + .driver_data = (void *)(BYT_RT5651_IN1_HS_IN3_MAP), }, { .callback = byt_rt5651_quirk_cb, @@ -312,7 +312,7 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = { DMI_MATCH(DMI_PRODUCT_NAME, "Minnowboard Turbot"), }, .driver_data = (void *)(BYT_RT5651_MCLK_EN | - BYT_RT5651_IN3_MAP), + BYT_RT5651_IN1_HS_IN3_MAP), }, { .callback = byt_rt5651_quirk_cb, @@ -354,9 +354,9 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) custom_map = byt_rt5651_intmic_in1_in2_map; num_routes = ARRAY_SIZE(byt_rt5651_intmic_in1_in2_map); break; - case BYT_RT5651_IN3_MAP: - custom_map = byt_rt5651_intmic_in3_map; - num_routes = ARRAY_SIZE(byt_rt5651_intmic_in3_map); + case BYT_RT5651_IN1_HS_IN3_MAP: + custom_map = byt_rt5651_intmic_in1_hs_in3_map; + num_routes = ARRAY_SIZE(byt_rt5651_intmic_in1_hs_in3_map); break; default: custom_map = byt_rt5651_intmic_dmic_map;
Add a new IN2_HS_IN3 input map and add a quirk for the input mapping and jack-detect source for the Chuwi Vi8 Plus tablet, which uses this new map.
Note the Chuwi Vi8 Plus lists an extra GPIO in its codecs ACPI resources which needs to be driven high to enable the external speaker amplifier, this is not supported yet and will be fixed in a future patch.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/intel/boards/bytcr_rt5651.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 608a3339d523..9a23ac9172b4 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -40,6 +40,7 @@ enum { BYT_RT5651_IN2_MAP, BYT_RT5651_IN1_IN2_MAP, BYT_RT5651_IN1_HS_IN3_MAP, + BYT_RT5651_IN2_HS_IN3_MAP, };
enum { @@ -88,6 +89,8 @@ static void log_quirks(struct device *dev) dev_info(dev, "quirk IN2_MAP enabled"); if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN1_HS_IN3_MAP) dev_info(dev, "quirk IN1_HS_IN3_MAP enabled"); + if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN2_HS_IN3_MAP) + dev_info(dev, "quirk IN2_HS_IN3_MAP enabled"); if (BYT_RT5651_JDSRC(byt_rt5651_quirk)) { dev_info(dev, "quirk jack-detect src %ld\n", BYT_RT5651_JDSRC(byt_rt5651_quirk)); @@ -259,6 +262,12 @@ static const struct snd_soc_dapm_route byt_rt5651_intmic_in1_hs_in3_map[] = { {"IN3P", NULL, "Headset Mic"}, };
+static const struct snd_soc_dapm_route byt_rt5651_intmic_in2_hs_in3_map[] = { + {"Internal Mic", NULL, "micbias1"}, + {"IN2P", NULL, "Internal Mic"}, + {"IN3P", NULL, "Headset Mic"}, +}; + static const struct snd_kcontrol_new byt_rt5651_controls[] = { SOC_DAPM_PIN_SWITCH("Headphone"), SOC_DAPM_PIN_SWITCH("Headset Mic"), @@ -326,6 +335,19 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = { BYT_RT5651_OVCD_SF_0P75 | BYT_RT5651_IN1_IN2_MAP), }, + { + /* Chuwi Vi8 Plus (CWI519) */ + .callback = byt_rt5651_quirk_cb, + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"), + DMI_MATCH(DMI_PRODUCT_NAME, "D2D3_Vi8A1"), + }, + .driver_data = (void *)(BYT_RT5651_MCLK_EN | + BYT_RT5651_JD1_1 | + BYT_RT5651_OVTH_2000UA | + BYT_RT5651_OVCD_SF_0P75 | + BYT_RT5651_IN2_HS_IN3_MAP), + }, {} };
@@ -358,6 +380,10 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) custom_map = byt_rt5651_intmic_in1_hs_in3_map; num_routes = ARRAY_SIZE(byt_rt5651_intmic_in1_hs_in3_map); break; + case BYT_RT5651_IN2_HS_IN3_MAP: + custom_map = byt_rt5651_intmic_in2_hs_in3_map; + num_routes = ARRAY_SIZE(byt_rt5651_intmic_in2_hs_in3_map); + break; default: custom_map = byt_rt5651_intmic_dmic_map; num_routes = ARRAY_SIZE(byt_rt5651_intmic_dmic_map);
Despite its name being prefixed with bytcr, before this commit the bytcr_rt5651 machine driver could not work with Bay Trail CR boards, as those only have SSP0 and it only supported SSP0-AIF1 setups.
This commit adds support for this, autodetecting AIF1 vs AIF2 based on BIOS tables.
While at it also add support for SSP2-AIF2 setups, as that requires only minimal extra code on top of the code adding SSP0-AIF1 / SSP0-AIF2 support.
Note this code is all copy-pasted from bytcr_rt5640.c. I've looked into merging the 2 machine drivers into 1 to avoid copy-pasting, but there are enough subtile differences to make this hard *and* with all the quirks the machine driver already is full with if (variant-foo) then ... else ... constructs adding more of these is going to make the code unreadable.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/intel/boards/bytcr_rt5651.c | 225 +++++++++++++++++++++++++++++++--- 1 file changed, 206 insertions(+), 19 deletions(-)
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 9a23ac9172b4..c14c5940ff87 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -25,6 +25,7 @@ #include <linux/device.h> #include <linux/dmi.h> #include <linux/slab.h> +#include <asm/cpu_device_id.h> #include <asm/platform_sst_audio.h> #include <sound/pcm.h> #include <sound/pcm_params.h> @@ -70,14 +71,16 @@ enum { #define BYT_RT5651_DMIC_EN BIT(16) #define BYT_RT5651_MCLK_EN BIT(17) #define BYT_RT5651_MCLK_25MHZ BIT(18) +#define BYT_RT5651_SSP2_AIF2 BIT(19) /* default is using AIF1 */ +#define BYT_RT5651_SSP0_AIF1 BIT(20) +#define BYT_RT5651_SSP0_AIF2 BIT(21)
struct byt_rt5651_private { struct clk *mclk; struct snd_soc_jack jack; };
-static unsigned long byt_rt5651_quirk = BYT_RT5651_DMIC_MAP | - BYT_RT5651_MCLK_EN; +static unsigned long byt_rt5651_quirk = BYT_RT5651_MCLK_EN;
static void log_quirks(struct device *dev) { @@ -105,9 +108,16 @@ static void log_quirks(struct device *dev) dev_info(dev, "quirk MCLK_EN enabled"); if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ) dev_info(dev, "quirk MCLK_25MHZ enabled"); + if (byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) + dev_info(dev, "quirk SSP2_AIF2 enabled\n"); + if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) + dev_info(dev, "quirk SSP0_AIF1 enabled\n"); + if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2) + dev_info(dev, "quirk SSP0_AIF2 enabled\n"); }
#define BYT_CODEC_DAI1 "rt5651-aif1" +#define BYT_CODEC_DAI2 "rt5651-aif2"
static int byt_rt5651_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai, int rate) @@ -116,10 +126,19 @@ static int byt_rt5651_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai,
/* Configure the PLL before selecting it */ if (!(byt_rt5651_quirk & BYT_RT5651_MCLK_EN)) { - /* 2x25 bit slots on SSP2 */ - ret = snd_soc_dai_set_pll(codec_dai, 0, - RT5651_PLL1_S_BCLK1, - rate * 50, rate * 512); + /* use bitclock as PLL input */ + if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) || + (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) { + /* 2x16 bit slots on SSP0 */ + ret = snd_soc_dai_set_pll(codec_dai, 0, + RT5651_PLL1_S_BCLK1, + rate * 32, rate * 512); + } else { + /* 2x25 bit slots on SSP2 */ + ret = snd_soc_dai_set_pll(codec_dai, 0, + RT5651_PLL1_S_BCLK1, + rate * 50, rate * 512); + } } else { if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ) { ret = snd_soc_dai_set_pll(codec_dai, 0, @@ -157,6 +176,8 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w, int ret;
codec_dai = snd_soc_card_get_codec_dai(card, BYT_CODEC_DAI1); + if (!codec_dai) + codec_dai = snd_soc_card_get_codec_dai(card, BYT_CODEC_DAI2); if (!codec_dai) { dev_err(card->dev, "Codec dai not found; Unable to set platform clock\n"); @@ -214,13 +235,6 @@ static const struct snd_soc_dapm_route byt_rt5651_audio_map[] = { {"Speaker", NULL, "Platform Clock"}, {"Line In", NULL, "Platform Clock"},
- {"AIF1 Playback", NULL, "ssp2 Tx"}, - {"ssp2 Tx", NULL, "codec_out0"}, - {"ssp2 Tx", NULL, "codec_out1"}, - {"codec_in0", NULL, "ssp2 Rx"}, - {"codec_in1", NULL, "ssp2 Rx"}, - {"ssp2 Rx", NULL, "AIF1 Capture"}, - {"Headset Mic", NULL, "micbias1"}, /* lowercase for rt5651 */ {"Headphone", NULL, "HPOL"}, {"Headphone", NULL, "HPOR"}, @@ -268,6 +282,42 @@ static const struct snd_soc_dapm_route byt_rt5651_intmic_in2_hs_in3_map[] = { {"IN3P", NULL, "Headset Mic"}, };
+static const struct snd_soc_dapm_route byt_rt5651_ssp0_aif1_map[] = { + {"ssp0 Tx", NULL, "modem_out"}, + {"modem_in", NULL, "ssp0 Rx"}, + + {"AIF1 Playback", NULL, "ssp0 Tx"}, + {"ssp0 Rx", NULL, "AIF1 Capture"}, +}; + +static const struct snd_soc_dapm_route byt_rt5651_ssp0_aif2_map[] = { + {"ssp0 Tx", NULL, "modem_out"}, + {"modem_in", NULL, "ssp0 Rx"}, + + {"AIF2 Playback", NULL, "ssp0 Tx"}, + {"ssp0 Rx", NULL, "AIF2 Capture"}, +}; + +static const struct snd_soc_dapm_route byt_rt5651_ssp2_aif1_map[] = { + {"ssp2 Tx", NULL, "codec_out0"}, + {"ssp2 Tx", NULL, "codec_out1"}, + {"codec_in0", NULL, "ssp2 Rx"}, + {"codec_in1", NULL, "ssp2 Rx"}, + + {"AIF1 Playback", NULL, "ssp2 Tx"}, + {"ssp2 Rx", NULL, "AIF1 Capture"}, +}; + +static const struct snd_soc_dapm_route byt_rt5651_ssp2_aif2_map[] = { + {"ssp2 Tx", NULL, "codec_out0"}, + {"ssp2 Tx", NULL, "codec_out1"}, + {"codec_in0", NULL, "ssp2 Rx"}, + {"codec_in1", NULL, "ssp2 Rx"}, + + {"AIF2 Playback", NULL, "ssp2 Tx"}, + {"ssp2 Rx", NULL, "AIF2 Capture"}, +}; + static const struct snd_kcontrol_new byt_rt5651_controls[] = { SOC_DAPM_PIN_SWITCH("Headphone"), SOC_DAPM_PIN_SWITCH("Headset Mic"), @@ -392,6 +442,26 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) if (ret) return ret;
+ if (byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) { + ret = snd_soc_dapm_add_routes(&card->dapm, + byt_rt5651_ssp2_aif2_map, + ARRAY_SIZE(byt_rt5651_ssp2_aif2_map)); + } else if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) { + ret = snd_soc_dapm_add_routes(&card->dapm, + byt_rt5651_ssp0_aif1_map, + ARRAY_SIZE(byt_rt5651_ssp0_aif1_map)); + } else if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2) { + ret = snd_soc_dapm_add_routes(&card->dapm, + byt_rt5651_ssp0_aif2_map, + ARRAY_SIZE(byt_rt5651_ssp0_aif2_map)); + } else { + ret = snd_soc_dapm_add_routes(&card->dapm, + byt_rt5651_ssp2_aif1_map, + ARRAY_SIZE(byt_rt5651_ssp2_aif1_map)); + } + if (ret) + return ret; + ret = snd_soc_add_card_controls(card, byt_rt5651_controls, ARRAY_SIZE(byt_rt5651_controls)); if (ret) { @@ -464,18 +534,26 @@ static int byt_rt5651_codec_fixup(struct snd_soc_pcm_runtime *rtd, SNDRV_PCM_HW_PARAM_RATE); struct snd_interval *channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); - int ret; + int ret, bits;
- /* The DSP will covert the FE rate to 48k, stereo, 24bits */ + /* The DSP will covert the FE rate to 48k, stereo */ rate->min = rate->max = 48000; channels->min = channels->max = 2;
- /* set SSP2 to 24-bit */ - params_set_format(params, SNDRV_PCM_FORMAT_S24_LE); + if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) || + (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) { + /* set SSP0 to 16-bit */ + params_set_format(params, SNDRV_PCM_FORMAT_S16_LE); + bits = 16; + } else { + /* set SSP2 to 24-bit */ + params_set_format(params, SNDRV_PCM_FORMAT_S24_LE); + bits = 24; + }
/* * Default mode for SSP configuration is TDM 4 slot, override config - * with explicit setting to I2S 2ch 24-bit. The word length is set with + * with explicit setting to I2S 2ch. The word length is set with * dai_set_tdm_slot() since there is no other API exposed */ ret = snd_soc_dai_set_fmt(rtd->cpu_dai, @@ -489,7 +567,7 @@ static int byt_rt5651_codec_fixup(struct snd_soc_pcm_runtime *rtd, return ret; }
- ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x3, 0x3, 2, 24); + ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x3, 0x3, 2, bits); if (ret < 0) { dev_err(rtd->dev, "can't set I2S config, err %d\n", ret); return ret; @@ -583,12 +661,32 @@ static struct snd_soc_card byt_rt5651_card = { };
static char byt_rt5651_codec_name[SND_ACPI_I2C_ID_LEN]; +static char byt_rt5651_codec_aif_name[12]; /* = "rt5651-aif[1|2]" */ +static char byt_rt5651_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ + +static bool is_valleyview(void) +{ + static const struct x86_cpu_id cpu_ids[] = { + { X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */ + {} + }; + + if (!x86_match_cpu(cpu_ids)) + return false; + return true; +} + +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 */ +};
static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) { struct byt_rt5651_private *priv; struct snd_soc_acpi_mach *mach; const char *i2c_name = NULL; + bool is_bytcr = false; int ret_val = 0; int dai_index = 0; int i; @@ -620,10 +718,99 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) byt_rt5651_dais[dai_index].codec_name = byt_rt5651_codec_name; }
+ /* + * swap SSP0 if bytcr is detected + * (will be overridden if DMI quirk is detected) + */ + if (is_valleyview()) { + struct sst_platform_info *p_info = mach->pdata; + const struct sst_res_info *res_info = p_info->res_info; + + if (res_info->acpi_ipc_irq_index == 0) + is_bytcr = true; + } + + if (is_bytcr) { + /* + * Baytrail CR platforms may have CHAN package in BIOS, try + * to find relevant routing quirk based as done on Windows + * platforms. We have to read the information directly from the + * BIOS, at this stage the card is not created and the links + * with the codec driver/pdata are non-existent + */ + + struct acpi_chan_package chan_package; + + /* format specified: 2 64-bit integers */ + struct acpi_buffer format = {sizeof("NN"), "NN"}; + struct acpi_buffer state = {0, NULL}; + struct snd_soc_acpi_package_context pkg_ctx; + bool pkg_found = false; + + state.length = sizeof(chan_package); + state.pointer = &chan_package; + + pkg_ctx.name = "CHAN"; + pkg_ctx.length = 2; + pkg_ctx.format = &format; + pkg_ctx.state = &state; + pkg_ctx.data_valid = false; + + pkg_found = snd_soc_acpi_find_package_from_hid(mach->id, + &pkg_ctx); + if (pkg_found) { + if (chan_package.aif_value == 1) { + dev_info(&pdev->dev, "BIOS Routing: AIF1 connected\n"); + byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF1; + } else if (chan_package.aif_value == 2) { + dev_info(&pdev->dev, "BIOS Routing: AIF2 connected\n"); + byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF2; + } else { + dev_info(&pdev->dev, "BIOS Routing isn't valid, ignored\n"); + pkg_found = false; + } + } + + if (!pkg_found) { + /* no BIOS indications, assume SSP0-AIF2 connection */ + byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF2; + } + + /* change defaults for Baytrail-CR capture */ + byt_rt5651_quirk |= BYT_RT5651_JD1_1 | + BYT_RT5651_OVTH_2000UA | + BYT_RT5651_OVCD_SF_0P75 | + BYT_RT5651_IN2_HS_IN3_MAP; + } else { + byt_rt5651_quirk |= BYT_RT5651_DMIC_MAP; + } + /* check quirks before creating card */ dmi_check_system(byt_rt5651_quirk_table); log_quirks(&pdev->dev);
+ if ((byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) || + (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) { + /* fixup codec aif name */ + snprintf(byt_rt5651_codec_aif_name, + sizeof(byt_rt5651_codec_aif_name), + "%s", "rt5651-aif2"); + + byt_rt5651_dais[dai_index].codec_dai_name = + byt_rt5651_codec_aif_name; + } + + if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) || + (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) { + /* fixup cpu dai name name */ + snprintf(byt_rt5651_cpu_dai_name, + sizeof(byt_rt5651_cpu_dai_name), + "%s", "ssp0-port"); + + byt_rt5651_dais[dai_index].cpu_dai_name = + byt_rt5651_cpu_dai_name; + } + if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) { priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3"); if (IS_ERR(priv->mclk)) {
On 2/20/18 4:15 PM, Hans de Goede wrote:
Despite its name being prefixed with bytcr, before this commit the bytcr_rt5651 machine driver could not work with Bay Trail CR boards, as those only have SSP0 and it only supported SSP0-AIF1 setups.
This commit adds support for this, autodetecting AIF1 vs AIF2 based on BIOS tables.
While at it also add support for SSP2-AIF2 setups, as that requires only minimal extra code on top of the code adding SSP0-AIF1 / SSP0-AIF2 support.
Note this code is all copy-pasted from bytcr_rt5640.c. I've looked into merging the 2 machine drivers into 1 to avoid copy-pasting, but there are enough subtile differences to make this hard *and* with all the quirks the machine driver already is full with if (variant-foo) then ... else ... constructs adding more of these is going to make the code unreadable.
Signed-off-by: Hans de Goede hdegoede@redhat.com
sound/soc/intel/boards/bytcr_rt5651.c | 225 +++++++++++++++++++++++++++++++--- 1 file changed, 206 insertions(+), 19 deletions(-)
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 9a23ac9172b4..c14c5940ff87 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -25,6 +25,7 @@ #include <linux/device.h> #include <linux/dmi.h> #include <linux/slab.h> +#include <asm/cpu_device_id.h> #include <asm/platform_sst_audio.h> #include <sound/pcm.h> #include <sound/pcm_params.h> @@ -70,14 +71,16 @@ enum { #define BYT_RT5651_DMIC_EN BIT(16) #define BYT_RT5651_MCLK_EN BIT(17) #define BYT_RT5651_MCLK_25MHZ BIT(18) +#define BYT_RT5651_SSP2_AIF2 BIT(19) /* default is using AIF1 */ +#define BYT_RT5651_SSP0_AIF1 BIT(20) +#define BYT_RT5651_SSP0_AIF2 BIT(21)
struct byt_rt5651_private { struct clk *mclk; struct snd_soc_jack jack; };
-static unsigned long byt_rt5651_quirk = BYT_RT5651_DMIC_MAP |
BYT_RT5651_MCLK_EN;+static unsigned long byt_rt5651_quirk = BYT_RT5651_MCLK_EN;
static void log_quirks(struct device *dev) { @@ -105,9 +108,16 @@ static void log_quirks(struct device *dev) dev_info(dev, "quirk MCLK_EN enabled"); if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ) dev_info(dev, "quirk MCLK_25MHZ enabled");
if (byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2)
dev_info(dev, "quirk SSP2_AIF2 enabled\n");if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1)
dev_info(dev, "quirk SSP0_AIF1 enabled\n");if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)
dev_info(dev, "quirk SSP0_AIF2 enabled\n");}
#define BYT_CODEC_DAI1 "rt5651-aif1"
+#define BYT_CODEC_DAI2 "rt5651-aif2"
static int byt_rt5651_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai, int rate) @@ -116,10 +126,19 @@ static int byt_rt5651_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai,
/* Configure the PLL before selecting it */ if (!(byt_rt5651_quirk & BYT_RT5651_MCLK_EN)) {
/* 2x25 bit slots on SSP2 */ret = snd_soc_dai_set_pll(codec_dai, 0,RT5651_PLL1_S_BCLK1,rate * 50, rate * 512);
/* use bitclock as PLL input */if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) ||(byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) {/* 2x16 bit slots on SSP0 */ret = snd_soc_dai_set_pll(codec_dai, 0,RT5651_PLL1_S_BCLK1,rate * 32, rate * 512);} else {/* 2x25 bit slots on SSP2 */ret = snd_soc_dai_set_pll(codec_dai, 0,RT5651_PLL1_S_BCLK1,rate * 50, rate * 512);}
This part is conflicting a bit with what we are currently enabling for SOF - and that's probably because the quirks combine two separate pieces of information. 1. the SSP0-AIF1 routing 2. the limitation to 16-bit on SSP0 with the legacy closed-source firmware.
Could we instead use information coming from the actual hardware params, as done in the fixup() function below? If possible I'd like to limit this 16/24 configuration to the fixup, if we add it it the PLL settings it's not going to work so well.
Thanks!
} else { if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ) { ret = snd_soc_dai_set_pll(codec_dai, 0, @@ -157,6 +176,8 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w, int ret;
codec_dai = snd_soc_card_get_codec_dai(card, BYT_CODEC_DAI1);
- if (!codec_dai)
codec_dai = snd_soc_card_get_codec_dai(card, BYT_CODEC_DAI2);@@ -214,13 +235,6 @@ static const struct snd_soc_dapm_route byt_rt5651_audio_map[] = { {"Speaker", NULL, "Platform Clock"}, {"Line In", NULL, "Platform Clock"},
- {"AIF1 Playback", NULL, "ssp2 Tx"},
- {"ssp2 Tx", NULL, "codec_out0"},
- {"ssp2 Tx", NULL, "codec_out1"},
- {"codec_in0", NULL, "ssp2 Rx"},
- {"codec_in1", NULL, "ssp2 Rx"},
- {"ssp2 Rx", NULL, "AIF1 Capture"},
- {"Headset Mic", NULL, "micbias1"}, /* lowercase for rt5651 */ {"Headphone", NULL, "HPOL"}, {"Headphone", NULL, "HPOR"},
@@ -268,6 +282,42 @@ static const struct snd_soc_dapm_route byt_rt5651_intmic_in2_hs_in3_map[] = { {"IN3P", NULL, "Headset Mic"}, };
+static const struct snd_soc_dapm_route byt_rt5651_ssp0_aif1_map[] = {
- {"ssp0 Tx", NULL, "modem_out"},
- {"modem_in", NULL, "ssp0 Rx"},
- {"AIF1 Playback", NULL, "ssp0 Tx"},
- {"ssp0 Rx", NULL, "AIF1 Capture"},
+};
+static const struct snd_soc_dapm_route byt_rt5651_ssp0_aif2_map[] = {
- {"ssp0 Tx", NULL, "modem_out"},
- {"modem_in", NULL, "ssp0 Rx"},
- {"AIF2 Playback", NULL, "ssp0 Tx"},
- {"ssp0 Rx", NULL, "AIF2 Capture"},
+};
+static const struct snd_soc_dapm_route byt_rt5651_ssp2_aif1_map[] = {
- {"ssp2 Tx", NULL, "codec_out0"},
- {"ssp2 Tx", NULL, "codec_out1"},
- {"codec_in0", NULL, "ssp2 Rx"},
- {"codec_in1", NULL, "ssp2 Rx"},
- {"AIF1 Playback", NULL, "ssp2 Tx"},
- {"ssp2 Rx", NULL, "AIF1 Capture"},
+};
+static const struct snd_soc_dapm_route byt_rt5651_ssp2_aif2_map[] = {
- {"ssp2 Tx", NULL, "codec_out0"},
- {"ssp2 Tx", NULL, "codec_out1"},
- {"codec_in0", NULL, "ssp2 Rx"},
- {"codec_in1", NULL, "ssp2 Rx"},
- {"AIF2 Playback", NULL, "ssp2 Tx"},
- {"ssp2 Rx", NULL, "AIF2 Capture"},
+};
- static const struct snd_kcontrol_new byt_rt5651_controls[] = { SOC_DAPM_PIN_SWITCH("Headphone"), SOC_DAPM_PIN_SWITCH("Headset Mic"),
@@ -392,6 +442,26 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) if (ret) return ret;
- if (byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) {
ret = snd_soc_dapm_add_routes(&card->dapm,byt_rt5651_ssp2_aif2_map,ARRAY_SIZE(byt_rt5651_ssp2_aif2_map));- } else if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) {
ret = snd_soc_dapm_add_routes(&card->dapm,byt_rt5651_ssp0_aif1_map,ARRAY_SIZE(byt_rt5651_ssp0_aif1_map));- } else if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2) {
ret = snd_soc_dapm_add_routes(&card->dapm,byt_rt5651_ssp0_aif2_map,ARRAY_SIZE(byt_rt5651_ssp0_aif2_map));- } else {
ret = snd_soc_dapm_add_routes(&card->dapm,byt_rt5651_ssp2_aif1_map,ARRAY_SIZE(byt_rt5651_ssp2_aif1_map));- }
- if (ret)
return ret;- ret = snd_soc_add_card_controls(card, byt_rt5651_controls, ARRAY_SIZE(byt_rt5651_controls)); if (ret) {
@@ -464,18 +534,26 @@ static int byt_rt5651_codec_fixup(struct snd_soc_pcm_runtime *rtd, SNDRV_PCM_HW_PARAM_RATE); struct snd_interval *channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
- int ret;
- int ret, bits;
- /* The DSP will covert the FE rate to 48k, stereo, 24bits */
- /* The DSP will covert the FE rate to 48k, stereo */ rate->min = rate->max = 48000; channels->min = channels->max = 2;
- /* set SSP2 to 24-bit */
- params_set_format(params, SNDRV_PCM_FORMAT_S24_LE);
if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) ||
(byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) {/* set SSP0 to 16-bit */params_set_format(params, SNDRV_PCM_FORMAT_S16_LE);bits = 16;} else {
/* set SSP2 to 24-bit */params_set_format(params, SNDRV_PCM_FORMAT_S24_LE);bits = 24;}
/*
- Default mode for SSP configuration is TDM 4 slot, override config
* with explicit setting to I2S 2ch 24-bit. The word length is set with
* with explicit setting to I2S 2ch. The word length is set with*/ ret = snd_soc_dai_set_fmt(rtd->cpu_dai,
- dai_set_tdm_slot() since there is no other API exposed
@@ -489,7 +567,7 @@ static int byt_rt5651_codec_fixup(struct snd_soc_pcm_runtime *rtd, return ret; }
- ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x3, 0x3, 2, 24);
- ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x3, 0x3, 2, bits); if (ret < 0) { dev_err(rtd->dev, "can't set I2S config, err %d\n", ret); return ret;
@@ -583,12 +661,32 @@ static struct snd_soc_card byt_rt5651_card = { };
static char byt_rt5651_codec_name[SND_ACPI_I2C_ID_LEN]; +static char byt_rt5651_codec_aif_name[12]; /* = "rt5651-aif[1|2]" */ +static char byt_rt5651_cpu_dai_name[10]; /* = "ssp[0|2]-port" */
+static bool is_valleyview(void) +{
- static const struct x86_cpu_id cpu_ids[] = {
{ X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */{}- };
- if (!x86_match_cpu(cpu_ids))
return false;- return true;
+}
+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 */
+};
static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) { struct byt_rt5651_private *priv; struct snd_soc_acpi_mach *mach; const char *i2c_name = NULL;
- bool is_bytcr = false; int ret_val = 0; int dai_index = 0; int i;
@@ -620,10 +718,99 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) byt_rt5651_dais[dai_index].codec_name = byt_rt5651_codec_name; }
/*
* swap SSP0 if bytcr is detected* (will be overridden if DMI quirk is detected)*/if (is_valleyview()) {
struct sst_platform_info *p_info = mach->pdata;const struct sst_res_info *res_info = p_info->res_info;if (res_info->acpi_ipc_irq_index == 0)is_bytcr = true;}
if (is_bytcr) {
/** Baytrail CR platforms may have CHAN package in BIOS, try* to find relevant routing quirk based as done on Windows* platforms. We have to read the information directly from the* BIOS, at this stage the card is not created and the links* with the codec driver/pdata are non-existent*/struct acpi_chan_package chan_package;/* format specified: 2 64-bit integers */struct acpi_buffer format = {sizeof("NN"), "NN"};struct acpi_buffer state = {0, NULL};struct snd_soc_acpi_package_context pkg_ctx;bool pkg_found = false;state.length = sizeof(chan_package);state.pointer = &chan_package;pkg_ctx.name = "CHAN";pkg_ctx.length = 2;pkg_ctx.format = &format;pkg_ctx.state = &state;pkg_ctx.data_valid = false;pkg_found = snd_soc_acpi_find_package_from_hid(mach->id,&pkg_ctx);if (pkg_found) {if (chan_package.aif_value == 1) {dev_info(&pdev->dev, "BIOS Routing: AIF1 connected\n");byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF1;} else if (chan_package.aif_value == 2) {dev_info(&pdev->dev, "BIOS Routing: AIF2 connected\n");byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF2;} else {dev_info(&pdev->dev, "BIOS Routing isn't valid, ignored\n");pkg_found = false;}}if (!pkg_found) {/* no BIOS indications, assume SSP0-AIF2 connection */byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF2;}/* change defaults for Baytrail-CR capture */byt_rt5651_quirk |= BYT_RT5651_JD1_1 |BYT_RT5651_OVTH_2000UA |BYT_RT5651_OVCD_SF_0P75 |BYT_RT5651_IN2_HS_IN3_MAP;} else {
byt_rt5651_quirk |= BYT_RT5651_DMIC_MAP;}
/* check quirks before creating card */ dmi_check_system(byt_rt5651_quirk_table); log_quirks(&pdev->dev);
if ((byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) ||
(byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) {/* fixup codec aif name */snprintf(byt_rt5651_codec_aif_name,sizeof(byt_rt5651_codec_aif_name),"%s", "rt5651-aif2");byt_rt5651_dais[dai_index].codec_dai_name =byt_rt5651_codec_aif_name;}
if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) ||
(byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) {/* fixup cpu dai name name */snprintf(byt_rt5651_cpu_dai_name,sizeof(byt_rt5651_cpu_dai_name),"%s", "ssp0-port");byt_rt5651_dais[dai_index].cpu_dai_name =byt_rt5651_cpu_dai_name;}
if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) { priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3"); if (IS_ERR(priv->mclk)) {
Hi,
On 20-02-18 23:44, Pierre-Louis Bossart wrote:
On 2/20/18 4:15 PM, Hans de Goede wrote:
Despite its name being prefixed with bytcr, before this commit the bytcr_rt5651 machine driver could not work with Bay Trail CR boards, as those only have SSP0 and it only supported SSP0-AIF1 setups.
This commit adds support for this, autodetecting AIF1 vs AIF2 based on BIOS tables.
While at it also add support for SSP2-AIF2 setups, as that requires only minimal extra code on top of the code adding SSP0-AIF1 / SSP0-AIF2 support.
Note this code is all copy-pasted from bytcr_rt5640.c. I've looked into merging the 2 machine drivers into 1 to avoid copy-pasting, but there are enough subtile differences to make this hard *and* with all the quirks the machine driver already is full with if (variant-foo) then ... else ... constructs adding more of these is going to make the code unreadable.
Signed-off-by: Hans de Goede hdegoede@redhat.com
sound/soc/intel/boards/bytcr_rt5651.c | 225 +++++++++++++++++++++++++++++++--- 1 file changed, 206 insertions(+), 19 deletions(-)
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 9a23ac9172b4..c14c5940ff87 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -25,6 +25,7 @@ #include <linux/device.h> #include <linux/dmi.h> #include <linux/slab.h> +#include <asm/cpu_device_id.h> #include <asm/platform_sst_audio.h> #include <sound/pcm.h> #include <sound/pcm_params.h> @@ -70,14 +71,16 @@ enum { #define BYT_RT5651_DMIC_EN BIT(16) #define BYT_RT5651_MCLK_EN BIT(17) #define BYT_RT5651_MCLK_25MHZ BIT(18) +#define BYT_RT5651_SSP2_AIF2 BIT(19) /* default is using AIF1 */ +#define BYT_RT5651_SSP0_AIF1 BIT(20) +#define BYT_RT5651_SSP0_AIF2 BIT(21) struct byt_rt5651_private { struct clk *mclk; struct snd_soc_jack jack; }; -static unsigned long byt_rt5651_quirk = BYT_RT5651_DMIC_MAP | - BYT_RT5651_MCLK_EN; +static unsigned long byt_rt5651_quirk = BYT_RT5651_MCLK_EN; static void log_quirks(struct device *dev) { @@ -105,9 +108,16 @@ static void log_quirks(struct device *dev) dev_info(dev, "quirk MCLK_EN enabled"); if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ) dev_info(dev, "quirk MCLK_25MHZ enabled"); + if (byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) + dev_info(dev, "quirk SSP2_AIF2 enabled\n"); + if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) + dev_info(dev, "quirk SSP0_AIF1 enabled\n"); + if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2) + dev_info(dev, "quirk SSP0_AIF2 enabled\n"); } #define BYT_CODEC_DAI1 "rt5651-aif1" +#define BYT_CODEC_DAI2 "rt5651-aif2" static int byt_rt5651_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai, int rate) @@ -116,10 +126,19 @@ static int byt_rt5651_prepare_and_enable_pll1(struct snd_soc_dai *codec_dai, /* Configure the PLL before selecting it */ if (!(byt_rt5651_quirk & BYT_RT5651_MCLK_EN)) { - /* 2x25 bit slots on SSP2 */ - ret = snd_soc_dai_set_pll(codec_dai, 0, - RT5651_PLL1_S_BCLK1, - rate * 50, rate * 512); + /* use bitclock as PLL input */ + if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) || + (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) { + /* 2x16 bit slots on SSP0 */ + ret = snd_soc_dai_set_pll(codec_dai, 0, + RT5651_PLL1_S_BCLK1, + rate * 32, rate * 512); + } else { + /* 2x25 bit slots on SSP2 */ + ret = snd_soc_dai_set_pll(codec_dai, 0, + RT5651_PLL1_S_BCLK1, + rate * 50, rate * 512); + }
This part is conflicting a bit with what we are currently enabling for SOF - and that's probably because the quirks combine two separate pieces of information.
- the SSP0-AIF1 routing
- the limitation to 16-bit on SSP0 with the legacy closed-source firmware.
Could we instead use information coming from the actual hardware params, as done in the fixup() function below? If possible I'd like to limit this 16/24 configuration to the fixup, if we add it it the PLL settings it's not going to work so well.
Ok, I believe I've fixed this to be more to your liking for v2 of this patch-set which I'm working on.
Regards,
Hans
Thanks!
} else { if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ) { ret = snd_soc_dai_set_pll(codec_dai, 0, @@ -157,6 +176,8 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w, int ret; codec_dai = snd_soc_card_get_codec_dai(card, BYT_CODEC_DAI1); + if (!codec_dai) + codec_dai = snd_soc_card_get_codec_dai(card, BYT_CODEC_DAI2); if (!codec_dai) { dev_err(card->dev, "Codec dai not found; Unable to set platform clock\n"); @@ -214,13 +235,6 @@ static const struct snd_soc_dapm_route byt_rt5651_audio_map[] = { {"Speaker", NULL, "Platform Clock"}, {"Line In", NULL, "Platform Clock"}, - {"AIF1 Playback", NULL, "ssp2 Tx"}, - {"ssp2 Tx", NULL, "codec_out0"}, - {"ssp2 Tx", NULL, "codec_out1"}, - {"codec_in0", NULL, "ssp2 Rx"}, - {"codec_in1", NULL, "ssp2 Rx"}, - {"ssp2 Rx", NULL, "AIF1 Capture"},
{"Headset Mic", NULL, "micbias1"}, /* lowercase for rt5651 */ {"Headphone", NULL, "HPOL"}, {"Headphone", NULL, "HPOR"}, @@ -268,6 +282,42 @@ static const struct snd_soc_dapm_route byt_rt5651_intmic_in2_hs_in3_map[] = { {"IN3P", NULL, "Headset Mic"}, }; +static const struct snd_soc_dapm_route byt_rt5651_ssp0_aif1_map[] = { + {"ssp0 Tx", NULL, "modem_out"}, + {"modem_in", NULL, "ssp0 Rx"},
+ {"AIF1 Playback", NULL, "ssp0 Tx"}, + {"ssp0 Rx", NULL, "AIF1 Capture"}, +};
+static const struct snd_soc_dapm_route byt_rt5651_ssp0_aif2_map[] = { + {"ssp0 Tx", NULL, "modem_out"}, + {"modem_in", NULL, "ssp0 Rx"},
+ {"AIF2 Playback", NULL, "ssp0 Tx"}, + {"ssp0 Rx", NULL, "AIF2 Capture"}, +};
+static const struct snd_soc_dapm_route byt_rt5651_ssp2_aif1_map[] = { + {"ssp2 Tx", NULL, "codec_out0"}, + {"ssp2 Tx", NULL, "codec_out1"}, + {"codec_in0", NULL, "ssp2 Rx"}, + {"codec_in1", NULL, "ssp2 Rx"},
+ {"AIF1 Playback", NULL, "ssp2 Tx"}, + {"ssp2 Rx", NULL, "AIF1 Capture"}, +};
+static const struct snd_soc_dapm_route byt_rt5651_ssp2_aif2_map[] = { + {"ssp2 Tx", NULL, "codec_out0"}, + {"ssp2 Tx", NULL, "codec_out1"}, + {"codec_in0", NULL, "ssp2 Rx"}, + {"codec_in1", NULL, "ssp2 Rx"},
+ {"AIF2 Playback", NULL, "ssp2 Tx"}, + {"ssp2 Rx", NULL, "AIF2 Capture"}, +};
static const struct snd_kcontrol_new byt_rt5651_controls[] = { SOC_DAPM_PIN_SWITCH("Headphone"), SOC_DAPM_PIN_SWITCH("Headset Mic"), @@ -392,6 +442,26 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime) if (ret) return ret; + if (byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) { + ret = snd_soc_dapm_add_routes(&card->dapm, + byt_rt5651_ssp2_aif2_map, + ARRAY_SIZE(byt_rt5651_ssp2_aif2_map)); + } else if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) { + ret = snd_soc_dapm_add_routes(&card->dapm, + byt_rt5651_ssp0_aif1_map, + ARRAY_SIZE(byt_rt5651_ssp0_aif1_map)); + } else if (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2) { + ret = snd_soc_dapm_add_routes(&card->dapm, + byt_rt5651_ssp0_aif2_map, + ARRAY_SIZE(byt_rt5651_ssp0_aif2_map)); + } else { + ret = snd_soc_dapm_add_routes(&card->dapm, + byt_rt5651_ssp2_aif1_map, + ARRAY_SIZE(byt_rt5651_ssp2_aif1_map)); + } + if (ret) + return ret;
ret = snd_soc_add_card_controls(card, byt_rt5651_controls, ARRAY_SIZE(byt_rt5651_controls)); if (ret) { @@ -464,18 +534,26 @@ static int byt_rt5651_codec_fixup(struct snd_soc_pcm_runtime *rtd, SNDRV_PCM_HW_PARAM_RATE); struct snd_interval *channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); - int ret; + int ret, bits; - /* The DSP will covert the FE rate to 48k, stereo, 24bits */ + /* The DSP will covert the FE rate to 48k, stereo */ rate->min = rate->max = 48000; channels->min = channels->max = 2; - /* set SSP2 to 24-bit */ - params_set_format(params, SNDRV_PCM_FORMAT_S24_LE); + if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) || + (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) { + /* set SSP0 to 16-bit */ + params_set_format(params, SNDRV_PCM_FORMAT_S16_LE); + bits = 16; + } else { + /* set SSP2 to 24-bit */ + params_set_format(params, SNDRV_PCM_FORMAT_S24_LE); + bits = 24; + } /* * Default mode for SSP configuration is TDM 4 slot, override config - * with explicit setting to I2S 2ch 24-bit. The word length is set with + * with explicit setting to I2S 2ch. The word length is set with * dai_set_tdm_slot() since there is no other API exposed */ ret = snd_soc_dai_set_fmt(rtd->cpu_dai, @@ -489,7 +567,7 @@ static int byt_rt5651_codec_fixup(struct snd_soc_pcm_runtime *rtd, return ret; } - ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x3, 0x3, 2, 24); + ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x3, 0x3, 2, bits); if (ret < 0) { dev_err(rtd->dev, "can't set I2S config, err %d\n", ret); return ret; @@ -583,12 +661,32 @@ static struct snd_soc_card byt_rt5651_card = { }; static char byt_rt5651_codec_name[SND_ACPI_I2C_ID_LEN]; +static char byt_rt5651_codec_aif_name[12]; /* = "rt5651-aif[1|2]" */ +static char byt_rt5651_cpu_dai_name[10]; /* = "ssp[0|2]-port" */
+static bool is_valleyview(void) +{ + static const struct x86_cpu_id cpu_ids[] = { + { X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */ + {} + };
+ if (!x86_match_cpu(cpu_ids)) + return false; + return true; +}
+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 */ +}; static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) { struct byt_rt5651_private *priv; struct snd_soc_acpi_mach *mach; const char *i2c_name = NULL; + bool is_bytcr = false; int ret_val = 0; int dai_index = 0; int i; @@ -620,10 +718,99 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) byt_rt5651_dais[dai_index].codec_name = byt_rt5651_codec_name; } + /* + * swap SSP0 if bytcr is detected + * (will be overridden if DMI quirk is detected) + */ + if (is_valleyview()) { + struct sst_platform_info *p_info = mach->pdata; + const struct sst_res_info *res_info = p_info->res_info;
+ if (res_info->acpi_ipc_irq_index == 0) + is_bytcr = true; + }
+ if (is_bytcr) { + /* + * Baytrail CR platforms may have CHAN package in BIOS, try + * to find relevant routing quirk based as done on Windows + * platforms. We have to read the information directly from the + * BIOS, at this stage the card is not created and the links + * with the codec driver/pdata are non-existent + */
+ struct acpi_chan_package chan_package;
+ /* format specified: 2 64-bit integers */ + struct acpi_buffer format = {sizeof("NN"), "NN"}; + struct acpi_buffer state = {0, NULL}; + struct snd_soc_acpi_package_context pkg_ctx; + bool pkg_found = false;
+ state.length = sizeof(chan_package); + state.pointer = &chan_package;
+ pkg_ctx.name = "CHAN"; + pkg_ctx.length = 2; + pkg_ctx.format = &format; + pkg_ctx.state = &state; + pkg_ctx.data_valid = false;
+ pkg_found = snd_soc_acpi_find_package_from_hid(mach->id, + &pkg_ctx); + if (pkg_found) { + if (chan_package.aif_value == 1) { + dev_info(&pdev->dev, "BIOS Routing: AIF1 connected\n"); + byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF1; + } else if (chan_package.aif_value == 2) { + dev_info(&pdev->dev, "BIOS Routing: AIF2 connected\n"); + byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF2; + } else { + dev_info(&pdev->dev, "BIOS Routing isn't valid, ignored\n"); + pkg_found = false; + } + }
+ if (!pkg_found) { + /* no BIOS indications, assume SSP0-AIF2 connection */ + byt_rt5651_quirk |= BYT_RT5651_SSP0_AIF2; + }
+ /* change defaults for Baytrail-CR capture */ + byt_rt5651_quirk |= BYT_RT5651_JD1_1 | + BYT_RT5651_OVTH_2000UA | + BYT_RT5651_OVCD_SF_0P75 | + BYT_RT5651_IN2_HS_IN3_MAP; + } else { + byt_rt5651_quirk |= BYT_RT5651_DMIC_MAP; + }
/* check quirks before creating card */ dmi_check_system(byt_rt5651_quirk_table); log_quirks(&pdev->dev); + if ((byt_rt5651_quirk & BYT_RT5651_SSP2_AIF2) || + (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) { + /* fixup codec aif name */ + snprintf(byt_rt5651_codec_aif_name, + sizeof(byt_rt5651_codec_aif_name), + "%s", "rt5651-aif2");
+ byt_rt5651_dais[dai_index].codec_dai_name = + byt_rt5651_codec_aif_name; + }
+ if ((byt_rt5651_quirk & BYT_RT5651_SSP0_AIF1) || + (byt_rt5651_quirk & BYT_RT5651_SSP0_AIF2)) { + /* fixup cpu dai name name */ + snprintf(byt_rt5651_cpu_dai_name, + sizeof(byt_rt5651_cpu_dai_name), + "%s", "ssp0-port");
+ byt_rt5651_dais[dai_index].cpu_dai_name = + byt_rt5651_cpu_dai_name; + }
if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) { priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3"); if (IS_ERR(priv->mclk)) {
Add a quirk setting up jack-detect and input routing for the VIOS LTH17 laptop.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/soc/intel/boards/bytcr_rt5651.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index c14c5940ff87..c4260ca3cbed 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -398,6 +398,19 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = { BYT_RT5651_OVCD_SF_0P75 | BYT_RT5651_IN2_HS_IN3_MAP), }, + { + /* VIOS LTH17 */ + .callback = byt_rt5651_quirk_cb, + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "VIOS"), + DMI_MATCH(DMI_PRODUCT_NAME, "LTH17"), + }, + .driver_data = (void *)(BYT_RT5651_MCLK_EN | + BYT_RT5651_JD1_1 | + BYT_RT5651_OVTH_2000UA | + BYT_RT5651_OVCD_SF_1P0 | + BYT_RT5651_IN1_IN2_MAP), + }, {} };
On Tue, Feb 20, 2018 at 11:15:01PM +0100, Hans de Goede wrote:
Instead of duplicating the DMI quirks between the codec and machine driver, add a rt5651_set_pdata() codec private function which the machine driver can use to pass in pdata.
It's not clear to me that this is a great idea. The goal in general is to keep as much of the CODEC configuration in the CODEC driver as possible so we can move more towards generic machine drivers, this is moving us in the opposite drection quite dramatically. That might get in the way of improvements we could be making when we get the component based changes further on and can avoid having all the DPCM stuff in the Intel drivers.
It is really unfortunate that ACPI based systems aren't embracing standards here and using this mess of open coded quirks but it feels like the way to do this is to keep the code as close as possible to well designed firmware interfaces and hope that they come round.
Hi,
On 21-02-18 12:54, Mark Brown wrote:
On Tue, Feb 20, 2018 at 11:15:01PM +0100, Hans de Goede wrote:
Instead of duplicating the DMI quirks between the codec and machine driver, add a rt5651_set_pdata() codec private function which the machine driver can use to pass in pdata.
It's not clear to me that this is a great idea. The goal in general is to keep as much of the CODEC configuration in the CODEC driver as possible so we can move more towards generic machine drivers, this is moving us in the opposite drection quite dramatically.
Not really, instead of having Intel/ACPI specific quirks in the codec driver it moves everything over to pdata, so in a way this unifies the device-tree and ACPI paths.
Also to me it seems that the Intel specific machine driver is the logical place to put quirks for Intel platforms rather then poluting the platform-agnostic codec driver with this.
That might get in the way of improvements we could be making when we get the component based changes further on and can avoid having all the DPCM stuff in the Intel drivers.
It is really unfortunate that ACPI based systems aren't embracing standards here and using this mess of open coded quirks but it feels like the way to do this is to keep the code as close as possible to well designed firmware interfaces and hope that they come round.
Both devicetree and ACPI code-paths are filling pdata and that is the only thing the codec driver cares about after this patch. so this is using a well designed interface (but a non firmware one). I can understand if you would prefer to use the functions from include/linux/property.h for this, but those simply do not work for ACPI platforms atm.
If you look further in this series then quirks for at least 2 more models are added and I expect more to show up in the future, I really do not want to duplicate these over 2 files.
One possible solution here would be to add device-properties to the codec i2c-device from the machine-driver using device_add_properties() and make the codec driver consume those.
This has probe-ordering issues, but those can be worked around by exporting something like the rt5651_apply_pdata() function from this patch and make the machine driver call that after it has attached the properties.
I'm not sure if this extra level of indirection buys us much though, the DT binding maintainers will not accept binding docs for these kind of kernel internal only device-properties until there are actual DT users of them, so for now this would mainly add a bunch of extra code for little gain.
Regards,
Hans
Hi Mark,
On 21-02-18 20:23, Hans de Goede wrote:
Hi,
On 21-02-18 12:54, Mark Brown wrote:
On Tue, Feb 20, 2018 at 11:15:01PM +0100, Hans de Goede wrote:
Instead of duplicating the DMI quirks between the codec and machine driver, add a rt5651_set_pdata() codec private function which the machine driver can use to pass in pdata.
It's not clear to me that this is a great idea. The goal in general is to keep as much of the CODEC configuration in the CODEC driver as possible so we can move more towards generic machine drivers, this is moving us in the opposite drection quite dramatically.
Not really, instead of having Intel/ACPI specific quirks in the codec driver it moves everything over to pdata, so in a way this unifies the device-tree and ACPI paths.
Also to me it seems that the Intel specific machine driver is the logical place to put quirks for Intel platforms rather then poluting the platform-agnostic codec driver with this.
That might get in the way of improvements we could be making when we get the component based changes further on and can avoid having all the DPCM stuff in the Intel drivers.
It is really unfortunate that ACPI based systems aren't embracing standards here and using this mess of open coded quirks but it feels like the way to do this is to keep the code as close as possible to well designed firmware interfaces and hope that they come round.
Both devicetree and ACPI code-paths are filling pdata and that is the only thing the codec driver cares about after this patch. so this is using a well designed interface (but a non firmware one). I can understand if you would prefer to use the functions from include/linux/property.h for this, but those simply do not work for ACPI platforms atm.
If you look further in this series then quirks for at least 2 more models are added and I expect more to show up in the future, I really do not want to duplicate these over 2 files.
One possible solution here would be to add device-properties to the codec i2c-device from the machine-driver using device_add_properties() and make the codec driver consume those.
This has probe-ordering issues, but those can be worked around by exporting something like the rt5651_apply_pdata() function from this patch and make the machine driver call that after it has attached the properties.
I'm not sure if this extra level of indirection buys us much though, the DT binding maintainers will not accept binding docs for these kind of kernel internal only device-properties until there are actual DT users of them, so for now this would mainly add a bunch of extra code for little gain.
So thinking more about this I think that having the codec driver only check device-properties and completely killing platform_data is the best way forward / best way to clean this up.
Combined with either device-tree or the machine driver setting these device properties for now, and in the future the ACPI tables can set these properties directly and we can hopefully won't need to add new quirks to the machine driver.
I plan to start reworking this series this afternoon, so any feedback on the device-properties plan would be appreciated...
Regards,
Hans
On Thu, Feb 22, 2018 at 12:08:29PM +0100, Hans de Goede wrote:
So thinking more about this I think that having the codec driver only check device-properties and completely killing platform_data is the best way forward / best way to clean this up.
Combined with either device-tree or the machine driver setting these device properties for now, and in the future the ACPI tables can set these properties directly and we can hopefully won't need to add new quirks to the machine driver.
The device properties thing seems reasonable enough. I'm still not super convinced the machine driver is the right place, but let's see the code. I think ideally the arch code would have a way to put all the device property based quirks for these systems like we IIRC do for some DT fixups.
Hi,
On 22-02-18 12:35, Mark Brown wrote:
On Thu, Feb 22, 2018 at 12:08:29PM +0100, Hans de Goede wrote:
So thinking more about this I think that having the codec driver only check device-properties and completely killing platform_data is the best way forward / best way to clean this up.
Combined with either device-tree or the machine driver setting these device properties for now, and in the future the ACPI tables can set these properties directly and we can hopefully won't need to add new quirks to the machine driver.
The device properties thing seems reasonable enough. I'm still not super convinced the machine driver is the right place, but let's see the code. I think ideally the arch code would have a way to put all the device property based quirks for these systems like we IIRC do for some DT fixups.
We've already tried something like that for fixing a similar situation (not enough info in ACPI for the driver to function) for silead touchscreens:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drive...
Note that this device-property providing driver can only be builtin, it cannot be built as a module (otherwise we're back to having ordering issues). We (Dmitry and I) have gotten quite some flack about needlessly growing the bzImage size with data only used on a small subset of x86 devices and this construction is not something which I want to repeat (nor would advise others to repeat).
We really need to have a way to have these quirks table in a module which only loads on devices which need it. So in my mind the machine driver is a good place for this.
Regards,
Hans
On Wed, Feb 21, 2018 at 08:23:01PM +0100, Hans de Goede wrote:
On 21-02-18 12:54, Mark Brown wrote:
It's not clear to me that this is a great idea. The goal in general is to keep as much of the CODEC configuration in the CODEC driver as possible so we can move more towards generic machine drivers, this is moving us in the opposite drection quite dramatically.
Not really, instead of having Intel/ACPI specific quirks in the codec driver it moves everything over to pdata, so in a way this unifies the device-tree and ACPI paths.
Bear in mind that ACPI isn't x86 specific any more, and on x86 those Intel drivers really are Intel specific rather than x86 specific - AMD are doing their own completely different thing. Much ACPI, so standards, sadly :(
Also to me it seems that the Intel specific machine driver is the logical place to put quirks for Intel platforms rather then poluting the platform-agnostic codec driver with this.
It is really unfortunate that ACPI based systems aren't embracing standards here and using this mess of open coded quirks but it feels like the way to do this is to keep the code as close as possible to well designed firmware interfaces and hope that they come round.
Both devicetree and ACPI code-paths are filling pdata and that is the only thing the codec driver cares about after this patch. so this is using a well designed interface (but a non firmware one). I can understand
It's not just platform data, it's also putting the platform data in through a regular driver outside of the usual device model stuff.
if you would prefer to use the functions from include/linux/property.h for this, but those simply do not work for ACPI platforms atm.
Yeah, they work fine with DSD which is the official ACPI way to pass device specific data through.
One possible solution here would be to add device-properties to the codec i2c-device from the machine-driver using device_add_properties() and make the codec driver consume those.
This has probe-ordering issues, but those can be worked around by exporting something like the rt5651_apply_pdata() function from this patch and make the machine driver call that after it has attached the properties.
The ordering is part of the issue, yeah. It increases fragility if we have to wait to parse the platform data until later on in init, but only on some systems.
I'm not sure if this extra level of indirection buys us much though, the DT binding maintainers will not accept binding docs for these kind of kernel internal only device-properties until there are actual DT users of them, so for now this would mainly add a bunch of extra code for little gain.
I'm not aware of any such policy from the DT people, and in any case the individual driver bindings go through subsystems so...
On Tue, Feb 20, 2018 at 10:15 PM, Hans de Goede hdegoede@redhat.com wrote:
Instead of duplicating the DMI quirks between the codec and machine driver, add a rt5651_set_pdata() codec private function which the machine driver can use to pass in pdata.
Commenting here for the whole series (I don't see the cover letter):
Tested-by: Carlo Caione carlo@endlessm.com
(On the quirked KIANO laptop).
Thanks!
Hi,
On 21-02-18 18:36, Carlo Caione wrote:
On Tue, Feb 20, 2018 at 10:15 PM, Hans de Goede hdegoede@redhat.com wrote:
Instead of duplicating the DMI quirks between the codec and machine driver, add a rt5651_set_pdata() codec private function which the machine driver can use to pass in pdata.
Commenting here for the whole series (I don't see the cover letter):
Tested-by: Carlo Caione carlo@endlessm.com
(On the quirked KIANO laptop).
Great, thank you for testing, so are the issues with headset vs headphone detection not working fixed after this series?
Regards,
Hans
participants (4)
-
Carlo Caione -
Hans de Goede -
Mark Brown -
Pierre-Louis Bossart