diff mbox series

[1/1] gpio: mpfs - add polarfire soc gpio support

Message ID 20220705134912.2740421-2-lewis.hanly@microchip.com (mailing list archive)
State New, archived
Headers show
Series Add Polarfire SoC GPIO support | expand

Commit Message

Lewis Hanly July 5, 2022, 1:49 p.m. UTC 
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     |   7 +
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-mpfs.c | 358 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 366 insertions(+)
 create mode 100644 drivers/gpio/gpio-mpfs.c

Comments

Conor Dooley July 5, 2022, 3:17 p.m. UTC | #1 
On 05/07/2022 14:49, 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>

Hey Lewis,
Got an error building this against v5.19-rc4 w/ the default
riscv defconfig + CONFIG_GPIO_POLARFIRE_SOC=y

/stuff/linux/drivers/gpio/gpio-mpfs.c: In function 'mpfs_gpio_probe':
/stuff/linux/drivers/gpio/gpio-mpfs.c:290:20: error: 'struct irq_chip' has no member named 'parent_device'
  290 |         irq_c->chip->parent_device = dev;
      |                    ^~
make[5]: *** [/stuff/linux/scripts/Makefile.build:249: drivers/gpio/gpio-mpfs.o] Error 1
make[4]: *** [/stuff/linux/scripts/Makefile.build:466: drivers/gpio] Error 2
make[4]: *** Waiting for unfinished jobs....

Also, please add the GPIO maintainers to V2:
Linus Walleij <linus.walleij@linaro.org> (maintainer:GPIO SUBSYSTEM)
Bartosz Golaszewski <brgl@bgdev.pl> (maintainer:GPIO SUBSYSTEM)

Thanks,
Conor.

> ---
>  drivers/gpio/Kconfig     |   7 +
>  drivers/gpio/Makefile    |   1 +
>  drivers/gpio/gpio-mpfs.c | 358 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 366 insertions(+)
>  create mode 100644 drivers/gpio/gpio-mpfs.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index b01961999ced..e279eac198da 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -490,6 +490,13 @@ 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
> +	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..df48f2836e97
> --- /dev/null
> +++ b/drivers/gpio/gpio-mpfs.c
> @@ -0,0 +1,358 @@
> +// 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;
> +	spinlock_t lock; /* lock */
> +	struct gpio_chip gc;
> +};
> +
> +static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int bit_offset, int value)
> +{
> +	u32 output = readl(addr);
> +
> +	if (value)
> +		output |= BIT(bit_offset);
> +	else
> +		output &= ~BIT(bit_offset);
> +
> +	writel(output, 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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;
> +
> +	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));
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;
> +
> +	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);
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;
> +
> +	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);
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return;
> +
> +	spin_lock_irqsave(&mpfs_gpio->lock, flags);
> +
> +	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG,
> +			     gpio_index, value);
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;
> +
> +	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:
> +	default:
> +		interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW;
> +		break;
> +	}
> +
> +	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));
> +
> +	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);
> +	int gpio_index = irqd_to_hwirq(data) % NUM_GPIO;
> +
> +	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);
> +	int gpio_index = irqd_to_hwirq(data) % 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);
> +}
> +
> +static struct irq_chip mpfs_gpio_irqchip = {
> +	.name = "mpfs_gpio_irqchip",
> +	.irq_set_type = mpfs_gpio_irq_set_type,
> +	.irq_enable = mpfs_gpio_irq_enable,
> +	.irq_disable = mpfs_gpio_irq_disable,
> +	.flags = IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +static irqreturn_t mpfs_gpio_irq_handler(int irq, void *mpfs_gpio_data)
> +{
> +	struct mpfs_gpio_chip *mpfs_gpio = mpfs_gpio_data;
> +	unsigned long status;
> +	int offset;
> +
> +	status = readl(mpfs_gpio->base + MPFS_IRQ_REG);
> +
> +	for_each_set_bit(offset, &status, mpfs_gpio->gc.ngpio) {
> +		mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, offset, 1);
> +		generic_handle_irq(irq_find_mapping(mpfs_gpio->gc.irq.domain, offset));
> +	}
> +	return IRQ_HANDLED;
> +}
> +
> +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 mpfs_gpio_chip *mpfs_gpio;
> +	int i, ret, ngpio;
> +	struct gpio_irq_chip *irq_c;
> +
> +	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)) {
> +		dev_err(dev, "failed to allocate device memory\n");
> +		return PTR_ERR(mpfs_gpio->base);
> +	}
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to get clock\n");
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to enable clock\n");
> +
> +	mpfs_gpio->clk = clk;
> +
> +	spin_lock_init(&mpfs_gpio->lock);
> +
> +	ngpio = of_irq_count(node);
> +	if (ngpio > NUM_GPIO) {
> +		dev_err(dev, "too many interrupts\n");
> +		goto cleanup_clock;
> +	}
> +
> +	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;
> +
> +	irq_c = &mpfs_gpio->gc.irq;
> +	irq_c->chip = &mpfs_gpio_irqchip;
> +	irq_c->chip->parent_device = dev;
> +	irq_c->handler = handle_simple_irq;
> +
> +	ret = devm_irq_alloc_descs(&pdev->dev, -1, 0, ngpio, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to allocate descs\n");
> +		goto cleanup_clock;
> +	}
> +
> +	/*
> +	 * Setup the interrupt handlers. Interrupts can be
> +	 * direct and/or non-direct mode, based on register value:
> +	 * GPIO_INTERRUPT_FAB_CR.
> +	 */
> +	for (i = 0; i < ngpio; i++) {
> +		int irq = platform_get_irq_optional(pdev, i);
> +
> +		if (irq < 0)
> +			continue;
> +
> +		ret = devm_request_irq(&pdev->dev, irq,
> +				       mpfs_gpio_irq_handler,
> +				       IRQF_SHARED, mpfs_gpio->gc.label, mpfs_gpio);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> +				irq, ret);
> +			goto cleanup_clock;
> +		}
> +	}
> +
> +	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 %d GPIOs\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);
Lewis Hanly July 6, 2022, 5:34 a.m. UTC | #2 
On Tue, 2022-07-05 at 15:17 +0000, Conor Dooley - M52691 wrote:
> On 05/07/2022 14:49, 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>
> 
> Hey Lewis,
> Got an error building this against v5.19-rc4 w/ the default
> riscv defconfig + CONFIG_GPIO_POLARFIRE_SOC=y
> 
> /stuff/linux/drivers/gpio/gpio-mpfs.c: In function 'mpfs_gpio_probe':
> /stuff/linux/drivers/gpio/gpio-mpfs.c:290:20: error: 'struct
> irq_chip' has no member named 'parent_device'
>   290 |         irq_c->chip->parent_device = dev;
>       |                    ^~
> make[5]: *** [/stuff/linux/scripts/Makefile.build:249:
> drivers/gpio/gpio-mpfs.o] Error 1
> make[4]: *** [/stuff/linux/scripts/Makefile.build:466: drivers/gpio]
> Error 2
> make[4]: *** Waiting for unfinished jobs....

