* Re: [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support
2022-07-05 13:49 ` [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support lewis.hanly
@ 2022-07-05 15:17 ` Conor.Dooley
2022-07-06 5:34 ` Lewis.Hanly
2022-07-06 6:50 ` Marc Zyngier
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Conor.Dooley @ 2022-07-05 15:17 UTC (permalink / raw)
To: Lewis.Hanly, linux-gpio, linux-riscv, palmer, maz
Cc: paul.walmsley, Daire.McNamara
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);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support
2022-07-05 15:17 ` Conor.Dooley
@ 2022-07-06 5:34 ` Lewis.Hanly
0 siblings, 0 replies; 9+ messages in thread
From: Lewis.Hanly @ 2022-07-06 5:34 UTC (permalink / raw)
To: maz, linux-riscv, linux-gpio, Conor.Dooley, palmer
Cc: paul.walmsley, Daire.McNamara
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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support
2022-07-05 13:49 ` [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support lewis.hanly
2022-07-05 15:17 ` Conor.Dooley
@ 2022-07-06 6:50 ` Marc Zyngier
2022-07-06 10:03 ` Ben Dooks
2022-07-07 4:46 ` kernel test robot
3 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2022-07-06 6:50 UTC (permalink / raw)
To: lewis.hanly
Cc: linux-gpio, linux-riscv, palmer, paul.walmsley, conor.dooley,
daire.mcnamara
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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support
2022-07-05 13:49 ` [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support lewis.hanly
2022-07-05 15:17 ` Conor.Dooley
2022-07-06 6:50 ` Marc Zyngier
@ 2022-07-06 10:03 ` Ben Dooks
2022-07-11 12:26 ` Lewis.Hanly
2022-07-07 4:46 ` kernel test robot
3 siblings, 1 reply; 9+ messages in thread
From: Ben Dooks @ 2022-07-06 10:03 UTC (permalink / raw)
To: lewis.hanly, linux-gpio, linux-riscv, palmer, maz
Cc: paul.walmsley, conor.dooley, daire.mcnamara
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.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support
2022-07-06 10:03 ` Ben Dooks
@ 2022-07-11 12:26 ` Lewis.Hanly
0 siblings, 0 replies; 9+ messages in thread
From: Lewis.Hanly @ 2022-07-11 12:26 UTC (permalink / raw)
To: maz, linux-riscv, linux-gpio, ben.dooks, palmer
Cc: paul.walmsley, Conor.Dooley, Daire.McNamara
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support
2022-07-05 13:49 ` [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support lewis.hanly
` (2 preceding siblings ...)
2022-07-06 10:03 ` Ben Dooks
@ 2022-07-07 4:46 ` kernel test robot
2022-07-07 13:03 ` Marc Zyngier
3 siblings, 1 reply; 9+ messages in thread
From: kernel test robot @ 2022-07-07 4:46 UTC (permalink / raw)
To: lewis.hanly, linux-gpio, linux-riscv, palmer, maz
Cc: kbuild-all, paul.walmsley, conor.dooley, daire.mcnamara,
lewis.hanly
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] gpio: mpfs - add polarfire soc gpio support
2022-07-07 4:46 ` kernel test robot
@ 2022-07-07 13:03 ` Marc Zyngier
0 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2022-07-07 13:03 UTC (permalink / raw)
To: kernel test robot, lewis.hanly
Cc: linux-gpio, linux-riscv, palmer, kbuild-all, paul.walmsley,
conor.dooley, daire.mcnamara
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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 9+ messages in thread