On Wed, Mar 21, 2012 at 11:48:20PM +0800, Stephen Warren wrote:
> On 03/21/2012 01:31 AM, Dong Aisheng wrote:
> > On Wed, Mar 21, 2012 at 01:44:36AM +0800, Stephen Warren wrote:
> >> 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.
>
> >> +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.
> >> +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.
> >> +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...
>
> >> + /* 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);
> ...
> >> + /* 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.
>
> >> + 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.
> >> + }
> >> +
> >> + /* 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.
>
> >> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
