[PATCH v2 0/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[PATCH v2 0/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[Benoit Cousson]

Hi Kevin,

This is the updated version of the initial series that introduced a
notifier in order to create an omap_device from a platform_device bound
to DT node as suggested by Grant.

For the moment, the informations are all extracted from the hwmod data.
The idea is to focus first on the devices / board static init removal.
The other issue is that some bindings, like dma, are still not present
in the DT core code. The reg and irq bindings are there, but cannot
be used by some drivers due to the lack of named resources in DT for
the moment.
We agreed with Grant about the strategy to introduce the name without
breaking the compatibility and the default assumption about the order.

I'll update that in another series after 3.2 and then these informations
will be moved from hwmod to DT.

Patches are based on my for_3.2/1_omap_device_cleanup branch and are
available here:
git://gitorious.org/omap-pm/linux.git for_3.2/2_omap_device_dt

It is tested on OMAP4 SDP, Panda and Beagle-xM with and without CONFIG_OF.

Regards,
Benoit


Changes since v1: http://www.spinics.net/lists/linux-omap/msg55814.html
 - delete omap_device structure during BUS_NOTIFY_DEL_DEVICE callback
   as suggested by Kevin
 - merge previous patches 2&3 as suggested by Kevin
 - delete pm_lats in omap_device_delete since this is now kmalloc-ed
 - use kmemdup instead of kzalloc + memcopy for hmwods creation
 - Fix the wrong usage of of_*.h includes to build properly without
   CONFIG_OF.


Benoit Cousson (2):
  OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration
  OMAP: omap_device: Add a method to build an omap_device from a DT node


 arch/arm/plat-omap/omap_device.c |  321 +++++++++++++++++++++++++++++++-------
 1 files changed, 263 insertions(+), 58 deletions(-)

[PATCH v2 0/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[Benoit Cousson]

Hi Kevin,

This is the updated version of the initial series that introduced a
notifier in order to create an omap_device from a platform_device bound
to DT node as suggested by Grant.

For the moment, the informations are all extracted from the hwmod data.
The idea is to focus first on the devices / board static init removal.
The other issue is that some bindings, like dma, are still not present
in the DT core code. The reg and irq bindings are there, but cannot
be used by some drivers due to the lack of named resources in DT for
the moment.
We agreed with Grant about the strategy to introduce the name without
breaking the compatibility and the default assumption about the order.

I'll update that in another series after 3.2 and then these informations
will be moved from hwmod to DT.

Patches are based on my for_3.2/1_omap_device_cleanup branch and are
available here:
git://gitorious.org/omap-pm/linux.git for_3.2/2_omap_device_dt

It is tested on OMAP4 SDP, Panda and Beagle-xM with and without CONFIG_OF.

Regards,
Benoit


Changes since v1: http://www.spinics.net/lists/linux-omap/msg55814.html
 - delete omap_device structure during BUS_NOTIFY_DEL_DEVICE callback
   as suggested by Kevin
 - merge previous patches 2&3 as suggested by Kevin
 - delete pm_lats in omap_device_delete since this is now kmalloc-ed
 - use kmemdup instead of kzalloc + memcopy for hmwods creation
 - Fix the wrong usage of of_*.h includes to build properly without
   CONFIG_OF.


Benoit Cousson (2):
  OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration
  OMAP: omap_device: Add a method to build an omap_device from a DT node


 arch/arm/plat-omap/omap_device.c |  321 +++++++++++++++++++++++++++++++-------
 1 files changed, 263 insertions(+), 58 deletions(-)

[PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Benoit Cousson]

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@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
---
 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);
+	if (!od) {
+		ret = -ENOMEM;
+		goto oda_exit1;
+	}
+	od->hwmods_cnt = oh_cnt;
+
+	hwmods = kmemdup(ohs, sizeof(struct omap_hwmod *) * oh_cnt, GFP_KERNEL);
+	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);
+
+		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);
 
-	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

[PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Benoit Cousson]

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@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
---
 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);
+	if (!od) {
+		ret = -ENOMEM;
+		goto oda_exit1;
+	}
+	od->hwmods_cnt = oh_cnt;
+
+	hwmods = kmemdup(ohs, sizeof(struct omap_hwmod *) * oh_cnt, GFP_KERNEL);
+	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);
+
+		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);
 
-	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

