[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Sergei Shtylyov]

Add Ether clock and platform device for R8A7779 SoC; add a function to register
this device with board-specific platform data.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
This patch is atop of the 'next' branch of Simon Horman's renesas.git.

 arch/arm/mach-shmobile/clock-r8a7779.c        |    4 ++-
 arch/arm/mach-shmobile/include/mach/r8a7779.h |    1 
 arch/arm/mach-shmobile/setup-r8a7779.c        |   29 +++++++++++++++++++++++++-
 3 files changed, 32 insertions(+), 2 deletions(-)

Index: renesas/arch/arm/mach-shmobile/clock-r8a7779.c
=================================--- renesas.orig/arch/arm/mach-shmobile/clock-r8a7779.c
+++ renesas/arch/arm/mach-shmobile/clock-r8a7779.c
@@ -93,7 +93,7 @@ static struct clk *main_clks[] = {
 };
 
 enum { MSTP323, MSTP322, MSTP321, MSTP320,
-	MSTP115,
+	MSTP115, MSTP114,
 	MSTP103, MSTP101, MSTP100,
 	MSTP030,
 	MSTP029, MSTP028, MSTP027, MSTP026, MSTP025, MSTP024, MSTP023, MSTP022, MSTP021,
@@ -107,6 +107,7 @@ static struct clk mstp_clks[MSTP_NR] = {
 	[MSTP321] = SH_CLK_MSTP32(&clkp_clk, MSTPCR3, 21, 0), /* SDHI2 */
 	[MSTP320] = SH_CLK_MSTP32(&clkp_clk, MSTPCR3, 20, 0), /* SDHI3 */
 	[MSTP115] = SH_CLK_MSTP32(&clkp_clk, MSTPCR1, 15, 0), /* SATA */
+	[MSTP114] = SH_CLK_MSTP32(&clkp_clk, MSTPCR1, 14, 0), /* Ether */
 	[MSTP103] = SH_CLK_MSTP32(&clks_clk, MSTPCR1,  3, 0), /* DU */
 	[MSTP101] = SH_CLK_MSTP32(&clkp_clk, MSTPCR1,  1, 0), /* USB2 */
 	[MSTP100] = SH_CLK_MSTP32(&clkp_clk, MSTPCR1,  0, 0), /* USB0/1 */
@@ -143,6 +144,7 @@ static struct clk_lookup lookups[] = {
 	/* MSTP32 clocks */
 	CLKDEV_DEV_ID("sata_rcar", &mstp_clks[MSTP115]), /* SATA */
 	CLKDEV_DEV_ID("fc600000.sata", &mstp_clks[MSTP115]), /* SATA w/DT */
+	CLKDEV_DEV_ID("sh-eth", &mstp_clks[MSTP114]), /* Ether */
 	CLKDEV_DEV_ID("ehci-platform.1", &mstp_clks[MSTP101]), /* USB EHCI port2 */
 	CLKDEV_DEV_ID("ohci-platform.1", &mstp_clks[MSTP101]), /* USB OHCI port2 */
 	CLKDEV_DEV_ID("ehci-platform.0", &mstp_clks[MSTP100]), /* USB EHCI port0/1 */
Index: renesas/arch/arm/mach-shmobile/include/mach/r8a7779.h
=================================--- renesas.orig/arch/arm/mach-shmobile/include/mach/r8a7779.h
+++ renesas/arch/arm/mach-shmobile/include/mach/r8a7779.h
@@ -31,6 +31,7 @@ extern void r8a7779_earlytimer_init(void
 extern void r8a7779_add_early_devices(void);
 extern void r8a7779_add_standard_devices(void);
 extern void r8a7779_add_standard_devices_dt(void);
+extern void r8a7779_add_ether_device(void *pdata);
 extern void r8a7779_clock_init(void);
 extern void r8a7779_pinmux_init(void);
 extern void r8a7779_pm_init(void);
Index: renesas/arch/arm/mach-shmobile/setup-r8a7779.c
=================================--- renesas.orig/arch/arm/mach-shmobile/setup-r8a7779.c
+++ renesas/arch/arm/mach-shmobile/setup-r8a7779.c
@@ -1,8 +1,9 @@
 /*
  * r8a7779 processor support
  *
- * Copyright (C) 2011  Renesas Solutions Corp.
+ * Copyright (C) 2011, 2013  Renesas Solutions Corp.
  * Copyright (C) 2011  Magnus Damm
+ * Copyright (C) 2013  Cogent Embedded, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -369,6 +370,25 @@ static struct platform_device i2c3_devic
 	.num_resources	= ARRAY_SIZE(rcar_i2c3_res),
 };
 
+/* Ether */
+static struct resource ether_resources[] = {
+	{
+		.start	= 0xfde00000,
+		.end	= 0xfde003ff,
+		.flags	= IORESOURCE_MEM,
+	}, {
+		.start	= gic_iid(0xb4),
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device ether_device = {
+	.name		= "sh-eth",
+	.id		= -1,
+	.resource	= ether_resources,
+	.num_resources	= ARRAY_SIZE(ether_resources),
+};
+
 static struct resource sata_resources[] = {
 	[0] = {
 		.name	= "rcar-sata",
@@ -428,6 +448,13 @@ void __init r8a7779_add_standard_devices
 			    ARRAY_SIZE(r8a7779_late_devices));
 }
 
+void __init r8a7779_add_ether_device(void *pdata)
+{
+	ether_device.dev.platform_data = pdata;
+
+	platform_device_register(&ether_device);
+}
+
 /* do nothing for !CONFIG_SMP or !CONFIG_HAVE_TWD */
 void __init __weak r8a7779_register_twd(void) { }
 

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Sergei Shtylyov]

Add Ether clock and platform device for R8A7779 SoC; add a function to register
this device with board-specific platform data.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
This patch is atop of the 'next' branch of Simon Horman's renesas.git.

 arch/arm/mach-shmobile/clock-r8a7779.c        |    4 ++-
 arch/arm/mach-shmobile/include/mach/r8a7779.h |    1 
 arch/arm/mach-shmobile/setup-r8a7779.c        |   29 +++++++++++++++++++++++++-
 3 files changed, 32 insertions(+), 2 deletions(-)

Index: renesas/arch/arm/mach-shmobile/clock-r8a7779.c
===================================================================
--- renesas.orig/arch/arm/mach-shmobile/clock-r8a7779.c
+++ renesas/arch/arm/mach-shmobile/clock-r8a7779.c
@@ -93,7 +93,7 @@ static struct clk *main_clks[] = {
 };
 
 enum { MSTP323, MSTP322, MSTP321, MSTP320,
-	MSTP115,
+	MSTP115, MSTP114,
 	MSTP103, MSTP101, MSTP100,
 	MSTP030,
 	MSTP029, MSTP028, MSTP027, MSTP026, MSTP025, MSTP024, MSTP023, MSTP022, MSTP021,
@@ -107,6 +107,7 @@ static struct clk mstp_clks[MSTP_NR] = {
 	[MSTP321] = SH_CLK_MSTP32(&clkp_clk, MSTPCR3, 21, 0), /* SDHI2 */
 	[MSTP320] = SH_CLK_MSTP32(&clkp_clk, MSTPCR3, 20, 0), /* SDHI3 */
 	[MSTP115] = SH_CLK_MSTP32(&clkp_clk, MSTPCR1, 15, 0), /* SATA */
+	[MSTP114] = SH_CLK_MSTP32(&clkp_clk, MSTPCR1, 14, 0), /* Ether */
 	[MSTP103] = SH_CLK_MSTP32(&clks_clk, MSTPCR1,  3, 0), /* DU */
 	[MSTP101] = SH_CLK_MSTP32(&clkp_clk, MSTPCR1,  1, 0), /* USB2 */
 	[MSTP100] = SH_CLK_MSTP32(&clkp_clk, MSTPCR1,  0, 0), /* USB0/1 */
@@ -143,6 +144,7 @@ static struct clk_lookup lookups[] = {
 	/* MSTP32 clocks */
 	CLKDEV_DEV_ID("sata_rcar", &mstp_clks[MSTP115]), /* SATA */
 	CLKDEV_DEV_ID("fc600000.sata", &mstp_clks[MSTP115]), /* SATA w/DT */
+	CLKDEV_DEV_ID("sh-eth", &mstp_clks[MSTP114]), /* Ether */
 	CLKDEV_DEV_ID("ehci-platform.1", &mstp_clks[MSTP101]), /* USB EHCI port2 */
 	CLKDEV_DEV_ID("ohci-platform.1", &mstp_clks[MSTP101]), /* USB OHCI port2 */
 	CLKDEV_DEV_ID("ehci-platform.0", &mstp_clks[MSTP100]), /* USB EHCI port0/1 */
Index: renesas/arch/arm/mach-shmobile/include/mach/r8a7779.h
===================================================================
--- renesas.orig/arch/arm/mach-shmobile/include/mach/r8a7779.h
+++ renesas/arch/arm/mach-shmobile/include/mach/r8a7779.h
@@ -31,6 +31,7 @@ extern void r8a7779_earlytimer_init(void
 extern void r8a7779_add_early_devices(void);
 extern void r8a7779_add_standard_devices(void);
 extern void r8a7779_add_standard_devices_dt(void);
+extern void r8a7779_add_ether_device(void *pdata);
 extern void r8a7779_clock_init(void);
 extern void r8a7779_pinmux_init(void);
 extern void r8a7779_pm_init(void);
Index: renesas/arch/arm/mach-shmobile/setup-r8a7779.c
===================================================================
--- renesas.orig/arch/arm/mach-shmobile/setup-r8a7779.c
+++ renesas/arch/arm/mach-shmobile/setup-r8a7779.c
@@ -1,8 +1,9 @@
 /*
  * r8a7779 processor support
  *
- * Copyright (C) 2011  Renesas Solutions Corp.
+ * Copyright (C) 2011, 2013  Renesas Solutions Corp.
  * Copyright (C) 2011  Magnus Damm
+ * Copyright (C) 2013  Cogent Embedded, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -369,6 +370,25 @@ static struct platform_device i2c3_devic
 	.num_resources	= ARRAY_SIZE(rcar_i2c3_res),
 };
 
+/* Ether */
+static struct resource ether_resources[] = {
+	{
+		.start	= 0xfde00000,
+		.end	= 0xfde003ff,
+		.flags	= IORESOURCE_MEM,
+	}, {
+		.start	= gic_iid(0xb4),
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device ether_device = {
+	.name		= "sh-eth",
+	.id		= -1,
+	.resource	= ether_resources,
+	.num_resources	= ARRAY_SIZE(ether_resources),
+};
+
 static struct resource sata_resources[] = {
 	[0] = {
 		.name	= "rcar-sata",
@@ -428,6 +448,13 @@ void __init r8a7779_add_standard_devices
 			    ARRAY_SIZE(r8a7779_late_devices));
 }
 
+void __init r8a7779_add_ether_device(void *pdata)
+{
+	ether_device.dev.platform_data = pdata;
+
+	platform_device_register(&ether_device);
+}
+
 /* do nothing for !CONFIG_SMP or !CONFIG_HAVE_TWD */
 void __init __weak r8a7779_register_twd(void) { }
 

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Kuninori Morimoto]

Hi Sergei

> +void __init r8a7779_add_ether_device(void *pdata)
> +{
> +	ether_device.dev.platform_data = pdata;
> +
> +	platform_device_register(&ether_device);
> +}

Current ARM SoC is trying to not use platform_device_register()
Please use platform_device_register_xxx()
Same comment for [2/2]

Best regards
---
Kuninori Morimoto

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Kuninori Morimoto]

Hi Sergei

> +void __init r8a7779_add_ether_device(void *pdata)
> +{
> +	ether_device.dev.platform_data = pdata;
> +
> +	platform_device_register(&ether_device);
> +}

Current ARM SoC is trying to not use platform_device_register()
Please use platform_device_register_xxx()
Same comment for [2/2]

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Kuninori Morimoto]

Hi Sergei again

> +void __init r8a7779_add_ether_device(void *pdata)
> +{
> +	ether_device.dev.platform_data = pdata;
> +
> +	platform_device_register(&ether_device);
> +}

We didn't use this style of registration before,
but, it is OK.
Then, this (void *pdata) should be
(struct sh_eth_plat_data *pdata) IMO

Best regards
---
Kuninori Morimoto

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Kuninori Morimoto]

Hi Sergei again

> +void __init r8a7779_add_ether_device(void *pdata)
> +{
> +	ether_device.dev.platform_data = pdata;
> +
> +	platform_device_register(&ether_device);
> +}

We didn't use this style of registration before,
but, it is OK.
Then, this (void *pdata) should be
(struct sh_eth_plat_data *pdata) IMO

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Sergei Shtylyov]

Hello.

On 02-04-2013 4:17, Kuninori Morimoto wrote:

>> +void __init r8a7779_add_ether_device(void *pdata)
>> +{
>> +	ether_device.dev.platform_data = pdata;
>> +
>> +	platform_device_register(&ether_device);
>> +}

> Current ARM SoC is trying to not use platform_device_register()
> Please use platform_device_register_xxx()
> Same comment for [2/2]

    Is there some rationale to it? I don't find platform_device_register_xxx()
especially handy to use.

WBR, Sergei

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Sergei Shtylyov]

Hello.

On 02-04-2013 4:17, Kuninori Morimoto wrote:

>> +void __init r8a7779_add_ether_device(void *pdata)
>> +{
>> +	ether_device.dev.platform_data = pdata;
>> +
>> +	platform_device_register(&ether_device);
>> +}

> Current ARM SoC is trying to not use platform_device_register()
> Please use platform_device_register_xxx()
> Same comment for [2/2]

    Is there some rationale to it? I don't find platform_device_register_xxx()
especially handy to use.

WBR, Sergei

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Sergei Shtylyov]

Hello.

On 02-04-2013 4:27, Kuninori Morimoto wrote:

>> +void __init r8a7779_add_ether_device(void *pdata)
>> +{
>> +	ether_device.dev.platform_data = pdata;
>> +
>> +	platform_device_register(&ether_device);
>> +}

> We didn't use this style of registration before,
> but, it is OK.

     Se the DU support patches, I'm copying the approach from them.

> Then, this (void *pdata) should be
> (struct sh_eth_plat_data *pdata) IMO

    ether_device.dev.platform_data is 'void *'. I didn't want to bring in 
extra header for the little use.

WBR, Sergei

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Sergei Shtylyov]

Hello.

On 02-04-2013 4:27, Kuninori Morimoto wrote:

>> +void __init r8a7779_add_ether_device(void *pdata)
>> +{
>> +	ether_device.dev.platform_data = pdata;
>> +
>> +	platform_device_register(&ether_device);
>> +}

> We didn't use this style of registration before,
> but, it is OK.

     Se the DU support patches, I'm copying the approach from them.

> Then, this (void *pdata) should be
> (struct sh_eth_plat_data *pdata) IMO

    ether_device.dev.platform_data is 'void *'. I didn't want to bring in 
extra header for the little use.

WBR, Sergei

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Kuninori Morimoto]

Hi Sergei

> > Then, this (void *pdata) should be
> > (struct sh_eth_plat_data *pdata) IMO
> 
>     ether_device.dev.platform_data is 'void *'. I didn't want to bring in 
> extra header for the little use.

Not enough reason for me.

"void *" means there is no pointer check,
and extra header is just 1 line. No ?

If you want to use this style,
then, additional extra header is fate, IMO

Best regards
---
Kuninori Morimoto

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Kuninori Morimoto]

Hi Sergei

> > Then, this (void *pdata) should be
> > (struct sh_eth_plat_data *pdata) IMO
> 
>     ether_device.dev.platform_data is 'void *'. I didn't want to bring in 
> extra header for the little use.

Not enough reason for me.

"void *" means there is no pointer check,
and extra header is just 1 line. No ?

If you want to use this style,
then, additional extra header is fate, IMO

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Kuninori Morimoto]