Error acknowledged, will followup with version 1.
Regards,
Lewis

> 
> Also, please add the GPIO maintainers to V2:
> Linus Walleij <linus.walleij@linaro.org> (maintainer:GPIO SUBSYSTEM)
> Bartosz Golaszewski <brgl@bgdev.pl> (maintainer:GPIO SUBSYSTEM)
> 
> Thanks,
> Conor.
> 
> > ---
> >  drivers/gpio/Kconfig     |   7 +
> >  drivers/gpio/Makefile    |   1 +
> >  drivers/gpio/gpio-mpfs.c | 358
> > +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 366 insertions(+)
> >  create mode 100644 drivers/gpio/gpio-mpfs.c
> > 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index b01961999ced..e279eac198da 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -490,6 +490,13 @@ 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
> > +	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..df48f2836e97
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-mpfs.c
> > @@ -0,0 +1,358 @@
> > +// 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;
> > +	spinlock_t lock; /* lock */
> > +	struct gpio_chip gc;
> > +};
> > +
> > +static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int
> > bit_offset, int value)
> > +{
> > +	u32 output = readl(addr);
> > +
> > +	if (value)
> > +		output |= BIT(bit_offset);
> > +	else
> > +		output &= ~BIT(bit_offset);
> > +
> > +	writel(output, 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;
> > +
> > +	if (gpio_index >= NUM_GPIO)
> > +		return -EINVAL;
> > +
> > +	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));
> > +
> > +	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;
> > +
> > +	if (gpio_index >= NUM_GPIO)
> > +		return -EINVAL;
> > +
> > +	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);
> > +
> > +	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;
> > +
> > +	if (gpio_index >= NUM_GPIO)
> > +		return -EINVAL;
> > +
> > +	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);
> > +
> > +	if (gpio_index >= NUM_GPIO)
> > +		return -EINVAL;
> > +
> > +	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;
> > +
> > +	if (gpio_index >= NUM_GPIO)
> > +		return;
> > +
> > +	spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > +
> > +	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG,
> > +			     gpio_index, value);
> > +
> > +	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;
> > +
> > +	if (gpio_index >= NUM_GPIO)
> > +		return -EINVAL;
> > +
> > +	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:
> > +	default:
> > +		interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW;
> > +		break;
> > +	}
> > +
> > +	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));
> > +
> > +	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);
> > +	int gpio_index = irqd_to_hwirq(data) % NUM_GPIO;
> > +
> > +	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);
> > +	int gpio_index = irqd_to_hwirq(data) % 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);
> > +}
> > +
> > +static struct irq_chip mpfs_gpio_irqchip = {
> > +	.name = "mpfs_gpio_irqchip",
> > +	.irq_set_type = mpfs_gpio_irq_set_type,
> > +	.irq_enable = mpfs_gpio_irq_enable,
> > +	.irq_disable = mpfs_gpio_irq_disable,
> > +	.flags = IRQCHIP_MASK_ON_SUSPEND,
> > +};
> > +
> > +static irqreturn_t mpfs_gpio_irq_handler(int irq, void
> > *mpfs_gpio_data)
> > +{
> > +	struct mpfs_gpio_chip *mpfs_gpio = mpfs_gpio_data;
> > +	unsigned long status;
> > +	int offset;
> > +
> > +	status = readl(mpfs_gpio->base + MPFS_IRQ_REG);
> > +
> > +	for_each_set_bit(offset, &status, mpfs_gpio->gc.ngpio) {
> > +		mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG,
> > offset, 1);
> > +		generic_handle_irq(irq_find_mapping(mpfs_gpio-
> > >gc.irq.domain, offset));
> > +	}
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +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 mpfs_gpio_chip *mpfs_gpio;
> > +	int i, ret, ngpio;
> > +	struct gpio_irq_chip *irq_c;
> > +
> > +	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)) {
> > +		dev_err(dev, "failed to allocate device memory\n");
> > +		return PTR_ERR(mpfs_gpio->base);
> > +	}
> > +	clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed
> > to get clock\n");
> > +
> > +	ret = clk_prepare_enable(clk);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret, "failed to enable
> > clock\n");
> > +
> > +	mpfs_gpio->clk = clk;
> > +
> > +	spin_lock_init(&mpfs_gpio->lock);
> > +
> > +	ngpio = of_irq_count(node);
> > +	if (ngpio > NUM_GPIO) {
> > +		dev_err(dev, "too many interrupts\n");
> > +		goto cleanup_clock;
> > +	}
> > +
> > +	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;
> > +
> > +	irq_c = &mpfs_gpio->gc.irq;
> > +	irq_c->chip = &mpfs_gpio_irqchip;
> > +	irq_c->chip->parent_device = dev;
> > +	irq_c->handler = handle_simple_irq;
> > +
> > +	ret = devm_irq_alloc_descs(&pdev->dev, -1, 0, ngpio, 0);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to allocate descs\n");
> > +		goto cleanup_clock;
> > +	}
> > +
> > +	/*
> > +	 * Setup the interrupt handlers. Interrupts can be
> > +	 * direct and/or non-direct mode, based on register value:
> > +	 * GPIO_INTERRUPT_FAB_CR.
> > +	 */
> > +	for (i = 0; i < ngpio; i++) {
> > +		int irq = platform_get_irq_optional(pdev, i);
> > +
> > +		if (irq < 0)
> > +			continue;
> > +
> > +		ret = devm_request_irq(&pdev->dev, irq,
> > +				       mpfs_gpio_irq_handler,
> > +				       IRQF_SHARED, mpfs_gpio-
> > >gc.label, mpfs_gpio);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "failed to request irq %d:
> > %d\n",
> > +				irq, ret);
> > +			goto cleanup_clock;
> > +		}
> > +	}
> > +
> > +	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 %d GPIOs\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);
Marc Zyngier July 6, 2022, 6:50 a.m. UTC | #3 
On Tue, 05 Jul 2022 14:49:12 +0100,
<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     |   7 +
>  drivers/gpio/Makefile    |   1 +
>  drivers/gpio/gpio-mpfs.c | 358 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 366 insertions(+)
>  create mode 100644 drivers/gpio/gpio-mpfs.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index b01961999ced..e279eac198da 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -490,6 +490,13 @@ 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
> +	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..df48f2836e97
> --- /dev/null
> +++ b/drivers/gpio/gpio-mpfs.c
> @@ -0,0 +1,358 @@
> +// 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;
> +	spinlock_t lock; /* lock */

