mfd sharing support

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

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

---
 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(-)

drivers/hwmon/jz4740-hwmon.c +1 -1

index 1c8b3d9..40f106d 100644
@@ -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) {

drivers/mfd/jz4740-adc.c +0 -4

index 0cc5979..aa518b9 100644
@@ -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,

drivers/power/jz4740-battery.c +0 -0

index 02414db..0938650 100644

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<[email protected]>

---
  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(-)

arch/arm/mach-u300/i2c.c +0 -2

index f0394ba..1ff7a82 100644
@@ -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),
  };

drivers/mfd/ab3550-core.c +0 -6

index 5fbca34..47625b9 100644
@@ -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);

include/linux/mfd/abx500.h +0 -2

index 67bd6f7..a735210 100644
@@ -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;
  };

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<[email protected]>

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 <[email protected]>

---
 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(-)

drivers/dma/timb_dma.c +0 -0

index 3b88a4e..aa06ca4 100644

drivers/dma/timb_dma.c +0 -0

 	struct timb_dma *td;
 	struct resource *iomem;
 	int irq;

drivers/gpio/timbgpio.c +2 -3

index 58c8f30..e404487 100644
@@ -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);
 		}

drivers/i2c/busses/i2c-ocores.c +1 -1

index ef3bcb1..dc203ec 100644
@@ -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;

drivers/i2c/busses/i2c-xiic.c +1 -1

index a9c419e..830b8c1 100644
@@ -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;
 

drivers/media/radio/radio-timb.c +1 -1

index a185610..e7baf26 100644
@@ -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;
 

drivers/media/video/timblogiw.c +1 -1

index fc611eb..61aa67a 100644
@@ -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");

drivers/mfd/timberdale.c +27 -54

index 6ad8a7f..e9ae162 100644
@@ -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,
 	},
 };
 

drivers/net/ks8842.c +1 -1

index 928b2b8..7f0f51f 100644
@@ -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;
 

drivers/spi/xilinx_spi.c +1 -1

index 7adaef6..2926dec 100644
@@ -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;

Signature · 1 line

1.7.2.3

Quoted 25 lines · 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 <[email protected]>

---
 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(-)

drivers/dma/timb_dma.c +0 -0

index 3b88a4e..aa06ca4 100644

drivers/dma/timb_dma.c static irqreturn_t td_irq(int irq, void *devid) · line 687

Quoted 5 lines

drivers/dma/timb_dma.c +1 -1

@@ -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.

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:

Quoted 6 lines

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(-)

drivers/mfd/mfd-core.c +10 -3

index d01574d..8abe510 100644
@@ -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;
+	}
 

drivers/mfd/mfd-core.c +0 -0

 	for (r = 0; r < cell->num_resources; r++) {
 		res[r].name = cell->resources[r].name;

Quoted 3 lines · On Fri, 1 Apr 2011 13:20:31 +0200 Samuel Ortiz…

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:

[...]

Quoted 6 lines

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 39 lines

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(-)

drivers/mfd/mfd-core.c +10 -3

index d01574d..8abe510 100644
@@ -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;

Quoted 13 lines · Andres Salomon

Quoted 3 lines · On Fri, 1 Apr 2011 13:20:31 +0200 Samuel Ortiz <[email protected]> wrote:

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:

[...]

Quoted 6 lines

Quoted 6 lines

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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 13 lines · Andres Salomon

Quoted 3 lines · On Fri, 1 Apr 2011 13:20:31 +0200 Samuel Ortiz…

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:

[...]

Quoted 6 lines

Quoted 6 lines

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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 25 lines · Grant Likely

Quoted 13 lines · Andres Salomon

Quoted 3 lines · On Fri, 1 Apr 2011 13:20:31 +0200 Samuel Ortiz…

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:

[...]

Quoted 6 lines

Quoted 6 lines

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.

Quoted 3 lines

Quoted 3 lines

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;
};

Quoted 8 lines

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.

Quoted 36 lines · Samuel Ortiz

Quoted 27 lines · Grant Likely

Quoted 14 lines · Andres Salomon

Quoted 3 lines · On Fri, 1 Apr 2011 13:20:31 +0200 Samuel Ortiz…

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:

[...]

Quoted 7 lines

Quoted 7 lines

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.

Quoted 3 lines

Quoted 3 lines

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;
};

Quoted 8 lines

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.

Quoted 6 lines

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?

Quoted 34 lines · Samuel Ortiz

Quoted 25 lines · Grant Likely

Quoted 13 lines · Andres Salomon