Hi Sergei

> > Current ARM SoC is trying to not use platform_device_register()
> > Please use platform_device_register_xxx()
> > Same comment for [2/2]
> 
>     Is there some rationale to it? I don't find platform_device_register_xxx()
> especially handy to use.

http://www.spinics.net/lists/linux-sh/msg16871.html

Best regards
---
Kuninori Morimoto

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Kuninori Morimoto]

Hi Sergei

> > Current ARM SoC is trying to not use platform_device_register()
> > Please use platform_device_register_xxx()
> > Same comment for [2/2]
> 
>     Is there some rationale to it? I don't find platform_device_register_xxx()
> especially handy to use.

http://www.spinics.net/lists/linux-sh/msg16871.html

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Sergei Shtylyov]

Hello.

On 03-04-2013 4:08, Kuninori Morimoto wrote:

>>> Then, this (void *pdata) should be
>>> (struct sh_eth_plat_data *pdata) IMO

>>      ether_device.dev.platform_data is 'void *'. I didn't want to bring in
>> extra header for the little use.

> Not enough reason for me.

> "void *" means there is no pointer check,
> and extra header is just 1 line. No ?

    There's no pointer check either if we just initialize the 'platform_data'
member as part of the platfrom device initializer, so we can actually stuff 
pointer to any nonsense there. Why make this case different?

