LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Samuel Ortiz <sameo@linux.intel.com>
To: Grant Likely <grant.likely@secretlab.ca>,
	Andres Salomon <dilinger@queued.net>
Cc: linux-kernel@vger.kernel.org,
	Mark Brown <broonie@opensource.wolfsonmicro.com>,
	khali@linux-fr.org, ben-linux@fluff.org,
	Peter Korsgaard <jacmet@sunsite.dk>,
	Mauro Carvalho Chehab <mchehab@infradead.org>,
	David Brownell <dbrownell@users.sourceforge.net>,
	linux-i2c@vger.kernel.org, linux-media@vger.kernel.org,
	netdev@vger.kernel.org, spi-devel-general@lists.sourceforge.net,
	Mocean Laboratories <info@mocean-labs.com>
Subject: Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers
Date: Fri, 1 Apr 2011 13:20:31 +0200	[thread overview]
Message-ID: <20110401112030.GA3447@sortiz-mobl> (raw)
In-Reply-To: <20110331230522.GI437@ponder.secretlab.ca>

Hi Grant,

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


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

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

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

Patch for solution 1:


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

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


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

  reply	other threads:[~2011-04-01 11:20 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-03  3:54 [RFC] [PATCH 0/19] mfd sharing support Andres Salomon
2011-02-03  3:55 ` [PATCH 01/19] mfd-core: unconditionally add mfd_cell to every platform_device Andres Salomon
2011-02-03  3:58 ` [PATCH 02/19] jz4740: mfd_cell is now implicitly available to drivers Andres Salomon
2011-02-03  8:09   ` Jean Delvare
2011-02-03  4:01 ` [PATCH 03/19] ab3550: " Andres Salomon
2011-02-04  8:20   ` Mattias Wallin
2011-02-03  4:03 ` [PATCH 04/19] ab3100: " Andres Salomon
2011-02-03 12:52   ` Linus Walleij
2011-02-03  4:04 ` [PATCH 05/19] asic3: " Andres Salomon
2011-02-03  4:05 ` [PATCH 06/19] htc-pasic3: " Andres Salomon
2011-02-03  4:08 ` [PATCH 07/19] timberdale: " Andres Salomon
2011-03-31 23:05   ` Grant Likely
2011-04-01 11:20     ` Samuel Ortiz [this message]
2011-04-01 17:47       ` Andres Salomon
2011-04-01 17:56         ` Grant Likely
2011-04-01 18:00           ` Grant Likely
2011-04-01 23:52           ` Samuel Ortiz
2011-04-01 23:58             ` Grant Likely
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
2011-04-07  8:04                               ` Grant Likely
2011-04-06 18:47                           ` Samuel Ortiz
2011-04-06 18:59                             ` Felipe Balbi
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
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
2011-02-03  4:09 ` [PATCH 08/19] t7166xb: " Andres Salomon
2011-02-03  4:11 ` [PATCH 09/19] wl1273: " Andres Salomon
2011-02-03  4:12 ` [PATCH 10/19] sh_mobile_sdhi: " Andres Salomon
2011-02-03  4:13 ` [PATCH 11/19] tc6393xb: " Andres Salomon
2011-02-03  4:15 ` [PATCH 12/19] twl4030: " Andres Salomon
2011-02-03  6:05   ` Dmitry Torokhov
2011-02-03  6:39     ` Andres Salomon
2011-02-03  6:53       ` Dmitry Torokhov
2011-02-03  7:03         ` Andres Salomon
2011-02-03  9:31           ` Mark Brown
2011-02-05  2:39             ` Andres Salomon
2011-02-05  3:25               ` Andres Salomon
2011-02-03 12:23           ` Peter Ujfalusi
2011-02-04 10:41           ` Uwe Kleine-König
2011-02-03  4:16 ` [PATCH 13/19] tc6387xb: " Andres Salomon
2011-02-03  4:17 ` [PATCH 14/19] janz: " Andres Salomon
2011-02-03  4:20 ` [PATCH 15/19] mc13xxx: " Andres Salomon
2011-02-04  9:34   ` Uwe Kleine-König
2011-02-04 10:13     ` Uwe Kleine-König
2011-02-04 10:16     ` Andres Salomon
2011-02-03  4:21 ` [PATCH 16/19] mfd-core: drop platform_data/data_size from core mfd_cell struct Andres Salomon
2011-02-03  4:22 ` [PATCH 17/19] mfd-core: add refcounting support to mfd_cells Andres Salomon
2011-02-03  4:23 ` [PATCH 18/19] mfd-core: add platform_device sharing support for mfd Andres Salomon
2011-02-03  4:26 ` [PATCH 19/19] cs5535-mfd: add sharing for acpi/pms cells Andres Salomon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110401112030.GA3447@sortiz-mobl \
    --to=sameo@linux.intel.com \
    --cc=ben-linux@fluff.org \
    --cc=broonie@opensource.wolfsonmicro.com \
    --cc=dbrownell@users.sourceforge.net \
    --cc=dilinger@queued.net \
    --cc=grant.likely@secretlab.ca \
    --cc=info@mocean-labs.com \
    --cc=jacmet@sunsite.dk \
    --cc=khali@linux-fr.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@infradead.org \
    --cc=netdev@vger.kernel.org \
    --cc=spi-devel-general@lists.sourceforge.net \
    --subject='Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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