From: Lewis Hanly <[email protected]>
Add a driver to support the Polarfire SoC gpio controller.
Signed-off-by: Lewis Hanly <[email protected]>
---
drivers/gpio/Kconfig | 9 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-mpfs.c | 379 +++++++++++++++++++++++++++++++++++++++
3 files changed, 389 insertions(+)
create mode 100644 drivers/gpio/gpio-mpfs.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b01961999ced..004b377b73f6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -490,6 +490,15 @@ config GPIO_PMIC_EIC_SPRD
help
Say yes here to support Spreadtrum PMIC EIC device.
+config GPIO_POLARFIRE_SOC
+ bool "Microchip FPGA GPIO support"
+ depends on OF_GPIO
+ select GPIOLIB_IRQCHIP
+ select IRQ_DOMAIN_HIERARCHY
+ select GPIO_GENERIC
+ help
+ Say yes here to support the GPIO device on Microchip FPGAs.
+
config GPIO_PXA
bool "PXA GPIO support"
depends on ARCH_PXA || ARCH_MMP || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 14352f6dfe8e..3b8b6703e593 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -119,6 +119,7 @@ obj-$(CONFIG_GPIO_PCI_IDIO_16) += gpio-pci-idio-16.o
obj-$(CONFIG_GPIO_PISOSR) += gpio-pisosr.o
obj-$(CONFIG_GPIO_PL061) += gpio-pl061.o
obj-$(CONFIG_GPIO_PMIC_EIC_SPRD) += gpio-pmic-eic-sprd.o
+obj-$(CONFIG_GPIO_POLARFIRE_SOC) += gpio-mpfs.o
obj-$(CONFIG_GPIO_PXA) += gpio-pxa.o
obj-$(CONFIG_GPIO_RASPBERRYPI_EXP) += gpio-raspberrypi-exp.o
obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o
diff --git a/drivers/gpio/gpio-mpfs.c b/drivers/gpio/gpio-mpfs.c
new file mode 100644
index 000000000000..1b342f307d85
--- /dev/null
+++ b/drivers/gpio/gpio-mpfs.c
@@ -0,0 +1,379 @@
+// SPDX-License-Identifier: (GPL-2.0)
+/*
+ * Microchip PolarFire SoC (MPFS) GPIO controller driver
+ *
+ * Copyright (c) 2018-2022 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Lewis Hanly <[email protected]>
+ *
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define NUM_GPIO 32
+#define BYTE_BOUNDARY 0x04
+#define MPFS_GPIO_EN_INT 3
+#define MPFS_GPIO_EN_OUT_BUF BIT(2)
+#define MPFS_GPIO_EN_IN BIT(1)
+#define MPFS_GPIO_EN_OUT BIT(0)
+
+#define MPFS_GPIO_TYPE_INT_EDGE_BOTH 0x80
+#define MPFS_GPIO_TYPE_INT_EDGE_NEG 0x60
+#define MPFS_GPIO_TYPE_INT_EDGE_POS 0x40
+#define MPFS_GPIO_TYPE_INT_LEVEL_LOW 0x20
+#define MPFS_GPIO_TYPE_INT_LEVEL_HIGH 0x00
+#define MPFS_GPIO_TYPE_INT_MASK GENMASK(7, 5)
+#define MPFS_IRQ_REG 0x80
+#define MPFS_INP_REG 0x84
+#define MPFS_OUTP_REG 0x88
+
+struct mpfs_gpio_chip {
+ void __iomem *base;
+ struct clk *clk;
+ raw_spinlock_t lock;
+ struct gpio_chip gc;
+ unsigned int irq_number[NUM_GPIO];
+};
+
+static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int bit_offset, bool value)
+{
+ unsigned long reg = readl(addr);
+
+ __assign_bit(bit_offset, ®, value);
+ writel(reg, addr);
+}
+
+static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
+{
+ struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+ u32 gpio_cfg;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+ gpio_cfg = readl(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY));
+ gpio_cfg |= MPFS_GPIO_EN_IN;
+ gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);
+ writel(gpio_cfg, mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY));
+
+ raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+
+ return 0;
+}
+
+static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
+{
+ struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+ u32 gpio_cfg;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+ gpio_cfg = readl(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY));
+ gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;
+ gpio_cfg &= ~MPFS_GPIO_EN_IN;
+ writel(gpio_cfg, mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY));
+
+ mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG, gpio_index, value);
+
+ raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+
+ return 0;
+}
+
+static int mpfs_gpio_get_direction(struct gpio_chip *gc,
+ unsigned int gpio_index)
+{
+ struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+ u32 gpio_cfg;
+
+ gpio_cfg = readl(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY));
+
+ if (gpio_cfg & MPFS_GPIO_EN_IN)
+ return 1;
+
+ return 0;
+}
+
+static int mpfs_gpio_get(struct gpio_chip *gc,
+ unsigned int gpio_index)
+{
+ struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+
+ return !!(readl(mpfs_gpio->base + MPFS_INP_REG) & BIT(gpio_index));
+}
+
+static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)
+{
+ struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+ mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG,
+ gpio_index, value);
+
+ raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+}
+
+static int mpfs_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ 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);
+ u32 gpio_cfg;
+ unsigned long flags;
+
+ switch (type) {
+ case IRQ_TYPE_EDGE_BOTH:
+ interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_BOTH;
+ break;
+
+ 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;
+ }
+
+ raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+ gpio_cfg = readl(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY));
+ gpio_cfg &= ~MPFS_GPIO_TYPE_INT_MASK;
+ gpio_cfg |= interrupt_type;
+ writel(gpio_cfg, mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY));
+
+ raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+
+ return 0;
+}
+
+static void mpfs_gpio_irq_enable(struct irq_data *data)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+ struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+ irq_hw_number_t hwirq = irqd_to_hwirq(data);
+ int gpio_index = hwirq % NUM_GPIO;
+
+ gpiochip_enable_irq(gc, hwirq);
+ irq_chip_enable_parent(data);
+
+ mpfs_gpio_direction_input(gc, gpio_index);
+ mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1);
+ mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY),
+ MPFS_GPIO_EN_INT, 1);
+}
+
+static void mpfs_gpio_irq_disable(struct irq_data *data)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+ struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+ irq_hw_number_t hwirq = irqd_to_hwirq(data);
+ int gpio_index = hwirq % NUM_GPIO;
+
+ mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1);
+ mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index * BYTE_BOUNDARY),
+ MPFS_GPIO_EN_INT, 0);
+
+ irq_chip_disable_parent(data);
+ gpiochip_disable_irq(gc, hwirq);
+}
+
+static void mpfs_gpio_irq_eoi(struct irq_data *data)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+ struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+ int offset = irqd_to_hwirq(data) % NUM_GPIO;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+ /* Clear pending interrupt */
+ mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, offset, 1);
+ raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+
+ irq_chip_eoi_parent(data);
+}
+
+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;
+}
+
+static const struct irq_chip mpfs_gpio_irqchip = {
+ .name = "mpfs",
+ .irq_set_type = mpfs_gpio_irq_set_type,
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = irq_chip_unmask_parent,
+ .irq_enable = mpfs_gpio_irq_enable,
+ .irq_disable = mpfs_gpio_irq_disable,
+ .irq_eoi = mpfs_gpio_irq_eoi,
+ .irq_set_affinity = mpfs_gpio_irq_set_affinity,
+ .flags = IRQCHIP_IMMUTABLE,
+ GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static int mpfs_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
+ unsigned int child,
+ unsigned int child_type,
+ unsigned int *parent,
+ unsigned int *parent_type)
+{
+ struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+ struct irq_data *d = irq_get_irq_data(mpfs_gpio->irq_number[child]);
+ *parent_type = IRQ_TYPE_NONE;
+ *parent = irqd_to_hwirq(d);
+
+ return 0;
+}
+
+static int mpfs_gpio_probe(struct platform_device *pdev)
+{
+ struct clk *clk;
+ struct device *dev = &pdev->dev;
+ struct device_node *node = pdev->dev.of_node;
+ struct device_node *irq_parent;
+ struct gpio_irq_chip *girq;
+ struct irq_domain *parent;
+ struct mpfs_gpio_chip *mpfs_gpio;
+ int i, ret, ngpio;
+
+ mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL);
+ if (!mpfs_gpio)
+ return -ENOMEM;
+
+ mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(mpfs_gpio->base))
+ return PTR_ERR(mpfs_gpio->base);
+
+ clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(clk)) {
+ dev_err(&pdev->dev, "devm_clk_get failed\n");
+ return PTR_ERR(clk);
+ }
+
+ ret = clk_prepare_enable(clk);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to enable clock\n");
+ return ret;
+ }
+
+ mpfs_gpio->clk = clk;
+
+ 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;
+ }
+
+ irq_parent = of_irq_find_parent(node);
+ if (!irq_parent) {
+ dev_err(dev, "no IRQ parent node\n");
+ ret = -ENODEV;
+ goto cleanup_clock;
+ }
+ parent = irq_find_host(irq_parent);
+ if (!parent) {
+ dev_err(dev, "no IRQ parent domain\n");
+ ret = -ENODEV;
+ goto cleanup_clock;
+ }
+
+ /* Get the interrupt numbers.
+ * Clear/Disable All interrupts before enabling parent interrupts.
+ */
+ for (i = 0; i < ngpio; i++) {
+ mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i);
+ mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, i, 1);
+ mpfs_gpio_assign_bit(mpfs_gpio->base + (i * BYTE_BOUNDARY),
+ MPFS_GPIO_EN_INT, 0);
+ }
+
+ raw_spin_lock_init(&mpfs_gpio->lock);
+
+ mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input;
+ mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output;
+ mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction;
+ mpfs_gpio->gc.get = mpfs_gpio_get;
+ mpfs_gpio->gc.set = mpfs_gpio_set;
+ mpfs_gpio->gc.base = -1;
+ mpfs_gpio->gc.ngpio = ngpio;
+ mpfs_gpio->gc.label = dev_name(dev);
+ mpfs_gpio->gc.parent = dev;
+ mpfs_gpio->gc.owner = THIS_MODULE;
+
+ /* Get a pointer to the gpio_irq_chip */
+ girq = &mpfs_gpio->gc.irq;
+ gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip);
+ girq->fwnode = of_node_to_fwnode(node);
+ girq->parent_domain = parent;
+ girq->child_to_parent_hwirq = mpfs_gpio_child_to_parent_hwirq;
+ girq->handler = handle_bad_irq;
+ girq->default_type = IRQ_TYPE_NONE;
+
+ ret = gpiochip_add_data(&mpfs_gpio->gc, mpfs_gpio);
+ if (ret)
+ goto cleanup_clock;
+
+ platform_set_drvdata(pdev, mpfs_gpio);
+ dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", ngpio);
+
+ return 0;
+
+cleanup_clock:
+ clk_disable_unprepare(mpfs_gpio->clk);
+ return ret;
+}
+
+static int mpfs_gpio_remove(struct platform_device *pdev)
+{
+ struct mpfs_gpio_chip *mpfs_gpio = platform_get_drvdata(pdev);
+
+ gpiochip_remove(&mpfs_gpio->gc);
+ clk_disable_unprepare(mpfs_gpio->clk);
+
+ return 0;
+}
+
+static const struct of_device_id mpfs_gpio_match[] = {
+ { .compatible = "microchip,mpfs-gpio", },
+ { /* end of list */ },
+};
+
+static struct platform_driver mpfs_gpio_driver = {
+ .probe = mpfs_gpio_probe,
+ .driver = {
+ .name = "microchip,mpfs-gpio",
+ .of_match_table = of_match_ptr(mpfs_gpio_match),
+ },
+ .remove = mpfs_gpio_remove,
+};
+builtin_platform_driver(mpfs_gpio_driver)
--
2.25.1
On Wed, Jul 13, 2022 at 1:00 PM <[email protected]> wrote:
>
> From: Lewis Hanly <[email protected]>
>
> 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 <[email protected]>
> + *
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
Hey Lewis,
Couple comments.
On 13/07/2022 11:59, [email protected] wrote:
> From: Lewis Hanly <[email protected]>
>
> Add a driver to support the Polarfire SoC gpio controller.
>
> Signed-off-by: Lewis Hanly <[email protected]>
> ---
> drivers/gpio/Kconfig | 9 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-mpfs.c | 379 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 389 insertions(+)
> create mode 100644 drivers/gpio/gpio-mpfs.c
>
> +
> +static int mpfs_gpio_probe(struct platform_device *pdev)
> +{
> + struct clk *clk;
> + struct device *dev = &pdev->dev;
> + struct device_node *node = pdev->dev.of_node;
> + struct device_node *irq_parent;
> + struct gpio_irq_chip *girq;
> + struct irq_domain *parent;
> + struct mpfs_gpio_chip *mpfs_gpio;
> + int i, ret, ngpio;
> +
> + mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL);
> + if (!mpfs_gpio)
> + return -ENOMEM;
> +
> + mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(mpfs_gpio->base))
> + return PTR_ERR(mpfs_gpio->base);
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "devm_clk_get failed\n");
> + return PTR_ERR(clk);
> + }
> +
> + ret = clk_prepare_enable(clk);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to enable clock\n");
> + return ret;
> + }
> +
> + mpfs_gpio->clk = clk;
> +
> + ngpio = of_irq_count(node);
> + if (ngpio > NUM_GPIO) {
> + dev_err(dev, "Too many GPIO interrupts (max=%d)\n",
> + NUM_GPIO);
This should fit on one line
> + ret = -ENXIO;
> + goto cleanup_clock;
> + }
> +
> + irq_parent = of_irq_find_parent(node);
> + if (!irq_parent) {
> + dev_err(dev, "no IRQ parent node\n");
> + ret = -ENODEV;
> + goto cleanup_clock;
> + }
> + parent = irq_find_host(irq_parent);
> + if (!parent) {
> + dev_err(dev, "no IRQ parent domain\n");
> + ret = -ENODEV;
> + goto cleanup_clock;
> + }
> +
> + /* Get the interrupt numbers.
netdev style comment
> + * Clear/Disable All interrupts before enabling parent interrupts.
> + */
> + for (i = 0; i < ngpio; i++) {
> + mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i);
You need to handle the case where platform_get_irq returns an error right?
> + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, i, 1);
> + mpfs_gpio_assign_bit(mpfs_gpio->base + (i * BYTE_BOUNDARY),
> + MPFS_GPIO_EN_INT, 0);
> + }
> +
> + raw_spin_lock_init(&mpfs_gpio->lock);
> +
> + mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input;
> + mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output;
> + mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction;
> + mpfs_gpio->gc.get = mpfs_gpio_get;
> + mpfs_gpio->gc.set = mpfs_gpio_set;
> + mpfs_gpio->gc.base = -1;
> + mpfs_gpio->gc.ngpio = ngpio;
> + mpfs_gpio->gc.label = dev_name(dev);
> + mpfs_gpio->gc.parent = dev;
> + mpfs_gpio->gc.owner = THIS_MODULE;
> +
> + /* Get a pointer to the gpio_irq_chip */
I doubt this comment is needed
> + girq = &mpfs_gpio->gc.irq;
> + gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip);
> + girq->fwnode = of_node_to_fwnode(node);
> + girq->parent_domain = parent;
> + girq->child_to_parent_hwirq = mpfs_gpio_child_to_parent_hwirq;
> + girq->handler = handle_bad_irq;
> + girq->default_type = IRQ_TYPE_NONE;
> +
> + ret = gpiochip_add_data(&mpfs_gpio->gc, mpfs_gpio);
Can we use a devm_ variant instead here...
> + if (ret)
> + goto cleanup_clock;
> +
> + platform_set_drvdata(pdev, mpfs_gpio);
> + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n", ngpio);
> +
> + return 0;
> +
> +cleanup_clock:
> + clk_disable_unprepare(mpfs_gpio->clk);
> + return ret;
> +}
> +
> +static int mpfs_gpio_remove(struct platform_device *pdev)
> +{
> + struct mpfs_gpio_chip *mpfs_gpio = platform_get_drvdata(pdev);
> +
> + gpiochip_remove(&mpfs_gpio->gc);
... and drop this?
> + clk_disable_unprepare(mpfs_gpio->clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id mpfs_gpio_match[] = {
> + { .compatible = "microchip,mpfs-gpio", },
> + { /* end of list */ },
Don't need a comma after the sentinel
> +};
> +
> +static struct platform_driver mpfs_gpio_driver = {
> + .probe = mpfs_gpio_probe,
> + .driver = {
> + .name = "microchip,mpfs-gpio",
> + .of_match_table = of_match_ptr(mpfs_gpio_match),
> + },
> + .remove = mpfs_gpio_remove,
> +};
> +builtin_platform_driver(mpfs_gpio_driver)
Mising semicolon?
Thanks,
Conor.
On 13/07/2022 12:59, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>
> Below my comments, I have tried hard not to duplicate what Conor
> already mentioned. So consider this as additional part.
>
>> +#define NUM_GPIO 32
>> +#define BYTE_BOUNDARY 0x04
>
> Without namespace?
Does byte_boundary even need to be defined?
is incrementing an address by 0x4 not kinda obvious on its own
as to what it is doing?
>> + if (gpio_cfg & MPFS_GPIO_EN_IN)
>> + return 1;
>> +
>> + return 0;
>
> Don't we have specific definitions for the directions?
FWIW Lewis, they're GPIO_LINE_DIRECTION_IN & GPIO_LINE_DIRECTION_OUT
I thought something like this would surely exist but wasn't sure.
Thanks for pointing it out Andy.
Conor.
On Wed, Jul 13, 2022 at 7:44 PM <[email protected]> wrote:
> On 13/07/2022 12:59, Andy Shevchenko wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
...
> >> +#define BYTE_BOUNDARY 0x04
> >
> > Without namespace?
>
> Does byte_boundary even need to be defined?
> is incrementing an address by 0x4 not kinda obvious on its own
> as to what it is doing?
The less magic is the better.
Btw, have you considered gpio-regmap? Can it be utilized here?
--
With Best Regards,
Andy Shevchenko
Thanks Conor.
On Wed, 2022-07-13 at 11:26 +0000, Conor Dooley - M52691 wrote:
> Hey Lewis,
> Couple comments.
>
> On 13/07/2022 11:59, [email protected] wrote:
> > From: Lewis Hanly <[email protected]>
> >
> > Add a driver to support the Polarfire SoC gpio controller.
> >
> > Signed-off-by: Lewis Hanly <[email protected]>
> > ---
> > drivers/gpio/Kconfig | 9 +
> > drivers/gpio/Makefile | 1 +
> > drivers/gpio/gpio-mpfs.c | 379
> > +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 389 insertions(+)
> > create mode 100644 drivers/gpio/gpio-mpfs.c
> >
> > +
> > +static int mpfs_gpio_probe(struct platform_device *pdev)
> > +{
> > + struct clk *clk;
> > + struct device *dev = &pdev->dev;
> > + struct device_node *node = pdev->dev.of_node;
> > + struct device_node *irq_parent;
> > + struct gpio_irq_chip *girq;
> > + struct irq_domain *parent;
> > + struct mpfs_gpio_chip *mpfs_gpio;
> > + int i, ret, ngpio;
> > +
> > + mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL);
> > + if (!mpfs_gpio)
> > + return -ENOMEM;
> > +
> > + mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(mpfs_gpio->base))
> > + return PTR_ERR(mpfs_gpio->base);
> > +
> > + clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(clk)) {
> > + dev_err(&pdev->dev, "devm_clk_get failed\n");
> > + return PTR_ERR(clk);
> > + }
> > +
> > + ret = clk_prepare_enable(clk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to enable clock\n");
> > + return ret;
> > + }
> > +
> > + mpfs_gpio->clk = clk;
> > +
> > + ngpio = of_irq_count(node);
> > + if (ngpio > NUM_GPIO) {
> > + dev_err(dev, "Too many GPIO interrupts (max=%d)\n",
> > + NUM_GPIO);
>
> This should fit on one line
Can do.
>
> > + ret = -ENXIO;
> > + goto cleanup_clock;
> > + }
> > +
> > + irq_parent = of_irq_find_parent(node);
> > + if (!irq_parent) {
> > + dev_err(dev, "no IRQ parent node\n");
> > + ret = -ENODEV;
> > + goto cleanup_clock;
> > + }
> > + parent = irq_find_host(irq_parent);
> > + if (!parent) {
> > + dev_err(dev, "no IRQ parent domain\n");
> > + ret = -ENODEV;
> > + goto cleanup_clock;
> > + }
> > +
> > + /* Get the interrupt numbers.
>
> netdev style comment
>
> > + * Clear/Disable All interrupts before enabling parent
> > interrupts.
> > + */
> > + for (i = 0; i < ngpio; i++) {
> > + mpfs_gpio->irq_number[i] = platform_get_irq(pdev, i);
>
> You need to handle the case where platform_get_irq returns an error
> right?
Was not going to as I don't think it breaks if error. I will do a
little more checking if required.
>
> > + mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, i,
> > 1);
> > + mpfs_gpio_assign_bit(mpfs_gpio->base + (i *
> > BYTE_BOUNDARY),
> > + MPFS_GPIO_EN_INT, 0);
> > + }
> > +
> > + raw_spin_lock_init(&mpfs_gpio->lock);
> > +
> > + mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input;
> > + mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output;
> > + mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction;
> > + mpfs_gpio->gc.get = mpfs_gpio_get;
> > + mpfs_gpio->gc.set = mpfs_gpio_set;
> > + mpfs_gpio->gc.base = -1;
> > + mpfs_gpio->gc.ngpio = ngpio;
> > + mpfs_gpio->gc.label = dev_name(dev);
> > + mpfs_gpio->gc.parent = dev;
> > + mpfs_gpio->gc.owner = THIS_MODULE;
> > +
> > + /* Get a pointer to the gpio_irq_chip */
>
> I doubt this comment is needed
OK.
>
> > + girq = &mpfs_gpio->gc.irq;
> > + gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip);
> > + girq->fwnode = of_node_to_fwnode(node);
> > + girq->parent_domain = parent;
> > + girq->child_to_parent_hwirq = mpfs_gpio_child_to_parent_hwirq;
> > + girq->handler = handle_bad_irq;
> > + girq->default_type = IRQ_TYPE_NONE;
> > +
> > + ret = gpiochip_add_data(&mpfs_gpio->gc, mpfs_gpio);
>
> Can we use a devm_ variant instead here...
Maybe, will check.
>
> > + if (ret)
> > + goto cleanup_clock;
> > +
> > + platform_set_drvdata(pdev, mpfs_gpio);
> > + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n",
> > ngpio);
> > +
> > + return 0;
> > +
> > +cleanup_clock:
> > + clk_disable_unprepare(mpfs_gpio->clk);
> > + return ret;
> > +}
> > +
> > +static int mpfs_gpio_remove(struct platform_device *pdev)
> > +{
> > + struct mpfs_gpio_chip *mpfs_gpio = platform_get_drvdata(pdev);
> > +
> > + gpiochip_remove(&mpfs_gpio->gc);
>
> ... and drop this?
eh, OK.
>
> > + clk_disable_unprepare(mpfs_gpio->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id mpfs_gpio_match[] = {
> > + { .compatible = "microchip,mpfs-gpio", },
> > + { /* end of list */ },
>
> Don't need a comma after the sentinel
OK.
>
> > +};
> > +
> > +static struct platform_driver mpfs_gpio_driver = {
> > + .probe = mpfs_gpio_probe,
> > + .driver = {
> > + .name = "microchip,mpfs-gpio",
> > + .of_match_table = of_match_ptr(mpfs_gpio_match),
> > + },
> > + .remove = mpfs_gpio_remove,
> > +};
> > +builtin_platform_driver(mpfs_gpio_driver)
>
> Mising semicolon?
Can add..
>
> Thanks,
> Conor.
On Wed, 2022-07-13 at 19:50 +0200, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Wed, Jul 13, 2022 at 7:44 PM <[email protected]> wrote:
> > On 13/07/2022 12:59, Andy Shevchenko wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
>
> ...
>
> > > > +#define BYTE_BOUNDARY 0x04
> > >
> > > Without namespace?
> >
> > Does byte_boundary even need to be defined?
> > is incrementing an address by 0x4 not kinda obvious on its own
> > as to what it is doing?
>
> The less magic is the better.
>
> Btw, have you considered gpio-regmap? Can it be utilized here?
Yes I have considered regmap, our register map is not mapped out to
fully utilize regmap. We could use for one/two registers but not fully.
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks Andy for the feedback.
Points noted and updates will be in next revision.
On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Wed, Jul 13, 2022 at 1:00 PM <[email protected]> wrote:
> > From: Lewis Hanly <[email protected]>
> >
> > 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?
OK.
>
> > + depends on OF_GPIO
>
> Why?
>
> > + select GPIOLIB_IRQCHIP
> > + select IRQ_DOMAIN_HIERARCHY
>
> More naturally this line looks if put before GPIOLB_IRQCHIP one.
Yes
>
> > + 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.
Not loading as a module.
>
> ...
>
> > +/*
> > + * Microchip PolarFire SoC (MPFS) GPIO controller driver
> > + *
> > + * Copyright (c) 2018-2022 Microchip Technology Inc. and its
> > subsidiaries
> > + *
> > + * Author: Lewis Hanly <[email protected]>
> > + *
>
> This line is not needed.
OK
>
> > + */
>
> ...
>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
>
> Why?
Not sure, will check again.
>
> Also don't forget mod_devicetable.h.
Can't see why I need this header.
>
> ...
>
> > +#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?
No defines in file.
>
> ...
>
> > + 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.
OK
>
> > + 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.
OK
>
> > + 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.
I do think it makes reading easier.
>
> ...
>
> > + 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?
Yes I believe we do/should, data->parent_data is used in
irq_chip_set_affinity_parent(..) without checking so hence checked
before calling.
>
> ...
>
> > + 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?
Yes they are needed. Both of the same type but used in different
places. I don't think I can reuse.
>
> > + 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.
Will use return dev_err_probe.
>
> > + ret = clk_prepare_enable(clk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to enable clock\n");
> > + return ret;
>
> return dev_err_probe(...);
Yes
>
> > + }
>
> 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(...);
I need to cleanup clock before returning, will use 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.
Yes I believe we do need all this information. Yes it is similiar
implementation to sifive. Not the only driver using this pattern, check
gpio-ixp4xxx.c
>
> ...
>
> > + 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.
Maybe, but useful information to know the ngpio.
>
> ...
>
> > + .of_match_table = of_match_ptr(mpfs_gpio_match),
>
> Please, avoid using of_match_ptr(). It brings more harm than
> usefulness.
OK
>
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Jul 13, 2022 at 10:44 PM <[email protected]> wrote:
> On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 1:00 PM <[email protected]> wrote:
...
> > > +config GPIO_POLARFIRE_SOC
> > > + bool "Microchip FPGA GPIO support"
> >
> > Why not tristate?
> OK.
(1)
...
> > > + 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.
> Not loading as a module.
I didn't get this. Together with (1) it makes nonsense. What did you
mean by (1) _or_ by this?
...
> > Also don't forget mod_devicetable.h.
> Can't see why I need this header.
Because you are using data types from it.
...
> > > + if (gpio_cfg & MPFS_GPIO_EN_IN)
> > > + return 1;
> > > +
> > > + return 0;
> >
> > Don't we have specific definitions for the directions?
> No defines in file.
We have. Please, check again.
...
> > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (gpio_index *
> > > BYTE_BOUNDARY),
> > > + MPFS_GPIO_EN_INT, 1);
> >
> > Too many parentheses.
> I do think it makes reading easier.
You can multiply inside mpfs_gpio_assign_bit(), no?
...
> > > + 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?
> Yes I believe we do/should, data->parent_data is used in
> irq_chip_set_affinity_parent(..) without checking so hence checked
> before calling.
I mean the entire function. Isn't it the default in the IRQ chip core?
Or can it be made as a default with some flags set?
...
> > > + struct device_node *node = pdev->dev.of_node;
> > > + struct device_node *irq_parent;
> >
> > Why do you need these?
> Yes they are needed. Both of the same type but used in different
> places. I don't think I can reuse.
This is related to the pattern of how you are enumerating IRQs. If the
pattern is changed, it would be not needed anymore.
...
> > > + 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(...);
> I need to cleanup clock before returning, will use dev_err_probe.
Have you read what I wrote above?
I wrote that you need to wrap the clk_disable_unprepare() into devm,
so you may use as I pointed out, i.e. return dev_err_probe()
everywhere in the ->probe().
> > > + }
...
> > Why do you need all these? Seems a copy'n'paste from gpio-sifive,
> > which is the only GPIO driver using this pattern.
> Yes I believe we do need all this information. Yes it is similiar
> implementation to sifive. Not the only driver using this pattern, check
> gpio-ixp4xxx.c
My question: why do you need this? What is so special about this driver?
...
> > > + mpfs_gpio_assign_bit(mpfs_gpio->base + (i *
> > > BYTE_BOUNDARY),
> > > + MPFS_GPIO_EN_INT, 0);
> >
> > Too many parentheses.
As per above.
...
> > > + dev_info(dev, "Microchip MPFS GPIO registered, ngpio=%d\n",
> > > ngpio);
> >
> > Noise.
> Maybe, but useful information to know the ngpio.
Nope. Use sysfs / debugfs / etc. No need to noise the log.
--
With Best Regards,
Andy Shevchenko
On 13/07/2022 23:14, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Wed, Jul 13, 2022 at 10:44 PM <[email protected]> wrote:
>> On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote:
>>> On Wed, Jul 13, 2022 at 1:00 PM <[email protected]> wrote:
>>>> + if (gpio_cfg & MPFS_GPIO_EN_IN)
>>>> + return 1;
>>>> +
>>>> + return 0;
>>>
>>> Don't we have specific definitions for the directions?
>> No defines in file.
>
> We have. Please, check again.
I said what they were on another reply Lewis:
https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/driver.h#L25
OK, I have gone through all comments and taken on board your review
comments. Version 3 will follow later today.
On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Wed, Jul 13, 2022 at 1:00 PM <[email protected]> wrote:
> > From: Lewis Hanly <[email protected]>
> >
> > 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?
Not a module, compile time kernel driver allways for Polarfire SoC
>
> > + depends on OF_GPIO
>
> Why?
>
> > + select GPIOLIB_IRQCHIP
> > + select IRQ_DOMAIN_HIERARCHY
>
> More naturally this line looks if put before GPIOLB_IRQCHIP one.
OK
>
> > + 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 <[email protected]>
> > + *
>
> This line is not needed.
OK
>
> > + */
>
> ...
>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
>
> Why?
I am using data defined in these headers.
>
> Also don't forget mod_devicetable.h.
OK
>
> ...
>
> > +#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.
Yes updated in next revision, using macro rather than * BYTE_BOUNDARY.
>
> ...
>
> > + 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?
irq_chip_set_affinity could be setup directly at irq_chip strcuture
initialization, but I am checking parent_data before calling. So I
would say yes I do need this.
>
> ...
>
> > + 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?
Yes I, for setting up the hierarchical interrupt controller.
>
> > + 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.
Using dev_err_probe instead of dev_err.
>
> > + 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.
OK.
>
> ...
>
> > + 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.
As above setup of hierarchical interrupt controller, very similiar to
the sifive architecture.
>
> ...
>
> > + 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.
Noise removed.
>
> ...
>
> > + .of_match_table = of_match_ptr(mpfs_gpio_match),
>
> Please, avoid using of_match_ptr(). It brings more harm than
> usefulness.
OK
>
> --
> With Best Regards,
> Andy Shevchenko