[PATCH 1/1] gpio: mpfs - add polarfire soc gpio support
Marc Zyngier
maz at kernel.org
Tue Jul 5 23:50:00 PDT 2022
On Tue, 05 Jul 2022 14:49:12 +0100,
<lewis.hanly at microchip.com> wrote:
>
> From: Lewis Hanly <lewis.hanly at microchip.com>
>
> Add a driver to support the Polarfire SoC gpio controller.
>
> Signed-off-by: Lewis Hanly <lewis.hanly at 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 at 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.
More information about the linux-riscv
mailing list