lkml.org 
[lkml]   [2012]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V2 3/6] pinctrl: core device tree mapping table parsing support
    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.


    \
     
     \ /
      Last update: 2012-03-21 16:51    [W:0.031 / U:0.064 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site