Add i2s nodes on Exynos5420 and enable sound support on sdmk5420

From: Andrew Bresticker <[email protected]>

This adds device-tree bindings for the i2s controllers on Exynos 5420.

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Padmavathi Venna <[email protected]>

Reviewed-on: https://gerrit.chromium.org/gerrit/57713

---
 arch/arm/boot/dts/exynos5420.dtsi |   32

++++++++++++++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/arch/arm/boot/dts/exynos5420.dtsi index d2fdb87..8d57369 100644

arch/arm/boot/dts/exynos5420.dtsi +4 -0

@@ -242,4 +242,36 @@
 		pinctrl-names = "default";
 		pinctrl-0 = <&i2c3_bus>;
 	};
+
+	i2s_0: i2s at 03830000 {
+		compatible = "samsung,exynos5420-i2s";
+		dmas = <&adma 0

Review

Quoted 2 lines · On Monday 12 of August 2013 15:37:47 Padmavathi Venna wrote:

(quoted diff) +2 -0

+	i2s_0: i2s at 03830000 {
+		status = "disabled";

If a node does not require any board-specific properties for the device to operate properly, there is no point in disabling it, just to add a single status property at board level.

Quoted 7 lines · Tomasz Figa

Review

Quoted 2 lines · On Monday 12 of August 2013 15:37:47 Padmavathi Venna wrote:

(quoted diff) +2 -0

+	i2s_0: i2s at 03830000 {
+		status = "disabled";

If a node does not require any board-specific properties for the device to operate properly, there is no point in disabling it, just to add a single status property at board level.

I'd expect that to interact badly with the pinmuxing - unless the device is disabled it'll try to grab its pins on probe which is not going to be a good idea unless it is actually wired up for use in the system. Or is there some other mechanism for handling that?

Quoted 4 lines · On Monday 12 of August 2013 12:34:48 Mark Brown wrote:

I'd expect that to interact badly with the pinmuxing - unless the device is disabled it'll try to grab its pins on probe which is not going to be a good idea unless it is actually wired up for use in the system. Or is there some other mechanism for handling that?

Ah, good point. Now I wonder whether pinctrl nodes shouldn't be considered board-specific and specified in board-level dts instead?

Quoted 8 lines · Tomasz Figa

Quoted 4 lines · On Monday 12 of August 2013 12:34:48 Mark Brown wrote:

I'd expect that to interact badly with the pinmuxing - unless the device is disabled it'll try to grab its pins on probe which is not going to be a good idea unless it is actually wired up for use in the system. Or is there some other mechanism for handling that?

Ah, good point. Now I wonder whether pinctrl nodes shouldn't be considered board-specific and specified in board-level dts instead?

It seems a bit cleaner to use the current mechanism in that it stops the device appearing at all and hence repeated efforts to probe, plus a simple enable is less error prone, the way these SoCs are designed you don't have to pick which pinmux is in use for most of the IPs. Where there are multiple options it does seem like a good approach though.

Tastes may differ though.

Quoted 16 lines · On Monday 12 of August 2013 14:12:36 Mark Brown wrote:

Quoted 8 lines · Tomasz Figa

Quoted 4 lines · On Monday 12 of August 2013 12:34:48 Mark Brown wrote:

I'd expect that to interact badly with the pinmuxing - unless the device is disabled it'll try to grab its pins on probe which is not going to be a good idea unless it is actually wired up for use in the system. Or is there some other mechanism for handling that?

Ah, good point. Now I wonder whether pinctrl nodes shouldn't be considered board-specific and specified in board-level dts instead?

It seems a bit cleaner to use the current mechanism in that it stops the device appearing at all and hence repeated efforts to probe, plus a simple enable is less error prone, the way these SoCs are designed you don't have to pick which pinmux is in use for most of the IPs. Where there are multiple options it does seem like a good approach though.

Quoted 1 line

Tastes may differ though.

Right, if this SoC has only one pinmux setting for this IP, then it's fine.

Padmavathi, this was the only issue I spotted, so have my:

Reviewed-by Tomasz Figa <[email protected]>

Best regards,
Tomasz

Hi Tomasz,

On Mon, Aug 12, 2013 at 10:32 PM, Tomasz Figa <[email protected]>

Quoted 19 lines · On Monday 12 of August 2013 14:12:36 Mark Brown wrote:

Quoted 18 lines · Tomasz Figa

Quoted 9 lines · On Monday 12 of August 2013 12:34:48 Mark Brown wrote:

Quoted 5 lines

I'd expect that to interact badly with the pinmuxing - unless the device is disabled it'll try to grab its pins on probe which is not
going to be a good idea unless it is actually wired up for use in the system. Or is there some other mechanism for handling that?

Ah, good point. Now I wonder whether pinctrl nodes shouldn't be considered board-specific and specified in board-level dts instead?

It seems a bit cleaner to use the current mechanism in that it stops the device appearing at all and hence repeated efforts to probe, plus a simple enable is less error prone, the way these SoCs are designed you don't have to pick which pinmux is in use for most of the IPs. Where there are multiple options it does seem like a good approach though.

Quoted 4 lines

Quoted 1 line

Tastes may differ though.

Right, if this SoC has only one pinmux setting for this IP, then it's fine.

Yes. This IP has only default pin configuration.

Quoted 3 lines

Padmavathi, this was the only issue I spotted, so have my:

Reviewed-by Tomasz Figa <[email protected]>

Thanks for your review.

This patch enables i2s0 and sound support on smdk5420.

Signed-off-by Padmavathi Venna <[email protected]>

---
 arch/arm/boot/dts/exynos5420-smdk5420.dts |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/arch/arm/boot/dts/exynos5420-smdk5420.dts index a898b3f..b1b745c 100644

Changes since V3:
- used existent fixed-clock binding for registering oscillator clock as fixed rate clock as pointed by Tomasz Figa
- Made some changes in wm8994 regulator nodes as suggested by Tomasz
Figa - Separated out only adding i2s nodes and enabling sound support on smdk5420 into different patch set as they are dependent on on some of already posted but not yet merged i2c, dwmmc, dma and audss clock controller patches on Kukjin for-next branch.

Changes since V2:
- Separated out driver side changes and dts changes in two patch sets
- Added proper names for wm8994 regulators as commented by Mark
- Moved common i2s nodes into the exynos5.dtsi
- Added clock info in wm8994 node as requested by Mark.
- Registered the 16.9MHz oscillator clock as fixed clock in the machine file. Right now no user of this clock but as Mark
requested to add mclk info in wm8994 node, I added this part.

This patch set is dependent on the following i2c, dma and audio subsystem clk controller patches.
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/20077
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/20661
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/20668

This patch set is made based on Kukjin Kim for-next branch.

Andrew Bresticker (1):
ARM: dts: exynos5420: add i2s controllers

Padmavathi Venna (3):
ARM: dts: Add i2c bus 1 and it's audio codec child node on smdk5420
ARM: dts: Add osc clock node on smdk5420.
ARM: dts: Enable sound support on smdk5420

 arch/arm/boot/dts/exynos5420-smdk5420.dts |   81
+++++++++++++++++++++++++++++ arch/arm/boot/dts/exynos5420.dtsi        
|   32 +++++++++++
 2 files changed, 113 insertions(+), 0 deletions(-)