| Message ID | 20190918104634.15216-1-s.nawrocki@samsung.com |
|---|---|
| Headers | show |
| Series | Basic sound support for Arndale board / wm8994 updates | expand |
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: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: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: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: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; > + } > + } > /* 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); 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 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: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", > .pm = &snd_soc_pm_ops, > - .of_match_table = of_match_ptr(samsung_arndale_rt5631_of_match), > + .of_match_table = arndale_audio_of_match,
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: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
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 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
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
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 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 >
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
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 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, 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, 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 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 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.
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 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
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.
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(-)