Interrupt controllers must use a raw spinlock. Please drop the
pointless comment.

> +	struct gpio_chip gc;
> +};
> +
> +static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int bit_offset, int value)
> +{
> +	u32 output = readl(addr);
> +
> +	if (value)
> +		output |= BIT(bit_offset);
> +	else
> +		output &= ~BIT(bit_offset);

Use __assign_bit() instead. and make value a bool to reflect the fact
that this is a single bit.

> +
> +	writel(output, 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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;

How can this happen?

> +
> +	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));
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;
> +
> +	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);
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;
> +
> +	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);
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return;
> +
> +	spin_lock_irqsave(&mpfs_gpio->lock, flags);
> +
> +	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG,
> +			     gpio_index, value);
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;
> +
> +	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:
> +	default:

What's this default for?

> +		interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW;
> +		break;
> +	}
> +
> +	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));
> +
> +	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);
> +	int gpio_index = irqd_to_hwirq(data) % NUM_GPIO;
> +
> +	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);
> +	int gpio_index = irqd_to_hwirq(data) % 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);
> +}
> +
> +static struct irq_chip mpfs_gpio_irqchip = {

Must be const.

> +	.name = "mpfs_gpio_irqchip",

Drop the pointless "gpio_irqchip".

> +	.irq_set_type = mpfs_gpio_irq_set_type,
> +	.irq_enable = mpfs_gpio_irq_enable,
> +	.irq_disable = mpfs_gpio_irq_disable,

These should be unmask/mask. enable/disable *supplement* unmask/mask
on startup.

> +	.flags = IRQCHIP_MASK_ON_SUSPEND,
> +};

No. Please read the documentation and use the current API. The kernel
should already be telling you that your driver needs fixing.