> If you want to use this style,
> then, additional extra header is fate, IMO

    We can agree to disagree here. :-)

WBR, Sergei

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Sergei Shtylyov]

Hello.

On 03-04-2013 4:08, Kuninori Morimoto wrote:

>>> Then, this (void *pdata) should be
>>> (struct sh_eth_plat_data *pdata) IMO

>>      ether_device.dev.platform_data is 'void *'. I didn't want to bring in
>> extra header for the little use.

> Not enough reason for me.

> "void *" means there is no pointer check,
> and extra header is just 1 line. No ?

    There's no pointer check either if we just initialize the 'platform_data'
member as part of the platfrom device initializer, so we can actually stuff 
pointer to any nonsense there. Why make this case different?

> If you want to use this style,
> then, additional extra header is fate, IMO

    We can agree to disagree here. :-)

WBR, Sergei

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Sergei Shtylyov]

Hello.

On 03-04-2013 4:25, Kuninori Morimoto wrote:

>>> Current ARM SoC is trying to not use platform_device_register()
>>> Please use platform_device_register_xxx()
>>> Same comment for [2/2]

>>      Is there some rationale to it? I don't find platform_device_register_xxx()
>> especially handy to use.

> http://www.spinics.net/lists/linux-sh/msg16871.html

    I don't see there actual Greg KH's rationale for not using 