Quoted 3 lines · On Fri, 1 Apr 2011 13:20:31 +0200 Samuel Ortiz…

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:

[...]

Quoted 6 lines

Quoted 6 lines

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.

Quoted 3 lines

Quoted 3 lines

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;
};

Quoted 8 lines

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.

Quoted 6 lines

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?

Quoted 42 lines · Grant Likely

Quoted 34 lines · Samuel Ortiz

Quoted 25 lines · Grant Likely

Quoted 13 lines · Andres Salomon

Quoted 3 lines · On Fri, 1 Apr 2011 13:20:31 +0200 Samuel Ortiz <[email protected]> wrote:

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:

[...]

Quoted 6 lines

Quoted 6 lines

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.

Quoted 3 lines

Quoted 3 lines

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;
};

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

[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(-)

drivers/mfd/mfd-core.c +4 -0

index d01574d..c0fc1c0 100644
@@ -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;
+

drivers/mfd/mfd-core.c +5 -4

+
 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);

drivers/mfd/mfd-core.c +0 -0

 	platform_device_unregister(pdev);
 	return 0;
 }

include/linux/mfd/core.h +1 -1

index ad1b19a..0e4d3a6 100644
@@ -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;
 }
 
 /*

include/linux/mfd/core.h +0 -0

 }
 
 extern int mfd_add_devices(struct device *parent, int id,

include/linux/platform_device.h +2 -0

index d96db98..734d254 100644
@@ -14,6 +14,8 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 
+struct mfd_cell;
+
 struct platform_device {
 	const char	* name;
 	int		id;

include/linux/platform_device.h +0 -0

 	/* arch specific additions */
 	struct pdev_archdata	archdata;
 };

Signature · 3 lines

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

Quoted 4 lines

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.

include/linux/device.h +1 -0

@@ -33,6 +33,7 @@ struct class;
 struct subsys_private;
 struct bus_type;
 struct device_node;