> +
> +static irqreturn_t mpfs_gpio_irq_handler(int irq, void *mpfs_gpio_data)
> +{
> +	struct mpfs_gpio_chip *mpfs_gpio = mpfs_gpio_data;
> +	unsigned long status;
> +	int offset;
> +
> +	status = readl(mpfs_gpio->base + MPFS_IRQ_REG);
> +
> +	for_each_set_bit(offset, &status, mpfs_gpio->gc.ngpio) {
> +		mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, offset, 1);
> +		generic_handle_irq(irq_find_mapping(mpfs_gpio->gc.irq.domain, offset));
> +	}
> +	return IRQ_HANDLED;
> +}
> +
> +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 mpfs_gpio_chip *mpfs_gpio;
> +	int i, ret, ngpio;
> +	struct gpio_irq_chip *irq_c;
> +
> +	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)) {
> +		dev_err(dev, "failed to allocate device memory\n");
> +		return PTR_ERR(mpfs_gpio->base);
> +	}
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to get clock\n");
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to enable clock\n");
> +
> +	mpfs_gpio->clk = clk;
> +
> +	spin_lock_init(&mpfs_gpio->lock);
> +
> +	ngpio = of_irq_count(node);
> +	if (ngpio > NUM_GPIO) {
> +		dev_err(dev, "too many interrupts\n");
> +		goto cleanup_clock;
> +	}
> +
> +	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;
> +
> +	irq_c = &mpfs_gpio->gc.irq;
> +	irq_c->chip = &mpfs_gpio_irqchip;

Same thing about the use of the current API.

> +	irq_c->chip->parent_device = dev;

OK, you clearly are developing against an ancient kernel. Don't bother
posting another version if you haven't tested your code on top of a
recent -rc.

> +	irq_c->handler = handle_simple_irq;

Why? This looks broken. You should use the flow that actually matches
the triggering, and not this.

Thanks,

	M.
Ben Dooks July 6, 2022, 10:03 a.m. UTC | #4 
On 05/07/2022 14:49, 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     |   7 +
>   drivers/gpio/Makefile    |   1 +
>   drivers/gpio/gpio-mpfs.c | 358 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 366 insertions(+)
>   create mode 100644 drivers/gpio/gpio-mpfs.c
> 

snip

> diff --git a/drivers/gpio/gpio-mpfs.c b/drivers/gpio/gpio-mpfs.c
> new file mode 100644
> index 000000000000..df48f2836e97
> --- /dev/null
> +++ b/drivers/gpio/gpio-mpfs.c
> @@ -0,0 +1,358 @@
> +// 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;
> +	spinlock_t lock; /* lock */
> +	struct gpio_chip gc;
> +};
> +
> +static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int bit_offset, int value)
> +{
> +	u32 output = readl(addr);
> +
> +	if (value)
> +		output |= BIT(bit_offset);
> +	else
> +		output &= ~BIT(bit_offset);
> +
> +	writel(output, 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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;



> +	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));
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;

Is this necessary, shouldn't gpio layer have checked this already?

> +	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);
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;

Is this necessary, shouldn't gpio layer have checked this already?

> +	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);
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;

Is this necessary, shouldn't gpio layer have checked this already?

> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return;

Is this necessary, shouldn't gpio layer have checked this already?

> +	spin_lock_irqsave(&mpfs_gpio->lock, flags);
> +
> +	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG,
> +			     gpio_index, value);
> +
> +	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;
> +
> +	if (gpio_index >= NUM_GPIO)
> +		return -EINVAL;
>

Is this necessary, shouldn't gpio layer have checked this already?

> +	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:
> +	default:
> +		interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW;
> +		break;
> +	}
> +
> +	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));
> +
> +	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);
> +	int gpio_index = irqd_to_hwirq(data) % NUM_GPIO;
> +
> +	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);
> +	int gpio_index = irqd_to_hwirq(data) % 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);
> +}
> +
> +static struct irq_chip mpfs_gpio_irqchip = {
> +	.name = "mpfs_gpio_irqchip",
> +	.irq_set_type = mpfs_gpio_irq_set_type,
> +	.irq_enable = mpfs_gpio_irq_enable,
> +	.irq_disable = mpfs_gpio_irq_disable,
> +	.flags = IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +static irqreturn_t mpfs_gpio_irq_handler(int irq, void *mpfs_gpio_data)
> +{
> +	struct mpfs_gpio_chip *mpfs_gpio = mpfs_gpio_data;
> +	unsigned long status;
> +	int offset;
> +
> +	status = readl(mpfs_gpio->base + MPFS_IRQ_REG);
> +
> +	for_each_set_bit(offset, &status, mpfs_gpio->gc.ngpio) {
> +		mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, offset, 1);
> +		generic_handle_irq(irq_find_mapping(mpfs_gpio->gc.irq.domain, offset));
> +	}
> +	return IRQ_HANDLED;
> +}
> +
> +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 mpfs_gpio_chip *mpfs_gpio;
> +	int i, ret, ngpio;
> +	struct gpio_irq_chip *irq_c;
> +
> +	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)) {
> +		dev_err(dev, "failed to allocate device memory\n");
> +		return PTR_ERR(mpfs_gpio->base);
> +	}

