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

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

[Andres Salomon]

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

In this case, move the various platform_data pointers
to driver_data.  All of the clients which make use of it
are also changed.

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

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

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

[Dmitry Torokhov]

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

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

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

-- 
Dmitry

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

[Andres Salomon]

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

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

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


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

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

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

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

[Dmitry Torokhov]

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

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

-- 
Dmitry

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

[Andres Salomon]

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

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

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

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

[Mark Brown]

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

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

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

That's the current best practice approach.

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

[Andres Salomon]

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

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

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

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

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

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

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

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

[Andres Salomon]

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

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

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



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

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

[Peter Ujfalusi]

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

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

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

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

-- 
Péter

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

[Uwe Kleine-König]

Hello Andres,

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

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

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

Uwe

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