+struct mfd_cell;
 
 struct bus_attribute {
 	struct attribute	attr;

Quoted 14 lines · Samuel Ortiz

include/linux/device.h +1 -0

@@ -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 */
 

include/linux/device.h struct device { · line 449

Quoted 2 lines

include/linux/device.h +2 -0

+	struct mfd_cell	*mfd_cell; /* MFD cell pointer */
+

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

Hi Greg,

Quoted 15 lines · Greg KH

Quoted 14 lines · Samuel Ortiz

include/linux/device.h +1 -0

@@ -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 */
 

Quoted 4 lines

include/linux/device.h struct device { · line 449

Quoted 2 lines

include/linux/device.h +2 -0

+	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.

Quoted 3 lines

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?

Hi Ben,

Quoted 5 lines · Ben Hutchings

Quoted 3 lines

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 ?

Hi Greg,

Quoted 15 lines · Greg KH

Quoted 14 lines · Samuel Ortiz

include/linux/device.h +1 -0

@@ -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 */
 

Quoted 4 lines

include/linux/device.h struct device { · line 449

Quoted 2 lines

include/linux/device.h +2 -0

+	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.

Quoted 23 lines · Samuel Ortiz

Hi Greg,

Quoted 9 lines · Greg KH

Quoted 8 lines · Samuel Ortiz

include/linux/device.h +1 -0

@@ -33,6 +33,7 @@ struct class;
 struct subsys_private;
 struct bus_type;
 struct device_node;
+struct mfd_cell;
 

Quoted 11 lines

Quoted 8 lines

include/linux/device.h +0 -0

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

include/linux/device.h +1 -0

+	struct mfd_cell	*mfd_cell; /* MFD cell pointer

*/ +

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

Quoted 7 lines

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?

Quoted 8 lines

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.

Quoted 4 lines

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

Quoted 8 lines

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 13 lines · Andres Salomon

Quoted 11 lines

Quoted 7 lines

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.

Quoted 1 line

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

Quoted 22 lines · Samuel Ortiz

Hi Greg,

Quoted 15 lines · Greg KH

Quoted 14 lines · Samuel Ortiz

include/linux/device.h +1 -0

@@ -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 */
 

Quoted 4 lines

include/linux/device.h struct device { · line 449

Quoted 2 lines

include/linux/device.h +2 -0

+	struct mfd_cell	*mfd_cell; /* MFD cell pointer */
+

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

Quoted 6 lines

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?

Quoted 3 lines

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.

Quoted 8 lines

Quoted 7 lines

Quoted 1 line

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.

Quoted 2 lines

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.

Hi,

Quoted 8 lines · Samuel Ortiz

Quoted 8 lines

Quoted 7 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 24 lines · Felipe Balbi

Hi,

Quoted 8 lines · Samuel Ortiz

Quoted 8 lines

Quoted 7 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Hi,

Quoted 8 lines · Samuel Ortiz

Quoted 8 lines

Quoted 7 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 2 lines

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.

Hi Felipe,

Quoted 17 lines · Felipe Balbi

Hi,

Quoted 8 lines · Samuel Ortiz

Quoted 8 lines

Quoted 7 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 6 lines

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(-)

drivers/mfd/timberdale.c +4 -4

index 94c6c8a..c9220ce 100644
@@ -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,

drivers/spi/xilinx_spi.c +18 -1

index c69c6f2..3287b84 100644
@@ -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)

include/linux/mfd/core.h +3 -0

index ad1b19a..13f31f4 100644
@@ -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.

Signature · 3 lines

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

Quoted 5 lines

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.

Quoted 109 lines

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

drivers/mfd/timberdale.c +4 -4

index 94c6c8a..c9220ce 100644
@@ -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,

drivers/spi/xilinx_spi.c +18 -1

index c69c6f2..3287b84 100644
@@ -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)

include/linux/mfd/core.h +3 -0

index ad1b19a..13f31f4 100644
@@ -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.

Signature · 3 lines

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

Quoted 6 lines · Grant Likely

Quoted 5 lines

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.

Quoted 5 lines

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 ?

Quoted 110 lines

Quoted 110 lines

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

drivers/mfd/timberdale.c +4 -4

index 94c6c8a..c9220ce 100644
@@ -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,

drivers/spi/xilinx_spi.c +18 -1

index c69c6f2..3287b84 100644
@@ -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)

include/linux/mfd/core.h +3 -0

index ad1b19a..13f31f4 100644
@@ -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.

Signature · 3 lines

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

Signature · 2 lines

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

Quoted 15 lines · Samuel Ortiz

include/linux/platform_device.h +2 -0

index d96db98..734d254 100644
@@ -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 {
 

include/linux/platform_device.h struct platform_device { · line 30

Quoted 5 lines

include/linux/platform_device.h +3 -0

       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.

Quoted 3 lines · On Fri, 1 Apr 2011 13:20:31 +0200 Samuel Ortiz <[email protected]> wrote:

Hi Grant,

On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:

[...]

Quoted 6 lines

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 6 lines · Andres Salomon

static int __devinit twl4030_vibra_probe(struct platform_device
*pdev) {

(quoted diff) +0 -1

-	struct twl4030_codec_vibra_data *pdata =

pdev->dev.platform_data;

(quoted diff) +1 -0

+	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.

Quoted 10 lines · On Wed, 2 Feb 2011 22:05:21 -0800 Dmitry Torokhov <[email protected]>…

Quoted 6 lines · Andres Salomon

static int __devinit twl4030_vibra_probe(struct platform_device
*pdev) {

(quoted diff) +0 -1

-	struct twl4030_codec_vibra_data *pdata =

pdev->dev.platform_data;

(quoted diff) +1 -0

+	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.

Quoted 6 lines

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

Quoted 29 lines · Andres Salomon

Quoted 10 lines · On Wed, 2 Feb 2011 22:05:21 -0800 Dmitry Torokhov <[email protected]>…

Quoted 6 lines · Andres Salomon

static int __devinit twl4030_vibra_probe(struct platform_device
*pdev) {

(quoted diff) +0 -1

-	struct twl4030_codec_vibra_data *pdata =

pdev->dev.platform_data;

(quoted diff) +1 -0

+	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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 5 lines · Andres Salomon

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 4 lines

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.

Quoted 7 lines · On Thu, 3 Feb 2011 09:31:54 +0000 Mark Brown…

Quoted 5 lines · Andres Salomon

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 6 lines

Quoted 4 lines

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.

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.

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.

Quoted 30 lines · On Wed, 2 Feb 2011 22:53:39 -0800 Dmitry Torokhov <[email protected]>…

Quoted 29 lines · Andres Salomon

Quoted 10 lines · On Wed, 2 Feb 2011 22:05:21 -0800 Dmitry Torokhov <[email protected]>…

Quoted 6 lines · Andres Salomon

static int __devinit twl4030_vibra_probe(struct platform_device
*pdev) {

(quoted diff) +0 -1

-	struct twl4030_codec_vibra_data *pdata =

pdev->dev.platform_data;

(quoted diff) +1 -0

+	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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 3 lines

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.