platform_register_device().

WBR, Sergei

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Sergei Shtylyov]

Hello.

On 03-04-2013 4:25, Kuninori Morimoto wrote:

>>> Current ARM SoC is trying to not use platform_device_register()
>>> Please use platform_device_register_xxx()
>>> Same comment for [2/2]

>>      Is there some rationale to it? I don't find platform_device_register_xxx()
>> especially handy to use.

> http://www.spinics.net/lists/linux-sh/msg16871.html

    I don't see there actual Greg KH's rationale for not using 
platform_register_device().

WBR, Sergei

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Simon Horman]

On Wed, Apr 03, 2013 at 05:22:22PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 03-04-2013 4:08, Kuninori Morimoto wrote:
> 
> >>>Then, this (void *pdata) should be
> >>>(struct sh_eth_plat_data *pdata) IMO
> 
> >>     ether_device.dev.platform_data is 'void *'. I didn't want to bring in
> >>extra header for the little use.
> 
> >Not enough reason for me.
> 
> >"void *" means there is no pointer check,
> >and extra header is just 1 line. No ?
> 
>    There's no pointer check either if we just initialize the 'platform_data'
> member as part of the platfrom device initializer, so we can
> actually stuff pointer to any nonsense there. Why make this case
> different?
> 
> >If you want to use this style,
> >then, additional extra header is fate, IMO
> 
>    We can agree to disagree here. :-)

