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

Subsystems: arm port, led subsystem, multifunction devices (mfd), the rest, voltage and current regulator framework

STALE5597d

4 messages, 2 authors, 2011-02-04 · open the first message on its own page

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

From: Andres Salomon <hidden>
Date: 2011-02-03 04:20:20

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 <redacted>
---
 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

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

From: Uwe Kleine-König <hidden>
Date: 2011-02-04 09:35:13

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 ...
quoted hunk
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.
quoted hunk
+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/  |

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

From: Uwe Kleine-König <hidden>
Date: 2011-02-04 10:13:48

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 <redacted>
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 <redacted>
---
 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/  |

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

From: Andres Salomon <hidden>
Date: 2011-02-04 10:16:48

On Fri, 4 Feb 2011 10:34:58 +0100
Uwe Kleine-K?nig [off-list ref] wrote:
Hello Andres,

On Wed, Feb 02, 2011 at 08:20:15PM -0800, Andres Salomon wrote:
quoted
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help