Re: [PATCH 2/4] pinctrl: add helpers for group based drivers

[Antoine Tenart <antoine.tenart@free-electrons.com>]

Hi Linus,

On Tue, Oct 28, 2014 at 04:38:27PM +0100, Linus Walleij wrote:
> On Mon, Oct 20, 2014 at 10:04 AM, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
> 
> > +#ifdef CONFIG_OF
> > +int of_pinctrl_utils_read_function(struct pinctrl_dev *pctldev,
> > +               struct device_node *node, const char **function_name,
> > +               int *ngroups)
> > +{
> > +       int ret;
> > +
> > +       ret = of_property_read_string(node, "function", function_name);
> > +       if (ret) {
> > +               dev_err(pctldev->dev, "missing function property in node %s\n",
> > +                       node->name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       *ngroups = of_property_count_strings(node, "groups");
> > +       if (ngroups <= 0) {
> > +               dev_err(pctldev->dev, "missing groups property in node %s\n",
> > +                       node->name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_pinctrl_utils_read_function);
> > +#endif
> 
> Isn't it nasty to print dev_err() here if you maybe just want
> to check if this node has a "function" element and proceed to
> see if it's a pin config group in case it hasn't?
> 
> Or should we add of_pinctrl_utils_node_is_mux() and
> of_pinctrl_utils_node_is_config() to check this?

We could have an additional pin_name parameter and return an error if both
function_name *and* pin_name are NULL. That could also allow to have nodes with
both a function and a pin, if that make any sense.

Maybe the of_pinctrl_utils_node_is_mux/config() solution is more
appropriate to avoid having a confusing function. Plus we would have a
dedicated of_pinctrl_utils_read_pins() function.

I think I prefer the second solution because it could avoid some
confusion and the logic would stay in the pinctrl core functions. 

I'll cook up a new version if the of_pinctrl_utils_node_is_mux/config()
is okay with you.

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com