I would like void * changed to struct sh_eth_plat_data * so
that callers of r8a7779_add_ether_device() will get a warning
if they pass an argument of the wrong type.

Other than that I believe that I am happy with this patch.

Ditto for the similar patch for the R8A7778.

Thanks

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Simon Horman]

On Wed, Apr 03, 2013 at 05:22:22PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 03-04-2013 4:08, Kuninori Morimoto wrote:
> 
> >>>Then, this (void *pdata) should be
> >>>(struct sh_eth_plat_data *pdata) IMO
> 
> >>     ether_device.dev.platform_data is 'void *'. I didn't want to bring in
> >>extra header for the little use.
> 
> >Not enough reason for me.
> 
> >"void *" means there is no pointer check,
> >and extra header is just 1 line. No ?
> 
>    There's no pointer check either if we just initialize the 'platform_data'
> member as part of the platfrom device initializer, so we can
> actually stuff pointer to any nonsense there. Why make this case
> different?
> 
> >If you want to use this style,
> >then, additional extra header is fate, IMO
> 
>    We can agree to disagree here. :-)

I would like void * changed to struct sh_eth_plat_data * so
that callers of r8a7779_add_ether_device() will get a warning
if they pass an argument of the wrong type.

Other than that I believe that I am happy with this patch.

Ditto for the similar patch for the R8A7778.

Thanks

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Simon Horman]

On Wed, Apr 03, 2013 at 05:27:00PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 03-04-2013 4:25, Kuninori Morimoto wrote:
> 
> >>>Current ARM SoC is trying to not use platform_device_register()
> >>>Please use platform_device_register_xxx()
> >>>Same comment for [2/2]
> 
> >>     Is there some rationale to it? I don't find platform_device_register_xxx()
> >>especially handy to use.
> 
> >http://www.spinics.net/lists/linux-sh/msg16871.html
> 
>    I don't see there actual Greg KH's rationale for not using
> platform_register_device().

I'm sure that you can find the rationale if you look.
If not you can ask him, if it is important to you.
Failing that, I can ask him, if its important to you.

