Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 13 Jul 2022 13:59:32 +0200 | Subject | Re: [PATCH v2 1/1] gpio: mpfs: add polarfire soc gpio support |
| |
On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@microchip.com> wrote: > > From: Lewis Hanly <lewis.hanly@microchip.com> > > 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@microchip.com>
> + *
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
| |