seems to be a mix of dev_err and dev_err_probe() in here?

> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to get clock\n");
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to enable clock\n");
> +
> +	mpfs_gpio->clk = clk;
> +
> +	spin_lock_init(&mpfs_gpio->lock);
> +
> +	ngpio = of_irq_count(node);
> +	if (ngpio > NUM_GPIO) {
> +		dev_err(dev, "too many interrupts\n");
> +		goto cleanup_clock;
> +	}
> +
> +	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;
> +
> +	irq_c = &mpfs_gpio->gc.irq;
> +	irq_c->chip = &mpfs_gpio_irqchip;
> +	irq_c->chip->parent_device = dev;
> +	irq_c->handler = handle_simple_irq;
> +
> +	ret = devm_irq_alloc_descs(&pdev->dev, -1, 0, ngpio, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to allocate descs\n");
> +		goto cleanup_clock;
> +	}
> +
> +	/*
> +	 * Setup the interrupt handlers. Interrupts can be
> +	 * direct and/or non-direct mode, based on register value:
> +	 * GPIO_INTERRUPT_FAB_CR.
> +	 */
> +	for (i = 0; i < ngpio; i++) {
> +		int irq = platform_get_irq_optional(pdev, i);
> +
> +		if (irq < 0)
> +			continue;
> +
> +		ret = devm_request_irq(&pdev->dev, irq,
> +				       mpfs_gpio_irq_handler,
> +				       IRQF_SHARED, mpfs_gpio->gc.label, mpfs_gpio);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to request irq %d: %d\n",
> +				irq, ret);
> +			goto cleanup_clock;
> +		}
> +	}
> +
> +	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 %d GPIOs\n", ngpio);
> +
> +	return 0;
> +
> +cleanup_clock:
> +	clk_disable_unprepare(mpfs_gpio->clk);
> +	return ret;
> +}

do you need to deal with EPROBEDEFER in the above probe?

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

MODULE_AUTHOR and associated info could do with being at the bottom.
kernel test robot July 7, 2022, 4:46 a.m. UTC | #5 
Hi,

I love your patch! Yet something to improve:

[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on linus/master v5.19-rc5 next-20220706]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/lewis-hanly-microchip-com/Add-Polarfire-SoC-GPIO-support/20220705-220421
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220707/202207071219.0Jw0owWG-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/f1c1cf42734c00cf02babb6220f8b68a62e86d0e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review lewis-hanly-microchip-com/Add-Polarfire-SoC-GPIO-support/20220705-220421
        git checkout f1c1cf42734c00cf02babb6220f8b68a62e86d0e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-mpfs.c: In function 'mpfs_gpio_probe':
>> drivers/gpio/gpio-mpfs.c:290:20: error: 'struct irq_chip' has no member named 'parent_device'
     290 |         irq_c->chip->parent_device = dev;
         |                    ^~
>> drivers/gpio/gpio-mpfs.c:310:23: error: implicit declaration of function 'devm_request_irq'; did you mean 'can_request_irq'? [-Werror=implicit-function-declaration]
     310 |                 ret = devm_request_irq(&pdev->dev, irq,
         |                       ^~~~~~~~~~~~~~~~
         |                       can_request_irq
>> drivers/gpio/gpio-mpfs.c:312:40: error: 'IRQF_SHARED' undeclared (first use in this function)
     312 |                                        IRQF_SHARED, mpfs_gpio->gc.label, mpfs_gpio);
         |                                        ^~~~~~~~~~~
   drivers/gpio/gpio-mpfs.c:312:40: note: each undeclared identifier is reported only once for each function it appears in
   cc1: some warnings being treated as errors