However, the point is that Arnd, who is the upstream for shmobile, has
indicated that he would prefer
platform_device_register_{full,resndata,data,simple} used instead of
platform_register_device(). And furthermore Magnus appears to have started
following this advice in his series "[PATCH 01/04] ARM: shmobile: r8a7740
pinmux platform device cleanup".

I would like you to update this patch to follow that advice too.
Likewise for the similar R8A7778 patch.

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Simon Horman]

On Wed, Apr 03, 2013 at 05:27:00PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 03-04-2013 4:25, Kuninori Morimoto wrote:
> 
> >>>Current ARM SoC is trying to not use platform_device_register()
> >>>Please use platform_device_register_xxx()
> >>>Same comment for [2/2]
> 
> >>     Is there some rationale to it? I don't find platform_device_register_xxx()
> >>especially handy to use.
> 
> >http://www.spinics.net/lists/linux-sh/msg16871.html
> 
>    I don't see there actual Greg KH's rationale for not using
> platform_register_device().

I'm sure that you can find the rationale if you look.
If not you can ask him, if it is important to you.
Failing that, I can ask him, if its important to you.

However, the point is that Arnd, who is the upstream for shmobile, has
indicated that he would prefer
platform_device_register_{full,resndata,data,simple} used instead of
platform_register_device(). And furthermore Magnus appears to have started
following this advice in his series "[PATCH 01/04] ARM: shmobile: r8a7740
pinmux platform device cleanup".

I would like you to update this patch to follow that advice too.
Likewise for the similar R8A7778 patch.

Re: [PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Simon Horman]

On Thu, Apr 04, 2013 at 03:34:10PM +0900, Simon Horman wrote:
> On Wed, Apr 03, 2013 at 05:22:22PM +0400, Sergei Shtylyov wrote:
> > Hello.
> > 
> > On 03-04-2013 4:08, Kuninori Morimoto wrote:
> > 
> > >>>Then, this (void *pdata) should be
> > >>>(struct sh_eth_plat_data *pdata) IMO
> > 
> > >>     ether_device.dev.platform_data is 'void *'. I didn't want to bring in
> > >>extra header for the little use.
> > 
> > >Not enough reason for me.
> > 
> > >"void *" means there is no pointer check,
> > >and extra header is just 1 line. No ?
> > 
> >    There's no pointer check either if we just initialize the 'platform_data'
> > member as part of the platfrom device initializer, so we can
> > actually stuff pointer to any nonsense there. Why make this case
> > different?
> > 
> > >If you want to use this style,
> > >then, additional extra header is fate, IMO
> > 
> >    We can agree to disagree here. :-)
> 
> I would like void * changed to struct sh_eth_plat_data * so
> that callers of r8a7779_add_ether_device() will get a warning
> if they pass an argument of the wrong type.
> 
> Other than that I believe that I am happy with this patch.
>
> Ditto for the similar patch for the R8A7778.

As mentioned in a subsequent email in another subthread,
I would also like you to update the patches to not use
platform_register_device().

[PATCH 1/2] ARM: shmobile: R8A7779: add Ether support

[Simon Horman]

On Thu, Apr 04, 2013 at 03:34:10PM +0900, Simon Horman wrote:
> On Wed, Apr 03, 2013 at 05:22:22PM +0400, Sergei Shtylyov wrote:
> > Hello.
> > 
> > On 03-04-2013 4:08, Kuninori Morimoto wrote:
> > 
> > >>>Then, this (void *pdata) should be
> > >>>(struct sh_eth_plat_data *pdata) IMO
> > 
> > >>     ether_device.dev.platform_data is 'void *'. I didn't want to bring in
> > >>extra header for the little use.
> > 
> > >Not enough reason for me.
> > 
> > >"void *" means there is no pointer check,
> > >and extra header is just 1 line. No ?
> > 
> >    There's no pointer check either if we just initialize the 'platform_data'
> > member as part of the platfrom device initializer, so we can
> > actually stuff pointer to any nonsense there. Why make this case
> > different?
> > 
> > >If you want to use this style,
> > >then, additional extra header is fate, IMO
> > 
> >    We can agree to disagree here. :-)
> 
> I would like void * changed to struct sh_eth_plat_data * so
> that callers of r8a7779_add_ether_device() will get a warning
> if they pass an argument of the wrong type.
> 
> Other than that I believe that I am happy with this patch.
>
> Ditto for the similar patch for the R8A7778.

As mentioned in a subsequent email in another subthread,
I would also like you to update the patches to not use
platform_register_device().