Hi,<br><br><div class="gmail_quote">On Tue, Oct 9, 2012 at 11:58 PM, Olof Johansson <span dir="ltr"><<a href="mailto:olof@lixom.net" target="_blank">olof@lixom.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<div class="im"><br>
<br>
On Tue, Oct 09, 2012 at 05:18:48PM +0530, Vasanth Ananthan wrote:<br>
> This patch adds Device Nodes for SATA and SATA PHY device.<br>
><br>
> Signed-off-by: Vasanth Ananthan <<a href="mailto:vasanth.a@samsung.com">vasanth.a@samsung.com</a>><br>
> ---<br>
> arch/arm/boot/dts/exynos5250-smdk5250.dts | 11 +++++++++++<br>
> arch/arm/boot/dts/exynos5250.dtsi | 20 ++++++++++++++++++++<br>
> arch/arm/mach-exynos/include/mach/map.h | 7 +++++++<br>
> arch/arm/mach-exynos/mach-exynos5-dt.c | 6 ++++++<br>
> 4 files changed, 44 insertions(+), 0 deletions(-)<br>
><br>
> diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts<br>
> index 8a5e348..bb262ce 100644<br>
> --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts<br>
> +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts<br>
> @@ -48,6 +48,17 @@<br>
> };<br>
> };<br>
><br>
> + i2c@121D0000 {<br>
> + samsung,i2c-sda-delay = <100>;<br>
> + samsung,i2c-max-bus-freq = <40000>;<br>
> + samsung,i2c-slave-addr = <0x38>;<br>
<br>
</div>Whitespace is off above.<br>
<br>
> +<br>
> + sataphy@70 {<br>
<br>
sata-phy would be a more conventional name.<br>
<br>
> + compatible = "samsung,i2c-phy";<br>
<br>
i2c-phy? Seems like an odd choice of name. What is this device?<br></blockquote><div><br>The SATA physical layer controller is both a platform device and a i2c slave device.<br>This compatible string is for the i2c client driver. Hence I have used i2c-phy here.<br>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + reg = <0x38>;<br>
<br>
70 is unit address but here it's 0x38? One of them is wrong. No need for a unit<br>
address if it's a unique name, by the way.<br>
<div class="im"><br>
<br>
> + };<br>
> + };<br>
> +<br>
> i2c@12C80000 {<br>
> status = "disabled";<br>
> };<br>
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi<br>
> index 004aaa8..5a47a8f 100644<br>
> --- a/arch/arm/boot/dts/exynos5250.dtsi<br>
> +++ b/arch/arm/boot/dts/exynos5250.dtsi<br>
> @@ -88,6 +88,18 @@<br>
> interrupts = <0 54 0>;<br>
> };<br>
><br>
> + sata@122F0000 {<br>
> + compatible = "samsung,exynos-sata-ahci";<br>
> + reg = <0x122F0000 0x1ff>;<br>
> + interrupts = <0 115 0>;<br>
<br>
</div>More whitespace damage. And need binding.<br>
<div class="im"><br>
> + };<br>
> +<br>
> + sata-phy@12170000 {<br>
> + compatible = "samsung,exynos-sata-phy";<br>
> + reg = <0x12170000 0x1ff>;<br>
> + };<br>
<br>
</div>Should have binding too. How does this relate to the i2c device above.<br></blockquote><div><br>As mentioned earlier SATA physical layer controller is both a platform device and also an i2c slave device. <br>This Node is for the SATA physical layer platform device, the previous node is for i2c slave device.<br>
Certain initialization settings done directly and other settings has to be done through i2c. <br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> +<br>
> i2c@12C60000 {<br>
> compatible = "samsung,s3c2440-i2c";<br>
> reg = <0x12C60000 0x100>;<br>
> @@ -152,6 +164,13 @@<br>
> #size-cells = <0>;<br>
> };<br>
><br>
> + i2c@121D0000 {<br>
> + compatible = "samsung,s3c2440-sataphy-i2c";<br>
<br>
</div>Is this a unique i2c controller, or is it just another one like the others on<br>
the chip? If it's the latter, it should use the regular compatible string.<br></blockquote><div><br>Yes, its a unique i2c controller which lacks an interrupt line while others are interrupt driven. <br>Hence I have used a distinct compatible string for the driver to distinguish the controller. <br>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> + reg = <0x121D0000 0x100>;<br>
> + #address-cells = <1>;<br>
> + #size-cells = <0>;<br>
> + };<br>
> +<br>
> spi_0: spi@12d20000 {<br>
> compatible = "samsung,exynos4210-spi";<br>
> reg = <0x12d20000 0x100>;<br>
> @@ -460,4 +479,5 @@<br>
> #gpio-cells = <4>;<br>
> };<br>
> };<br>
> +<br>
<br>
</div>Stray whitespace change.<br>
<div class="im"><br>
> };<br>
> diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h<br>
> index c72b675..6827190 100644<br>
> --- a/arch/arm/mach-exynos/include/mach/map.h<br>
> +++ b/arch/arm/mach-exynos/include/mach/map.h<br>
> @@ -177,9 +177,16 @@<br>
> #define EXYNOS4_PA_HSOTG 0x12480000<br>
> #define EXYNOS4_PA_USB_HSPHY 0x125B0000<br>
><br>
> +#ifdef CONFIG_ARCH_EXYNOS4<br>
<br>
</div>No need to ifdef since namespace isn't overlapped.<br>
<div class="im"><br>
> #define EXYNOS4_PA_SATA 0x12560000<br>
> #define EXYNOS4_PA_SATAPHY 0x125D0000<br>
> #define EXYNOS4_PA_SATAPHY_CTRL 0x126B0000<br>
> +#endif<br>
> +#ifdef CONFIG_ARCH_EXYNOS5<br>
<br>
</div>Same here.<br>
<div class="HOEnZb"><div class="h5"><br>
> +#define EXYNOS5_PA_SATA_PHY_CTRL 0x12170000<br>
> +#define EXYNOS5_PA_SATA_PHY_I2C 0x121D0000<br>
> +#define EXYNOS5_PA_SATA_BASE 0x122F0000<br>
> +#endif<br>
<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">-Olof<br>
</font></span></blockquote></div><br>I will incorporate the other comments and resubmit the patch. Thanks.<br clear="all"><br>-- <br>Vasanth Ananthan.<br>