vim +290 drivers/gpio/gpio-mpfs.c

   240	
   241	static int mpfs_gpio_probe(struct platform_device *pdev)
   242	{
   243		struct clk *clk;
   244		struct device *dev = &pdev->dev;
   245		struct device_node *node = pdev->dev.of_node;
   246		struct mpfs_gpio_chip *mpfs_gpio;
   247		int i, ret, ngpio;
   248		struct gpio_irq_chip *irq_c;
   249	
   250		mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL);
   251		if (!mpfs_gpio)
   252			return -ENOMEM;
   253	
   254		mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0);
   255		if (IS_ERR(mpfs_gpio->base)) {
   256			dev_err(dev, "failed to allocate device memory\n");
   257			return PTR_ERR(mpfs_gpio->base);
   258		}
   259		clk = devm_clk_get(&pdev->dev, NULL);
   260		if (IS_ERR(clk))
   261			return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to get clock\n");
   262	
   263		ret = clk_prepare_enable(clk);
   264		if (ret)
   265			return dev_err_probe(&pdev->dev, ret, "failed to enable clock\n");
   266	
   267		mpfs_gpio->clk = clk;
   268	
   269		spin_lock_init(&mpfs_gpio->lock);
   270	
   271		ngpio = of_irq_count(node);
   272		if (ngpio > NUM_GPIO) {
   273			dev_err(dev, "too many interrupts\n");
   274			goto cleanup_clock;
   275		}
   276	
   277		mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input;
   278		mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output;
   279		mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction;
   280		mpfs_gpio->gc.get = mpfs_gpio_get;
   281		mpfs_gpio->gc.set = mpfs_gpio_set;
   282		mpfs_gpio->gc.base = -1;
   283		mpfs_gpio->gc.ngpio = ngpio;
   284		mpfs_gpio->gc.label = dev_name(dev);
   285		mpfs_gpio->gc.parent = dev;
   286		mpfs_gpio->gc.owner = THIS_MODULE;
   287	
   288		irq_c = &mpfs_gpio->gc.irq;
   289		irq_c->chip = &mpfs_gpio_irqchip;
 > 290		irq_c->chip->parent_device = dev;
   291		irq_c->handler = handle_simple_irq;
   292	
   293		ret = devm_irq_alloc_descs(&pdev->dev, -1, 0, ngpio, 0);
   294		if (ret < 0) {
   295			dev_err(dev, "failed to allocate descs\n");
   296			goto cleanup_clock;
   297		}
   298	
   299		/*
   300		 * Setup the interrupt handlers. Interrupts can be
   301		 * direct and/or non-direct mode, based on register value:
   302		 * GPIO_INTERRUPT_FAB_CR.
   303		 */
   304		for (i = 0; i < ngpio; i++) {
   305			int irq = platform_get_irq_optional(pdev, i);
   306	
   307			if (irq < 0)
   308				continue;
   309	
 > 310			ret = devm_request_irq(&pdev->dev, irq,
   311					       mpfs_gpio_irq_handler,
 > 312					       IRQF_SHARED, mpfs_gpio->gc.label, mpfs_gpio);
   313			if (ret) {
   314				dev_err(&pdev->dev, "failed to request irq %d: %d\n",
   315					irq, ret);
   316				goto cleanup_clock;
   317			}
   318		}
   319	
   320		ret = gpiochip_add_data(&mpfs_gpio->gc, mpfs_gpio);
   321		if (ret)
   322			goto cleanup_clock;
   323	
   324		platform_set_drvdata(pdev, mpfs_gpio);
   325		dev_info(dev, "Microchip MPFS GPIO registered %d GPIOs\n", ngpio);
   326	
   327		return 0;
   328	
   329	cleanup_clock:
   330		clk_disable_unprepare(mpfs_gpio->clk);
   331		return ret;
   332	}
   333
