On Fri, Aug 26, 2011 at 1:43 AM, Stephen Warren <swarren@xxxxxxxxxx> wrote: > diff --git a/drivers/of/of_pinmux.c b/drivers/of/of_pinmux.c > +int of_pinmux_parse(const struct of_pinmux_ctrl *ctrl, > + struct of_pinmux_cfg *cfg) OK... > +{ > + struct device_node *np; > + > + if (!ctrl || !ctrl->dev || !ctrl->node || !ctrl->configure) > + return -EINVAL; > + > + for_each_child_of_node(ctrl->node, np) { > + int ret; > + bool hadpins = 0; > + struct of_iter_string_prop iter; > + > + cfg->node = np; > + > + ret = of_property_read_string(np, "function", > + &cfg->function); > + if (ret < 0) { > + dev_err(ctrl->dev, "no function for node %s\n", > + np->name); > + continue; > + } I buy this part. > + > + cfg->flags &= 0; > + > + if (of_find_property(np, "pull-up", NULL)) > + cfg->flags |= OF_PINMUX_PULL_UP; > + if (of_find_property(np, "pull-down", NULL)) > + cfg->flags |= OF_PINMUX_PULL_DOWN; > + > + if ((cfg->flags & OF_PINMUX_PULL_MASK) == > + OF_PINMUX_PULL_MASK) { > + dev_warn(ctrl->dev, "node %s has both " > + "pull-up and pull-down properties - " > + "defaulting to no pull\n", > + np->name); > + cfg->flags &= ~OF_PINMUX_PULL_MASK; > + } > + > + if (of_find_property(np, "tristate", NULL)) > + cfg->flags |= OF_PINMUX_TRISTATE; But what does this stuff has to do with pinmux? I call this "pin biasing" and it has very little to do with muxing. If a broader, generic term is to be used, I'd prefer "pin control" which sort of nails the thing. > + for_each_string_property_value(iter, np, "pins") { > + hadpins = 1; > + > + cfg->pin = iter.value; > + > + dev_dbg(ctrl->dev, > + "configure pin %s func=%s flags=0x%lx\n", > + cfg->pin, cfg->function, cfg->flags); > + if (ctrl->configure(ctrl, cfg)) > + dev_warn(ctrl->dev, > + "failed to configure pin %s\n", > + cfg->pin); > + } > + > + if (!hadpins) > + dev_warn(ctrl->dev, "no pins for node %s\n", > + np->name); > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(of_pinmux_parse); Renamed of_pinctrl_parse I'm happier with it. > +/** > + * struct of_pinmux_cfg - configuration state for a single pinmux entry. > + * > + * @function: the name of the function that the pinmux entry should be > + * configured to. > + * @pin: the device_node of the pinmux entry that should be configured. > + * Platform specific properties that aren't in the generic binding may be > + * obtained from this device node. > + * @flags: flags for common pinmux options such as pull and tristate. I don't think these things has anything to do with pinmux at all. But with the struct renamed of_pinctrl_cfg I'm again happier. > + */ > +struct of_pinmux_cfg { > + struct device_node *node; > + const char *pin; > + const char *function; > + unsigned long flags; > +}; The current pinctrl patch set would probably want an unsigned "position" attribute too. (If we should build on that.) > +/** > + * struct of_pinmux_ctrl - platform specific pinmux control state. > + * > + * @pinmux: the pinmux device node. All child nodes are required to be the > + * pinmux entry definitions. Depending on the platform, this may either be > + * a single pin or a group of pins where they can be set to a common > + * function. > + * @configure: platform specific callback to configure the pinmux entry. > + */ > +struct of_pinmux_ctrl { > + struct device *dev; > + struct device_node *node; > + int (*parse)(const struct of_pinmux_ctrl *ctrl, > + struct of_pinmux_cfg *cfg); > + int (*configure)(const struct of_pinmux_ctrl *ctrl, > + const struct of_pinmux_cfg *cfg); > +}; s/pinmux/pinctrl/g Yours, Linus Walleij -- 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