[RFC/PATCH 7/7] WIP: HACK/RFC: omap_device: begin to decouple platform_device from omap_device
From: Grant Likely <hidden>
Date: 2011-07-22 02:20:37
Also in:
linux-devicetree, linux-omap
On Thu, Jul 21, 2011 at 5:52 PM, Kevin Hilman [off-list ref] wrote:
Rather than embedding a struct platform_device inside a struct omap_device, decouple them, leaving only a pointer to the platform_device inside the omap_device. This patch uses devres to allocate and attach the omap_device to the struct device, so finding an omap_device from a struct device means using devres_find(), and the to_omap_device() helper function was modified accordingly. RFC/Hack alert: Currently the driver core (drivers/base/dd.c) doesn't expect any devres resources to exist before the driver's ->probe() is called. ?In this patch, I just comment out the warning, but we'll need to understand why that limitation exists, and if it's a real limitation. A first glance suggests that it's not really needed. ?If this is a true limitation, we'll need to find some way other than devres to attach an omap_device to a struct device.
The devres lifetime is scoped to binding a driver; it is added at probe time and removed at release. For this use-case, it needs to be scoped to the lifetime of the struct device. To make this work, you'll need to add a flag to devres so that the driver core can differentiate between driver-scoped and device-scoped lifetimes (which I do think is the right thing to do). Without fixing this, the driver core will remove all the devres when a driver gets unbound, or if a .probe() hook fails, which completely breaks the driver model. g.
quoted hunk ↗ jump to hunk
On OMAP, we will need an omap_device attached to a struct device before probe because device HW may be disabled in probe and drivers are expected to use runtime PM in ->probe() to activate the hardware before access. ?Because the runtime PM API calls use omap_device (via our PM domain layer), we need omap_device attached to a platform_device before probe. --- ?arch/arm/mach-omap2/opp.c ? ? ? ? ? ? ? ? ? ? | ? ?2 +- ?arch/arm/plat-omap/include/plat/omap_device.h | ? ?4 +- ?arch/arm/plat-omap/omap_device.c ? ? ? ? ? ? ?| ? 78 +++++++++++++++---------- ?drivers/base/dd.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?2 +- ?4 files changed, 50 insertions(+), 36 deletions(-)diff --git a/arch/arm/mach-omap2/opp.c b/arch/arm/mach-omap2/opp.c index ab8b35b..9262a6b 100644 --- a/arch/arm/mach-omap2/opp.c +++ b/arch/arm/mach-omap2/opp.c@@ -69,7 +69,7 @@ int __init omap_init_opp_table(struct omap_opp_def *opp_def,? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?opp_def->hwmod_name, i); ? ? ? ? ? ? ? ? ? ? ? ?return -EINVAL; ? ? ? ? ? ? ? ?} - ? ? ? ? ? ? ? dev = &oh->od->pdev.dev; + ? ? ? ? ? ? ? dev = &oh->od->pdev->dev; ? ? ? ? ? ? ? ?r = opp_add(dev, opp_def->freq, opp_def->u_volt); ? ? ? ? ? ? ? ?if (r) {diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h index 7a3ec4f..6bd4f6f 100644 --- a/arch/arm/plat-omap/include/plat/omap_device.h +++ b/arch/arm/plat-omap/include/plat/omap_device.h@@ -64,7 +64,7 @@ extern struct device omap_device_parent;?* ?*/ ?struct omap_device { - ? ? ? struct platform_device ? ? ? ? ?pdev; + ? ? ? struct platform_device ? ? ? ? ?*pdev; ? ? ? ?struct omap_hwmod ? ? ? ? ? ? ? **hwmods; ? ? ? ?struct omap_device_pm_latency ? *pm_lats; ? ? ? ?u32 ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_wakeup_lat;@@ -142,6 +142,6 @@ struct omap_device_pm_latency {?#define OMAP_DEVICE_LATENCY_AUTO_ADJUST BIT(1) ?/* Get omap_device pointer from platform_device pointer */ -#define to_omap_device(x) container_of((x), struct omap_device, pdev) +struct omap_device *to_omap_device(struct platform_device *pdev); ?#endifdiff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c index c420b94..52ce013 100644 --- a/arch/arm/plat-omap/omap_device.c +++ b/arch/arm/plat-omap/omap_device.c@@ -117,7 +117,7 @@ static int _omap_device_activate(struct omap_device *od, u8 ignore_lat)?{ ? ? ? ?struct timespec a, b, c; - ? ? ? dev_dbg(&od->pdev.dev, "omap_device: activating\n"); + ? ? ? dev_dbg(&od->pdev->dev, "omap_device: activating\n"); ? ? ? ?while (od->pm_lat_level > 0) { ? ? ? ? ? ? ? ?struct omap_device_pm_latency *odpl;@@ -141,7 +141,7 @@ static int _omap_device_activate(struct omap_device *od, u8 ignore_lat)? ? ? ? ? ? ? ?c = timespec_sub(b, a); ? ? ? ? ? ? ? ?act_lat = timespec_to_ns(&c); - ? ? ? ? ? ? ? dev_dbg(&od->pdev.dev, + ? ? ? ? ? ? ? dev_dbg(&od->pdev->dev, ? ? ? ? ? ? ? ? ? ? ? ?"omap_device: pm_lat %d: activate: elapsed time " ? ? ? ? ? ? ? ? ? ? ? ?"%llu nsec\n", od->pm_lat_level, act_lat);@@ -149,12 +149,12 @@ static int _omap_device_activate(struct omap_device *od, u8 ignore_lat)? ? ? ? ? ? ? ? ? ? ? ?odpl->activate_lat_worst = act_lat; ? ? ? ? ? ? ? ? ? ? ? ?if (odpl->flags & OMAP_DEVICE_LATENCY_AUTO_ADJUST) { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?odpl->activate_lat = act_lat; - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_warn(&od->pdev.dev, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_warn(&od->pdev->dev, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "new worst case activate latency " ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "%d: %llu\n", ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? od->pm_lat_level, act_lat); ? ? ? ? ? ? ? ? ? ? ? ?} else - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_warn(&od->pdev.dev, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_warn(&od->pdev->dev, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "activate latency %d " ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "higher than exptected. (%llu > %d)\n", ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? od->pm_lat_level, act_lat,@@ -185,7 +185,7 @@ static int _omap_device_deactivate(struct omap_device *od, u8 ignore_lat)?{ ? ? ? ?struct timespec a, b, c; - ? ? ? dev_dbg(&od->pdev.dev, "omap_device: deactivating\n"); + ? ? ? dev_dbg(&od->pdev->dev, "omap_device: deactivating\n"); ? ? ? ?while (od->pm_lat_level < od->pm_lats_cnt) { ? ? ? ? ? ? ? ?struct omap_device_pm_latency *odpl;@@ -208,7 +208,7 @@ static int _omap_device_deactivate(struct omap_device *od, u8 ignore_lat)? ? ? ? ? ? ? ?c = timespec_sub(b, a); ? ? ? ? ? ? ? ?deact_lat = timespec_to_ns(&c); - ? ? ? ? ? ? ? dev_dbg(&od->pdev.dev, + ? ? ? ? ? ? ? dev_dbg(&od->pdev->dev, ? ? ? ? ? ? ? ? ? ? ? ?"omap_device: pm_lat %d: deactivate: elapsed time " ? ? ? ? ? ? ? ? ? ? ? ?"%llu nsec\n", od->pm_lat_level, deact_lat);@@ -216,12 +216,12 @@ static int _omap_device_deactivate(struct omap_device *od, u8 ignore_lat)? ? ? ? ? ? ? ? ? ? ? ?odpl->deactivate_lat_worst = deact_lat; ? ? ? ? ? ? ? ? ? ? ? ?if (odpl->flags & OMAP_DEVICE_LATENCY_AUTO_ADJUST) { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?odpl->deactivate_lat = deact_lat; - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_warn(&od->pdev.dev, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_warn(&od->pdev->dev, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "new worst case deactivate latency " ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "%d: %llu\n", ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? od->pm_lat_level, deact_lat); ? ? ? ? ? ? ? ? ? ? ? ?} else - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_warn(&od->pdev.dev, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_warn(&od->pdev->dev, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "deactivate latency %d " ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "higher than exptected. (%llu > %d)\n", ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? od->pm_lat_level, deact_lat,@@ -245,11 +245,11 @@ static void _add_clkdev(struct omap_device *od, const char *clk_alias,? ? ? ?if (!clk_alias || !clk_name) ? ? ? ? ? ? ? ?return; - ? ? ? dev_dbg(&od->pdev.dev, "Creating %s -> %s\n", clk_alias, clk_name); + ? ? ? dev_dbg(&od->pdev->dev, "Creating %s -> %s\n", clk_alias, clk_name); - ? ? ? r = clk_get_sys(dev_name(&od->pdev.dev), clk_alias); + ? ? ? r = clk_get_sys(dev_name(&od->pdev->dev), clk_alias); ? ? ? ?if (!IS_ERR(r)) { - ? ? ? ? ? ? ? dev_warn(&od->pdev.dev, + ? ? ? ? ? ? ? dev_warn(&od->pdev->dev, ? ? ? ? ? ? ? ? ? ? ? ? "alias %s already exists\n", clk_alias); ? ? ? ? ? ? ? ?clk_put(r); ? ? ? ? ? ? ? ?return;@@ -257,14 +257,14 @@ static void _add_clkdev(struct omap_device *od, const char *clk_alias,? ? ? ?r = omap_clk_get_by_name(clk_name); ? ? ? ?if (IS_ERR(r)) { - ? ? ? ? ? ? ? dev_err(&od->pdev.dev, + ? ? ? ? ? ? ? dev_err(&od->pdev->dev, ? ? ? ? ? ? ? ? ? ? ? ?"omap_clk_get_by_name for %s failed\n", clk_name); ? ? ? ? ? ? ? ?return; ? ? ? ?} - ? ? ? l = clkdev_alloc(r, clk_alias, dev_name(&od->pdev.dev)); + ? ? ? l = clkdev_alloc(r, clk_alias, dev_name(&od->pdev->dev)); ? ? ? ?if (!l) { - ? ? ? ? ? ? ? dev_err(&od->pdev.dev, + ? ? ? ? ? ? ? dev_err(&od->pdev->dev, ? ? ? ? ? ? ? ? ? ? ? ?"clkdev_alloc for %s failed\n", clk_alias); ? ? ? ? ? ? ? ?return; ? ? ? ?}@@ -351,7 +351,7 @@ static int omap_device_count_resources(struct omap_device *od)? ? ? ? ? ? ? ?c += omap_hwmod_count_resources(od->hwmods[i]); ? ? ? ?pr_debug("omap_device: %s: counted %d total resources across %d " - ? ? ? ? ? ? ? ?"hwmods\n", od->pdev.name, c, od->hwmods_cnt); + ? ? ? ? ? ? ? ?"hwmods\n", od->pdev->name, c, od->hwmods_cnt); ? ? ? ?return c; ?}@@ -388,6 +388,21 @@ static int omap_device_fill_resources(struct omap_device *od,? ? ? ?return 0; ?} +static void _od_dr_release(struct device *dev, void *res) +{ + ? ? ? kfree(res); +} + +struct omap_device *to_omap_device(struct platform_device *pdev) +{ + ? ? ? void *res; + + ? ? ? res = devres_find(&pdev->dev, _od_dr_release, NULL, NULL); + ? ? ? WARN_ON(!res); + + ? ? ? return (struct omap_device *)res; +} + ?/** ?* omap_device_build - build and register an omap_device with one omap_hwmod ?* @pdev_name: name of the platform_device driver to use@@ -445,8 +460,8 @@ struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int pm_lats_cnt, int is_early_device) ?{ ? ? ? ?int ret = -ENOMEM; + ? ? ? struct platform_device *pdev; ? ? ? ?struct omap_device *od; - ? ? ? char *pdev_name2; ? ? ? ?struct resource *res = NULL; ? ? ? ?int i, res_count; ? ? ? ?struct omap_hwmod **hwmods;@@ -460,7 +475,8 @@ struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,? ? ? ?pr_debug("omap_device: %s: building with %d hwmods\n", pdev_name, ? ? ? ? ? ? ? ? oh_cnt); - ? ? ? od = kzalloc(sizeof(struct omap_device), GFP_KERNEL); + ? ? ? od = devres_alloc(_od_dr_release, sizeof(struct omap_device), + ? ? ? ? ? ? ? ? ? ? ? ? GFP_KERNEL); ? ? ? ?if (!od) ? ? ? ? ? ? ? ?return ERR_PTR(-ENOMEM);@@ -474,13 +490,9 @@ struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,? ? ? ?memcpy(hwmods, ohs, sizeof(struct omap_hwmod *) * oh_cnt); ? ? ? ?od->hwmods = hwmods; - ? ? ? pdev_name2 = kzalloc(strlen(pdev_name) + 1, GFP_KERNEL); - ? ? ? if (!pdev_name2) + ? ? ? pdev = platform_device_alloc(pdev_name, pdev_id); + ? ? ? if (!pdev) ? ? ? ? ? ? ? ?goto odbs_exit2; - ? ? ? strcpy(pdev_name2, pdev_name); - - ? ? ? od->pdev.name = pdev_name2; - ? ? ? od->pdev.id = pdev_id; ? ? ? ?res_count = omap_device_count_resources(od); ? ? ? ?if (res_count > 0) {@@ -490,35 +502,37 @@ struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,? ? ? ?} ? ? ? ?omap_device_fill_resources(od, res); - ? ? ? od->pdev.num_resources = res_count; - ? ? ? od->pdev.resource = res; + ? ? ? pdev->num_resources = res_count; + ? ? ? pdev->resource = res; - ? ? ? ret = platform_device_add_data(&od->pdev, pdata, pdata_len); + ? ? ? ret = platform_device_add_data(pdev, pdata, pdata_len); ? ? ? ?if (ret) ? ? ? ? ? ? ? ?goto odbs_exit4; ? ? ? ?od->pm_lats = pm_lats; ? ? ? ?od->pm_lats_cnt = pm_lats_cnt; - ? ? ? if (is_early_device) - ? ? ? ? ? ? ? ret = omap_early_device_register(&od->pdev); - ? ? ? else - ? ? ? ? ? ? ? ret = omap_device_register(&od->pdev); + ? ? ? od->pdev = pdev; + ? ? ? devres_add(&pdev->dev, od); ? ? ? ?for (i = 0; i < oh_cnt; i++) { ? ? ? ? ? ? ? ?hwmods[i]->od = od; ? ? ? ? ? ? ? ?_add_hwmod_clocks_clkdev(od, hwmods[i]); ? ? ? ?} + ? ? ? if (is_early_device) + ? ? ? ? ? ? ? ret = omap_early_device_register(pdev); + ? ? ? else + ? ? ? ? ? ? ? ret = omap_device_register(pdev); + ? ? ? ?if (ret) ? ? ? ? ? ? ? ?goto odbs_exit4; - ? ? ? return &od->pdev; + ? ? ? return pdev; ?odbs_exit4: ? ? ? ?kfree(res); ?odbs_exit3: - ? ? ? kfree(pdev_name2); ?odbs_exit2: ? ? ? ?kfree(hwmods); ?odbs_exit1:diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 6658da7..9289dac 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c@@ -112,7 +112,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)? ? ? ?atomic_inc(&probe_count); ? ? ? ?pr_debug("bus: '%s': %s: probing driver %s with device %s\n", ? ? ? ? ? ? ? ? drv->bus->name, __func__, drv->name, dev_name(dev)); - ? ? ? WARN_ON(!list_empty(&dev->devres_head)); + ? ? ? /* WARN_ON(!list_empty(&dev->devres_head)); */ ? ? ? ?dev->driver = drv; ? ? ? ?if (driver_sysfs_add(dev)) { -- 1.7.6
-- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.