[PATCH 1/3 v3] ARM: shmobile: r8a7778: add I2C support

[PATCH 1/3 v3] ARM: shmobile: r8a7778: add I2C support

[Kuninori Morimoto]

Add a platform device for the r8a7778 I2C.

Signed-off-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v2 -> v3

 - __initdata was added

 arch/arm/mach-shmobile/clock-r8a7778.c        |   14 +++++++++++++-
 arch/arm/mach-shmobile/include/mach/r8a7778.h |    2 ++
 arch/arm/mach-shmobile/setup-r8a7778.c        |   25 +++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-shmobile/clock-r8a7778.c b/arch/arm/mach-shmobile/clock-r8a7778.c
index b251e4d..1386c59 100644
--- a/arch/arm/mach-shmobile/clock-r8a7778.c
+++ b/arch/arm/mach-shmobile/clock-r8a7778.c
@@ -105,7 +105,8 @@ static struct clk *main_clks[] = {
 enum {
 	MSTP323, MSTP322, MSTP321,
 	MSTP114,
-	MSTP026, MSTP025, MSTP024, MSTP023, MSTP022, MSTP021,
+	MSTP030,
+	MSTP029, MSTP028, MSTP027, MSTP026, MSTP025, MSTP024, MSTP023, MSTP022, MSTP021,
 	MSTP016, MSTP015,
 	MSTP_NR };
 
@@ -114,6 +115,10 @@ static struct clk mstp_clks[MSTP_NR] = {
 	[MSTP322] = SH_CLK_MSTP32(&p_clk, MSTPCR3, 22, 0), /* SDHI1 */
 	[MSTP321] = SH_CLK_MSTP32(&p_clk, MSTPCR3, 21, 0), /* SDHI2 */
 	[MSTP114] = SH_CLK_MSTP32(&p_clk, MSTPCR1, 14, 0), /* Ether */
+	[MSTP030] = SH_CLK_MSTP32(&p_clk, MSTPCR0, 30, 0), /* I2C0 */
+	[MSTP029] = SH_CLK_MSTP32(&p_clk, MSTPCR0, 29, 0), /* I2C1 */
+	[MSTP028] = SH_CLK_MSTP32(&p_clk, MSTPCR0, 28, 0), /* I2C2 */
+	[MSTP027] = SH_CLK_MSTP32(&p_clk, MSTPCR0, 27, 0), /* I2C3 */
 	[MSTP026] = SH_CLK_MSTP32(&p_clk, MSTPCR0, 26, 0), /* SCIF0 */
 	[MSTP025] = SH_CLK_MSTP32(&p_clk, MSTPCR0, 25, 0), /* SCIF1 */
 	[MSTP024] = SH_CLK_MSTP32(&p_clk, MSTPCR0, 24, 0), /* SCIF2 */
@@ -125,11 +130,18 @@ static struct clk mstp_clks[MSTP_NR] = {
 };
 
 static struct clk_lookup lookups[] = {
+	/* main */
+	CLKDEV_CON_ID("peripheral_clk",	&p_clk),
+
 	/* MSTP32 clocks */
 	CLKDEV_DEV_ID("sh_mobile_sdhi.0", &mstp_clks[MSTP323]), /* SDHI0 */
 	CLKDEV_DEV_ID("sh_mobile_sdhi.1", &mstp_clks[MSTP322]), /* SDHI1 */
 	CLKDEV_DEV_ID("sh_mobile_sdhi.2", &mstp_clks[MSTP321]), /* SDHI2 */
 	CLKDEV_DEV_ID("sh-eth",	&mstp_clks[MSTP114]), /* Ether */
+	CLKDEV_DEV_ID("i2c-rcar.0", &mstp_clks[MSTP030]), /* I2C0 */
+	CLKDEV_DEV_ID("i2c-rcar.1", &mstp_clks[MSTP029]), /* I2C1 */
+	CLKDEV_DEV_ID("i2c-rcar.2", &mstp_clks[MSTP028]), /* I2C2 */
+	CLKDEV_DEV_ID("i2c-rcar.3", &mstp_clks[MSTP027]), /* I2C3 */
 	CLKDEV_DEV_ID("sh-sci.0", &mstp_clks[MSTP026]), /* SCIF0 */
 	CLKDEV_DEV_ID("sh-sci.1", &mstp_clks[MSTP025]), /* SCIF1 */
 	CLKDEV_DEV_ID("sh-sci.2", &mstp_clks[MSTP024]), /* SCIF2 */
diff --git a/arch/arm/mach-shmobile/include/mach/r8a7778.h b/arch/arm/mach-shmobile/include/mach/r8a7778.h
index ae65b45..a428bc6 100644
--- a/arch/arm/mach-shmobile/include/mach/r8a7778.h
+++ b/arch/arm/mach-shmobile/include/mach/r8a7778.h
@@ -24,6 +24,8 @@
 extern void r8a7778_add_standard_devices(void);
 extern void r8a7778_add_standard_devices_dt(void);
 extern void r8a7778_add_ether_device(struct sh_eth_plat_data *pdata);
+extern void r8a7778_add_i2c_device(int id);
+
 extern void r8a7778_init_delay(void);
 extern void r8a7778_init_irq(void);
 extern void r8a7778_init_irq_dt(void);
diff --git a/arch/arm/mach-shmobile/setup-r8a7778.c b/arch/arm/mach-shmobile/setup-r8a7778.c
index 9191acc..eaa6b06 100644
--- a/arch/arm/mach-shmobile/setup-r8a7778.c
+++ b/arch/arm/mach-shmobile/setup-r8a7778.c
@@ -173,6 +173,31 @@ void __init r8a7778_sdhi_init(int id,
 		info, sizeof(*info));
 }
 
+/* I2C */
+static struct resource i2c_resources[] __initdata = {
+	/* I2C0 */
+	DEFINE_RES_MEM(0xffc70000, 0x1000),
+	DEFINE_RES_IRQ(gic_iid(0x63)),
+	/* I2C1 */
+	DEFINE_RES_MEM(0xffc71000, 0x1000),
+	DEFINE_RES_IRQ(gic_iid(0x6e)),
+	/* I2C2 */
+	DEFINE_RES_MEM(0xffc72000, 0x1000),
+	DEFINE_RES_IRQ(gic_iid(0x6c)),
+	/* I2C3 */
+	DEFINE_RES_MEM(0xffc73000, 0x1000),
+	DEFINE_RES_IRQ(gic_iid(0x6d)),
+};
+
+void __init r8a7778_add_i2c_device(int id)
+{
+	BUG_ON(id < 0 || id > 3);
+
+	platform_device_register_simple(
+		"i2c-rcar", id,
+		i2c_resources + (2 * id), 2);
+}
+
 void __init r8a7778_add_standard_devices(void)
 {
 	int i;
-- 
1.7.9.5

Re: [PATCH 1/3 v3] ARM: shmobile: r8a7778: add I2C support

[Sergei Shtylyov]

Hello.

On 05/28/2013 08:22 AM, Kuninori Morimoto wrote:

> Add a platform device for the r8a7778 I2C.

    You're also adding several clocks and not mentioning it.
Please just copy the description from our I2C patch (to be dumped now),
I'm serious.

> Signed-off-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

WBR, Sergei

Re: [PATCH 1/3 v3] ARM: shmobile: r8a7778: add I2C support

[Sergei Shtylyov]

Hello.

On 05/28/2013 08:22 AM, Kuninori Morimoto wrote:

> Add a platform device for the r8a7778 I2C.
>
> Signed-off-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
[...]
> diff --git a/arch/arm/mach-shmobile/setup-r8a7778.c b/arch/arm/mach-shmobile/setup-r8a7778.c
> index 9191acc..eaa6b06 100644
> --- a/arch/arm/mach-shmobile/setup-r8a7778.c
> +++ b/arch/arm/mach-shmobile/setup-r8a7778.c
> @@ -173,6 +173,31 @@ void __init r8a7778_sdhi_init(int id,
[...]
> +void __init r8a7778_add_i2c_device(int id)
> +{
> +	BUG_ON(id < 0 || id > 3);
> +
> +	platform_device_register_simple(
> +		"i2c-rcar", id,
> +		i2c_resources + (2 * id), 2);

     I don't understand why are you wrapping the lines so early here 
(this remark
relates to all the series). This is not a standard way to indent the 
function call
continuation lines, normally you should align them to the next character 
after (.

WBR, Sergei

Re: [PATCH 1/3 v3] ARM: shmobile: r8a7778: add I2C support

[Simon Horman]

On Sun, Jun 02, 2013 at 09:16:27PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 05/28/2013 08:22 AM, Kuninori Morimoto wrote:
> 
> >Add a platform device for the r8a7778 I2C.
> 
>    You're also adding several clocks and not mentioning it.
> Please just copy the description from our I2C patch (to be dumped now),

It seems that the arm-soc people would like a separate clocks branch.
So would it be possible to split the patch into a clocks and a non-clocks
patch, each with an appropriate changelog?

> 
> >Signed-off-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
> >Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> WBR, Sergei
> 

Re: [PATCH 1/3 v3] ARM: shmobile: r8a7778: add I2C support

[Kuninori Morimoto]

Hi Simon

Thank you for your reply

> > >Add a platform device for the r8a7778 I2C.
> > 
> >    You're also adding several clocks and not mentioning it.
> > Please just copy the description from our I2C patch (to be dumped now),
> 
> It seems that the arm-soc people would like a separate clocks branch.
> So would it be possible to split the patch into a clocks and a non-clocks
> patch, each with an appropriate changelog?

I understand. will do soon.

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/3 v3] ARM: shmobile: r8a7778: add I2C support

[Simon Horman]

On Tue, Jun 04, 2013 at 11:28:56AM +0900, Simon Horman wrote:
> On Sun, Jun 02, 2013 at 09:16:27PM +0400, Sergei Shtylyov wrote:
> > Hello.
> > 
> > On 05/28/2013 08:22 AM, Kuninori Morimoto wrote:
> > 
> > >Add a platform device for the r8a7778 I2C.
> > 
> >    You're also adding several clocks and not mentioning it.
> > Please just copy the description from our I2C patch (to be dumped now),
> 
> It seems that the arm-soc people would like a separate clocks branch.
> So would it be possible to split the patch into a clocks and a non-clocks
> patch, each with an appropriate changelog?

Scratch that, but could you enhance the changelog anyway?

> > >Signed-off-by: Yusuke Goda <yusuke.goda.sx@renesas.com>
> > >Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > WBR, Sergei
> > 

[PATCH 1/3 v3] ARM: shmobile: r8a7778: add usb phy power control function

[Kuninori Morimoto]

USB phy initialisation function is needed from not only
USB Host but also USB Function too.
This patch adds usb phy common control function.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v2 -> v3
 - no change

 arch/arm/mach-shmobile/include/mach/r8a7778.h |    2 ++
 arch/arm/mach-shmobile/setup-r8a7778.c        |   37 +++++++++++++++++--------
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-shmobile/include/mach/r8a7778.h b/arch/arm/mach-shmobile/include/mach/r8a7778.h
index 41fd6da..b5706fb 100644
--- a/arch/arm/mach-shmobile/include/mach/r8a7778.h
+++ b/arch/arm/mach-shmobile/include/mach/r8a7778.h
@@ -32,4 +32,6 @@ extern void r8a7778_clock_init(void);
 extern void r8a7778_init_irq_extpin(int irlm);
 extern void r8a7778_pinmux_init(void);
 
+extern int r8a7778_usb_phy_power(bool enable);
+
 #endif /* __ASM_R8A7778_H__ */
diff --git a/arch/arm/mach-shmobile/setup-r8a7778.c b/arch/arm/mach-shmobile/setup-r8a7778.c
index 1a154d4..b65044a 100644
--- a/arch/arm/mach-shmobile/setup-r8a7778.c
+++ b/arch/arm/mach-shmobile/setup-r8a7778.c
@@ -95,29 +95,46 @@ static struct sh_timer_config sh_tmu1_platform_data __initdata = {
 		&sh_tmu##idx##_platform_data,		\
 		sizeof(sh_tmu##idx##_platform_data))
 
-/* USB */
-static struct usb_phy *phy;
+int r8a7778_usb_phy_power(bool enable)
+{
+	static struct usb_phy *phy = NULL;
+	int ret = 0;
+
+	if (!phy)
+		phy = usb_get_phy(USB_PHY_TYPE_USB2);
+
+	if (IS_ERR(phy)) {
+		pr_err("it doesn't have usb phy driver\n");
+		return PTR_ERR(phy);
+	}
+
+	if (enable)
+		ret = usb_phy_init(phy);
+	else
+		usb_phy_shutdown(phy);
 
+	return ret;
+}
+
+/* USB */
 static int usb_power_on(struct platform_device *pdev)
 {
-	if (IS_ERR(phy))
-		return PTR_ERR(phy);
+	int ret = r8a7778_usb_phy_power(true);
+
+	if (ret)
+		return ret;
 
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
-	usb_phy_init(phy);
-
 	return 0;
 }
 
 static void usb_power_off(struct platform_device *pdev)
 {
-	if (IS_ERR(phy))
+	if (r8a7778_usb_phy_power(false))
 		return;
 
-	usb_phy_shutdown(phy);
-
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 }
@@ -319,8 +336,6 @@ void __init r8a7778_add_standard_devices(void)
 
 void __init r8a7778_init_late(void)
 {
-	phy = usb_get_phy(USB_PHY_TYPE_USB2);
-
 	platform_device_register_full(&ehci_info);
 	platform_device_register_full(&ohci_info);
 }
-- 
1.7.9.5

Re: [PATCH 1/3 v3] ARM: shmobile: r8a7778: add usb phy power control function

[Sergei Shtylyov]

Hello.

On 02-08-2013 5:02, Kuninori Morimoto wrote:

> USB phy initialisation function is needed from not only
> USB Host but also USB Function too.
> This patch adds usb phy common control function.

> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
[...]

> diff --git a/arch/arm/mach-shmobile/setup-r8a7778.c b/arch/arm/mach-shmobile/setup-r8a7778.c
> index 1a154d4..b65044a 100644
> --- a/arch/arm/mach-shmobile/setup-r8a7778.c
> +++ b/arch/arm/mach-shmobile/setup-r8a7778.c
> @@ -95,29 +95,46 @@ static struct sh_timer_config sh_tmu1_platform_data __initdata = {
>   		&sh_tmu##idx##_platform_data,		\
>   		sizeof(sh_tmu##idx##_platform_data))
>
> -/* USB */
> -static struct usb_phy *phy;
> +int r8a7778_usb_phy_power(bool enable)
> +{
> +	static struct usb_phy *phy = NULL;
> +	int ret = 0;
> +
> +	if (!phy)
> +		phy = usb_get_phy(USB_PHY_TYPE_USB2);
> +
> +	if (IS_ERR(phy)) {
> +		pr_err("it doesn't have usb phy driver\n");

    I would still like this message corrected.

WBR, Sergei

Re: [PATCH 1/3 v3] ARM: shmobile: r8a7778: add usb phy power control function

[Ben Dooks]

On 02/08/13 13:17, Sergei Shtylyov wrote:
> Hello.
>
> On 02-08-2013 5:02, Kuninori Morimoto wrote:
>
>> USB phy initialisation function is needed from not only
>> USB Host but also USB Function too.
>> This patch adds usb phy common control function.
>
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> [...]
>
>> diff --git a/arch/arm/mach-shmobile/setup-r8a7778.c
>> b/arch/arm/mach-shmobile/setup-r8a7778.c
>> index 1a154d4..b65044a 100644
>> --- a/arch/arm/mach-shmobile/setup-r8a7778.c
>> +++ b/arch/arm/mach-shmobile/setup-r8a7778.c
>> @@ -95,29 +95,46 @@ static struct sh_timer_config
>> sh_tmu1_platform_data __initdata = {
>> &sh_tmu##idx##_platform_data, \
>> sizeof(sh_tmu##idx##_platform_data))
>>
>> -/* USB */
>> -static struct usb_phy *phy;
>> +int r8a7778_usb_phy_power(bool enable)
>> +{
>> + static struct usb_phy *phy = NULL;
>> + int ret = 0;
>> +
>> + if (!phy)
>> + phy = usb_get_phy(USB_PHY_TYPE_USB2);
>> +
>> + if (IS_ERR(phy)) {
>> + pr_err("it doesn't have usb phy driver\n");
>
> I would still like this message corrected.

Yes, something like "cannot get usb2 phy driver"

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH 1/3 v3] ARM: shmobile: r8a7778: add usb phy power control function

[Kuninori Morimoto]

Hi Sergei, Ben

Thank you for your reply

> >> + if (IS_ERR(phy)) {
> >> + pr_err("it doesn't have usb phy driver\n");
> >
> > I would still like this message corrected.
> 
> Yes, something like "cannot get usb2 phy driver"

Thank you for re-pointing it
I will fix it and send v4 patch soon

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/3 v3] ARM: shmobile: r8a7778: add usb phy power control function

[Kuninori Morimoto]

Hi again

> > >> + if (IS_ERR(phy)) {
> > >> + pr_err("it doesn't have usb phy driver\n");
> > >
> > > I would still like this message corrected.
> > 
> > Yes, something like "cannot get usb2 phy driver"
> 
> Thank you for re-pointing it
> I will fix it and send v4 patch soon

int r8a7778_usb_phy_power(bool enable)
{
	static struct usb_phy *phy = NULL;
	int ret = 0;

	if (!phy)
		phy = usb_get_phy(USB_PHY_TYPE_USB2);

	if (IS_ERR(phy)) {
		pr_err("it doesn't have usb phy driver\n");
		return PTR_ERR(phy);
	}
	...
}

This r8a7778_usb_phy_power() can be called many times on r8a7778.
Then, usb_get_phy() line is called only 1 time,
(because, "phy" is static, and default was NULL)
but, IS_ERR() check will be called many times.

So, for me, error messages like

cannot get usb2 phy driver
cannot get usb2 phy driver
cannot get usb2 phy driver
cannot get usb2 phy driver

are miss leading user.

it doesn't have usb phy driver
it doesn't have usb phy driver
it doesn't have usb phy driver
it doesn't have usb phy driver

are reasonable I think, but yes, this "it" is unclear word here

Kernel doesn't have usb phy driver
Kernel doesn't have usb phy driver
Kernel doesn't have usb phy driver
Kernel doesn't have usb phy driver

I will use "Kernel" here

Thankk you

Best regards
---
Kuninori Morimoto