[PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

platform_device_register_xxx() uses kmemdup() for res / data.
This means we can move these codes into init function if
it was defined.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
>> Simon, Magnus

This is not exciting patch, but cleanup code.
This is focusing to lager only at this point.
Actually, I would like to create board_add_xxx() function
for all devices, on all board-xxx.c / setup-xxx.c
Because, board-xxx.c has many global variable, but, almost all are copied
by kmemdup() and removed by __init.
It will be more readable if these are inside board_add_xxx() function.
Yes, not exciting...

 arch/arm/mach-shmobile/board-lager.c |  144 +++++++++++++++++-----------------
 1 file changed, 72 insertions(+), 72 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
index f0104bf..78625ff 100644
--- a/arch/arm/mach-shmobile/board-lager.c
+++ b/arch/arm/mach-shmobile/board-lager.c
@@ -116,17 +116,17 @@ static const struct rcar_du_platform_data lager_du_pdata __initconst = {
 	.num_encoders = ARRAY_SIZE(lager_du_encoders),
 };
 
-static const struct resource du_resources[] __initconst = {
-	DEFINE_RES_MEM(0xfeb00000, 0x70000),
-	DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
-	DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
-	DEFINE_RES_IRQ(gic_spi(256)),
-	DEFINE_RES_IRQ(gic_spi(268)),
-	DEFINE_RES_IRQ(gic_spi(269)),
-};
-
 static void __init lager_add_du_device(void)
 {
+	struct resource du_resources[] = {
+		DEFINE_RES_MEM(0xfeb00000, 0x70000),
+		DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
+		DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
+		DEFINE_RES_IRQ(gic_spi(256)),
+		DEFINE_RES_IRQ(gic_spi(268)),
+		DEFINE_RES_IRQ(gic_spi(269)),
+	};
+
 	struct platform_device_info info = {
 		.name = "rcar-du-r8a7790",
 		.id = -1,
@@ -341,18 +341,18 @@ static const struct resource qspi_resources[] __initconst = {
 };
 
 /* VIN */
-static const struct resource vin_resources[] __initconst = {
-	/* VIN0 */
-	DEFINE_RES_MEM(0xe6ef0000, 0x1000),
-	DEFINE_RES_IRQ(gic_spi(188)),
-	/* VIN1 */
-	DEFINE_RES_MEM(0xe6ef1000, 0x1000),
-	DEFINE_RES_IRQ(gic_spi(189)),
-};
-
 static void __init lager_add_vin_device(unsigned idx,
 					struct rcar_vin_platform_data *pdata)
 {
+	struct resource vin_resources[] = {
+		/* VIN0 */
+		DEFINE_RES_MEM(0xe6ef0000, 0x1000),
+		DEFINE_RES_IRQ(gic_spi(188)),
+		/* VIN1 */
+		DEFINE_RES_MEM(0xe6ef1000, 0x1000),
+		DEFINE_RES_IRQ(gic_spi(189)),
+	};
+
 	struct platform_device_info vin_info = {
 		.parent		= &platform_bus,
 		.name		= "r8a7790-vin",
@@ -559,13 +559,6 @@ static struct i2c_board_info i2c2_devices[] = {
 };
 
 /* Sound */
-static struct resource rsnd_resources[] __initdata = {
-	[RSND_GEN2_SCU]  = DEFINE_RES_MEM(0xec500000, 0x1000),
-	[RSND_GEN2_ADG]  = DEFINE_RES_MEM(0xec5a0000, 0x100),
-	[RSND_GEN2_SSIU] = DEFINE_RES_MEM(0xec540000, 0x1000),
-	[RSND_GEN2_SSI]  = DEFINE_RES_MEM(0xec541000, 0x1280),
-};
-
 static struct rsnd_ssi_platform_info rsnd_ssi[] = {
 	RSND_SSI_SET(0, 0, gic_spi(370), RSND_SSI_PLAY),
 	RSND_SSI_SET(0, 0, gic_spi(371), RSND_SSI_CLK_PIN_SHARE),
@@ -583,25 +576,32 @@ static struct rcar_snd_info rsnd_info = {
 	.scu_info_nr	= ARRAY_SIZE(rsnd_scu),
 };
 
-static struct asoc_simple_card_info rsnd_card_info = {
-	.name		= "AK4643",
-	.card		= "SSI01-AK4643",
-	.codec		= "ak4642-codec.2-0012",
-	.platform	= "rcar_sound",
-	.daifmt		= SND_SOC_DAIFMT_LEFT_J,
-	.cpu_dai = {
-		.name	= "rcar_sound",
-		.fmt	= SND_SOC_DAIFMT_CBS_CFS,
-	},
-	.codec_dai = {
-		.name	= "ak4642-hifi",
-		.fmt	= SND_SOC_DAIFMT_CBM_CFM,
-		.sysclk	= 11289600,
-	},
-};
-
 static void __init lager_add_rsnd_device(void)
 {
+	struct resource rsnd_resources[] = {
+		[RSND_GEN2_SCU]  = DEFINE_RES_MEM(0xec500000, 0x1000),
+		[RSND_GEN2_ADG]  = DEFINE_RES_MEM(0xec5a0000, 0x100),
+		[RSND_GEN2_SSIU] = DEFINE_RES_MEM(0xec540000, 0x1000),
+		[RSND_GEN2_SSI]  = DEFINE_RES_MEM(0xec541000, 0x1280),
+	};
+
+	struct asoc_simple_card_info rsnd_card_info = {
+		.name		= "AK4643",
+		.card		= "SSI01-AK4643",
+		.codec		= "ak4642-codec.2-0012",
+		.platform	= "rcar_sound",
+		.daifmt		= SND_SOC_DAIFMT_LEFT_J,
+		.cpu_dai = {
+			.name	= "rcar_sound",
+			.fmt	= SND_SOC_DAIFMT_CBS_CFS,
+		},
+		.codec_dai = {
+			.name	= "ak4642-hifi",
+			.fmt	= SND_SOC_DAIFMT_CBM_CFM,
+			.sysclk	= 11289600,
+		},
+	};
+
 	struct platform_device_info cardinfo = {
 		.parent         = &platform_bus,
 		.name           = "asoc-simple-card",
@@ -651,44 +651,44 @@ static struct resource sdhi2_resources[] __initdata = {
 };
 
 /* Internal PCI1 */
-static const struct resource pci1_resources[] __initconst = {
-	DEFINE_RES_MEM(0xee0b0000, 0x10000),	/* CFG */
-	DEFINE_RES_MEM(0xee0a0000, 0x10000),	/* MEM */
-	DEFINE_RES_IRQ(gic_spi(112)),
-};
-
-static const struct platform_device_info pci1_info __initconst = {
-	.parent		= &platform_bus,
-	.name		= "pci-rcar-gen2",
-	.id		= 1,
-	.res		= pci1_resources,
-	.num_res	= ARRAY_SIZE(pci1_resources),
-	.dma_mask	= DMA_BIT_MASK(32),
-};
-
 static void __init lager_add_usb1_device(void)
 {
+	struct resource pci1_resources[] = {
+		DEFINE_RES_MEM(0xee0b0000, 0x10000),	/* CFG */
+		DEFINE_RES_MEM(0xee0a0000, 0x10000),	/* MEM */
+		DEFINE_RES_IRQ(gic_spi(112)),
+	};
+
+	struct platform_device_info pci1_info = {
+		.parent		= &platform_bus,
+		.name		= "pci-rcar-gen2",
+		.id		= 1,
+		.res		= pci1_resources,
+		.num_res	= ARRAY_SIZE(pci1_resources),
+		.dma_mask	= DMA_BIT_MASK(32),
+	};
+
 	platform_device_register_full(&pci1_info);
 }
 
 /* Internal PCI2 */
-static const struct resource pci2_resources[] __initconst = {
-	DEFINE_RES_MEM(0xee0d0000, 0x10000),	/* CFG */
-	DEFINE_RES_MEM(0xee0c0000, 0x10000),	/* MEM */
-	DEFINE_RES_IRQ(gic_spi(113)),
-};
-
-static const struct platform_device_info pci2_info __initconst = {
-	.parent		= &platform_bus,
-	.name		= "pci-rcar-gen2",
-	.id		= 2,
-	.res		= pci2_resources,
-	.num_res	= ARRAY_SIZE(pci2_resources),
-	.dma_mask	= DMA_BIT_MASK(32),
-};
-
 static void __init lager_add_usb2_device(void)
 {
+	struct resource pci2_resources[] = {
+		DEFINE_RES_MEM(0xee0d0000, 0x10000),	/* CFG */
+		DEFINE_RES_MEM(0xee0c0000, 0x10000),	/* MEM */
+		DEFINE_RES_IRQ(gic_spi(113)),
+	};
+
+	struct platform_device_info pci2_info = {
+		.parent		= &platform_bus,
+		.name		= "pci-rcar-gen2",
+		.id		= 2,
+		.res		= pci2_resources,
+		.num_res	= ARRAY_SIZE(pci2_resources),
+		.dma_mask	= DMA_BIT_MASK(32),
+	};
+
 	platform_device_register_full(&pci2_info);
 }
 
-- 
1.7.9.5

Re: [PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[Geert Uytterhoeven]

Hi Morimoto-san,

On Tue, Apr 8, 2014 at 9:15 AM, Kuninori Morimoto
<kuninori.morimoto.gx@gmail.com> wrote:
> +++ b/arch/arm/mach-shmobile/board-lager.c
> @@ -116,17 +116,17 @@ static const struct rcar_du_platform_data lager_du_pdata __initconst = {
>         .num_encoders = ARRAY_SIZE(lager_du_encoders),
>  };
>
> -static const struct resource du_resources[] __initconst = {
> -       DEFINE_RES_MEM(0xfeb00000, 0x70000),
> -       DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
> -       DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
> -       DEFINE_RES_IRQ(gic_spi(256)),
> -       DEFINE_RES_IRQ(gic_spi(268)),
> -       DEFINE_RES_IRQ(gic_spi(269)),
> -};
> -
>  static void __init lager_add_du_device(void)
>  {
> +       struct resource du_resources[] = {
> +               DEFINE_RES_MEM(0xfeb00000, 0x70000),
> +               DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
> +               DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
> +               DEFINE_RES_IRQ(gic_spi(256)),
> +               DEFINE_RES_IRQ(gic_spi(268)),
> +               DEFINE_RES_IRQ(gic_spi(269)),
> +       };

Shouldn't the above retain the "static const"?
Without the static, there will be additonal code to perform the initialization
on the stack.

> +
>         struct platform_device_info info = {
>                 .name = "rcar-du-r8a7790",
>                 .id = -1,

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[Simon Horman]

On Tue, Apr 08, 2014 at 12:15:21AM -0700, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> platform_device_register_xxx() uses kmemdup() for res / data.
> This means we can move these codes into init function if
> it was defined.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> >> Simon, Magnus
> 
> This is not exciting patch, but cleanup code.
> This is focusing to lager only at this point.
> Actually, I would like to create board_add_xxx() function
> for all devices, on all board-xxx.c / setup-xxx.c
> Because, board-xxx.c has many global variable, but, almost all are copied
> by kmemdup() and removed by __init.
> It will be more readable if these are inside board_add_xxx() function.
> Yes, not exciting...

I'm not sure that I understand the motivation for this change.
Especially as the long term goal is to remove board files entirely.

> 
>  arch/arm/mach-shmobile/board-lager.c |  144 +++++++++++++++++-----------------
>  1 file changed, 72 insertions(+), 72 deletions(-)
> 
> diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
> index f0104bf..78625ff 100644
> --- a/arch/arm/mach-shmobile/board-lager.c
> +++ b/arch/arm/mach-shmobile/board-lager.c
> @@ -116,17 +116,17 @@ static const struct rcar_du_platform_data lager_du_pdata __initconst = {
>  	.num_encoders = ARRAY_SIZE(lager_du_encoders),
>  };
>  
> -static const struct resource du_resources[] __initconst = {
> -	DEFINE_RES_MEM(0xfeb00000, 0x70000),
> -	DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
> -	DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
> -	DEFINE_RES_IRQ(gic_spi(256)),
> -	DEFINE_RES_IRQ(gic_spi(268)),
> -	DEFINE_RES_IRQ(gic_spi(269)),
> -};
> -
>  static void __init lager_add_du_device(void)
>  {
> +	struct resource du_resources[] = {
> +		DEFINE_RES_MEM(0xfeb00000, 0x70000),
> +		DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
> +		DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
> +		DEFINE_RES_IRQ(gic_spi(256)),
> +		DEFINE_RES_IRQ(gic_spi(268)),
> +		DEFINE_RES_IRQ(gic_spi(269)),
> +	};
> +
>  	struct platform_device_info info = {
>  		.name = "rcar-du-r8a7790",
>  		.id = -1,
> @@ -341,18 +341,18 @@ static const struct resource qspi_resources[] __initconst = {
>  };
>  
>  /* VIN */
> -static const struct resource vin_resources[] __initconst = {
> -	/* VIN0 */
> -	DEFINE_RES_MEM(0xe6ef0000, 0x1000),
> -	DEFINE_RES_IRQ(gic_spi(188)),
> -	/* VIN1 */
> -	DEFINE_RES_MEM(0xe6ef1000, 0x1000),
> -	DEFINE_RES_IRQ(gic_spi(189)),
> -};
> -
>  static void __init lager_add_vin_device(unsigned idx,
>  					struct rcar_vin_platform_data *pdata)
>  {
> +	struct resource vin_resources[] = {
> +		/* VIN0 */
> +		DEFINE_RES_MEM(0xe6ef0000, 0x1000),
> +		DEFINE_RES_IRQ(gic_spi(188)),
> +		/* VIN1 */
> +		DEFINE_RES_MEM(0xe6ef1000, 0x1000),
> +		DEFINE_RES_IRQ(gic_spi(189)),
> +	};
> +
>  	struct platform_device_info vin_info = {
>  		.parent		= &platform_bus,
>  		.name		= "r8a7790-vin",
> @@ -559,13 +559,6 @@ static struct i2c_board_info i2c2_devices[] = {
>  };
>  
>  /* Sound */
> -static struct resource rsnd_resources[] __initdata = {
> -	[RSND_GEN2_SCU]  = DEFINE_RES_MEM(0xec500000, 0x1000),
> -	[RSND_GEN2_ADG]  = DEFINE_RES_MEM(0xec5a0000, 0x100),
> -	[RSND_GEN2_SSIU] = DEFINE_RES_MEM(0xec540000, 0x1000),
> -	[RSND_GEN2_SSI]  = DEFINE_RES_MEM(0xec541000, 0x1280),
> -};
> -
>  static struct rsnd_ssi_platform_info rsnd_ssi[] = {
>  	RSND_SSI_SET(0, 0, gic_spi(370), RSND_SSI_PLAY),
>  	RSND_SSI_SET(0, 0, gic_spi(371), RSND_SSI_CLK_PIN_SHARE),
> @@ -583,25 +576,32 @@ static struct rcar_snd_info rsnd_info = {
>  	.scu_info_nr	= ARRAY_SIZE(rsnd_scu),
>  };
>  
> -static struct asoc_simple_card_info rsnd_card_info = {
> -	.name		= "AK4643",
> -	.card		= "SSI01-AK4643",
> -	.codec		= "ak4642-codec.2-0012",
> -	.platform	= "rcar_sound",
> -	.daifmt		= SND_SOC_DAIFMT_LEFT_J,
> -	.cpu_dai = {
> -		.name	= "rcar_sound",
> -		.fmt	= SND_SOC_DAIFMT_CBS_CFS,
> -	},
> -	.codec_dai = {
> -		.name	= "ak4642-hifi",
> -		.fmt	= SND_SOC_DAIFMT_CBM_CFM,
> -		.sysclk	= 11289600,
> -	},
> -};
> -
>  static void __init lager_add_rsnd_device(void)
>  {
> +	struct resource rsnd_resources[] = {
> +		[RSND_GEN2_SCU]  = DEFINE_RES_MEM(0xec500000, 0x1000),
> +		[RSND_GEN2_ADG]  = DEFINE_RES_MEM(0xec5a0000, 0x100),
> +		[RSND_GEN2_SSIU] = DEFINE_RES_MEM(0xec540000, 0x1000),
> +		[RSND_GEN2_SSI]  = DEFINE_RES_MEM(0xec541000, 0x1280),
> +	};
> +
> +	struct asoc_simple_card_info rsnd_card_info = {
> +		.name		= "AK4643",
> +		.card		= "SSI01-AK4643",
> +		.codec		= "ak4642-codec.2-0012",
> +		.platform	= "rcar_sound",
> +		.daifmt		= SND_SOC_DAIFMT_LEFT_J,
> +		.cpu_dai = {
> +			.name	= "rcar_sound",
> +			.fmt	= SND_SOC_DAIFMT_CBS_CFS,
> +		},
> +		.codec_dai = {
> +			.name	= "ak4642-hifi",
> +			.fmt	= SND_SOC_DAIFMT_CBM_CFM,
> +			.sysclk	= 11289600,
> +		},
> +	};
> +
>  	struct platform_device_info cardinfo = {
>  		.parent         = &platform_bus,
>  		.name           = "asoc-simple-card",
> @@ -651,44 +651,44 @@ static struct resource sdhi2_resources[] __initdata = {
>  };
>  
>  /* Internal PCI1 */
> -static const struct resource pci1_resources[] __initconst = {
> -	DEFINE_RES_MEM(0xee0b0000, 0x10000),	/* CFG */
> -	DEFINE_RES_MEM(0xee0a0000, 0x10000),	/* MEM */
> -	DEFINE_RES_IRQ(gic_spi(112)),
> -};
> -
> -static const struct platform_device_info pci1_info __initconst = {
> -	.parent		= &platform_bus,
> -	.name		= "pci-rcar-gen2",
> -	.id		= 1,
> -	.res		= pci1_resources,
> -	.num_res	= ARRAY_SIZE(pci1_resources),
> -	.dma_mask	= DMA_BIT_MASK(32),
> -};
> -
>  static void __init lager_add_usb1_device(void)
>  {
> +	struct resource pci1_resources[] = {
> +		DEFINE_RES_MEM(0xee0b0000, 0x10000),	/* CFG */
> +		DEFINE_RES_MEM(0xee0a0000, 0x10000),	/* MEM */
> +		DEFINE_RES_IRQ(gic_spi(112)),
> +	};
> +
> +	struct platform_device_info pci1_info = {
> +		.parent		= &platform_bus,
> +		.name		= "pci-rcar-gen2",
> +		.id		= 1,
> +		.res		= pci1_resources,
> +		.num_res	= ARRAY_SIZE(pci1_resources),
> +		.dma_mask	= DMA_BIT_MASK(32),
> +	};
> +
>  	platform_device_register_full(&pci1_info);
>  }
>  
>  /* Internal PCI2 */
> -static const struct resource pci2_resources[] __initconst = {
> -	DEFINE_RES_MEM(0xee0d0000, 0x10000),	/* CFG */
> -	DEFINE_RES_MEM(0xee0c0000, 0x10000),	/* MEM */
> -	DEFINE_RES_IRQ(gic_spi(113)),
> -};
> -
> -static const struct platform_device_info pci2_info __initconst = {
> -	.parent		= &platform_bus,
> -	.name		= "pci-rcar-gen2",
> -	.id		= 2,
> -	.res		= pci2_resources,
> -	.num_res	= ARRAY_SIZE(pci2_resources),
> -	.dma_mask	= DMA_BIT_MASK(32),
> -};
> -
>  static void __init lager_add_usb2_device(void)
>  {
> +	struct resource pci2_resources[] = {
> +		DEFINE_RES_MEM(0xee0d0000, 0x10000),	/* CFG */
> +		DEFINE_RES_MEM(0xee0c0000, 0x10000),	/* MEM */
> +		DEFINE_RES_IRQ(gic_spi(113)),
> +	};
> +
> +	struct platform_device_info pci2_info = {
> +		.parent		= &platform_bus,
> +		.name		= "pci-rcar-gen2",
> +		.id		= 2,
> +		.res		= pci2_resources,
> +		.num_res	= ARRAY_SIZE(pci2_resources),
> +		.dma_mask	= DMA_BIT_MASK(32),
> +	};
> +
>  	platform_device_register_full(&pci2_info);
>  }
>  
> -- 
> 1.7.9.5
> 

Re: [PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[Kuninori Morimoto]

Hi Geert

Thank you for your review

> > -static const struct resource du_resources[] __initconst = {
> > -       DEFINE_RES_MEM(0xfeb00000, 0x70000),
> > -       DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
> > -       DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
> > -       DEFINE_RES_IRQ(gic_spi(256)),
> > -       DEFINE_RES_IRQ(gic_spi(268)),
> > -       DEFINE_RES_IRQ(gic_spi(269)),
> > -};
> > -
> >  static void __init lager_add_du_device(void)
> >  {
> > +       struct resource du_resources[] = {
> > +               DEFINE_RES_MEM(0xfeb00000, 0x70000),
> > +               DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
> > +               DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
> > +               DEFINE_RES_IRQ(gic_spi(256)),
> > +               DEFINE_RES_IRQ(gic_spi(268)),
> > +               DEFINE_RES_IRQ(gic_spi(269)),
> > +       };
> 
> Shouldn't the above retain the "static const"?
> Without the static, there will be additonal code to perform the initialization
> on the stack.

"static" is not needed.
Becouse ...

platform_device_register_xxx()
 -> platform_device_register_full()
   -> platform_device_add_resources()
   -> platform_device_add_data()

platform_device_add_resources/data() are using kmemdup().
This means, driver uses alloced and copied resource/data,
not above one.

Best regards
---
Kuninori Morimoto

Re: [PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[Kuninori Morimoto]

Hi Simon

> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > 
> > platform_device_register_xxx() uses kmemdup() for res / data.
> > This means we can move these codes into init function if
> > it was defined.
> > 
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
> > >> Simon, Magnus
> > 
> > This is not exciting patch, but cleanup code.
> > This is focusing to lager only at this point.
> > Actually, I would like to create board_add_xxx() function
> > for all devices, on all board-xxx.c / setup-xxx.c
> > Because, board-xxx.c has many global variable, but, almost all are copied
> > by kmemdup() and removed by __init.
> > It will be more readable if these are inside board_add_xxx() function.
> > Yes, not exciting...
> 
> I'm not sure that I understand the motivation for this change.
> Especially as the long term goal is to remove board files entirely.

Grr, indeed !
This is not needed on board code.
But, how about setup code side which has same issue ?

Re: [PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[Geert Uytterhoeven]

Hi Morimoto-san,

On Fri, Apr 11, 2014 at 2:39 AM, Kuninori Morimoto
<kuninori.morimoto.gx@gmail.com> wrote:
>> > -static const struct resource du_resources[] __initconst = {
>> > -       DEFINE_RES_MEM(0xfeb00000, 0x70000),
>> > -       DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
>> > -       DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
>> > -       DEFINE_RES_IRQ(gic_spi(256)),
>> > -       DEFINE_RES_IRQ(gic_spi(268)),
>> > -       DEFINE_RES_IRQ(gic_spi(269)),
>> > -};
>> > -
>> >  static void __init lager_add_du_device(void)
>> >  {
>> > +       struct resource du_resources[] = {
>> > +               DEFINE_RES_MEM(0xfeb00000, 0x70000),
>> > +               DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
>> > +               DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
>> > +               DEFINE_RES_IRQ(gic_spi(256)),
>> > +               DEFINE_RES_IRQ(gic_spi(268)),
>> > +               DEFINE_RES_IRQ(gic_spi(269)),
>> > +       };
>>
>> Shouldn't the above retain the "static const"?
>> Without the static, there will be additonal code to perform the initialization
>> on the stack.
>
> "static" is not needed.
> Becouse ...
>
> platform_device_register_xxx()
>  -> platform_device_register_full()
>    -> platform_device_add_resources()
>    -> platform_device_add_data()
>
> platform_device_add_resources/data() are using kmemdup().
> This means, driver uses alloced and copied resource/data,
> not above one.

Sure, it's copied, so the original data doesn't have to persist at that
address.

My comment was more about code/size optimizations: if you declare
du_resources[] as a local (non-static) variable on the stack, the compiler
has to construct it on the stack. For basic data types (e.g. int, long) this
requires not that much code (the variable may end up in a register), but
for more complex structures, the compiler may emit quite some code,
making it more economic to make the variable static.
For functions that are called multiple times (which is not the case here),
there's another reason to use static: the variable on the stack will be
constructed on every function call, over and over again.

Perhaps you can have a look at the generated assembler, to see whether
it's worth it?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[Sergei Shtylyov]

Hello.

On 11-04-2014 4:39, Kuninori Morimoto wrote:

> Thank you for your review

>>> -static const struct resource du_resources[] __initconst = {
>>> -       DEFINE_RES_MEM(0xfeb00000, 0x70000),
>>> -       DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
>>> -       DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
>>> -       DEFINE_RES_IRQ(gic_spi(256)),
>>> -       DEFINE_RES_IRQ(gic_spi(268)),
>>> -       DEFINE_RES_IRQ(gic_spi(269)),
>>> -};
>>> -
>>>   static void __init lager_add_du_device(void)
>>>   {
>>> +       struct resource du_resources[] = {
>>> +               DEFINE_RES_MEM(0xfeb00000, 0x70000),
>>> +               DEFINE_RES_MEM_NAMED(0xfeb90000, 0x1c, "lvds.0"),
>>> +               DEFINE_RES_MEM_NAMED(0xfeb94000, 0x1c, "lvds.1"),
>>> +               DEFINE_RES_IRQ(gic_spi(256)),
>>> +               DEFINE_RES_IRQ(gic_spi(268)),
>>> +               DEFINE_RES_IRQ(gic_spi(269)),
>>> +       };

>> Shouldn't the above retain the "static const"?
>> Without the static, there will be additonal code to perform the initialization
>> on the stack.

> "static" is not needed.

    It is needed in order to avoid the extra local variable initialization 
code in this function -- exactly what Geert said.

> Becouse ...

> platform_device_register_xxx()
>   -> platform_device_register_full()
>     -> platform_device_add_resources()
>     -> platform_device_add_data()

> platform_device_add_resources/data() are using kmemdup().
> This means, driver uses alloced and copied resource/data,
> not above one.

    You are answering the question Geert didn't ask. :-)

> Best regards
> ---
> Kuninori Morimoto

WBR, Sergei

Re: [PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[Kuninori Morimoto]

Hi Geert

> My comment was more about code/size optimizations: if you declare
> du_resources[] as a local (non-static) variable on the stack, the compiler
> has to construct it on the stack. For basic data types (e.g. int, long) this
> requires not that much code (the variable may end up in a register), but
> for more complex structures, the compiler may emit quite some code,
> making it more economic to make the variable static.
> For functions that are called multiple times (which is not the case here),
> there's another reason to use static: the variable on the stack will be
> constructed on every function call, over and over again.

Thank you for explaining about detail of that.
I understood.
But, I wonder is it so big issue ?
Because, these are called only when kernel booting,
and only once.

And, these functions has __init.
Then, what happen after initialization if I add static in local variable ?
Was static variable keeped ? or removed ?

But, anyway, this patch will not be used...

Re: [PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[Simon Horman]

On Thu, Apr 10, 2014 at 05:42:24PM -0700, Kuninori Morimoto wrote:
> 
> Hi Simon
> 
> > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > > 
> > > platform_device_register_xxx() uses kmemdup() for res / data.
> > > This means we can move these codes into init function if
> > > it was defined.
> > > 
> > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > > ---
> > > >> Simon, Magnus
> > > 
> > > This is not exciting patch, but cleanup code.
> > > This is focusing to lager only at this point.
> > > Actually, I would like to create board_add_xxx() function
> > > for all devices, on all board-xxx.c / setup-xxx.c
> > > Because, board-xxx.c has many global variable, but, almost all are copied
> > > by kmemdup() and removed by __init.
> > > It will be more readable if these are inside board_add_xxx() function.
> > > Yes, not exciting...
> > 
> > I'm not sure that I understand the motivation for this change.
> > Especially as the long term goal is to remove board files entirely.
> 
> Grr, indeed !
> This is not needed on board code.
> But, how about setup code side which has same issue ?

Magnus may have a different opinion, but I feel that the SoC setup code
is also legacy code, though likely to hang around longer than board code.
So my personal feeling is that it would be better to use your energy
on other areas.

Re: [PATCH][RFC] ARM: shmobile: lager: move res/data into init function

[Kuninori Morimoto]

Hi Simon

> > Grr, indeed !
> > This is not needed on board code.
> > But, how about setup code side which has same issue ?
> 
> Magnus may have a different opinion, but I feel that the SoC setup code
> is also legacy code, though likely to hang around longer than board code.
> So my personal feeling is that it would be better to use your energy
> on other areas.

Hehe, I see, will do  :)