On Wed, Mar 21, 2012 at 11:48:20PM +0800, Stephen Warren wrote:

On 03/21/2012 01:31 AM, Dong Aisheng wrote:

quoted

On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:

quoted

During pinctrl_get(), if the client device has a device tree node, look
for the common pinctrl properties there. If found, parse the referenced
device tree nodes, with the help of the pinctrl drivers, and generate
mapping table entries from them.

During pinctrl_put(), free any results of device tree parsing.

quoted

quoted

+static void dt_free_map(struct pinctrl_dev *pctldev,
+		     struct pinctrl_map *map, unsigned num_maps)
+{
+	if (pctldev) {
+		struct pinctrl_ops *ops = pctldev->desc->pctlops;
+		ops->dt_free_map(pctldev, map, num_maps);
+	} else {

I remember for hog on functions the pctldev becomes pinctrl devices itself,
so in which case pctldev can be NULL?

PIN_MAP_TYPE_DUMMY_STATE has no pctldev.

Oh, get it now.
Maybe we could a line of comment in the generating dummy state code telling about
this.

quoted

quoted

+static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
+				struct device_node *np_config)
+{
+	struct device_node *np_pctldev;
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_ops *ops;
+	int ret;
+	struct pinctrl_map *map;
+	unsigned num_maps;
+
+	/* Find the pin controller containing np_config */
+	np_pctldev = of_node_get(np_config);

It seems the np_config node is already got when call of_find_node_by_phandle.
So do we still need this line?

Right below that code, we traverse up the tree using
of_get_next_parent(). Internally, this calls of_node_put() on the node
pointer that's passed in. Hence, we need an extra get() to match this.

Yes, it's true.
So it's reasonable here.

quoted

quoted

+int pinctrl_dt_to_map(struct pinctrl *p)
+{
+	struct device_node *np = p->dev->of_node;
+	int state, ret;
+	char *propname;
+	struct property *prop;
+	const char *statename;
+	const __be32 *list;
+	int size, config;
+	phandle phandle;
+	struct device_node *np_config;
+	struct pinctrl_dt_map *dt_map;

Add NULL np checking?

Oops yes. I though I had that somewhere, but evidently not...

quoted

quoted

+	/* We may store pointers to property names within the node */
+	of_node_get(np);
+
+	/* For each defined state ID */
+	for (state = 0; ; state++) {
+		/* Retrieve the pinctrl-* property */
+		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
+		prop = of_find_property(np, propname, &size);

...

quoted

quoted

+			/* strlen("pinctrl-") == 8 */
+			if (strlen(prop->name) < 8) {

Do we really need this extra checking?
It seems the prop->name is the "pinctrl-%d" you searched above, so the
strlen(prop->name) must not < 8, right?

Assuming of_find_property works correctly, I guess that's true. We can
remove that if check.

quoted

quoted

+				dev_err(p->dev, "prop %s inconsistent length\n",
+					prop->name);
+				ret = -EINVAL;
+				goto err;
+			}
+			statename = prop->name + 8;

From this code, it seems actually we provide user the option by chance to define
state as pinctrl-syspend which is out of our binding doc.

The user can place a property with name "pinctrl-suspend" into the DT.
However, since we only look for properties named pinctrl-%d, then the
code will never read/use it, just like any other unexpected property.

Yes, you're correct.
I misunderstood that.

quoted

quoted

+		}
+
+		/* For every referenced pin configuration node in it */
+		for (config = 0; config < size; config++) {
+			phandle = be32_to_cpup(list++);
+
+			/* Look up the pin configuration node */
+			np_config = of_find_node_by_phandle(phandle);

One option is using of_parse_phandle, then we do not need calculate
the phandle offset by ourselves.
Like:
np_config = of_parse_phandle(propname , config);

Yes, that's a good idea. I'll try that.

quoted

quoted

+			if (!np_config) {
+				dev_err(p->dev,
+					"prop %s index %i invalid phandle\n",
+					prop->name, config);
+				ret = -EINVAL;
+				goto err;
+			}
+
+			/* Parse the node */
+			ret = dt_to_map_one_config(p, statename, np_config);
+			of_node_put(np_config);
+			if (ret < 0)
+				goto err;
+		}
+
+		/* No entries in DT? Generate a dummy state table entry */
+		if (!size) {
+			ret = dt_remember_dummy_state(p, statename);
+			if (ret < 0)
+				goto err;
+		}
+	}
+
+	list_for_each_entry(dt_map, &p->dt_maps, node) {
+		ret = pinctrl_register_map(dt_map->map, dt_map->num_maps,
+					   false, true);
+		if (ret < 0)
+			goto err;
+	}

What's main purpose of differing the map registration and introduce a
intermediate pinctrl_dt_map(dt_remember_or_free_map)?
What about directly register maps once it's parsed?

s/differing/deferring/ I assume.

IIRC, this was mainly to simplify error handling; by deferring it, you
don't have to unregister everything when undoing a failed parse.
However, I guess that pinctrl_dt_free_maps() already cleans up
everything anyway, so we couuld just register everything as soon as its
parsed. I'll think a little more about this and switch to doing that if
will work.

Yes, since it introduces extra complexities which i'm not sure is worth
enough, we can double check it.

Regards
Dong Aisheng