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.
>> +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.
>> +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.
>> + }
>> +
>> + /* 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.
--
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
