| Message ID | 201304020204.54546.sergei.shtylyov@cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Sergei > +void __init r8a7779_add_ether_device(void *pdata) > +{ > + ether_device.dev.platform_data = pdata; > + > + platform_device_register(ðer_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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei again > +void __init r8a7779_add_ether_device(void *pdata) > +{ > + ether_device.dev.platform_data = pdata; > + > + platform_device_register(ðer_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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(ðer_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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(ðer_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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(). -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(ðer_device); +} + /* do nothing for !CONFIG_SMP or !CONFIG_HAVE_TWD */ void __init __weak r8a7779_register_twd(void) { }
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html