mc13xxx: mfd_cell is now implicitly available to drivers

PATCH In review Andres Salomon 4 participants 11 messages Feb 12, 2011 – Feb 19, 2011

Subsystems (6)

Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape Multifunction devices (MFD) +2 more subsystems

lore

Share & tools

lore

Andres Salomon

author

2011-02-12 02:19 UTC #0 lore raw

PATCH 16/17 mc13xxx: mfd_cell is now implicitly available to drivers

The cell's platform_data is now accessed with a helper function; change clients to use that, and remove the now-unused data_size.

Note that mfd-core no longer makes a copy of platform_data, but the mc13xxx-core driver creates the pdata structures on the stack. In order to get around that, the various ARM mach types that set the pdata have been changed to hold the variable in static (global) memory. Also note that __initdata references in aforementioned pdata structs have been dropped.

Signed-off-by Andres Salomon <[email protected]>

---
 arch/arm/mach-imx/mach-mx27_3ds.c     |    6 +++++-
 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           |    6 +++---
 drivers/mfd/mc13xxx-core.c            |   18 +++++-------------
 drivers/regulator/mc13783-regulator.c |    6 ++----
 drivers/regulator/mc13892-regulator.c |    6 ++----
 include/linux/mfd/mc13xxx.h           |    3 +--
 9 files changed, 33 insertions(+), 30 deletions(-)

arch/arm/mach-imx/mach-mx27_3ds.c +5 -1

index 1643315..f223371 100644
@@ -227,9 +227,13 @@ 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_3ds_regs = {
 	.regulators = mx27_3ds_regulators,
 	.num_regulators = ARRAY_SIZE(mx27_3ds_regulators),
+};
+
+static struct mc13783_platform_data mc13783_pdata = {
+	.regulators = &mx27_3ds_regs,
 	.flags  = MC13783_USE_REGULATOR,
 };

arch/arm/mach-imx/mach-pcm038.c +5 -1

index 5056148..4b37494 100644
@@ -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_pmic_regs = {
 	.regulators = pcm038_regulators,
 	.num_regulators = ARRAY_SIZE(pcm038_regulators),
+};
+
+static struct mc13783_platform_data pcm038_pmic = {
+	.regulators = &pcm038_pmic_regs,
 	.flags = MC13783_USE_ADC | MC13783_USE_REGULATOR |
 		 MC13783_USE_TOUCHSCREEN,
 };

arch/arm/mach-mx3/mach-mx31_3ds.c +5 -1

index 0d65db8..8005788 100644
@@ -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 mx31_3ds_regs = {
 	.regulators = mx31_3ds_regulators,
 	.num_regulators = ARRAY_SIZE(mx31_3ds_regulators),
+};
+
+static struct mc13783_platform_data mc13783_pdata = {
+	.regulators = &mx31_3ds_regs,
 	.flags  = MC13783_USE_REGULATOR | MC13783_USE_TOUCHSCREEN,
 };

arch/arm/mach-mx3/mach-mx31moboard.c +5 -1

