On Fri, Sep 16, 2011 at 04:43:18PM +0200, Benoit Cousson wrote:
> Split the omap_device_build_ss into two smaller functions
> that will allow to populate a platform_device already allocated by
> device-tree.
> The functionality of the omap_device_build_ss is still the same, but
> the omap_device_alloc will be usable with devices already built by
> device-tree.
>
> Signed-off-by: Benoit Cousson <b-cousson@xxxxxx>
> Cc: Kevin Hilman <khilman@xxxxxx>
Looks pretty good. Comments below.
g.
> ---
> arch/arm/plat-omap/omap_device.c | 177 +++++++++++++++++++++++++------------
> 1 files changed, 119 insertions(+), 58 deletions(-)
>
> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> index 54bbe7b..cac7b9a 100644
> --- a/arch/arm/plat-omap/omap_device.c
> +++ b/arch/arm/plat-omap/omap_device.c
> @@ -96,6 +96,11 @@
>
> static int omap_device_register(struct platform_device *pdev);
> static int omap_early_device_register(struct platform_device *pdev);
> +static struct omap_device *omap_device_alloc(struct platform_device *pdev,
> + struct omap_hwmod **ohs, int oh_cnt,
> + struct omap_device_pm_latency *pm_lats,
> + int pm_lats_cnt);
> +
>
> static struct omap_device_pm_latency omap_default_latency[] = {
> {
> @@ -397,6 +402,110 @@ static int omap_device_fill_resources(struct omap_device *od,
> }
>
> /**
> + * omap_device_alloc - allocate an omap_device
> + * @pdev: platform_device that will be included in this omap_device
> + * @oh: ptr to the single omap_hwmod that backs this omap_device
> + * @pdata: platform_data ptr to associate with the platform_device
> + * @pdata_len: amount of memory pointed to by @pdata
> + * @pm_lats: pointer to a omap_device_pm_latency array for this device
> + * @pm_lats_cnt: ARRAY_SIZE() of @pm_lats
> + *
> + * Convenience function for allocating an omap_device structure and filling
> + * hwmods, resources and pm_latency attributes.
> + *
> + * Returns an struct omap_device pointer or ERR_PTR() on error;
> + */
> +static struct omap_device *omap_device_alloc(struct platform_device *pdev,
> + struct omap_hwmod **ohs, int oh_cnt,
> + struct omap_device_pm_latency *pm_lats,
> + int pm_lats_cnt)
> +{
> + int ret = -ENOMEM;
> + struct omap_device *od;
> + struct resource *res = NULL;
> + int i, res_count;
> + struct omap_hwmod **hwmods;
> +
> + od = kzalloc(sizeof(struct omap_device), GFP_KERNEL);
possible enhancement: devm_kzalloc() perhaps? Would simplify the cleanup paths.
> + if (!od) {
> + ret = -ENOMEM;
> + goto oda_exit1;
> + }
> + od->hwmods_cnt = oh_cnt;
> +
> + hwmods = kmemdup(ohs, sizeof(struct omap_hwmod *) * oh_cnt, GFP_KERNEL);
Ditto here. would require creation of devm_kmemdup()
> + if (!hwmods)
> + goto oda_exit2;
> +
> + od->hwmods = hwmods;
> + od->pdev = pdev;
> +
> + /*
> + * HACK: Ideally the resources from DT should match, and hwmod
> + * should just add the missing ones. Since the name is not
> + * properly populated by DT, stick to hwmod resources only.
> + */
> + if (pdev->num_resources && pdev->resource)
> + dev_warn(&pdev->dev, "%s(): resources already allocated %d\n",
> + __func__, pdev->num_resources);
> +
> + res_count = omap_device_count_resources(od);
> + if (res_count > 0) {
> + dev_dbg(&pdev->dev, "%s(): resources allocated from hwmod %d\n",
> + __func__, res_count);
> + res = kzalloc(sizeof(struct resource) * res_count, GFP_KERNEL);
> + if (!res)
> + goto oda_exit3;
> +
> + omap_device_fill_resources(od, res);
> +
> + ret = platform_device_add_resources(pdev, res, res_count);
> + kfree(res);
How big is res_count? Struct resource isn't very big. It can
probably be on the stack.
> +
> + if (ret)
> + goto oda_exit3;
> + }
> +
> + if (!pm_lats) {
> + pm_lats = omap_default_latency;
> + pm_lats_cnt = ARRAY_SIZE(omap_default_latency);
> + }
> +
> + od->pm_lats_cnt = pm_lats_cnt;
> + od->pm_lats = kmemdup(pm_lats,
> + sizeof(struct omap_device_pm_latency) * pm_lats_cnt,
> + GFP_KERNEL);
> + if (!od->pm_lats)
> + goto oda_exit3;
> +
> + pdev->archdata.od = od;
> +
> + for (i = 0; i < oh_cnt; i++) {
> + hwmods[i]->od = od;
> + _add_hwmod_clocks_clkdev(od, hwmods[i]);
> + }
> +
> + return od;
> +
> +oda_exit3:
> + kfree(hwmods);
> +oda_exit2:
> + kfree(od);
> +oda_exit1:
> + dev_err(&pdev->dev, "omap_device: build failed (%d)\n", ret);
> +
> + return ERR_PTR(ret);
> +}
> +
> +static void omap_device_delete(struct omap_device *od)
> +{
> + od->pdev->archdata.od = NULL;
> + kfree(od->pm_lats);
> + kfree(od->hwmods);
> + kfree(od);
> +}
> +
> +/**
> * omap_device_build - build and register an omap_device with one omap_hwmod
> * @pdev_name: name of the platform_device driver to use
> * @pdev_id: this platform_device's connection ID
> @@ -455,9 +564,6 @@ struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
> int ret = -ENOMEM;
> struct platform_device *pdev;
> struct omap_device *od;
> - struct resource *res = NULL;
> - int i, res_count;
> - struct omap_hwmod **hwmods;
>
> if (!ohs || oh_cnt == 0 || !pdev_name)
> return ERR_PTR(-EINVAL);
> @@ -471,76 +577,31 @@ struct platform_device *omap_device_build_ss(const char *pdev_name, int pdev_id,
> goto odbs_exit;
> }
>
> - pr_debug("omap_device: %s: building with %d hwmods\n", pdev_name,
> - oh_cnt);
> + /* Set the dev_name early to allow dev_xxx in omap_device_alloc */
> + if (pdev->id != -1)
> + dev_set_name(&pdev->dev, "%s.%d", pdev->name, pdev->id);
> + else
> + dev_set_name(&pdev->dev, "%s", pdev->name);
This is duplicated from the core platform_device code. What is the
reasoning for doing it again here?
>
> - od = kzalloc(sizeof(struct omap_device), GFP_KERNEL);
> - if (!od) {
> - ret = -ENOMEM;
> + od = omap_device_alloc(pdev, ohs, oh_cnt, pm_lats, pm_lats_cnt);
> + if (!od)
> goto odbs_exit1;
> - }
> - od->hwmods_cnt = oh_cnt;
> -
> - hwmods = kzalloc(sizeof(struct omap_hwmod *) * oh_cnt,
> - GFP_KERNEL);
> - if (!hwmods)
> - goto odbs_exit2;
> -
> - memcpy(hwmods, ohs, sizeof(struct omap_hwmod *) * oh_cnt);
> - od->hwmods = hwmods;
> - od->pdev = pdev;
> -
> - res_count = omap_device_count_resources(od);
> - if (res_count > 0) {
> - res = kzalloc(sizeof(struct resource) * res_count, GFP_KERNEL);
> - if (!res)
> - goto odbs_exit3;
> -
> - omap_device_fill_resources(od, res);
> -
> - ret = platform_device_add_resources(pdev, res, res_count);
> - kfree(res);
> -
> - if (ret)
> - goto odbs_exit3;
> - }
>
> ret = platform_device_add_data(pdev, pdata, pdata_len);
> if (ret)
> - goto odbs_exit3;
> -
> - pdev->archdata.od = od;
> + goto odbs_exit2;
>
> if (is_early_device)
> ret = omap_early_device_register(pdev);
> else
> ret = omap_device_register(pdev);
> if (ret)
> - goto odbs_exit3;
> -
> - if (!pm_lats) {
> - pm_lats = omap_default_latency;
> - pm_lats_cnt = ARRAY_SIZE(omap_default_latency);
> - }
> -
> - od->pm_lats_cnt = pm_lats_cnt;
> - od->pm_lats = kmemdup(pm_lats,
> - sizeof(struct omap_device_pm_latency) * pm_lats_cnt,
> - GFP_KERNEL);
> - if (!od->pm_lats)
> - goto odbs_exit3;
> -
> - for (i = 0; i < oh_cnt; i++) {
> - hwmods[i]->od = od;
> - _add_hwmod_clocks_clkdev(od, hwmods[i]);
> - }
> + goto odbs_exit2;
>
> return pdev;
>
> -odbs_exit3:
> - kfree(hwmods);
> odbs_exit2:
> - kfree(od);
> + omap_device_delete(od);
> odbs_exit1:
> platform_device_put(pdev);
> odbs_exit:
> --
> 1.7.0.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