[PATCH v2 2/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[Benoit Cousson]

Add a notifier called during device_add phase. If an of_node is present,
retrieve the hwmod entry in order to populate properly the omap_device
structure.
For the moment the resource from the device-tree are overloaded.
DT does not support named resource yet, and thus, most driver
will not work without that information.

Add two helpers function to parse a property that contains multiple
strings.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
---
 arch/arm/plat-omap/omap_device.c |  144 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 144 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index cac7b9a..da13630 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -85,6 +85,8 @@
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 #include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/notifier.h>
 
 #include <plat/omap_device.h>
 #include <plat/omap_hwmod.h>
@@ -94,12 +96,15 @@
 #define USE_WAKEUP_LAT			0
 #define IGNORE_WAKEUP_LAT		1
 
+#define MAX_HWMOD_NAME_SIZE		32
+
 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 void omap_device_delete(struct omap_device *od);
 
 
 static struct omap_device_pm_latency omap_default_latency[] = {
@@ -315,6 +320,138 @@ static void _add_hwmod_clocks_clkdev(struct omap_device *od,
 		_add_clkdev(od, oh->opt_clks[i].role, oh->opt_clks[i].clk);
 }
 
+/*
+ * XXX: DT helper functions that should probably move elsewhere if
+ * they become usefull for other needs.
+ */
+static int _dt_count_property_string(const char *prop, int len)
+{
+	int i = 0;
+	size_t l = 0, total = 0;
+
+	if (!prop || !len)
+		return -EINVAL;
+
+	for (i = 0; len >= total; total += l, prop += l) {
+		l = strlen(prop) + 1;
+		if (*prop != 0)
+			i++;
+	}
+	return i;
+}
+
+static int _dt_get_property(const char *prop, int len, int index, char *output,
+			    int size)
+{
+	int i = 0;
+	size_t l = 0, total = 0;
+
+	if (!prop || !len)
+		return -EINVAL;
+
+	for (i = 0; len >= total; total += l, prop += l) {
+		l = strlcpy(output, prop, size) + 1;
+		if (*prop != 0) {
+			if (i++ == index)
+				return 0;
+		}
+	}
+	return -ENODEV;
+}
+
+static struct dev_pm_domain omap_device_pm_domain;
+
+/**
+ * omap_device_build_from_dt - build an omap_device with multiple hwmods
+ * @pdev_name: name of the platform_device driver to use
+ * @pdev_id: this platform_device's connection ID
+ * @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
+ * @is_early_device: should the device be registered as an early device or not
+ *
+ * Function for building an omap_device already registered from device-tree
+ *
+ * Returns 0 or PTR_ERR() on error.
+ */
+static int omap_device_build_from_dt(struct platform_device *pdev)
+{
+	struct omap_hwmod **hwmods;
+	struct omap_device *od;
+	struct omap_hwmod *oh;
+	char oh_name[MAX_HWMOD_NAME_SIZE];
+	const char *prop;
+	int oh_cnt, i, prop_len;
+	int ret = 0;
+
+	prop = of_get_property(pdev->dev.of_node, "hwmods", &prop_len);
+	oh_cnt = _dt_count_property_string(prop, prop_len);
+	if (!oh_cnt || IS_ERR_VALUE(oh_cnt)) {
+		dev_warn(&pdev->dev, "No 'hwmods' to build omap_device\n");
+		return -ENODEV;
+	}
+
+	hwmods = kzalloc(sizeof(struct omap_hwmod *) * oh_cnt, GFP_KERNEL);
+	if (!hwmods) {
+		ret = -ENOMEM;
+		goto odbfd_exit;
+	}
+
+	for (i = 0; i < oh_cnt; i++) {
+		_dt_get_property(prop, prop_len, i, oh_name,
+				 MAX_HWMOD_NAME_SIZE);
+
+		oh = omap_hwmod_lookup(oh_name);
+		if (!oh) {
+			dev_err(&pdev->dev, "Cannot lookup hwmod '%s'\n",
+				oh_name);
+			ret = -EINVAL;
+			goto odbfd_exit1;
+		}
+		hwmods[i] = oh;
+	}
+
+	od = omap_device_alloc(pdev, hwmods, oh_cnt, NULL, 0);
+	if (!od) {
+		dev_err(&pdev->dev, "Cannot allocate omap_device for :%s\n",
+			oh_name);
+		ret = PTR_ERR(od);
+		goto odbfd_exit1;
+	}
+
+	if (of_get_property(pdev->dev.of_node, "no_idle_on_suspend", NULL))
+		omap_device_disable_idle_on_suspend(pdev);
+
+	pdev->dev.pm_domain = &omap_device_pm_domain;
+
+odbfd_exit1:
+	kfree(hwmods);
+odbfd_exit:
+	return ret;
+}
+
+static int _omap_device_notifier_call(struct notifier_block *nb,
+				      unsigned long event, void *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		if (pdev->dev.of_node)
+			omap_device_build_from_dt(pdev);
+		break;
+
+	case BUS_NOTIFY_DEL_DEVICE:
+		if (pdev->archdata.od)
+			omap_device_delete(pdev->archdata.od);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
 
 /* Public functions for use by core code */
 
@@ -499,6 +636,9 @@ oda_exit1:
 
 static void omap_device_delete(struct omap_device *od)
 {
+	if (!od)
+		return;
+
 	od->pdev->archdata.od = NULL;
 	kfree(od->pm_lats);
 	kfree(od->hwmods);
@@ -1034,8 +1174,12 @@ struct device omap_device_parent = {
 	.parent         = &platform_bus,
 };
 
+static struct notifier_block platform_nb;
+
 static int __init omap_device_init(void)
 {
+	platform_nb.notifier_call = _omap_device_notifier_call;
+	bus_register_notifier(&platform_bus_type, &platform_nb);
 	return device_register(&omap_device_parent);
 }
 core_initcall(omap_device_init);
-- 
1.7.0.4

[PATCH v2 2/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[Benoit Cousson]

Add a notifier called during device_add phase. If an of_node is present,
retrieve the hwmod entry in order to populate properly the omap_device
structure.
For the moment the resource from the device-tree are overloaded.
DT does not support named resource yet, and thus, most driver
will not work without that information.

Add two helpers function to parse a property that contains multiple
strings.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
---
 arch/arm/plat-omap/omap_device.c |  144 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 144 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index cac7b9a..da13630 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -85,6 +85,8 @@
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 #include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/notifier.h>
 
 #include <plat/omap_device.h>
 #include <plat/omap_hwmod.h>
@@ -94,12 +96,15 @@
 #define USE_WAKEUP_LAT			0
 #define IGNORE_WAKEUP_LAT		1
 
+#define MAX_HWMOD_NAME_SIZE		32
+
 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 void omap_device_delete(struct omap_device *od);
 
 
 static struct omap_device_pm_latency omap_default_latency[] = {
@@ -315,6 +320,138 @@ static void _add_hwmod_clocks_clkdev(struct omap_device *od,
 		_add_clkdev(od, oh->opt_clks[i].role, oh->opt_clks[i].clk);
 }
 
+/*
+ * XXX: DT helper functions that should probably move elsewhere if
+ * they become usefull for other needs.
+ */
+static int _dt_count_property_string(const char *prop, int len)
+{
+	int i = 0;
+	size_t l = 0, total = 0;
+
+	if (!prop || !len)
+		return -EINVAL;
+
+	for (i = 0; len >= total; total += l, prop += l) {
+		l = strlen(prop) + 1;
+		if (*prop != 0)
+			i++;
+	}
+	return i;
+}
+
+static int _dt_get_property(const char *prop, int len, int index, char *output,
+			    int size)
+{
+	int i = 0;
+	size_t l = 0, total = 0;
+
+	if (!prop || !len)
+		return -EINVAL;
+
+	for (i = 0; len >= total; total += l, prop += l) {
+		l = strlcpy(output, prop, size) + 1;
+		if (*prop != 0) {
+			if (i++ == index)
+				return 0;
+		}
+	}
+	return -ENODEV;
+}
+
+static struct dev_pm_domain omap_device_pm_domain;
+
+/**
+ * omap_device_build_from_dt - build an omap_device with multiple hwmods
+ * @pdev_name: name of the platform_device driver to use
+ * @pdev_id: this platform_device's connection ID
+ * @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
+ * @is_early_device: should the device be registered as an early device or not
+ *
+ * Function for building an omap_device already registered from device-tree
+ *
+ * Returns 0 or PTR_ERR() on error.
+ */
+static int omap_device_build_from_dt(struct platform_device *pdev)
+{
+	struct omap_hwmod **hwmods;
+	struct omap_device *od;
+	struct omap_hwmod *oh;
+	char oh_name[MAX_HWMOD_NAME_SIZE];
+	const char *prop;
+	int oh_cnt, i, prop_len;
+	int ret = 0;
+
+	prop = of_get_property(pdev->dev.of_node, "hwmods", &prop_len);
+	oh_cnt = _dt_count_property_string(prop, prop_len);
+	if (!oh_cnt || IS_ERR_VALUE(oh_cnt)) {
+		dev_warn(&pdev->dev, "No 'hwmods' to build omap_device\n");
+		return -ENODEV;
+	}
+
+	hwmods = kzalloc(sizeof(struct omap_hwmod *) * oh_cnt, GFP_KERNEL);
+	if (!hwmods) {
+		ret = -ENOMEM;
+		goto odbfd_exit;
+	}
+
+	for (i = 0; i < oh_cnt; i++) {
+		_dt_get_property(prop, prop_len, i, oh_name,
+				 MAX_HWMOD_NAME_SIZE);
+
+		oh = omap_hwmod_lookup(oh_name);
+		if (!oh) {
+			dev_err(&pdev->dev, "Cannot lookup hwmod '%s'\n",
+				oh_name);
+			ret = -EINVAL;
+			goto odbfd_exit1;
+		}
+		hwmods[i] = oh;
+	}
+
+	od = omap_device_alloc(pdev, hwmods, oh_cnt, NULL, 0);
+	if (!od) {
+		dev_err(&pdev->dev, "Cannot allocate omap_device for :%s\n",
+			oh_name);
+		ret = PTR_ERR(od);
+		goto odbfd_exit1;
+	}
+
+	if (of_get_property(pdev->dev.of_node, "no_idle_on_suspend", NULL))
+		omap_device_disable_idle_on_suspend(pdev);
+
+	pdev->dev.pm_domain = &omap_device_pm_domain;
+
+odbfd_exit1:
+	kfree(hwmods);
+odbfd_exit:
+	return ret;
+}
+
+static int _omap_device_notifier_call(struct notifier_block *nb,
+				      unsigned long event, void *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	switch (event) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		if (pdev->dev.of_node)
+			omap_device_build_from_dt(pdev);
+		break;
+
+	case BUS_NOTIFY_DEL_DEVICE:
+		if (pdev->archdata.od)
+			omap_device_delete(pdev->archdata.od);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
 
 /* Public functions for use by core code */
 
@@ -499,6 +636,9 @@ oda_exit1:
 
 static void omap_device_delete(struct omap_device *od)
 {
+	if (!od)
+		return;
+
 	od->pdev->archdata.od = NULL;
 	kfree(od->pm_lats);
 	kfree(od->hwmods);
@@ -1034,8 +1174,12 @@ struct device omap_device_parent = {
 	.parent         = &platform_bus,
 };
 
+static struct notifier_block platform_nb;
+
 static int __init omap_device_init(void)
 {
+	platform_nb.notifier_call = _omap_device_notifier_call;
+	bus_register_notifier(&platform_bus_type, &platform_nb);
 	return device_register(&omap_device_parent);
 }
 core_initcall(omap_device_init);
-- 
1.7.0.4

Re: [PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Grant Likely]

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@ti.com>
> Cc: Kevin Hilman <khilman@ti.com>

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
> 

[PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Grant Likely]

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@ti.com>
> Cc: Kevin Hilman <khilman@ti.com>

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
> 

Re: [PATCH v2 2/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[Grant Likely]

On Fri, Sep 16, 2011 at 04:43:19PM +0200, Benoit Cousson wrote:
> Add a notifier called during device_add phase. If an of_node is present,
> retrieve the hwmod entry in order to populate properly the omap_device
> structure.
> For the moment the resource from the device-tree are overloaded.
> DT does not support named resource yet, and thus, most driver
> will not work without that information.
> 
> Add two helpers function to parse a property that contains multiple
> strings.
> 
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Kevin Hilman <khilman@ti.com>
> ---
>  arch/arm/plat-omap/omap_device.c |  144 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 144 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> index cac7b9a..da13630 100644
> --- a/arch/arm/plat-omap/omap_device.c
> +++ b/arch/arm/plat-omap/omap_device.c
> @@ -85,6 +85,8 @@
>  #include <linux/clk.h>
>  #include <linux/clkdev.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/of.h>
> +#include <linux/notifier.h>
>  
>  #include <plat/omap_device.h>
>  #include <plat/omap_hwmod.h>
> @@ -94,12 +96,15 @@
>  #define USE_WAKEUP_LAT			0
>  #define IGNORE_WAKEUP_LAT		1
>  
> +#define MAX_HWMOD_NAME_SIZE		32
> +
>  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 void omap_device_delete(struct omap_device *od);
>  
>  
>  static struct omap_device_pm_latency omap_default_latency[] = {
> @@ -315,6 +320,138 @@ static void _add_hwmod_clocks_clkdev(struct omap_device *od,
>  		_add_clkdev(od, oh->opt_clks[i].role, oh->opt_clks[i].clk);
>  }
>  
> +/*
> + * XXX: DT helper functions that should probably move elsewhere if
> + * they become usefull for other needs.
> + */

Lets just *assume* these are useful for other needs and start with
them in drivers/of so that other people know that they actually exist.
Otherwise they will never be made generic. :-)

> +static int _dt_count_property_string(const char *prop, int len)
> +{
> +	int i = 0;
> +	size_t l = 0, total = 0;
> +
> +	if (!prop || !len)
> +		return -EINVAL;
> +
> +	for (i = 0; len >= total; total += l, prop += l) {
> +		l = strlen(prop) + 1;
> +		if (*prop != 0)
> +			i++;
> +	}
> +	return i;
> +}
> +
> +static int _dt_get_property(const char *prop, int len, int index, char *output,
> +			    int size)
> +{
> +	int i = 0;
> +	size_t l = 0, total = 0;
> +
> +	if (!prop || !len)
> +		return -EINVAL;
> +
> +	for (i = 0; len >= total; total += l, prop += l) {
> +		l = strlcpy(output, prop, size) + 1;
> +		if (*prop != 0) {
> +			if (i++ == index)
> +				return 0;
> +		}
> +	}
> +	return -ENODEV;
> +}
> +
> +static struct dev_pm_domain omap_device_pm_domain;
> +
> +/**
> + * omap_device_build_from_dt - build an omap_device with multiple hwmods
> + * @pdev_name: name of the platform_device driver to use
> + * @pdev_id: this platform_device's connection ID
> + * @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
> + * @is_early_device: should the device be registered as an early device or not
> + *
> + * Function for building an omap_device already registered from device-tree
> + *
> + * Returns 0 or PTR_ERR() on error.
> + */
> +static int omap_device_build_from_dt(struct platform_device *pdev)
> +{
> +	struct omap_hwmod **hwmods;
> +	struct omap_device *od;
> +	struct omap_hwmod *oh;
> +	char oh_name[MAX_HWMOD_NAME_SIZE];
> +	const char *prop;
> +	int oh_cnt, i, prop_len;
> +	int ret = 0;
> +
> +	prop = of_get_property(pdev->dev.of_node, "hwmods", &prop_len);

I know you've posted it before, but what is the status of the "hwmods"
binding documentation.  I would expect it to be part of this patch.

> +	oh_cnt = _dt_count_property_string(prop, prop_len);
> +	if (!oh_cnt || IS_ERR_VALUE(oh_cnt)) {
> +		dev_warn(&pdev->dev, "No 'hwmods' to build omap_device\n");
> +		return -ENODEV;
> +	}
> +
> +	hwmods = kzalloc(sizeof(struct omap_hwmod *) * oh_cnt, GFP_KERNEL);
> +	if (!hwmods) {
> +		ret = -ENOMEM;
> +		goto odbfd_exit;
> +	}
> +
> +	for (i = 0; i < oh_cnt; i++) {
> +		_dt_get_property(prop, prop_len, i, oh_name,
> +				 MAX_HWMOD_NAME_SIZE);
> +
> +		oh = omap_hwmod_lookup(oh_name);
> +		if (!oh) {
> +			dev_err(&pdev->dev, "Cannot lookup hwmod '%s'\n",
> +				oh_name);
> +			ret = -EINVAL;
> +			goto odbfd_exit1;
> +		}
> +		hwmods[i] = oh;
> +	}
> +
> +	od = omap_device_alloc(pdev, hwmods, oh_cnt, NULL, 0);
> +	if (!od) {
> +		dev_err(&pdev->dev, "Cannot allocate omap_device for :%s\n",
> +			oh_name);
> +		ret = PTR_ERR(od);
> +		goto odbfd_exit1;
> +	}
> +
> +	if (of_get_property(pdev->dev.of_node, "no_idle_on_suspend", NULL))
> +		omap_device_disable_idle_on_suspend(pdev);
> +
> +	pdev->dev.pm_domain = &omap_device_pm_domain;
> +
> +odbfd_exit1:
> +	kfree(hwmods);
> +odbfd_exit:
> +	return ret;
> +}
> +
> +static int _omap_device_notifier_call(struct notifier_block *nb,
> +				      unsigned long event, void *dev)

Nit: Why the preceding underscore?  Generally that is only done for
'special' variants of public functions.  ie. for a variant that
expects a lock to already be held.

> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +
> +	switch (event) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		if (pdev->dev.of_node)
> +			omap_device_build_from_dt(pdev);
> +		break;
> +
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		if (pdev->archdata.od)
> +			omap_device_delete(pdev->archdata.od);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
>  
>  /* Public functions for use by core code */
>  
> @@ -499,6 +636,9 @@ oda_exit1:
>  
>  static void omap_device_delete(struct omap_device *od)
>  {
> +	if (!od)
> +		return;
> +
>  	od->pdev->archdata.od = NULL;
>  	kfree(od->pm_lats);
>  	kfree(od->hwmods);
> @@ -1034,8 +1174,12 @@ struct device omap_device_parent = {
>  	.parent         = &platform_bus,
>  };
>  
> +static struct notifier_block platform_nb;

Nit: Since it is static initialization, you can do the following
instead of setting it up in the init function.

static struct notifier_block platform_nb = {
	.notifier_call = _omap_device_notifier_call;
};

> +
>  static int __init omap_device_init(void)
>  {
> +	platform_nb.notifier_call = _omap_device_notifier_call;
> +	bus_register_notifier(&platform_bus_type, &platform_nb);
>  	return device_register(&omap_device_parent);
>  }
>  core_initcall(omap_device_init);
> -- 
> 1.7.0.4
> 

[PATCH v2 2/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[Grant Likely]

On Fri, Sep 16, 2011 at 04:43:19PM +0200, Benoit Cousson wrote:
> Add a notifier called during device_add phase. If an of_node is present,
> retrieve the hwmod entry in order to populate properly the omap_device
> structure.
> For the moment the resource from the device-tree are overloaded.
> DT does not support named resource yet, and thus, most driver
> will not work without that information.
> 
> Add two helpers function to parse a property that contains multiple
> strings.
> 
> Signed-off-by: Benoit Cousson <b-cousson@ti.com>
> Cc: Kevin Hilman <khilman@ti.com>
> ---
>  arch/arm/plat-omap/omap_device.c |  144 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 144 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> index cac7b9a..da13630 100644
> --- a/arch/arm/plat-omap/omap_device.c
> +++ b/arch/arm/plat-omap/omap_device.c
> @@ -85,6 +85,8 @@
>  #include <linux/clk.h>
>  #include <linux/clkdev.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/of.h>
> +#include <linux/notifier.h>
>  
>  #include <plat/omap_device.h>
>  #include <plat/omap_hwmod.h>
> @@ -94,12 +96,15 @@
>  #define USE_WAKEUP_LAT			0
>  #define IGNORE_WAKEUP_LAT		1
>  
> +#define MAX_HWMOD_NAME_SIZE		32
> +
>  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 void omap_device_delete(struct omap_device *od);
>  
>  
>  static struct omap_device_pm_latency omap_default_latency[] = {
> @@ -315,6 +320,138 @@ static void _add_hwmod_clocks_clkdev(struct omap_device *od,
>  		_add_clkdev(od, oh->opt_clks[i].role, oh->opt_clks[i].clk);
>  }
>  
> +/*
> + * XXX: DT helper functions that should probably move elsewhere if
> + * they become usefull for other needs.
> + */

Lets just *assume* these are useful for other needs and start with
them in drivers/of so that other people know that they actually exist.
Otherwise they will never be made generic. :-)

> +static int _dt_count_property_string(const char *prop, int len)
> +{
> +	int i = 0;
> +	size_t l = 0, total = 0;
> +
> +	if (!prop || !len)
> +		return -EINVAL;
> +
> +	for (i = 0; len >= total; total += l, prop += l) {
> +		l = strlen(prop) + 1;
> +		if (*prop != 0)
> +			i++;
> +	}
> +	return i;
> +}
> +
> +static int _dt_get_property(const char *prop, int len, int index, char *output,
> +			    int size)
> +{
> +	int i = 0;
> +	size_t l = 0, total = 0;
> +
> +	if (!prop || !len)
> +		return -EINVAL;
> +
> +	for (i = 0; len >= total; total += l, prop += l) {
> +		l = strlcpy(output, prop, size) + 1;
> +		if (*prop != 0) {
> +			if (i++ == index)
> +				return 0;
> +		}
> +	}
> +	return -ENODEV;
> +}
> +
> +static struct dev_pm_domain omap_device_pm_domain;
> +
> +/**
> + * omap_device_build_from_dt - build an omap_device with multiple hwmods
> + * @pdev_name: name of the platform_device driver to use
> + * @pdev_id: this platform_device's connection ID
> + * @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
> + * @is_early_device: should the device be registered as an early device or not
> + *
> + * Function for building an omap_device already registered from device-tree
> + *
> + * Returns 0 or PTR_ERR() on error.
> + */
> +static int omap_device_build_from_dt(struct platform_device *pdev)
> +{
> +	struct omap_hwmod **hwmods;
> +	struct omap_device *od;
> +	struct omap_hwmod *oh;
> +	char oh_name[MAX_HWMOD_NAME_SIZE];
> +	const char *prop;
> +	int oh_cnt, i, prop_len;
> +	int ret = 0;
> +
> +	prop = of_get_property(pdev->dev.of_node, "hwmods", &prop_len);

I know you've posted it before, but what is the status of the "hwmods"
binding documentation.  I would expect it to be part of this patch.

> +	oh_cnt = _dt_count_property_string(prop, prop_len);
> +	if (!oh_cnt || IS_ERR_VALUE(oh_cnt)) {
> +		dev_warn(&pdev->dev, "No 'hwmods' to build omap_device\n");
> +		return -ENODEV;
> +	}
> +
> +	hwmods = kzalloc(sizeof(struct omap_hwmod *) * oh_cnt, GFP_KERNEL);
> +	if (!hwmods) {
> +		ret = -ENOMEM;
> +		goto odbfd_exit;
> +	}
> +
> +	for (i = 0; i < oh_cnt; i++) {
> +		_dt_get_property(prop, prop_len, i, oh_name,
> +				 MAX_HWMOD_NAME_SIZE);
> +
> +		oh = omap_hwmod_lookup(oh_name);
> +		if (!oh) {
> +			dev_err(&pdev->dev, "Cannot lookup hwmod '%s'\n",
> +				oh_name);
> +			ret = -EINVAL;
> +			goto odbfd_exit1;
> +		}
> +		hwmods[i] = oh;
> +	}
> +
> +	od = omap_device_alloc(pdev, hwmods, oh_cnt, NULL, 0);
> +	if (!od) {
> +		dev_err(&pdev->dev, "Cannot allocate omap_device for :%s\n",
> +			oh_name);
> +		ret = PTR_ERR(od);
> +		goto odbfd_exit1;
> +	}
> +
> +	if (of_get_property(pdev->dev.of_node, "no_idle_on_suspend", NULL))
> +		omap_device_disable_idle_on_suspend(pdev);
> +
> +	pdev->dev.pm_domain = &omap_device_pm_domain;
> +
> +odbfd_exit1:
> +	kfree(hwmods);
> +odbfd_exit:
> +	return ret;
> +}
> +
> +static int _omap_device_notifier_call(struct notifier_block *nb,
> +				      unsigned long event, void *dev)

Nit: Why the preceding underscore?  Generally that is only done for
'special' variants of public functions.  ie. for a variant that
expects a lock to already be held.

> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +
> +	switch (event) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		if (pdev->dev.of_node)
> +			omap_device_build_from_dt(pdev);
> +		break;
> +
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		if (pdev->archdata.od)
> +			omap_device_delete(pdev->archdata.od);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
>  
>  /* Public functions for use by core code */
>  
> @@ -499,6 +636,9 @@ oda_exit1:
>  
>  static void omap_device_delete(struct omap_device *od)
>  {
> +	if (!od)
> +		return;
> +
>  	od->pdev->archdata.od = NULL;
>  	kfree(od->pm_lats);
>  	kfree(od->hwmods);
> @@ -1034,8 +1174,12 @@ struct device omap_device_parent = {
>  	.parent         = &platform_bus,
>  };
>  
> +static struct notifier_block platform_nb;

Nit: Since it is static initialization, you can do the following
instead of setting it up in the init function.

static struct notifier_block platform_nb = {
	.notifier_call = _omap_device_notifier_call;
};

> +
>  static int __init omap_device_init(void)
>  {
> +	platform_nb.notifier_call = _omap_device_notifier_call;
> +	bus_register_notifier(&platform_bus_type, &platform_nb);
>  	return device_register(&omap_device_parent);
>  }
>  core_initcall(omap_device_init);
> -- 
> 1.7.0.4
> 

Re: [PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Russell King - ARM Linux]

On Sat, Sep 17, 2011 at 10:05:14AM -0600, Grant Likely wrote:
> > +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.

Are you sure about that - have you thought about the lifetime issues?
devm_*() lifetime is supposed to be from driver binding to driver
unbinding.

The lifetime of 'omap_device' appears to be from device creation to
device destruction, which is different from what devm_*() gives you.

So, using devm_*() will result in the 'omap_device' being destroyed
when a driver is unbound - and presumably an oops if its attempted to
be re-bound later (because the 'omap_device' structure will have been
freed.)

[PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Russell King - ARM Linux]

On Sat, Sep 17, 2011 at 10:05:14AM -0600, Grant Likely wrote:
> > +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.

Are you sure about that - have you thought about the lifetime issues?
devm_*() lifetime is supposed to be from driver binding to driver
unbinding.

The lifetime of 'omap_device' appears to be from device creation to
device destruction, which is different from what devm_*() gives you.

So, using devm_*() will result in the 'omap_device' being destroyed
when a driver is unbound - and presumably an oops if its attempted to
be re-bound later (because the 'omap_device' structure will have been
freed.)

Re: [PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Grant Likely]

On Sat, Sep 17, 2011 at 05:27:01PM +0100, Russell King - ARM Linux wrote:
> On Sat, Sep 17, 2011 at 10:05:14AM -0600, Grant Likely wrote:
> > > +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.
> 
> Are you sure about that - have you thought about the lifetime issues?
> devm_*() lifetime is supposed to be from driver binding to driver
> unbinding.
> 
> The lifetime of 'omap_device' appears to be from device creation to
> device destruction, which is different from what devm_*() gives you.
> 
> So, using devm_*() will result in the 'omap_device' being destroyed
> when a driver is unbound - and presumably an oops if its attempted to
> be re-bound later (because the 'omap_device' structure will have been
> freed.)

Ugh, yes.  You're absolutely right.  devm_*() is doesn't at all work here.

g.

[PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Grant Likely]

On Sat, Sep 17, 2011 at 05:27:01PM +0100, Russell King - ARM Linux wrote:
> On Sat, Sep 17, 2011 at 10:05:14AM -0600, Grant Likely wrote:
> > > +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.
> 
> Are you sure about that - have you thought about the lifetime issues?
> devm_*() lifetime is supposed to be from driver binding to driver
> unbinding.
> 
> The lifetime of 'omap_device' appears to be from device creation to
> device destruction, which is different from what devm_*() gives you.
> 
> So, using devm_*() will result in the 'omap_device' being destroyed
> when a driver is unbound - and presumably an oops if its attempted to
> be re-bound later (because the 'omap_device' structure will have been
> freed.)

Ugh, yes.  You're absolutely right.  devm_*() is doesn't at all work here.

g.

Re: [PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Cousson, Benoit]

On 9/17/2011 6:05 PM, Grant Likely wrote:
> 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@ti.com>
>> Cc: Kevin Hilman<khilman@ti.com>
>
> 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.

It will depend of the number of entries in the hwmod DB that can vary 
from one address space to 10 addresses + a bunch of irqs + a bunch of 
dmas. To be honest, that code is just a cut&paste of the original one.
But since that number can vary based on another file content and 
considering that this code is supposed to disappear as soon as the 
resource will come from DT, I'm not 100% convince it worth the effort.

>> +
>> +		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?

Well, it is written in the comment... But this is maybe not that obvious :-)
That part is only needed for the legacy path that will create a 
omap_device before having created the device, and thus at that time the 
dev_xxx will not give the device name. This is not a big deal, but that 
was painful for the debug.

That being said, by writing that, I'm now realizing that this is due to 
the way the legacy code was working, because I didn't try to change the 
sequence.
But maybe, I can easily avoid that by changing the original sequence.
In fact If I create the omap_device after the omap_device_register, the 
platform_device will already have the correct name...

That should work, I'll give it a try.

Thanks for the comments,
Benoit

[PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Cousson, Benoit]

On 9/17/2011 6:05 PM, Grant Likely wrote:
> 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@ti.com>
>> Cc: Kevin Hilman<khilman@ti.com>
>
> 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.

It will depend of the number of entries in the hwmod DB that can vary 
from one address space to 10 addresses + a bunch of irqs + a bunch of 
dmas. To be honest, that code is just a cut&paste of the original one.
But since that number can vary based on another file content and 
considering that this code is supposed to disappear as soon as the 
resource will come from DT, I'm not 100% convince it worth the effort.

>> +
>> +		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?

Well, it is written in the comment... But this is maybe not that obvious :-)
That part is only needed for the legacy path that will create a 
omap_device before having created the device, and thus at that time the 
dev_xxx will not give the device name. This is not a big deal, but that 
was painful for the debug.

That being said, by writing that, I'm now realizing that this is due to 
the way the legacy code was working, because I didn't try to change the 
sequence.
But maybe, I can easily avoid that by changing the original sequence.
In fact If I create the omap_device after the omap_device_register, the 
platform_device will already have the correct name...

That should work, I'll give it a try.

Thanks for the comments,
Benoit

Re: [PATCH v2 2/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[Cousson, Benoit]

On 9/17/2011 6:13 PM, Grant Likely wrote:
> On Fri, Sep 16, 2011 at 04:43:19PM +0200, Benoit Cousson wrote:
>> Add a notifier called during device_add phase. If an of_node is present,
>> retrieve the hwmod entry in order to populate properly the omap_device
>> structure.
>> For the moment the resource from the device-tree are overloaded.
>> DT does not support named resource yet, and thus, most driver
>> will not work without that information.
>>
>> Add two helpers function to parse a property that contains multiple
>> strings.
>>
>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>> Cc: Kevin Hilman<khilman@ti.com>
>> ---
>>   arch/arm/plat-omap/omap_device.c |  144 ++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 144 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>> index cac7b9a..da13630 100644
>> --- a/arch/arm/plat-omap/omap_device.c
>> +++ b/arch/arm/plat-omap/omap_device.c
>> @@ -85,6 +85,8 @@
>>   #include<linux/clk.h>
>>   #include<linux/clkdev.h>
>>   #include<linux/pm_runtime.h>
>> +#include<linux/of.h>
>> +#include<linux/notifier.h>
>>
>>   #include<plat/omap_device.h>
>>   #include<plat/omap_hwmod.h>
>> @@ -94,12 +96,15 @@
>>   #define USE_WAKEUP_LAT			0
>>   #define IGNORE_WAKEUP_LAT		1
>>
>> +#define MAX_HWMOD_NAME_SIZE		32
>> +
>>   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 void omap_device_delete(struct omap_device *od);
>>
>>
>>   static struct omap_device_pm_latency omap_default_latency[] = {
>> @@ -315,6 +320,138 @@ static void _add_hwmod_clocks_clkdev(struct omap_device *od,
>>   		_add_clkdev(od, oh->opt_clks[i].role, oh->opt_clks[i].clk);
>>   }
>>
>> +/*
>> + * XXX: DT helper functions that should probably move elsewhere if
>> + * they become usefull for other needs.
>> + */
>
> Lets just *assume* these are useful for other needs and start with
> them in drivers/of so that other people know that they actually exist.
> Otherwise they will never be made generic. :-)

Good point...

But, meanwhile Stephen Warren proposed a much nicer iterator to deal 
with that. Since these patches are still not in mainline, I didn't want 
to depend on them yet. So my "secret" plan was to remove these functions 
once the ones from Stephen will be merged.

>> +static int _dt_count_property_string(const char *prop, int len)
>> +{
>> +	int i = 0;
>> +	size_t l = 0, total = 0;
>> +
>> +	if (!prop || !len)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; len>= total; total += l, prop += l) {
>> +		l = strlen(prop) + 1;
>> +		if (*prop != 0)
>> +			i++;
>> +	}
>> +	return i;
>> +}
>> +
>> +static int _dt_get_property(const char *prop, int len, int index, char *output,
>> +			    int size)
>> +{
>> +	int i = 0;
>> +	size_t l = 0, total = 0;
>> +
>> +	if (!prop || !len)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; len>= total; total += l, prop += l) {
>> +		l = strlcpy(output, prop, size) + 1;
>> +		if (*prop != 0) {
>> +			if (i++ == index)
>> +				return 0;
>> +		}
>> +	}
>> +	return -ENODEV;
>> +}
>> +
>> +static struct dev_pm_domain omap_device_pm_domain;
>> +
>> +/**
>> + * omap_device_build_from_dt - build an omap_device with multiple hwmods
>> + * @pdev_name: name of the platform_device driver to use
>> + * @pdev_id: this platform_device's connection ID
>> + * @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
>> + * @is_early_device: should the device be registered as an early device or not
>> + *
>> + * Function for building an omap_device already registered from device-tree
>> + *
>> + * Returns 0 or PTR_ERR() on error.
>> + */
>> +static int omap_device_build_from_dt(struct platform_device *pdev)
>> +{
>> +	struct omap_hwmod **hwmods;
>> +	struct omap_device *od;
>> +	struct omap_hwmod *oh;
>> +	char oh_name[MAX_HWMOD_NAME_SIZE];
>> +	const char *prop;
>> +	int oh_cnt, i, prop_len;
>> +	int ret = 0;
>> +
>> +	prop = of_get_property(pdev->dev.of_node, "hwmods",&prop_len);
>
> I know you've posted it before, but what is the status of the "hwmods"
> binding documentation.  I would expect it to be part of this patch.

It was part of the series that introduced the first node with hwmods 
attribute 
(http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-August/007621.html).
I do agree, it makes sense to add it here.

>> +	oh_cnt = _dt_count_property_string(prop, prop_len);
>> +	if (!oh_cnt || IS_ERR_VALUE(oh_cnt)) {
>> +		dev_warn(&pdev->dev, "No 'hwmods' to build omap_device\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	hwmods = kzalloc(sizeof(struct omap_hwmod *) * oh_cnt, GFP_KERNEL);
>> +	if (!hwmods) {
>> +		ret = -ENOMEM;
>> +		goto odbfd_exit;
>> +	}
>> +
>> +	for (i = 0; i<  oh_cnt; i++) {
>> +		_dt_get_property(prop, prop_len, i, oh_name,
>> +				 MAX_HWMOD_NAME_SIZE);
>> +
>> +		oh = omap_hwmod_lookup(oh_name);
>> +		if (!oh) {
>> +			dev_err(&pdev->dev, "Cannot lookup hwmod '%s'\n",
>> +				oh_name);
>> +			ret = -EINVAL;
>> +			goto odbfd_exit1;
>> +		}
>> +		hwmods[i] = oh;
>> +	}
>> +
>> +	od = omap_device_alloc(pdev, hwmods, oh_cnt, NULL, 0);
>> +	if (!od) {
>> +		dev_err(&pdev->dev, "Cannot allocate omap_device for :%s\n",
>> +			oh_name);
>> +		ret = PTR_ERR(od);
>> +		goto odbfd_exit1;
>> +	}
>> +
>> +	if (of_get_property(pdev->dev.of_node, "no_idle_on_suspend", NULL))
>> +		omap_device_disable_idle_on_suspend(pdev);
>> +
>> +	pdev->dev.pm_domain =&omap_device_pm_domain;
>> +
>> +odbfd_exit1:
>> +	kfree(hwmods);
>> +odbfd_exit:
>> +	return ret;
>> +}
>> +
>> +static int _omap_device_notifier_call(struct notifier_block *nb,
>> +				      unsigned long event, void *dev)
>
> Nit: Why the preceding underscore?  Generally that is only done for
> 'special' variants of public functions.  ie. for a variant that
> expects a lock to already be held.

Yeah, the convention in this file is not that strict, and it is used for 
internal static helper function as well.
I'll let Kevin arbitrate that point :-)

>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +
>> +	switch (event) {
>> +	case BUS_NOTIFY_ADD_DEVICE:
>> +		if (pdev->dev.of_node)
>> +			omap_device_build_from_dt(pdev);
>> +		break;
>> +
>> +	case BUS_NOTIFY_DEL_DEVICE:
>> +		if (pdev->archdata.od)
>> +			omap_device_delete(pdev->archdata.od);
>> +		break;
>> +	}
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
>>
>>   /* Public functions for use by core code */
>>
>> @@ -499,6 +636,9 @@ oda_exit1:
>>
>>   static void omap_device_delete(struct omap_device *od)
>>   {
>> +	if (!od)
>> +		return;
>> +
>>   	od->pdev->archdata.od = NULL;
>>   	kfree(od->pm_lats);
>>   	kfree(od->hwmods);
>> @@ -1034,8 +1174,12 @@ struct device omap_device_parent = {
>>   	.parent         =&platform_bus,
>>   };
>>
>> +static struct notifier_block platform_nb;
>
> Nit: Since it is static initialization, you can do the following
> instead of setting it up in the init function.
>
> static struct notifier_block platform_nb = {
> 	.notifier_call = _omap_device_notifier_call;
> };

Much better, indeed.

Thanks,
Benoit

[PATCH v2 2/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[Cousson, Benoit]

On 9/17/2011 6:13 PM, Grant Likely wrote:
> On Fri, Sep 16, 2011 at 04:43:19PM +0200, Benoit Cousson wrote:
>> Add a notifier called during device_add phase. If an of_node is present,
>> retrieve the hwmod entry in order to populate properly the omap_device
>> structure.
>> For the moment the resource from the device-tree are overloaded.
>> DT does not support named resource yet, and thus, most driver
>> will not work without that information.
>>
>> Add two helpers function to parse a property that contains multiple
>> strings.
>>
>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>> Cc: Kevin Hilman<khilman@ti.com>
>> ---
>>   arch/arm/plat-omap/omap_device.c |  144 ++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 144 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>> index cac7b9a..da13630 100644
>> --- a/arch/arm/plat-omap/omap_device.c
>> +++ b/arch/arm/plat-omap/omap_device.c
>> @@ -85,6 +85,8 @@
>>   #include<linux/clk.h>
>>   #include<linux/clkdev.h>
>>   #include<linux/pm_runtime.h>
>> +#include<linux/of.h>
>> +#include<linux/notifier.h>
>>
>>   #include<plat/omap_device.h>
>>   #include<plat/omap_hwmod.h>
>> @@ -94,12 +96,15 @@
>>   #define USE_WAKEUP_LAT			0
>>   #define IGNORE_WAKEUP_LAT		1
>>
>> +#define MAX_HWMOD_NAME_SIZE		32
>> +
>>   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 void omap_device_delete(struct omap_device *od);
>>
>>
>>   static struct omap_device_pm_latency omap_default_latency[] = {
>> @@ -315,6 +320,138 @@ static void _add_hwmod_clocks_clkdev(struct omap_device *od,
>>   		_add_clkdev(od, oh->opt_clks[i].role, oh->opt_clks[i].clk);
>>   }
>>
>> +/*
>> + * XXX: DT helper functions that should probably move elsewhere if
>> + * they become usefull for other needs.
>> + */
>
> Lets just *assume* these are useful for other needs and start with
> them in drivers/of so that other people know that they actually exist.
> Otherwise they will never be made generic. :-)

Good point...

But, meanwhile Stephen Warren proposed a much nicer iterator to deal 
with that. Since these patches are still not in mainline, I didn't want 
to depend on them yet. So my "secret" plan was to remove these functions 
once the ones from Stephen will be merged.

>> +static int _dt_count_property_string(const char *prop, int len)
>> +{
>> +	int i = 0;
>> +	size_t l = 0, total = 0;
>> +
>> +	if (!prop || !len)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; len>= total; total += l, prop += l) {
>> +		l = strlen(prop) + 1;
>> +		if (*prop != 0)
>> +			i++;
>> +	}
>> +	return i;
>> +}
>> +
>> +static int _dt_get_property(const char *prop, int len, int index, char *output,
>> +			    int size)
>> +{
>> +	int i = 0;
>> +	size_t l = 0, total = 0;
>> +
>> +	if (!prop || !len)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; len>= total; total += l, prop += l) {
>> +		l = strlcpy(output, prop, size) + 1;
>> +		if (*prop != 0) {
>> +			if (i++ == index)
>> +				return 0;
>> +		}
>> +	}
>> +	return -ENODEV;
>> +}
>> +
>> +static struct dev_pm_domain omap_device_pm_domain;
>> +
>> +/**
>> + * omap_device_build_from_dt - build an omap_device with multiple hwmods
>> + * @pdev_name: name of the platform_device driver to use
>> + * @pdev_id: this platform_device's connection ID
>> + * @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
>> + * @is_early_device: should the device be registered as an early device or not
>> + *
>> + * Function for building an omap_device already registered from device-tree
>> + *
>> + * Returns 0 or PTR_ERR() on error.
>> + */
>> +static int omap_device_build_from_dt(struct platform_device *pdev)
>> +{
>> +	struct omap_hwmod **hwmods;
>> +	struct omap_device *od;
>> +	struct omap_hwmod *oh;
>> +	char oh_name[MAX_HWMOD_NAME_SIZE];
>> +	const char *prop;
>> +	int oh_cnt, i, prop_len;
>> +	int ret = 0;
>> +
>> +	prop = of_get_property(pdev->dev.of_node, "hwmods",&prop_len);
>
> I know you've posted it before, but what is the status of the "hwmods"
> binding documentation.  I would expect it to be part of this patch.

It was part of the series that introduced the first node with hwmods 
attribute 
(http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-August/007621.html).
I do agree, it makes sense to add it here.

>> +	oh_cnt = _dt_count_property_string(prop, prop_len);
>> +	if (!oh_cnt || IS_ERR_VALUE(oh_cnt)) {
>> +		dev_warn(&pdev->dev, "No 'hwmods' to build omap_device\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	hwmods = kzalloc(sizeof(struct omap_hwmod *) * oh_cnt, GFP_KERNEL);
>> +	if (!hwmods) {
>> +		ret = -ENOMEM;
>> +		goto odbfd_exit;
>> +	}
>> +
>> +	for (i = 0; i<  oh_cnt; i++) {
>> +		_dt_get_property(prop, prop_len, i, oh_name,
>> +				 MAX_HWMOD_NAME_SIZE);
>> +
>> +		oh = omap_hwmod_lookup(oh_name);
>> +		if (!oh) {
>> +			dev_err(&pdev->dev, "Cannot lookup hwmod '%s'\n",
>> +				oh_name);
>> +			ret = -EINVAL;
>> +			goto odbfd_exit1;
>> +		}
>> +		hwmods[i] = oh;
>> +	}
>> +
>> +	od = omap_device_alloc(pdev, hwmods, oh_cnt, NULL, 0);
>> +	if (!od) {
>> +		dev_err(&pdev->dev, "Cannot allocate omap_device for :%s\n",
>> +			oh_name);
>> +		ret = PTR_ERR(od);
>> +		goto odbfd_exit1;
>> +	}
>> +
>> +	if (of_get_property(pdev->dev.of_node, "no_idle_on_suspend", NULL))
>> +		omap_device_disable_idle_on_suspend(pdev);
>> +
>> +	pdev->dev.pm_domain =&omap_device_pm_domain;
>> +
>> +odbfd_exit1:
>> +	kfree(hwmods);
>> +odbfd_exit:
>> +	return ret;
>> +}
>> +
>> +static int _omap_device_notifier_call(struct notifier_block *nb,
>> +				      unsigned long event, void *dev)
>
> Nit: Why the preceding underscore?  Generally that is only done for
> 'special' variants of public functions.  ie. for a variant that
> expects a lock to already be held.

Yeah, the convention in this file is not that strict, and it is used for 
internal static helper function as well.
I'll let Kevin arbitrate that point :-)

>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +
>> +	switch (event) {
>> +	case BUS_NOTIFY_ADD_DEVICE:
>> +		if (pdev->dev.of_node)
>> +			omap_device_build_from_dt(pdev);
>> +		break;
>> +
>> +	case BUS_NOTIFY_DEL_DEVICE:
>> +		if (pdev->archdata.od)
>> +			omap_device_delete(pdev->archdata.od);
>> +		break;
>> +	}
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
>>
>>   /* Public functions for use by core code */
>>
>> @@ -499,6 +636,9 @@ oda_exit1:
>>
>>   static void omap_device_delete(struct omap_device *od)
>>   {
>> +	if (!od)
>> +		return;
>> +
>>   	od->pdev->archdata.od = NULL;
>>   	kfree(od->pm_lats);
>>   	kfree(od->hwmods);
>> @@ -1034,8 +1174,12 @@ struct device omap_device_parent = {
>>   	.parent         =&platform_bus,
>>   };
>>
>> +static struct notifier_block platform_nb;
>
> Nit: Since it is static initialization, you can do the following
> instead of setting it up in the init function.
>
> static struct notifier_block platform_nb = {
> 	.notifier_call = _omap_device_notifier_call;
> };

Much better, indeed.

Thanks,
Benoit

Re: [PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Cousson, Benoit]

On 9/19/2011 3:11 PM, Cousson, Benoit wrote:
> On 9/17/2011 6:05 PM, Grant Likely wrote:
>> On Fri, Sep 16, 2011 at 04:43:18PM +0200, Benoit Cousson wrote:

[...]

>>> - 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?
>
> Well, it is written in the comment... But this is maybe not that obvious
> :-)
> That part is only needed for the legacy path that will create a
> omap_device before having created the device, and thus at that time the
> dev_xxx will not give the device name. This is not a big deal, but that
> was painful for the debug.
>
> That being said, by writing that, I'm now realizing that this is due to
> the way the legacy code was working, because I didn't try to change the
> sequence.
> But maybe, I can easily avoid that by changing the original sequence.
> In fact If I create the omap_device after the omap_device_register, the
> platform_device will already have the correct name...
>
> That should work, I'll give it a try.

Mmm, that's funny because it still does require to copy some part of the 
platform_device_add to make that work.
This is now the resource name that are missing in that case :-(
So I will have to add at least that part to make that work:

		if (r->name == NULL)
			r->name = dev_name(&pdev->dev);

And the real function is doing s little bit more:

                 p = r->parent;
                 if (!p) {
                         if (resource_type(r) == IORESOURCE_MEM)
                                 p = &iomem_resource;
                         else if (resource_type(r) == IORESOURCE_IO)
                                 p = &ioport_resource;
                 }

                 if (p && insert_resource(p, r)) {
                         printk(KERN_ERR
                                "%s: failed to claim resource %d\n",
                                dev_name(&pdev->dev), i);
                         ret = -EBUSY;
                         goto failed;
                 }

It seems that in both cases, some part of the platform core code has to 
be copied to make that work properly.

Except if someone has a better idea, I'd rather stick to the original 
dev_set_name hack. Considering that this code should be removed at some 
point anyway.

Any thoughts?

Regards,
Benoit

[PATCH v2 1/2] OMAP: omap_device: Add omap_device_[alloc|delete] for DT integration

[Cousson, Benoit]

On 9/19/2011 3:11 PM, Cousson, Benoit wrote:
> On 9/17/2011 6:05 PM, Grant Likely wrote:
>> On Fri, Sep 16, 2011 at 04:43:18PM +0200, Benoit Cousson wrote:

[...]

>>> - 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?
>
> Well, it is written in the comment... But this is maybe not that obvious
> :-)
> That part is only needed for the legacy path that will create a
> omap_device before having created the device, and thus at that time the
> dev_xxx will not give the device name. This is not a big deal, but that
> was painful for the debug.
>
> That being said, by writing that, I'm now realizing that this is due to
> the way the legacy code was working, because I didn't try to change the
> sequence.
> But maybe, I can easily avoid that by changing the original sequence.
> In fact If I create the omap_device after the omap_device_register, the
> platform_device will already have the correct name...
>
> That should work, I'll give it a try.

Mmm, that's funny because it still does require to copy some part of the 
platform_device_add to make that work.
This is now the resource name that are missing in that case :-(
So I will have to add at least that part to make that work:

		if (r->name == NULL)
			r->name = dev_name(&pdev->dev);

And the real function is doing s little bit more:

                 p = r->parent;
                 if (!p) {
                         if (resource_type(r) == IORESOURCE_MEM)
                                 p = &iomem_resource;
                         else if (resource_type(r) == IORESOURCE_IO)
                                 p = &ioport_resource;
                 }

                 if (p && insert_resource(p, r)) {
                         printk(KERN_ERR
                                "%s: failed to claim resource %d\n",
                                dev_name(&pdev->dev), i);
                         ret = -EBUSY;
                         goto failed;
                 }

It seems that in both cases, some part of the platform core code has to 
be copied to make that work properly.

Except if someone has a better idea, I'd rather stick to the original 
dev_set_name hack. Considering that this code should be removed at some 
point anyway.

Any thoughts?

Regards,
Benoit

Re: [PATCH v2 2/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[Kevin Hilman]

"Cousson, Benoit" <b-cousson@ti.com> writes:

> On 9/17/2011 6:13 PM, Grant Likely wrote:
>> On Fri, Sep 16, 2011 at 04:43:19PM +0200, Benoit Cousson wrote:

[...]

>>> +}
>>> +
>>> +static int _omap_device_notifier_call(struct notifier_block *nb,
>>> +				      unsigned long event, void *dev)
>>
>> Nit: Why the preceding underscore?  Generally that is only done for
>> 'special' variants of public functions.  ie. for a variant that
>> expects a lock to already be held.
>
> Yeah, the convention in this file is not that strict, and it is used
> for internal static helper function as well.
> I'll let Kevin arbitrate that point :-)

The convention in this file is the leading '_' is used for internal
helper functions.

I'd prefer to keep it that way, and if we decide to change the coding
convention to match a coding convention elsewhere, we should do it all
at the same time.

Kevin

[PATCH v2 2/2] OMAP: omap_device: Add a method to build an omap_device from a DT node

[Kevin Hilman]

"Cousson, Benoit" <b-cousson@ti.com> writes:

> On 9/17/2011 6:13 PM, Grant Likely wrote:
>> On Fri, Sep 16, 2011 at 04:43:19PM +0200, Benoit Cousson wrote:

[...]

>>> +}
>>> +
>>> +static int _omap_device_notifier_call(struct notifier_block *nb,
>>> +				      unsigned long event, void *dev)
>>
>> Nit: Why the preceding underscore?  Generally that is only done for
>> 'special' variants of public functions.  ie. for a variant that
>> expects a lock to already be held.
>
> Yeah, the convention in this file is not that strict, and it is used
> for internal static helper function as well.
> I'll let Kevin arbitrate that point :-)

The convention in this file is the leading '_' is used for internal
helper functions.

I'd prefer to keep it that way, and if we decide to change the coding
convention to match a coding convention elsewhere, we should do it all
at the same time.

Kevin