* [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers [not found] ` <20110202195417.228e2656-pFFUokh25LWsTnJN9+BGXg@public.gmane.org> @ 2011-02-03 4:08 ` Andres Salomon 2011-03-31 23:05 ` Grant Likely 0 siblings, 1 reply; 31+ messages in thread From: Andres Salomon @ 2011-02-03 4:08 UTC (permalink / raw) To: Samuel Ortiz Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, Grant Likely, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-02-03 4:08 ` [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers Andres Salomon @ 2011-03-31 23:05 ` Grant Likely [not found] ` <20110331230522.GI437-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 0 siblings, 1 reply; 31+ messages in thread From: Grant Likely @ 2011-03-31 23:05 UTC (permalink / raw) To: Andres Salomon Cc: Samuel Ortiz, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories 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 > ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <20110331230522.GI437-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org>]
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers [not found] ` <20110331230522.GI437-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org> @ 2011-04-01 11:20 ` Samuel Ortiz 2011-04-01 17:47 ` Andres Salomon 0 siblings, 1 reply; 31+ messages in thread From: Samuel Ortiz @ 2011-04-01 11:20 UTC (permalink / raw) To: Grant Likely, Andres Salomon Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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/ ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-01 11:20 ` Samuel Ortiz @ 2011-04-01 17:47 ` Andres Salomon 2011-04-01 17:56 ` Grant Likely 2011-04-01 18:26 ` Samuel Ortiz 0 siblings, 2 replies; 31+ messages in thread From: Andres Salomon @ 2011-04-01 17:47 UTC (permalink / raw) To: Samuel Ortiz Cc: Grant Likely, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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; ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-01 17:47 ` Andres Salomon @ 2011-04-01 17:56 ` Grant Likely 2011-04-01 18:00 ` Grant Likely [not found] ` <BANLkTi=bCd_+f=EG-O=U5VH_ZNjFhxkziQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-04-01 18:26 ` Samuel Ortiz 1 sibling, 2 replies; 31+ messages in thread From: Grant Likely @ 2011-04-01 17:56 UTC (permalink / raw) To: Andres Salomon Cc: Samuel Ortiz, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-01 17:56 ` Grant Likely @ 2011-04-01 18:00 ` Grant Likely [not found] ` <BANLkTi=bCd_+f=EG-O=U5VH_ZNjFhxkziQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 0 replies; 31+ messages in thread From: Grant Likely @ 2011-04-01 18:00 UTC (permalink / raw) To: Andres Salomon Cc: Samuel Ortiz, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <BANLkTi=bCd_+f=EG-O=U5VH_ZNjFhxkziQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers [not found] ` <BANLkTi=bCd_+f=EG-O=U5VH_ZNjFhxkziQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2011-04-01 23:52 ` Samuel Ortiz 2011-04-01 23:58 ` Grant Likely 0 siblings, 1 reply; 31+ messages in thread From: Samuel Ortiz @ 2011-04-01 23:52 UTC (permalink / raw) To: Grant Likely Cc: Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-01 23:52 ` Samuel Ortiz @ 2011-04-01 23:58 ` Grant Likely [not found] ` <BANLkTi=bq=OGzXFp7qiBr7x_BnGOWf=DRQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 31+ messages in thread From: Grant Likely @ 2011-04-01 23:58 UTC (permalink / raw) To: Samuel Ortiz Cc: Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories, Greg Kroah-Hartman 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <BANLkTi=bq=OGzXFp7qiBr7x_BnGOWf=DRQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers [not found] ` <BANLkTi=bq=OGzXFp7qiBr7x_BnGOWf=DRQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2011-04-02 0:10 ` Andres Salomon 2011-04-04 10:03 ` Samuel Ortiz 1 sibling, 0 replies; 31+ messages in thread From: Andres Salomon @ 2011-04-02 0:10 UTC (permalink / raw) To: Grant Likely Cc: Samuel Ortiz, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories, Greg Kroah-Hartman 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). ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers [not found] ` <BANLkTi=bq=OGzXFp7qiBr7x_BnGOWf=DRQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-04-02 0:10 ` Andres Salomon @ 2011-04-04 10:03 ` Samuel Ortiz 2011-04-05 3:04 ` Grant Likely 1 sibling, 1 reply; 31+ messages in thread From: Samuel Ortiz @ 2011-04-04 10:03 UTC (permalink / raw) To: Grant Likely Cc: Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories, Greg Kroah-Hartman 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/ ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-04 10:03 ` Samuel Ortiz @ 2011-04-05 3:04 ` Grant Likely 2011-04-06 15:23 ` Samuel Ortiz 2011-04-07 16:24 ` Grant Likely 0 siblings, 2 replies; 31+ messages in thread From: Grant Likely @ 2011-04-05 3:04 UTC (permalink / raw) To: Samuel Ortiz Cc: Andres Salomon, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories, Greg Kroah-Hartman 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/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-05 3:04 ` Grant Likely @ 2011-04-06 15:23 ` Samuel Ortiz 2011-04-06 15:58 ` Greg KH 2011-04-07 16:24 ` Grant Likely 1 sibling, 1 reply; 31+ messages in thread From: Samuel Ortiz @ 2011-04-06 15:23 UTC (permalink / raw) To: Grant Likely Cc: Andres Salomon, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories, Greg Kroah-Hartman 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/ ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 15:23 ` Samuel Ortiz @ 2011-04-06 15:58 ` Greg KH 2011-04-06 17:05 ` Samuel Ortiz 0 siblings, 1 reply; 31+ messages in thread From: Greg KH @ 2011-04-06 15:58 UTC (permalink / raw) To: Samuel Ortiz Cc: Grant Likely, Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 15:58 ` Greg KH @ 2011-04-06 17:05 ` Samuel Ortiz 2011-04-06 17:16 ` Ben Hutchings 2011-04-06 17:56 ` Greg KH 0 siblings, 2 replies; 31+ messages in thread From: Samuel Ortiz @ 2011-04-06 17:05 UTC (permalink / raw) To: Greg KH Cc: Grant Likely, Andres Salomon, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories 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/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 17:05 ` Samuel Ortiz @ 2011-04-06 17:16 ` Ben Hutchings 2011-04-06 17:51 ` Samuel Ortiz 2011-04-06 17:56 ` Greg KH 1 sibling, 1 reply; 31+ messages in thread From: Ben Hutchings @ 2011-04-06 17:16 UTC (permalink / raw) To: Samuel Ortiz Cc: Greg KH, Grant Likely, Andres Salomon, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 17:16 ` Ben Hutchings @ 2011-04-06 17:51 ` Samuel Ortiz 2011-04-06 18:07 ` Ben Hutchings 0 siblings, 1 reply; 31+ messages in thread From: Samuel Ortiz @ 2011-04-06 17:51 UTC (permalink / raw) To: Ben Hutchings Cc: Greg KH, Grant Likely, Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 17:51 ` Samuel Ortiz @ 2011-04-06 18:07 ` Ben Hutchings 0 siblings, 0 replies; 31+ messages in thread From: Ben Hutchings @ 2011-04-06 18:07 UTC (permalink / raw) To: Samuel Ortiz Cc: Greg KH, Grant Likely, Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 17:05 ` Samuel Ortiz 2011-04-06 17:16 ` Ben Hutchings @ 2011-04-06 17:56 ` Greg KH 2011-04-06 18:25 ` Andres Salomon 2011-04-06 18:47 ` Samuel Ortiz 1 sibling, 2 replies; 31+ messages in thread From: Greg KH @ 2011-04-06 17:56 UTC (permalink / raw) To: Samuel Ortiz Cc: Grant Likely, Andres Salomon, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 17:56 ` Greg KH @ 2011-04-06 18:25 ` Andres Salomon 2011-04-06 18:38 ` Greg KH 2011-04-06 18:47 ` Samuel Ortiz 1 sibling, 1 reply; 31+ messages in thread From: Andres Salomon @ 2011-04-06 18:25 UTC (permalink / raw) To: Greg KH Cc: Samuel Ortiz, Grant Likely, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 18:25 ` Andres Salomon @ 2011-04-06 18:38 ` Greg KH [not found] ` <20110406183854.GA10058-l3A5Bk7waGM@public.gmane.org> 0 siblings, 1 reply; 31+ messages in thread From: Greg KH @ 2011-04-06 18:38 UTC (permalink / raw) To: Andres Salomon Cc: Samuel Ortiz, Grant Likely, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <20110406183854.GA10058-l3A5Bk7waGM@public.gmane.org>]
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers [not found] ` <20110406183854.GA10058-l3A5Bk7waGM@public.gmane.org> @ 2011-04-07 8:04 ` Grant Likely 0 siblings, 0 replies; 31+ messages in thread From: Grant Likely @ 2011-04-07 8:04 UTC (permalink / raw) To: Greg KH Cc: Andres Salomon, Samuel Ortiz, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 17:56 ` Greg KH 2011-04-06 18:25 ` Andres Salomon @ 2011-04-06 18:47 ` Samuel Ortiz 2011-04-06 18:59 ` Felipe Balbi 1 sibling, 1 reply; 31+ messages in thread From: Samuel Ortiz @ 2011-04-06 18:47 UTC (permalink / raw) To: Greg KH Cc: Grant Likely, Andres Salomon, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories 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/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 18:47 ` Samuel Ortiz @ 2011-04-06 18:59 ` Felipe Balbi [not found] ` <20110406185902.GN25654-UiBtZHVXSwEVvW8u9ZQWYwjfymiNCTlR@public.gmane.org> 0 siblings, 1 reply; 31+ messages in thread From: Felipe Balbi @ 2011-04-06 18:59 UTC (permalink / raw) To: Samuel Ortiz Cc: Greg KH, Grant Likely, Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <20110406185902.GN25654-UiBtZHVXSwEVvW8u9ZQWYwjfymiNCTlR@public.gmane.org>]
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers [not found] ` <20110406185902.GN25654-UiBtZHVXSwEVvW8u9ZQWYwjfymiNCTlR@public.gmane.org> @ 2011-04-06 22:09 ` Greg KH 2011-04-07 8:09 ` Felipe Balbi 2011-04-07 13:40 ` Samuel Ortiz 1 sibling, 1 reply; 31+ messages in thread From: Greg KH @ 2011-04-06 22:09 UTC (permalink / raw) To: Felipe Balbi Cc: Samuel Ortiz, Grant Likely, Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-06 22:09 ` Greg KH @ 2011-04-07 8:09 ` Felipe Balbi 0 siblings, 0 replies; 31+ messages in thread From: Felipe Balbi @ 2011-04-07 8:09 UTC (permalink / raw) To: Greg KH Cc: Felipe Balbi, Samuel Ortiz, Grant Likely, Andres Salomon, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers [not found] ` <20110406185902.GN25654-UiBtZHVXSwEVvW8u9ZQWYwjfymiNCTlR@public.gmane.org> 2011-04-06 22:09 ` Greg KH @ 2011-04-07 13:40 ` Samuel Ortiz 2011-04-07 14:35 ` Grant Likely 1 sibling, 1 reply; 31+ messages in thread From: Samuel Ortiz @ 2011-04-07 13:40 UTC (permalink / raw) To: Felipe Balbi Cc: Greg KH, Grant Likely, Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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/ ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-07 13:40 ` Samuel Ortiz @ 2011-04-07 14:35 ` Grant Likely [not found] ` <20110407143515.GC26452-MrY2KI0G/OVr83L8+7iqerDks+cytr/Z@public.gmane.org> 0 siblings, 1 reply; 31+ messages in thread From: Grant Likely @ 2011-04-07 14:35 UTC (permalink / raw) To: Samuel Ortiz Cc: Felipe Balbi, Greg KH, Andres Salomon, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories 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/ ^ permalink raw reply [flat|nested] 31+ messages in thread
[parent not found: <20110407143515.GC26452-MrY2KI0G/OVr83L8+7iqerDks+cytr/Z@public.gmane.org>]
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers [not found] ` <20110407143515.GC26452-MrY2KI0G/OVr83L8+7iqerDks+cytr/Z@public.gmane.org> @ 2011-04-07 15:03 ` Samuel Ortiz 2011-04-07 18:06 ` Grant Likely 0 siblings, 1 reply; 31+ messages in thread From: Samuel Ortiz @ 2011-04-07 15:03 UTC (permalink / raw) To: Grant Likely Cc: Felipe Balbi, Greg KH, Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-07 15:03 ` Samuel Ortiz @ 2011-04-07 18:06 ` Grant Likely 0 siblings, 0 replies; 31+ messages in thread From: Grant Likely @ 2011-04-07 18:06 UTC (permalink / raw) To: Samuel Ortiz Cc: Felipe Balbi, Greg KH, Andres Salomon, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, khali-PUYAD+kWke1g9hUCZPvPmw, ben-linux-elnMNo+KYs3YtjvyW6yDsg, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-media-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Mocean Laboratories 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/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-05 3:04 ` Grant Likely 2011-04-06 15:23 ` Samuel Ortiz @ 2011-04-07 16:24 ` Grant Likely 1 sibling, 0 replies; 31+ messages in thread From: Grant Likely @ 2011-04-07 16:24 UTC (permalink / raw) To: Samuel Ortiz Cc: Andres Salomon, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories, Greg Kroah-Hartman 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers 2011-04-01 17:47 ` Andres Salomon 2011-04-01 17:56 ` Grant Likely @ 2011-04-01 18:26 ` Samuel Ortiz 1 sibling, 0 replies; 31+ messages in thread From: Samuel Ortiz @ 2011-04-01 18:26 UTC (permalink / raw) To: Andres Salomon Cc: Grant Likely, linux-kernel, Mark Brown, khali, ben-linux, Peter Korsgaard, Mauro Carvalho Chehab, David Brownell, linux-i2c, linux-media, netdev, spi-devel-general, Mocean Laboratories 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/ ^ permalink raw reply related [flat|nested] 31+ messages in thread
end of thread, other threads:[~2011-04-07 18:06 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20110202195417.228e2656@queued.net> [not found] ` <20110202195417.228e2656-pFFUokh25LWsTnJN9+BGXg@public.gmane.org> 2011-02-03 4:08 ` [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers Andres Salomon 2011-03-31 23:05 ` Grant Likely [not found] ` <20110331230522.GI437-e0URQFbLeQY2iJbIjFUEsiwD8/FfD2ys@public.gmane.org> 2011-04-01 11:20 ` Samuel Ortiz 2011-04-01 17:47 ` Andres Salomon 2011-04-01 17:56 ` Grant Likely 2011-04-01 18:00 ` Grant Likely [not found] ` <BANLkTi=bCd_+f=EG-O=U5VH_ZNjFhxkziQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-04-01 23:52 ` Samuel Ortiz 2011-04-01 23:58 ` Grant Likely [not found] ` <BANLkTi=bq=OGzXFp7qiBr7x_BnGOWf=DRQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2011-04-02 0:10 ` Andres Salomon 2011-04-04 10:03 ` Samuel Ortiz 2011-04-05 3:04 ` Grant Likely 2011-04-06 15:23 ` Samuel Ortiz 2011-04-06 15:58 ` Greg KH 2011-04-06 17:05 ` Samuel Ortiz 2011-04-06 17:16 ` Ben Hutchings 2011-04-06 17:51 ` Samuel Ortiz 2011-04-06 18:07 ` Ben Hutchings 2011-04-06 17:56 ` Greg KH 2011-04-06 18:25 ` Andres Salomon 2011-04-06 18:38 ` Greg KH [not found] ` <20110406183854.GA10058-l3A5Bk7waGM@public.gmane.org> 2011-04-07 8:04 ` Grant Likely 2011-04-06 18:47 ` Samuel Ortiz 2011-04-06 18:59 ` Felipe Balbi [not found] ` <20110406185902.GN25654-UiBtZHVXSwEVvW8u9ZQWYwjfymiNCTlR@public.gmane.org> 2011-04-06 22:09 ` Greg KH 2011-04-07 8:09 ` Felipe Balbi 2011-04-07 13:40 ` Samuel Ortiz 2011-04-07 14:35 ` Grant Likely [not found] ` <20110407143515.GC26452-MrY2KI0G/OVr83L8+7iqerDks+cytr/Z@public.gmane.org> 2011-04-07 15:03 ` Samuel Ortiz 2011-04-07 18:06 ` Grant Likely 2011-04-07 16:24 ` Grant Likely 2011-04-01 18:26 ` Samuel Ortiz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).