[PATCH 03/19] ab3550: mfd_cell is now implicitly available to drivers

[PATCH 03/19] ab3550: mfd_cell is now implicitly available to drivers

[Andres Salomon]

No need to explicitly set the cell's platform_data/data_size.

This wasn't actually used anywhere by the ab3550 stuff; dev_data
in mach-u300's i2c code was empty.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 arch/arm/mach-u300/i2c.c   |    2 --
 drivers/mfd/ab3550-core.c  |    6 ------
 include/linux/mfd/abx500.h |    2 --
 3 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-u300/i2c.c b/arch/arm/mach-u300/i2c.c
index f0394ba..1ff7a82 100644
--- a/arch/arm/mach-u300/i2c.c
+++ b/arch/arm/mach-u300/i2c.c
@@ -291,8 +291,6 @@ static struct ab3550_platform_data ab3550_plf_data = {
 		.base = IRQ_AB3550_BASE,
 		.count = (IRQ_AB3550_END - IRQ_AB3550_BASE + 1),
 	},
-	.dev_data = {
-	},
 	.init_settings = ab3550_init_settings,
 	.init_settings_sz = ARRAY_SIZE(ab3550_init_settings),
 };
diff --git a/drivers/mfd/ab3550-core.c b/drivers/mfd/ab3550-core.c
index 5fbca34..47625b9 100644
--- a/drivers/mfd/ab3550-core.c
+++ b/drivers/mfd/ab3550-core.c
@@ -1319,12 +1319,6 @@ static int __init ab3550_probe(struct i2c_client *client,
 	if (err)
 		goto exit_no_ops;
 
-	/* Set up and register the platform devices. */
-	for (i = 0; i < AB3550_NUM_DEVICES; i++) {
-		ab3550_devs[i].platform_data = ab3550_plf_data->dev_data[i];
-		ab3550_devs[i].data_size = ab3550_plf_data->dev_data_sz[i];
-	}
-
 	err = mfd_add_devices(&client->dev, 0, ab3550_devs,
 		ARRAY_SIZE(ab3550_devs), NULL,
 		ab3550_plf_data->irq.base);
diff --git a/include/linux/mfd/abx500.h b/include/linux/mfd/abx500.h
index 67bd6f7..a735210 100644
--- a/include/linux/mfd/abx500.h
+++ b/include/linux/mfd/abx500.h
@@ -185,8 +185,6 @@ struct abx500_init_settings {
  */
 struct ab3550_platform_data {
 	struct {unsigned int base; unsigned int count; } irq;
-	void *dev_data[AB3550_NUM_DEVICES];
-	size_t dev_data_sz[AB3550_NUM_DEVICES];
 	struct abx500_init_settings *init_settings;
 	unsigned int init_settings_sz;
 };
-- 
1.7.2.3

[PATCH 04/19] ab3100: mfd_cell is now implicitly available to drivers

[Andres Salomon]

No need to explicitly set the cell's platform_data/data_size.

In this case, move the ab3100_platform_data pointer from platform_data
to driver_data.  The only client who makes use of it is also changed.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/mfd/ab3100-core.c  |    6 ++----
 drivers/regulator/ab3100.c |    2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/ab3100-core.c b/drivers/mfd/ab3100-core.c
index 4193af5..244c2a2 100644
--- a/drivers/mfd/ab3100-core.c
+++ b/drivers/mfd/ab3100-core.c
@@ -949,10 +949,8 @@ static int __devinit ab3100_probe(struct i2c_client *client,
 		goto exit_no_ops;
 
 	/* Set up and register the platform devices. */
-	for (i = 0; i < ARRAY_SIZE(ab3100_devs); i++) {
-		ab3100_devs[i].platform_data = ab3100_plf_data;
-		ab3100_devs[i].data_size = sizeof(struct ab3100_platform_data);
-	}
+	for (i = 0; i < ARRAY_SIZE(ab3100_devs); i++)
+		ab3100_devs[i].driver_data = ab3100_plf_data;
 
 	err = mfd_add_devices(&client->dev, 0, ab3100_devs,
 		ARRAY_SIZE(ab3100_devs), NULL, 0);
diff --git a/drivers/regulator/ab3100.c b/drivers/regulator/ab3100.c
index ed6feaf..1f266b9 100644
--- a/drivers/regulator/ab3100.c
+++ b/drivers/regulator/ab3100.c
@@ -576,7 +576,7 @@ ab3100_regulator_desc[AB3100_NUM_REGULATORS] = {
 
 static int __devinit ab3100_regulators_probe(struct platform_device *pdev)
 {
-	struct ab3100_platform_data *plfdata = pdev->dev.platform_data;
+	struct ab3100_platform_data *plfdata = platform_get_drvdata(pdev);
 	int err = 0;
 	u8 data;
 	int i;
-- 
1.7.2.3

[PATCH 15/19] mc13xxx: mfd_cell is now implicitly available to drivers

[Andres Salomon]

No need to explicitly set the cell's platform_data/data_size.

In this case, move the various platform_data pointers
to driver_data.  All of the clients which make use of it
are also changed.

Mfd-core makes a copy of platform_data, but driver_data keeps a pointer
to the original data.  Because each cell's platform_data previously
pointed to a local (stack) variable, the various ARM mach types that set
the pdata are updated to keep the memory around.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 arch/arm/mach-imx/mach-mx27_3ds.c     |    8 ++++++--
 arch/arm/mach-imx/mach-pcm038.c       |    6 +++++-
 arch/arm/mach-mx3/mach-mx31_3ds.c     |    6 +++++-
 arch/arm/mach-mx3/mach-mx31moboard.c  |    6 +++++-
 drivers/leds/leds-mc13783.c           |   15 +++++++++------
 drivers/mfd/mc13xxx-core.c            |   20 ++++++--------------
 drivers/regulator/mc13783-regulator.c |    6 +++---
 drivers/regulator/mc13892-regulator.c |    6 +++---
 drivers/regulator/mc13xxx.h           |    1 +
 include/linux/mfd/mc13xxx.h           |    3 +--
 10 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/arch/arm/mach-imx/mach-mx27_3ds.c b/arch/arm/mach-imx/mach-mx27_3ds.c
index 1643315..edcecaa 100644
--- a/arch/arm/mach-imx/mach-mx27_3ds.c
+++ b/arch/arm/mach-imx/mach-mx27_3ds.c
@@ -226,10 +226,14 @@ static struct mc13783_regulator_init_data mx27_3ds_regulators[] = {
 	},
 };
 
-/* MC13783 */
-static struct mc13783_platform_data mc13783_pdata __initdata = {
+static struct mc13783_regulator_platform_data mx27_regs = {
 	.regulators = mx27_3ds_regulators,
 	.num_regulators = ARRAY_SIZE(mx27_3ds_regulators),
+};
+
+/* MC13783 */
+static struct mc13783_platform_data mc13783_pdata __initdata = {
+	.regulators = &mx27_regs,
 	.flags  = MC13783_USE_REGULATOR,
 };
 
diff --git a/arch/arm/mach-imx/mach-pcm038.c b/arch/arm/mach-imx/mach-pcm038.c
index 5056148..b5ae8cc 100644
--- a/arch/arm/mach-imx/mach-pcm038.c
+++ b/arch/arm/mach-imx/mach-pcm038.c
@@ -262,9 +262,13 @@ static struct mc13783_regulator_init_data pcm038_regulators[] = {
 	},
 };
 
-static struct mc13783_platform_data pcm038_pmic = {
+static struct mc13783_regulator_platform_data pcm038_regs = {
 	.regulators = pcm038_regulators,
 	.num_regulators = ARRAY_SIZE(pcm038_regulators),
+};
+
+static struct mc13783_platform_data pcm038_pmic = {
+	.regulators = &pcm038_regs,
 	.flags = MC13783_USE_ADC | MC13783_USE_REGULATOR |
 		 MC13783_USE_TOUCHSCREEN,
 };
diff --git a/arch/arm/mach-mx3/mach-mx31_3ds.c b/arch/arm/mach-mx3/mach-mx31_3ds.c
index 0d65db8..3e613ee 100644
--- a/arch/arm/mach-mx3/mach-mx31_3ds.c
+++ b/arch/arm/mach-mx3/mach-mx31_3ds.c
@@ -156,9 +156,13 @@ static struct mc13783_regulator_init_data mx31_3ds_regulators[] = {
 };
 
 /* MC13783 */
-static struct mc13783_platform_data mc13783_pdata __initdata = {
+static struct mc13783_regulator_platform_data mc13783_regs = {
 	.regulators = mx31_3ds_regulators,
 	.num_regulators = ARRAY_SIZE(mx31_3ds_regulators),
+};
+
+static struct mc13783_platform_data mc13783_pdata __initdata = {
+	.regulators = &mc13783_regs,
 	.flags  = MC13783_USE_REGULATOR | MC13783_USE_TOUCHSCREEN,
 };
 
diff --git a/arch/arm/mach-mx3/mach-mx31moboard.c b/arch/arm/mach-mx3/mach-mx31moboard.c
index 1aa8d65..424fbd9 100644
--- a/arch/arm/mach-mx3/mach-mx31moboard.c
+++ b/arch/arm/mach-mx3/mach-mx31moboard.c
@@ -267,9 +267,13 @@ static struct mc13783_leds_platform_data moboard_leds = {
 	.tc2_period = MC13783_LED_PERIOD_10MS,
 };
 
-static struct mc13783_platform_data moboard_pmic = {
+static struct mc13783_regulator_platform_data moboard_regs = {
 	.regulators = moboard_regulators,
 	.num_regulators = ARRAY_SIZE(moboard_regulators),
+};
+
+static struct mc13783_platform_data moboard_pmic = {
+	.regulators = &moboard_regs,
 	.leds = &moboard_leds,
 	.flags = MC13783_USE_REGULATOR | MC13783_USE_RTC |
 		MC13783_USE_ADC | MC13783_USE_LED,
diff --git a/drivers/leds/leds-mc13783.c b/drivers/leds/leds-mc13783.c
index f05bb08..687fb13 100644
--- a/drivers/leds/leds-mc13783.c
+++ b/drivers/leds/leds-mc13783.c
@@ -30,6 +30,7 @@ struct mc13783_led {
 	struct mc13783		*master;
 	enum led_brightness	new_brightness;
 	int			id;
+	int			num_leds;
 };
 
 #define MC13783_REG_LED_CONTROL_0	51
@@ -181,9 +182,9 @@ static int __devinit mc13783_led_setup(struct mc13783_led *led, int max_current)
 	return ret;
 }
 
-static int __devinit mc13783_leds_prepare(struct platform_device *pdev)
+static int __devinit mc13783_leds_prepare(struct platform_device *pdev,
+		struct mc13783_leds_platform_data *pdata)
 {
-	struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct mc13783 *dev = dev_get_drvdata(pdev->dev.parent);
 	int ret = 0;
 	int reg = 0;
@@ -264,7 +265,7 @@ out:
 
 static int __devinit mc13783_led_probe(struct platform_device *pdev)
 {
-	struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct mc13783_leds_platform_data *pdata = platform_get_drvdata(pdev);
 	struct mc13783_led_platform_data *led_cur;
 	struct mc13783_led *led, *led_dat;
 	int ret, i;
@@ -286,12 +287,15 @@ static int __devinit mc13783_led_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	ret = mc13783_leds_prepare(pdev);
+	ret = mc13783_leds_prepare(pdev, pdata);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to init led driver\n");
 		goto err_free;
 	}
 
+	/* no need to save the num of LEDs for any other elements of 'led' */
+	led[0].num_leds = pdata->num_leds;
+
 	for (i = 0; i < pdata->num_leds; i++) {
 		led_dat = &led[i];
 		led_cur = &pdata->led[i];
@@ -351,12 +355,11 @@ err_free:
 
 static int __devexit mc13783_led_remove(struct platform_device *pdev)
 {
-	struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct mc13783_led *led = platform_get_drvdata(pdev);
 	struct mc13783 *dev = dev_get_drvdata(pdev->dev.parent);
 	int i;
 
-	for (i = 0; i < pdata->num_leds; i++) {
+	for (i = 0; i < led[0].num_leds; i++) {
 		led_classdev_unregister(&led[i].cdev);
 		cancel_work_sync(&led[i].work);
 	}
diff --git a/drivers/mfd/mc13xxx-core.c b/drivers/mfd/mc13xxx-core.c
index b9fcaf0..96842d9 100644
--- a/drivers/mfd/mc13xxx-core.c
+++ b/drivers/mfd/mc13xxx-core.c
@@ -683,14 +683,13 @@ out:
 EXPORT_SYMBOL_GPL(mc13783_adc_do_conversion);
 
 static int mc13xxx_add_subdevice_pdata(struct mc13xxx *mc13xxx,
-		const char *format, void *pdata, size_t pdata_size)
+		const char *format, void *pdata)
 {
 	char buf[30];
 	const char *name = mc13xxx_get_chipname(mc13xxx);
 
 	struct mfd_cell cell = {
-		.platform_data = pdata,
-		.data_size = pdata_size,
+		.driver_data = pdata,
 	};
 
 	/* there is no asnprintf in the kernel :-( */
@@ -706,7 +705,7 @@ static int mc13xxx_add_subdevice_pdata(struct mc13xxx *mc13xxx,
 
 static int mc13xxx_add_subdevice(struct mc13xxx *mc13xxx, const char *format)
 {
-	return mc13xxx_add_subdevice_pdata(mc13xxx, format, NULL, 0);
+	return mc13xxx_add_subdevice_pdata(mc13xxx, format, NULL);
 }
 
 static int mc13xxx_probe(struct spi_device *spi)
@@ -764,13 +763,8 @@ err_revision:
 		mc13xxx_add_subdevice(mc13xxx, "%s-codec");
 
 	if (pdata->flags & MC13XXX_USE_REGULATOR) {
-		struct mc13xxx_regulator_platform_data regulator_pdata = {
-			.num_regulators = pdata->num_regulators,
-			.regulators = pdata->regulators,
-		};
-
 		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-regulator",
-				&regulator_pdata, sizeof(regulator_pdata));
+				pdata->regulators);
 	}
 
 	if (pdata->flags & MC13XXX_USE_RTC)
@@ -779,10 +773,8 @@ err_revision:
 	if (pdata->flags & MC13XXX_USE_TOUCHSCREEN)
 		mc13xxx_add_subdevice(mc13xxx, "%s-ts");
 
-	if (pdata->flags & MC13XXX_USE_LED) {
-		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led",
-					pdata->leds, sizeof(*pdata->leds));
-	}
+	if (pdata->flags & MC13XXX_USE_LED)
+		mc13xxx_add_subdevice_pdata(mc13xxx, "%s-led", pdata->leds);
 
 	return 0;
 }
diff --git a/drivers/regulator/mc13783-regulator.c b/drivers/regulator/mc13783-regulator.c
index 3e5d0c3..40748b7 100644
--- a/drivers/regulator/mc13783-regulator.c
+++ b/drivers/regulator/mc13783-regulator.c
@@ -337,7 +337,7 @@ static int __devinit mc13783_regulator_probe(struct platform_device *pdev)
 	struct mc13xxx_regulator_priv *priv;
 	struct mc13xxx *mc13783 = dev_get_drvdata(pdev->dev.parent);
 	struct mc13783_regulator_platform_data *pdata =
-		dev_get_platdata(&pdev->dev);
+			platform_get_drvdata(pdev);
 	struct mc13783_regulator_init_data *init_data;
 	int i, ret;
 
@@ -351,6 +351,7 @@ static int __devinit mc13783_regulator_probe(struct platform_device *pdev)
 
 	priv->mc13xxx_regulators = mc13783_regulators;
 	priv->mc13xxx = mc13783;
+	priv->pdata = pdata;
 
 	for (i = 0; i < pdata->num_regulators; i++) {
 		init_data = &pdata->regulators[i];
@@ -381,8 +382,7 @@ err:
 static int __devexit mc13783_regulator_remove(struct platform_device *pdev)
 {
 	struct mc13xxx_regulator_priv *priv = platform_get_drvdata(pdev);
-	struct mc13783_regulator_platform_data *pdata =
-		dev_get_platdata(&pdev->dev);
+	struct mc13783_regulator_platform_data *pdata = priv->pdata;
 	int i;
 
 	platform_set_drvdata(pdev, NULL);
diff --git a/drivers/regulator/mc13892-regulator.c b/drivers/regulator/mc13892-regulator.c
index 1b8f739..4b3490b 100644
--- a/drivers/regulator/mc13892-regulator.c
+++ b/drivers/regulator/mc13892-regulator.c
@@ -521,7 +521,7 @@ static int __devinit mc13892_regulator_probe(struct platform_device *pdev)
 	struct mc13xxx_regulator_priv *priv;
 	struct mc13xxx *mc13892 = dev_get_drvdata(pdev->dev.parent);
 	struct mc13xxx_regulator_platform_data *pdata =
-		dev_get_platdata(&pdev->dev);
+			platform_get_drvdata(pdev);
 	struct mc13xxx_regulator_init_data *init_data;
 	int i, ret;
 	u32 val;
@@ -534,6 +534,7 @@ static int __devinit mc13892_regulator_probe(struct platform_device *pdev)
 
 	priv->mc13xxx_regulators = mc13892_regulators;
 	priv->mc13xxx = mc13892;
+	priv->pdata = pdata;
 
 	mc13xxx_lock(mc13892);
 	ret = mc13xxx_reg_read(mc13892, MC13892_REVISION, &val);
@@ -595,8 +596,7 @@ err_free:
 static int __devexit mc13892_regulator_remove(struct platform_device *pdev)
 {
 	struct mc13xxx_regulator_priv *priv = platform_get_drvdata(pdev);
-	struct mc13xxx_regulator_platform_data *pdata =
-		dev_get_platdata(&pdev->dev);
+	struct mc13xxx_regulator_platform_data *pdata = priv->pdata;
 	int i;
 
 	platform_set_drvdata(pdev, NULL);
diff --git a/drivers/regulator/mc13xxx.h b/drivers/regulator/mc13xxx.h
index 2775826..5430177 100644
--- a/drivers/regulator/mc13xxx.h
+++ b/drivers/regulator/mc13xxx.h
@@ -28,6 +28,7 @@ struct mc13xxx_regulator {
 struct mc13xxx_regulator_priv {
 	struct mc13xxx *mc13xxx;
 	u32 powermisc_pwgt_state;
+	struct mc13xxx_regulator_platform_data *pdata;
 	struct mc13xxx_regulator *mc13xxx_regulators;
 	struct regulator_dev *regulators[];
 };
diff --git a/include/linux/mfd/mc13xxx.h b/include/linux/mfd/mc13xxx.h
index a1d391b..052b133 100644
--- a/include/linux/mfd/mc13xxx.h
+++ b/include/linux/mfd/mc13xxx.h
@@ -146,8 +146,7 @@ struct mc13xxx_platform_data {
 #define MC13XXX_USE_LED		(1 << 5)
 	unsigned int flags;
 
-	int num_regulators;
-	struct mc13xxx_regulator_init_data *regulators;
+	struct mc13xxx_regulator_platform_data *regulators;
 	struct mc13xxx_leds_platform_data *leds;
 };
 
-- 
1.7.2.3

[PATCH 04/19] ab3100: mfd_cell is now implicitly available to drivers

[Linus Walleij]

On 02/03/2011 05:03 AM, Andres Salomon wrote:
> No need to explicitly set the cell's platform_data/data_size.
>
> In this case, move the ab3100_platform_data pointer from platform_data
> to driver_data.  The only client who makes use of it is also changed.
>
> Signed-off-by: Andres Salomon<dilinger@queued.net>
>    

Looks good, thanks Andres,
Acked-by: Linus Walleij <linus.walleij@stericsson.com>

Yours,
Linus Walleij

[PATCH 03/19] ab3550: mfd_cell is now implicitly available to drivers

[Mattias Wallin]

Hi Andres,
Thanks for your input on ab3550. You are absolutely right the platform 
data is not used in mainline right now. However we have a few ab3550 
drivers (regulator, gpadc..) that uses the platform data. They have been 
lying on the shelf for a while now but I at least plan to mainline them 
at some point.

BR,
/Mattias Wallin

On 02/03/2011 05:01 AM, Andres Salomon wrote:
>
> No need to explicitly set the cell's platform_data/data_size.
>
> This wasn't actually used anywhere by the ab3550 stuff; dev_data
> in mach-u300's i2c code was empty.
>
> Signed-off-by: Andres Salomon<dilinger@queued.net>
> ---
>   arch/arm/mach-u300/i2c.c   |    2 --
>   drivers/mfd/ab3550-core.c  |    6 ------
>   include/linux/mfd/abx500.h |    2 --
>   3 files changed, 0 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-u300/i2c.c b/arch/arm/mach-u300/i2c.c
> index f0394ba..1ff7a82 100644
> --- a/arch/arm/mach-u300/i2c.c
> +++ b/arch/arm/mach-u300/i2c.c
> @@ -291,8 +291,6 @@ static struct ab3550_platform_data ab3550_plf_data = {
>   		.base = IRQ_AB3550_BASE,
>   		.count = (IRQ_AB3550_END - IRQ_AB3550_BASE + 1),
>   	},
> -	.dev_data = {
> -	},
>   	.init_settings = ab3550_init_settings,
>   	.init_settings_sz = ARRAY_SIZE(ab3550_init_settings),
>   };
> diff --git a/drivers/mfd/ab3550-core.c b/drivers/mfd/ab3550-core.c
> index 5fbca34..47625b9 100644
> --- a/drivers/mfd/ab3550-core.c
> +++ b/drivers/mfd/ab3550-core.c
> @@ -1319,12 +1319,6 @@ static int __init ab3550_probe(struct i2c_client *client,
>   	if (err)
>   		goto exit_no_ops;
>
> -	/* Set up and register the platform devices. */
> -	for (i = 0; i<  AB3550_NUM_DEVICES; i++) {
> -		ab3550_devs[i].platform_data = ab3550_plf_data->dev_data[i];
> -		ab3550_devs[i].data_size = ab3550_plf_data->dev_data_sz[i];
> -	}
> -
>   	err = mfd_add_devices(&client->dev, 0, ab3550_devs,
>   		ARRAY_SIZE(ab3550_devs), NULL,
>   		ab3550_plf_data->irq.base);
> diff --git a/include/linux/mfd/abx500.h b/include/linux/mfd/abx500.h
> index 67bd6f7..a735210 100644
> --- a/include/linux/mfd/abx500.h
> +++ b/include/linux/mfd/abx500.h
> @@ -185,8 +185,6 @@ struct abx500_init_settings {
>    */
>   struct ab3550_platform_data {
>   	struct {unsigned int base; unsigned int count; } irq;
> -	void *dev_data[AB3550_NUM_DEVICES];
> -	size_t dev_data_sz[AB3550_NUM_DEVICES];
>   	struct abx500_init_settings *init_settings;
>   	unsigned int init_settings_sz;
>   };

[PATCH 15/19] mc13xxx: mfd_cell is now implicitly available to drivers

[Uwe Kleine-König]

Hello Andres,

On Wed, Feb 02, 2011 at 08:20:15PM -0800, Andres Salomon wrote:
> 
> No need to explicitly set the cell's platform_data/data_size.
> 
> In this case, move the various platform_data pointers
> to driver_data.  All of the clients which make use of it
> are also changed.
> 
> Mfd-core makes a copy of platform_data, but driver_data keeps a pointer
> to the original data.  Because each cell's platform_data previously
> pointed to a local (stack) variable, the various ARM mach types that set
> the pdata are updated to keep the memory around.
I didn't get this even after reading it 5 times.  You wrote in the
subject that drivers now have access to mfd_cell.  I don't see where
e.g. drivers/leds/leds-mc13783.c uses that?!  Does this depend on some
mfd-changes I don't see and this is just a first step?

After reading the changes I think I understood:

 - You made things that were passed as platform_data before available
   via driver_data.
 - Because platform_data is copied and driver_data is not at register
   time, the data being platform_data cannot be __initdata or stack
   local anymore, so this needs fixing.

In sum this results in .data becoming bigger (which is bad).

And I think this patch has a conceptual problem, too.  In my opionion
platform_data is the point to hand over platform specific data to a
driver.  driver_data is something that is private to the driver and has
to be considered opaque for the platform.  The driver was sort of OK
before ...

> diff --git a/arch/arm/mach-imx/mach-mx27_3ds.c b/arch/arm/mach-imx/mach-mx27_3ds.c
> index 1643315..edcecaa 100644
> --- a/arch/arm/mach-imx/mach-mx27_3ds.c
> +++ b/arch/arm/mach-imx/mach-mx27_3ds.c
> @@ -226,10 +226,14 @@ static struct mc13783_regulator_init_data mx27_3ds_regulators[] = {
>  	},
>  };
>  
> -/* MC13783 */
> -static struct mc13783_platform_data mc13783_pdata __initdata = {
> +static struct mc13783_regulator_platform_data mx27_regs = {
The prefix mx27 is misleading. If you want to take the machine name
into account use mx27_3ds, otherwise just use mc13783_regulator_pdata or
similar. (And note that "regs" isn't that good IMHO, too, as it might
stand for registers, too. And even mx27_3ds_regulators would be too
general. This applies to the changes below, too.)

>  	.regulators = mx27_3ds_regulators,
>  	.num_regulators = ARRAY_SIZE(mx27_3ds_regulators),
> +};
> +
> +/* MC13783 */
a very minor nitpick is you moved the comment for mx27_3ds, but not for
e.g. mx31_3ds.

> +static struct mc13783_platform_data mc13783_pdata __initdata = {
> +	.regulators = &mx27_regs,
>  	.flags  = MC13783_USE_REGULATOR,
>  };
>  
> diff --git a/arch/arm/mach-imx/mach-pcm038.c b/arch/arm/mach-imx/mach-pcm038.c
> index 5056148..b5ae8cc 100644
> --- a/arch/arm/mach-imx/mach-pcm038.c
> +++ b/arch/arm/mach-imx/mach-pcm038.c
> @@ -262,9 +262,13 @@ static struct mc13783_regulator_init_data pcm038_regulators[] = {
>  	},
>  };
>  
> -static struct mc13783_platform_data pcm038_pmic = {
> +static struct mc13783_regulator_platform_data pcm038_regs = {
>  	.regulators = pcm038_regulators,
>  	.num_regulators = ARRAY_SIZE(pcm038_regulators),
> +};
> +
> +static struct mc13783_platform_data pcm038_pmic = {
> +	.regulators = &pcm038_regs,
>  	.flags = MC13783_USE_ADC | MC13783_USE_REGULATOR |
>  		 MC13783_USE_TOUCHSCREEN,
>  };
> diff --git a/arch/arm/mach-mx3/mach-mx31_3ds.c b/arch/arm/mach-mx3/mach-mx31_3ds.c
> index 0d65db8..3e613ee 100644
> --- a/arch/arm/mach-mx3/mach-mx31_3ds.c
> +++ b/arch/arm/mach-mx3/mach-mx31_3ds.c
> @@ -156,9 +156,13 @@ static struct mc13783_regulator_init_data mx31_3ds_regulators[] = {
>  };
>  
>  /* MC13783 */
> -static struct mc13783_platform_data mc13783_pdata __initdata = {
> +static struct mc13783_regulator_platform_data mc13783_regs = {
>  	.regulators = mx31_3ds_regulators,
>  	.num_regulators = ARRAY_SIZE(mx31_3ds_regulators),
> +};
> +
> +static struct mc13783_platform_data mc13783_pdata __initdata = {
> +	.regulators = &mc13783_regs,
>  	.flags  = MC13783_USE_REGULATOR | MC13783_USE_TOUCHSCREEN,
>  };
>  
> diff --git a/arch/arm/mach-mx3/mach-mx31moboard.c b/arch/arm/mach-mx3/mach-mx31moboard.c
> index 1aa8d65..424fbd9 100644
> --- a/arch/arm/mach-mx3/mach-mx31moboard.c
> +++ b/arch/arm/mach-mx3/mach-mx31moboard.c
> @@ -267,9 +267,13 @@ static struct mc13783_leds_platform_data moboard_leds = {
>  	.tc2_period = MC13783_LED_PERIOD_10MS,
>  };
>  
> -static struct mc13783_platform_data moboard_pmic = {
> +static struct mc13783_regulator_platform_data moboard_regs = {
>  	.regulators = moboard_regulators,
>  	.num_regulators = ARRAY_SIZE(moboard_regulators),
> +};
> +
> +static struct mc13783_platform_data moboard_pmic = {
> +	.regulators = &moboard_regs,
>  	.leds = &moboard_leds,
>  	.flags = MC13783_USE_REGULATOR | MC13783_USE_RTC |
>  		MC13783_USE_ADC | MC13783_USE_LED,
> diff --git a/drivers/leds/leds-mc13783.c b/drivers/leds/leds-mc13783.c
> index f05bb08..687fb13 100644
> --- a/drivers/leds/leds-mc13783.c
> +++ b/drivers/leds/leds-mc13783.c
> @@ -30,6 +30,7 @@ struct mc13783_led {
>  	struct mc13783		*master;
>  	enum led_brightness	new_brightness;
>  	int			id;
> +	int			num_leds;
>  };
>  
>  #define MC13783_REG_LED_CONTROL_0	51
> @@ -181,9 +182,9 @@ static int __devinit mc13783_led_setup(struct mc13783_led *led, int max_current)
>  	return ret;
>  }
>  
> -static int __devinit mc13783_leds_prepare(struct platform_device *pdev)
> +static int __devinit mc13783_leds_prepare(struct platform_device *pdev,
> +		struct mc13783_leds_platform_data *pdata)
>  {
> -	struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev);
>  	struct mc13783 *dev = dev_get_drvdata(pdev->dev.parent);
>  	int ret = 0;
>  	int reg = 0;
> @@ -264,7 +265,7 @@ out:
>  
>  static int __devinit mc13783_led_probe(struct platform_device *pdev)
>  {
> -	struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct mc13783_leds_platform_data *pdata = platform_get_drvdata(pdev);
>  	struct mc13783_led_platform_data *led_cur;
>  	struct mc13783_led *led, *led_dat;
>  	int ret, i;
> @@ -286,12 +287,15 @@ static int __devinit mc13783_led_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	ret = mc13783_leds_prepare(pdev);
> +	ret = mc13783_leds_prepare(pdev, pdata);
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to init led driver\n");
>  		goto err_free;
>  	}
>  
> +	/* no need to save the num of LEDs for any other elements of 'led' */
> +	led[0].num_leds = pdata->num_leds;
> +
So for n leds you introduced n-1 useless ints.

So all in all, I don't like that change, maybe just because I don't
understand your motivation.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 15/19] mc13xxx: mfd_cell is now implicitly available to drivers

[Uwe Kleine-König]

Hello,

On Fri, Feb 04, 2011 at 10:34:58AM +0100, Uwe Kleine-K?nig wrote:
> And I think this patch has a conceptual problem, too.  In my opionion
> platform_data is the point to hand over platform specific data to a
> driver.  driver_data is something that is private to the driver and has
> to be considered opaque for the platform.  The driver was sort of OK
> before ...

So consequently I propose the patch below.  I'm sure that a few drivers
will break, but IMHO that's OK.

And by the way, did you know that platform_set_drvdata can fail and you
have no nice way to notice that but to do:

	dev_set_drvdata(dev, mypreciousdata);
	if (dev_get_drvdata(dev) != mypreciousdata)
		goto fail;

Best regards
Uwe

------>8-----------
From: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Date: Fri, 4 Feb 2011 11:00:42 +0100
Subject: [PATCH] MFD: platform_set_drvdata should be only called by drivers

driver data is data private to drivers so it's not a point to hand over
data to the driver by the platform.  That's what platform_data is available
for.  The only place platform_set_drvdata should be called is in a
driver that has bound the device in question.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 drivers/mfd/mfd-core.c   |    1 -
 include/linux/mfd/core.h |    3 ---
 2 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d83ad0f..8d06e0c 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -37,7 +37,6 @@ static int mfd_add_device(struct device *parent, int id,
 		goto fail_device;
 
 	pdev->dev.parent = parent;
-	platform_set_drvdata(pdev, cell->driver_data);
 
 	if (cell->data_size) {
 		ret = platform_device_add_data(pdev,
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 835996e..88bb7b5 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -30,9 +30,6 @@ struct mfd_cell {
 	int			(*suspend)(struct platform_device *dev);
 	int			(*resume)(struct platform_device *dev);
 
-	/* driver-specific data for MFD-aware "cell" drivers */
-	void			*driver_data;
-
 	/* platform_data can be used to either pass data to "generic"
 	   driver or as a hook to mfd_cell for the "cell" drivers */
 	void			*platform_data;
-- 
1.7.2.3

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 15/19] mc13xxx: mfd_cell is now implicitly available to drivers

[Andres Salomon]

On Fri, 4 Feb 2011 10:34:58 +0100
Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:

> Hello Andres,
> 
> On Wed, Feb 02, 2011 at 08:20:15PM -0800, Andres Salomon wrote:
> > 
> > No need to explicitly set the cell's platform_data/data_size.
> > 
> > In this case, move the various platform_data pointers
> > to driver_data.  All of the clients which make use of it
> > are also changed.
> > 
> > Mfd-core makes a copy of platform_data, but driver_data keeps a
> > pointer to the original data.  Because each cell's platform_data
> > previously pointed to a local (stack) variable, the various ARM
> > mach types that set the pdata are updated to keep the memory around.
> I didn't get this even after reading it 5 times.  You wrote in the
> subject that drivers now have access to mfd_cell.  I don't see where
> e.g. drivers/leds/leds-mc13783.c uses that?!  Does this depend on some
> mfd-changes I don't see and this is just a first step?
> 
> After reading the changes I think I understood:
> 
>  - You made things that were passed as platform_data before available
>    via driver_data.

Right.  And as someone pointed out, this doesn't really work as well as
I'd hoped, so I'll have to refine my approach.  Ideally, something
simpler..


>  - Because platform_data is copied and driver_data is not at register
>    time, the data being platform_data cannot be __initdata or stack
>    local anymore, so this needs fixing.
> 
> In sum this results in .data becoming bigger (which is bad).
> 
> And I think this patch has a conceptual problem, too.  In my opionion
> platform_data is the point to hand over platform specific data to a
> driver.  driver_data is something that is private to the driver and
> has to be considered opaque for the platform.  The driver was sort of
> OK before ...


I'll be sending updated patches once I've reworked things.