Re: [PATCH v2 1/1] gpio: mpfs: add polarfire soc gpio support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@xxxxxxxxxxxxx> wrote:
>
> From: Lewis Hanly <lewis.hanly@xxxxxxxxxxxxx>
>
> Add a driver to support the Polarfire SoC gpio controller.

GPIO

...

Below my comments, I have tried hard not to duplicate what Conor
already mentioned. So consider this as additional part.

...

> +config GPIO_POLARFIRE_SOC
> +       bool "Microchip FPGA GPIO support"

Why not tristate?

> +       depends on OF_GPIO

Why?

> +       select GPIOLIB_IRQCHIP
> +       select IRQ_DOMAIN_HIERARCHY

More naturally this line looks if put before GPIOLB_IRQCHIP one.

> +       select GPIO_GENERIC
> +       help
> +         Say yes here to support the GPIO device on Microchip FPGAs.

When allowing it to be a module, add a text that tells how the driver
will be called.

...

> +/*
> + * Microchip PolarFire SoC (MPFS) GPIO controller driver
> + *
> + * Copyright (c) 2018-2022 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Lewis Hanly <lewis.hanly@xxxxxxxxxxxxx>

> + *

This line is not needed.

> + */

...

> +#include <linux/of.h>
> +#include <linux/of_irq.h>

Why?

Also don't forget mod_devicetable.h.

...

> +#define NUM_GPIO                       32
> +#define BYTE_BOUNDARY                  0x04

Without namespace?

...

> +       gpio_cfg = readl(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY));

> +

Unneeded line.

> +       if (gpio_cfg & MPFS_GPIO_EN_IN)
> +               return 1;
> +
> +       return 0;

Don't we have specific definitions for the directions?

...

> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +       int gpio_index = irqd_to_hwirq(data);
> +       u32 interrupt_type;

> +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);

This line naturally looks better before interrupt_type definition.
Try to keep the "longest line first" everywhere in the driver.

> +       u32 gpio_cfg;
> +       unsigned long flags;

...

> +       switch (type) {
> +       case IRQ_TYPE_EDGE_BOTH:
> +               interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_BOTH;
> +               break;

> +

Unneeded line here and everywhere in the similar cases in the entire code.

> +       case IRQ_TYPE_EDGE_FALLING:
> +               interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_NEG;
> +               break;
> +
> +       case IRQ_TYPE_EDGE_RISING:
> +               interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_POS;
> +               break;
> +
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_HIGH;
> +               break;
> +
> +       case IRQ_TYPE_LEVEL_LOW:
> +               interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW;
> +               break;
> +       }

...

> +       mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY),
> +                            MPFS_GPIO_EN_INT, 1);

Too many parentheses.

...

> +       mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY),
> +                            MPFS_GPIO_EN_INT, 0);

Ditto.

...

> +static int mpfs_gpio_irq_set_affinity(struct irq_data *data,
> +                                     const struct cpumask *dest,
> +                                     bool force)
> +{

> +       if (data->parent_data)

> +               return irq_chip_set_affinity_parent(data, dest, force);
> +
> +       return -EINVAL;
> +}

Why do you need this? Isn't it taken care of by the IRQ chip core?

...

> +       struct clk *clk;
> +       struct device *dev = &pdev->dev;

> +       struct device_node *node = pdev->dev.of_node;
> +       struct device_node *irq_parent;

Why do you need these?

> +       struct gpio_irq_chip *girq;
> +       struct irq_domain *parent;
> +       struct mpfs_gpio_chip *mpfs_gpio;
> +       int i, ret, ngpio;

...

> +       clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(clk)) {
> +               dev_err(&pdev->dev, "devm_clk_get failed\n");
> +               return PTR_ERR(clk);
> +       }

return dev_err_probe(...);

It's not only convenient, but here it fixes a bug.

> +       ret = clk_prepare_enable(clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to enable clock\n");
> +               return ret;

return dev_err_probe(...);

> +       }

Make it managed as well in addition to gpiochip_add_data(), otherwise
you will have wrong ordering.

...

> +       ngpio = of_irq_count(node);
> +       if (ngpio > NUM_GPIO) {

> +               dev_err(dev, "Too many GPIO interrupts (max=%d)\n",
> +                       NUM_GPIO);
> +               ret = -ENXIO;
> +               goto cleanup_clock;

return dev_err_probe(...);

> +       }
> +
> +       irq_parent = of_irq_find_parent(node);
> +       if (!irq_parent) {
> +               dev_err(dev, "no IRQ parent node\n");
> +               ret = -ENODEV;
> +               goto cleanup_clock;

Ditto.

> +       }
> +       parent = irq_find_host(irq_parent);
> +       if (!parent) {
> +               dev_err(dev, "no IRQ parent domain\n");
> +               ret = -ENODEV;
> +               goto cleanup_clock;

Ditto.

> +       }

Why do you need all these? Seems a copy'n'paste from gpio-sifive,
which is the only GPIO driver using this pattern.

...

> +               mpfs_gpio_assign_bit(mpfs_gpio->base + (i * BYTE_BOUNDARY),
> +                                    MPFS_GPIO_EN_INT, 0);

Too many parentheses.


> +       girq->fwnode = of_node_to_fwnode(node);

This is an interesting way of

    ...->fwnode = dev_fwnode(dev);


...

> +       dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", ngpio);

Noise.

...

> +               .of_match_table = of_match_ptr(mpfs_gpio_match),

Please, avoid using of_match_ptr(). It brings more harm than usefulness.

-- 
With Best Regards,
Andy Shevchenko