Hi,

Document the device tree binding for the pin controllers found on the Armada 37xx SoCs.

Update the binding documention of the xtal clk which is a subnode of this syscon node.

Signed-off-by Gregory CLEMENT <[email protected]>

Quoted 6 lines · Gregory CLEMENT

Document the device tree binding for the pin controllers found on the Armada 37xx SoCs.

Update the binding documention of the xtal clk which is a subnode of this syscon node.

Quoted 1 line

Signed-off-by Gregory CLEMENT <[email protected]>

These look all right to me, but let's see what the DT people say.

This business with a subnode named "gpio-controller" that the code is checking for is not in the bindings or examples, is this part of the old Armada bindings?

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.

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c new file +7 -0

+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) {

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c new file +89 -0

+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;
+

Quoted 6 lines · Gregory CLEMENT

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 11 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c new file +11 -0

+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);

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c line 331

Quoted 6 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c new file +6 -0

+
+       return regmap_update_bits(info->regmap, reg, mask, 0);
+}
+
+
+

A bit of excess whitespace, OK nitpicking.

Then this stuff:

Quoted 95 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c new file +95 -0

+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++;
+                               }
+                       }

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c line 237

Quoted 4 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c new file +4 -0

+               }
+       }
+       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.

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c line 586

Quoted 3 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c new file +2 -0

+       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()

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c line 605

Quoted 11 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c new file +9 -0

+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.

On Tue, Mar 21, 2017 at 7:28 PM, Gregory CLEMENT
<[email protected]> wrote:

You should add something to your Kconfig including:

select GPIOLIB
select OF_GPIO

or so... or depends on. You certainly need them.

Quoted 11 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +10 -0

