devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH/RFC 12/14] ASoC: samsung: i2s: Add clock provider for the I2S internal clocks
       [not found] <1418319952-10163-1-git-send-email-s.nawrocki@samsung.com>
@ 2014-12-11 17:45 ` Sylwester Nawrocki
  2014-12-12 20:03   ` [alsa-devel] " Mark Brown
  2014-12-11 17:45 ` [PATCH/RFC 13/14] ARM: dts: Exynos4 and Odroid X2/U3 sound device nodes update Sylwester Nawrocki
  2014-12-11 17:45 ` [PATCH/RFC 14/14] ARM: dts: Switch Odroid X2/U2 to simple-audio-card Sylwester Nawrocki
  2 siblings, 1 reply; 5+ messages in thread
From: Sylwester Nawrocki @ 2014-12-11 17:45 UTC (permalink / raw)
  To: alsa-devel, linux-samsung-soc
  Cc: broonie, padma.v, drake, sbkim73, Sylwester Nawrocki, devicetree

This patch adds clock provider (currently only for DT platforms) for
the CODECLKO (CDCLK) gate, RCLKSRC mux and RCLK pre-scaler divider
divider clock. Those all tree clock are only available in the IIS
Multi Audio Interface (I2S0), the regular IIS Bus Interface has only
CDCLK gate clock.

The motivation behind this patch is to expose the I2S internal clocks
which are currently controlled through set_sysclk() through the clk
API, so dedicated sound machine driver per each board can be avoided.

The intention is also to fix the CDCLK gating issue reported by
Daniel Drake:
http://mailman.alsa-project.org/pipermail/alsa-devel/2014-September/081753.html

This patch also reverts commit b97c60abf9a561f86ae71bd741add02673cc1
("ASoC: samsung-i2s: Maintain CDCLK settings across i2s_{shutdown/
startup}") The problem that commit attempted to solve only affects
the Odroid X2/U3, which doesn't configure the CDCLK clock in
struct snd_soc_dai_ops hw_params callback and the issue should be
now resolved by using clk API, i.e. having the codec enabling/
disabling the CDCLK clock as required.

Cc: devicetree@vger.kernel.org
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 .../devicetree/bindings/sound/samsung-i2s.txt      |   18 ++-
 sound/soc/samsung/i2s.c                            |  116 ++++++++++++++++----
 2 files changed, 113 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt b/Documentation/devicetree/bindings/sound/samsung-i2s.txt
index d188296..fb3b777 100644
--- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt
+++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt
@@ -34,12 +34,26 @@ Required SoC Specific Properties:
   clk. i2s0 has internal mux to select the source of root clk and i2s1 and i2s2
   doesn't have any such mux.

-Optional SoC Specific Properties:
+Optional Properties:

 - samsung,idma-addr: Internal DMA register base address of the audio
   sub system(used in secondary sound source).
 - pinctrl-0: Should specify pin control groups used for this controller.
 - pinctrl-names: Should contain only one value - "default".
+- #clock-cells: should be 1, this property must be present if the I2S device
+  is a clock provider in terms of the common clock bindings, described in
+  ../clock/clock-bindings.txt.
+- clock-output-names: from the common clock bindings, names of the CDCLK
+  I2S output clocks, suggested values are "i2s_cdclk0", "i2s_cdclk1",
+  "i2s_cdclk3" for the I2S0, I2S1, I2S2 devices recpectively.
+
+The assignment of indices for the I2Sx clock provider is as follows:
+ 0 - the CDCLK (CODECLKO) gate clock,
+ 1 - the RCLK prescaler divider clock (corresponding to the IISPSR register),
+ 2 - the RCLKSRC mux clock (corresponding to RCLKSRC bit in register IISMOD).
+
+Clocks 1, 2 are normally only available in the IIS Mutli Audio Interface (I2S0).
+

 Example:

@@ -54,6 +68,8 @@ i2s0: i2s@03830000 {
 		<&clock_audss EXYNOS_I2S_BUS>,
 		<&clock_audss EXYNOS_SCLK_I2S>;
 	clock-names = "iis", "i2s_opclk0", "i2s_opclk1";
+	#clock-cells;
+	clock-output-names = "i2s_cdclk0";
 	samsung,idma-addr = <0x03000000>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2s0_bus>;
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 825138b..3f4d2c0 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -13,6 +13,7 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -81,8 +82,6 @@ struct i2s_dai {
 #define DAI_OPENED	(1 << 0) /* Dai is opened */
 #define DAI_MANAGER	(1 << 1) /* Dai is the manager */
 	unsigned mode;
-	/* CDCLK pin direction: 0  - input, 1 - output */
-	unsigned int cdclk_out:1;
 	/* Driver for this DAI */
 	struct snd_soc_dai_driver i2s_dai_drv;
 	/* DMA parameters */
@@ -98,6 +97,10 @@ struct i2s_dai {
 	/* Spinlock protecting access to the device's registers */
 	spinlock_t spinlock;
 	spinlock_t *lock;
+
+	/* Below fields are only valid if this is the primary FIFO */
+	struct clk *clk_table[3];
+	struct clk_onecell_data clk_data;
 };

 /* Lock for cross i/f checks */
@@ -774,9 +777,6 @@ static int i2s_startup(struct snd_pcm_substream *substream,

 	spin_unlock_irqrestore(&lock, flags);

-	if (!is_opened(other) && i2s->cdclk_out)
-		i2s_set_sysclk(dai, SAMSUNG_I2S_CDCLK,
-				0, SND_SOC_CLOCK_OUT);
 	return 0;
 }

@@ -786,31 +786,20 @@ static void i2s_shutdown(struct snd_pcm_substream *substream,
 	struct i2s_dai *i2s = to_info(dai);
 	struct i2s_dai *other = get_other_dai(i2s);
 	unsigned long flags;
-	const struct samsung_i2s_variant_regs *i2s_regs = i2s->variant_regs;

 	spin_lock_irqsave(&lock, flags);

 	i2s->mode &= ~DAI_OPENED;
 	i2s->mode &= ~DAI_MANAGER;

-	if (is_opened(other)) {
+	if (is_opened(other))
 		other->mode |= DAI_MANAGER;
-	} else {
-		u32 mod = readl(i2s->addr + I2SMOD);
-		i2s->cdclk_out = !(mod & (1 << i2s_regs->cdclkcon_off));
-		if (other)
-			other->cdclk_out = i2s->cdclk_out;
-	}
+
 	/* Reset any constraint on RFS and BFS */
 	i2s->rfs = 0;
 	i2s->bfs = 0;

 	spin_unlock_irqrestore(&lock, flags);
-
-	/* Gate CDCLK by default */
-	if (!is_opened(other))
-		i2s_set_sysclk(dai, SAMSUNG_I2S_CDCLK,
-				0, SND_SOC_CLOCK_IN);
 }

 static int config_setup(struct i2s_dai *i2s)
@@ -1147,6 +1136,91 @@ static int i2s_runtime_resume(struct device *dev)
 }
 #endif /* CONFIG_PM_RUNTIME */

+static void i2s_unregister_clocks(struct i2s_dai *i2s)
+{
+	int i;
+
+	for (i = 0; i < i2s->clk_data.clk_num; i++) {
+		if (!IS_ERR(i2s->clk_table[i]))
+			clk_unregister(i2s->clk_table[i]);
+	}
+}
+
+static void i2s_unregister_clock_provider(struct platform_device *pdev)
+{
+	struct i2s_dai *i2s = dev_get_drvdata(&pdev->dev);
+
+	of_clk_del_provider(pdev->dev.of_node);
+	i2s_unregister_clocks(i2s);
+}
+
+static int i2s_register_clock_provider(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct i2s_dai *i2s = dev_get_drvdata(dev);
+	const char *clk_name[2] = { "i2s_opclk0", "i2s_opclk1" };
+	const char *p_names[2] = { NULL };
+	const struct samsung_i2s_variant_regs *reg_info = i2s->variant_regs;
+	struct clk *rclksrc;
+	int ret, i;
+
+	/* Register the clock provider only if it's expected in the DTB */
+	if (!of_find_property(dev->of_node, "#clock-cells", NULL))
+		return 0;
+
+	/* Get the RCLKSRC mux clock parent clock names */
+	for (i = 0; i < ARRAY_SIZE(p_names); i++) {
+		rclksrc = clk_get(dev, clk_name[i]);
+		if (IS_ERR(rclksrc))
+			continue;
+		p_names[i] = __clk_get_name(rclksrc);
+		clk_put(rclksrc);
+	}
+	/*
+	 * Register the mux and div clocks only if both source clocks
+	 * are available, i.e. for the I2S0 instance and versions with
+	 * QUIRK_NO_MUXPSR quirk not set.
+	 */
+	if (p_names[1] && !(i2s->quirks & QUIRK_NO_MUXPSR)) {
+		/* Activate the prescaler */
+		u32 val = readl(i2s->addr + I2SPSR);
+		writel(val | PSR_PSREN, i2s->addr + I2SPSR);
+
+		i2s->clk_table[1] = clk_register_mux(NULL, "i2s_rclksrc",
+				p_names, ARRAY_SIZE(p_names),
+				CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT,
+				i2s->addr + I2SMOD, reg_info->rclksrc_off,
+				1, 0, i2s->lock);
+
+		i2s->clk_table[2] = clk_register_divider(NULL,
+				"i2s_presc", "i2s_rclksrc",
+				CLK_SET_RATE_PARENT,
+				i2s->addr + I2SPSR, 8, 6, 0, i2s->lock);
+
+		p_names[0] = "i2s_presc";
+		i2s->clk_data.clk_num = 2;
+	}
+	of_property_read_string_index(dev->of_node,
+				"clock-output-names", 0, &clk_name[0]);
+
+	i2s->clk_table[0] = clk_register_gate(NULL, clk_name[0],
+				p_names[0], CLK_SET_RATE_PARENT,
+				i2s->addr + I2SMOD, reg_info->cdclkcon_off,
+				CLK_GATE_SET_TO_DISABLE, i2s->lock);
+
+	i2s->clk_data.clk_num += 1;
+	i2s->clk_data.clks = i2s->clk_table;
+
+	ret = of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
+				  &i2s->clk_data);
+	if (ret < 0) {
+		dev_err(dev, "failed to add clock provider\n");
+		i2s_unregister_clocks(i2s);
+	}
+
+	return ret;
+}
+
 static int samsung_i2s_probe(struct platform_device *pdev)
 {
 	struct i2s_dai *pri_dai, *sec_dai = NULL;
@@ -1297,7 +1371,7 @@ static int samsung_i2s_probe(struct platform_device *pdev)
 	if (ret != 0)
 		return ret;

-	return 0;
+	return i2s_register_clock_provider(pdev);
 }

 static int samsung_i2s_remove(struct platform_device *pdev)
@@ -1314,8 +1388,10 @@ static int samsung_i2s_remove(struct platform_device *pdev)
 		pm_runtime_disable(&pdev->dev);
 	}

-	if (!is_secondary(i2s))
+	if (!is_secondary(i2s)) {
+		i2s_unregister_clock_provider(pdev);
 		clk_disable_unprepare(i2s->clk);
+	}

 	i2s->pri_dai = NULL;
 	i2s->sec_dai = NULL;
--
1.7.9.5

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH/RFC 13/14] ARM: dts: Exynos4 and Odroid X2/U3 sound device nodes update
       [not found] <1418319952-10163-1-git-send-email-s.nawrocki@samsung.com>
  2014-12-11 17:45 ` [PATCH/RFC 12/14] ASoC: samsung: i2s: Add clock provider for the I2S internal clocks Sylwester Nawrocki
@ 2014-12-11 17:45 ` Sylwester Nawrocki
  2014-12-11 17:45 ` [PATCH/RFC 14/14] ARM: dts: Switch Odroid X2/U2 to simple-audio-card Sylwester Nawrocki
  2 siblings, 0 replies; 5+ messages in thread
From: Sylwester Nawrocki @ 2014-12-11 17:45 UTC (permalink / raw)
  To: alsa-devel, linux-samsung-soc
  Cc: broonie, padma.v, drake, sbkim73, Sylwester Nawrocki, devicetree

Clock related properties are added to the Exynos4 I2S device nodes
so they can be referred to as clock providers. Missing i2s_opclk1
clock is added to the I2S0 node and clock properties are added
to the MAX98090 codec node to allow it to control/read frequency
of the MCLK clock directly.

Cc: devicetree@vger.kernel.org
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 arch/arm/boot/dts/exynos4.dtsi                  |    6 ++++++
 arch/arm/boot/dts/exynos4412-odroid-common.dtsi |    7 +++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index e0278ec..e1ce457 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -61,6 +61,8 @@
 		reg = <0x03830000 0x100>;
 		clocks = <&clock_audss EXYNOS_I2S_BUS>;
 		clock-names = "iis";
+		#clock-cells = <1>;
+		clock-output-names = "i2s_cdclk0";
 		dmas = <&pdma0 12>, <&pdma0 11>, <&pdma0 10>;
 		dma-names = "tx", "rx", "tx-sec";
 		samsung,idma-addr = <0x03000000>;
@@ -372,6 +374,8 @@
 		reg = <0x13960000 0x100>;
 		clocks = <&clock CLK_I2S1>;
 		clock-names = "iis";
+		#clock-cells = <1>;
+		clock-output-names = "i2s_cdclk1";
 		dmas = <&pdma1 12>, <&pdma1 11>;
 		dma-names = "tx", "rx";
 		status = "disabled";
@@ -382,6 +386,8 @@
 		reg = <0x13970000 0x100>;
 		clocks = <&clock CLK_I2S2>;
 		clock-names = "iis";
+		#clock-cells = <1>;
+		clock-output-names = "i2s_cdclk2";
 		dmas = <&pdma0 14>, <&pdma0 13>;
 		dma-names = "tx", "rx";
 		status = "disabled";
diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
index b41950f..956737e 100644
--- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
+++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
@@ -37,8 +37,9 @@
 		pinctrl-names = "default";
 		status = "okay";
 		clocks = <&clock_audss EXYNOS_I2S_BUS>,
-			 <&clock_audss EXYNOS_DOUT_AUD_BUS>;
-		clock-names = "iis", "i2s_opclk0";
+			 <&clock_audss EXYNOS_DOUT_AUD_BUS>,
+			 <&clock_audss EXYNOS_SCLK_I2S>;
+		clock-names = "iis", "i2s_opclk0", "i2s_opclk1";
 	};

 	sound: sound {
@@ -357,6 +358,8 @@
 			reg = <0x10>;
 			interrupt-parent = <&gpx0>;
 			interrupts = <0 0>;
+			clocks = <&i2s0 0>;
+			clock-names = "mclk";
 		};
 	};

--
1.7.9.5

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH/RFC 14/14] ARM: dts: Switch Odroid X2/U2 to simple-audio-card
       [not found] <1418319952-10163-1-git-send-email-s.nawrocki@samsung.com>
  2014-12-11 17:45 ` [PATCH/RFC 12/14] ASoC: samsung: i2s: Add clock provider for the I2S internal clocks Sylwester Nawrocki
  2014-12-11 17:45 ` [PATCH/RFC 13/14] ARM: dts: Exynos4 and Odroid X2/U3 sound device nodes update Sylwester Nawrocki
@ 2014-12-11 17:45 ` Sylwester Nawrocki
  2 siblings, 0 replies; 5+ messages in thread
From: Sylwester Nawrocki @ 2014-12-11 17:45 UTC (permalink / raw)
  To: alsa-devel, linux-samsung-soc
  Cc: broonie, padma.v, drake, sbkim73, Sylwester Nawrocki, devicetree

Now when the CDCLK I2S output clock can be handled through the clock
API the Odroid X2/U3 can be switched to the simple-audio-card DT binding.

Cc: devicetree@vger.kernel.org
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
 arch/arm/boot/dts/exynos4.dtsi                  |    3 +++
 arch/arm/boot/dts/exynos4412-odroid-common.dtsi |   19 ++++++++++++++++---
 arch/arm/boot/dts/exynos4412-odroidu3.dts       |    8 +++++---
 arch/arm/boot/dts/exynos4412-odroidx2.dts       |    8 ++++++--
 4 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index e1ce457..5f9e72f 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -66,6 +66,7 @@
 		dmas = <&pdma0 12>, <&pdma0 11>, <&pdma0 10>;
 		dma-names = "tx", "rx", "tx-sec";
 		samsung,idma-addr = <0x03000000>;
+		#sound-dai-cells = <1>;
 		status = "disabled";
 	};

@@ -378,6 +379,7 @@
 		clock-output-names = "i2s_cdclk1";
 		dmas = <&pdma1 12>, <&pdma1 11>;
 		dma-names = "tx", "rx";
