Re: [PATCH v3 10/13] of: add a generic pinmux helper

On Fri, Aug 26, 2011 at 1:43 AM, Stephen Warren [off-list ref] wrote:

+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