[alsa-devel] [PATCH v1 0/9] Basic sound support for Arndale board / wm8994 updates
This patch series adds basic audio support for Exynos5250 SoC based Arndale board, the Bluetooth receiver source and HDMI output are not covered yet.
There is also one fix for wm8994 driver related to WM1811 CODEC and wm8994 updates to handle MCLK clocks, similar to patches: ae1ea48c5c59 ("ASoC: arizona: Add gating for source clocks of the FLLs") 7a4413d0dc96 ("ASoC: arizona: Add gating for clock when used for direct MCLK")
Sylwester Nawrocki (9): ASoC: wm8994: Do not register inapplicable controls for WM1811 mfd: wm8994: Add support for MCLKn clock control ASoC: wm8994: Add support for setting MCLKn clock rate ASoC: wm8994: Add support for MCLKn clock gating ASoC: samsung: arndale: Simplify DAI link initialization ASoC: dt-bindings: Document "samsung,arndale-wm1811" compatible ASoC: samsung: arndale: Add support for WM1811 CODEC ASoC: samsung: arndale: Add missing OF node dereferencing ARM: dts: arndale: Add audio support (WM1811 CODEC boards)
.../devicetree/bindings/sound/arndale.txt | 5 +- arch/arm/boot/dts/exynos5250-arndale.dts | 27 ++- drivers/mfd/wm8994-core.c | 9 + include/linux/mfd/wm8994/core.h | 9 + sound/soc/codecs/wm8994.c | 164 +++++++++++++++--- sound/soc/samsung/arndale_rt5631.c | 155 +++++++++++++---- 6 files changed, 306 insertions(+), 63 deletions(-)
In case of WM1811 device there are currently being registered controls referring to registers not existing on that device. It has been noticed when getting values of "AIF1ADC2 Volume", "AIF1DAC2 Volume" controls was failing during ALSA state restoring at boot time: "amixer: Mixer hw:0 load error: Device or resource busy"
Reading some registers through I2C was failing with EBUSY error and indeed those registers were not available according to the datasheet.
To fix this controls not available on WM1811 are moved to a separate array and registered only for WM8994 and WM8958.
There are some further differences between WM8994 and WM1811, e.g. registers 603h, 604h, 605h, which are not covered in this patch.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com --- sound/soc/codecs/wm8994.c | 43 +++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c index c3d06e8bc54f..d5fb7f5dd551 100644 --- a/sound/soc/codecs/wm8994.c +++ b/sound/soc/codecs/wm8994.c @@ -533,13 +533,10 @@ static SOC_ENUM_SINGLE_DECL(dac_osr, static SOC_ENUM_SINGLE_DECL(adc_osr, WM8994_OVERSAMPLING, 1, osr_text);
-static const struct snd_kcontrol_new wm8994_snd_controls[] = { +static const struct snd_kcontrol_new wm8994_common_snd_controls[] = { SOC_DOUBLE_R_TLV("AIF1ADC1 Volume", WM8994_AIF1_ADC1_LEFT_VOLUME, WM8994_AIF1_ADC1_RIGHT_VOLUME, 1, 119, 0, digital_tlv), -SOC_DOUBLE_R_TLV("AIF1ADC2 Volume", WM8994_AIF1_ADC2_LEFT_VOLUME, - WM8994_AIF1_ADC2_RIGHT_VOLUME, - 1, 119, 0, digital_tlv), SOC_DOUBLE_R_TLV("AIF2ADC Volume", WM8994_AIF2_ADC_LEFT_VOLUME, WM8994_AIF2_ADC_RIGHT_VOLUME, 1, 119, 0, digital_tlv), @@ -556,8 +553,6 @@ SOC_ENUM("AIF2DACR Source", aif2dacr_src),
SOC_DOUBLE_R_TLV("AIF1DAC1 Volume", WM8994_AIF1_DAC1_LEFT_VOLUME, WM8994_AIF1_DAC1_RIGHT_VOLUME, 1, 96, 0, digital_tlv), -SOC_DOUBLE_R_TLV("AIF1DAC2 Volume", WM8994_AIF1_DAC2_LEFT_VOLUME, - WM8994_AIF1_DAC2_RIGHT_VOLUME, 1, 96, 0, digital_tlv), SOC_DOUBLE_R_TLV("AIF2DAC Volume", WM8994_AIF2_DAC_LEFT_VOLUME, WM8994_AIF2_DAC_RIGHT_VOLUME, 1, 96, 0, digital_tlv),
@@ -565,17 +560,12 @@ SOC_SINGLE_TLV("AIF1 Boost Volume", WM8994_AIF1_CONTROL_2, 10, 3, 0, aif_tlv), SOC_SINGLE_TLV("AIF2 Boost Volume", WM8994_AIF2_CONTROL_2, 10, 3, 0, aif_tlv),
SOC_SINGLE("AIF1DAC1 EQ Switch", WM8994_AIF1_DAC1_EQ_GAINS_1, 0, 1, 0), -SOC_SINGLE("AIF1DAC2 EQ Switch", WM8994_AIF1_DAC2_EQ_GAINS_1, 0, 1, 0), SOC_SINGLE("AIF2 EQ Switch", WM8994_AIF2_EQ_GAINS_1, 0, 1, 0),
WM8994_DRC_SWITCH("AIF1DAC1 DRC Switch", WM8994_AIF1_DRC1_1, 2), WM8994_DRC_SWITCH("AIF1ADC1L DRC Switch", WM8994_AIF1_DRC1_1, 1), WM8994_DRC_SWITCH("AIF1ADC1R DRC Switch", WM8994_AIF1_DRC1_1, 0),
-WM8994_DRC_SWITCH("AIF1DAC2 DRC Switch", WM8994_AIF1_DRC2_1, 2), -WM8994_DRC_SWITCH("AIF1ADC2L DRC Switch", WM8994_AIF1_DRC2_1, 1), -WM8994_DRC_SWITCH("AIF1ADC2R DRC Switch", WM8994_AIF1_DRC2_1, 0), - WM8994_DRC_SWITCH("AIF2DAC DRC Switch", WM8994_AIF2_DRC_1, 2), WM8994_DRC_SWITCH("AIF2ADCL DRC Switch", WM8994_AIF2_DRC_1, 1), WM8994_DRC_SWITCH("AIF2ADCR DRC Switch", WM8994_AIF2_DRC_1, 0), @@ -594,9 +584,6 @@ SOC_SINGLE("Sidetone HPF Switch", WM8994_SIDETONE, 6, 1, 0), SOC_ENUM("AIF1ADC1 HPF Mode", aif1adc1_hpf), SOC_DOUBLE("AIF1ADC1 HPF Switch", WM8994_AIF1_ADC1_FILTERS, 12, 11, 1, 0),
-SOC_ENUM("AIF1ADC2 HPF Mode", aif1adc2_hpf), -SOC_DOUBLE("AIF1ADC2 HPF Switch", WM8994_AIF1_ADC2_FILTERS, 12, 11, 1, 0), - SOC_ENUM("AIF2ADC HPF Mode", aif2adc_hpf), SOC_DOUBLE("AIF2ADC HPF Switch", WM8994_AIF2_ADC_FILTERS, 12, 11, 1, 0),
@@ -637,6 +624,24 @@ SOC_SINGLE("AIF2DAC 3D Stereo Switch", WM8994_AIF2_DAC_FILTERS_2, 8, 1, 0), };
+/* Controls not available on WM1811 */ +static const struct snd_kcontrol_new wm8994_snd_controls[] = { +SOC_DOUBLE_R_TLV("AIF1ADC2 Volume", WM8994_AIF1_ADC2_LEFT_VOLUME, + WM8994_AIF1_ADC2_RIGHT_VOLUME, + 1, 119, 0, digital_tlv), +SOC_DOUBLE_R_TLV("AIF1DAC2 Volume", WM8994_AIF1_DAC2_LEFT_VOLUME, + WM8994_AIF1_DAC2_RIGHT_VOLUME, 1, 96, 0, digital_tlv), + +SOC_SINGLE("AIF1DAC2 EQ Switch", WM8994_AIF1_DAC2_EQ_GAINS_1, 0, 1, 0), + +WM8994_DRC_SWITCH("AIF1DAC2 DRC Switch", WM8994_AIF1_DRC2_1, 2), +WM8994_DRC_SWITCH("AIF1ADC2L DRC Switch", WM8994_AIF1_DRC2_1, 1), +WM8994_DRC_SWITCH("AIF1ADC2R DRC Switch", WM8994_AIF1_DRC2_1, 0), + +SOC_ENUM("AIF1ADC2 HPF Mode", aif1adc2_hpf), +SOC_DOUBLE("AIF1ADC2 HPF Switch", WM8994_AIF1_ADC2_FILTERS, 12, 11, 1, 0), +}; + static const struct snd_kcontrol_new wm8994_eq_controls[] = { SOC_SINGLE_TLV("AIF1DAC1 EQ1 Volume", WM8994_AIF1_DAC1_EQ_GAINS_1, 11, 31, 0, eq_tlv), @@ -4258,13 +4263,15 @@ static int wm8994_component_probe(struct snd_soc_component *component) wm8994_handle_pdata(wm8994);
wm_hubs_add_analogue_controls(component); - snd_soc_add_component_controls(component, wm8994_snd_controls, - ARRAY_SIZE(wm8994_snd_controls)); + snd_soc_add_component_controls(component, wm8994_common_snd_controls, + ARRAY_SIZE(wm8994_common_snd_controls)); snd_soc_dapm_new_controls(dapm, wm8994_dapm_widgets, ARRAY_SIZE(wm8994_dapm_widgets));
switch (control->type) { case WM8994: + snd_soc_add_component_controls(component, wm8994_snd_controls, + ARRAY_SIZE(wm8994_snd_controls)); snd_soc_dapm_new_controls(dapm, wm8994_specific_dapm_widgets, ARRAY_SIZE(wm8994_specific_dapm_widgets)); if (control->revision < 4) { @@ -4284,8 +4291,10 @@ static int wm8994_component_probe(struct snd_soc_component *component) } break; case WM8958: + snd_soc_add_component_controls(component, wm8994_snd_controls, + ARRAY_SIZE(wm8994_snd_controls)); snd_soc_add_component_controls(component, wm8958_snd_controls, - ARRAY_SIZE(wm8958_snd_controls)); + ARRAY_SIZE(wm8958_snd_controls)); snd_soc_dapm_new_controls(dapm, wm8958_dapm_widgets, ARRAY_SIZE(wm8958_dapm_widgets)); if (control->revision < 1) {
On Wed, Sep 18, 2019 at 12:46:26PM +0200, Sylwester Nawrocki wrote:
In case of WM1811 device there are currently being registered controls referring to registers not existing on that device. It has been noticed when getting values of "AIF1ADC2 Volume", "AIF1DAC2 Volume" controls was failing during ALSA state restoring at boot time: "amixer: Mixer hw:0 load error: Device or resource busy"
Reading some registers through I2C was failing with EBUSY error and indeed those registers were not available according to the datasheet.
To fix this controls not available on WM1811 are moved to a separate array and registered only for WM8994 and WM8958.
There are some further differences between WM8994 and WM1811, e.g. registers 603h, 604h, 605h, which are not covered in this patch.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
Acked-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
On Wed, Sep 18, 2019 at 12:46:26PM +0200, Sylwester Nawrocki wrote:
In case of WM1811 device there are currently being registered controls referring to registers not existing on that device. It has been noticed when getting values of "AIF1ADC2 Volume", "AIF1DAC2 Volume" controls was failing during ALSA state restoring at boot time: "amixer: Mixer hw:0 load error: Device or resource busy"
Reading some registers through I2C was failing with EBUSY error and indeed those registers were not available according to the datasheet.
To fix this controls not available on WM1811 are moved to a separate array and registered only for WM8994 and WM8958.
There are some further differences between WM8994 and WM1811, e.g. registers 603h, 604h, 605h, which are not covered in this patch.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
sound/soc/codecs/wm8994.c | 43 +++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 17 deletions(-)
Acked-by: Krzysztof Kozlowski krzk@kernel.org
Best regards, Krzysztof
The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD part of the wm8994 driver so they can be further handled in the audio CODEC part.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com --- drivers/mfd/wm8994-core.c | 9 +++++++++ include/linux/mfd/wm8994/core.h | 9 +++++++++ 2 files changed, 18 insertions(+)
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 1e9fe7d92597..02c19a0bdeb0 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -7,6 +7,7 @@ * Author: Mark Brown broonie@opensource.wolfsonmicro.com */
+#include <linux/clk.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/slab.h> @@ -333,6 +334,14 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq)
dev_set_drvdata(wm8994->dev, wm8994);
+ wm8994->mclk[WM8994_MCLK1].id = "MCLK1"; + wm8994->mclk[WM8994_MCLK2].id = "MCLK2"; + + ret = devm_clk_bulk_get_optional(wm8994->dev, ARRAY_SIZE(wm8994->mclk), + wm8994->mclk); + if (ret != 0) + return ret; + /* Add the on-chip regulators first for bootstrapping */ ret = mfd_add_devices(wm8994->dev, 0, wm8994_regulator_devs, diff --git a/include/linux/mfd/wm8994/core.h b/include/linux/mfd/wm8994/core.h index e8b093522ffd..320297a1b70c 100644 --- a/include/linux/mfd/wm8994/core.h +++ b/include/linux/mfd/wm8994/core.h @@ -10,12 +10,19 @@ #ifndef __MFD_WM8994_CORE_H__ #define __MFD_WM8994_CORE_H__
+#include <linux/clk.h> #include <linux/mutex.h> #include <linux/interrupt.h> #include <linux/regmap.h>
#include <linux/mfd/wm8994/pdata.h>
+enum { + WM8994_MCLK1, + WM8994_MCLK2, + WM8994_NUM_MCLK +}; + enum wm8994_type { WM8994 = 0, WM8958 = 1, @@ -60,6 +67,8 @@ struct wm8994 { struct device *dev; struct regmap *regmap;
+ struct clk_bulk_data mclk[WM8994_NUM_MCLK]; + bool ldo_ena_always_driven;
int gpio_base;
Cc: lee.jones@linaro.org
Excuse me Lee, I forgot to Cc entire series to you, will do it in case of posting v2.
On 9/18/19 12:46, Sylwester Nawrocki wrote:
The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD part of the wm8994 driver so they can be further handled in the audio CODEC part.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
drivers/mfd/wm8994-core.c | 9 +++++++++ include/linux/mfd/wm8994/core.h | 9 +++++++++ 2 files changed, 18 insertions(+)
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 1e9fe7d92597..02c19a0bdeb0 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -7,6 +7,7 @@
- Author: Mark Brown broonie@opensource.wolfsonmicro.com
*/
+#include <linux/clk.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/slab.h> @@ -333,6 +334,14 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq)
dev_set_drvdata(wm8994->dev, wm8994);
- wm8994->mclk[WM8994_MCLK1].id = "MCLK1";
- wm8994->mclk[WM8994_MCLK2].id = "MCLK2";
- ret = devm_clk_bulk_get_optional(wm8994->dev, ARRAY_SIZE(wm8994->mclk),
wm8994->mclk);- if (ret != 0)
return ret;- /* Add the on-chip regulators first for bootstrapping */ ret = mfd_add_devices(wm8994->dev, 0, wm8994_regulator_devs,
diff --git a/include/linux/mfd/wm8994/core.h b/include/linux/mfd/wm8994/core.h index e8b093522ffd..320297a1b70c 100644 --- a/include/linux/mfd/wm8994/core.h +++ b/include/linux/mfd/wm8994/core.h @@ -10,12 +10,19 @@ #ifndef __MFD_WM8994_CORE_H__ #define __MFD_WM8994_CORE_H__
+#include <linux/clk.h> #include <linux/mutex.h> #include <linux/interrupt.h> #include <linux/regmap.h>
#include <linux/mfd/wm8994/pdata.h>
+enum {
- WM8994_MCLK1,
- WM8994_MCLK2,
- WM8994_NUM_MCLK
+};
enum wm8994_type { WM8994 = 0, WM8958 = 1, @@ -60,6 +67,8 @@ struct wm8994 { struct device *dev; struct regmap *regmap;
struct clk_bulk_data mclk[WM8994_NUM_MCLK];
bool ldo_ena_always_driven;
int gpio_base;
-- Regards, Sylwester
On Wed, Sep 18, 2019 at 12:59:28PM +0200, Sylwester Nawrocki wrote:
On 9/18/19 12:46, Sylwester Nawrocki wrote:
The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD part of the wm8994 driver so they can be further handled in the audio CODEC part.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
- wm8994->mclk[WM8994_MCLK1].id = "MCLK1";
- wm8994->mclk[WM8994_MCLK2].id = "MCLK2";
- ret = devm_clk_bulk_get_optional(wm8994->dev, ARRAY_SIZE(wm8994->mclk),
wm8994->mclk);- if (ret != 0)
return ret;
Would be nice to print a message here as well, make it clear what failed in the log. Apart from that minor nit:
Acked-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
On 9/18/19 14:54, Charles Keepax wrote:
- ret = devm_clk_bulk_get_optional(wm8994->dev, ARRAY_SIZE(wm8994->mclk),
wm8994->mclk);- if (ret != 0)
return ret;Would be nice to print a message here as well, make it clear what failed in the log. Apart from that minor nit:
Thanks for reviewing, I will add that modification for v2.
On Wed, Sep 18, 2019 at 12:46:27PM +0200, Sylwester Nawrocki wrote:
The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD part of the wm8994 driver so they can be further handled in the audio CODEC part.
I think these are needed only for the codec so how about getting them in codec's probe?
Best regards, Krzysztof
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
drivers/mfd/wm8994-core.c | 9 +++++++++ include/linux/mfd/wm8994/core.h | 9 +++++++++ 2 files changed, 18 insertions(+)
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 1e9fe7d92597..02c19a0bdeb0 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -7,6 +7,7 @@
- Author: Mark Brown broonie@opensource.wolfsonmicro.com
*/
+#include <linux/clk.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/slab.h> @@ -333,6 +334,14 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq)
dev_set_drvdata(wm8994->dev, wm8994);
- wm8994->mclk[WM8994_MCLK1].id = "MCLK1";
- wm8994->mclk[WM8994_MCLK2].id = "MCLK2";
- ret = devm_clk_bulk_get_optional(wm8994->dev, ARRAY_SIZE(wm8994->mclk),
wm8994->mclk);- if (ret != 0)
return ret;- /* Add the on-chip regulators first for bootstrapping */ ret = mfd_add_devices(wm8994->dev, 0, wm8994_regulator_devs,
diff --git a/include/linux/mfd/wm8994/core.h b/include/linux/mfd/wm8994/core.h index e8b093522ffd..320297a1b70c 100644 --- a/include/linux/mfd/wm8994/core.h +++ b/include/linux/mfd/wm8994/core.h @@ -10,12 +10,19 @@ #ifndef __MFD_WM8994_CORE_H__ #define __MFD_WM8994_CORE_H__
+#include <linux/clk.h> #include <linux/mutex.h> #include <linux/interrupt.h> #include <linux/regmap.h>
#include <linux/mfd/wm8994/pdata.h>
+enum {
- WM8994_MCLK1,
- WM8994_MCLK2,
- WM8994_NUM_MCLK
+};
enum wm8994_type { WM8994 = 0, WM8958 = 1, @@ -60,6 +67,8 @@ struct wm8994 { struct device *dev; struct regmap *regmap;
struct clk_bulk_data mclk[WM8994_NUM_MCLK];
bool ldo_ena_always_driven;
int gpio_base;
-- 2.17.1
On 9/19/19 09:59, Krzysztof Kozlowski wrote:
On Wed, Sep 18, 2019 at 12:46:27PM +0200, Sylwester Nawrocki wrote:
The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD part of the wm8994 driver so they can be further handled in the audio CODEC part.
I think these are needed only for the codec so how about getting them in codec's probe?
The clocks are specified in the (I2C slave device) DT node which corresponds to the device as a whole and to which binds the MFD driver. AFAICT those clocks are only needed by the CODEC sub-driver but in general such clocks could be used to provide device's system clock used by other functionalities than audio. We are doing something similar for other MFD drivers already and I would rather stick to one pattern. IMHO it's clearer to see what device the clocks belong to that way.
On Thu, 19 Sep 2019 at 14:49, Sylwester Nawrocki s.nawrocki@samsung.com wrote:
On 9/19/19 09:59, Krzysztof Kozlowski wrote:
On Wed, Sep 18, 2019 at 12:46:27PM +0200, Sylwester Nawrocki wrote:
The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD part of the wm8994 driver so they can be further handled in the audio CODEC part.
I think these are needed only for the codec so how about getting them in codec's probe?
The clocks are specified in the (I2C slave device) DT node which corresponds to the device as a whole and to which binds the MFD driver. AFAICT those clocks are only needed by the CODEC sub-driver but in general such clocks could be used to provide device's system clock used by other functionalities than audio. We are doing something similar for other MFD drivers already and I would rather stick to one pattern. IMHO it's clearer to see what device the clocks belong to that way.
The existing pattern is not an excuse for doing some things in a uglier way... If we agree that these are codec-only resources, then they should be acquired by codec driver. This is one minor step to solve difficult inter-device dependencies which stops from reusing parts of the code (some child of MFD heavily depends on parent). Existing MFD drivers sometimes follow this pattern but that's wrong. They follow the ugly or even crappy pattern.
Your codec driver still refers to parent and it has its resources, e.g. parent's device node pointer.
Best regards, Krzysztof
On Thu, Sep 19, 2019 at 09:59:24AM +0200, Krzysztof Kozlowski wrote:
On Wed, Sep 18, 2019 at 12:46:27PM +0200, Sylwester Nawrocki wrote:
The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD part of the wm8994 driver so they can be further handled in the audio CODEC part.
I think these are needed only for the codec so how about getting them in codec's probe?
Yeah. IIRC when these were added a machine driver was grabbing them. I don't think that machine driver ever made it's way upstream though.
On Thu, Sep 19, 2019 at 01:50:20PM +0100, Mark Brown wrote:
On Thu, Sep 19, 2019 at 09:59:24AM +0200, Krzysztof Kozlowski wrote:
On Wed, Sep 18, 2019 at 12:46:27PM +0200, Sylwester Nawrocki wrote:
The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD part of the wm8994 driver so they can be further handled in the audio CODEC part.
I think these are needed only for the codec so how about getting them in codec's probe?
Yeah. IIRC when these were added a machine driver was grabbing them. I don't think that machine driver ever made it's way upstream though.
If you mean for the Arizona stuff, the machine driver using that was sound/soc/samsung/tm2_wm5110.c. Sylwester upstreamed it along with the patches.
I think on wm8994 the clocks probably are only needed by the audio part of the driver, so probably can be moved in there, although as a disclaimer I have done a lot less with parts of that era. However on Arizona the clocking is needed from various parts of the driver so couldn't be moved exclusively to the codec driver.
Thanks, Charles
On Thu, Sep 19, 2019 at 02:31:16PM +0000, Charles Keepax wrote:
On Thu, Sep 19, 2019 at 01:50:20PM +0100, Mark Brown wrote:
Yeah. IIRC when these were added a machine driver was grabbing them. I don't think that machine driver ever made it's way upstream though.
If you mean for the Arizona stuff, the machine driver using that was sound/soc/samsung/tm2_wm5110.c. Sylwester upstreamed it along with the patches.
No, there was a WM8994 thing before that.
I think on wm8994 the clocks probably are only needed by the audio part of the driver, so probably can be moved in there, although as a disclaimer I have done a lot less with parts of that era. However on Arizona the clocking is needed from various parts of the driver so couldn't be moved exclusively to the codec driver.
Yes, they're only needed by the audio part of the driver.
Extend the set_sysclk() handler so we also set frequency of the MCLK1, MCLK2 clocks through clk API when those clocks are specified in DT for the device.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com --- sound/soc/codecs/wm8994.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c index d5fb7f5dd551..b6b0842ae1fc 100644 --- a/sound/soc/codecs/wm8994.c +++ b/sound/soc/codecs/wm8994.c @@ -2372,12 +2372,30 @@ static int wm8994_set_fll(struct snd_soc_dai *dai, int id, int src, return _wm8994_set_fll(dai->component, id, src, freq_in, freq_out); }
+static int wm8994_set_mclk_rate(struct wm8994_priv *wm8994, unsigned int id, + unsigned int *freq) +{ + struct wm8994 *control = wm8994->wm8994; + int ret; + + if (!control->mclk[id].clk || *freq == wm8994->mclk[id]) + return 0; + + ret = clk_set_rate(control->mclk[id].clk, *freq); + if (ret < 0) + return ret; + + *freq = clk_get_rate(control->mclk[id].clk); + + return 0; +} + static int wm8994_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir) { struct snd_soc_component *component = dai->component; struct wm8994_priv *wm8994 = snd_soc_component_get_drvdata(component); - int i; + int ret, i;
switch (dai->id) { case 1: @@ -2392,6 +2410,11 @@ static int wm8994_set_dai_sysclk(struct snd_soc_dai *dai, switch (clk_id) { case WM8994_SYSCLK_MCLK1: wm8994->sysclk[dai->id - 1] = WM8994_SYSCLK_MCLK1; + + ret = wm8994_set_mclk_rate(wm8994, dai->id - 1, &freq); + if (ret < 0) + return ret; + wm8994->mclk[0] = freq; dev_dbg(dai->dev, "AIF%d using MCLK1 at %uHz\n", dai->id, freq); @@ -2400,6 +2423,11 @@ static int wm8994_set_dai_sysclk(struct snd_soc_dai *dai, case WM8994_SYSCLK_MCLK2: /* TODO: Set GPIO AF */ wm8994->sysclk[dai->id - 1] = WM8994_SYSCLK_MCLK2; + + ret = wm8994_set_mclk_rate(wm8994, dai->id - 1, &freq); + if (ret < 0) + return ret; + wm8994->mclk[1] = freq; dev_dbg(dai->dev, "AIF%d using MCLK2 at %uHz\n", dai->id, freq);
On Wed, Sep 18, 2019 at 12:46:28PM +0200, Sylwester Nawrocki wrote:
Extend the set_sysclk() handler so we also set frequency of the MCLK1, MCLK2 clocks through clk API when those clocks are specified in DT for the device.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
Acked-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
On Wed, Sep 18, 2019 at 12:46:28PM +0200, Sylwester Nawrocki wrote:
Extend the set_sysclk() handler so we also set frequency of the MCLK1, MCLK2 clocks through clk API when those clocks are specified in DT for the device.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
sound/soc/codecs/wm8994.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
Acked-by: Krzysztof Kozlowski krzk@kernel.org
Best regards, Krzysztof
As an intermediate step before covering the clocking subsystem of the CODEC entirely by the clk API add handling of external CODEC's master clocks in DAPM events when the AIFn clocks are sourced directly from MCLKn; when FLLn are used we enable/disable respective MCLKn before/after FLLn is enabled/disabled.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com --- sound/soc/codecs/wm8994.c | 91 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 87 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c index b6b0842ae1fc..bf02e8908d5a 100644 --- a/sound/soc/codecs/wm8994.c +++ b/sound/soc/codecs/wm8994.c @@ -1038,6 +1038,46 @@ static bool wm8994_check_class_w_digital(struct snd_soc_component *component) return true; }
+static int aif_mclk_set(struct snd_soc_component *component, int aif, bool enable) +{ + struct wm8994_priv *wm8994 = snd_soc_component_get_drvdata(component); + struct wm8994 *control = wm8994->wm8994; + unsigned int offset, val, clk_idx; + int ret; + + if (aif) + offset = 4; + else + offset = 0; + + val = snd_soc_component_read32(component, WM8994_AIF1_CLOCKING_1 + offset); + val &= WM8994_AIF1CLK_SRC_MASK; + + switch (val) { + case 0: + clk_idx = WM8994_MCLK1; + break; + case 1: + clk_idx = WM8994_MCLK2; + break; + default: + return 0; + } + + if (enable) { + ret = clk_prepare_enable(control->mclk[clk_idx].clk); + if (ret < 0) { + dev_err(component->dev, "Failed to enable MCLK%d\n", + clk_idx); + return ret; + } + } else { + clk_disable_unprepare(control->mclk[clk_idx].clk); + } + + return 0; +} + static int aif1clk_ev(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { @@ -1045,7 +1085,7 @@ static int aif1clk_ev(struct snd_soc_dapm_widget *w, struct wm8994_priv *wm8994 = snd_soc_component_get_drvdata(component); struct wm8994 *control = wm8994->wm8994; int mask = WM8994_AIF1DAC1L_ENA | WM8994_AIF1DAC1R_ENA; - int i; + int ret, i; int dac; int adc; int val; @@ -1061,6 +1101,10 @@ static int aif1clk_ev(struct snd_soc_dapm_widget *w,
switch (event) { case SND_SOC_DAPM_PRE_PMU: + ret = aif_mclk_set(component, 0, true); + if (ret < 0) + return ret; + /* Don't enable timeslot 2 if not in use */ if (wm8994->channels[0] <= 2) mask &= ~(WM8994_AIF1DAC2L_ENA | WM8994_AIF1DAC2R_ENA); @@ -1133,6 +1177,12 @@ static int aif1clk_ev(struct snd_soc_dapm_widget *w, break; }
+ switch (event) { + case SND_SOC_DAPM_POST_PMD: + aif_mclk_set(component, 0, false); + break; + } + return 0; }
@@ -1140,13 +1190,17 @@ static int aif2clk_ev(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm); - int i; + int ret, i; int dac; int adc; int val;
switch (event) { case SND_SOC_DAPM_PRE_PMU: + ret = aif_mclk_set(component, 1, true); + if (ret < 0) + return ret; + val = snd_soc_component_read32(component, WM8994_AIF2_CONTROL_1); if ((val & WM8994_AIF2ADCL_SRC) && (val & WM8994_AIF2ADCR_SRC)) @@ -1218,6 +1272,12 @@ static int aif2clk_ev(struct snd_soc_dapm_widget *w, break; }
+ switch (event) { + case SND_SOC_DAPM_POST_PMD: + aif_mclk_set(component, 1, false); + break; + } + return 0; }
@@ -1623,10 +1683,10 @@ SND_SOC_DAPM_POST("Late Disable PGA", late_disable_ev) static const struct snd_soc_dapm_widget wm8994_lateclk_widgets[] = { SND_SOC_DAPM_SUPPLY("AIF1CLK", WM8994_AIF1_CLOCKING_1, 0, 0, aif1clk_ev, SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_PRE_PMD), + SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_SUPPLY("AIF2CLK", WM8994_AIF2_CLOCKING_1, 0, 0, aif2clk_ev, SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU | - SND_SOC_DAPM_PRE_PMD), + SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_PGA("Direct Voice", SND_SOC_NOPM, 0, 0, NULL, 0), SND_SOC_DAPM_MIXER("SPKL", WM8994_POWER_MANAGEMENT_3, 8, 0, left_speaker_mixer, ARRAY_SIZE(left_speaker_mixer)), @@ -2141,6 +2201,7 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src, u16 reg, clk1, aif_reg, aif_src; unsigned long timeout; bool was_enabled; + struct clk *mclk;
switch (id) { case WM8994_FLL1: @@ -2260,8 +2321,28 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src, /* Clear any pending completion from a previous failure */ try_wait_for_completion(&wm8994->fll_locked[id]);
+ switch (src) { + case WM8994_FLL_SRC_MCLK1: + mclk = control->mclk[0].clk; + break; + case WM8994_FLL_SRC_MCLK2: + mclk = control->mclk[1].clk; + break; + default: + mclk = NULL; + } + /* Enable (with fractional mode if required) */ if (freq_out) { + if (mclk) { + ret = clk_prepare_enable(mclk); + if (ret < 0) { + dev_err(component->dev, + "Failed to enable MCLK for FLL%d\n", + id + 1); + return ret; + } + } /* Enable VMID if we need it */ if (!was_enabled) { active_reference(component); @@ -2315,6 +2396,8 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
active_dereference(component); } + if (mclk) + clk_disable_unprepare(mclk); }
out:
On Wed, Sep 18, 2019 at 12:46:29PM +0200, Sylwester Nawrocki wrote:
As an intermediate step before covering the clocking subsystem of the CODEC entirely by the clk API add handling of external CODEC's master clocks in DAPM events when the AIFn clocks are sourced directly from MCLKn; when FLLn are used we enable/disable respective MCLKn before/after FLLn is enabled/disabled.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
@@ -2260,8 +2321,28 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src, /* Clear any pending completion from a previous failure */ try_wait_for_completion(&wm8994->fll_locked[id]);
- switch (src) {
- case WM8994_FLL_SRC_MCLK1:
mclk = control->mclk[0].clk;break;- case WM8994_FLL_SRC_MCLK2:
mclk = control->mclk[1].clk;break;- default:
mclk = NULL;- }
- /* Enable (with fractional mode if required) */ if (freq_out) {
if (mclk) {ret = clk_prepare_enable(mclk);if (ret < 0) {dev_err(component->dev,"Failed to enable MCLK for FLL%d\n",id + 1);return ret;}}@@ -2315,6 +2396,8 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
active_dereference(component); }
if (mclk)clk_disable_unprepare(mclk);
I don't think this works in the case of changing active FLLs. The driver looks like it allows changing the FLL configuration whilst the FLL is already active in which case it you would have two wm8994_set_fll calls enabling the FLL but only a single one disabling it. Resulting in the FLL being off but the MCLK being left enabled.
Thanks, Charles
On 9/18/19 16:31, Charles Keepax wrote:
@@ -2315,6 +2396,8 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
active_dereference(component); }
if (mclk)clk_disable_unprepare(mclk);I don't think this works in the case of changing active FLLs. The driver looks like it allows changing the FLL configuration whilst the FLL is already active in which case it you would have two wm8994_set_fll calls enabling the FLL but only a single one disabling it. Resulting in the FLL being off but the MCLK being left enabled.
Indeed I missed this scenario, or rather assumed it won't be used. But since the driver allows reconfiguring active FLLs we should make sure such use case remains properly supported.
What I came up so far as a fix is reading current FLL refclk source and if FLL was enabled with that source disabling refclk, before we change FLL configuration to new one. So we have clk_disable_unprepare(MCLK) more closely following FLL enable bit changes. I have tested it and it seems to work - something like below. Do you think it makes sense?
On Thu, Sep 19, 2019 at 01:58:35PM +0200, Sylwester Nawrocki wrote:
On 9/18/19 16:31, Charles Keepax wrote:
@@ -2315,6 +2396,8 @@ static int _wm8994_set_fll(struct snd_soc_component *component, int id, int src,
active_dereference(component); }
if (mclk)clk_disable_unprepare(mclk);I don't think this works in the case of changing active FLLs. The driver looks like it allows changing the FLL configuration whilst the FLL is already active in which case it you would have two wm8994_set_fll calls enabling the FLL but only a single one disabling it. Resulting in the FLL being off but the MCLK being left enabled.
Indeed I missed this scenario, or rather assumed it won't be used. But since the driver allows reconfiguring active FLLs we should make sure such use case remains properly supported.
What I came up so far as a fix is reading current FLL refclk source and if FLL was enabled with that source disabling refclk, before we change FLL configuration to new one. So we have clk_disable_unprepare(MCLK) more closely following FLL enable bit changes. I have tested it and it seems to work - something like below. Do you think it makes sense?
Yeah I think that looks good, it is very similar to what we did on Arizona and I haven't found any problems with that yet :-)
Thanks, Charles
There is only one DAI link so we can drop an unnecessary loop statement. Use card->dai_link in place of direct static arndale_rt5631_dai[] array dereference as a prerequisite for adding support for other CODECs. Unnecessary assignment of dai_link->codecs->name to NULL is removed.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com --- sound/soc/samsung/arndale_rt5631.c | 42 ++++++++++++------------------ 1 file changed, 17 insertions(+), 25 deletions(-)
diff --git a/sound/soc/samsung/arndale_rt5631.c b/sound/soc/samsung/arndale_rt5631.c index c213913eb984..c5c8e3b5772f 100644 --- a/sound/soc/samsung/arndale_rt5631.c +++ b/sound/soc/samsung/arndale_rt5631.c @@ -76,41 +76,33 @@ static struct snd_soc_card arndale_rt5631 = {
static int arndale_audio_probe(struct platform_device *pdev) { - int n, ret; struct device_node *np = pdev->dev.of_node; struct snd_soc_card *card = &arndale_rt5631; + struct snd_soc_dai_link *dai_link; + int ret;
card->dev = &pdev->dev; + dai_link = card->dai_link;
- for (n = 0; np && n < ARRAY_SIZE(arndale_rt5631_dai); n++) { - if (!arndale_rt5631_dai[n].cpus->dai_name) { - arndale_rt5631_dai[n].cpus->of_node = of_parse_phandle(np, - "samsung,audio-cpu", n); - - if (!arndale_rt5631_dai[n].cpus->of_node) { - dev_err(&pdev->dev, - "Property 'samsung,audio-cpu' missing or invalid\n"); - return -EINVAL; - } - } - if (!arndale_rt5631_dai[n].platforms->name) - arndale_rt5631_dai[n].platforms->of_node = - arndale_rt5631_dai[n].cpus->of_node; - - arndale_rt5631_dai[n].codecs->name = NULL; - arndale_rt5631_dai[n].codecs->of_node = of_parse_phandle(np, - "samsung,audio-codec", n); - if (!arndale_rt5631_dai[0].codecs->of_node) { - dev_err(&pdev->dev, - "Property 'samsung,audio-codec' missing or invalid\n"); + dai_link->cpus->of_node = of_parse_phandle(np, "samsung,audio-cpu", 0); + if (!dai_link->cpus->of_node) { + dev_err(&pdev->dev, "Property 'samsung,audio-cpu' missing or invalid\n"); return -EINVAL; - } }
- ret = devm_snd_soc_register_card(card->dev, card); + if (!dai_link->platforms->name) + dai_link->platforms->of_node = dai_link->cpus->of_node; + + dai_link->codecs->of_node = of_parse_phandle(np, "samsung,audio-codec", 0); + if (!dai_link->codecs->of_node) { + dev_err(&pdev->dev, + "Property 'samsung,audio-codec' missing or invalid\n"); + return -EINVAL; + }
+ ret = devm_snd_soc_register_card(card->dev, card); if (ret) - dev_err(&pdev->dev, "snd_soc_register_card() failed:%d\n", ret); + dev_err(&pdev->dev, "snd_soc_register_card() failed: %d\n", ret);
return ret; }
On Wed, Sep 18, 2019 at 12:46:30PM +0200, Sylwester Nawrocki wrote:
There is only one DAI link so we can drop an unnecessary loop statement. Use card->dai_link in place of direct static arndale_rt5631_dai[] array dereference as a prerequisite for adding support for other CODECs. Unnecessary assignment of dai_link->codecs->name to NULL is removed.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
Reviewed-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
On Wed, Sep 18, 2019 at 12:46:30PM +0200, Sylwester Nawrocki wrote:
There is only one DAI link so we can drop an unnecessary loop statement. Use card->dai_link in place of direct static arndale_rt5631_dai[] array dereference as a prerequisite for adding support for other CODECs. Unnecessary assignment of dai_link->codecs->name to NULL is removed.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
sound/soc/samsung/arndale_rt5631.c | 42 ++++++++++++------------------ 1 file changed, 17 insertions(+), 25 deletions(-)
Acked-by: Krzysztof Kozlowski krzk@kernel.org
Best regards, Krzysztof
Add compatible string for boards with WM1811 CODEC to the list.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com --- Documentation/devicetree/bindings/sound/arndale.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/arndale.txt b/Documentation/devicetree/bindings/sound/arndale.txt index 0e76946385ae..17530120ccfc 100644 --- a/Documentation/devicetree/bindings/sound/arndale.txt +++ b/Documentation/devicetree/bindings/sound/arndale.txt @@ -1,8 +1,9 @@ Audio Binding for Arndale boards
Required properties: -- compatible : Can be the following, - "samsung,arndale-rt5631" +- compatible : Can be one of the following: + "samsung,arndale-rt5631", + "samsung,arndale-wm1811"
- samsung,audio-cpu: The phandle of the Samsung I2S controller - samsung,audio-codec: The phandle of the audio codec
On Wed, Sep 18, 2019 at 12:46:31PM +0200, Sylwester Nawrocki wrote:
Add compatible string for boards with WM1811 CODEC to the list.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
Documentation/devicetree/bindings/sound/arndale.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Acked-by: Krzysztof Kozlowski krzk@kernel.org
Best regards, Krzysztof
The Arndale boards come with different types of the audio daughter board. In order to support the WM1811 one we add new definition of an ASoC card which will be registered when the driver matches on "samsung,arndale-wm1811" compatible. There is no runtime detection of the audio daughter board type at the moment, compatible string of the audio card needs to be adjusted in DT, e.g. by the bootloader, depending on actual audio board (CODEC) used.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com --- sound/soc/samsung/arndale_rt5631.c | 86 +++++++++++++++++++++++++----- 1 file changed, 73 insertions(+), 13 deletions(-)
diff --git a/sound/soc/samsung/arndale_rt5631.c b/sound/soc/samsung/arndale_rt5631.c index c5c8e3b5772f..3744c47742b8 100644 --- a/sound/soc/samsung/arndale_rt5631.c +++ b/sound/soc/samsung/arndale_rt5631.c @@ -5,6 +5,7 @@ // Author: Claude claude@insginal.co.kr
#include <linux/module.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/clk.h>
@@ -13,10 +14,11 @@ #include <sound/pcm.h> #include <sound/pcm_params.h>
+#include "../codecs/wm8994.h" #include "i2s.h"
-static int arndale_hw_params(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params) +static int arndale_rt5631_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_dai *cpu_dai = rtd->cpu_dai; @@ -46,13 +48,45 @@ static int arndale_hw_params(struct snd_pcm_substream *substream, return 0; }
-static struct snd_soc_ops arndale_ops = { - .hw_params = arndale_hw_params, +static struct snd_soc_ops arndale_rt5631_ops = { + .hw_params = arndale_rt5631_hw_params, +}; + +static int arndale_wm1811_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *codec_dai = rtd->codec_dai; + unsigned int rfs, rclk; + + /* Ensure AIF1CLK is >= 3 MHz for optimal performance */ + if (params_width(params) == 24) + rfs = 384; + else if (params_rate(params) == 8000 || params_rate(params) == 11025) + rfs = 512; + else + rfs = 256; + + rclk = params_rate(params) * rfs; + + /* + * We add 1 to the frequency value to ensure proper EPLL setting + * for each audio sampling rate (see epll_24mhz_tbl in drivers/clk/ + * samsung/clk-exynos5250.c for list of available EPLL rates). + * The CODEC uses clk API and the value will be rounded hence the MCLK1 + * clock's frequency will still be exact multiple of the sample rate. + */ + return snd_soc_dai_set_sysclk(codec_dai, WM8994_SYSCLK_MCLK1, + rclk + 1, SND_SOC_CLOCK_IN); +} + +static struct snd_soc_ops arndale_wm1811_ops = { + .hw_params = arndale_wm1811_hw_params, };
SND_SOC_DAILINK_DEFS(rt5631_hifi, DAILINK_COMP_ARRAY(COMP_EMPTY()), - DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "rt5631-hifi")), + DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "rt5631-aif1")), DAILINK_COMP_ARRAY(COMP_EMPTY()));
static struct snd_soc_dai_link arndale_rt5631_dai[] = { @@ -62,11 +96,28 @@ static struct snd_soc_dai_link arndale_rt5631_dai[] = { .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS, - .ops = &arndale_ops, + .ops = &arndale_rt5631_ops, SND_SOC_DAILINK_REG(rt5631_hifi), }, };
+SND_SOC_DAILINK_DEFS(wm1811_hifi, + DAILINK_COMP_ARRAY(COMP_EMPTY()), + DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "wm8994-aif1")), + DAILINK_COMP_ARRAY(COMP_EMPTY())); + +static struct snd_soc_dai_link arndale_wm1811_dai[] = { + { + .name = "WM1811 HiFi", + .stream_name = "Primary", + .dai_fmt = SND_SOC_DAIFMT_I2S + | SND_SOC_DAIFMT_NB_NF + | SND_SOC_DAIFMT_CBM_CFM, + .ops = &arndale_wm1811_ops, + SND_SOC_DAILINK_REG(wm1811_hifi), + }, +}; + static struct snd_soc_card arndale_rt5631 = { .name = "Arndale RT5631", .owner = THIS_MODULE, @@ -74,13 +125,21 @@ static struct snd_soc_card arndale_rt5631 = { .num_links = ARRAY_SIZE(arndale_rt5631_dai), };
+static struct snd_soc_card arndale_wm1811 = { + .name = "Arndale WM1811", + .owner = THIS_MODULE, + .dai_link = arndale_wm1811_dai, + .num_links = ARRAY_SIZE(arndale_wm1811_dai), +}; + static int arndale_audio_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; - struct snd_soc_card *card = &arndale_rt5631; + struct snd_soc_card *card; struct snd_soc_dai_link *dai_link; int ret;
+ card = (struct snd_soc_card *)of_device_get_match_data(&pdev->dev); card->dev = &pdev->dev; dai_link = card->dai_link;
@@ -107,18 +166,19 @@ static int arndale_audio_probe(struct platform_device *pdev) return ret; }
-static const struct of_device_id samsung_arndale_rt5631_of_match[] __maybe_unused = { - { .compatible = "samsung,arndale-rt5631", }, - { .compatible = "samsung,arndale-alc5631", }, +static const struct of_device_id arndale_audio_of_match[] __maybe_unused = { + { .compatible = "samsung,arndale-rt5631", .data = &arndale_rt5631 }, + { .compatible = "samsung,arndale-alc5631", .data = &arndale_rt5631 }, + { .compatible = "samsung,arndale-wm1811", .data = &arndale_wm1811 }, {}, }; -MODULE_DEVICE_TABLE(of, samsung_arndale_rt5631_of_match); +MODULE_DEVICE_TABLE(of, arndale_of_match);
static struct platform_driver arndale_audio_driver = { .driver = { - .name = "arndale-audio", + .name = "arndale-audio", .pm = &snd_soc_pm_ops, - .of_match_table = of_match_ptr(samsung_arndale_rt5631_of_match), + .of_match_table = arndale_audio_of_match, }, .probe = arndale_audio_probe, };
On Wed, Sep 18, 2019 at 12:46:32PM +0200, Sylwester Nawrocki wrote:
The Arndale boards come with different types of the audio daughter board. In order to support the WM1811 one we add new definition of an ASoC card which will be registered when the driver matches on "samsung,arndale-wm1811" compatible. There is no runtime detection of the audio daughter board type at the moment, compatible string of the audio card needs to be adjusted in DT, e.g. by the bootloader, depending on actual audio board (CODEC) used.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
-static const struct of_device_id samsung_arndale_rt5631_of_match[] __maybe_unused = {
- { .compatible = "samsung,arndale-rt5631", },
- { .compatible = "samsung,arndale-alc5631", },
+static const struct of_device_id arndale_audio_of_match[] __maybe_unused = {
If your removing the of_match_ptr below I think the __maybe_unused should be removed as well.
Thanks, Charles
- { .compatible = "samsung,arndale-rt5631", .data = &arndale_rt5631 },
- { .compatible = "samsung,arndale-alc5631", .data = &arndale_rt5631 },
- { .compatible = "samsung,arndale-wm1811", .data = &arndale_wm1811 }, {},
}; -MODULE_DEVICE_TABLE(of, samsung_arndale_rt5631_of_match); +MODULE_DEVICE_TABLE(of, arndale_of_match);
static struct platform_driver arndale_audio_driver = { .driver = {
.name = "arndale-audio",
.name = "arndale-audio",
.of_match_table = of_match_ptr(samsung_arndale_rt5631_of_match),
.of_match_table = arndale_audio_of_match,
On 9/18/19 16:45, Charles Keepax wrote:
If your removing the of_match_ptr below I think the __maybe_unused should be removed as well.
Good point, I will remove the now unneeded __maybe_unused as well.
-- Thanks, Sylwester
Ensure there is no OF node references kept when the driver is removed/unbound.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com --- sound/soc/samsung/arndale_rt5631.c | 31 ++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/sound/soc/samsung/arndale_rt5631.c b/sound/soc/samsung/arndale_rt5631.c index 3744c47742b8..d8da313e898a 100644 --- a/sound/soc/samsung/arndale_rt5631.c +++ b/sound/soc/samsung/arndale_rt5631.c @@ -132,6 +132,17 @@ static struct snd_soc_card arndale_wm1811 = { .num_links = ARRAY_SIZE(arndale_wm1811_dai), };
+static void arndale_put_of_nodes(struct snd_soc_card *card) +{ + struct snd_soc_dai_link *dai_link; + int i; + + for_each_card_prelinks(card, i, dai_link) { + of_node_put(dai_link->cpus->of_node); + of_node_put(dai_link->codecs->of_node); + } +} + static int arndale_audio_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -156,16 +167,31 @@ static int arndale_audio_probe(struct platform_device *pdev) if (!dai_link->codecs->of_node) { dev_err(&pdev->dev, "Property 'samsung,audio-codec' missing or invalid\n"); - return -EINVAL; + ret = -EINVAL; + goto err_put_of_nodes; }
ret = devm_snd_soc_register_card(card->dev, card); - if (ret) + if (ret) { dev_err(&pdev->dev, "snd_soc_register_card() failed: %d\n", ret); + goto err_put_of_nodes; + }
+ return 0; + +err_put_of_nodes: + arndale_put_of_nodes(card); return ret; }
+static int arndale_audio_remove(struct platform_device *pdev) +{ + struct snd_soc_card *card = platform_get_drvdata(pdev); + + arndale_put_of_nodes(card); + return 0; +} + static const struct of_device_id arndale_audio_of_match[] __maybe_unused = { { .compatible = "samsung,arndale-rt5631", .data = &arndale_rt5631 }, { .compatible = "samsung,arndale-alc5631", .data = &arndale_rt5631 }, @@ -181,6 +207,7 @@ static struct platform_driver arndale_audio_driver = { .of_match_table = arndale_audio_of_match, }, .probe = arndale_audio_probe, + .remove = arndale_audio_remove, };
module_platform_driver(arndale_audio_driver);
On Wed, Sep 18, 2019 at 12:46:33PM +0200, Sylwester Nawrocki wrote:
Ensure there is no OF node references kept when the driver is removed/unbound.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
Reviewed-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
On Wed, Sep 18, 2019 at 12:46:33PM +0200, Sylwester Nawrocki wrote:
Ensure there is no OF node references kept when the driver is removed/unbound.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
sound/soc/samsung/arndale_rt5631.c | 31 ++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
Wasn't this issue introduced in 5/9? It looks weird to fix it here...
Best regards, Krzysztof
diff --git a/sound/soc/samsung/arndale_rt5631.c b/sound/soc/samsung/arndale_rt5631.c index 3744c47742b8..d8da313e898a 100644 --- a/sound/soc/samsung/arndale_rt5631.c +++ b/sound/soc/samsung/arndale_rt5631.c @@ -132,6 +132,17 @@ static struct snd_soc_card arndale_wm1811 = { .num_links = ARRAY_SIZE(arndale_wm1811_dai), };
+static void arndale_put_of_nodes(struct snd_soc_card *card) +{
- struct snd_soc_dai_link *dai_link;
- int i;
- for_each_card_prelinks(card, i, dai_link) {
of_node_put(dai_link->cpus->of_node);of_node_put(dai_link->codecs->of_node);- }
+}
static int arndale_audio_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -156,16 +167,31 @@ static int arndale_audio_probe(struct platform_device *pdev) if (!dai_link->codecs->of_node) { dev_err(&pdev->dev, "Property 'samsung,audio-codec' missing or invalid\n");
return -EINVAL;
ret = -EINVAL;goto err_put_of_nodes;}
ret = devm_snd_soc_register_card(card->dev, card);
- if (ret)
if (ret) { dev_err(&pdev->dev, "snd_soc_register_card() failed: %d\n", ret);
goto err_put_of_nodes;}
return 0;
+err_put_of_nodes:
- arndale_put_of_nodes(card); return ret;
}
+static int arndale_audio_remove(struct platform_device *pdev) +{
- struct snd_soc_card *card = platform_get_drvdata(pdev);
- arndale_put_of_nodes(card);
- return 0;
+}
static const struct of_device_id arndale_audio_of_match[] __maybe_unused = { { .compatible = "samsung,arndale-rt5631", .data = &arndale_rt5631 }, { .compatible = "samsung,arndale-alc5631", .data = &arndale_rt5631 }, @@ -181,6 +207,7 @@ static struct platform_driver arndale_audio_driver = { .of_match_table = arndale_audio_of_match, }, .probe = arndale_audio_probe,
- .remove = arndale_audio_remove,
};
module_platform_driver(arndale_audio_driver);
2.17.1
On 9/19/19 10:22, Krzysztof Kozlowski wrote:
Wasn't this issue introduced in 5/9? It looks weird to fix it here...
It has not been introduced by 5/9, the issue was there already before my patch, there was even no remove() callback where OF node references could be dropped.
On Thu, 19 Sep 2019 at 14:49, Sylwester Nawrocki s.nawrocki@samsung.com wrote:
On 9/19/19 10:22, Krzysztof Kozlowski wrote:
Wasn't this issue introduced in 5/9? It looks weird to fix it here...
It has not been introduced by 5/9, the issue was there already before my patch, there was even no remove() callback where OF node references could be dropped.
I see. Then please put it as first patch. You should not mix bugfixes with features. If mixing, be sure they can be applied before the features.
Best regards, Krzysztof
On 9/19/19 14:58, Krzysztof Kozlowski wrote:
On Thu, 19 Sep 2019 at 14:49, Sylwester Nawrocki s.nawrocki@samsung.com wrote:
On 9/19/19 10:22, Krzysztof Kozlowski wrote:
Wasn't this issue introduced in 5/9? It looks weird to fix it here...
It has not been introduced by 5/9, the issue was there already before my patch, there was even no remove() callback where OF node references could be dropped.
I see. Then please put it as first patch. You should not mix bugfixes with features. If mixing, be sure they can be applied before the features.
I will see if it is worth the effort to rebase this patch. I didn't bother with that because this sound card driver is not used in mainline (there is no related dts changes) and the patch is a fix for minor bug which I found just before posting first version of the patch series.
Add sound node and the clock configurations for the I2S controller for audio support on the Exynos5250 SoC Arndale boards with WM1811 based audio daugther board.
We need to increase drive strength of the I2S bus, otherwise the audio CODEC doesn't work. Likely the CODEC's master clock is the main issue here.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com --- arch/arm/boot/dts/exynos5250-arndale.dts | 27 +++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts index dc6fa6fe83f1..62aa6720aa88 100644 --- a/arch/arm/boot/dts/exynos5250-arndale.dts +++ b/arch/arm/boot/dts/exynos5250-arndale.dts @@ -11,6 +11,7 @@ #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/input/input.h> #include <dt-bindings/clock/samsung,s2mps11.h> +#include <dt-bindings/sound/samsung-i2s.h> #include "exynos5250.dtsi"
/ { @@ -135,6 +136,12 @@ }; };
+ sound { + compatible = "samsung,arndale-wm1811"; + samsung,audio-cpu = <&i2s0>; + samsung,audio-codec = <&wm1811>; + }; + fixed-rate-clocks { xxti { compatible = "samsung,clock-xxti"; @@ -499,12 +506,24 @@ }; };
+&clock { + assigned-clocks = <&clock CLK_FOUT_EPLL>; + assigned-clock-rates = <49152000>; +}; + +&clock_audss { + assigned-clocks = <&clock_audss EXYNOS_MOUT_AUDSS>; + assigned-clock-parents = <&clock CLK_FOUT_EPLL>; +}; + &i2c_3 { status = "okay";
- wm1811a@1a { + wm1811: codec@1a { compatible = "wlf,wm1811"; reg = <0x1a>; + clocks = <&i2s0 CLK_I2S_CDCLK>; + clock-names = "MCLK1";
AVDD2-supply = <&main_dc_reg>; CPVDD-supply = <&main_dc_reg>; @@ -540,9 +559,15 @@ };
&i2s0 { + assigned-clocks = <&i2s0 CLK_I2S_RCLK_SRC>; + assigned-clock-parents = <&clock_audss EXYNOS_I2S_BUS>; status = "okay"; };
+&i2s0_bus { + samsung,pin-drv = <EXYNOS4_PIN_DRV_LV2>; +}; + &mixer { status = "okay"; };
On Wed, Sep 18, 2019 at 12:46:34PM +0200, Sylwester Nawrocki wrote:
Add sound node and the clock configurations for the I2S controller for audio support on the Exynos5250 SoC Arndale boards with WM1811 based audio daugther board.
We need to increase drive strength of the I2S bus, otherwise the audio CODEC doesn't work. Likely the CODEC's master clock is the main issue here.
Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
arch/arm/boot/dts/exynos5250-arndale.dts | 27 +++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts index dc6fa6fe83f1..62aa6720aa88 100644 --- a/arch/arm/boot/dts/exynos5250-arndale.dts +++ b/arch/arm/boot/dts/exynos5250-arndale.dts @@ -11,6 +11,7 @@ #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/input/input.h> #include <dt-bindings/clock/samsung,s2mps11.h> +#include <dt-bindings/sound/samsung-i2s.h> #include "exynos5250.dtsi"
/ { @@ -135,6 +136,12 @@ }; };
- sound {
compatible = "samsung,arndale-wm1811";samsung,audio-cpu = <&i2s0>;samsung,audio-codec = <&wm1811>;- };
- fixed-rate-clocks { xxti { compatible = "samsung,clock-xxti";
@@ -499,12 +506,24 @@ }; };
+&clock {
- assigned-clocks = <&clock CLK_FOUT_EPLL>;
- assigned-clock-rates = <49152000>;
+};
+&clock_audss {
- assigned-clocks = <&clock_audss EXYNOS_MOUT_AUDSS>;
- assigned-clock-parents = <&clock CLK_FOUT_EPLL>;
+};
Put them before "cpu" so alphabetical order is preserved.
Best regards, Krzysztof
&i2c_3 { status = "okay";
- wm1811a@1a {
wm1811: codec@1a { compatible = "wlf,wm1811"; reg = <0x1a>;
clocks = <&i2s0 CLK_I2S_CDCLK>;clock-names = "MCLK1";AVDD2-supply = <&main_dc_reg>; CPVDD-supply = <&main_dc_reg>;
@@ -540,9 +559,15 @@ };
&i2s0 {
- assigned-clocks = <&i2s0 CLK_I2S_RCLK_SRC>;
- assigned-clock-parents = <&clock_audss EXYNOS_I2S_BUS>; status = "okay";
};
+&i2s0_bus {
- samsung,pin-drv = <EXYNOS4_PIN_DRV_LV2>;
+};
&mixer { status = "okay"; }; -- 2.17.1
participants (4)
-
Charles Keepax -
Krzysztof Kozlowski -
Mark Brown -
Sylwester Nawrocki