[PATCH v2 0/1] Add Polarfire SoC GPIO support

[PATCH v2 0/1] Add Polarfire SoC GPIO support

[lewis.hanly]

From: Lewis Hanly <lewis.hanly@microchip.com>

Add a driver to support the Polarfire SoC gpio controller.
Tested with 5.19-rc5

MPFS gpio interrupts can be configured as direct or
non direct connections to the PLIC (Platform Level Interrupt Controller).
GPIO_INTERRUPT_FAB_CR(31:0) system register will enable GPIO2(31:0)
corresponding interrupt on PLIC. e.g. If GPIO_INTERRUPT_FAB_CR bit0 is set
then GPIO2 bit0 interrupt is available on the direct input pin on the PLIC.


Changes in v2:
Use raw_spinlock.
Use __assign_bit() to assign bit, added a bool variable for value.
Remove unnecessary checking gpio_index.
Remove default from switch statement.
Use const for irq_chip, name updated and use mask/unmask.
Use latest kernel api irq set_chip.
Implemented hierarchical interrupt chip support, although
suggested to use chained interrupt flow I believe this fits better.

Lewis Hanly (1):
  gpio: mpfs: add polarfire soc gpio support

 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

-- 
2.25.1

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

[lewis.hanly]

From: Lewis Hanly <lewis.hanly@microchip.com>

Add a driver to support the Polarfire SoC gpio controller.

Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
---
 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 <lewis.hanly@microchip.com>
+ *
+ */
+
+#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, &reg, 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

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

[Conor.Dooley]

Hey Lewis,
Couple comments.

On 13/07/2022 11:59, lewis.hanly@microchip.com wrote:
> From: Lewis Hanly <lewis.hanly@microchip.com>
> 
> Add a driver to support the Polarfire SoC gpio controller.
> 
> Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> ---
>   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.

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

[Lewis.Hanly]

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, lewis.hanly@microchip.com wrote:
> > From: Lewis Hanly <lewis.hanly@microchip.com>
> > 
> > Add a driver to support the Polarfire SoC gpio controller.
> > 
> > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> > ---
> >   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.

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

[Andy Shevchenko]

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

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

[Conor.Dooley]

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.

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

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 7:44 PM <Conor.Dooley@microchip.com> 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

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

[Lewis.Hanly]

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 <Conor.Dooley@microchip.com> 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

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

[Lewis.Hanly]

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 <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?
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 <lewis.hanly@microchip.com>
> > + *
> 
> 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

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

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 10:44 PM <Lewis.Hanly@microchip.com> wrote:
> On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@microchip.com> 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

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

[Conor.Dooley]

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 <Lewis.Hanly@microchip.com> wrote:
>> On Wed, 2022-07-13 at 13:59 +0200, Andy Shevchenko wrote:
>>> On Wed, Jul 13, 2022 at 1:00 PM <lewis.hanly@microchip.com> 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

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

[Lewis.Hanly]

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 <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?
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 <lewis.hanly@microchip.com>
> > + *
> 
> 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