Re: [PATCH v3 03/14] pinctrl-ingenic: add a pinctrl driver for the Ingenic jz47xx SoCs

Linus Walleij
2017-01-31T14:05:43Z

On Wed, Jan 25, 2017 at 7:51 PM, Paul Cercueil <paul@crapouillou.net> wrote:

> This driver handles pin configuration and pin muxing for the
> JZ4740 and JZ4780 SoCs from Ingenic.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

This is starting to look very nice.

> +#include <linux/compiler.h>
> +#include <linux/gpio.h>

Use <linux/gpio/driver.h> if it is a GPIO driver. I should
be enough ... I think.

> +static int ingenic_pinctrl_parse_dt_func(struct ingenic_pinctrl *jzpc,
> + struct device_node *np)
> +{
> + unsigned int num_groups;
> + struct device_node *group_node;
> + unsigned int i, j;
> + int err, npins, *pins, *confs;
> + const char **groups;
> +
> + num_groups = of_get_child_count(np);
> + groups = devm_kzalloc(jzpc->dev,
> + sizeof(*groups) * num_groups, GFP_KERNEL);
> + if (!groups)
> + return -ENOMEM;
> +
> + i = 0;
> + for_each_child_of_node(np, group_node) {
> + groups[i++] = group_node->name;
> +
> + npins = of_property_count_elems_of_size(group_node,
> + "ingenic,pins", 8);
> + if (npins < 0)
> + return npins;
> +
> + pins = devm_kzalloc(jzpc->dev,
> + sizeof(*pins) * npins, GFP_KERNEL);
> + confs = devm_kzalloc(jzpc->dev,
> + sizeof(*confs) * npins, GFP_KERNEL);
> + if (!pins || !confs)
> + return -ENOMEM;
> +
> + for (j = 0; j < npins; j++) {
> + of_property_read_u32_index(group_node,
> + "ingenic,pins", j * 2, &pins[j]);
> +
> + of_property_read_u32_index(group_node,
> + "ingenic,pins", j * 2 + 1, &confs[j]);

If I didn't mention before this could pperhaps just use "pins"?

Or does these DT entries not match the generic bindings?

> + }
> +
> + err = pinctrl_generic_add_group(jzpc->pctl, group_node->name,
> + pins, npins, confs);
> + if (err)
> + return err;
> + }
> +
> + return pinmux_generic_add_function(jzpc->pctl, np->name,
> + groups, num_groups, NULL);
> +}

If you just use "pins" can this even be parsed by a generic parser function
pinconf_generic_dt_subnode_to_map()?

Yours,
Linus Walleij