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
