| Message ID | 1376302070-1877-2-git-send-email-padma.v@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Padmavathi, Andrew, On Monday 12 of August 2013 15:37:47 Padmavathi Venna wrote: > From: Andrew Bresticker <abrestic@chromium.org> > > This adds device-tree bindings for the i2s controllers on Exynos 5420. > > Signed-off-by: Andrew Bresticker <abrestic@chromium.org> > Signed-off-by: Padmavathi Venna <padma.v@samsung.com> > Reviewed-on: https://gerrit.chromium.org/gerrit/57713 > --- > arch/arm/boot/dts/exynos5420.dtsi | 32 > ++++++++++++++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 > deletions(-) > > diff --git a/arch/arm/boot/dts/exynos5420.dtsi > b/arch/arm/boot/dts/exynos5420.dtsi index d2fdb87..8d57369 100644 > --- a/arch/arm/boot/dts/exynos5420.dtsi > +++ b/arch/arm/boot/dts/exynos5420.dtsi > @@ -242,4 +242,36 @@ > pinctrl-names = "default"; > pinctrl-0 = <&i2c3_bus>; > }; > + > + i2s_0: i2s@03830000 { > + compatible = "samsung,exynos5420-i2s"; > + dmas = <&adma 0 > + &adma 2 > + &adma 1>; > + dma-names = "tx", "rx", "tx-sec"; > + clocks = <&clock_audss EXYNOS_I2S_BUS>, > + <&clock_audss EXYNOS_I2S_BUS>, > + <&clock_audss EXYNOS_SCLK_I2S>; > + clock-names = "iis", "i2s_opclk0", "i2s_opclk1"; > + pinctrl-names = "default"; > + pinctrl-0 = <&i2s0_bus>; > + 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. > + }; > + > + i2s_1: i2s@12D60000 { > + clocks = <&clock 275>, <&clock 138>; > + clock-names = "iis", "i2s_opclk0"; > + pinctrl-names = "default"; > + pinctrl-0 = <&i2s1_bus>; > + status = "disabled"; Ditto. > + }; > + > + i2s_2: i2s@12D70000 { > + clocks = <&clock 276>, <&clock 139>; > + clock-names = "iis", "i2s_opclk0"; > + pinctrl-names = "default"; > + pinctrl-0 = <&i2s2_bus>; > + status = "disabled"; Ditto. Best regards, Tomasz
On Mon, Aug 12, 2013 at 01:14:20PM +0200, Tomasz Figa wrote: > On Monday 12 of August 2013 15:37:47 Padmavathi Venna wrote: > > + i2s_0: i2s@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?
On Monday 12 of August 2013 12:34:48 Mark Brown wrote: > On Mon, Aug 12, 2013 at 01:14:20PM +0200, Tomasz Figa wrote: > > On Monday 12 of August 2013 15:37:47 Padmavathi Venna wrote: > > > + i2s_0: i2s@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? Ah, good point. Now I wonder whether pinctrl nodes shouldn't be considered board-specific and specified in board-level dts instead? Best regards, Tomasz
On Mon, Aug 12, 2013 at 01:41:23PM +0200, Tomasz Figa wrote: > 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.
On Monday 12 of August 2013 14:12:36 Mark Brown wrote: > On Mon, Aug 12, 2013 at 01:41:23PM +0200, Tomasz Figa wrote: > > 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. 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 <t.figa@samsung.com> Best regards, Tomasz
Hi Tomasz, On Mon, Aug 12, 2013 at 10:32 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On Monday 12 of August 2013 14:12:36 Mark Brown wrote: >> On Mon, Aug 12, 2013 at 01:41:23PM +0200, Tomasz Figa wrote: >> > 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. > > Right, if this SoC has only one pinmux setting for this IP, then it's > fine. Yes. This IP has only default pin configuration. > > Padmavathi, this was the only issue I spotted, so have my: > > Reviewed-by: Tomasz Figa <t.figa@samsung.com> Thanks for your review. > > Best regards, > Tomasz > Best Regards, Padma
On Wednesday 14 of August 2013 11:21:40 Padma Venkat wrote: > Hi Tomasz, > > On Mon, Aug 12, 2013 at 10:32 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > > On Monday 12 of August 2013 14:12:36 Mark Brown wrote: > >> On Mon, Aug 12, 2013 at 01:41:23PM +0200, Tomasz Figa wrote: > >> > 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. > > > > Right, if this SoC has only one pinmux setting for this IP, then it's > > fine. > > Yes. This IP has only default pin configuration. > > > Padmavathi, this was the only issue I spotted, so have my: > > > > Reviewed-by: Tomasz Figa <t.figa@samsung.com> > > Thanks for your review. You're welcome. Thanks for keeping up with this series and addressing all the comments. :) Best regards, Tomasz
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi index d2fdb87..8d57369 100644 --- a/arch/arm/boot/dts/exynos5420.dtsi +++ b/arch/arm/boot/dts/exynos5420.dtsi @@ -242,4 +242,36 @@ pinctrl-names = "default"; pinctrl-0 = <&i2c3_bus>; }; + + i2s_0: i2s@03830000 { + compatible = "samsung,exynos5420-i2s"; + dmas = <&adma 0 + &adma 2 + &adma 1>; + dma-names = "tx", "rx", "tx-sec"; + clocks = <&clock_audss EXYNOS_I2S_BUS>, + <&clock_audss EXYNOS_I2S_BUS>, + <&clock_audss EXYNOS_SCLK_I2S>; + clock-names = "iis", "i2s_opclk0", "i2s_opclk1"; + pinctrl-names = "default"; + pinctrl-0 = <&i2s0_bus>; + status = "disabled"; + }; + + i2s_1: i2s@12D60000 { + clocks = <&clock 275>, <&clock 138>; + clock-names = "iis", "i2s_opclk0"; + pinctrl-names = "default"; + pinctrl-0 = <&i2s1_bus>; + status = "disabled"; + }; + + i2s_2: i2s@12D70000 { + clocks = <&clock 276>, <&clock 139>; + clock-names = "iis", "i2s_opclk0"; + pinctrl-names = "default"; + pinctrl-0 = <&i2s2_bus>; + status = "disabled"; + }; + };