+		#sound-dai-cells = <1>;
 		status = "disabled";
 	};

@@ -390,6 +392,7 @@
 		clock-output-names = "i2s_cdclk2";
 		dmas = <&pdma0 14>, <&pdma0 13>;
 		dma-names = "tx", "rx";
+		#sound-dai-cells = <1>;
 		status = "disabled";
 	};

diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
index 956737e..89fed7a 100644
--- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
+++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
@@ -43,9 +43,7 @@
 	};

 	sound: sound {
-		compatible = "samsung,odroidx2-audio";
-		samsung,i2s-controller = <&i2s0>;
-		samsung,audio-codec = <&max98090>;
+		compatible = "simple-audio-card";
 		assigned-clocks = <&clock_audss EXYNOS_MOUT_AUDSS>,
 				<&clock_audss EXYNOS_MOUT_I2S>,
 				<&clock_audss EXYNOS_DOUT_SRP>,
@@ -56,6 +54,20 @@
 				<0>,
 				<192000000>,
 				<19200000>;
+
+		simple-audio-card,format = "i2s";
+		simple-audio-card,bitclock-master = <&link0_codec>;
+		simple-audio-card,frame-master = <&link0_codec>;
+
+		simple-audio-card,cpu {
+			sound-dai = <&i2s0 0>;
+			system-clock-frequency = <19200000>;
+		};
+
+		link0_codec: simple-audio-card,codec {
+			sound-dai = <&max98090>;
+			clocks = <&i2s0 0>;
+		};
 	};

 	mmc@12550000 {
@@ -360,6 +372,7 @@
 			interrupts = <0 0>;
 			clocks = <&i2s0 0>;
 			clock-names = "mclk";
+			#sound-dai-cells = <0>;
 		};
 	};