Marc Zyngier July 7, 2022, 1:03 p.m. UTC | #6 
On Thu, 07 Jul 2022 05:46:44 +0100,
kernel test robot <lkp@intel.com> wrote:
> 
> Hi,
> 
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on brgl/gpio/for-next]
> [also build test ERROR on linus/master v5.19-rc5 next-20220706]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/lewis-hanly-microchip-com/Add-Polarfire-SoC-GPIO-support/20220705-220421
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
> config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220707/202207071219.0Jw0owWG-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
> reproduce (this is a W=1 build):
>         # https://github.com/intel-lab-lkp/linux/commit/f1c1cf42734c00cf02babb6220f8b68a62e86d0e
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review lewis-hanly-microchip-com/Add-Polarfire-SoC-GPIO-support/20220705-220421
>         git checkout f1c1cf42734c00cf02babb6220f8b68a62e86d0e
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/gpio/gpio-mpfs.c: In function 'mpfs_gpio_probe':
> >> drivers/gpio/gpio-mpfs.c:290:20: error: 'struct irq_chip' has no member named 'parent_device'
>      290 |         irq_c->chip->parent_device = dev;
>          |                    ^~
> >> drivers/gpio/gpio-mpfs.c:310:23: error: implicit declaration of function 'devm_request_irq'; did you mean 'can_request_irq'? [-Werror=implicit-function-declaration]
>      310 |                 ret = devm_request_irq(&pdev->dev, irq,

I missed this in my initial eyeballing of this patch. This should
implement a chained interrupt flow, and not request the mux interrupt
directly. That's yet another level of brokenness in this driver.

	M.
Lewis Hanly July 11, 2022, 12:26 p.m. UTC | #7 
Thanks Ben for feedback,working on next version with some of your
updates and also from Marc.

Regards,
Lewis

On Wed, 2022-07-06 at 11:03 +0100, Ben Dooks wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On 05/07/2022 14:49, 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     |   7 +
> >   drivers/gpio/Makefile    |   1 +
> >   drivers/gpio/gpio-mpfs.c | 358
> > +++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 366 insertions(+)
> >   create mode 100644 drivers/gpio/gpio-mpfs.c
> > 
> 
> snip
> 
> > diff --git a/drivers/gpio/gpio-mpfs.c b/drivers/gpio/gpio-mpfs.c
> > new file mode 100644
> > index 000000000000..df48f2836e97
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-mpfs.c
> > @@ -0,0 +1,358 @@
> > +// 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;
> > +     spinlock_t lock; /* lock */
> > +     struct gpio_chip gc;
> > +};
> > +
> > +static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int
> > bit_offset, int value)
> > +{
> > +     u32 output = readl(addr);
> > +
> > +     if (value)
> > +             output |= BIT(bit_offset);
> > +     else
> > +             output &= ~BIT(bit_offset);
> > +
> > +     writel(output, 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;
> > +
> > +     if (gpio_index >= NUM_GPIO)
> > +             return -EINVAL;
> 
> 
> > +     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));
> > +
> > +     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;
> > +
> > +     if (gpio_index >= NUM_GPIO)
> > +             return -EINVAL;
> 
> Is this necessary, shouldn't gpio layer have checked this already?
No, removing all instances.
> 
> > +     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);
> > +
> > +     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;
> > +
> > +     if (gpio_index >= NUM_GPIO)
> > +             return -EINVAL;
> 
> Is this necessary, shouldn't gpio layer have checked this already?
> 
> > +     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);
> > +
> > +     if (gpio_index >= NUM_GPIO)
> > +             return -EINVAL;
> 
> Is this necessary, shouldn't gpio layer have checked this already?
> 
> > +     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;
> > +
> > +     if (gpio_index >= NUM_GPIO)
> > +             return;
> 
> Is this necessary, shouldn't gpio layer have checked this already?
> 
> > +     spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > +
> > +     mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG,
> > +                          gpio_index, value);
> > +
> > +     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;
> > +
> > +     if (gpio_index >= NUM_GPIO)
> > +             return -EINVAL;
> > 
> 
> Is this necessary, shouldn't gpio layer have checked this already?
> 
> > +     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:
> > +     default:
> > +             interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW;
> > +             break;
> > +     }
> > +
> > +     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));
> > +
> > +     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);
> > +     int gpio_index = irqd_to_hwirq(data) % NUM_GPIO;
> > +
> > +     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);
> > +     int gpio_index = irqd_to_hwirq(data) % 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);
> > +}
> > +
> > +static struct irq_chip mpfs_gpio_irqchip = {
> > +     .name = "mpfs_gpio_irqchip",
> > +     .irq_set_type = mpfs_gpio_irq_set_type,
> > +     .irq_enable = mpfs_gpio_irq_enable,
> > +     .irq_disable = mpfs_gpio_irq_disable,
> > +     .flags = IRQCHIP_MASK_ON_SUSPEND,
> > +};
> > +
> > +static irqreturn_t mpfs_gpio_irq_handler(int irq, void
> > *mpfs_gpio_data)
> > +{
> > +     struct mpfs_gpio_chip *mpfs_gpio = mpfs_gpio_data;
> > +     unsigned long status;
> > +     int offset;
> > +
> > +     status = readl(mpfs_gpio->base + MPFS_IRQ_REG);
> > +
> > +     for_each_set_bit(offset, &status, mpfs_gpio->gc.ngpio) {
> > +             mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG,
> > offset, 1);
> > +             generic_handle_irq(irq_find_mapping(mpfs_gpio-
> > >gc.irq.domain, offset));
> > +     }
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +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 mpfs_gpio_chip *mpfs_gpio;
> > +     int i, ret, ngpio;
> > +     struct gpio_irq_chip *irq_c;
> > +
> > +     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)) {
> > +             dev_err(dev, "failed to allocate device memory\n");
> > +             return PTR_ERR(mpfs_gpio->base);
> > +     }
> 
> seems to be a mix of dev_err and dev_err_probe() in here?
will update to to use only dev_err_probe
> 
> > +     clk = devm_clk_get(&pdev->dev, NULL);
> > +     if (IS_ERR(clk))
> > +             return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> > "failed to get clock\n");
> > +
> > +     ret = clk_prepare_enable(clk);
> > +     if (ret)
> > +             return dev_err_probe(&pdev->dev, ret, "failed to
> > enable clock\n");
> > +
> > +     mpfs_gpio->clk = clk;
> > +
> > +     spin_lock_init(&mpfs_gpio->lock);
> > +
> > +     ngpio = of_irq_count(node);
> > +     if (ngpio > NUM_GPIO) {
> > +             dev_err(dev, "too many interrupts\n");
> > +             goto cleanup_clock;
> > +     }
> > +
> > +     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;
> > +
> > +     irq_c = &mpfs_gpio->gc.irq;
> > +     irq_c->chip = &mpfs_gpio_irqchip;
> > +     irq_c->chip->parent_device = dev;
> > +     irq_c->handler = handle_simple_irq;
> > +
> > +     ret = devm_irq_alloc_descs(&pdev->dev, -1, 0, ngpio, 0);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to allocate descs\n");
> > +             goto cleanup_clock;
> > +     }
> > +
> > +     /*
> > +      * Setup the interrupt handlers. Interrupts can be
> > +      * direct and/or non-direct mode, based on register value:
> > +      * GPIO_INTERRUPT_FAB_CR.
> > +      */
> > +     for (i = 0; i < ngpio; i++) {
> > +             int irq = platform_get_irq_optional(pdev, i);
> > +
> > +             if (irq < 0)
> > +                     continue;
> > +
> > +             ret = devm_request_irq(&pdev->dev, irq,
> > +                                    mpfs_gpio_irq_handler,
> > +                                    IRQF_SHARED, mpfs_gpio-
> > >gc.label, mpfs_gpio);
> > +             if (ret) {
> > +                     dev_err(&pdev->dev, "failed to request irq
> > %d: %d\n",
> > +                             irq, ret);
> > +                     goto cleanup_clock;
> > +             }
> > +     }
> > +
> > +     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 %d GPIOs\n",
> > ngpio);
> > +
> > +     return 0;
> > +
> > +cleanup_clock:
> > +     clk_disable_unprepare(mpfs_gpio->clk);
> > +     return ret;
> > +}
> 
> do you need to deal with EPROBEDEFER in the above probe?
> 
> > +
> > +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);
> 
> MODULE_AUTHOR and associated info could do with being at the bottom.

