[RFC] [PATCH 0/19] mfd sharing support

[RFC] [PATCH 0/19] mfd sharing support

[Andres Salomon]

This patchset sets up support for sharing of MFD cells across multiple
drivers.  This is something we'd like for OLPC power management, but
plenty of other drivers do similar things.  Every driver seems to have
its own way of accomplishing the task, and rather than go that route,
I'd rather have a common way to do it in mfd-core.

The parts of this patch series are somewhat separate, so that one
can pick and choose.  The first group (patches 1-16) makes mfd
cells always available (and adjusts any drivers that are using
platform_data to use driver_data instead) to mfd clients.  The second
group (patch 17) adds optional wrappers for the mfd_cell's
enable/disable hooks to handle reference counting.


The third group (patch 18) adds support for shared platform_devices,
and the last one (patch 19) adapts the cs5535-mfd/olpc-xo1 drivers to
make use of the sharing code.  These last two groups still need a bit
of work, but I was hoping to get feedback on the whole set.

[PATCH 01/19] mfd-core: unconditionally add mfd_cell to every platform_device

[Andres Salomon]

Previously, one would set the mfd_cell's platform_data/data_size to point
to the current mfd_cell in order to pass that information along to drivers.

This causes the current mfd_cell to always be available to drivers.  It
also adds a wrapper function for fetching the mfd cell from a platform
device.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/mfd/mfd-core.c   |    9 +++------
 include/linux/mfd/core.h |   12 ++++++++++--
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d83ad0f..d8b9e35 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -39,12 +39,9 @@ static int mfd_add_device(struct device *parent, int id,
 	pdev->dev.parent = parent;
 	platform_set_drvdata(pdev, cell->driver_data);
 
-	if (cell->data_size) {
-		ret = platform_device_add_data(pdev,
-					cell->platform_data, cell->data_size);
-		if (ret)
-			goto fail_res;
-	}
+	ret = platform_device_add_data(pdev, cell, sizeof(struct mfd_cell));
+	if (ret)
+		goto fail_res;
 
 	for (r = 0; r < cell->num_resources; r++) {
 		res[r].name = cell->resources[r].name;
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 835996e..82b2a5c 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -33,8 +33,7 @@ struct mfd_cell {
 	/* 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 */
+	/* These two are unused */
 	void			*platform_data;
 	size_t			data_size;
 
@@ -55,6 +54,15 @@ struct mfd_cell {
 	bool			pm_runtime_no_callbacks;
 };
 
+/*
+ * Given a platform device that's been created by mfd_add_devices(), fetch
+ * the mfd_cell that created it.
+ */
+static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
+{
+	return pdev->dev.platform_data;
+}
+
 extern int mfd_add_devices(struct device *parent, int id,
 			   const struct mfd_cell *cells, int n_devs,
 			   struct resource *mem_base,
-- 
1.7.2.3

[PATCH 02/19] jz4740: mfd_cell is now implicitly available to drivers

[Andres Salomon]

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

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/hwmon/jz4740-hwmon.c   |    2 +-
 drivers/mfd/jz4740-adc.c       |    4 ----
 drivers/power/jz4740-battery.c |    2 +-
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/jz4740-hwmon.c b/drivers/hwmon/jz4740-hwmon.c
index 1c8b3d9..40f106d 100644
--- a/drivers/hwmon/jz4740-hwmon.c
+++ b/drivers/hwmon/jz4740-hwmon.c
@@ -112,7 +112,7 @@ static int __devinit jz4740_hwmon_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	hwmon->cell = pdev->dev.platform_data;
+	hwmon->cell = mfd_get_cell(pdev);
 
 	hwmon->irq = platform_get_irq(pdev, 0);
 	if (hwmon->irq < 0) {
diff --git a/drivers/mfd/jz4740-adc.c b/drivers/mfd/jz4740-adc.c
index 0cc5979..aa518b9 100644
--- a/drivers/mfd/jz4740-adc.c
+++ b/drivers/mfd/jz4740-adc.c
@@ -232,8 +232,6 @@ const struct mfd_cell jz4740_adc_cells[] = {
 		.name = "jz4740-hwmon",
 		.num_resources = ARRAY_SIZE(jz4740_hwmon_resources),
 		.resources = jz4740_hwmon_resources,
-		.platform_data = (void *)&jz4740_adc_cells[0],
-		.data_size = sizeof(struct mfd_cell),
 
 		.enable = jz4740_adc_cell_enable,
 		.disable = jz4740_adc_cell_disable,
@@ -243,8 +241,6 @@ const struct mfd_cell jz4740_adc_cells[] = {
 		.name = "jz4740-battery",
 		.num_resources = ARRAY_SIZE(jz4740_battery_resources),
 		.resources = jz4740_battery_resources,
-		.platform_data = (void *)&jz4740_adc_cells[1],
-		.data_size = sizeof(struct mfd_cell),
 
 		.enable = jz4740_adc_cell_enable,
 		.disable = jz4740_adc_cell_disable,
diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
index 02414db..0938650 100644
--- a/drivers/power/jz4740-battery.c
+++ b/drivers/power/jz4740-battery.c
@@ -258,7 +258,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	jz_battery->cell = pdev->dev.platform_data;
+	jz_battery->cell = mfd_get_cell(pdev);
 
 	jz_battery->irq = platform_get_irq(pdev, 0);
 	if (jz_battery->irq < 0) {
-- 
1.7.2.3

Re: [PATCH 02/19] jz4740: mfd_cell is now implicitly available to drivers

[Jean Delvare]

Hi Andres,

On Wed, 2 Feb 2011 19:58:24 -0800, Andres Salomon wrote:
> 
> No need to explicitly set the cell's platform_data/data_size.
> 
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> ---
>  drivers/hwmon/jz4740-hwmon.c   |    2 +-
>  drivers/mfd/jz4740-adc.c       |    4 ----
>  drivers/power/jz4740-battery.c |    2 +-
>  3 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/jz4740-hwmon.c b/drivers/hwmon/jz4740-hwmon.c
> index 1c8b3d9..40f106d 100644
> --- a/drivers/hwmon/jz4740-hwmon.c
> +++ b/drivers/hwmon/jz4740-hwmon.c
> @@ -112,7 +112,7 @@ static int __devinit jz4740_hwmon_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	hwmon->cell = pdev->dev.platform_data;
> +	hwmon->cell = mfd_get_cell(pdev);
>  
>  	hwmon->irq = platform_get_irq(pdev, 0);
>  	if (hwmon->irq < 0) {
> diff --git a/drivers/mfd/jz4740-adc.c b/drivers/mfd/jz4740-adc.c
> index 0cc5979..aa518b9 100644
> --- a/drivers/mfd/jz4740-adc.c
> +++ b/drivers/mfd/jz4740-adc.c
> @@ -232,8 +232,6 @@ const struct mfd_cell jz4740_adc_cells[] = {
>  		.name = "jz4740-hwmon",
>  		.num_resources = ARRAY_SIZE(jz4740_hwmon_resources),
>  		.resources = jz4740_hwmon_resources,
> -		.platform_data = (void *)&jz4740_adc_cells[0],
> -		.data_size = sizeof(struct mfd_cell),
>  
>  		.enable = jz4740_adc_cell_enable,
>  		.disable = jz4740_adc_cell_disable,
> @@ -243,8 +241,6 @@ const struct mfd_cell jz4740_adc_cells[] = {
>  		.name = "jz4740-battery",
>  		.num_resources = ARRAY_SIZE(jz4740_battery_resources),
>  		.resources = jz4740_battery_resources,
> -		.platform_data = (void *)&jz4740_adc_cells[1],
> -		.data_size = sizeof(struct mfd_cell),
>  
>  		.enable = jz4740_adc_cell_enable,
>  		.disable = jz4740_adc_cell_disable,
> diff --git a/drivers/power/jz4740-battery.c b/drivers/power/jz4740-battery.c
> index 02414db..0938650 100644
> --- a/drivers/power/jz4740-battery.c
> +++ b/drivers/power/jz4740-battery.c
> @@ -258,7 +258,7 @@ static int __devinit jz_battery_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	jz_battery->cell = pdev->dev.platform_data;
> +	jz_battery->cell = mfd_get_cell(pdev);
>  
>  	jz_battery->irq = platform_get_irq(pdev, 0);
>  	if (jz_battery->irq < 0) {

I have no objection, but I can't test the changes either. I presume
these changes should be merged through the mfd tree?

Acked-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

[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

Re: [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 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

Re: [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 05/19] asic3: mfd_cell is now implicitly available to drivers

[Andres Salomon]

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

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/mfd/asic3.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/asic3.c b/drivers/mfd/asic3.c
index 6a1f940..71d2901 100644
--- a/drivers/mfd/asic3.c
+++ b/drivers/mfd/asic3.c
@@ -810,9 +810,6 @@ static int __init asic3_mfd_probe(struct platform_device *pdev,
 	ds1wm_resources[0].start >>= asic->bus_shift;
 	ds1wm_resources[0].end   >>= asic->bus_shift;
 
-	asic3_cell_ds1wm.platform_data = &asic3_cell_ds1wm;
-	asic3_cell_ds1wm.data_size = sizeof(asic3_cell_ds1wm);
-
 	/* MMC */
 	asic->tmio_cnf = ioremap((ASIC3_SD_CONFIG_BASE >> asic->bus_shift) +
 				 mem_sdio->start, 0x400 >> asic->bus_shift);
@@ -824,9 +821,6 @@ static int __init asic3_mfd_probe(struct platform_device *pdev,
 	asic3_mmc_resources[0].start >>= asic->bus_shift;
 	asic3_mmc_resources[0].end   >>= asic->bus_shift;
 
-	asic3_cell_mmc.platform_data = &asic3_cell_mmc;
-	asic3_cell_mmc.data_size = sizeof(asic3_cell_mmc);
-
 	ret = mfd_add_devices(&pdev->dev, pdev->id,
 			&asic3_cell_ds1wm, 1, mem, asic->irq_base);
 	if (ret < 0)
-- 
1.7.2.3

[PATCH 06/19] htc-pasic3: mfd_cell is now implicitly available to drivers

[Andres Salomon]

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

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/mfd/htc-pasic3.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/htc-pasic3.c b/drivers/mfd/htc-pasic3.c
index 7bc7522..079d396 100644
--- a/drivers/mfd/htc-pasic3.c
+++ b/drivers/mfd/htc-pasic3.c
@@ -165,8 +165,6 @@ static int __init pasic3_probe(struct platform_device *pdev)
 		ds1wm_pdata.clock_rate = pdata->clock_rate;
 		/* the first 5 PASIC3 registers control the DS1WM */
 		ds1wm_resources[0].end = (5 << asic->bus_shift) - 1;
-		ds1wm_cell.platform_data = &ds1wm_cell;
-		ds1wm_cell.data_size = sizeof(ds1wm_cell);
 		ret = mfd_add_devices(&pdev->dev, pdev->id,
 				&ds1wm_cell, 1, r, irq);
 		if (ret < 0)
@@ -175,8 +173,6 @@ static int __init pasic3_probe(struct platform_device *pdev)
 
 	if (pdata && pdata->led_pdata) {
 		led_cell.driver_data = pdata->led_pdata;
-		led_cell.platform_data = &led_cell;
-		led_cell.data_size = sizeof(ds1wm_cell);
 		ret = mfd_add_devices(&pdev->dev, pdev->id, &led_cell, 1, r, 0);
 		if (ret < 0)
 			dev_warn(dev, "failed to register LED device\n");
-- 
1.7.2.3

[PATCH 07/19] timberdale: 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.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/dma/timb_dma.c           |    2 +-
 drivers/gpio/timbgpio.c          |    5 +-
 drivers/i2c/busses/i2c-ocores.c  |    2 +-
 drivers/i2c/busses/i2c-xiic.c    |    2 +-
 drivers/media/radio/radio-timb.c |    2 +-
 drivers/media/video/timblogiw.c  |    2 +-
 drivers/mfd/timberdale.c         |   81 +++++++++++++-------------------------
 drivers/net/ks8842.c             |    2 +-
 drivers/spi/xilinx_spi.c         |    2 +-
 9 files changed, 36 insertions(+), 64 deletions(-)

diff --git a/drivers/dma/timb_dma.c b/drivers/dma/timb_dma.c
index 3b88a4e..aa06ca4 100644
--- a/drivers/dma/timb_dma.c
+++ b/drivers/dma/timb_dma.c
@@ -684,7 +684,7 @@ static irqreturn_t td_irq(int irq, void *devid)
 
 static int __devinit td_probe(struct platform_device *pdev)
 {
-	struct timb_dma_platform_data *pdata = pdev->dev.platform_data;
+	struct timb_dma_platform_data *pdata = platform_get_drvdata(pdev);
 	struct timb_dma *td;
 	struct resource *iomem;
 	int irq;
diff --git a/drivers/gpio/timbgpio.c b/drivers/gpio/timbgpio.c
index 58c8f30..e404487 100644
--- a/drivers/gpio/timbgpio.c
+++ b/drivers/gpio/timbgpio.c
@@ -228,7 +228,7 @@ static int __devinit timbgpio_probe(struct platform_device *pdev)
 	struct gpio_chip *gc;
 	struct timbgpio *tgpio;
 	struct resource *iomem;
-	struct timbgpio_platform_data *pdata = pdev->dev.platform_data;
+	struct timbgpio_platform_data *pdata = platform_get_drvdata(pdev);
 	int irq = platform_get_irq(pdev, 0);
 
 	if (!pdata || pdata->nr_pins > 32) {
@@ -319,14 +319,13 @@ err_mem:
 static int __devexit timbgpio_remove(struct platform_device *pdev)
 {
 	int err;
-	struct timbgpio_platform_data *pdata = pdev->dev.platform_data;
 	struct timbgpio *tgpio = platform_get_drvdata(pdev);
 	struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	int irq = platform_get_irq(pdev, 0);
 
 	if (irq >= 0 && tgpio->irq_base > 0) {
 		int i;
-		for (i = 0; i < pdata->nr_pins; i++) {
+		for (i = 0; i < tgpio->gpio.ngpio; i++) {
 			set_irq_chip(tgpio->irq_base + i, NULL);
 			set_irq_chip_data(tgpio->irq_base + i, NULL);
 		}
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index ef3bcb1..dc203ec 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -305,7 +305,7 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 		return -EIO;
 	}
 
-	pdata = pdev->dev.platform_data;
+	pdata = platform_get_drvdata(pdev);
 	if (pdata) {
 		i2c->regstep = pdata->regstep;
 		i2c->clock_khz = pdata->clock_khz;
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index a9c419e..830b8c1 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -704,7 +704,7 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
 	if (irq < 0)
 		goto resource_missing;
 
-	pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
+	pdata = platform_get_drvdata(pdev);
 	if (!pdata)
 		return -EINVAL;
 
diff --git a/drivers/media/radio/radio-timb.c b/drivers/media/radio/radio-timb.c
index a185610..e7baf26 100644
--- a/drivers/media/radio/radio-timb.c
+++ b/drivers/media/radio/radio-timb.c
@@ -148,7 +148,7 @@ static const struct v4l2_file_operations timbradio_fops = {
 
 static int __devinit timbradio_probe(struct platform_device *pdev)
 {
-	struct timb_radio_platform_data *pdata = pdev->dev.platform_data;
+	struct timb_radio_platform_data *pdata = platform_get_drvdata(pdev);
 	struct timbradio *tr;
 	int err;
 
diff --git a/drivers/media/video/timblogiw.c b/drivers/media/video/timblogiw.c
index fc611eb..61aa67a 100644
--- a/drivers/media/video/timblogiw.c
+++ b/drivers/media/video/timblogiw.c
@@ -790,7 +790,7 @@ static int __devinit timblogiw_probe(struct platform_device *pdev)
 {
 	int err;
 	struct timblogiw *lw = NULL;
-	struct timb_video_platform_data *pdata = pdev->dev.platform_data;
+	struct timb_video_platform_data *pdata = platform_get_drvdata(pdev);
 
 	if (!pdata) {
 		dev_err(&pdev->dev, "No platform data\n");
diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
index 6ad8a7f..e9ae162 100644
--- a/drivers/mfd/timberdale.c
+++ b/drivers/mfd/timberdale.c
@@ -384,8 +384,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg0[] = {
 		.name = "timb-dma",
 		.num_resources = ARRAY_SIZE(timberdale_dma_resources),
 		.resources = timberdale_dma_resources,
-		.platform_data = &timb_dma_platform_data,
-		.data_size = sizeof(timb_dma_platform_data),
+		.driver_data = &timb_dma_platform_data,
 	},
 	{
 		.name = "timb-uart",
@@ -396,43 +395,37 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg0[] = {
 		.name = "xiic-i2c",
 		.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
 		.resources = timberdale_xiic_resources,
-		.platform_data = &timberdale_xiic_platform_data,
-		.data_size = sizeof(timberdale_xiic_platform_data),
+		.driver_data = &timberdale_xiic_platform_data,
 	},
 	{
 		.name = "timb-gpio",
 		.num_resources = ARRAY_SIZE(timberdale_gpio_resources),
 		.resources = timberdale_gpio_resources,
-		.platform_data = &timberdale_gpio_platform_data,
-		.data_size = sizeof(timberdale_gpio_platform_data),
+		.driver_data = &timberdale_gpio_platform_data,
 	},
 	{
 		.name = "timb-video",
 		.num_resources = ARRAY_SIZE(timberdale_video_resources),
 		.resources = timberdale_video_resources,
-		.platform_data = &timberdale_video_platform_data,
-		.data_size = sizeof(timberdale_video_platform_data),
+		.driver_data = &timberdale_video_platform_data,
 	},
 	{
 		.name = "timb-radio",
 		.num_resources = ARRAY_SIZE(timberdale_radio_resources),
 		.resources = timberdale_radio_resources,
-		.platform_data = &timberdale_radio_platform_data,
-		.data_size = sizeof(timberdale_radio_platform_data),
+		.driver_data = &timberdale_radio_platform_data,
 	},
 	{
 		.name = "xilinx_spi",
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
-		.platform_data = &timberdale_xspi_platform_data,
-		.data_size = sizeof(timberdale_xspi_platform_data),
+		.driver_data = &timberdale_xspi_platform_data,
 	},
 	{
 		.name = "ks8842",
 		.num_resources = ARRAY_SIZE(timberdale_eth_resources),
 		.resources = timberdale_eth_resources,
-		.platform_data = &timberdale_ks8842_platform_data,
-		.data_size = sizeof(timberdale_ks8842_platform_data)
+		.driver_data = &timberdale_ks8842_platform_data,
 	},
 };
 
@@ -441,8 +434,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
 		.name = "timb-dma",
 		.num_resources = ARRAY_SIZE(timberdale_dma_resources),
 		.resources = timberdale_dma_resources,
-		.platform_data = &timb_dma_platform_data,
-		.data_size = sizeof(timb_dma_platform_data),
+		.driver_data = &timb_dma_platform_data,
 	},
 	{
 		.name = "timb-uart",
@@ -458,15 +450,13 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
 		.name = "xiic-i2c",
 		.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
 		.resources = timberdale_xiic_resources,
-		.platform_data = &timberdale_xiic_platform_data,
-		.data_size = sizeof(timberdale_xiic_platform_data),
+		.driver_data = &timberdale_xiic_platform_data,
 	},
 	{
 		.name = "timb-gpio",
 		.num_resources = ARRAY_SIZE(timberdale_gpio_resources),
 		.resources = timberdale_gpio_resources,
-		.platform_data = &timberdale_gpio_platform_data,
-		.data_size = sizeof(timberdale_gpio_platform_data),
+		.driver_data = &timberdale_gpio_platform_data,
 	},
 	{
 		.name = "timb-mlogicore",
@@ -477,29 +467,25 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
 		.name = "timb-video",
 		.num_resources = ARRAY_SIZE(timberdale_video_resources),
 		.resources = timberdale_video_resources,
-		.platform_data = &timberdale_video_platform_data,
-		.data_size = sizeof(timberdale_video_platform_data),
+		.driver_data = &timberdale_video_platform_data,
 	},
 	{
 		.name = "timb-radio",
 		.num_resources = ARRAY_SIZE(timberdale_radio_resources),
 		.resources = timberdale_radio_resources,
-		.platform_data = &timberdale_radio_platform_data,
-		.data_size = sizeof(timberdale_radio_platform_data),
+		.driver_data = &timberdale_radio_platform_data,
 	},
 	{
 		.name = "xilinx_spi",
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
-		.platform_data = &timberdale_xspi_platform_data,
-		.data_size = sizeof(timberdale_xspi_platform_data),
+		.driver_data = &timberdale_xspi_platform_data,
 	},
 	{
 		.name = "ks8842",
 		.num_resources = ARRAY_SIZE(timberdale_eth_resources),
 		.resources = timberdale_eth_resources,
-		.platform_data = &timberdale_ks8842_platform_data,
-		.data_size = sizeof(timberdale_ks8842_platform_data)
+		.driver_data = &timberdale_ks8842_platform_data,
 	},
 };
 
@@ -508,8 +494,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg2[] = {
 		.name = "timb-dma",
 		.num_resources = ARRAY_SIZE(timberdale_dma_resources),
 		.resources = timberdale_dma_resources,
-		.platform_data = &timb_dma_platform_data,
-		.data_size = sizeof(timb_dma_platform_data),
+		.driver_data = &timb_dma_platform_data,
 	},
 	{
 		.name = "timb-uart",
@@ -520,36 +505,31 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg2[] = {
 		.name = "xiic-i2c",
 		.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
 		.resources = timberdale_xiic_resources,
-		.platform_data = &timberdale_xiic_platform_data,
-		.data_size = sizeof(timberdale_xiic_platform_data),
+		.driver_data = &timberdale_xiic_platform_data,
 	},
 	{
 		.name = "timb-gpio",
 		.num_resources = ARRAY_SIZE(timberdale_gpio_resources),
 		.resources = timberdale_gpio_resources,
-		.platform_data = &timberdale_gpio_platform_data,
-		.data_size = sizeof(timberdale_gpio_platform_data),
+		.driver_data = &timberdale_gpio_platform_data,
 	},
 	{
 		.name = "timb-video",
 		.num_resources = ARRAY_SIZE(timberdale_video_resources),
 		.resources = timberdale_video_resources,
-		.platform_data = &timberdale_video_platform_data,
-		.data_size = sizeof(timberdale_video_platform_data),
+		.driver_data = &timberdale_video_platform_data,
 	},
 	{
 		.name = "timb-radio",
 		.num_resources = ARRAY_SIZE(timberdale_radio_resources),
 		.resources = timberdale_radio_resources,
-		.platform_data = &timberdale_radio_platform_data,
-		.data_size = sizeof(timberdale_radio_platform_data),
+		.driver_data = &timberdale_radio_platform_data,
 	},
 	{
 		.name = "xilinx_spi",
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
-		.platform_data = &timberdale_xspi_platform_data,
-		.data_size = sizeof(timberdale_xspi_platform_data),
+		.driver_data = &timberdale_xspi_platform_data,
 	},
 };
 
@@ -558,8 +538,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg3[] = {
 		.name = "timb-dma",
 		.num_resources = ARRAY_SIZE(timberdale_dma_resources),
 		.resources = timberdale_dma_resources,
-		.platform_data = &timb_dma_platform_data,
-		.data_size = sizeof(timb_dma_platform_data),
+		.driver_data = &timb_dma_platform_data,
 	},
 	{
 		.name = "timb-uart",
@@ -570,43 +549,37 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg3[] = {
 		.name = "ocores-i2c",
 		.num_resources = ARRAY_SIZE(timberdale_ocores_resources),
 		.resources = timberdale_ocores_resources,
-		.platform_data = &timberdale_ocores_platform_data,
-		.data_size = sizeof(timberdale_ocores_platform_data),
+		.driver_data = &timberdale_ocores_platform_data,
 	},
 	{
 		.name = "timb-gpio",
 		.num_resources = ARRAY_SIZE(timberdale_gpio_resources),
 		.resources = timberdale_gpio_resources,
-		.platform_data = &timberdale_gpio_platform_data,
-		.data_size = sizeof(timberdale_gpio_platform_data),
+		.driver_data = &timberdale_gpio_platform_data,
 	},
 	{
 		.name = "timb-video",
 		.num_resources = ARRAY_SIZE(timberdale_video_resources),
 		.resources = timberdale_video_resources,
-		.platform_data = &timberdale_video_platform_data,
-		.data_size = sizeof(timberdale_video_platform_data),
+		.driver_data = &timberdale_video_platform_data,
 	},
 	{
 		.name = "timb-radio",
 		.num_resources = ARRAY_SIZE(timberdale_radio_resources),
 		.resources = timberdale_radio_resources,
-		.platform_data = &timberdale_radio_platform_data,
-		.data_size = sizeof(timberdale_radio_platform_data),
+		.driver_data = &timberdale_radio_platform_data,
 	},
 	{
 		.name = "xilinx_spi",
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
-		.platform_data = &timberdale_xspi_platform_data,
-		.data_size = sizeof(timberdale_xspi_platform_data),
+		.driver_data = &timberdale_xspi_platform_data,
 	},
 	{
 		.name = "ks8842",
 		.num_resources = ARRAY_SIZE(timberdale_eth_resources),
 		.resources = timberdale_eth_resources,
-		.platform_data = &timberdale_ks8842_platform_data,
-		.data_size = sizeof(timberdale_ks8842_platform_data)
+		.driver_data = &timberdale_ks8842_platform_data,
 	},
 };
 
diff --git a/drivers/net/ks8842.c b/drivers/net/ks8842.c
index 928b2b8..7f0f51f 100644
--- a/drivers/net/ks8842.c
+++ b/drivers/net/ks8842.c
@@ -1145,7 +1145,7 @@ static int __devinit ks8842_probe(struct platform_device *pdev)
 	struct resource *iomem;
 	struct net_device *netdev;
 	struct ks8842_adapter *adapter;
-	struct ks8842_platform_data *pdata = pdev->dev.platform_data;
+	struct ks8842_platform_data *pdata = platform_get_drvdata(pdev);
 	u16 id;
 	unsigned i;
 
diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
index 7adaef6..2926dec 100644
--- a/drivers/spi/xilinx_spi.c
+++ b/drivers/spi/xilinx_spi.c
@@ -474,7 +474,7 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)
 	struct spi_master *master;
 	u8 i;
 
-	pdata = dev->dev.platform_data;
+	pdata = platform_get_drvdata(dev);
 	if (pdata) {
 		num_cs = pdata->num_chipselect;
 		little_endian = pdata->little_endian;
-- 
1.7.2.3

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Grant Likely]

On Wed, Feb 02, 2011 at 08:08:12PM -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.
> 
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> ---
>  drivers/dma/timb_dma.c           |    2 +-
>  drivers/gpio/timbgpio.c          |    5 +-
>  drivers/i2c/busses/i2c-ocores.c  |    2 +-
>  drivers/i2c/busses/i2c-xiic.c    |    2 +-
>  drivers/media/radio/radio-timb.c |    2 +-
>  drivers/media/video/timblogiw.c  |    2 +-
>  drivers/mfd/timberdale.c         |   81 +++++++++++++-------------------------
>  drivers/net/ks8842.c             |    2 +-
>  drivers/spi/xilinx_spi.c         |    2 +-
>  9 files changed, 36 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/dma/timb_dma.c b/drivers/dma/timb_dma.c
> index 3b88a4e..aa06ca4 100644
> --- a/drivers/dma/timb_dma.c
> +++ b/drivers/dma/timb_dma.c
> @@ -684,7 +684,7 @@ static irqreturn_t td_irq(int irq, void *devid)
>  
>  static int __devinit td_probe(struct platform_device *pdev)
>  {
> -	struct timb_dma_platform_data *pdata = pdev->dev.platform_data;
> +	struct timb_dma_platform_data *pdata = platform_get_drvdata(pdev);

Hold the phone.  I know this has already been merged, but this isn't correct.

drvdata is under the ownership of the driver, which means it should
*not* be set when .probe() gets called.  It is for driver private data
to do with as it needs, usually for internal state.

Gah.  Not all devices instantiated via mfd will be an mfd device,
which means that the driver may very well expect an *entirely
different* platform_device pointer; which further means a very high
potential of incorrectly dereferenced structures (as evidenced by a
patch series that is not bisectable).  For instance, the xilinx ip
cores are used by more than just mfd.

This is really bad.  Since this is new in .39-rc1, I really think this
series needs to be reverted.  Samuel, can you look at this please?

g.


>  	struct timb_dma *td;
>  	struct resource *iomem;
>  	int irq;
> diff --git a/drivers/gpio/timbgpio.c b/drivers/gpio/timbgpio.c
> index 58c8f30..e404487 100644
> --- a/drivers/gpio/timbgpio.c
> +++ b/drivers/gpio/timbgpio.c
> @@ -228,7 +228,7 @@ static int __devinit timbgpio_probe(struct platform_device *pdev)
>  	struct gpio_chip *gc;
>  	struct timbgpio *tgpio;
>  	struct resource *iomem;
> -	struct timbgpio_platform_data *pdata = pdev->dev.platform_data;
> +	struct timbgpio_platform_data *pdata = platform_get_drvdata(pdev);
>  	int irq = platform_get_irq(pdev, 0);
>  
>  	if (!pdata || pdata->nr_pins > 32) {
> @@ -319,14 +319,13 @@ err_mem:
>  static int __devexit timbgpio_remove(struct platform_device *pdev)
>  {
>  	int err;
> -	struct timbgpio_platform_data *pdata = pdev->dev.platform_data;
>  	struct timbgpio *tgpio = platform_get_drvdata(pdev);
>  	struct resource *iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	int irq = platform_get_irq(pdev, 0);
>  
>  	if (irq >= 0 && tgpio->irq_base > 0) {
>  		int i;
> -		for (i = 0; i < pdata->nr_pins; i++) {
> +		for (i = 0; i < tgpio->gpio.ngpio; i++) {
>  			set_irq_chip(tgpio->irq_base + i, NULL);
>  			set_irq_chip_data(tgpio->irq_base + i, NULL);
>  		}
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index ef3bcb1..dc203ec 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -305,7 +305,7 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
>  		return -EIO;
>  	}
>  
> -	pdata = pdev->dev.platform_data;
> +	pdata = platform_get_drvdata(pdev);
>  	if (pdata) {
>  		i2c->regstep = pdata->regstep;
>  		i2c->clock_khz = pdata->clock_khz;
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index a9c419e..830b8c1 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -704,7 +704,7 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		goto resource_missing;
>  
> -	pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
> +	pdata = platform_get_drvdata(pdev);
>  	if (!pdata)
>  		return -EINVAL;
>  
> diff --git a/drivers/media/radio/radio-timb.c b/drivers/media/radio/radio-timb.c
> index a185610..e7baf26 100644
> --- a/drivers/media/radio/radio-timb.c
> +++ b/drivers/media/radio/radio-timb.c
> @@ -148,7 +148,7 @@ static const struct v4l2_file_operations timbradio_fops = {
>  
>  static int __devinit timbradio_probe(struct platform_device *pdev)
>  {
> -	struct timb_radio_platform_data *pdata = pdev->dev.platform_data;
> +	struct timb_radio_platform_data *pdata = platform_get_drvdata(pdev);
>  	struct timbradio *tr;
>  	int err;
>  
> diff --git a/drivers/media/video/timblogiw.c b/drivers/media/video/timblogiw.c
> index fc611eb..61aa67a 100644
> --- a/drivers/media/video/timblogiw.c
> +++ b/drivers/media/video/timblogiw.c
> @@ -790,7 +790,7 @@ static int __devinit timblogiw_probe(struct platform_device *pdev)
>  {
>  	int err;
>  	struct timblogiw *lw = NULL;
> -	struct timb_video_platform_data *pdata = pdev->dev.platform_data;
> +	struct timb_video_platform_data *pdata = platform_get_drvdata(pdev);
>  
>  	if (!pdata) {
>  		dev_err(&pdev->dev, "No platform data\n");
> diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
> index 6ad8a7f..e9ae162 100644
> --- a/drivers/mfd/timberdale.c
> +++ b/drivers/mfd/timberdale.c
> @@ -384,8 +384,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg0[] = {
>  		.name = "timb-dma",
>  		.num_resources = ARRAY_SIZE(timberdale_dma_resources),
>  		.resources = timberdale_dma_resources,
> -		.platform_data = &timb_dma_platform_data,
> -		.data_size = sizeof(timb_dma_platform_data),
> +		.driver_data = &timb_dma_platform_data,
>  	},
>  	{
>  		.name = "timb-uart",
> @@ -396,43 +395,37 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg0[] = {
>  		.name = "xiic-i2c",
>  		.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
>  		.resources = timberdale_xiic_resources,
> -		.platform_data = &timberdale_xiic_platform_data,
> -		.data_size = sizeof(timberdale_xiic_platform_data),
> +		.driver_data = &timberdale_xiic_platform_data,
>  	},
>  	{
>  		.name = "timb-gpio",
>  		.num_resources = ARRAY_SIZE(timberdale_gpio_resources),
>  		.resources = timberdale_gpio_resources,
> -		.platform_data = &timberdale_gpio_platform_data,
> -		.data_size = sizeof(timberdale_gpio_platform_data),
> +		.driver_data = &timberdale_gpio_platform_data,
>  	},
>  	{
>  		.name = "timb-video",
>  		.num_resources = ARRAY_SIZE(timberdale_video_resources),
>  		.resources = timberdale_video_resources,
> -		.platform_data = &timberdale_video_platform_data,
> -		.data_size = sizeof(timberdale_video_platform_data),
> +		.driver_data = &timberdale_video_platform_data,
>  	},
>  	{
>  		.name = "timb-radio",
>  		.num_resources = ARRAY_SIZE(timberdale_radio_resources),
>  		.resources = timberdale_radio_resources,
> -		.platform_data = &timberdale_radio_platform_data,
> -		.data_size = sizeof(timberdale_radio_platform_data),
> +		.driver_data = &timberdale_radio_platform_data,
>  	},
>  	{
>  		.name = "xilinx_spi",
>  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
>  		.resources = timberdale_spi_resources,
> -		.platform_data = &timberdale_xspi_platform_data,
> -		.data_size = sizeof(timberdale_xspi_platform_data),
> +		.driver_data = &timberdale_xspi_platform_data,
>  	},
>  	{
>  		.name = "ks8842",
>  		.num_resources = ARRAY_SIZE(timberdale_eth_resources),
>  		.resources = timberdale_eth_resources,
> -		.platform_data = &timberdale_ks8842_platform_data,
> -		.data_size = sizeof(timberdale_ks8842_platform_data)
> +		.driver_data = &timberdale_ks8842_platform_data,
>  	},
>  };
>  
> @@ -441,8 +434,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
>  		.name = "timb-dma",
>  		.num_resources = ARRAY_SIZE(timberdale_dma_resources),
>  		.resources = timberdale_dma_resources,
> -		.platform_data = &timb_dma_platform_data,
> -		.data_size = sizeof(timb_dma_platform_data),
> +		.driver_data = &timb_dma_platform_data,
>  	},
>  	{
>  		.name = "timb-uart",
> @@ -458,15 +450,13 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
>  		.name = "xiic-i2c",
>  		.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
>  		.resources = timberdale_xiic_resources,
> -		.platform_data = &timberdale_xiic_platform_data,
> -		.data_size = sizeof(timberdale_xiic_platform_data),
> +		.driver_data = &timberdale_xiic_platform_data,
>  	},
>  	{
>  		.name = "timb-gpio",
>  		.num_resources = ARRAY_SIZE(timberdale_gpio_resources),
>  		.resources = timberdale_gpio_resources,
> -		.platform_data = &timberdale_gpio_platform_data,
> -		.data_size = sizeof(timberdale_gpio_platform_data),
> +		.driver_data = &timberdale_gpio_platform_data,
>  	},
>  	{
>  		.name = "timb-mlogicore",
> @@ -477,29 +467,25 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
>  		.name = "timb-video",
>  		.num_resources = ARRAY_SIZE(timberdale_video_resources),
>  		.resources = timberdale_video_resources,
> -		.platform_data = &timberdale_video_platform_data,
> -		.data_size = sizeof(timberdale_video_platform_data),
> +		.driver_data = &timberdale_video_platform_data,
>  	},
>  	{
>  		.name = "timb-radio",
>  		.num_resources = ARRAY_SIZE(timberdale_radio_resources),
>  		.resources = timberdale_radio_resources,
> -		.platform_data = &timberdale_radio_platform_data,
> -		.data_size = sizeof(timberdale_radio_platform_data),
> +		.driver_data = &timberdale_radio_platform_data,
>  	},
>  	{
>  		.name = "xilinx_spi",
>  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
>  		.resources = timberdale_spi_resources,
> -		.platform_data = &timberdale_xspi_platform_data,
> -		.data_size = sizeof(timberdale_xspi_platform_data),
> +		.driver_data = &timberdale_xspi_platform_data,
>  	},
>  	{
>  		.name = "ks8842",
>  		.num_resources = ARRAY_SIZE(timberdale_eth_resources),
>  		.resources = timberdale_eth_resources,
> -		.platform_data = &timberdale_ks8842_platform_data,
> -		.data_size = sizeof(timberdale_ks8842_platform_data)
> +		.driver_data = &timberdale_ks8842_platform_data,
>  	},
>  };
>  
> @@ -508,8 +494,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg2[] = {
>  		.name = "timb-dma",
>  		.num_resources = ARRAY_SIZE(timberdale_dma_resources),
>  		.resources = timberdale_dma_resources,
> -		.platform_data = &timb_dma_platform_data,
> -		.data_size = sizeof(timb_dma_platform_data),
> +		.driver_data = &timb_dma_platform_data,
>  	},
>  	{
>  		.name = "timb-uart",
> @@ -520,36 +505,31 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg2[] = {
>  		.name = "xiic-i2c",
>  		.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
>  		.resources = timberdale_xiic_resources,
> -		.platform_data = &timberdale_xiic_platform_data,
> -		.data_size = sizeof(timberdale_xiic_platform_data),
> +		.driver_data = &timberdale_xiic_platform_data,
>  	},
>  	{
>  		.name = "timb-gpio",
>  		.num_resources = ARRAY_SIZE(timberdale_gpio_resources),
>  		.resources = timberdale_gpio_resources,
> -		.platform_data = &timberdale_gpio_platform_data,
> -		.data_size = sizeof(timberdale_gpio_platform_data),
> +		.driver_data = &timberdale_gpio_platform_data,
>  	},
>  	{
>  		.name = "timb-video",
>  		.num_resources = ARRAY_SIZE(timberdale_video_resources),
>  		.resources = timberdale_video_resources,
> -		.platform_data = &timberdale_video_platform_data,
> -		.data_size = sizeof(timberdale_video_platform_data),
> +		.driver_data = &timberdale_video_platform_data,
>  	},
>  	{
>  		.name = "timb-radio",
>  		.num_resources = ARRAY_SIZE(timberdale_radio_resources),
>  		.resources = timberdale_radio_resources,
> -		.platform_data = &timberdale_radio_platform_data,
> -		.data_size = sizeof(timberdale_radio_platform_data),
> +		.driver_data = &timberdale_radio_platform_data,
>  	},
>  	{
>  		.name = "xilinx_spi",
>  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
>  		.resources = timberdale_spi_resources,
> -		.platform_data = &timberdale_xspi_platform_data,
> -		.data_size = sizeof(timberdale_xspi_platform_data),
> +		.driver_data = &timberdale_xspi_platform_data,
>  	},
>  };
>  
> @@ -558,8 +538,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg3[] = {
>  		.name = "timb-dma",
>  		.num_resources = ARRAY_SIZE(timberdale_dma_resources),
>  		.resources = timberdale_dma_resources,
> -		.platform_data = &timb_dma_platform_data,
> -		.data_size = sizeof(timb_dma_platform_data),
> +		.driver_data = &timb_dma_platform_data,
>  	},
>  	{
>  		.name = "timb-uart",
> @@ -570,43 +549,37 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg3[] = {
>  		.name = "ocores-i2c",
>  		.num_resources = ARRAY_SIZE(timberdale_ocores_resources),
>  		.resources = timberdale_ocores_resources,
> -		.platform_data = &timberdale_ocores_platform_data,
> -		.data_size = sizeof(timberdale_ocores_platform_data),
> +		.driver_data = &timberdale_ocores_platform_data,
>  	},
>  	{
>  		.name = "timb-gpio",
>  		.num_resources = ARRAY_SIZE(timberdale_gpio_resources),
>  		.resources = timberdale_gpio_resources,
> -		.platform_data = &timberdale_gpio_platform_data,
> -		.data_size = sizeof(timberdale_gpio_platform_data),
> +		.driver_data = &timberdale_gpio_platform_data,
>  	},
>  	{
>  		.name = "timb-video",
>  		.num_resources = ARRAY_SIZE(timberdale_video_resources),
>  		.resources = timberdale_video_resources,
> -		.platform_data = &timberdale_video_platform_data,
> -		.data_size = sizeof(timberdale_video_platform_data),
> +		.driver_data = &timberdale_video_platform_data,
>  	},
>  	{
>  		.name = "timb-radio",
>  		.num_resources = ARRAY_SIZE(timberdale_radio_resources),
>  		.resources = timberdale_radio_resources,
> -		.platform_data = &timberdale_radio_platform_data,
> -		.data_size = sizeof(timberdale_radio_platform_data),
> +		.driver_data = &timberdale_radio_platform_data,
>  	},
>  	{
>  		.name = "xilinx_spi",
>  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
>  		.resources = timberdale_spi_resources,
> -		.platform_data = &timberdale_xspi_platform_data,
> -		.data_size = sizeof(timberdale_xspi_platform_data),
> +		.driver_data = &timberdale_xspi_platform_data,
>  	},
>  	{
>  		.name = "ks8842",
>  		.num_resources = ARRAY_SIZE(timberdale_eth_resources),
>  		.resources = timberdale_eth_resources,
> -		.platform_data = &timberdale_ks8842_platform_data,
> -		.data_size = sizeof(timberdale_ks8842_platform_data)
> +		.driver_data = &timberdale_ks8842_platform_data,
>  	},
>  };
>  
> diff --git a/drivers/net/ks8842.c b/drivers/net/ks8842.c
> index 928b2b8..7f0f51f 100644
> --- a/drivers/net/ks8842.c
> +++ b/drivers/net/ks8842.c
> @@ -1145,7 +1145,7 @@ static int __devinit ks8842_probe(struct platform_device *pdev)
>  	struct resource *iomem;
>  	struct net_device *netdev;
>  	struct ks8842_adapter *adapter;
> -	struct ks8842_platform_data *pdata = pdev->dev.platform_data;
> +	struct ks8842_platform_data *pdata = platform_get_drvdata(pdev);
>  	u16 id;
>  	unsigned i;
>  
> diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
> index 7adaef6..2926dec 100644
> --- a/drivers/spi/xilinx_spi.c
> +++ b/drivers/spi/xilinx_spi.c
> @@ -474,7 +474,7 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)
>  	struct spi_master *master;
>  	u8 i;
>  
> -	pdata = dev->dev.platform_data;
> +	pdata = platform_get_drvdata(dev);
>  	if (pdata) {
>  		num_cs = pdata->num_chipselect;
>  		little_endian = pdata->little_endian;
> -- 
> 1.7.2.3
> 

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> On Wed, Feb 02, 2011 at 08:08:12PM -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.
> > 
> > Signed-off-by: Andres Salomon <dilinger@queued.net>
> > ---
> >  drivers/dma/timb_dma.c           |    2 +-
> >  drivers/gpio/timbgpio.c          |    5 +-
> >  drivers/i2c/busses/i2c-ocores.c  |    2 +-
> >  drivers/i2c/busses/i2c-xiic.c    |    2 +-
> >  drivers/media/radio/radio-timb.c |    2 +-
> >  drivers/media/video/timblogiw.c  |    2 +-
> >  drivers/mfd/timberdale.c         |   81 +++++++++++++-------------------------
> >  drivers/net/ks8842.c             |    2 +-
> >  drivers/spi/xilinx_spi.c         |    2 +-
> >  9 files changed, 36 insertions(+), 64 deletions(-)
> > 
> > diff --git a/drivers/dma/timb_dma.c b/drivers/dma/timb_dma.c
> > index 3b88a4e..aa06ca4 100644
> > --- a/drivers/dma/timb_dma.c
> > +++ b/drivers/dma/timb_dma.c
> > @@ -684,7 +684,7 @@ static irqreturn_t td_irq(int irq, void *devid)
> >  
> >  static int __devinit td_probe(struct platform_device *pdev)
> >  {
> > -	struct timb_dma_platform_data *pdata = pdev->dev.platform_data;
> > +	struct timb_dma_platform_data *pdata = platform_get_drvdata(pdev);
> 
> Hold the phone.  I know this has already been merged, but this isn't correct.
> 
> drvdata is under the ownership of the driver, which means it should
> *not* be set when .probe() gets called.  It is for driver private data
> to do with as it needs, usually for internal state.
We didn't merge that version of the patchset, but one getting the
platform_data pointer from mfd_get_data(). That introduces the issue you're
talking about below.


> Gah.  Not all devices instantiated via mfd will be an mfd device,
> which means that the driver may very well expect an *entirely
> different* platform_device pointer; which further means a very high
> potential of incorrectly dereferenced structures (as evidenced by a
> patch series that is not bisectable).  For instance, the xilinx ip
> cores are used by more than just mfd.
I agree. Since the vast majority of the MFD subdevices are MFD specific IPs, I
overlooked that part. The impacted drivers are the timberdale and the DaVinci
voice codec ones.
To fix that problem I propose 2 alternatives:

1) When declaring the sub devices cells, the MFD driver should specify an
mfd_data_size value for sub devices that are not MFD specific. It's the MFD
driver responsibility to set the cell properly, and the non MFD specific
drivers are kept MFD agnostic.
See my patch below for the timberdale case.

2) Revert the mfd_get_data() call for getting sub devices platform data
pointers. That was introduced to ease the MFD cell sharing work, so if we take
this route we'll need the cs5535 MFD driver to pass its cells as platform_data
pointer. Andres, can you confirm that this would be fine for the
mfd_clone_cell() routine to keep working ?

Patch for solution 1:


 drivers/mfd/mfd-core.c          |   13 ++++++++++---
 drivers/mfd/timberdale.c        |   11 +++++++++++
 include/linux/mfd/core.h        |    1 +
 drivers/i2c/busses/i2c-ocores.c |    3 +--
 drivers/i2c/busses/i2c-xiic.c   |    3 +--
 drivers/net/ks8842.c            |    3 +--
 drivers/spi/xilinx_spi.c        |    3 +--
 7 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d01574d..8abe510 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -75,9 +75,16 @@ static int mfd_add_device(struct device *parent, int id,
 
 	pdev->dev.parent = parent;
 
-	ret = platform_device_add_data(pdev, cell, sizeof(*cell));
-	if (ret)
-		goto fail_res;
+	if (cell->mfd_data_size > 0) {
+		ret = platform_device_add_data(pdev,
+					cell->mfd_data, cell->mfd_data_size);
+		if (ret)
+			goto fail_res;
+	} else {
+		ret = platform_device_add_data(pdev, cell, sizeof(*cell));
+		if (ret)
+			goto fail_res;
+	}
 
 	for (r = 0; r < cell->num_resources; r++) {
 		res[r].name = cell->resources[r].name;
diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
index 94c6c8a..b4d2d09 100644
--- a/drivers/mfd/timberdale.c
+++ b/drivers/mfd/timberdale.c
@@ -396,6 +396,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg0[] = {
 		.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
 		.resources = timberdale_xiic_resources,
 		.mfd_data = &timberdale_xiic_platform_data,
+		.mfd_data_size = sizeof(timberdale_xiic_platform_data)
 	},
 	{
 		.name = "timb-gpio",
@@ -420,12 +421,14 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg0[] = {
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
 		.mfd_data = &timberdale_xspi_platform_data,
+		.mfd_data_size = sizeof(timberdale_xspi_platform_data)
 	},
 	{
 		.name = "ks8842",
 		.num_resources = ARRAY_SIZE(timberdale_eth_resources),
 		.resources = timberdale_eth_resources,
 		.mfd_data = &timberdale_ks8842_platform_data,
+		.mfd_data_size = sizeof(timberdale_ks8842_platform_data)
 	},
 };
 
@@ -451,6 +454,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
 		.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
 		.resources = timberdale_xiic_resources,
 		.mfd_data = &timberdale_xiic_platform_data,
+		.mfd_data_size = sizeof(timberdale_xiic_platform_data)
 	},
 	{
 		.name = "timb-gpio",
@@ -480,12 +484,14 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
 		.mfd_data = &timberdale_xspi_platform_data,
+		.mfd_data_size = sizeof(timberdale_xspi_platform_data)
 	},
 	{
 		.name = "ks8842",
 		.num_resources = ARRAY_SIZE(timberdale_eth_resources),
 		.resources = timberdale_eth_resources,
 		.mfd_data = &timberdale_ks8842_platform_data,
+		.mfd_data_size = sizeof(timberdale_ks8842_platform_data)
 	},
 };
 
@@ -506,6 +512,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg2[] = {
 		.num_resources = ARRAY_SIZE(timberdale_xiic_resources),
 		.resources = timberdale_xiic_resources,
 		.mfd_data = &timberdale_xiic_platform_data,
+		.mfd_data_size = sizeof(timberdale_xiic_platform_data)
 	},
 	{
 		.name = "timb-gpio",
@@ -530,6 +537,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg2[] = {
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
 		.mfd_data = &timberdale_xspi_platform_data,
+		.mfd_data_size = sizeof(timberdale_xspi_platform_data)
 	},
 };
 
@@ -550,6 +558,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg3[] = {
 		.num_resources = ARRAY_SIZE(timberdale_ocores_resources),
 		.resources = timberdale_ocores_resources,
 		.mfd_data = &timberdale_ocores_platform_data,
+		.mfd_data_size = sizeof(timberdale_ocores_platform_data)
 	},
 	{
 		.name = "timb-gpio",
@@ -574,12 +583,14 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg3[] = {
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
 		.mfd_data = &timberdale_xspi_platform_data,
+		.mfd_data_size = sizeof(timberdale_xspi_platform_data)
 	},
 	{
 		.name = "ks8842",
 		.num_resources = ARRAY_SIZE(timberdale_eth_resources),
 		.resources = timberdale_eth_resources,
 		.mfd_data = &timberdale_ks8842_platform_data,
+		.mfd_data_size = sizeof(timberdale_ks8842_platform_data)
 	},
 };
 
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index ad1b19a..3687e10 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -35,6 +35,7 @@ struct mfd_cell {
 
 	/* mfd_data can be used to pass data to client drivers */
 	void			*mfd_data;
+	size_t			mfd_data_size;
 
 	/*
 	 * These resources can be specified relative to the parent device.
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index fee1a26..1b46a9d 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -49,7 +49,6 @@
 #include <linux/init.h>
 #include <linux/errno.h>
 #include <linux/platform_device.h>
-#include <linux/mfd/core.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/wait.h>
@@ -306,7 +305,7 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 		return -EIO;
 	}
 
-	pdata = mfd_get_data(pdev);
+	pdata = pdev->dev.platform_data;
 	if (pdata) {
 		i2c->regstep = pdata->regstep;
 		i2c->clock_khz = pdata->clock_khz;
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 9fbd7e6..a9c419e 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -34,7 +34,6 @@
 #include <linux/errno.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
-#include <linux/mfd/core.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/wait.h>
@@ -705,7 +704,7 @@ static int __devinit xiic_i2c_probe(struct platform_device *pdev)
 	if (irq < 0)
 		goto resource_missing;
 
-	pdata = mfd_get_data(pdev);
+	pdata = (struct xiic_i2c_platform_data *) pdev->dev.platform_data;
 	if (!pdata)
 		return -EINVAL;
 
diff --git a/drivers/net/ks8842.c b/drivers/net/ks8842.c
index efd44af..928b2b8 100644
--- a/drivers/net/ks8842.c
+++ b/drivers/net/ks8842.c
@@ -26,7 +26,6 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/mfd/core.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
@@ -1146,7 +1145,7 @@ static int __devinit ks8842_probe(struct platform_device *pdev)
 	struct resource *iomem;
 	struct net_device *netdev;
 	struct ks8842_adapter *adapter;
-	struct ks8842_platform_data *pdata = mfd_get_data(pdev);
+	struct ks8842_platform_data *pdata = pdev->dev.platform_data;
 	u16 id;
 	unsigned i;
 
diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
index c69c6f2..4d2c75d 100644
--- a/drivers/spi/xilinx_spi.c
+++ b/drivers/spi/xilinx_spi.c
@@ -18,7 +18,6 @@
 #include <linux/interrupt.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <linux/mfd/core.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/spi_bitbang.h>
 #include <linux/spi/xilinx_spi.h>
@@ -471,7 +470,7 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)
 	struct spi_master *master;
 	u8 i;
 
-	pdata = mfd_get_data(dev);
+	pdata = dev->dev.platform_data;
 	if (pdata) {
 		num_cs = pdata->num_chipselect;
 		little_endian = pdata->little_endian;


-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Andres Salomon]

On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz <sameo@linux.intel.com> wrote:

> Hi Grant,
> 
> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
> > Gah.  Not all devices instantiated via mfd will be an mfd device,
> > which means that the driver may very well expect an *entirely
> > different* platform_device pointer; which further means a very high
> > potential of incorrectly dereferenced structures (as evidenced by a
> > patch series that is not bisectable).  For instance, the xilinx ip
> > cores are used by more than just mfd.
> I agree. Since the vast majority of the MFD subdevices are MFD
> specific IPs, I overlooked that part. The impacted drivers are the
> timberdale and the DaVinci voice codec ones.

Can you please provide pointers to what you're referring to?  The only
code that I could find that created platform devices prefixed with
'timb-' or named 'xilinx_spi' was drivers/mfd/timberdale.c.



> To fix that problem I propose 2 alternatives:
> 
> 1) When declaring the sub devices cells, the MFD driver should
> specify an mfd_data_size value for sub devices that are not MFD
> specific. It's the MFD driver responsibility to set the cell
> properly, and the non MFD specific drivers are kept MFD agnostic.
> See my patch below for the timberdale case.
> 
> 2) Revert the mfd_get_data() call for getting sub devices platform
> data pointers. That was introduced to ease the MFD cell sharing work,
> so if we take this route we'll need the cs5535 MFD driver to pass its
> cells as platform_data pointer. Andres, can you confirm that this
> would be fine for the mfd_clone_cell() routine to keep working ?

It would break mfd_clone_cell, as it uses mfd_get_cell to grab the one
to clone.  We could change it to accept the cell as an argument.  It
would also break mfd_cell_enable/disable, of course.



> 
> Patch for solution 1:
> 
> 
>  drivers/mfd/mfd-core.c          |   13 ++++++++++---
>  drivers/mfd/timberdale.c        |   11 +++++++++++
>  include/linux/mfd/core.h        |    1 +
>  drivers/i2c/busses/i2c-ocores.c |    3 +--
>  drivers/i2c/busses/i2c-xiic.c   |    3 +--
>  drivers/net/ks8842.c            |    3 +--
>  drivers/spi/xilinx_spi.c        |    3 +--
>  7 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index d01574d..8abe510 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -75,9 +75,16 @@ static int mfd_add_device(struct device *parent,
> int id, 
>  	pdev->dev.parent = parent;
>  
> -	ret = platform_device_add_data(pdev, cell, sizeof(*cell));
> -	if (ret)
> -		goto fail_res;
> +	if (cell->mfd_data_size > 0) {
> +		ret = platform_device_add_data(pdev,
> +					cell->mfd_data,
> cell->mfd_data_size);
> +		if (ret)
> +			goto fail_res;
> +	} else {
> +		ret = platform_device_add_data(pdev, cell,
> sizeof(*cell));
> +		if (ret)
> +			goto fail_res;
> +	}
>  
>  	for (r = 0; r < cell->num_resources; r++) {
>  		res[r].name = cell->resources[r].name;

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Grant Likely]

On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon <dilinger@queued.net> wrote:
> On Fri, 1 Apr 2011 13:20:31 +0200
> Samuel Ortiz <sameo@linux.intel.com> wrote:
>
>> Hi Grant,
>>
>> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> [...]
>> > Gah.  Not all devices instantiated via mfd will be an mfd device,
>> > which means that the driver may very well expect an *entirely
>> > different* platform_device pointer; which further means a very high
>> > potential of incorrectly dereferenced structures (as evidenced by a
>> > patch series that is not bisectable).  For instance, the xilinx ip
>> > cores are used by more than just mfd.
>> I agree. Since the vast majority of the MFD subdevices are MFD
>> specific IPs, I overlooked that part. The impacted drivers are the
>> timberdale and the DaVinci voice codec ones.

Another option is you could do this for MFD devices:

struct mfd_device {
        struct platform_devce pdev;
        struct mfd_cell *cell;
};

However, that requires that drivers using the mfd_cell will *never*
get instantiated outside of the mfd infrastructure, and there is no
way to protect against this so it is probably a bad idea.

Or, mfd_cell could be added to platform_device directly which would
*by far* be the safest option at the cost of every platform_device
having a mostly unused mfd_cell pointer.  Not a significant cost in my
opinion.

One last option is I'm prototyping a way to add type-safe structure
pointers to a device, but that requires nasty CPP tricks and it's not
complete yet.  The cure might be worse than the disease here.

g.

>
> Can you please provide pointers to what you're referring to?  The only
> code that I could find that created platform devices prefixed with
> 'timb-' or named 'xilinx_spi' was drivers/mfd/timberdale.c.
>
>
>
>> To fix that problem I propose 2 alternatives:
>>
>> 1) When declaring the sub devices cells, the MFD driver should
>> specify an mfd_data_size value for sub devices that are not MFD
>> specific. It's the MFD driver responsibility to set the cell
>> properly, and the non MFD specific drivers are kept MFD agnostic.
>> See my patch below for the timberdale case.

This approach worries me because it changes the behaviour on a
per-device basis.  That could be difficult to maintain a mental model
for.  I'd rather see consistent behaviour.

>>
>> 2) Revert the mfd_get_data() call for getting sub devices platform
>> data pointers. That was introduced to ease the MFD cell sharing work,
>> so if we take this route we'll need the cs5535 MFD driver to pass its
>> cells as platform_data pointer. Andres, can you confirm that this
>> would be fine for the mfd_clone_cell() routine to keep working ?
>
> It would break mfd_clone_cell, as it uses mfd_get_cell to grab the one
> to clone.  We could change it to accept the cell as an argument.  It
> would also break mfd_cell_enable/disable, of course.
>
>
>
>>
>> Patch for solution 1:
>>
>>
>>  drivers/mfd/mfd-core.c          |   13 ++++++++++---
>>  drivers/mfd/timberdale.c        |   11 +++++++++++
>>  include/linux/mfd/core.h        |    1 +
>>  drivers/i2c/busses/i2c-ocores.c |    3 +--
>>  drivers/i2c/busses/i2c-xiic.c   |    3 +--
>>  drivers/net/ks8842.c            |    3 +--
>>  drivers/spi/xilinx_spi.c        |    3 +--
>>  7 files changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
>> index d01574d..8abe510 100644
>> --- a/drivers/mfd/mfd-core.c
>> +++ b/drivers/mfd/mfd-core.c
>> @@ -75,9 +75,16 @@ static int mfd_add_device(struct device *parent,
>> int id,
>>       pdev->dev.parent = parent;
>>
>> -     ret = platform_device_add_data(pdev, cell, sizeof(*cell));
>> -     if (ret)
>> -             goto fail_res;
>> +     if (cell->mfd_data_size > 0) {
>> +             ret = platform_device_add_data(pdev,
>> +                                     cell->mfd_data,
>> cell->mfd_data_size);
>> +             if (ret)
>> +                     goto fail_res;
>> +     } else {
>> +             ret = platform_device_add_data(pdev, cell,
>> sizeof(*cell));
>> +             if (ret)
>> +                     goto fail_res;
>> +     }
>>
>>       for (r = 0; r < cell->num_resources; r++) {
>>               res[r].name = cell->resources[r].name;
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Grant Likely]

On Fri, Apr 1, 2011 at 11:56 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon <dilinger@queued.net> wrote:
>> On Fri, 1 Apr 2011 13:20:31 +0200
>> Samuel Ortiz <sameo@linux.intel.com> wrote:
>>
>>> Hi Grant,
>>>
>>> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
>> [...]
>>> > Gah.  Not all devices instantiated via mfd will be an mfd device,
>>> > which means that the driver may very well expect an *entirely
>>> > different* platform_device pointer; which further means a very high
>>> > potential of incorrectly dereferenced structures (as evidenced by a
>>> > patch series that is not bisectable).  For instance, the xilinx ip
>>> > cores are used by more than just mfd.
>>> I agree. Since the vast majority of the MFD subdevices are MFD
>>> specific IPs, I overlooked that part. The impacted drivers are the
>>> timberdale and the DaVinci voice codec ones.
>
> Another option is you could do this for MFD devices:
>
> struct mfd_device {
>        struct platform_devce pdev;
>        struct mfd_cell *cell;
> };
>
> However, that requires that drivers using the mfd_cell will *never*
> get instantiated outside of the mfd infrastructure, and there is no
> way to protect against this so it is probably a bad idea.
>
> Or, mfd_cell could be added to platform_device directly which would
> *by far* be the safest option at the cost of every platform_device
> having a mostly unused mfd_cell pointer.  Not a significant cost in my
> opinion.
>
> One last option is I'm prototyping a way to add type-safe structure
> pointers to a device, but that requires nasty CPP tricks and it's not
> complete yet.  The cure might be worse than the disease here.

And yet another option is to create a mfd_bus_type, but that probably
isn't helpful since the one of the purposes of MFDs is that it is a
collection of non-detectable memory mapped devices that
platform_bus_type is intended to handle.

g.

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
> On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon <dilinger@queued.net> wrote:
> > On Fri, 1 Apr 2011 13:20:31 +0200
> > Samuel Ortiz <sameo@linux.intel.com> wrote:
> >
> >> Hi Grant,
> >>
> >> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> > [...]
> >> > Gah.  Not all devices instantiated via mfd will be an mfd device,
> >> > which means that the driver may very well expect an *entirely
> >> > different* platform_device pointer; which further means a very high
> >> > potential of incorrectly dereferenced structures (as evidenced by a
> >> > patch series that is not bisectable).  For instance, the xilinx ip
> >> > cores are used by more than just mfd.
> >> I agree. Since the vast majority of the MFD subdevices are MFD
> >> specific IPs, I overlooked that part. The impacted drivers are the
> >> timberdale and the DaVinci voice codec ones.
> 
> Another option is you could do this for MFD devices:
> 
> struct mfd_device {
>         struct platform_devce pdev;
>         struct mfd_cell *cell;
> };
> 
> However, that requires that drivers using the mfd_cell will *never*
> get instantiated outside of the mfd infrastructure, and there is no
> way to protect against this so it is probably a bad idea.
> 
> Or, mfd_cell could be added to platform_device directly which would
> *by far* be the safest option at the cost of every platform_device
> having a mostly unused mfd_cell pointer.  Not a significant cost in my
> opinion.
I thought about this one, but I had the impression people would want to kill
me for adding an MFD specific pointer to platform_device. I guess it's worth
giving it a try since it would be a simple and safe solution.
I'll look at it later this weekend.

Thanks for the input.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Grant Likely]

On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
>> On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon <dilinger@queued.net> wrote:
>> > On Fri, 1 Apr 2011 13:20:31 +0200
>> > Samuel Ortiz <sameo@linux.intel.com> wrote:
>> >
>> >> Hi Grant,
>> >>
>> >> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
>> > [...]
>> >> > Gah.  Not all devices instantiated via mfd will be an mfd device,
>> >> > which means that the driver may very well expect an *entirely
>> >> > different* platform_device pointer; which further means a very high
>> >> > potential of incorrectly dereferenced structures (as evidenced by a
>> >> > patch series that is not bisectable).  For instance, the xilinx ip
>> >> > cores are used by more than just mfd.
>> >> I agree. Since the vast majority of the MFD subdevices are MFD
>> >> specific IPs, I overlooked that part. The impacted drivers are the
>> >> timberdale and the DaVinci voice codec ones.
>>
>> Another option is you could do this for MFD devices:
>>
>> struct mfd_device {
>>         struct platform_devce pdev;
>>         struct mfd_cell *cell;
>> };
>>
>> However, that requires that drivers using the mfd_cell will *never*
>> get instantiated outside of the mfd infrastructure, and there is no
>> way to protect against this so it is probably a bad idea.
>>
>> Or, mfd_cell could be added to platform_device directly which would
>> *by far* be the safest option at the cost of every platform_device
>> having a mostly unused mfd_cell pointer.  Not a significant cost in my
>> opinion.
> I thought about this one, but I had the impression people would want to kill
> me for adding an MFD specific pointer to platform_device. I guess it's worth
> giving it a try since it would be a simple and safe solution.
> I'll look at it later this weekend.
>
> Thanks for the input.

[cc'ing gregkh because we're talking about modifying struct platform_device]

I'll back you up on this one.  It is a far better solution than the
alternatives.  At least with mfd, it covers a large set of devices.  I
think there is a strong argument for doing this.  Or alternatively,
the particular interesting fields from mfd_cell could be added to
platform_device.  What information do child devices need access to?

g.

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Andres Salomon]

On Fri, 1 Apr 2011 17:58:44 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz <sameo@linux.intel.com>
> wrote:
> > On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
> >> On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon
> >> <dilinger@queued.net> wrote:
> >> > On Fri, 1 Apr 2011 13:20:31 +0200
> >> > Samuel Ortiz <sameo@linux.intel.com> wrote:
> >> >
> >> >> Hi Grant,
> >> >>
> >> >> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> >> > [...]
> >> >> > Gah.  Not all devices instantiated via mfd will be an mfd
> >> >> > device, which means that the driver may very well expect an
> >> >> > *entirely different* platform_device pointer; which further
> >> >> > means a very high potential of incorrectly dereferenced
> >> >> > structures (as evidenced by a patch series that is not
> >> >> > bisectable).  For instance, the xilinx ip cores are used by
> >> >> > more than just mfd.
> >> >> I agree. Since the vast majority of the MFD subdevices are MFD
> >> >> specific IPs, I overlooked that part. The impacted drivers are
> >> >> the timberdale and the DaVinci voice codec ones.
> >>
> >> Another option is you could do this for MFD devices:
> >>
> >> struct mfd_device {
> >>         struct platform_devce pdev;
> >>         struct mfd_cell *cell;
> >> };
> >>
> >> However, that requires that drivers using the mfd_cell will *never*
> >> get instantiated outside of the mfd infrastructure, and there is no
> >> way to protect against this so it is probably a bad idea.
> >>
> >> Or, mfd_cell could be added to platform_device directly which would
> >> *by far* be the safest option at the cost of every platform_device
> >> having a mostly unused mfd_cell pointer.  Not a significant cost
> >> in my opinion.
> > I thought about this one, but I had the impression people would
> > want to kill me for adding an MFD specific pointer to
> > platform_device. I guess it's worth giving it a try since it would
> > be a simple and safe solution. I'll look at it later this weekend.
> >
> > Thanks for the input.
> 
> [cc'ing gregkh because we're talking about modifying struct
> platform_device]
> 
> I'll back you up on this one.  It is a far better solution than the
> alternatives.  At least with mfd, it covers a large set of devices.  I
> think there is a strong argument for doing this.  Or alternatively,
> the particular interesting fields from mfd_cell could be added to
> platform_device.  What information do child devices need access to?
> 

This was one of the things I was originally tempted to do (adding
mfd fields to platform_device).  I didn't think it would fly.

I can look at this stuff or help out once I have a stable internet
connection and I'm all moved in to my new place (which should be
Wednesday).

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Fri, Apr 01, 2011 at 05:58:44PM -0600, Grant Likely wrote:
> On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> > On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
> >> On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon <dilinger@queued.net> wrote:
> >> > On Fri, 1 Apr 2011 13:20:31 +0200
> >> > Samuel Ortiz <sameo@linux.intel.com> wrote:
> >> >
> >> >> Hi Grant,
> >> >>
> >> >> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> >> > [...]
> >> >> > Gah.  Not all devices instantiated via mfd will be an mfd device,
> >> >> > which means that the driver may very well expect an *entirely
> >> >> > different* platform_device pointer; which further means a very high
> >> >> > potential of incorrectly dereferenced structures (as evidenced by a
> >> >> > patch series that is not bisectable).  For instance, the xilinx ip
> >> >> > cores are used by more than just mfd.
> >> >> I agree. Since the vast majority of the MFD subdevices are MFD
> >> >> specific IPs, I overlooked that part. The impacted drivers are the
> >> >> timberdale and the DaVinci voice codec ones.
> >>
> >> Another option is you could do this for MFD devices:
> >>
> >> struct mfd_device {
> >>         struct platform_devce pdev;
> >>         struct mfd_cell *cell;
> >> };
> >>
> >> However, that requires that drivers using the mfd_cell will *never*
> >> get instantiated outside of the mfd infrastructure, and there is no
> >> way to protect against this so it is probably a bad idea.
> >>
> >> Or, mfd_cell could be added to platform_device directly which would
> >> *by far* be the safest option at the cost of every platform_device
> >> having a mostly unused mfd_cell pointer.  Not a significant cost in my
> >> opinion.
> > I thought about this one, but I had the impression people would want to kill
> > me for adding an MFD specific pointer to platform_device. I guess it's worth
> > giving it a try since it would be a simple and safe solution.
> > I'll look at it later this weekend.
> >
> > Thanks for the input.
> 
> [cc'ing gregkh because we're talking about modifying struct platform_device]
> 
> I'll back you up on this one.  It is a far better solution than the
> alternatives.  At least with mfd, it covers a large set of devices.  I
> think there is a strong argument for doing this.  Or alternatively,
> the particular interesting fields from mfd_cell could be added to
> platform_device.  What information do child devices need access to?
In some cases, they need the whole cell to clone it. So I'm up for adding an
mfd_cell pointer to the platform_device structure.
Below is a tentative patch. This is a first step and would fix all
regressions. I tried to keep the MFD dependencies as small as possible, which
is why I placed the pdev->mfd_cell building code in mfd-core.c
The second step would be to get rid of mfd_get_data() and have all subdrivers
going back to the regular platform_data way. They would no longer be dependant
on the MFD code except for those who really need it. In that case they could
just call mfd_get_cell() and get full access to their MFD cell.

--- 
 drivers/mfd/mfd-core.c          |   27 ++++++++++++++++++++++-----
 include/linux/mfd/core.h        |    7 +++++--
 include/linux/platform_device.h |    5 +++++
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d01574d..c0fc1c0 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -18,6 +18,21 @@
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
+static int mfd_platform_add_cell(struct platform_device *pdev, const struct mfd_cell *cell)
+{
+	struct mfd_cell *c;
+
+	if (cell == NULL)
+		return 0;
+
+	c = kmemdup(cell, sizeof(struct mfd_cell), GFP_KERNEL);
+	if (c == NULL)
+		return -ENOMEM;
+
+	pdev->mfd_cell = c;
+	return 0;
+}
+
 int mfd_cell_enable(struct platform_device *pdev)
 {
 	const struct mfd_cell *cell = mfd_get_cell(pdev);
@@ -75,7 +90,7 @@ static int mfd_add_device(struct device *parent, int id,
 
 	pdev->dev.parent = parent;
 
-	ret = platform_device_add_data(pdev, cell, sizeof(*cell));
+	ret = mfd_platform_add_cell(pdev, cell);
 	if (ret)
 		goto fail_res;
 
@@ -104,17 +119,17 @@ static int mfd_add_device(struct device *parent, int id,
 		if (!cell->ignore_resource_conflicts) {
 			ret = acpi_check_resource_conflict(res);
 			if (ret)
-				goto fail_res;
+				goto fail_cell;
 		}
 	}
 
 	ret = platform_device_add_resources(pdev, res, cell->num_resources);
 	if (ret)
-		goto fail_res;
+		goto fail_cell;
 
 	ret = platform_device_add(pdev);
 	if (ret)
-		goto fail_res;
+		goto fail_cell;
 
 	if (cell->pm_runtime_no_callbacks)
 		pm_runtime_no_callbacks(&pdev->dev);
@@ -123,7 +138,8 @@ static int mfd_add_device(struct device *parent, int id,
 
 	return 0;
 
-/*	platform_device_del(pdev); */
+fail_cell:
+	kfree(pdev->mfd_cell);
 fail_res:
 	kfree(res);
 fail_device:
@@ -171,6 +187,7 @@ static int mfd_remove_devices_fn(struct device *dev, void *c)
 	if (!*usage_count || (cell->usage_count < *usage_count))
 		*usage_count = cell->usage_count;
 
+	kfree(pdev->mfd_cell);
 	platform_device_unregister(pdev);
 	return 0;
 }
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index ad1b19a..0e4d3a6 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -86,7 +86,7 @@ extern int mfd_clone_cell(const char *cell, const char **clones,
  */
 static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
 {
-	return pdev->dev.platform_data;
+	return pdev->mfd_cell;
 }
 
 /*
@@ -95,7 +95,10 @@ static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
  */
 static inline void *mfd_get_data(struct platform_device *pdev)
 {
-	return mfd_get_cell(pdev)->mfd_data;
+	if (pdev->mfd_cell != NULL)
+		return mfd_get_cell(pdev)->mfd_data;
+	else
+		return pdev->dev.platform_data;
 }
 
 extern int mfd_add_devices(struct device *parent, int id,
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index d96db98..734d254 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -14,6 +14,8 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 
+struct mfd_cell;
+
 struct platform_device {
 	const char	* name;
 	int		id;
@@ -23,6 +25,9 @@ struct platform_device {
 
 	const struct platform_device_id	*id_entry;
 
+	/* MFD cell pointer */
+	struct mfd_cell	*mfd_cell;
+
 	/* arch specific additions */
 	struct pdev_archdata	archdata;
 };

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Grant Likely]

On Mon, Apr 04, 2011 at 12:03:15PM +0200, Samuel Ortiz wrote:
> On Fri, Apr 01, 2011 at 05:58:44PM -0600, Grant Likely wrote:
> > On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> > > On Fri, Apr 01, 2011 at 11:56:35AM -0600, Grant Likely wrote:
> > >> On Fri, Apr 1, 2011 at 11:47 AM, Andres Salomon <dilinger@queued.net> wrote:
> > >> > On Fri, 1 Apr 2011 13:20:31 +0200
> > >> > Samuel Ortiz <sameo@linux.intel.com> wrote:
> > >> >
> > >> >> Hi Grant,
> > >> >>
> > >> >> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> > >> > [...]
> > >> >> > Gah.  Not all devices instantiated via mfd will be an mfd device,
> > >> >> > which means that the driver may very well expect an *entirely
> > >> >> > different* platform_device pointer; which further means a very high
> > >> >> > potential of incorrectly dereferenced structures (as evidenced by a
> > >> >> > patch series that is not bisectable).  For instance, the xilinx ip
> > >> >> > cores are used by more than just mfd.
> > >> >> I agree. Since the vast majority of the MFD subdevices are MFD
> > >> >> specific IPs, I overlooked that part. The impacted drivers are the
> > >> >> timberdale and the DaVinci voice codec ones.
> > >>
> > >> Another option is you could do this for MFD devices:
> > >>
> > >> struct mfd_device {
> > >>         struct platform_devce pdev;
> > >>         struct mfd_cell *cell;
> > >> };
> > >>
> > >> However, that requires that drivers using the mfd_cell will *never*
> > >> get instantiated outside of the mfd infrastructure, and there is no
> > >> way to protect against this so it is probably a bad idea.
> > >>
> > >> Or, mfd_cell could be added to platform_device directly which would
> > >> *by far* be the safest option at the cost of every platform_device
> > >> having a mostly unused mfd_cell pointer.  Not a significant cost in my
> > >> opinion.
> > > I thought about this one, but I had the impression people would want to kill
> > > me for adding an MFD specific pointer to platform_device. I guess it's worth
> > > giving it a try since it would be a simple and safe solution.
> > > I'll look at it later this weekend.
> > >
> > > Thanks for the input.
> > 
> > [cc'ing gregkh because we're talking about modifying struct platform_device]
> > 
> > I'll back you up on this one.  It is a far better solution than the
> > alternatives.  At least with mfd, it covers a large set of devices.  I
> > think there is a strong argument for doing this.  Or alternatively,
> > the particular interesting fields from mfd_cell could be added to
> > platform_device.  What information do child devices need access to?
> In some cases, they need the whole cell to clone it. So I'm up for adding an
> mfd_cell pointer to the platform_device structure.
> Below is a tentative patch. This is a first step and would fix all
> regressions. I tried to keep the MFD dependencies as small as possible, which
> is why I placed the pdev->mfd_cell building code in mfd-core.c

Okay.

> The second step would be to get rid of mfd_get_data() and have all subdrivers
> going back to the regular platform_data way. They would no longer be dependant
> on the MFD code except for those who really need it. In that case they could
> just call mfd_get_cell() and get full access to their MFD cell.

The revert to platform_data needs to happen ASAP though.  If this
second step isn't ready really quickly, then the current patches
should be reverted to give some breathing room for creating the
replacement patches.  However, it's not such a rush if the below
patch really does eliminate all of the nastiness of the original
series. (I haven't looked and a rolled up diff of the first series and
this change, so I don't know for sure).

In principle I agree with this patch.  Some comments below.

g.

> 
> --- 
>  drivers/mfd/mfd-core.c          |   27 ++++++++++++++++++++++-----
>  include/linux/mfd/core.h        |    7 +++++--
>  include/linux/platform_device.h |    5 +++++
>  3 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index d01574d..c0fc1c0 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -18,6 +18,21 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  
> +static int mfd_platform_add_cell(struct platform_device *pdev, const struct mfd_cell *cell)
> +{
> +	struct mfd_cell *c;
> +
> +	if (cell == NULL)
> +		return 0;
> +
> +	c = kmemdup(cell, sizeof(struct mfd_cell), GFP_KERNEL);
> +	if (c == NULL)
> +		return -ENOMEM;
> +
> +	pdev->mfd_cell = c;
> +	return 0;
> +}

'sizeof(*cell) is a teensy bit safer.  Also, this can be more concise:

static int mfd_platform_add_cell(struct platform_device *pdev,
				 const struct mfd_cell *cell)
{
	if (!cell)
		return 0;

	pdev->mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
	return pdev->mfd_cell ? 0 : -ENOMEM;
}

> +
>  int mfd_cell_enable(struct platform_device *pdev)
>  {
>  	const struct mfd_cell *cell = mfd_get_cell(pdev);
> @@ -75,7 +90,7 @@ static int mfd_add_device(struct device *parent, int id,
>  
>  	pdev->dev.parent = parent;
>  
> -	ret = platform_device_add_data(pdev, cell, sizeof(*cell));
> +	ret = mfd_platform_add_cell(pdev, cell);
>  	if (ret)
>  		goto fail_res;
>  
> @@ -104,17 +119,17 @@ static int mfd_add_device(struct device *parent, int id,
>  		if (!cell->ignore_resource_conflicts) {
>  			ret = acpi_check_resource_conflict(res);
>  			if (ret)
> -				goto fail_res;
> +				goto fail_cell;
>  		}
>  	}
>  
>  	ret = platform_device_add_resources(pdev, res, cell->num_resources);
>  	if (ret)
> -		goto fail_res;
> +		goto fail_cell;
>  
>  	ret = platform_device_add(pdev);
>  	if (ret)
> -		goto fail_res;
> +		goto fail_cell;
>  
>  	if (cell->pm_runtime_no_callbacks)
>  		pm_runtime_no_callbacks(&pdev->dev);
> @@ -123,7 +138,8 @@ static int mfd_add_device(struct device *parent, int id,
>  
>  	return 0;
>  
> -/*	platform_device_del(pdev); */
> +fail_cell:
> +	kfree(pdev->mfd_cell);

Looks like kfreeing the cell should become part of the
platform_device_release() function.  Which would remove it from here,
and also ...

>  fail_res:
>  	kfree(res);
>  fail_device:
> @@ -171,6 +187,7 @@ static int mfd_remove_devices_fn(struct device *dev, void *c)
>  	if (!*usage_count || (cell->usage_count < *usage_count))
>  		*usage_count = cell->usage_count;
>  
> +	kfree(pdev->mfd_cell);

... from here.

>  	platform_device_unregister(pdev);
>  	return 0;
>  }
> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> index ad1b19a..0e4d3a6 100644
> --- a/include/linux/mfd/core.h
> +++ b/include/linux/mfd/core.h
> @@ -86,7 +86,7 @@ extern int mfd_clone_cell(const char *cell, const char **clones,
>   */
>  static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
>  {
> -	return pdev->dev.platform_data;
> +	return pdev->mfd_cell;
>  }
>  
>  /*
> @@ -95,7 +95,10 @@ static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
>   */
>  static inline void *mfd_get_data(struct platform_device *pdev)
>  {
> -	return mfd_get_cell(pdev)->mfd_data;
> +	if (pdev->mfd_cell != NULL)
> +		return mfd_get_cell(pdev)->mfd_data;
> +	else
> +		return pdev->dev.platform_data;

Blech!  Yeah, this should become consistent that platform data
*always* comes from pdev->dev.platform_data.

>  }
>  
>  extern int mfd_add_devices(struct device *parent, int id,
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index d96db98..734d254 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -14,6 +14,8 @@
>  #include <linux/device.h>
>  #include <linux/mod_devicetable.h>
>  
> +struct mfd_cell;
> +
>  struct platform_device {
>  	const char	* name;
>  	int		id;
> @@ -23,6 +25,9 @@ struct platform_device {
>  
>  	const struct platform_device_id	*id_entry;
>  
> +	/* MFD cell pointer */
> +	struct mfd_cell	*mfd_cell;
> +

Move this down to by the of_node pointer.  May as well collect all the
supplemental data about the device in the same place.

>  	/* arch specific additions */
>  	struct pdev_archdata	archdata;
>  };
> 
> -- 
> Intel Open Source Technology Centre
> http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Mon, Apr 04, 2011 at 09:04:29PM -0600, Grant Likely wrote:
> > The second step would be to get rid of mfd_get_data() and have all subdrivers
> > going back to the regular platform_data way. They would no longer be dependant
> > on the MFD code except for those who really need it. In that case they could
> > just call mfd_get_cell() and get full access to their MFD cell.
> 
> The revert to platform_data needs to happen ASAP though.  If this
> second step isn't ready really quickly, then the current patches
> should be reverted to give some breathing room for creating the
> replacement patches.  However, it's not such a rush if the below
> patch really does eliminate all of the nastiness of the original
> series. (I haven't looked and a rolled up diff of the first series and
> this change, so I don't know for sure).
I am done reverting these changes, with a final patch getting rid of
mfd_get_data. See
git://git.kernel.org/pub/scm/linux/kernel/git/sameo/mfd-2.6.git for-linus

I still need to give it a second review before pushing it to lkml for
comments. It's 20 patches long, so I'm not entirely sure Linus would take that
at that point.
Pushing patch #1 would be enough for fixing the issues introduced by the
original patchset, so I'm leaning toward pushing it and leaving the 19 other
patches for the next merge window.


> In principle I agree with this patch.  Some comments below.
Thanks for the comments. I think I addressed all of them in patch #1:


---
 drivers/base/platform.c  |    1 +
 drivers/mfd/mfd-core.c   |   15 +++++++++++++--
 include/linux/device.h   |    3 +++
 include/linux/mfd/core.h |    7 +++++--
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f051cff..bde6b97 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -149,6 +149,7 @@ static void platform_device_release(struct device *dev)
 
 	of_device_node_put(&pa->pdev.dev);
 	kfree(pa->pdev.dev.platform_data);
+	kfree(pa->pdev.dev.mfd_cell);
 	kfree(pa->pdev.resource);
 	kfree(pa);
 }
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d01574d..99b0d6d 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -18,6 +18,18 @@
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
+static int mfd_platform_add_cell(struct platform_device *pdev, const struct mfd_cell *cell)
+{
+	if (!cell)
+		return 0;
+
+	pdev->dev.mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
+	if (!pdev->dev.mfd_cell)
+		return -ENOMEM;
+
+	return 0;
+}
+
 int mfd_cell_enable(struct platform_device *pdev)
 {
 	const struct mfd_cell *cell = mfd_get_cell(pdev);
@@ -75,7 +87,7 @@ static int mfd_add_device(struct device *parent, int id,
 
 	pdev->dev.parent = parent;
 
-	ret = platform_device_add_data(pdev, cell, sizeof(*cell));
+	ret = mfd_platform_add_cell(pdev, cell);
 	if (ret)
 		goto fail_res;
 
@@ -123,7 +135,6 @@ static int mfd_add_device(struct device *parent, int id,
 
 	return 0;
 
-/*	platform_device_del(pdev); */
 fail_res:
 	kfree(res);
 fail_device:
diff --git a/include/linux/device.h b/include/linux/device.h
index ab8dfc0..cf353cf 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -33,6 +33,7 @@ struct class;
 struct subsys_private;
 struct bus_type;
 struct device_node;
+struct mfd_cell;
 
 struct bus_attribute {
 	struct attribute	attr;
@@ -444,6 +445,8 @@ struct device {
 	struct device_node	*of_node; /* associated device tree node */
 	const struct of_device_id *of_match; /* matching of_device_id from driver */
 
+	struct mfd_cell	*mfd_cell; /* MFD cell pointer */
+
 	dev_t			devt;	/* dev_t, creates the sysfs "dev" */
 
 	spinlock_t		devres_lock;
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index ad1b19a..28f81cf 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -86,7 +86,7 @@ extern int mfd_clone_cell(const char *cell, const char **clones,
  */
 static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
 {
-	return pdev->dev.platform_data;
+	return pdev->dev.mfd_cell;
 }
 
 /*
@@ -95,7 +95,10 @@ static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
  */
 static inline void *mfd_get_data(struct platform_device *pdev)
 {
-	return mfd_get_cell(pdev)->mfd_data;
+	if (pdev->dev.mfd_cell)
+		return mfd_get_cell(pdev)->mfd_data;
+	else
+		return pdev->dev.platform_data;
 }
 
 extern int mfd_add_devices(struct device *parent, int id,
-- 
1.7.2.3

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Greg KH]

On Wed, Apr 06, 2011 at 05:23:23PM +0200, Samuel Ortiz wrote:
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -33,6 +33,7 @@ struct class;
>  struct subsys_private;
>  struct bus_type;
>  struct device_node;
> +struct mfd_cell;
>  
>  struct bus_attribute {
>  	struct attribute	attr;
> @@ -444,6 +445,8 @@ struct device {
>  	struct device_node	*of_node; /* associated device tree node */
>  	const struct of_device_id *of_match; /* matching of_device_id from driver */
>  
> +	struct mfd_cell	*mfd_cell; /* MFD cell pointer */
> +

What is a "MFD cell pointer" and why is it needed in struct device?

thanks,

greg k-h

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

Hi Greg,

On Wed, Apr 06, 2011 at 08:58:05AM -0700, Greg KH wrote:
> On Wed, Apr 06, 2011 at 05:23:23PM +0200, Samuel Ortiz wrote:
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -33,6 +33,7 @@ struct class;
> >  struct subsys_private;
> >  struct bus_type;
> >  struct device_node;
> > +struct mfd_cell;
> >  
> >  struct bus_attribute {
> >  	struct attribute	attr;
> > @@ -444,6 +445,8 @@ struct device {
> >  	struct device_node	*of_node; /* associated device tree node */
> >  	const struct of_device_id *of_match; /* matching of_device_id from driver */
> >  
> > +	struct mfd_cell	*mfd_cell; /* MFD cell pointer */
> > +
> 
> What is a "MFD cell pointer" and why is it needed in struct device?
An MFD cell is an MFD instantiated device.
MFD (Multi Function Device) drivers instantiate platform devices. Those
devices drivers sometimes need a platform data pointer, sometimes an MFD
specific pointer, and sometimes both. Also, some of those drivers have been
implemented as MFD sub drivers, while others know nothing about MFD and just
expect a plain platform_data pointer.

We've been faced with the problem of being able to pass both MFD related data
and a platform_data pointer to some of those drivers. Squeezing the MFD bits
in the sub driver platform_data pointer doesn't work for drivers that know
nothing about MFDs. It also adds an additional dependency on the MFD API to
all MFD sub drivers. That prevents any of those drivers to eventually be used
as plain platform device drivers.
So, adding an MFD cell pointer to the device structure allows us to cleanly
pass both pieces of information, while keeping all the MFD sub drivers
independant from the MFD core if they want/can.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Ben Hutchings]

On Wed, 2011-04-06 at 19:05 +0200, Samuel Ortiz wrote:
> Hi Greg,
> 
> On Wed, Apr 06, 2011 at 08:58:05AM -0700, Greg KH wrote:
> > On Wed, Apr 06, 2011 at 05:23:23PM +0200, Samuel Ortiz wrote:
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -33,6 +33,7 @@ struct class;
> > >  struct subsys_private;
> > >  struct bus_type;
> > >  struct device_node;
> > > +struct mfd_cell;
> > >  
> > >  struct bus_attribute {
> > >  	struct attribute	attr;
> > > @@ -444,6 +445,8 @@ struct device {
> > >  	struct device_node	*of_node; /* associated device tree node */
> > >  	const struct of_device_id *of_match; /* matching of_device_id from driver */
> > >  
> > > +	struct mfd_cell	*mfd_cell; /* MFD cell pointer */
> > > +
> > 
> > What is a "MFD cell pointer" and why is it needed in struct device?
> An MFD cell is an MFD instantiated device.
> MFD (Multi Function Device) drivers instantiate platform devices. Those
> devices drivers sometimes need a platform data pointer, sometimes an MFD
> specific pointer, and sometimes both. Also, some of those drivers have been
> implemented as MFD sub drivers, while others know nothing about MFD and just
> expect a plain platform_data pointer.
> 
> We've been faced with the problem of being able to pass both MFD related data
> and a platform_data pointer to some of those drivers. Squeezing the MFD bits
> in the sub driver platform_data pointer doesn't work for drivers that know
> nothing about MFDs. It also adds an additional dependency on the MFD API to
> all MFD sub drivers. That prevents any of those drivers to eventually be used
> as plain platform device drivers.
> So, adding an MFD cell pointer to the device structure allows us to cleanly
> pass both pieces of information, while keeping all the MFD sub drivers
> independant from the MFD core if they want/can.

Why isn't an MFD the parent of its component devices?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

Hi Ben,

On Wed, Apr 06, 2011 at 06:16:49PM +0100, Ben Hutchings wrote:
> > So, adding an MFD cell pointer to the device structure allows us to cleanly
> > pass both pieces of information, while keeping all the MFD sub drivers
> > independant from the MFD core if they want/can.
> 
> Why isn't an MFD the parent of its component devices?
It actually is. How would that help here ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Ben Hutchings]

On Wed, 2011-04-06 at 19:51 +0200, Samuel Ortiz wrote:
> Hi Ben,
> 
> On Wed, Apr 06, 2011 at 06:16:49PM +0100, Ben Hutchings wrote:
> > > So, adding an MFD cell pointer to the device structure allows us to cleanly
> > > pass both pieces of information, while keeping all the MFD sub drivers
> > > independant from the MFD core if they want/can.
> > 
> > Why isn't an MFD the parent of its component devices?
> It actually is. How would that help here ?

I was thinking you could encode the component address in the
platform_device name (just as the bus address is the name of a normal
bus device).  That plus the parent device pointer would be sufficient
information to look up the mfd_cell.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Greg KH]

On Wed, Apr 06, 2011 at 07:05:38PM +0200, Samuel Ortiz wrote:
> Hi Greg,
> 
> On Wed, Apr 06, 2011 at 08:58:05AM -0700, Greg KH wrote:
> > On Wed, Apr 06, 2011 at 05:23:23PM +0200, Samuel Ortiz wrote:
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -33,6 +33,7 @@ struct class;
> > >  struct subsys_private;
> > >  struct bus_type;
> > >  struct device_node;
> > > +struct mfd_cell;
> > >  
> > >  struct bus_attribute {
> > >  	struct attribute	attr;
> > > @@ -444,6 +445,8 @@ struct device {
> > >  	struct device_node	*of_node; /* associated device tree node */
> > >  	const struct of_device_id *of_match; /* matching of_device_id from driver */
> > >  
> > > +	struct mfd_cell	*mfd_cell; /* MFD cell pointer */
> > > +
> > 
> > What is a "MFD cell pointer" and why is it needed in struct device?
> An MFD cell is an MFD instantiated device.
> MFD (Multi Function Device) drivers instantiate platform devices. Those
> devices drivers sometimes need a platform data pointer, sometimes an MFD
> specific pointer, and sometimes both. Also, some of those drivers have been
> implemented as MFD sub drivers, while others know nothing about MFD and just
> expect a plain platform_data pointer.

That sounds like a bug in those drivers, why not fix them to properly
pass in the correct pointer?

> We've been faced with the problem of being able to pass both MFD related data
> and a platform_data pointer to some of those drivers. Squeezing the MFD bits
> in the sub driver platform_data pointer doesn't work for drivers that know
> nothing about MFDs. It also adds an additional dependency on the MFD API to
> all MFD sub drivers. That prevents any of those drivers to eventually be used
> as plain platform device drivers.

Then they shouldn't be "plain" platform drivers, that should only be
reserved for drivers that are the "lowest" type.  Just make them MFD
devices and go from there.

> So, adding an MFD cell pointer to the device structure allows us to cleanly
> pass both pieces of information, while keeping all the MFD sub drivers
> independant from the MFD core if they want/can.

They shouldn't be "independant", make them "dependant" and go from
there.

thanks,

greg k-h

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Andres Salomon]

On Wed, 6 Apr 2011 10:56:47 -0700
Greg KH <gregkh@suse.de> wrote:

> On Wed, Apr 06, 2011 at 07:05:38PM +0200, Samuel Ortiz wrote:
> > Hi Greg,
> > 
> > On Wed, Apr 06, 2011 at 08:58:05AM -0700, Greg KH wrote:
> > > On Wed, Apr 06, 2011 at 05:23:23PM +0200, Samuel Ortiz wrote:
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -33,6 +33,7 @@ struct class;
> > > >  struct subsys_private;
> > > >  struct bus_type;
> > > >  struct device_node;
> > > > +struct mfd_cell;
> > > >  
> > > >  struct bus_attribute {
> > > >  	struct attribute	attr;
> > > > @@ -444,6 +445,8 @@ struct device {
> > > >  	struct device_node	*of_node; /* associated
> > > > device tree node */ const struct of_device_id *of_match; /*
> > > > matching of_device_id from driver */ 
> > > > +	struct mfd_cell	*mfd_cell; /* MFD cell pointer
> > > > */ +
> > > 
> > > What is a "MFD cell pointer" and why is it needed in struct
> > > device?
> > An MFD cell is an MFD instantiated device.
> > MFD (Multi Function Device) drivers instantiate platform devices.
> > Those devices drivers sometimes need a platform data pointer,
> > sometimes an MFD specific pointer, and sometimes both. Also, some
> > of those drivers have been implemented as MFD sub drivers, while
> > others know nothing about MFD and just expect a plain platform_data
> > pointer.
> 
> That sounds like a bug in those drivers, why not fix them to properly
> pass in the correct pointer?

I'm still trying to understand if this is a theoretical problem, or if
Grant has actually experienced a regression.  His mention of bisecting
made it sound like the latter was the case, but I've yet to hear of
specifically what drivers this was breaking.  Samuel described some
potential driver breakage, but nothing concrete.

I do agree that this needs a better solution, given the theoretical
breakage.

> 
> > We've been faced with the problem of being able to pass both MFD
> > related data and a platform_data pointer to some of those drivers.
> > Squeezing the MFD bits in the sub driver platform_data pointer
> > doesn't work for drivers that know nothing about MFDs. It also adds
> > an additional dependency on the MFD API to all MFD sub drivers.
> > That prevents any of those drivers to eventually be used as plain
> > platform device drivers.
> 
> Then they shouldn't be "plain" platform drivers, that should only be
> reserved for drivers that are the "lowest" type.  Just make them MFD
> devices and go from there.


The problem is of mixing "plain" platform devices and MFD devices.
It's reasonable to assume that different hardware may be using
one method or the other to create devices; in order to maintain
compatibility with the driver, one either needs to use a plain platform
device.  Alternatively, if an MFD-specific device class is created,
then MFD devices would start showing up in weird places.

> 
> > So, adding an MFD cell pointer to the device structure allows us to
> > cleanly pass both pieces of information, while keeping all the MFD
> > sub drivers independant from the MFD core if they want/can.
> 
> They shouldn't be "independant", make them "dependant" and go from
> there.
> 
> thanks,
> 
> greg k-h

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Greg KH]

On Wed, Apr 06, 2011 at 11:25:57AM -0700, Andres Salomon wrote:
> > > We've been faced with the problem of being able to pass both MFD
> > > related data and a platform_data pointer to some of those drivers.
> > > Squeezing the MFD bits in the sub driver platform_data pointer
> > > doesn't work for drivers that know nothing about MFDs. It also adds
> > > an additional dependency on the MFD API to all MFD sub drivers.
> > > That prevents any of those drivers to eventually be used as plain
> > > platform device drivers.
> > 
> > Then they shouldn't be "plain" platform drivers, that should only be
> > reserved for drivers that are the "lowest" type.  Just make them MFD
> > devices and go from there.
> 
> 
> The problem is of mixing "plain" platform devices and MFD devices.

Then don't do that.

> It's reasonable to assume that different hardware may be using
> one method or the other to create devices; in order to maintain
> compatibility with the driver, one either needs to use a plain platform
> device.  Alternatively, if an MFD-specific device class is created,
> then MFD devices would start showing up in weird places.

Then fix it.  Lots of other drivers handle different "bus types" just
fine (look at the EHCI USB driver for an example.)  Don't polute the
driver core just because you don't want to fix up the individual driver
issues that are quite obvious.

thanks,

greg k-h

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Grant Likely]

On Wed, Apr 06, 2011 at 11:38:54AM -0700, Greg KH wrote:
> On Wed, Apr 06, 2011 at 11:25:57AM -0700, Andres Salomon wrote:
> > > > We've been faced with the problem of being able to pass both MFD
> > > > related data and a platform_data pointer to some of those drivers.
> > > > Squeezing the MFD bits in the sub driver platform_data pointer
> > > > doesn't work for drivers that know nothing about MFDs. It also adds
> > > > an additional dependency on the MFD API to all MFD sub drivers.
> > > > That prevents any of those drivers to eventually be used as plain
> > > > platform device drivers.
> > > 
> > > Then they shouldn't be "plain" platform drivers, that should only be
> > > reserved for drivers that are the "lowest" type.  Just make them MFD
> > > devices and go from there.
> > 
> > 
> > The problem is of mixing "plain" platform devices and MFD devices.
> 
> Then don't do that.

>From my perspective, MFD devices are little more than a bag of
platform_devices, with the MFD layer provides infrastructure for
managing it.  It isn't that there are 'plain' platform device and
'mfd' devices.  There are only platform_devices, but some of the
drivers use additional data stored in a struct mfd.

Personally, I'm not thrilled with the approach of using struct mfd, or
more specifically making it available to drivers, but on the ugly
scale it isn't very high.

However, the changes on how struct mfd is passed that were merged in
2.6.39 were actively dangerous and are going to be reverted.  Yet
a method is still needed to pass the struct mfd in a safe way.  I
don't have a problem with adding the mfd pointer to struct
platform_device, even if it should just be a stop gap to something
better.

Independently, I have been experimenting with typesafe methods for
attaching data to devices which may very well be the long term
approach, but for the short term I see no problem with adding the mfd
pointer, particularly because it is by far safer than any of the other
immediately available options.

g.

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Wed, Apr 06, 2011 at 10:56:47AM -0700, Greg KH wrote:
> On Wed, Apr 06, 2011 at 07:05:38PM +0200, Samuel Ortiz wrote:
> > Hi Greg,
> > 
> > On Wed, Apr 06, 2011 at 08:58:05AM -0700, Greg KH wrote:
> > > On Wed, Apr 06, 2011 at 05:23:23PM +0200, Samuel Ortiz wrote:
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -33,6 +33,7 @@ struct class;
> > > >  struct subsys_private;
> > > >  struct bus_type;
> > > >  struct device_node;
> > > > +struct mfd_cell;
> > > >  
> > > >  struct bus_attribute {
> > > >  	struct attribute	attr;
> > > > @@ -444,6 +445,8 @@ struct device {
> > > >  	struct device_node	*of_node; /* associated device tree node */
> > > >  	const struct of_device_id *of_match; /* matching of_device_id from driver */
> > > >  
> > > > +	struct mfd_cell	*mfd_cell; /* MFD cell pointer */
> > > > +
> > > 
> > > What is a "MFD cell pointer" and why is it needed in struct device?
> > An MFD cell is an MFD instantiated device.
> > MFD (Multi Function Device) drivers instantiate platform devices. Those
> > devices drivers sometimes need a platform data pointer, sometimes an MFD
> > specific pointer, and sometimes both. Also, some of those drivers have been
> > implemented as MFD sub drivers, while others know nothing about MFD and just
> > expect a plain platform_data pointer.
> 
> That sounds like a bug in those drivers, why not fix them to properly
> pass in the correct pointer?
Because they're drivers for generic IPs, not MFD ones. By forcing them to use
MFD specific structure and APIs, we make it more difficult for platform code
to instantiate them.
The timberdale MFD for example is built with a Xilinx SPI controller, and a
Micrel ks8842 ethernet switch IP. Forcing those devices into being MFD devices
would mean any platform willing to instantiate them would have to use the MFD
APIs. That sounds a bit artificial to me.
Although there is currently no drivers instantiated by both an MFD driver
and some platform code, Grant complaint about the Xilinx SPI driver moving
from a platform driver to an MFD one makes sense to me. 

> > So, adding an MFD cell pointer to the device structure allows us to cleanly
> > pass both pieces of information, while keeping all the MFD sub drivers
> > independant from the MFD core if they want/can.
> 
> They shouldn't be "independant", 
Excuse my poor spelling.

> make them "dependant" and go from there.
That's what the code currently does.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Felipe Balbi]

Hi,

On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
> > > > What is a "MFD cell pointer" and why is it needed in struct device?
> > > An MFD cell is an MFD instantiated device.
> > > MFD (Multi Function Device) drivers instantiate platform devices. Those
> > > devices drivers sometimes need a platform data pointer, sometimes an MFD
> > > specific pointer, and sometimes both. Also, some of those drivers have been
> > > implemented as MFD sub drivers, while others know nothing about MFD and just
> > > expect a plain platform_data pointer.
> > 
> > That sounds like a bug in those drivers, why not fix them to properly
> > pass in the correct pointer?
> Because they're drivers for generic IPs, not MFD ones. By forcing them to use
> MFD specific structure and APIs, we make it more difficult for platform code
> to instantiate them.

I agree. What I do on those cases is to have a simple platform_device
for the core IP driver and use platform_device_id tables to do runtime
checks of the small differences. If one platform X doesn't use a
platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
allocates a platform_device with the correct name and adds that to the
driver model.

See [1] (for the core driver) and [2] (for a PCI bridge driver) for an
example of what I'm talking about.

> The timberdale MFD for example is built with a Xilinx SPI controller, and a
> Micrel ks8842 ethernet switch IP. Forcing those devices into being MFD devices
> would mean any platform willing to instantiate them would have to use the MFD
> APIs. That sounds a bit artificial to me.

do they share any address space ? If they do, then you'd need something
to synchronize, right ? If they don't, then you just add two separate
devices, they don't have to be MFD.

> Although there is currently no drivers instantiated by both an MFD driver
> and some platform code, Grant complaint about the Xilinx SPI driver moving
> from a platform driver to an MFD one makes sense to me. 

I don't think so. That's not really an MFD device is it ? It's just two
different IPs instantianted on the same ASIC/FPGA, right ? Unless they
share the register space, IMHO, there's no need to make them MFD.

[1] http://gitorious.org/usb/usb/blobs/dwc3/drivers/usb/dwc3/core.c
[2] http://gitorious.org/usb/usb/blobs/dwc3/drivers/usb/dwc3/dwc3-haps.c

-- 
balbi

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Greg KH]

On Wed, Apr 06, 2011 at 09:59:02PM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
> > > > > What is a "MFD cell pointer" and why is it needed in struct device?
> > > > An MFD cell is an MFD instantiated device.
> > > > MFD (Multi Function Device) drivers instantiate platform devices. Those
> > > > devices drivers sometimes need a platform data pointer, sometimes an MFD
> > > > specific pointer, and sometimes both. Also, some of those drivers have been
> > > > implemented as MFD sub drivers, while others know nothing about MFD and just
> > > > expect a plain platform_data pointer.
> > > 
> > > That sounds like a bug in those drivers, why not fix them to properly
> > > pass in the correct pointer?
> > Because they're drivers for generic IPs, not MFD ones. By forcing them to use
> > MFD specific structure and APIs, we make it more difficult for platform code
> > to instantiate them.
> 
> I agree. What I do on those cases is to have a simple platform_device
> for the core IP driver and use platform_device_id tables to do runtime
> checks of the small differences. If one platform X doesn't use a
> platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
> allocates a platform_device with the correct name and adds that to the
> driver model.
> 
> See [1] (for the core driver) and [2] (for a PCI bridge driver) for an
> example of what I'm talking about.

Yes, thanks for providing a real example, this is the best way to handle
this.

thanks,

greg k-h

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Felipe Balbi]

Hi,

On Wed, Apr 06, 2011 at 03:09:00PM -0700, Greg KH wrote:
> On Wed, Apr 06, 2011 at 09:59:02PM +0300, Felipe Balbi wrote:
> > Hi,
> > 
> > On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
> > > > > > What is a "MFD cell pointer" and why is it needed in struct device?
> > > > > An MFD cell is an MFD instantiated device.
> > > > > MFD (Multi Function Device) drivers instantiate platform devices. Those
> > > > > devices drivers sometimes need a platform data pointer, sometimes an MFD
> > > > > specific pointer, and sometimes both. Also, some of those drivers have been
> > > > > implemented as MFD sub drivers, while others know nothing about MFD and just
> > > > > expect a plain platform_data pointer.
> > > > 
> > > > That sounds like a bug in those drivers, why not fix them to properly
> > > > pass in the correct pointer?
> > > Because they're drivers for generic IPs, not MFD ones. By forcing them to use
> > > MFD specific structure and APIs, we make it more difficult for platform code
> > > to instantiate them.
> > 
> > I agree. What I do on those cases is to have a simple platform_device
> > for the core IP driver and use platform_device_id tables to do runtime
> > checks of the small differences. If one platform X doesn't use a
> > platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
> > allocates a platform_device with the correct name and adds that to the
> > driver model.
> > 
> > See [1] (for the core driver) and [2] (for a PCI bridge driver) for an
> > example of what I'm talking about.
> 
> Yes, thanks for providing a real example, this is the best way to handle
> this.

no problem.

ps: that's the driver for the USB3 controller which will come on OMAP5.
Driver being validate on a pre-silicon platform right now :-D In a few
weeks I'll send the driver for integration.

-- 
balbi

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

Hi Felipe,

On Wed, Apr 06, 2011 at 09:59:02PM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
> > > > > What is a "MFD cell pointer" and why is it needed in struct device?
> > > > An MFD cell is an MFD instantiated device.
> > > > MFD (Multi Function Device) drivers instantiate platform devices. Those
> > > > devices drivers sometimes need a platform data pointer, sometimes an MFD
> > > > specific pointer, and sometimes both. Also, some of those drivers have been
> > > > implemented as MFD sub drivers, while others know nothing about MFD and just
> > > > expect a plain platform_data pointer.
> > > 
> > > That sounds like a bug in those drivers, why not fix them to properly
> > > pass in the correct pointer?
> > Because they're drivers for generic IPs, not MFD ones. By forcing them to use
> > MFD specific structure and APIs, we make it more difficult for platform code
> > to instantiate them.
> 
> I agree. What I do on those cases is to have a simple platform_device
> for the core IP driver and use platform_device_id tables to do runtime
> checks of the small differences. If one platform X doesn't use a
> platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
> allocates a platform_device with the correct name and adds that to the
> driver model.
I see, thanks.
Below is a patch for the Xilinx SPI example. Although this would fix the
issue, we'd still have to do that on device per device basis. I had a similar
solution where MFD drivers would set a flag for sub drivers that don't need
any of the MFD bits. In that case the MFD core code would just forward the
platform data, instead of embedding it through an MFD cell.

Cheers,
Samuel.

---
 drivers/mfd/timberdale.c |    8 ++++----
 drivers/spi/xilinx_spi.c |   19 ++++++++++++++++++-
 include/linux/mfd/core.h |    3 +++
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
index 94c6c8a..c9220ce 100644
--- a/drivers/mfd/timberdale.c
+++ b/drivers/mfd/timberdale.c
@@ -416,7 +416,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg0[] = {
 		.mfd_data = &timberdale_radio_platform_data,
 	},
 	{
-		.name = "xilinx_spi",
+		.name = "mfd_xilinx_spi",
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
 		.mfd_data = &timberdale_xspi_platform_data,
@@ -476,7 +476,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
 		.mfd_data = &timberdale_radio_platform_data,
 	},
 	{
-		.name = "xilinx_spi",
+		.name = "mfd_xilinx_spi",
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
 		.mfd_data = &timberdale_xspi_platform_data,
@@ -526,7 +526,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg2[] = {
 		.mfd_data = &timberdale_radio_platform_data,
 	},
 	{
-		.name = "xilinx_spi",
+		.name = "mfd_xilinx_spi",
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
 		.mfd_data = &timberdale_xspi_platform_data,
@@ -570,7 +570,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg3[] = {
 		.mfd_data = &timberdale_radio_platform_data,
 	},
 	{
-		.name = "xilinx_spi",
+		.name = "mfd_xilinx_spi",
 		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
 		.resources = timberdale_spi_resources,
 		.mfd_data = &timberdale_xspi_platform_data,
diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
index c69c6f2..3287b84 100644
--- a/drivers/spi/xilinx_spi.c
+++ b/drivers/spi/xilinx_spi.c
@@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)
 	struct spi_master *master;
 	u8 i;
 
-	pdata = mfd_get_data(dev);
+	if (platform_get_device_id(dev) &&
+	    platform_get_device_id(dev)->driver_data & MFD_PLATFORM_DEVICE)
+		pdata = mfd_get_data(dev);
+	else
+		pdata = dev->dev.platform_data;
 	if (pdata) {
 		num_cs = pdata->num_chipselect;
 		little_endian = pdata->little_endian;
@@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct platform_device *dev)
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:" XILINX_SPI_NAME);
 
+static const struct platform_device_id xilinx_spi_id_table[] = {
+	{
+		.name	= XILINX_SPI_NAME,
+	},
+	{
+		.name	= "mfd_xilinx_spi",
+		.driver_data = MFD_PLATFORM_DEVICE,
+	},
+	{  },	/* Terminating Entry */
+};
+MODULE_DEVICE_TABLE(platform, xilinx_spi_id_table);
+
 static struct platform_driver xilinx_spi_driver = {
 	.probe = xilinx_spi_probe,
 	.remove = __devexit_p(xilinx_spi_remove),
@@ -538,6 +554,7 @@ static struct platform_driver xilinx_spi_driver = {
 		.owner = THIS_MODULE,
 		.of_match_table = xilinx_spi_of_match,
 	},
+	.id_table	= xilinx_spi_id_table,
 };
 
 static int __init xilinx_spi_pltfm_init(void)
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index ad1b19a..13f31f4 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -89,6 +89,9 @@ static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
 	return pdev->dev.platform_data;
 }
 
+/* */
+#define MFD_PLATFORM_DEVICE BIT(0)
+
 /*
  * Given a platform device that's been created by mfd_add_devices(), fetch
  * the .mfd_data entry from the mfd_cell that created it.


-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Grant Likely]

On Thu, Apr 07, 2011 at 03:40:23PM +0200, Samuel Ortiz wrote:
> Hi Felipe,
> 
> On Wed, Apr 06, 2011 at 09:59:02PM +0300, Felipe Balbi wrote:
> > Hi,
> > 
> > On Wed, Apr 06, 2011 at 08:47:34PM +0200, Samuel Ortiz wrote:
> > > > > > What is a "MFD cell pointer" and why is it needed in struct device?
> > > > > An MFD cell is an MFD instantiated device.
> > > > > MFD (Multi Function Device) drivers instantiate platform devices. Those
> > > > > devices drivers sometimes need a platform data pointer, sometimes an MFD
> > > > > specific pointer, and sometimes both. Also, some of those drivers have been
> > > > > implemented as MFD sub drivers, while others know nothing about MFD and just
> > > > > expect a plain platform_data pointer.
> > > > 
> > > > That sounds like a bug in those drivers, why not fix them to properly
> > > > pass in the correct pointer?
> > > Because they're drivers for generic IPs, not MFD ones. By forcing them to use
> > > MFD specific structure and APIs, we make it more difficult for platform code
> > > to instantiate them.
> > 
> > I agree. What I do on those cases is to have a simple platform_device
> > for the core IP driver and use platform_device_id tables to do runtime
> > checks of the small differences. If one platform X doesn't use a
> > platform_bus, it uses e.g. PCI, then you make a PCI "bridge" which
> > allocates a platform_device with the correct name and adds that to the
> > driver model.
> I see, thanks.
> Below is a patch for the Xilinx SPI example. Although this would fix the
> issue, we'd still have to do that on device per device basis. I had a similar
> solution where MFD drivers would set a flag for sub drivers that don't need
> any of the MFD bits. In that case the MFD core code would just forward the
> platform data, instead of embedding it through an MFD cell.

platform_data is already a fiddly bit where you don't know what
structure type platform_data points at; it is implicitly known and
easy to get wrong.  This solution makes me *very* nervous
because it would become even easier to get a mismatch on the
platform_data pointer type.

g.

> 
> Cheers,
> Samuel.
> 
> ---
>  drivers/mfd/timberdale.c |    8 ++++----
>  drivers/spi/xilinx_spi.c |   19 ++++++++++++++++++-
>  include/linux/mfd/core.h |    3 +++
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
> index 94c6c8a..c9220ce 100644
> --- a/drivers/mfd/timberdale.c
> +++ b/drivers/mfd/timberdale.c
> @@ -416,7 +416,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg0[] = {
>  		.mfd_data = &timberdale_radio_platform_data,
>  	},
>  	{
> -		.name = "xilinx_spi",
> +		.name = "mfd_xilinx_spi",
>  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
>  		.resources = timberdale_spi_resources,
>  		.mfd_data = &timberdale_xspi_platform_data,
> @@ -476,7 +476,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
>  		.mfd_data = &timberdale_radio_platform_data,
>  	},
>  	{
> -		.name = "xilinx_spi",
> +		.name = "mfd_xilinx_spi",
>  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
>  		.resources = timberdale_spi_resources,
>  		.mfd_data = &timberdale_xspi_platform_data,
> @@ -526,7 +526,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg2[] = {
>  		.mfd_data = &timberdale_radio_platform_data,
>  	},
>  	{
> -		.name = "xilinx_spi",
> +		.name = "mfd_xilinx_spi",
>  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
>  		.resources = timberdale_spi_resources,
>  		.mfd_data = &timberdale_xspi_platform_data,
> @@ -570,7 +570,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg3[] = {
>  		.mfd_data = &timberdale_radio_platform_data,
>  	},
>  	{
> -		.name = "xilinx_spi",
> +		.name = "mfd_xilinx_spi",
>  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
>  		.resources = timberdale_spi_resources,
>  		.mfd_data = &timberdale_xspi_platform_data,
> diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
> index c69c6f2..3287b84 100644
> --- a/drivers/spi/xilinx_spi.c
> +++ b/drivers/spi/xilinx_spi.c
> @@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)
>  	struct spi_master *master;
>  	u8 i;
>  
> -	pdata = mfd_get_data(dev);
> +	if (platform_get_device_id(dev) &&
> +	    platform_get_device_id(dev)->driver_data & MFD_PLATFORM_DEVICE)
> +		pdata = mfd_get_data(dev);
> +	else
> +		pdata = dev->dev.platform_data;
>  	if (pdata) {
>  		num_cs = pdata->num_chipselect;
>  		little_endian = pdata->little_endian;
> @@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct platform_device *dev)
>  /* work with hotplug and coldplug */
>  MODULE_ALIAS("platform:" XILINX_SPI_NAME);
>  
> +static const struct platform_device_id xilinx_spi_id_table[] = {
> +	{
> +		.name	= XILINX_SPI_NAME,
> +	},
> +	{
> +		.name	= "mfd_xilinx_spi",
> +		.driver_data = MFD_PLATFORM_DEVICE,
> +	},
> +	{  },	/* Terminating Entry */
> +};
> +MODULE_DEVICE_TABLE(platform, xilinx_spi_id_table);
> +
>  static struct platform_driver xilinx_spi_driver = {
>  	.probe = xilinx_spi_probe,
>  	.remove = __devexit_p(xilinx_spi_remove),
> @@ -538,6 +554,7 @@ static struct platform_driver xilinx_spi_driver = {
>  		.owner = THIS_MODULE,
>  		.of_match_table = xilinx_spi_of_match,
>  	},
> +	.id_table	= xilinx_spi_id_table,
>  };
>  
>  static int __init xilinx_spi_pltfm_init(void)
> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> index ad1b19a..13f31f4 100644
> --- a/include/linux/mfd/core.h
> +++ b/include/linux/mfd/core.h
> @@ -89,6 +89,9 @@ static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
>  	return pdev->dev.platform_data;
>  }
>  
> +/* */
> +#define MFD_PLATFORM_DEVICE BIT(0)
> +
>  /*
>   * Given a platform device that's been created by mfd_add_devices(), fetch
>   * the .mfd_data entry from the mfd_cell that created it.
> 
> 
> -- 
> Intel Open Source Technology Centre
> http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Thu, Apr 07, 2011 at 07:35:15AM -0700, Grant Likely wrote:
> > Below is a patch for the Xilinx SPI example. Although this would fix the
> > issue, we'd still have to do that on device per device basis. I had a similar
> > solution where MFD drivers would set a flag for sub drivers that don't need
> > any of the MFD bits. In that case the MFD core code would just forward the
> > platform data, instead of embedding it through an MFD cell.
> 
> platform_data is already a fiddly bit where you don't know what
> structure type platform_data points at; it is implicitly known and
> easy to get wrong.  This solution makes me *very* nervous
> because it would become even easier to get a mismatch on the
> platform_data pointer type.
How would that be more error prone than say a board file instantiating a
platform device after having set the platform_data pointer to point to an
implicitely know structure reference ?

Cheers,
Samuel.

P.S.: Would you be ok with something like the patch below ?

> > ---
> >  drivers/mfd/timberdale.c |    8 ++++----
> >  drivers/spi/xilinx_spi.c |   19 ++++++++++++++++++-
> >  include/linux/mfd/core.h |    3 +++
> >  3 files changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
> > index 94c6c8a..c9220ce 100644
> > --- a/drivers/mfd/timberdale.c
> > +++ b/drivers/mfd/timberdale.c
> > @@ -416,7 +416,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg0[] = {
> >  		.mfd_data = &timberdale_radio_platform_data,
> >  	},
> >  	{
> > -		.name = "xilinx_spi",
> > +		.name = "mfd_xilinx_spi",
> >  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
> >  		.resources = timberdale_spi_resources,
> >  		.mfd_data = &timberdale_xspi_platform_data,
> > @@ -476,7 +476,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
> >  		.mfd_data = &timberdale_radio_platform_data,
> >  	},
> >  	{
> > -		.name = "xilinx_spi",
> > +		.name = "mfd_xilinx_spi",
> >  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
> >  		.resources = timberdale_spi_resources,
> >  		.mfd_data = &timberdale_xspi_platform_data,
> > @@ -526,7 +526,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg2[] = {
> >  		.mfd_data = &timberdale_radio_platform_data,
> >  	},
> >  	{
> > -		.name = "xilinx_spi",
> > +		.name = "mfd_xilinx_spi",
> >  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
> >  		.resources = timberdale_spi_resources,
> >  		.mfd_data = &timberdale_xspi_platform_data,
> > @@ -570,7 +570,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg3[] = {
> >  		.mfd_data = &timberdale_radio_platform_data,
> >  	},
> >  	{
> > -		.name = "xilinx_spi",
> > +		.name = "mfd_xilinx_spi",
> >  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
> >  		.resources = timberdale_spi_resources,
> >  		.mfd_data = &timberdale_xspi_platform_data,
> > diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
> > index c69c6f2..3287b84 100644
> > --- a/drivers/spi/xilinx_spi.c
> > +++ b/drivers/spi/xilinx_spi.c
> > @@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)
> >  	struct spi_master *master;
> >  	u8 i;
> >  
> > -	pdata = mfd_get_data(dev);
> > +	if (platform_get_device_id(dev) &&
> > +	    platform_get_device_id(dev)->driver_data & MFD_PLATFORM_DEVICE)
> > +		pdata = mfd_get_data(dev);
> > +	else
> > +		pdata = dev->dev.platform_data;
> >  	if (pdata) {
> >  		num_cs = pdata->num_chipselect;
> >  		little_endian = pdata->little_endian;
> > @@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct platform_device *dev)
> >  /* work with hotplug and coldplug */
> >  MODULE_ALIAS("platform:" XILINX_SPI_NAME);
> >  
> > +static const struct platform_device_id xilinx_spi_id_table[] = {
> > +	{
> > +		.name	= XILINX_SPI_NAME,
> > +	},
> > +	{
> > +		.name	= "mfd_xilinx_spi",
> > +		.driver_data = MFD_PLATFORM_DEVICE,
> > +	},
> > +	{  },	/* Terminating Entry */
> > +};
> > +MODULE_DEVICE_TABLE(platform, xilinx_spi_id_table);
> > +
> >  static struct platform_driver xilinx_spi_driver = {
> >  	.probe = xilinx_spi_probe,
> >  	.remove = __devexit_p(xilinx_spi_remove),
> > @@ -538,6 +554,7 @@ static struct platform_driver xilinx_spi_driver = {
> >  		.owner = THIS_MODULE,
> >  		.of_match_table = xilinx_spi_of_match,
> >  	},
> > +	.id_table	= xilinx_spi_id_table,
> >  };
> >  
> >  static int __init xilinx_spi_pltfm_init(void)
> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > index ad1b19a..13f31f4 100644
> > --- a/include/linux/mfd/core.h
> > +++ b/include/linux/mfd/core.h
> > @@ -89,6 +89,9 @@ static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
> >  	return pdev->dev.platform_data;
> >  }
> >  
> > +/* */
> > +#define MFD_PLATFORM_DEVICE BIT(0)
> > +
> >  /*
> >   * Given a platform device that's been created by mfd_add_devices(), fetch
> >   * the .mfd_data entry from the mfd_cell that created it.
> > 
> > 
> > -- 
> > Intel Open Source Technology Centre
> > http://oss.intel.com/

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Grant Likely]

On Thu, Apr 07, 2011 at 05:03:23PM +0200, Samuel Ortiz wrote:
> On Thu, Apr 07, 2011 at 07:35:15AM -0700, Grant Likely wrote:
> > > Below is a patch for the Xilinx SPI example. Although this would fix the
> > > issue, we'd still have to do that on device per device basis. I had a similar
> > > solution where MFD drivers would set a flag for sub drivers that don't need
> > > any of the MFD bits. In that case the MFD core code would just forward the
> > > platform data, instead of embedding it through an MFD cell.
> > 
> > platform_data is already a fiddly bit where you don't know what
> > structure type platform_data points at; it is implicitly known and
> > easy to get wrong.  This solution makes me *very* nervous
> > because it would become even easier to get a mismatch on the
> > platform_data pointer type.
> How would that be more error prone than say a board file instantiating a
> platform device after having set the platform_data pointer to point to an
> implicitely know structure reference ?

Yes, platform_data is already troublesome, but at least current
convention is a 1:1 relationship between driver and platform_data
type.  I still hate it and want something better, but it is what we
have.  The problem with what having a different platform_data pointer
depending on the instantiation means that it adds yet another level of
decision that needs to be made and is very easy to get wrong.

So, yes, platform_data is bad.  I don't want to see it get any worse.

> 
> Cheers,
> Samuel.
> 
> P.S.: Would you be ok with something like the patch below ?

Not really because it requires the driver to make the correct decision
about the platform_data type depending on the driver name.  It is easy
to get wrong and the compiler cannot help you catch it.

I've talked with Greg, and adding the mfd_cell pointer to
platform_device will be okay in the short term.  In the long term I'm
looking at creating a better way of attaching type-safe data to
devices that will pretty much eliminate this issue.

> 
> > > ---
> > >  drivers/mfd/timberdale.c |    8 ++++----
> > >  drivers/spi/xilinx_spi.c |   19 ++++++++++++++++++-
> > >  include/linux/mfd/core.h |    3 +++
> > >  3 files changed, 25 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/timberdale.c b/drivers/mfd/timberdale.c
> > > index 94c6c8a..c9220ce 100644
> > > --- a/drivers/mfd/timberdale.c
> > > +++ b/drivers/mfd/timberdale.c
> > > @@ -416,7 +416,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg0[] = {
> > >  		.mfd_data = &timberdale_radio_platform_data,
> > >  	},
> > >  	{
> > > -		.name = "xilinx_spi",
> > > +		.name = "mfd_xilinx_spi",
> > >  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
> > >  		.resources = timberdale_spi_resources,
> > >  		.mfd_data = &timberdale_xspi_platform_data,
> > > @@ -476,7 +476,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg1[] = {
> > >  		.mfd_data = &timberdale_radio_platform_data,
> > >  	},
> > >  	{
> > > -		.name = "xilinx_spi",
> > > +		.name = "mfd_xilinx_spi",
> > >  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
> > >  		.resources = timberdale_spi_resources,
> > >  		.mfd_data = &timberdale_xspi_platform_data,
> > > @@ -526,7 +526,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg2[] = {
> > >  		.mfd_data = &timberdale_radio_platform_data,
> > >  	},
> > >  	{
> > > -		.name = "xilinx_spi",
> > > +		.name = "mfd_xilinx_spi",
> > >  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
> > >  		.resources = timberdale_spi_resources,
> > >  		.mfd_data = &timberdale_xspi_platform_data,
> > > @@ -570,7 +570,7 @@ static __devinitdata struct mfd_cell timberdale_cells_bar0_cfg3[] = {
> > >  		.mfd_data = &timberdale_radio_platform_data,
> > >  	},
> > >  	{
> > > -		.name = "xilinx_spi",
> > > +		.name = "mfd_xilinx_spi",
> > >  		.num_resources = ARRAY_SIZE(timberdale_spi_resources),
> > >  		.resources = timberdale_spi_resources,
> > >  		.mfd_data = &timberdale_xspi_platform_data,
> > > diff --git a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c
> > > index c69c6f2..3287b84 100644
> > > --- a/drivers/spi/xilinx_spi.c
> > > +++ b/drivers/spi/xilinx_spi.c
> > > @@ -471,7 +471,11 @@ static int __devinit xilinx_spi_probe(struct platform_device *dev)
> > >  	struct spi_master *master;
> > >  	u8 i;
> > >  
> > > -	pdata = mfd_get_data(dev);
> > > +	if (platform_get_device_id(dev) &&
> > > +	    platform_get_device_id(dev)->driver_data & MFD_PLATFORM_DEVICE)
> > > +		pdata = mfd_get_data(dev);
> > > +	else
> > > +		pdata = dev->dev.platform_data;
> > >  	if (pdata) {
> > >  		num_cs = pdata->num_chipselect;
> > >  		little_endian = pdata->little_endian;
> > > @@ -530,6 +534,18 @@ static int __devexit xilinx_spi_remove(struct platform_device *dev)
> > >  /* work with hotplug and coldplug */
> > >  MODULE_ALIAS("platform:" XILINX_SPI_NAME);
> > >  
> > > +static const struct platform_device_id xilinx_spi_id_table[] = {
> > > +	{
> > > +		.name	= XILINX_SPI_NAME,
> > > +	},
> > > +	{
> > > +		.name	= "mfd_xilinx_spi",
> > > +		.driver_data = MFD_PLATFORM_DEVICE,
> > > +	},
> > > +	{  },	/* Terminating Entry */
> > > +};
> > > +MODULE_DEVICE_TABLE(platform, xilinx_spi_id_table);
> > > +
> > >  static struct platform_driver xilinx_spi_driver = {
> > >  	.probe = xilinx_spi_probe,
> > >  	.remove = __devexit_p(xilinx_spi_remove),
> > > @@ -538,6 +554,7 @@ static struct platform_driver xilinx_spi_driver = {
> > >  		.owner = THIS_MODULE,
> > >  		.of_match_table = xilinx_spi_of_match,
> > >  	},
> > > +	.id_table	= xilinx_spi_id_table,
> > >  };
> > >  
> > >  static int __init xilinx_spi_pltfm_init(void)
> > > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> > > index ad1b19a..13f31f4 100644
> > > --- a/include/linux/mfd/core.h
> > > +++ b/include/linux/mfd/core.h
> > > @@ -89,6 +89,9 @@ static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
> > >  	return pdev->dev.platform_data;
> > >  }
> > >  
> > > +/* */
> > > +#define MFD_PLATFORM_DEVICE BIT(0)
> > > +
> > >  /*
> > >   * Given a platform device that's been created by mfd_add_devices(), fetch
> > >   * the .mfd_data entry from the mfd_cell that created it.
> > > 
> > > 
> > > -- 
> > > Intel Open Source Technology Centre
> > > http://oss.intel.com/
> 
> -- 
> Intel Open Source Technology Centre
> http://oss.intel.com/

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Grant Likely]

On Mon, Apr 4, 2011 at 8:04 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Apr 04, 2011 at 12:03:15PM +0200, Samuel Ortiz wrote:
>> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
>> index d96db98..734d254 100644
>> --- a/include/linux/platform_device.h
>> +++ b/include/linux/platform_device.h
>> @@ -14,6 +14,8 @@
>>  #include <linux/device.h>
>>  #include <linux/mod_devicetable.h>
>>
>> +struct mfd_cell;
>> +
>>  struct platform_device {
>>       const char      * name;
>>       int             id;
>> @@ -23,6 +25,9 @@ struct platform_device {
>>
>>       const struct platform_device_id *id_entry;
>>
>> +     /* MFD cell pointer */
>> +     struct mfd_cell *mfd_cell;
>> +
>
> Move this down to by the of_node pointer.  May as well collect all the
> supplemental data about the device in the same place.

So, okay.  wow.  I have *no* idea what I was smoking at this point in
time.  The of_node pointer is in struct device which is definitely not
the place to put the mfd_cell pointer (and you probably though I was
crazy when I suggested it).  Greg was totally right to complain about
moving it into struct device.  Sorry for causing trouble.

Move it back into struct platform_device and you should be good.  I
just talked to greg, and there should be any issues with locating it
there.

g.

Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Samuel Ortiz]

On Fri, Apr 01, 2011 at 10:47:56AM -0700, Andres Salomon wrote:
> On Fri, 1 Apr 2011 13:20:31 +0200
> Samuel Ortiz <sameo@linux.intel.com> wrote:
> 
> > Hi Grant,
> > 
> > On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
> [...]
> > > Gah.  Not all devices instantiated via mfd will be an mfd device,
> > > which means that the driver may very well expect an *entirely
> > > different* platform_device pointer; which further means a very high
> > > potential of incorrectly dereferenced structures (as evidenced by a
> > > patch series that is not bisectable).  For instance, the xilinx ip
> > > cores are used by more than just mfd.
> > I agree. Since the vast majority of the MFD subdevices are MFD
> > specific IPs, I overlooked that part. The impacted drivers are the
> > timberdale and the DaVinci voice codec ones.
> 
> Can you please provide pointers to what you're referring to?  The only
> code that I could find that created platform devices prefixed with
> 'timb-' or named 'xilinx_spi' was drivers/mfd/timberdale.c.
The xilinx-spi, ocores-i2c, i2c-xiic drivers and to some extend the
ks8842 ethernet driver are generic IPs that the timberdale SOC happens to
use. So I agree it's extremely unlikely that anyone could come up with a
platform that would be re-using e.g. the timb-radio IP, but I think it's less
unikely for more generic IPs such as the xilinx-spi one.


> > To fix that problem I propose 2 alternatives:
> > 
> > 1) When declaring the sub devices cells, the MFD driver should
> > specify an mfd_data_size value for sub devices that are not MFD
> > specific. It's the MFD driver responsibility to set the cell
> > properly, and the non MFD specific drivers are kept MFD agnostic.
> > See my patch below for the timberdale case.
> > 
> > 2) Revert the mfd_get_data() call for getting sub devices platform
> > data pointers. That was introduced to ease the MFD cell sharing work,
> > so if we take this route we'll need the cs5535 MFD driver to pass its
> > cells as platform_data pointer. Andres, can you confirm that this
> > would be fine for the mfd_clone_cell() routine to keep working ?
> 
> It would break mfd_clone_cell, as it uses mfd_get_cell to grab the one
> to clone.  We could change it to accept the cell as an argument.  It
> would also break mfd_cell_enable/disable, of course.
I'm talking about reverting the default behaviour of passing the MFD cell as
the platform data, and going back to the cell definitions setting their
platform_data pointer explicitely. In that case, the cs5535 driver would have
to do something like:

diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
index 155fa04..3e3841d 100644
--- a/drivers/mfd/cs5535-mfd.c
+++ b/drivers/mfd/cs5535-mfd.c
@@ -106,6 +106,7 @@ static __devinitdata struct mfd_cell cs5535_mfd_cells[] =
{
                .name = "cs5535-acpi",
                .num_resources = 1,
                .resources = &cs5535_mfd_resources[ACPI_BAR],
+               .platform_data = &cs5535_mfd_cells[ACPI_BAR],
 
                .enable = cs5535_mfd_res_enable,
                .disable = cs5535_mfd_res_disable,

mfd_get_cell would then return &cs5535_mfd_cells[ACPI_BAR].

This fix would put all sub devices drivers back to an MFD agnostic state,
although the vast majority of them will certainly never be found anywhere else
than in their current MFD SoC. That's why I'm still not sure which way to go
to fix that problem.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 08/19] t7166xb: mfd_cell is now implicitly available to drivers

[Andres Salomon]

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

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/mfd/t7l66xb.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/t7l66xb.c b/drivers/mfd/t7l66xb.c
index 9caeb4a..b9c1e4c 100644
--- a/drivers/mfd/t7l66xb.c
+++ b/drivers/mfd/t7l66xb.c
@@ -384,15 +384,6 @@ static int t7l66xb_probe(struct platform_device *dev)
 	t7l66xb_attach_irq(dev);
 
 	t7l66xb_cells[T7L66XB_CELL_NAND].driver_data = pdata->nand_data;
-	t7l66xb_cells[T7L66XB_CELL_NAND].platform_data =
-		&t7l66xb_cells[T7L66XB_CELL_NAND];
-	t7l66xb_cells[T7L66XB_CELL_NAND].data_size =
-		sizeof(t7l66xb_cells[T7L66XB_CELL_NAND]);
-
-	t7l66xb_cells[T7L66XB_CELL_MMC].platform_data =
-		&t7l66xb_cells[T7L66XB_CELL_MMC];
-	t7l66xb_cells[T7L66XB_CELL_MMC].data_size =
-		sizeof(t7l66xb_cells[T7L66XB_CELL_MMC]);
 
 	ret = mfd_add_devices(&dev->dev, dev->id,
 			      t7l66xb_cells, ARRAY_SIZE(t7l66xb_cells),
-- 
1.7.2.3

[PATCH 09/19] wl1273: 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.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/media/radio/radio-wl1273.c |    2 +-
 drivers/mfd/wl1273-core.c          |    6 ++----
 sound/soc/codecs/wl1273.c          |    2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/media/radio/radio-wl1273.c b/drivers/media/radio/radio-wl1273.c
index 7ecc8e6..ebb6eb5 100644
--- a/drivers/media/radio/radio-wl1273.c
+++ b/drivers/media/radio/radio-wl1273.c
@@ -2138,7 +2138,7 @@ static int wl1273_fm_radio_remove(struct platform_device *pdev)
 
 static int __devinit wl1273_fm_radio_probe(struct platform_device *pdev)
 {
-	struct wl1273_core **core = pdev->dev.platform_data;
+	struct wl1273_core **core = platform_get_drvdata(pdev);
 	struct wl1273_device *radio;
 	struct v4l2_ctrl *ctrl;
 	int r = 0;
diff --git a/drivers/mfd/wl1273-core.c b/drivers/mfd/wl1273-core.c
index d2ecc24..61ec252 100644
--- a/drivers/mfd/wl1273-core.c
+++ b/drivers/mfd/wl1273-core.c
@@ -79,8 +79,7 @@ static int __devinit wl1273_core_probe(struct i2c_client *client,
 
 	cell = &core->cells[children];
 	cell->name = "wl1273_fm_radio";
-	cell->platform_data = &core;
-	cell->data_size = sizeof(core);
+	cell->driver_data = &core;
 	children++;
 
 	if (pdata->children & WL1273_CODEC_CHILD) {
@@ -88,8 +87,7 @@ static int __devinit wl1273_core_probe(struct i2c_client *client,
 
 		dev_dbg(&client->dev, "%s: Have codec.\n", __func__);
 		cell->name = "wl1273-codec";
-		cell->platform_data = &core;
-		cell->data_size = sizeof(core);
+		cell->driver_data = &core;
 		children++;
 	}
 
diff --git a/sound/soc/codecs/wl1273.c b/sound/soc/codecs/wl1273.c
index 861b28f..0af2c2d 100644
--- a/sound/soc/codecs/wl1273.c
+++ b/sound/soc/codecs/wl1273.c
@@ -436,7 +436,7 @@ EXPORT_SYMBOL_GPL(wl1273_get_format);
 
 static int wl1273_probe(struct snd_soc_codec *codec)
 {
-	struct wl1273_core **core = codec->dev->platform_data;
+	struct wl1273_core **core = dev_get_drvdata(codec->dev);
 	struct wl1273_priv *wl1273;
 	int r;
 
-- 
1.7.2.3

[PATCH 10/19] sh_mobile_sdhi: mfd_cell is now implicitly available to drivers

[Andres Salomon]

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

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/mfd/sh_mobile_sdhi.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/sh_mobile_sdhi.c b/drivers/mfd/sh_mobile_sdhi.c
index 0a7df44..b511e74 100644
--- a/drivers/mfd/sh_mobile_sdhi.c
+++ b/drivers/mfd/sh_mobile_sdhi.c
@@ -147,8 +147,6 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 
 	memcpy(&priv->cell_mmc, &sh_mobile_sdhi_cell, sizeof(priv->cell_mmc));
 	priv->cell_mmc.driver_data = mmc_data;
-	priv->cell_mmc.platform_data = &priv->cell_mmc;
-	priv->cell_mmc.data_size = sizeof(priv->cell_mmc);
 
 	platform_set_drvdata(pdev, priv);
 
-- 
1.7.2.3

[PATCH 11/19] tc6393xb: mfd_cell is now implicitly available to drivers

[Andres Salomon]

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

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/mfd/tc6393xb.c |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)

diff --git a/drivers/mfd/tc6393xb.c b/drivers/mfd/tc6393xb.c
index 9a23863..4080a79 100644
--- a/drivers/mfd/tc6393xb.c
+++ b/drivers/mfd/tc6393xb.c
@@ -694,26 +694,8 @@ static int __devinit tc6393xb_probe(struct platform_device *dev)
 	}
 
 	tc6393xb_cells[TC6393XB_CELL_NAND].driver_data = tcpd->nand_data;
-	tc6393xb_cells[TC6393XB_CELL_NAND].platform_data =
-		&tc6393xb_cells[TC6393XB_CELL_NAND];
-	tc6393xb_cells[TC6393XB_CELL_NAND].data_size =
-		sizeof(tc6393xb_cells[TC6393XB_CELL_NAND]);
-
-	tc6393xb_cells[TC6393XB_CELL_MMC].platform_data =
-		&tc6393xb_cells[TC6393XB_CELL_MMC];
-	tc6393xb_cells[TC6393XB_CELL_MMC].data_size =
-		sizeof(tc6393xb_cells[TC6393XB_CELL_MMC]);
-
-	tc6393xb_cells[TC6393XB_CELL_OHCI].platform_data =
-		&tc6393xb_cells[TC6393XB_CELL_OHCI];
-	tc6393xb_cells[TC6393XB_CELL_OHCI].data_size =
-		sizeof(tc6393xb_cells[TC6393XB_CELL_OHCI]);
 
 	tc6393xb_cells[TC6393XB_CELL_FB].driver_data = tcpd->fb_data;
-	tc6393xb_cells[TC6393XB_CELL_FB].platform_data =
-		&tc6393xb_cells[TC6393XB_CELL_FB];
-	tc6393xb_cells[TC6393XB_CELL_FB].data_size =
-		sizeof(tc6393xb_cells[TC6393XB_CELL_FB]);
 
 	ret = mfd_add_devices(&dev->dev, dev->id,
 			tc6393xb_cells, ARRAY_SIZE(tc6393xb_cells),
-- 
1.7.2.3

[PATCH 12/19] twl4030: 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.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/input/misc/twl4030-vibra.c |    2 +-
 drivers/mfd/twl4030-codec.c        |    6 ++----
 sound/soc/codecs/twl4030.c         |    9 ++++++---
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/input/misc/twl4030-vibra.c b/drivers/input/misc/twl4030-vibra.c
index 014dd4a..7d7602a 100644
--- a/drivers/input/misc/twl4030-vibra.c
+++ b/drivers/input/misc/twl4030-vibra.c
@@ -196,7 +196,7 @@ static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
 
 static int __devinit twl4030_vibra_probe(struct platform_device *pdev)
 {
-	struct twl4030_codec_vibra_data *pdata = pdev->dev.platform_data;
+	struct twl4030_codec_vibra_data *pdata = platform_get_drvdata(pdev);
 	struct vibra_info *info;
 	int ret;
 
diff --git a/drivers/mfd/twl4030-codec.c b/drivers/mfd/twl4030-codec.c
index 9a4b196..d44ad30 100644
--- a/drivers/mfd/twl4030-codec.c
+++ b/drivers/mfd/twl4030-codec.c
@@ -208,15 +208,13 @@ static int __devinit twl4030_codec_probe(struct platform_device *pdev)
 	if (pdata->audio) {
 		cell = &codec->cells[childs];
 		cell->name = "twl4030-codec";
-		cell->platform_data = pdata->audio;
-		cell->data_size = sizeof(*pdata->audio);
+		cell->driver_data = pdata->audio;
 		childs++;
 	}
 	if (pdata->vibra) {
 		cell = &codec->cells[childs];
 		cell->name = "twl4030-vibra";
-		cell->platform_data = pdata->vibra;
-		cell->data_size = sizeof(*pdata->vibra);
+		cell->driver_data = pdata->vibra;
 		childs++;
 	}
 
diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index e4d464b..3721671 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -130,6 +130,7 @@ static const u8 twl4030_reg[TWL4030_CACHEREGNUM] = {
 /* codec private data */
 struct twl4030_priv {
 	struct snd_soc_codec codec;
+	struct twl4030_codec_audio_data *pdata;
 
 	unsigned int codec_powered;
 
@@ -297,8 +298,8 @@ static inline void twl4030_reset_registers(struct snd_soc_codec *codec)
 
 static void twl4030_init_chip(struct snd_soc_codec *codec)
 {
-	struct twl4030_codec_audio_data *pdata = dev_get_platdata(codec->dev);
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
+	struct twl4030_codec_audio_data *pdata = twl4030->pdata;
 	u8 reg, byte;
 	int i = 0;
 
@@ -732,9 +733,9 @@ static int aif_event(struct snd_soc_dapm_widget *w,
 
 static void headset_ramp(struct snd_soc_codec *codec, int ramp)
 {
-	struct twl4030_codec_audio_data *pdata = codec->dev->platform_data;
 	unsigned char hs_gain, hs_pop;
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
+	struct twl4030_codec_audio_data *pdata = twl4030->pdata;
 	/* Base values for ramp delay calculation: 2^19 - 2^26 */
 	unsigned int ramp_base[] = {524288, 1048576, 2097152, 4194304,
 				    8388608, 16777216, 33554432, 67108864};
@@ -2251,6 +2252,7 @@ static int twl4030_soc_resume(struct snd_soc_codec *codec)
 
 static int twl4030_soc_probe(struct snd_soc_codec *codec)
 {
+	struct twl4030_codec_audio_data *pdata = dev_get_drvdata(codec->dev);
 	struct twl4030_priv *twl4030;
 
 	twl4030 = kzalloc(sizeof(struct twl4030_priv), GFP_KERNEL);
@@ -2258,6 +2260,7 @@ static int twl4030_soc_probe(struct snd_soc_codec *codec)
 		printk("Can not allocate memroy\n");
 		return -ENOMEM;
 	}
+	twl4030->pdata = pdata;
 	snd_soc_codec_set_drvdata(codec, twl4030);
 	/* Set the defaults, and power up the codec */
 	twl4030->sysclk = twl4030_codec_get_mclk() / 1000;
@@ -2297,7 +2300,7 @@ static struct snd_soc_codec_driver soc_codec_dev_twl4030 = {
 
 static int __devinit twl4030_codec_probe(struct platform_device *pdev)
 {
-	struct twl4030_codec_audio_data *pdata = pdev->dev.platform_data;
+	struct twl4030_codec_audio_data *pdata = platform_get_drvdata(pdev);
 
 	if (!pdata) {
 		dev_err(&pdev->dev, "platform_data is missing\n");
-- 
1.7.2.3

Re: [PATCH 12/19] twl4030: mfd_cell is now implicitly available to drivers

[Dmitry Torokhov]

On Wed, Feb 02, 2011 at 08:15:22PM -0800, Andres Salomon wrote:
>  static int __devinit twl4030_vibra_probe(struct platform_device *pdev)
>  {
> -	struct twl4030_codec_vibra_data *pdata = pdev->dev.platform_data;
> +	struct twl4030_codec_vibra_data *pdata = platform_get_drvdata(pdev);

No, device's drvdata belongs to _this_ driver, and it is supposed to
manage it and use as it sees fit.

Note platform_set_drvdata(pdev, info) later in this function along with
platform_set_drvdata(pdev, NULL) in twl4030_vibra_remove(), which means
that with your change you will be able to bind the device only once.

-- 
Dmitry

Re: [PATCH 12/19] twl4030: mfd_cell is now implicitly available to drivers

[Andres Salomon]

On Wed, 2 Feb 2011 22:05:21 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Wed, Feb 02, 2011 at 08:15:22PM -0800, Andres Salomon wrote:
> >  static int __devinit twl4030_vibra_probe(struct platform_device
> > *pdev) {
> > -	struct twl4030_codec_vibra_data *pdata =
> > pdev->dev.platform_data;
> > +	struct twl4030_codec_vibra_data *pdata =
> > platform_get_drvdata(pdev);
> 
> No, device's drvdata belongs to _this_ driver, and it is supposed to
> manage it and use as it sees fit.

Right, so it's used to pass data to the probe function; once the probe
function has obtained the pdata pointer, it's free to do with it what
it will.


> 
> Note platform_set_drvdata(pdev, info) later in this function along
> with platform_set_drvdata(pdev, NULL) in twl4030_vibra_remove(),
> which means that with your change you will be able to bind the device
> only once.
> 

Hm, good point; if the driver is reloaded, the pdev that was created by
mfd-core will have lost the pointer to pdata.

I wonder if I should be using mfd's driver_data instead. I used
platform_data because a bunch of drivers had already made use of it to
pass cell information..

Re: [PATCH 12/19] twl4030: mfd_cell is now implicitly available to drivers

[Dmitry Torokhov]

On Wed, Feb 02, 2011 at 10:39:59PM -0800, Andres Salomon wrote:
> On Wed, 2 Feb 2011 22:05:21 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Wed, Feb 02, 2011 at 08:15:22PM -0800, Andres Salomon wrote:
> > >  static int __devinit twl4030_vibra_probe(struct platform_device
> > > *pdev) {
> > > -	struct twl4030_codec_vibra_data *pdata =
> > > pdev->dev.platform_data;
> > > +	struct twl4030_codec_vibra_data *pdata =
> > > platform_get_drvdata(pdev);
> > 
> > No, device's drvdata belongs to _this_ driver, and it is supposed to
> > manage it and use as it sees fit.
> 
> Right, so it's used to pass data to the probe function; once the probe
> function has obtained the pdata pointer, it's free to do with it what
> it will.
> 
> 
> > 
> > Note platform_set_drvdata(pdev, info) later in this function along
> > with platform_set_drvdata(pdev, NULL) in twl4030_vibra_remove(),
> > which means that with your change you will be able to bind the device
> > only once.
> > 
> 
> Hm, good point; if the driver is reloaded, the pdev that was created by
> mfd-core will have lost the pointer to pdata.
> 
> I wonder if I should be using mfd's driver_data instead. I used
> platform_data because a bunch of drivers had already made use of it to
> pass cell information..

Then they are doing it incorrectly. One possible way is to have parent
device carry relevant data in its drvdata and have children get it from
there.

-- 
Dmitry

Re: [PATCH 12/19] twl4030: mfd_cell is now implicitly available to drivers

[Andres Salomon]

On Wed, 2 Feb 2011 22:53:39 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Wed, Feb 02, 2011 at 10:39:59PM -0800, Andres Salomon wrote:
> > On Wed, 2 Feb 2011 22:05:21 -0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > > On Wed, Feb 02, 2011 at 08:15:22PM -0800, Andres Salomon wrote:
> > > >  static int __devinit twl4030_vibra_probe(struct platform_device
> > > > *pdev) {
> > > > -	struct twl4030_codec_vibra_data *pdata =
> > > > pdev->dev.platform_data;
> > > > +	struct twl4030_codec_vibra_data *pdata =
> > > > platform_get_drvdata(pdev);
> > > 
> > > No, device's drvdata belongs to _this_ driver, and it is supposed
> > > to manage it and use as it sees fit.
> > 
> > Right, so it's used to pass data to the probe function; once the
> > probe function has obtained the pdata pointer, it's free to do with
> > it what it will.
> > 
> > 
> > > 
> > > Note platform_set_drvdata(pdev, info) later in this function along
> > > with platform_set_drvdata(pdev, NULL) in twl4030_vibra_remove(),
> > > which means that with your change you will be able to bind the
> > > device only once.
> > > 
> > 
> > Hm, good point; if the driver is reloaded, the pdev that was
> > created by mfd-core will have lost the pointer to pdata.
> > 
> > I wonder if I should be using mfd's driver_data instead. I used
> > platform_data because a bunch of drivers had already made use of it
> > to pass cell information..
> 
> Then they are doing it incorrectly. One possible way is to have parent
> device carry relevant data in its drvdata and have children get it
> from there.
> 

I believe some drivers are even using the parent device already.  See
drivers/leds/leds-mc13783.c, for example, whose parent device drvdata
is used to pass around a struct mc13783 to its children.  Sounds
like a possibility, will need to look into it further.

Re: [PATCH 12/19] twl4030: mfd_cell is now implicitly available to drivers

[Mark Brown]

On Wed, Feb 02, 2011 at 11:03:26PM -0800, Andres Salomon wrote:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> > Then they are doing it incorrectly. One possible way is to have parent
> > device carry relevant data in its drvdata and have children get it
> > from there.

> I believe some drivers are even using the parent device already.  See
> drivers/leds/leds-mc13783.c, for example, whose parent device drvdata
> is used to pass around a struct mc13783 to its children.  Sounds
> like a possibility, will need to look into it further.

That's the current best practice approach.

Re: [PATCH 12/19] twl4030: mfd_cell is now implicitly available to drivers

[Andres Salomon]

On Thu, 3 Feb 2011 09:31:54 +0000
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Wed, Feb 02, 2011 at 11:03:26PM -0800, Andres Salomon wrote:
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > > Then they are doing it incorrectly. One possible way is to have
> > > parent device carry relevant data in its drvdata and have
> > > children get it from there.
> 
> > I believe some drivers are even using the parent device already.
> > See drivers/leds/leds-mc13783.c, for example, whose parent device
> > drvdata is used to pass around a struct mc13783 to its children.
> > Sounds like a possibility, will need to look into it further.
> 
> That's the current best practice approach.

One of the main reasons to have the cell data in the platform
device rather than its parent device is because the parent device will
have the entire array of cells.  This isn't helpful when you want to
know specifically which cell's .enable hook to call.  One possibility
is to use the pdev->id field.  Currently, mfd-core allows drivers to
override this per-cell; depending upon how drivers make use of this,
I'm tempted to get rid of it from the mfd_cell struct and just always
have it be an index into the mfd_cell array (dictated by
mfd_add_devices).  Of course, wm831x-core/wm831x-dcdc does some pretty
weird stuff with ->id, so I still need to figure out if what it's doing
is really necessary.

I'm also struggling with a way to have cells automatically saved
by mfd-core. One (ugly) option would be to allow an mfd driver to set
drvdata, and mfd-core (in mfd_add_devices) would allocate a struct that
includes a pointer to drvdata as well as to the mfd_cells.  Drvdata
would be updated to point to this new struct instead.  In one scenario,
this requires evil macro redefining; in another, a weird API addition
to mfd_add_devices to pass the size of what's pointed to by the
parent's drvdata.  I don't like this option very much.

Another option is to require structs that mfd drivers assign to
drvdata to include a pointer to cells, but I'd really like a way to
enforce that with the compiler (BUILD_BUG_ON comes to mind).
It also runs into the problem of not knowing the type in drvdata. I'm
imagining something like:

#define mfd_get_cell(pdev, type) (((type *)platform_get_drvdata(pdev))->cell)

Even that would require the weird API of mfd_add_devices needing to be
passed the type of what's pointed to by the parent's drvdata.

Re: [PATCH 12/19] twl4030: mfd_cell is now implicitly available to drivers

[Andres Salomon]

On Fri, 4 Feb 2011 18:39:13 -0800
Andres Salomon <dilinger@queued.net> wrote:

> On Thu, 3 Feb 2011 09:31:54 +0000
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> 
> > On Wed, Feb 02, 2011 at 11:03:26PM -0800, Andres Salomon wrote:
> > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > > > Then they are doing it incorrectly. One possible way is to have
> > > > parent device carry relevant data in its drvdata and have
> > > > children get it from there.
> > 
> > > I believe some drivers are even using the parent device already.
> > > See drivers/leds/leds-mc13783.c, for example, whose parent device
> > > drvdata is used to pass around a struct mc13783 to its children.
> > > Sounds like a possibility, will need to look into it further.
> > 
> > That's the current best practice approach.
> 
[rambling]
> Even that would require the weird API of mfd_add_devices needing to be
> passed the type of what's pointed to by the parent's drvdata.
> 

Actually, how about something like the following?  This would leave
drvdata unaffected.



diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d83ad0f..a324a83 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -25,6 +25,7 @@ static int mfd_add_device(struct device *parent, int id,
 {
 	struct resource *res;
 	struct platform_device *pdev;
+	struct mfd_platform_data pdata = { NULL, NULL };
 	int ret = -ENOMEM;
 	int r;
 
@@ -39,12 +40,18 @@ static int mfd_add_device(struct device *parent, int id,
 	pdev->dev.parent = parent;
 	platform_set_drvdata(pdev, cell->driver_data);
 
+	pdata.cell = cell;
 	if (cell->data_size) {
-		ret = platform_device_add_data(pdev,
-					cell->platform_data, cell->data_size);
-		if (ret)
+		pdata.platform_data = kmemdup(cell->platform_data,
+				cell->data_size, GFP_KERNEL);
+		if  (!pdata.platform_data) {
+			ret = -ENOMEM;
 			goto fail_res;
+		}
 	}
+	ret = platform_device_add_data(pdev, &pdata, sizeof(pdata));
+	if (ret)
+		goto fail_pdata;
 
 	for (r = 0; r < cell->num_resources; r++) {
 		res[r].name = cell->resources[r].name;
@@ -91,6 +98,9 @@ static int mfd_add_device(struct device *parent, int id,
 	return 0;
 
 /*	platform_device_del(pdev); */
+fail_pdata:
+	if (pdata.platform_data)
+		kfree(pdata.platform_data);
 fail_res:
 	kfree(res);
 fail_device:
@@ -122,6 +132,7 @@ EXPORT_SYMBOL(mfd_add_devices);
 
 static int mfd_remove_devices_fn(struct device *dev, void *unused)
 {
+	/* TODO: nuke pdata memory */
 	platform_device_unregister(to_platform_device(dev));
 	return 0;
 }
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 835996e..dbc52a2 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -55,6 +55,26 @@ struct mfd_cell {
 	bool			pm_runtime_no_callbacks;
 };
 
+/* simple wrapper for a platform device's pdata */
+struct mfd_platform_data {
+	void *platform_data;
+	struct mfd_cell *cell;
+};
+
+/*
+ * Given a platform device that's been created by mfd_add_devices(), fetch
+ * the mfd_cell that created it.
+ */
+static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
+{
+	return ((struct mfd_platform_data *)pdev->dev.platform_data)->cell;
+}
+
+static inline void *mfd_platform_data(struct platform_device *pdev)
+{
+	return ((struct mfd_platform_data *)pdev->dev.platform_data)->data;
+}
+
 extern int mfd_add_devices(struct device *parent, int id,
 			   const struct mfd_cell *cells, int n_devs,
 			   struct resource *mem_base,

Re: [PATCH 12/19] twl4030: mfd_cell is now implicitly available to drivers

[Peter Ujfalusi]

On 02/03/11 09:03, ext Andres Salomon wrote:
> I believe some drivers are even using the parent device already.  See
> drivers/leds/leds-mc13783.c, for example, whose parent device drvdata
> is used to pass around a struct mc13783 to its children.  Sounds
> like a possibility, will need to look into it further.

I briefly looked at the drivers/leds/leds-mc13783.c, and the related
drivers/mfd/mc13xxx.c drivers.
This also uses the same way to pass the needed platform data to it's
child devices, as the twl4030 (audio/codec/vibra) MFD does.
Also the leds-mc13783.c does not actually uses any config values from
the parent dev. The mc13783_led->master is needed to be locally stored,
since the led driver calls functions from the MFD core, which needs that
as parameter.

I don't really see any need to change the drivers in this regard,
however it would be nicer, if we replace for example the:
	struct twl4030_codec_data *pdata = pdev->dev.platform_data;
with
	struct twl4030_codec_data *pdata = dev_get_platdata(&pdev->dev);

The information passed to the vibra, and ASoC codec driver is for them
only, so there is no need for the vibra driver to know anything about
things, which concerns only the ASoC codec driver.

-- 
Péter

Re: [PATCH 12/19] twl4030: mfd_cell is now implicitly available to drivers

[Uwe Kleine-König]

Hello Andres,

On Wed, Feb 02, 2011 at 11:03:26PM -0800, Andres Salomon wrote:
> On Wed, 2 Feb 2011 22:53:39 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Wed, Feb 02, 2011 at 10:39:59PM -0800, Andres Salomon wrote:
> > > On Wed, 2 Feb 2011 22:05:21 -0800
> > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > 
> > > > On Wed, Feb 02, 2011 at 08:15:22PM -0800, Andres Salomon wrote:
> > > > >  static int __devinit twl4030_vibra_probe(struct platform_device
> > > > > *pdev) {
> > > > > -	struct twl4030_codec_vibra_data *pdata =
> > > > > pdev->dev.platform_data;
> > > > > +	struct twl4030_codec_vibra_data *pdata =
> > > > > platform_get_drvdata(pdev);
> > > > 
> > > > No, device's drvdata belongs to _this_ driver, and it is supposed
> > > > to manage it and use as it sees fit.
> > > 
> > > Right, so it's used to pass data to the probe function; once the
> > > probe function has obtained the pdata pointer, it's free to do with
> > > it what it will.
> > > 
> > > 
> > > > 
> > > > Note platform_set_drvdata(pdev, info) later in this function along
> > > > with platform_set_drvdata(pdev, NULL) in twl4030_vibra_remove(),
> > > > which means that with your change you will be able to bind the
> > > > device only once.
> > > > 
> > > 
> > > Hm, good point; if the driver is reloaded, the pdev that was
> > > created by mfd-core will have lost the pointer to pdata.
> > > 
> > > I wonder if I should be using mfd's driver_data instead. I used
> > > platform_data because a bunch of drivers had already made use of it
> > > to pass cell information..
> > 
> > Then they are doing it incorrectly. One possible way is to have parent
> > device carry relevant data in its drvdata and have children get it
> > from there.
> > 
> 
> I believe some drivers are even using the parent device already.  See
> drivers/leds/leds-mc13783.c, for example, whose parent device drvdata
> is used to pass around a struct mc13783 to its children.  Sounds
> like a possibility, will need to look into it further.
IMHO this isn't optimal done.  The led driver somehow needs access to a
struct mc13xxx because that one defines how to change the led-related
registers.

If you ask me, the most clean solution would be that the functions like
mc13xxx_lock and mc13xxx_reg_rmw wouldn't take a struct mc13xxx * as
first parameter but a struct device *.  Because in fact it's not the led
driver's business what the mfd driver stores in his driver data.

(Note, I said clean, neither easy nor effective nor best.)

Uwe

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

[PATCH 13/19] tc6387xb: mfd_cell is now implicitly available to drivers

[Andres Salomon]

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

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/mfd/tc6387xb.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/tc6387xb.c b/drivers/mfd/tc6387xb.c
index 6315f63..6bf1775 100644
--- a/drivers/mfd/tc6387xb.c
+++ b/drivers/mfd/tc6387xb.c
@@ -190,11 +190,6 @@ static int __devinit tc6387xb_probe(struct platform_device *dev)
 
 	printk(KERN_INFO "Toshiba tc6387xb initialised\n");
 
-	tc6387xb_cells[TC6387XB_CELL_MMC].platform_data =
-		&tc6387xb_cells[TC6387XB_CELL_MMC];
-	tc6387xb_cells[TC6387XB_CELL_MMC].data_size =
-		sizeof(tc6387xb_cells[TC6387XB_CELL_MMC]);
-
 	ret = mfd_add_devices(&dev->dev, dev->id, tc6387xb_cells,
 			      ARRAY_SIZE(tc6387xb_cells), iomem, irq);
 
-- 
1.7.2.3

[PATCH 14/19] janz: 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.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/gpio/janz-ttl.c      |    2 +-
 drivers/mfd/janz-cmodio.c    |    3 +--
 drivers/net/can/janz-ican3.c |    2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/janz-ttl.c b/drivers/gpio/janz-ttl.c
index 813ac07..98a3f9d 100644
--- a/drivers/gpio/janz-ttl.c
+++ b/drivers/gpio/janz-ttl.c
@@ -149,7 +149,7 @@ static int __devinit ttl_probe(struct platform_device *pdev)
 	struct resource *res;
 	int ret;
 
-	pdata = pdev->dev.platform_data;
+	pdata = platform_get_drvdata(pdev);
 	if (!pdata) {
 		dev_err(dev, "no platform data\n");
 		ret = -ENXIO;
diff --git a/drivers/mfd/janz-cmodio.c b/drivers/mfd/janz-cmodio.c
index 36a166b..77e3a1f 100644
--- a/drivers/mfd/janz-cmodio.c
+++ b/drivers/mfd/janz-cmodio.c
@@ -86,8 +86,7 @@ static int __devinit cmodio_setup_subdevice(struct cmodio_device *priv,
 
 	/* Add platform data */
 	pdata->modno = modno;
-	cell->platform_data = pdata;
-	cell->data_size = sizeof(*pdata);
+	cell->driver_data = pdata;
 
 	/* MODULbus registers -- PCI BAR3 is big-endian MODULbus access */
 	res->flags = IORESOURCE_MEM;
diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index b9a6d7a..7d282c3 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -1643,7 +1643,7 @@ static int __devinit ican3_probe(struct platform_device *pdev)
 	struct device *dev;
 	int ret;
 
-	pdata = pdev->dev.platform_data;
+	pdata = platform_get_drvdata(pdev);
 	if (!pdata)
 		return -ENXIO;
 
-- 
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

Re: [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/  |

Re: [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/  |

Re: [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.

[PATCH 16/19] mfd-core: drop platform_data/data_size from core mfd_cell struct

[Andres Salomon]

Now that there are no more users of this, drop it.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 include/linux/mfd/core.h |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index 82b2a5c..c74089e 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -33,10 +33,6 @@ struct mfd_cell {
 	/* driver-specific data for MFD-aware "cell" drivers */
 	void			*driver_data;
 
-	/* These two are unused */
-	void			*platform_data;
-	size_t			data_size;
-
 	/*
 	 * This resources can be specified relatively to the parent device.
 	 * For accessing device you should use resources from device
-- 
1.7.2.3

[PATCH 17/19] mfd-core: add refcounting support to mfd_cells

[Andres Salomon]

This provides convenience functions for sharing of cells across
multiple mfd clients.  Mfd drivers can provide enable/disable hooks
to actually tweak the hardware, and clients can call
mfd_shared_cell_{en,dis}able without having to worry about whether
or not another client happens to have enabled or disabled the
cell/hardware.

Note that this is purely optional; drivers can continue to use
the mfd_cell's enable/disable hooks for their own purposes, if
desired.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/mfd/mfd-core.c   |   51 ++++++++++++++++++++++++++++++++++++++++++---
 include/linux/mfd/core.h |   14 +++++++++++-
 2 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index d8b9e35..90e9483 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -18,6 +18,30 @@
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
+int mfd_shared_cell_enable(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+
+	/* only call enable if the cell hasn't already been enabled */
+	if (atomic_inc_return(cell->usage_count) > 1)
+		return 0;
+
+	return cell->enable(pdev);
+}
+EXPORT_SYMBOL(mfd_shared_cell_enable);
+
+int mfd_shared_cell_disable(struct platform_device *pdev)
+{
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+
+	/* only disable if no other clients are using it */
+	if (atomic_dec_return(cell->usage_count) != 0)
+		return 0;
+
+	return cell->disable(pdev);
+}
+EXPORT_SYMBOL(mfd_shared_cell_disable);
+
 static int mfd_add_device(struct device *parent, int id,
 			  const struct mfd_cell *cell,
 			  struct resource *mem_base,
@@ -97,14 +121,22 @@ fail_alloc:
 }
 
 int mfd_add_devices(struct device *parent, int id,
-		    const struct mfd_cell *cells, int n_devs,
+		    struct mfd_cell *cells, int n_devs,
 		    struct resource *mem_base,
 		    int irq_base)
 {
 	int i;
 	int ret = 0;
+	atomic_t *cnts;
+
+	/* initialize reference counting for all cells */
+	cnts = kcalloc(sizeof(*cnts), n_devs, GFP_KERNEL);
+	if (!cnts)
+		return -ENOMEM;
 
 	for (i = 0; i < n_devs; i++) {
+		atomic_set(&cnts[i], 0);
+		cells[i].usage_count = &cnts[i];
 		ret = mfd_add_device(parent, id, cells + i, mem_base, irq_base);
 		if (ret)
 			break;
@@ -117,15 +149,26 @@ int mfd_add_devices(struct device *parent, int id,
 }
 EXPORT_SYMBOL(mfd_add_devices);
 
-static int mfd_remove_devices_fn(struct device *dev, void *unused)
+static int mfd_remove_devices_fn(struct device *dev, void *c)
 {
-	platform_device_unregister(to_platform_device(dev));
+	struct platform_device *pdev = to_platform_device(dev);
+	const struct mfd_cell *cell = mfd_get_cell(pdev);
+	atomic_t **usage_count = c;
+
+	/* find the lowest address of usage_count pointers (for freeing) */
+	if (!*usage_count || (cell->usage_count < *usage_count))
+		*usage_count = cell->usage_count;
+
+	platform_device_unregister(pdev);
 	return 0;
 }
 
 void mfd_remove_devices(struct device *parent)
 {
-	device_for_each_child(parent, NULL, mfd_remove_devices_fn);
+	atomic_t *cnts = NULL;
+
+	device_for_each_child(parent, &cnts, mfd_remove_devices_fn);
+	kfree(cnts);
 }
 EXPORT_SYMBOL(mfd_remove_devices);
 
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index c74089e..d13b992 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -25,8 +25,11 @@ struct mfd_cell {
 	const char		*name;
 	int			id;
 
+	/* refcounting for multiple drivers to use a single cell */
+	atomic_t		*usage_count;
 	int			(*enable)(struct platform_device *dev);
 	int			(*disable)(struct platform_device *dev);
+
 	int			(*suspend)(struct platform_device *dev);
 	int			(*resume)(struct platform_device *dev);
 
@@ -51,6 +54,15 @@ struct mfd_cell {
 };
 
 /*
+ * Convenience functions for clients using shared cells.  Refcounting
+ * happens automatically, with the cell's enable/disable callbacks
+ * being called only when a device is first being enabled or no other
+ * clients are making use of it.
+ */
+extern int mfd_shared_cell_enable(struct platform_device *pdev);
+extern int mfd_shared_cell_disable(struct platform_device *pdev);
+
+/*
  * Given a platform device that's been created by mfd_add_devices(), fetch
  * the mfd_cell that created it.
  */
@@ -60,7 +72,7 @@ static inline const struct mfd_cell *mfd_get_cell(struct platform_device *pdev)
 }
 
 extern int mfd_add_devices(struct device *parent, int id,
-			   const struct mfd_cell *cells, int n_devs,
+			   struct mfd_cell *cells, int n_devs,
 			   struct resource *mem_base,
 			   int irq_base);
 
-- 
1.7.2.3

[PATCH 18/19] mfd-core: add platform_device sharing support for mfd

[Andres Salomon]

This adds functions to enable platform_device sharing for mfd clients.

Each platform driver (mfd client) that wants to share an mfd_cell's
platform_device uses the mfd_shared_platform_driver_{un,}register()
functions instead of platform_driver_{un,}register().  Along with
registering the platform driver, these also register a new platform
device with the same characteristics as the original, but a different
name.  Given an mfd_cell with the name "foo", the first call to
mfd_shared_platform_driver_register will register a platform device
named "foo0", as well as an associated platform_driver named "foo0".
The second call would register a platform device and driver with the
name "foo1", and so on.

This deals with platform handling only; mfd driver-specific details,
hardware handling, refcounting, etc are all dealt with separately.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/mfd/mfd-core.c   |  115 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/core.h |    9 ++++
 2 files changed, 124 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 90e9483..a27a377 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -172,5 +172,120 @@ void mfd_remove_devices(struct device *parent)
 }
 EXPORT_SYMBOL(mfd_remove_devices);
 
+/*
+ * Find the first available platform_driver name, appending decimals
+ * after the name.  That is, given the string "foo", check whether
+ * "foo0", "foo1", "foo2", etc are in use.  Once a name is found
+ * that isn't in use, fill in *newname with it.
+ *
+ * If this returns non-zero, *newname must be kfree()d.
+ */
+static int get_unique_driver_name(const char *oldname, char **newname)
+{
+	struct device_driver *drv;
+	char *endp;
+	size_t len;
+	int i;
+
+	/* allocate memory for new name; includes space for extra 2 digits */
+	len = strlen(oldname);
+	*newname = kmalloc(len + 3, GFP_KERNEL);
+	if (!*newname)
+		return -ENOMEM;
+	endp = strcpy(*newname, oldname) + len;
+	BUG_ON(*endp != '\0');
+
+	/* search for the first driver name available */
+	for (i = 0; i < 99; i++) {
+		sprintf(endp, "%d", i);
+		drv = driver_find(*newname, &platform_bus_type);
+		if (!drv)
+			break;
+		put_driver(drv);
+	}
+
+	if (i > 99) {
+		/* we have too many subdevices registered?  allow up to 99. */
+		kfree(*newname);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int add_shared_platform_device(const char *cell, const char *name)
+{
+	struct mfd_cell cell_entry;
+	struct device *dev;
+	struct platform_device *pdev;
+	int err;
+
+	/* check if we've already registered a device (don't fail if we have) */
+	if (bus_find_device_by_name(&platform_bus_type, NULL, name))
+		return 0;
+
+	/* fetch the parent cell's device (should already be registered!) */
+	dev = bus_find_device_by_name(&platform_bus_type, NULL, cell);
+	if (!dev) {
+		printk(KERN_ERR "failed to find device for cell %s\n", cell);
+		return -ENODEV;
+	}
+	pdev = to_platform_device(dev);
+	memcpy(&cell_entry, mfd_get_cell(pdev), sizeof(cell_entry));
+
+	WARN_ON(!cell_entry.enable);
+
+printk(KERN_WARNING "found cell %s, registering platform device %s\n", cell_entry.name, name);
+	cell_entry.name = name;
+	err = mfd_add_device(pdev->dev.parent, -1, &cell_entry, NULL, 0);
+	if (err)
+		dev_err(dev, "MFD add devices failed: %d\n", err);
+	return err;
+}
+
+int mfd_shared_platform_driver_register(struct platform_driver *drv,
+		const char *cellname)
+{
+	char *unique = NULL;
+	int err;
+
+	err = get_unique_driver_name(cellname, &unique);
+	if (err)
+		return err;
+printk(KERN_WARNING "registering platform device %s\n", unique);
+
+	err = add_shared_platform_device(cellname, unique);
+	if (err)
+		printk(KERN_ERR "failed to add platform device %s\n", unique);
+
+printk(KERN_WARNING "registering platform driver %s\n", unique);
+
+	drv->driver.name = unique;
+	err = platform_driver_register(drv);
+	if (err)
+		kfree(unique);
+/* TODO: if cs5535-mfd is already loaded, need to create devices as well */
+
+	return err;
+}
+EXPORT_SYMBOL(mfd_shared_platform_driver_register);
+
+void mfd_shared_platform_driver_unregister(struct platform_driver *drv)
+{
+	struct device *dev;
+
+printk(KERN_WARNING "unregistering platform device %s\n", drv->driver.name);
+	dev = bus_find_device_by_name(&platform_bus_type, NULL,
+			drv->driver.name);
+	if (dev)
+		platform_device_unregister(to_platform_device(dev));
+
+printk(KERN_WARNING "unregistering platform driver %s\n", drv->driver.name);
+	platform_driver_unregister(drv);
+	kfree(drv->driver.name);
+	drv->driver.name = NULL;
+}
+EXPORT_SYMBOL(mfd_shared_platform_driver_unregister);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Ian Molton, Dmitry Baryshkov");
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index d13b992..ee1dd83 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -78,4 +78,13 @@ extern int mfd_add_devices(struct device *parent, int id,
 
 extern void mfd_remove_devices(struct device *parent);
 
+/*
+ * For MFD drivers with clients sharing access to resources, these create
+ * multiple platform devices per cell.  Contention handling must still be
+ * handled via drivers (ie, with enable/disable hooks).
+ */
+extern int mfd_shared_platform_driver_register(struct platform_driver *drv,
+		const char *cellname);
+extern void mfd_shared_platform_driver_unregister(struct platform_driver *drv);
+
 #endif
-- 
1.7.2.3

[PATCH 19/19] cs5535-mfd: add sharing for acpi/pms cells

[Andres Salomon]

This shows example usage by an mfd client.  Daniel's got the OLPC
patches that actually need the mfd sharing.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 arch/x86/platform/olpc/olpc-xo1.c |   40 ++++++++++++++++--------------------
 drivers/mfd/cs5535-mfd.c          |   39 ++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/arch/x86/platform/olpc/olpc-xo1.c b/arch/x86/platform/olpc/olpc-xo1.c
index 1277756..86da51f 100644
--- a/arch/x86/platform/olpc/olpc-xo1.c
+++ b/arch/x86/platform/olpc/olpc-xo1.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
+#include <linux/mfd/core.h>
 
 #include <asm/io.h>
 #include <asm/olpc.h>
@@ -56,25 +57,24 @@ static void xo1_power_off(void)
 static int __devinit olpc_xo1_probe(struct platform_device *pdev)
 {
 	struct resource *res;
+	int err;
 
 	/* don't run on non-XOs */
 	if (!machine_is_olpc())
 		return -ENODEV;
 
+	err = mfd_shared_cell_enable(pdev);
+	if (err)
+		return err;
+
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
 	if (!res) {
 		dev_err(&pdev->dev, "can't fetch device resource info\n");
 		return -EIO;
 	}
-
-	if (!request_region(res->start, resource_size(res), DRV_NAME)) {
-		dev_err(&pdev->dev, "can't request region\n");
-		return -EIO;
-	}
-
-	if (strcmp(pdev->name, "cs5535-pms") == 0)
+	if (strncmp(pdev->name, "cs5535-pms", strlen("cs5535-pms")) == 0)
 		pms_base = res->start;
-	else if (strcmp(pdev->name, "cs5535-acpi") == 0)
+	else if (strncmp(pdev->name, "cs5535-acpi", strlen("cs5535-acpi")) == 0)
 		acpi_base = res->start;
 
 	/* If we have both addresses, we can override the poweroff hook */
@@ -88,14 +88,11 @@ static int __devinit olpc_xo1_probe(struct platform_device *pdev)
 
 static int __devexit olpc_xo1_remove(struct platform_device *pdev)
 {
-	struct resource *r;
-
-	r = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	release_region(r->start, resource_size(r));
+	mfd_shared_cell_disable(pdev);
 
-	if (strcmp(pdev->name, "cs5535-pms") == 0)
+	if (strncmp(pdev->name, "cs5535-pms", strlen("cs5535-pms")) == 0)
 		pms_base = 0;
-	else if (strcmp(pdev->name, "cs5535-acpi") == 0)
+	else if (strncmp(pdev->name, "cs5535-acpi", strlen("cs5535-acpi")) == 0)
 		acpi_base = 0;
 
 	pm_power_off = NULL;
@@ -104,7 +101,6 @@ static int __devexit olpc_xo1_remove(struct platform_device *pdev)
 
 static struct platform_driver cs5535_pms_drv = {
 	.driver = {
-		.name = "cs5535-pms",
 		.owner = THIS_MODULE,
 	},
 	.probe = olpc_xo1_probe,
@@ -113,7 +109,6 @@ static struct platform_driver cs5535_pms_drv = {
 
 static struct platform_driver cs5535_acpi_drv = {
 	.driver = {
-		.name = "cs5535-acpi",
 		.owner = THIS_MODULE,
 	},
 	.probe = olpc_xo1_probe,
@@ -124,26 +119,27 @@ static int __init olpc_xo1_init(void)
 {
 	int r;
 
-	r = platform_driver_register(&cs5535_pms_drv);
+	r = mfd_shared_platform_driver_register(&cs5535_pms_drv, "cs5535-pms");
 	if (r)
 		return r;
 
-	r = platform_driver_register(&cs5535_acpi_drv);
+	r = mfd_shared_platform_driver_register(&cs5535_acpi_drv,
+			"cs5535-acpi");
 	if (r)
-		platform_driver_unregister(&cs5535_pms_drv);
+		mfd_shared_platform_driver_unregister(&cs5535_pms_drv);
 
 	return r;
 }
 
 static void __exit olpc_xo1_exit(void)
 {
-	platform_driver_unregister(&cs5535_acpi_drv);
-	platform_driver_unregister(&cs5535_pms_drv);
+	mfd_shared_platform_driver_unregister(&cs5535_acpi_drv);
+	mfd_shared_platform_driver_unregister(&cs5535_pms_drv);
 }
 
 MODULE_AUTHOR("Daniel Drake <dsd@laptop.org>");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:olpc-xo1");
+MODULE_ALIAS("platform:cs5535-pms");
 
 module_init(olpc_xo1_init);
 module_exit(olpc_xo1_exit);
diff --git a/drivers/mfd/cs5535-mfd.c b/drivers/mfd/cs5535-mfd.c
index 59ca6f1..5b08f16 100644
--- a/drivers/mfd/cs5535-mfd.c
+++ b/drivers/mfd/cs5535-mfd.c
@@ -39,6 +39,39 @@ enum cs5535_mfd_bars {
 	NR_BARS,
 };
 
+static int cs5535_mfd_res_enable(struct platform_device *pdev)
+{
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "can't fetch device resource info\n");
+		return -EIO;
+	}
+
+dev_warn(&pdev->dev, "%s(%s) called, requesting %pR\n", __func__, pdev->name, res);
+	if (!request_region(res->start, resource_size(res), DRV_NAME)) {
+		dev_err(&pdev->dev, "can't request region\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int cs5535_mfd_res_disable(struct platform_device *pdev)
+{
+	struct resource *res;
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "can't fetch device resource info\n");
+		return -EIO;
+	}
+
+dev_warn(&pdev->dev, "%s(%s) called, releasing %pR\n", __func__, pdev->name, res);
+	release_region(res->start, resource_size(res));
+	return 0;
+}
+
 static __devinitdata struct resource cs5535_mfd_resources[NR_BARS];
 
 static __devinitdata struct mfd_cell cs5535_mfd_cells[] = {
@@ -65,12 +98,18 @@ static __devinitdata struct mfd_cell cs5535_mfd_cells[] = {
 		.name = "cs5535-pms",
 		.num_resources = 1,
 		.resources = &cs5535_mfd_resources[PMS_BAR],
+
+		.enable = cs5535_mfd_res_enable,
+		.disable = cs5535_mfd_res_disable,
 	},
 	{
 		.id = ACPI_BAR,
 		.name = "cs5535-acpi",
 		.num_resources = 1,
 		.resources = &cs5535_mfd_resources[ACPI_BAR],
+
+		.enable = cs5535_mfd_res_enable,
+		.disable = cs5535_mfd_res_disable,
 	},
 };
 
-- 
1.7.2.3