diff --git a/arch/arm/boot/dts/exynos4412-odroidu3.dts b/arch/arm/boot/dts/exynos4412-odroidu3.dts
index c8a64be..44684e5 100644
--- a/arch/arm/boot/dts/exynos4412-odroidu3.dts
+++ b/arch/arm/boot/dts/exynos4412-odroidu3.dts
@@ -49,9 +49,11 @@
 };

 &sound {
-	compatible = "samsung,odroidu3-audio";
-	samsung,model = "Odroid-U3";
-	samsung,audio-routing =
+	simple-audio-card,name = "Odroid-U3";
+	simple-audio-card,widgets =
+		"Headphone", "Headphone Jack",
+		"Speakers", "Speakers";
+	simple-audio-card,routing =
 		"Headphone Jack", "HPL",
 		"Headphone Jack", "HPR",
 		"Headphone Jack", "MICBIAS",
diff --git a/arch/arm/boot/dts/exynos4412-odroidx2.dts b/arch/arm/boot/dts/exynos4412-odroidx2.dts
index 96b43f4..6e33678 100644
--- a/arch/arm/boot/dts/exynos4412-odroidx2.dts
+++ b/arch/arm/boot/dts/exynos4412-odroidx2.dts
@@ -23,8 +23,12 @@
 };

 &sound {
-	samsung,model = "Odroid-X2";
-	samsung,audio-routing =
+	simple-audio-card,name = "Odroid-X2";
+	simple-audio-card,widgets =
+		"Headphone", "Headphone Jack",
+		"Microphone", "Mic Jack",
+		"Microphone", "DMIC";
+	simple-audio-card,routing =
 		"Headphone Jack", "HPL",
 		"Headphone Jack", "HPR",
 		"IN1", "Mic Jack",
--
1.7.9.5

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [alsa-devel] [PATCH/RFC 12/14] ASoC: samsung: i2s: Add clock provider for the I2S internal clocks
  2014-12-11 17:45 ` [PATCH/RFC 12/14] ASoC: samsung: i2s: Add clock provider for the I2S internal clocks Sylwester Nawrocki
@ 2014-12-12 20:03   ` Mark Brown
  2014-12-17 17:00     ` Sylwester Nawrocki
  0 siblings, 1 reply; 5+ messages in thread
From: Mark Brown @ 2014-12-12 20:03 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: alsa-devel, linux-samsung-soc, devicetree, padma.v, sbkim73,
	drake

[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]

On Thu, Dec 11, 2014 at 06:45:50PM +0100, Sylwester Nawrocki wrote:

> +Optional Properties:
> 
>  - samsung,idma-addr: Internal DMA register base address of the audio
>    sub system(used in secondary sound source).
>  - pinctrl-0: Should specify pin control groups used for this controller.
>  - pinctrl-names: Should contain only one value - "default".
> +- #clock-cells: should be 1, this property must be present if the I2S device
> +  is a clock provider in terms of the common clock bindings, described in
> +  ../clock/clock-bindings.txt.
> +- clock-output-names: from the common clock bindings, names of the CDCLK
> +  I2S output clocks, suggested values are "i2s_cdclk0", "i2s_cdclk1",
> +  "i2s_cdclk3" for the I2S0, I2S1, I2S2 devices recpectively.
> +

Why not make this mandatory going forwards?  You can add a note saying
that older DTs may not have it.

> +The assignment of indices for the I2Sx clock provider is as follows:
> + 0 - the CDCLK (CODECLKO) gate clock,
> + 1 - the RCLK prescaler divider clock (corresponding to the IISPSR register),
> + 2 - the RCLKSRC mux clock (corresponding to RCLKSRC bit in register IISMOD).

Why use the indicies here, they only seem to add obscurity?

> +		clk_put(rclksrc);
> +	}
> +	/*

Missing blank line.

> +	 * Register the mux and div clocks only if both source clocks
> +	 * are available, i.e. for the I2S0 instance and versions with
> +	 * QUIRK_NO_MUXPSR quirk not set.
> +	 */

Why not proceed even if one is missing?  This repeats in words the if
statement but doesn't explain why we're doing this.

> +	ret = of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
> +				  &i2s->clk_data);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to add clock provider\n");

Best practice is to print the error code.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [alsa-devel] [PATCH/RFC 12/14] ASoC: samsung: i2s: Add clock provider for the I2S internal clocks
  2014-12-12 20:03   ` [alsa-devel] " Mark Brown
@ 2014-12-17 17:00     ` Sylwester Nawrocki
  0 siblings, 0 replies; 5+ messages in thread
From: Sylwester Nawrocki @ 2014-12-17 17:00 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, linux-samsung-soc, devicetree, padma.v, sbkim73,
	drake

On 12/12/14 21:03, Mark Brown wrote:
> On Thu, Dec 11, 2014 at 06:45:50PM +0100, Sylwester Nawrocki wrote:
> 
>> +Optional Properties:
>>
>>  - samsung,idma-addr: Internal DMA register base address of the audio
>>    sub system(used in secondary sound source).
>>  - pinctrl-0: Should specify pin control groups used for this controller.
>>  - pinctrl-names: Should contain only one value - "default".
>> +- #clock-cells: should be 1, this property must be present if the I2S device
>> +  is a clock provider in terms of the common clock bindings, described in
>> +  ../clock/clock-bindings.txt.
>> +- clock-output-names: from the common clock bindings, names of the CDCLK
>> +  I2S output clocks, suggested values are "i2s_cdclk0", "i2s_cdclk1",
>> +  "i2s_cdclk3" for the I2S0, I2S1, I2S2 devices recpectively.
>> +
> 
> Why not make this mandatory going forwards?  You can add a note saying
> that older DTs may not have it.

I guess that makes more sense, I'll move it to the mandatory properties
section.

>> +The assignment of indices for the I2Sx clock provider is as follows:
>> + 0 - the CDCLK (CODECLKO) gate clock,
>> + 1 - the RCLK prescaler divider clock (corresponding to the IISPSR register),
>> + 2 - the RCLKSRC mux clock (corresponding to RCLKSRC bit in register IISMOD).
> 
> Why use the indicies here, they only seem to add obscurity?

I didn't want to create a separate header file, with all the GPL copyright
notice header etc, for just this enumeration.
Anyway I'll put these as macro definitions into include/dt-bindings/sound
/samsung-i2s.h.

>> +		clk_put(rclksrc);
>> +	}
>> +	/*
> 
> Missing blank line.
> 
>> +	 * Register the mux and div clocks only if both source clocks
>> +	 * are available, i.e. for the I2S0 instance and versions with
>> +	 * QUIRK_NO_MUXPSR quirk not set.
>> +	 */
> 
> Why not proceed even if one is missing?  This repeats in words the if
> statement but doesn't explain why we're doing this.

Right, I should have said here that this is really to distinguish between
I2S0 and I2S1, I2S2. So we register the mux and div clocks only for
the IIS Multi Audio Interface (I2S0) and not for the IIS Bus Interface
(I2S1, I2S2) instances.
However, it seems I got confused by same compatible string set for
I2S0..I2S2 in arch/arm/boot/dts/exynos4.dtsi - "samsung,s5pv210-i2s",
where for I2S1, I2S2 it should be "samsung,s3c6410-i2s". So we have,
for example, QUIRK_NO_MUXPSR set for them. There seems to be no similar
bug in arch/arm/boot/dts/exynos5250.dtsi.
I'll drop that mux input clock test the and fix the compatible strings
for exynos4 in dts.

>> +	ret = of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
>> +				  &i2s->clk_data);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to add clock provider\n");
> 
> Best practice is to print the error code.

Yeah, I'll add it.

--
Regards,
Sylwester

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2014-12-17 17:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <1418319952-10163-1-git-send-email-s.nawrocki@samsung.com>
2014-12-11 17:45 ` [PATCH/RFC 12/14] ASoC: samsung: i2s: Add clock provider for the I2S internal clocks Sylwester Nawrocki
2014-12-12 20:03   ` [alsa-devel] " Mark Brown
2014-12-17 17:00     ` Sylwester Nawrocki
2014-12-11 17:45 ` [PATCH/RFC 13/14] ARM: dts: Exynos4 and Odroid X2/U3 sound device nodes update Sylwester Nawrocki
2014-12-11 17:45 ` [PATCH/RFC 14/14] ARM: dts: Switch Odroid X2/U2 to simple-audio-card Sylwester Nawrocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).