It's not a module, author info at top of file.

> 
> --
> Ben Dooks                               http://www.codethink.co.uk/
> Senior Engineer                         Codethink - Providing Genius
> 
> https://www.codethink.co.uk/privacy.html
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b01961999ced..e279eac198da 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -490,6 +490,13 @@  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
+	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..df48f2836e97
--- /dev/null
+++ b/drivers/gpio/gpio-mpfs.c
@@ -0,0 +1,358 @@ 
+// 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;
+	spinlock_t lock; /* lock */
+	struct gpio_chip gc;
+};
+
+static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int bit_offset, int value)
+{
+	u32 output = readl(addr);
+
+	if (value)
+		output |= BIT(bit_offset);
+	else
+		output &= ~BIT(bit_offset);
+
+	writel(output, 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;
+
+	if (gpio_index >= NUM_GPIO)
+		return -EINVAL;
+
+	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));
+
+	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;
+
+	if (gpio_index >= NUM_GPIO)
+		return -EINVAL;
+
+	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);
+
+	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;
+
+	if (gpio_index >= NUM_GPIO)
+		return -EINVAL;
+
+	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);
+
+	if (gpio_index >= NUM_GPIO)
+		return -EINVAL;
+
+	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;
+
+	if (gpio_index >= NUM_GPIO)
+		return;
+
+	spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG,
+			     gpio_index, value);
+
+	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;
+
+	if (gpio_index >= NUM_GPIO)
+		return -EINVAL;
+
+	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:
+	default:
+		interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW;
+		break;
+	}
+
+	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));
+
+	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);
+	int gpio_index = irqd_to_hwirq(data) % NUM_GPIO;
+
+	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);
+	int gpio_index = irqd_to_hwirq(data) % 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);
+}
+
+static struct irq_chip mpfs_gpio_irqchip = {
+	.name = "mpfs_gpio_irqchip",
+	.irq_set_type = mpfs_gpio_irq_set_type,
+	.irq_enable = mpfs_gpio_irq_enable,
+	.irq_disable = mpfs_gpio_irq_disable,
+	.flags = IRQCHIP_MASK_ON_SUSPEND,
+};
+
+static irqreturn_t mpfs_gpio_irq_handler(int irq, void *mpfs_gpio_data)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = mpfs_gpio_data;
+	unsigned long status;
+	int offset;
+
+	status = readl(mpfs_gpio->base + MPFS_IRQ_REG);
+
+	for_each_set_bit(offset, &status, mpfs_gpio->gc.ngpio) {
+		mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, offset, 1);
+		generic_handle_irq(irq_find_mapping(mpfs_gpio->gc.irq.domain, offset));
+	}
+	return IRQ_HANDLED;
+}
+
+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 mpfs_gpio_chip *mpfs_gpio;
+	int i, ret, ngpio;
+	struct gpio_irq_chip *irq_c;
+
+	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)) {
+		dev_err(dev, "failed to allocate device memory\n");
+		return PTR_ERR(mpfs_gpio->base);
+	}
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to get clock\n");
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to enable clock\n");
+
+	mpfs_gpio->clk = clk;
+
+	spin_lock_init(&mpfs_gpio->lock);
+
+	ngpio = of_irq_count(node);
+	if (ngpio > NUM_GPIO) {
+		dev_err(dev, "too many interrupts\n");
+		goto cleanup_clock;
+	}
+
+	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;
+
+	irq_c = &mpfs_gpio->gc.irq;
+	irq_c->chip = &mpfs_gpio_irqchip;
+	irq_c->chip->parent_device = dev;
+	irq_c->handler = handle_simple_irq;
+
+	ret = devm_irq_alloc_descs(&pdev->dev, -1, 0, ngpio, 0);
+	if (ret < 0) {
+		dev_err(dev, "failed to allocate descs\n");
+		goto cleanup_clock;
+	}
+
+	/*
+	 * Setup the interrupt handlers. Interrupts can be
+	 * direct and/or non-direct mode, based on register value:
+	 * GPIO_INTERRUPT_FAB_CR.
+	 */
+	for (i = 0; i < ngpio; i++) {
+		int irq = platform_get_irq_optional(pdev, i);
+
+		if (irq < 0)
+			continue;
+
+		ret = devm_request_irq(&pdev->dev, irq,
+				       mpfs_gpio_irq_handler,
+				       IRQF_SHARED, mpfs_gpio->gc.label, mpfs_gpio);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to request irq %d: %d\n",
+				irq, ret);
+			goto cleanup_clock;
+		}
+	}
+
+	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 %d GPIOs\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);