+static int armada_37xx_gpiochip_register(struct platform_device *pdev,
+                                       struct armada_37xx_pinctrl *info)
+{
+       struct device_node *np;
+       struct gpio_chip *gc;
+       int ret = -ENODEV;
+
+       for_each_child_of_node(info->dev->of_node, np) {
+               if (of_find_property(np, "gpio-controller", NULL)) {
+                       ret = 0;

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct pinmux_ops armada_37xx_pmx_ops = { · line 464

Quoted 3 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +3 -0

+                       break;
+               }
+       };

OK so several GPIO chips as subnodes, why not one device per chip? Or have we discussed this before? It seems a bit weird, apparently there is just one node with a gpio-controller, as you're just adding one pin range.

What happens if there would be two gpio-controllers? The second is just ignored without error?

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct pinmux_ops armada_37xx_pmx_ops = { · line 479

Quoted 4 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +3 -0

+       ret = gpiochip_add_data(gc, info);
+       if (ret)
+               return ret;

Can't you use devm_gpiochip_add_data()?

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static int armada_37xx_pinctrl_probe(struct platform_device *pdev) · line 694

Quoted 5 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +4 -0

+       ret = gpiochip_add_pin_range(&info->gpio_chip, dev_name(dev), 0,
+                                    pinbase, info->data->nr_pins);
+       if (ret)
+               return ret;

Why can't you put the range(s) into the device tree?

We already have code in drivers/gpio/gpiolib-of.c to do this for you. And generic range definition bindings.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The Armada 37xx SoCs can handle interrupt through GPIO. However it can only manage the edge ones.

The way the interrupt are managed are classical so we can use the generic interrupt chip model.

The only unusual "feature" is that many interrupts are connected to the parent interrupt controller. But we do not take advantage of this and use the chained irq with all of them.

Signed-off-by Gregory CLEMENT <[email protected]>

(quoted diff) +1 -1

-       aramda_37xx_update_reg(&reg, offset);
+       armada_37xx_update_reg(&reg, offset);

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +12 -0

+static void armada_37xx_irq_handler(struct irq_desc *desc)
+{
+       struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+       struct irq_chip *chip = irq_desc_get_chip(desc);
+       struct armada_37xx_pinctrl *info = gpiochip_get_data(gc);
+       struct irq_domain *d = gc->irqdomain;
+       int i;
+
+       chained_irq_enter(chip, desc);
+       for (i = 0; i <= d->revmap_size / GPIO_PER_REG; i++) {
+               u32 status;
+               unsigned long flags;

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +6 -0

+static int armada_37xx_irqchip_register(struct platform_device *pdev,
+                                       struct armada_37xx_pinctrl *info)
+{
+       struct device_node *np = info->dev->of_node;
+       int nrirqs = info->data->nr_pins;
+       struct gpio_chip *gc = &info->gpio_chip;

Quoted 10 lines · Gregory CLEMENT

The Armada 37xx SoCs can handle interrupt through GPIO. However it can only manage the edge ones.

The way the interrupt are managed are classical so we can use the generic interrupt chip model.

The only unusual "feature" is that many interrupts are connected to the parent interrupt controller. But we do not take advantage of this and use the chained irq with all of them.

Quoted 1 line

Signed-off-by Gregory CLEMENT <[email protected]>

You need something in your Kconfig
doing select GPIOLIB_IRQCHIP unless there is
something I miss here.

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c line 37

Quoted 4 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +3 -0

+#define IRQ_EN         0x0
+#define IRQ_POL                0x08
+#define IRQ_STATUS     0x10

This just cries out to me that there is a register 0x0c and I bet it handles edges vs levels so you could also implement level IRQs. Am I right?

Quoted 3 lines

(quoted diff) +1 -1

-       aramda_37xx_update_reg(&reg, offset);
+       armada_37xx_update_reg(&reg, offset);

(...)

Review

Quoted 2 lines

(quoted diff) +1 -1

-       aramda_37xx_update_reg(&reg, offset);
+       armada_37xx_update_reg(&reg, offset);

These spelling fixes, do not do them in this patch, fix the first patch adding them
instead. It's super-confusing. Applies everywhere.

Quoted 16 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +16 -0

+static void armada_37xx_irq_handler(struct irq_desc *desc)
+{
+       struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+       struct irq_chip *chip = irq_desc_get_chip(desc);
+       struct armada_37xx_pinctrl *info = gpiochip_get_data(gc);
+       struct irq_domain *d = gc->irqdomain;
+       int i;
+
+       chained_irq_enter(chip, desc);
+       for (i = 0; i <= d->revmap_size / GPIO_PER_REG; i++) {
+               u32 status;
+               unsigned long flags;
+
+               spin_lock_irqsave(&info->irq_lock, flags);
+               status = readl_relaxed(info->base + IRQ_STATUS + 4 * i);
+               /* Manage only the interrupt that was enabled */

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 580

Quoted 6 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +6 -0

+               status &= readl_relaxed(info->base + IRQ_EN + 4 * i);
+               spin_unlock_irqrestore(&info->irq_lock, flags);
+               while (status) {
+                       u32 hwirq = ffs(status) - 1;
+                       u32 virq = irq_linear_revmap(d, hwirq +
+                                                    i * GPIO_PER_REG);

Use irq_find_mapping() instead please.

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 583

Quoted 3 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +2 -0

+                       generic_handle_irq(virq);
+                       status &= ~(1 << hwirq);

Why not status &= ~BIT(hwirq);

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 586

Quoted 4 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +3 -0

+               }
+       }
+       chained_irq_exit(chip, desc);

Apart from that nice, it re-reads status on every iteration which is good.

Quoted 10 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +10 -0

+static int armada_37xx_irqchip_register(struct platform_device *pdev,
+                                       struct armada_37xx_pinctrl *info)
+{
+       struct device_node *np = info->dev->of_node;
+       int nrirqs = info->data->nr_pins;
+       struct gpio_chip *gc = &info->gpio_chip;
+       struct irq_chip *irqchip = &info->irq_chip;
+       struct resource res;
+       int ret, i, nr_irq_parent;
+

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 604

Quoted 6 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +6 -0

+       for_each_child_of_node(info->dev->of_node, np) {
+               if (of_find_property(np, "gpio-controller", NULL)) {
+                       ret = 0;
+                       break;
+               }
+       };

Now there is this thing again looping over the nodes.

Review

Quoted 3 lines

(quoted diff) +2 -0

+       if (ret)
+               return ret;

ret may be used uninitialized here, if you loop over all nodes and do not find any "gpio-controller".

The static code checks will just scream about this.

(Please fix in the other patch as well if present there.)

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 614

Quoted 8 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +7 -0

+       nr_irq_parent = of_irq_count(np);
+       spin_lock_init(&info->irq_lock);
+
+       if (!nr_irq_parent) {
+               dev_err(&pdev->dev, "Invalid or no IRQ\n");
+               return 0;
+       }

What if it is > 1? That doesn't seem to work but will pass this check silently.

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 633

Quoted 3 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +2 -0

+       ret = gpiochip_irqchip_add(gc, irqchip, 0,
+                                  handle_level_irq, IRQ_TYPE_NONE);

If you also set up the handler in .set_type() you can assign handle_bad_irq() here and let .set_type set the right handler as e.g. drivers/gpio/gpio-pl061.c.

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 643

Quoted 6 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +5 -0

+       for (i = 0; i < nrirqs; i++) {
+               struct irq_data *d = irq_get_irq_data(gc->irq_base + i);
+
+               d->mask = 1 << (i % GPIO_PER_REG);
+       }

What is this? It looks like a big hack. At least put in a fat comment about what is going on and why.

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 646

Quoted 3 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +2 -0

+       for (i = 0; i < nr_irq_parent; i++) {
+               int irq = irq_of_parse_and_map(np, i);

I think gpiochip_irqchip_add() will do this for you already, as it calls irq_create_mapping() for all offsets which will call irq_of_parse_and_map() am I right?

Quoted 4 lines · Linus Walleij

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 580

Quoted 2 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +2 -0

+                       u32 virq = irq_linear_revmap(d, hwirq +
+                                                    i * GPIO_PER_REG);

Use irq_find_mapping() instead please.

As we are in the interrupt handler I chose to use this function because according to its documentation: "This is a fast path alternative to irq_find_mapping() that can be called directly by irq controller code to save a handful of instructions".

The only restriction is "It is always safe to call, but won't find irqs mapped using the radix tree.". So I think that for this driver it is okay.

Quoted 8 lines

Quoted 7 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +7 -0

+       nr_irq_parent = of_irq_count(np);
+       spin_lock_init(&info->irq_lock);
+
+       if (!nr_irq_parent) {
+               dev_err(&pdev->dev, "Invalid or no IRQ\n");
+               return 0;
+       }

Quoted 2 lines

What if it is > 1? That doesn't seem to work but will pass this check silently.

If we have nr_irq_parent > 1, it will work and it is actually expected.

Quoted 6 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 633

Quoted 2 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +2 -0

+       ret = gpiochip_irqchip_add(gc, irqchip, 0,
+                                  handle_level_irq, IRQ_TYPE_NONE);

If you also set up the handler in .set_type() you can assign handle_bad_irq() here and let .set_type set the right handler as e.g. drivers/gpio/gpio-pl061.c.

Well the hardware can only manage the edge trigger, so there is no benefit to modify it each time we want to change the kind of edge we want (raising or falling). But your comment make me realized that I used the wrong one, I will move to handle_edge_irq in the v4.

Quoted 6 lines

Quoted 5 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +5 -0

+       for (i = 0; i < nrirqs; i++) {
+               struct irq_data *d = irq_get_irq_data(gc->irq_base + i);
+
+               d->mask = 1 << (i % GPIO_PER_REG);
+       }

Quoted 2 lines

What is this? It looks like a big hack. At least put in a fat comment about what is going on and why.

I can reuse a part of the commit log here: "The only unusual "feature" is that many interrupts are connected to the parent interrupt controller. But we do not take advantage of this and use the chained irq with all of them."

Quoted 6 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 646

Quoted 2 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +2 -0

+       for (i = 0; i < nr_irq_parent; i++) {
+               int irq = irq_of_parse_and_map(np, i);

I think gpiochip_irqchip_add() will do this for you already, as it calls irq_create_mapping() for all offsets which will call irq_of_parse_and_map() am I right?

After reading the code, it doesn't seem it is the case. At least there is no irq_of_parse_and_map() call from gpiochip_irqchip_add(). And waht we need here is to associate each IRQ to the same GPIO handler.

I can still try without this line to confirm it.

Quoted 7 lines · Gregory CLEMENT

Quoted 4 lines · Linus Walleij

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 580

Quoted 2 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +2 -0

+                       u32 virq = irq_linear_revmap(d, hwirq +
+                                                    i * GPIO_PER_REG);

Use irq_find_mapping() instead please.

Quoted 8 lines

As we are in the interrupt handler I chose to use this function because according to its documentation: "This is a fast path alternative to irq_find_mapping() that can be called directly by irq controller code to save a handful of instructions".

The only restriction is "It is always safe to call, but won't find irqs mapped using the radix tree.". So I think that for this driver it is okay.

So you are relying on the core code in gpiolib to select a linear map. That is implicit semantics, and either all drivers using GPIOLIB_IRQCHIP should be changed to irq_linear_revmap() or all stay with irq_find_mapping().

In this case, I doubt it that you are actually so timing critical that it matters. Please use irq_find_mapping().

Quoted 11 lines

Quoted 6 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 633

Quoted 2 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +2 -0

+       ret = gpiochip_irqchip_add(gc, irqchip, 0,
+                                  handle_level_irq, IRQ_TYPE_NONE);

If you also set up the handler in .set_type() you can assign handle_bad_irq() here and let .set_type set the right handler as e.g. drivers/gpio/gpio-pl061.c.

Well the hardware can only manage the edge trigger, so there is no benefit to modify it each time we want to change the kind of edge we want (raising or falling). But your comment make me realized that I used the wrong one, I will move to handle_edge_irq in the v4.

Ooops, yeah handle_edge_irq() is what calls the ACK callback.

Quoted 9 lines

Quoted 8 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 643

Quoted 5 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +5 -0

+       for (i = 0; i < nrirqs; i++) {
+               struct irq_data *d = irq_get_irq_data(gc->irq_base + i);
+
+               d->mask = 1 << (i % GPIO_PER_REG);
+       }

What is this? It looks like a big hack. At least put in a fat comment about what is going on and why.

Quoted 4 lines

I can reuse a part of the commit log here: "The only unusual "feature" is that many interrupts are connected to the parent interrupt controller. But we do not take advantage of this and use the chained irq with all of them."

What you're doing is mocking around with core irqchip semantics. Is ->mask really supposed to be played around with from the outsid like this?

Anyways: BIT(i % GPIO_PER_REG) is nicer.

Quoted 12 lines

Quoted 7 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c static const struct gpio_chip armada_37xx_gpiolib_chip = { · line 646

Quoted 3 lines

drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +2 -0

+       for (i = 0; i < nr_irq_parent; i++) {
+               int irq = irq_of_parse_and_map(np, i);

I think gpiochip_irqchip_add() will do this for you already, as it calls irq_create_mapping() for all offsets which will call irq_of_parse_and_map() am I right?

After reading the code, it doesn't seem it is the case. At least there is no irq_of_parse_and_map() call from gpiochip_irqchip_add(). And waht we need here is to associate each IRQ to the same GPIO handler.

Quoted 1 line

I can still try without this line to confirm it.

It has irq_create_mapping(gpiochip->irqdomain, offset); that get called for every IRQ, and that will eventually call irq_of_parse_and_map() if the IRQs are defined in the device tree. (IIRC)

Quoted 3 lines · Linus Walleij

What you're doing is mocking around with core irqchip semantics. Is ->mask really supposed to be played around with from the outsid like this?

According to the documentation mask is a "precomputed bitmask for accessing the chip registers" and it is exactly the way it is used in this driver.

Moreover, currently ->mask is only used by the generic irqchip framework and not by the core of the irqchip subsystem. So the mask initialization is not done from the oustide but at the same level as the generic irqchip which is not used here.

Quoted 3 lines

It has irq_create_mapping(gpiochip->irqdomain, offset); that get called for every IRQ, and that will eventually call irq_of_parse_and_map() if the IRQs are defined in the device tree. (IIRC)

When I followed the functions called I never find a call to irq_of_parse_and_map(), the closer things related to device tree I found was:
"virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node), NULL);"
http://elixir.free-electrons.com/source/kernel/irq/irqdomain.c?v=4.11-rc4#L507

Quoted 10 lines

Quoted 3 lines

It has irq_create_mapping(gpiochip->irqdomain, offset); that get called for every IRQ, and that will eventually call irq_of_parse_and_map() if the IRQs are defined in the device tree. (IIRC)

When I followed the functions called I never find a call to irq_of_parse_and_map(), the closer things related to device tree I found was:
"virq = irq_domain_alloc_descs(-1, 1, hwirq, of_node_to_nid(of_node), NULL);"
http://elixir.free-electrons.com/source/kernel/irq/irqdomain.c?v=4.11-rc4#L507

I don't know if I'm rambling or what. I'm pretty sure it gets called, maybe even earlier, like when the DT is parsed for the platform. We have so many drivers not seemingly needing this, but if your driver needs it, all others may need to be fixed too.

Can you put a print in irq_of_parse_and_map() and see what happens?

this series add support for the pin and gpio controllers present on the Armada 37xx SoCs.

Each Armada 37xx SoC comes 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.

The gpio controller is also capable to handle interrupt from gpio.

Gregory

Gregory CLEMENT (7):
pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers
arm64: marvell: enable the Armada 37xx pinctrl driver
pinctrl: armada-37xx: Add pin controller support for Armada 37xx
pinctrl: armada-37xx: Add gpio support
pinctrl: aramda-37xx: Add irqchip support
ARM64: dts: marvell: Add pinctrl nodes for Armada 3700
ARM64: dts: marvell: armada37xx: add pinctrl definition

 Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt         |   7 +-
 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt | 143 +++++++++++-
 arch/arm64/Kconfig.platforms                                              |   2 +-
 arch/arm64/boot/dts/marvell/armada-3720-db.dts                            |   8 +-
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi                              |  71 +++++-
 drivers/pinctrl/Makefile                                                  |   2 +-
 drivers/pinctrl/mvebu/Kconfig                                             |   7 +-
 drivers/pinctrl/mvebu/Makefile                                            |   3 +-
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c                               | 918 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 9 files changed, 1153 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
 create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c

base-commit: c1ae3cfa0e89fa1a7ecc4c99031f5e9ae99d9201

Signature · 1 line

git-series 0.9.1

Hi,

Obviously this cover letter is wrong. Actually I forgot to commit it in git series. Here it is the correct one:

After several months here it is the second version of the series adding support for the pin and gpio controllers present on the Armada 37xx SoCs.

Each Armada 37xx SoC comes 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.

The gpio controller is also capable to handle interrupt from gpio.

In the second version several changes has been done:

- Update binding documentation making clear that mfd and syscon must be used (patch 1).

- Split the fist patch adding pin controller support for Armada 37xx in arm64 part (for kconfig) and pinctrl part (patch 2 and 3)

- Add MFD_SYSCON dependency (patch 3)

- Add kerneldoc for the armada_37xx_pin_group struct (patch 3)

- Rename _add_function() to armada_37xx_add_function() (patch 3)

- Use an inline function to update the reg offset (patch 4)

- Rename gpiolib_register to gpiochip_register (patch 4)

- Add a comment about the two registers limit (patch 4)

- Add explicit gpio node in the device tree (patch 4)

- Convert the driver to use GPIOLIB_IRQCHIP (patch 5)

- Add a critical section when accessing the hardware registers (patch 5)

- Use the gpio sub-node (patch 5)

With these change most of the comment of the first revision have been addressed.

Thanks,

Gregory

Gregory CLEMENT (7):
pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers
arm64: marvell: enable the Armada 37xx pinctrl driver
pinctrl: armada-37xx: Add pin controller support for Armada 37xx
pinctrl: armada-37xx: Add gpio support
pinctrl: aramda-37xx: Add irqchip support
ARM64: dts: marvell: Add pinctrl nodes for Armada 3700
ARM64: dts: marvell: armada37xx: add pinctrl definition

 Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt         |   7 +-
 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt | 143 +++++++++++-
 arch/arm64/Kconfig.platforms                                              |   2 +-
 arch/arm64/boot/dts/marvell/armada-3720-db.dts                            |   8 +-
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi                              |  71 +++++-
 drivers/pinctrl/Makefile                                                  |   2 +-
 drivers/pinctrl/mvebu/Kconfig                                             |   7 +-
 drivers/pinctrl/mvebu/Makefile                                            |   3 +-
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c                               | 918 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 9 files changed, 1153 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
 create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c

Sorry for the noise,

Gregory

Quoted 38 lines · Gregory CLEMENT

this series add support for the pin and gpio controllers present on the Armada 37xx SoCs.

Each Armada 37xx SoC comes 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.

The gpio controller is also capable to handle interrupt from gpio.

Gregory

Gregory CLEMENT (7):
pinctrl: dt-bindings: Add documentation for Armada 37xx pin controllers
arm64: marvell: enable the Armada 37xx pinctrl driver
pinctrl: armada-37xx: Add pin controller support for Armada 37xx
pinctrl: armada-37xx: Add gpio support
pinctrl: aramda-37xx: Add irqchip support
ARM64: dts: marvell: Add pinctrl nodes for Armada 3700
ARM64: dts: marvell: armada37xx: add pinctrl definition

 Documentation/devicetree/bindings/clock/armada3700-xtal-clock.txt         |   7 +-
 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt | 143 +++++++++++-
 arch/arm64/Kconfig.platforms                                              |   2 +-
 arch/arm64/boot/dts/marvell/armada-3720-db.dts                            |   8 +-
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi                              |  71 +++++-
 drivers/pinctrl/Makefile                                                  |   2 +-
 drivers/pinctrl/mvebu/Kconfig                                             |   7 +-
 drivers/pinctrl/mvebu/Makefile                                            |   3 +-
 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c                               | 918 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 9 files changed, 1153 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,armada-37xx-pinctrl.txt
 create mode 100644 drivers/pinctrl/mvebu/pinctrl-armada-37xx.c

base-commit: c1ae3cfa0e89fa1a7ecc4c99031f5e9ae99d9201

Signature · 1 line

git-series 0.9.1

Signature · 9 lines

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel