Re: [PATCH v2 3/7] pinctrl: armada-37xx: Add pin controller support for Armada 37xx

Hi Linus,
 
 On lun., mars 27 2017, Linus Walleij [off-list ref] wrote:

On Tue, Mar 21, 2017 at 7:28 PM, Gregory CLEMENT
[off-list ref] wrote:

quoted

The Armada 37xx SoC come with 2 pin controllers: one on the south
bridge (managing 28 pins) and one on the north bridge (managing 36 pins).

At the hardware level the controller configure the pins by group and not
pin by pin. This constraint is reflected in the design of the driver:
only the group related functions are implemented.

Interesting!

quoted

+static int armada_37xx_pmx_direction_input(struct armada_37xx_pinctrl *info,
+                                          unsigned int offset)
+{
+       unsigned int reg = OUTPUT_EN;
+       unsigned int mask;
+
+       if (offset >= GPIO_PER_REG) {
+               offset -= GPIO_PER_REG;
+               reg += sizeof(u32);
+       }
+       mask = BIT(offset);
+
+       return regmap_update_bits(info->regmap, reg, mask, 0);
+}
+
+
+

A bit of excess whitespace, OK nitpicking.

As I need to send a v4, I will fix it in the same time.

Then this stuff:

quoted

+static int armada_37xx_add_function(struct armada_37xx_pmx_func *funcs,
+                                   int *funcsize, const char *name)
+{
+       int i = 0;
+
+       if (*funcsize <= 0)
+               return -EOVERFLOW;
+
+       while (funcs->ngroups) {
+               /* function already there */
+               if (strcmp(funcs->name, name) == 0) {
+                       funcs->ngroups++;
+
+                       return -EEXIST;
+               }
+               funcs++;
+               i++;
+       }
+
+       /* append new unique function */
+       funcs->name = name;
+       funcs->ngroups = 1;
+       (*funcsize)--;
+
+       return 0;
+}
+
+static int armada_37xx_fill_group(struct armada_37xx_pinctrl *info, int base)
+{
+       int n, num = 0, funcsize = info->data->nr_pins;
+
+       for (n = 0; n < info->ngroups; n++) {
+               struct armada_37xx_pin_group *grp = &info->groups[n];
+               int i, j, f;
+
+               grp->pins = devm_kzalloc(info->dev,
+                                        (grp->npins + grp->extra_npins) *
+                                        sizeof(*grp->pins), GFP_KERNEL);
+               if (!grp->pins)
+                       return -ENOMEM;
+
+               for (i = 0; i < grp->npins; i++)
+                       grp->pins[i] = grp->start_pin + base + i;
+
+               for (j = 0; j < grp->extra_npins; j++)
+                       grp->pins[i+j] = grp->extra_pin + base + j;
+
+               for (f = 0; f < NB_FUNCS; f++) {
+                       int ret;
+                       /* check for unique functions and count groups */
+                       ret = armada_37xx_add_function(info->funcs, &funcsize,
+                                           grp->funcs[f]);
+                       if (ret == -EOVERFLOW)
+                               dev_err(info->dev,
+                                       "More functions than pins(%d)\n",
+                                       info->data->nr_pins);
+                       if (ret < 0)
+                               continue;
+                       num++;
+               }
+       }
+
+       info->nfuncs = num;
+
+       return 0;
+}
+
+static int armada_37xx_fill_func(struct armada_37xx_pinctrl *info)
+{
+       struct armada_37xx_pmx_func *funcs = info->funcs;
+       int n;
+
+       for (n = 0; n < info->nfuncs; n++) {
+               const char *name = funcs[n].name;
+               const char **groups;
+               int g;
+
+               funcs[n].groups = devm_kzalloc(info->dev, funcs[n].ngroups *
+                                              sizeof(*(funcs[n].groups)),
+                                              GFP_KERNEL);
+               if (!funcs[n].groups)
+                       return -ENOMEM;
+
+               groups = funcs[n].groups;
+
+               for (g = 0; g < info->ngroups; g++) {
+                       struct armada_37xx_pin_group *gp = &info->groups[g];
+                       int f;
+
+                       for (f = 0; f < NB_FUNCS; f++) {
+                               if (strcmp(gp->funcs[f], name) == 0) {
+                                       *groups = gp->name;
+                                       groups++;
+                               }
+                       }
+               }
+       }
+       return 0;
+}

I would be happy if you add kerneldoc to these functions and explain
what they do. Because I don't get it. I guess they are filling in the data
structures but yeah. Hard to follow.

OK

quoted

+       match = of_match_node(armada_37xx_pinctrl_of_match, np);
+       info->data = (struct armada_37xx_pin_data *)match->data;

Use of_device_get_match_data()

OK

quoted

+static struct platform_driver armada_37xx_pinctrl_driver = {
+       .driver = {
+               .name = "armada-37xx-pinctrl",
+               .of_match_table = armada_37xx_pinctrl_of_match,
+       },
+       .probe = armada_37xx_pinctrl_probe,
+};
+
+builtin_platform_driver(armada_37xx_pinctrl_driver);

It almost looks like your could use builting_platform_driver_probe() actually,
and tag the costly initfunctions with __init so they get dicarded
after probe.

Sure, I will do it

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com