[alsa-devel] [PATCH 12/19] twl4030: mfd_cell is now implicitly available to drivers
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");
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.
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..
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.
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.
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.
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.
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,
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.
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
participants (5)
-
Andres Salomon -
Dmitry Torokhov -
Mark Brown -
Peter Ujfalusi -
Uwe Kleine-König