index 1aa8d65..e824d2e 100644
@@ -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_pmic_regs = {
 	.regulators = moboard_regulators,
 	.num_regulators = ARRAY_SIZE(moboard_regulators),
+};
+
+static struct mc13783_platform_data moboard_pmic = {
+	.regulators = &moboard_pmic_regs,
 	.leds = &moboard_leds,
 	.flags = MC13783_USE_REGULATOR | MC13783_USE_RTC |
 		MC13783_USE_ADC | MC13783_USE_LED,

drivers/leds/leds-mc13783.c +3 -3

index f05bb08..0d87477 100644
@@ -183,7 +183,7 @@ static int __devinit mc13783_led_setup(struct mc13783_led *led, int max_current)
 
 static int __devinit mc13783_leds_prepare(struct platform_device *pdev)
 {
-	struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct mc13783_leds_platform_data *pdata = mfd_get_data(pdev);
 	struct mc13783 *dev = dev_get_drvdata(pdev->dev.parent);
 	int ret = 0;
 	int reg = 0;

@@ -264,7 +264,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 = mfd_get_data(pdev);
 	struct mc13783_led_platform_data *led_cur;
 	struct mc13783_led *led, *led_dat;
 	int ret, i;

@@ -351,7 +351,7 @@ 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_leds_platform_data *pdata = mfd_get_data(pdev);
 	struct mc13783_led *led = platform_get_drvdata(pdev);
 	struct mc13783 *dev = dev_get_drvdata(pdev->dev.parent);
 	int i;

drivers/mfd/mc13xxx-core.c +5 ~~-13~~

index b9fcaf0..00c5490 100644
@@ -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,
 	};
 
 	/* 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;
 }

drivers/regulator/mc13783-regulator.c +2 -4

index 3e5d0c3..1b7cc16 100644
@@ -336,8 +336,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);
+	struct mc13783_regulator_platform_data *pdata = mfd_get_data(pdev);
 	struct mc13783_regulator_init_data *init_data;
 	int i, ret;

@@ -381,8 +380,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 = mfd_get_data(pdev);
 	int i;
 
 	platform_set_drvdata(pdev, NULL);

drivers/regulator/mc13892-regulator.c +2 -4

index 1b8f739..e23578a 100644
@@ -520,8 +520,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);
+	struct mc13xxx_regulator_platform_data *pdata = mfd_get_data(pdev);
 	struct mc13xxx_regulator_init_data *init_data;
 	int i, ret;
 	u32 val;

@@ -595,8 +594,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 = mfd_get_data(pdev);
 	int i;
 
 	platform_set_drvdata(pdev, NULL);

include/linux/mfd/mc13xxx.h +1 -2

index a1d391b..052b133 100644
@@ -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;
 };

Signature · 1 line

1.7.2.3

Uwe Kleine-König

2011-02-16 09:41 UTC #1 lore raw

Quoted 5 lines · Andres Salomon

The cell's platform_data is now accessed with a helper function; change clients to use that, and remove the now-unused data_size.
Note that mfd-core no longer makes a copy of platform_data, but the

why was this changed and where? I'm not able to find your complete series via gmane.

Quoted 24 lines

mc13xxx-core driver creates the pdata structures on the stack. In order to get around that, the various ARM mach types that set the pdata have been changed to hold the variable in static (global) memory. Also note that __initdata references in aforementioned pdata structs have been dropped.
Signed-off-by Andres Salomon <[email protected]>
---
 arch/arm/mach-imx/mach-mx27_3ds.c     |    6 +++++-
 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           |    6 +++---
 drivers/mfd/mc13xxx-core.c            |   18 +++++-------------
 drivers/regulator/mc13783-regulator.c |    6 ++----
 drivers/regulator/mc13892-regulator.c |    6 ++----
 include/linux/mfd/mc13xxx.h           |    3 +--
 9 files changed, 33 insertions(+), 30 deletions(-)
arch/arm/mach-imx/mach-mx27_3ds.c +0 -0
index 1643315..f223371 100644

arch/arm/mach-imx/mach-mx27_3ds.c static struct mc13783_regulator_init_data mx27_3ds_regulators[] = { · line 230

Quoted 4 lines

arch/arm/mach-imx/mach-mx27_3ds.c +0 -1

@@ -227,9 +227,13 @@ static struct mc13783_regulator_init_data mx27_3ds_regulators[] = {
 };
 
 /* MC13783 */
-static struct mc13783_platform_data mc13783_pdata __initdata = {

isn't using __initdata here broken already before your patch? Was introduced in c67a3e09. Fabio?

But in general I'd prefer to keep as much __initdata as possible because this occupies less memory when using a multi-machine kernel.

include/linux/mfd/mc13xxx.h struct mc13xxx_platform_data { · line 151

Quoted 13 lines

include/linux/mfd/mc13xxx.h +1 -2

index a1d391b..052b133 100644
@@ -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;
 };

... so I'd prefer to embed a struct mc13xxx_regulator_platform_data into mc13xxx_platform_data, not a pointer to it. This makes initialisation a bit easier, too.

Best regards
Uwe

Signature · 2 lines

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

Fabio Estevam

2011-02-16 12:26 UTC #2 lore raw

Hi Uwe,

On 2/16/2011 7:41 AM, Uwe Kleine-K?nig wrote:
...

Quoted 5 lines

Quoted 3 lines
/* MC13783 */
-static struct mc13783_platform_data mc13783_pdata __initdata = {
isn't using __initdata here broken already before your patch? Was introduced in c67a3e09. Fabio?

I followed the same approach as in the mx31_3ds.c file.

Quoted 2 lines

But in general I'd prefer to keep as much __initdata as possible because this occupies less memory when using a multi-machine kernel.

So keeping __initdata is OK then? Let me know and I can fix it needed.

Regards,

Fabio Estevam

2011-02-16 12:31 UTC #3 lore raw

In reply to Uwe Kleine-König

Hi Uwe,

On 2/16/2011 7:41 AM, Uwe Kleine-K?nig wrote:
...

Quoted 4 lines

Quoted 2 lines
/* MC13783 */
-static struct mc13783_platform_data mc13783_pdata __initdata = {
isn't using __initdata here broken already before your patch? Was introduced in c67a3e09. Fabio?

I followed the same approach as in mx31_3ds.c file.

Quoted 2 lines

But in general I'd prefer to keep as much __initdata as possible because this occupies less memory when using a multi-machine kernel.

Ok, is the __initdata usage correct then? If not, I can change it. Just let me know.

Regards,

Fabio Estevam

Uwe Kleine-König

2011-02-16 12:58 UTC #4 lore raw

PATCH 16/17 mc13xxx: mfd_cell is now implicitly available to drivers

Quoted 9 lines · Fabio Estevam

Hi Uwe,
On 2/16/2011 7:41 AM, Uwe Kleine-K?nig wrote:
...
Quoted 4 lines
Quoted 2 lines
/* MC13783 */
-static struct mc13783_platform_data mc13783_pdata __initdata = {
isn't using __initdata here broken already before your patch? Was introduced in c67a3e09. Fabio?

Quoted 6 lines

I followed the same approach as in mx31_3ds.c file.
Quoted 2 lines
But in general I'd prefer to keep as much __initdata as possible because this occupies less memory when using a multi-machine kernel.
Ok, is the __initdata usage correct then? If not, I can change it. Just let me know.

I guess that if you compile mc13xxx support as module it won't work.

Try that, and if it breaks, send a patch fixing mx31_3ds.c, too.

Uwe

Signature · 2 lines

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

Fabio Estevam

reviewer

2011-02-17 11:53 UTC #5 lore raw

PATCH 16/17 mc13xxx: mfd_cell is now implicitly available to drivers

Hi Uwe,

2011/2/16 Uwe Kleine-K?nig <[email protected]>:

Quoted 12 lines · Fabio Estevam

Quoted 11 lines
Hi Uwe,
On 2/16/2011 7:41 AM, Uwe Kleine-K?nig wrote:
...
Quoted 4 lines
Quoted 2 lines
?/* MC13783 */
-static struct mc13783_platform_data mc13783_pdata __initdata = {
isn't using __initdata here broken already before your patch? ?Was introduced in c67a3e09. ?Fabio?
I followed the same approach as in mx31_3ds.c file.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
But in general I'd prefer to keep as much __initdata as possible because this occupies less memory when using a multi-machine kernel.
Ok, is the __initdata usage correct then? If not, I can change it. Just let me know.
I guess that if you compile mc13xxx support as module it won't work.
Try that, and if it breaks, send a patch fixing mx31_3ds.c, too.

I built it as module and the build went fine.

Regards,

Fabio Estevam

Uwe Kleine-König

2011-02-17 13:10 UTC #6 lore raw

PATCH 16/17 mc13xxx: mfd_cell is now implicitly available to drivers

Quoted 18 lines · Fabio Estevam

Hi Uwe,
2011/2/16 Uwe Kleine-K?nig <[email protected]>:
Quoted 15 lines · Fabio Estevam
Quoted 14 lines
Hi Uwe,
On 2/16/2011 7:41 AM, Uwe Kleine-K?nig wrote:
...
Quoted 4 lines
Quoted 2 lines
?/* MC13783 */
-static struct mc13783_platform_data mc13783_pdata __initdata = {
isn't using __initdata here broken already before your patch? ?Was introduced in c67a3e09. ?Fabio?
I followed the same approach as in mx31_3ds.c file.
Quoted 2 lines
But in general I'd prefer to keep as much __initdata as possible because this occupies less memory when using a multi-machine kernel.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Ok, is the __initdata usage correct then? If not, I can change it. Just let me know.
I guess that if you compile mc13xxx support as module it won't work.
Try that, and if it breaks, send a patch fixing mx31_3ds.c, too.
I built it as module and the build went fine.

And you did a runtime test, too?

If you did and that went smooth, I'll take a look again, because I expected it to barf.

Best regards
Uwe

Signature · 2 lines

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

Fabio Estevam

reviewer

2011-02-19 03:31 UTC #7 lore raw

PATCH 16/17 mc13xxx: mfd_cell is now implicitly available to drivers

Hi Uwe,

2011/2/17 Uwe Kleine-K?nig <[email protected]>:
...

Quoted 5 lines

Quoted 1 line
I built it as module and the build went fine.
And you did a runtime test, too?
If you did and that went smooth, I'll take a look again, because I expected it to barf.

I haven?t performed a runtime test yet. I am travelling today and will be one week out of the office, but I can test it when I return.

Regards,

Fabio Estevam

Andres Salomon

author

2011-02-16 17:00 UTC #8 lore raw

In reply to Uwe Kleine-König

Quoted 8 lines · On Wed, 16 Feb 2011 10:41:54 +0100 Uwe Kleine-K?nig…

Quoted 5 lines · Andres Salomon
The cell's platform_data is now accessed with a helper function; change clients to use that, and remove the now-unused data_size.
Note that mfd-core no longer makes a copy of platform_data, but the
why was this changed and where? I'm not able to find your complete series via gmane.

http://permalink.gmane.org/gmane.linux.kernel/1099164
I screwed up the threading, so the patches aren't underneath that one. Patchwork provides them nicely, though.

Here are the relevant patches:
https://patchwork.kernel.org/patch/551241/
https://patchwork.kernel.org/patch/551251/
https://patchwork.kernel.org/patch/551261/
..through
https://patchwork.kernel.org/patch/551451/

The next series is here:
http://thread.gmane.org/gmane.linux.kernel/1100514/focus=6046

Uwe Kleine-König

2011-02-18 08:39 UTC #9 lore raw

Quoted 5 lines · Andres Salomon

Quoted 2 lines · On Wed, 16 Feb 2011 10:41:54 +0100 Uwe Kleine-K?nig…
Quoted 1 line · Andres Salomon

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
The cell's platform_data is now accessed with a helper function; change clients to use that, and remove the now-unused data_size.
Note that mfd-core no longer makes a copy of platform_data, but the
why was this changed and where? I'm not able to find your complete series via gmane.
http://permalink.gmane.org/gmane.linux.kernel/1099164

I still don't get the motivation of your patch set. For drivers that don't need the mfd_cells you introduce another level of indirection. And drivers that want to use mfd_cells as platform_data, they can already now.

Best regards
Uwe

Signature · 2 lines

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

Andres Salomon

author

2011-02-18 17:50 UTC #10 lore raw

Quoted 14 lines · On Fri, 18 Feb 2011 09:39:28 +0100 Uwe Kleine-K?nig…

Quoted 13 lines · Andres Salomon
Quoted 9 lines · On Wed, 16 Feb 2011 10:41:54 +0100 Uwe Kleine-K?nig…
Quoted 6 lines · Andres Salomon
The cell's platform_data is now accessed with a helper function; change clients to use that, and remove the now-unused data_size.
Note that mfd-core no longer makes a copy of platform_data, but the
why was this changed and where? I'm not able to find your complete series via gmane.

Quoted 9 lines

Quoted 1 line
http://permalink.gmane.org/gmane.linux.kernel/1099164
I still don't get the motivation of your patch set. For drivers that don't need the mfd_cells you introduce another level of indirection. And drivers that want to use mfd_cells as platform_data, they can already now.
Best regards
Uwe

The main motivation for making mfd_cell available to all was to be able to provide functions within mfd-core (introduced in later patches/patchsets) that accept platform devices and operate on their mfd_cells. The mfd_cell used to be automatically carried by the pdevs, and I'm not entirely sure the reason for getting rid of that. It also (imo) cleans up a lot of things. For example, the mfd_cell API platform_data/driver_data stuff was unclear when I first encountered it, as various drivers were doing things completely differently (some incorrectly, as witnessed by some of the drivers that I modified that were using driver_data and had clients later
calling set_drvdata on the same pdev). This is clearer, and provides
a uniform way to do things that, afaict, sacrifices absolutely no flexibility or functionality. Yes, it adds some overhead by having a copy of the mfd_cell around in memory, but if that's really an issue it could be optimized further by not making a full copy of the mfd_cell. With the new wrappers (mfd_get_data/mfd_get_cell), this could be done without having to mess with drivers.