Messages in this thread |  | | Date | Mon, 29 Aug 2011 13:09:19 +0200 | | Subject | Re: [PATCH v3 10/13] of: add a generic pinmux helper | | From | Linus Walleij <> |
| |
On Fri, Aug 26, 2011 at 1:43 AM, Stephen Warren <swarren@nvidia.com> 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |