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

[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-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
---
 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-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
> > ---
> >  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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-pFFUokh25LWsTnJN9+BGXg@public.gmane.org> wrote:
> On Fri, 1 Apr 2011 13:20:31 +0200
> Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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 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/

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-pFFUokh25LWsTnJN9+BGXg@public.gmane.org> wrote:
> > On Fri, 1 Apr 2011 13:20:31 +0200
> > Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-pFFUokh25LWsTnJN9+BGXg@public.gmane.org> wrote:
>> > On Fri, 1 Apr 2011 13:20:31 +0200
>> > Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> On Fri, Apr 1, 2011 at 5:52 PM, Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 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-pFFUokh25LWsTnJN9+BGXg@public.gmane.org> wrote:
> >> > On Fri, 1 Apr 2011 13:20:31 +0200
> >> > Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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

[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

[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

[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

[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

[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

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

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