linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] gpio: xilinx: Add clock adaptation support
@ 2020-02-17 10:57 Srinivas Neeli
  2020-02-17 10:57 ` [PATCH 2/2] gpio: xilinx: Add irq support to the driver Srinivas Neeli
  2020-02-18 16:02 ` [PATCH 1/2] gpio: xilinx: Add clock adaptation support Bartosz Golaszewski
  0 siblings, 2 replies; 7+ messages in thread
From: Srinivas Neeli @ 2020-02-17 10:57 UTC (permalink / raw)
  To: bgolaszewski, michal.simek, shubhrajyoti.datta, sgoud
  Cc: linus.walleij, linux-kernel, linux-arm-kernel, linux-gpio, git

Add support of clock adaptation for AXI GPIO driver.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
 drivers/gpio/gpio-xilinx.c | 105 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 103 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index a9748b5198e6..26753ae58295 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -14,6 +14,8 @@
 #include <linux/io.h>
 #include <linux/gpio/driver.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
+#include <linux/clk.h>
 
 /* Register Offset Definitions */
 #define XGPIO_DATA_OFFSET   (0x0)	/* Data register  */
@@ -38,6 +40,7 @@
  * @gpio_state: GPIO state shadow register
  * @gpio_dir: GPIO direction shadow register
  * @gpio_lock: Lock used for synchronization
+ * @clk: clock resource for this driver
  */
 struct xgpio_instance {
 	struct gpio_chip gc;
@@ -45,7 +48,8 @@ struct xgpio_instance {
 	unsigned int gpio_width[2];
 	u32 gpio_state[2];
 	u32 gpio_dir[2];
-	spinlock_t gpio_lock[2];
+	spinlock_t gpio_lock[2];	/* For serializing operations */
+	struct clk *clk;
 };
 
 static inline int xgpio_index(struct xgpio_instance *chip, int gpio)
@@ -255,6 +259,70 @@ static void xgpio_save_regs(struct xgpio_instance *chip)
 		       chip->gpio_dir[1]);
 }
 
+static int xgpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	int ret = pm_runtime_get_sync(chip->parent);
+
+	/*
+	 * If the device is already active pm_runtime_get() will return 1 on
+	 * success, but gpio_request still needs to return 0.
+	 */
+	return ret < 0 ? ret : 0;
+}
+
+static void xgpio_free(struct gpio_chip *chip, unsigned int offset)
+{
+	pm_runtime_put(chip->parent);
+}
+
+static int __maybe_unused xgpio_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	int irq = platform_get_irq(pdev, 0);
+	struct irq_data *data = irq_get_irq_data(irq);
+
+	if (!irqd_is_wakeup_set(data))
+		return pm_runtime_force_suspend(dev);
+
+	return 0;
+}
+
+static int __maybe_unused xgpio_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	int irq = platform_get_irq(pdev, 0);
+	struct irq_data *data = irq_get_irq_data(irq);
+
+	if (!irqd_is_wakeup_set(data))
+		return pm_runtime_force_resume(dev);
+
+	return 0;
+}
+
+static int __maybe_unused xgpio_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct xgpio_instance *gpio = platform_get_drvdata(pdev);
+
+	clk_disable(gpio->clk);
+
+	return 0;
+}
+
+static int __maybe_unused xgpio_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct xgpio_instance *gpio = platform_get_drvdata(pdev);
+
+	return clk_enable(gpio->clk);
+}
+
+static const struct dev_pm_ops xgpio_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(xgpio_suspend, xgpio_resume)
+	SET_RUNTIME_PM_OPS(xgpio_runtime_suspend,
+			   xgpio_runtime_resume, NULL)
+};
+
 /**
  * xgpio_of_probe - Probe method for the GPIO device.
  * @pdev: pointer to the platform device
@@ -323,6 +391,8 @@ static int xgpio_probe(struct platform_device *pdev)
 	chip->gc.direction_output = xgpio_dir_out;
 	chip->gc.get = xgpio_get;
 	chip->gc.set = xgpio_set;
+	chip->gc.request = xgpio_request;
+	chip->gc.free = xgpio_free;
 	chip->gc.set_multiple = xgpio_set_multiple;
 
 	chip->gc.label = dev_name(&pdev->dev);
@@ -333,15 +403,45 @@ static int xgpio_probe(struct platform_device *pdev)
 		return PTR_ERR(chip->regs);
 	}
 
+	chip->clk = devm_clk_get(&pdev->dev, "s_axi_aclk");
+	if (IS_ERR(chip->clk)) {
+		if (PTR_ERR(chip->clk) != -ENOENT) {
+			if (PTR_ERR(chip->clk) != -EPROBE_DEFER)
+				dev_err(&pdev->dev, "Input clock not found\n");
+			return PTR_ERR(chip->clk);
+		}
+		/*
+		 * Clock framework support is optional, continue on
+		 * anyways if we don't find a matching clock.
+		 */
+		chip->clk = NULL;
+	}
+	status = clk_prepare_enable(chip->clk);
+	if (status < 0) {
+		dev_err(&pdev->dev, "Failed to prepare clk\n");
+		return status;
+	}
+	pm_runtime_enable(&pdev->dev);
+	status = pm_runtime_get_sync(&pdev->dev);
+	if (status < 0)
+		goto err_unprepare_clk;
+
 	xgpio_save_regs(chip);
 
 	status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
 	if (status) {
 		dev_err(&pdev->dev, "failed to add GPIO chip\n");
-		return status;
+		goto err_pm_put;
 	}
 
+	pm_runtime_put(&pdev->dev);
 	return 0;
+err_pm_put:
+	pm_runtime_put(&pdev->dev);
+err_unprepare_clk:
+	pm_runtime_disable(&pdev->dev);
+	clk_unprepare(chip->clk);
+	return status;
 }
 
 static const struct of_device_id xgpio_of_match[] = {
@@ -356,6 +456,7 @@ static struct platform_driver xgpio_plat_driver = {
 	.driver		= {
 			.name = "gpio-xilinx",
 			.of_match_table	= xgpio_of_match,
+			.pm = &xgpio_dev_pm_ops,
 	},
 };
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] gpio: xilinx: Add irq support to the driver
  2020-02-17 10:57 [PATCH 1/2] gpio: xilinx: Add clock adaptation support Srinivas Neeli
@ 2020-02-17 10:57 ` Srinivas Neeli
  2020-02-19 12:39   ` kbuild test robot
                     ` (2 more replies)
  2020-02-18 16:02 ` [PATCH 1/2] gpio: xilinx: Add clock adaptation support Bartosz Golaszewski
  1 sibling, 3 replies; 7+ messages in thread
From: Srinivas Neeli @ 2020-02-17 10:57 UTC (permalink / raw)
  To: bgolaszewski, michal.simek, shubhrajyoti.datta, sgoud
  Cc: linus.walleij, linux-kernel, linux-arm-kernel, linux-gpio, git

Allocate single chip for both channels.
Add irq support to the driver.
Supporting rising edge interrupts and in cascade mode supporting
first channel for interrupts on 32bit machines.

Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
---
 drivers/gpio/gpio-xilinx.c | 233 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 232 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 26753ae58295..f6dd316b2c62 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -16,6 +16,11 @@
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
 #include <linux/clk.h>
+#include <linux/of_irq.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 
 /* Register Offset Definitions */
 #define XGPIO_DATA_OFFSET   (0x0)	/* Data register  */
@@ -23,8 +28,13 @@
 
 #define XGPIO_CHANNEL_OFFSET	0x8
 
+#define XGPIO_GIER_OFFSET      0x11c /* Global Interrupt Enable */
+#define XGPIO_GIER_IE          BIT(31)
+#define XGPIO_IPISR_OFFSET     0x120 /* IP Interrupt Status */
+#define XGPIO_IPIER_OFFSET     0x128 /* IP Interrupt Enable */
+
 /* Read/Write access to the GPIO registers */
-#if defined(CONFIG_ARCH_ZYNQ) || defined(CONFIG_X86)
+#if defined(CONFIG_ARCH_ZYNQ) || defined(CONFIG_X86) || defined(CONFIG_ARM64)
 # define xgpio_readreg(offset)		readl(offset)
 # define xgpio_writereg(offset, val)	writel(val, offset)
 #else
@@ -41,7 +51,11 @@
  * @gpio_dir: GPIO direction shadow register
  * @gpio_lock: Lock used for synchronization
  * @clk: clock resource for this driver
+ * @irq_base: GPIO channel irq base address
+ * @irq_enable: GPIO irq enable/disable bitfield
+ * @irq_domain: irq_domain of the controller
  */
+
 struct xgpio_instance {
 	struct gpio_chip gc;
 	void __iomem *regs;
@@ -50,6 +64,9 @@ struct xgpio_instance {
 	u32 gpio_dir[2];
 	spinlock_t gpio_lock[2];	/* For serializing operations */
 	struct clk *clk;
+	int irq_base;
+	u32 irq_enable;
+	struct irq_domain *irq_domain;
 };
 
 static inline int xgpio_index(struct xgpio_instance *chip, int gpio)
@@ -324,6 +341,211 @@ static const struct dev_pm_ops xgpio_dev_pm_ops = {
 };
 
 /**
+ * xgpiops_irq_mask - Write the specified signal of the GPIO device.
+ * @irq_data: per irq and chip data passed down to chip functions
+ */
+static void xgpio_irq_mask(struct irq_data *irq_data)
+{
+	unsigned long flags;
+	struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
+	u32 offset = irq_data->irq - chip->irq_base;
+	u32 temp;
+	s32 val;
+	int index = xgpio_index(chip, 0);
+
+	pr_debug("%s: Disable %d irq, irq_enable_mask 0x%x\n",
+		 __func__, offset, chip->irq_enable);
+
+	spin_lock_irqsave(&chip->gpio_lock[index], flags);
+
+	chip->irq_enable &= ~BIT(offset);
+
+	if (!chip->irq_enable) {
+		/* Enable per channel interrupt */
+		temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
+		val = offset - chip->gpio_width[0] + 1;
+		if (val > 0)
+			temp &= 1;
+		else
+			temp &= 2;
+		xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
+
+		/* Disable global interrupt if channel interrupts are unused */
+		temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
+		if (!temp)
+			xgpio_writereg(chip->regs + XGPIO_GIER_OFFSET,
+				       ~XGPIO_GIER_IE);
+	}
+	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
+}
+
+/**
+ * xgpio_irq_unmask - Write the specified signal of the GPIO device.
+ * @irq_data: per irq and chip data passed down to chip functions
+ */
+static void xgpio_irq_unmask(struct irq_data *irq_data)
+{
+	unsigned long flags;
+	struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
+	u32 offset = irq_data->irq - chip->irq_base;
+	u32 temp;
+	s32 val;
+	int index = xgpio_index(chip, 0);
+
+	pr_debug("%s: Enable %d irq, irq_enable_mask 0x%x\n",
+		 __func__, offset, chip->irq_enable);
+
+	/* Setup pin as input */
+	xgpio_dir_in(&chip->gc, offset);
+
+	spin_lock_irqsave(&chip->gpio_lock[index], flags);
+
+	chip->irq_enable |= BIT(offset);
+
+	if (chip->irq_enable) {
+		/* Enable per channel interrupt */
+		temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
+		val = offset - (chip->gpio_width[0] - 1);
+		if (val > 0)
+			temp |= 2;
+		else
+			temp |= 1;
+		xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
+
+		/* Enable global interrupts */
+		xgpio_writereg(chip->regs + XGPIO_GIER_OFFSET, XGPIO_GIER_IE);
+	}
+
+	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
+}
+
+/**
+ * xgpio_set_irq_type - Write the specified signal of the GPIO device.
+ * @irq_data: Per irq and chip data passed down to chip functions
+ * @type: Interrupt type that is to be set for the gpio pin
+ *
+ * Return:
+ * 0 if interrupt type is supported otherwise otherwise -EINVAL
+ */
+static int xgpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
+{
+	/* Only rising edge case is supported now */
+	if (type & IRQ_TYPE_EDGE_RISING)
+		return 0;
+
+	return -EINVAL;
+}
+
+/* irq chip descriptor */
+static struct irq_chip xgpio_irqchip = {
+	.name           = "xgpio",
+	.irq_mask       = xgpio_irq_mask,
+	.irq_unmask     = xgpio_irq_unmask,
+	.irq_set_type   = xgpio_set_irq_type,
+};
+
+/**
+ * xgpio_to_irq - Find out gpio to Linux irq mapping
+ * @gc: Pointer to gpio_chip device structure.
+ * @offset: Gpio pin offset
+ *
+ * Return:
+ * irq number otherwise -EINVAL
+ */
+static int xgpio_to_irq(struct gpio_chip *gc, unsigned int offset)
+{
+	struct xgpio_instance *chip = gpiochip_get_data(gc);
+
+	return irq_find_mapping(chip->irq_domain, offset);
+}
+
+/**
+ * xgpio_irqhandler - Gpio interrupt service routine
+ * @desc: Pointer to interrupt description
+ */
+static void xgpio_irqhandler(struct irq_desc *desc)
+{
+	unsigned int irq = irq_desc_get_irq(desc);
+	struct xgpio_instance *chip = (struct xgpio_instance *)
+		irq_get_handler_data(irq);
+	struct irq_chip *irqchip = irq_desc_get_chip(desc);
+	u32 offset, status, channel = 1;
+	unsigned long val;
+
+	chained_irq_enter(irqchip, desc);
+
+	val = xgpio_readreg(chip->regs);
+	if (!val) {
+		channel = 2;
+		val = xgpio_readreg(chip->regs + XGPIO_CHANNEL_OFFSET);
+		val = val << chip->gpio_width[0];
+	}
+
+	/* Only rising edge is supported */
+	val &= chip->irq_enable;
+	for_each_set_bit(offset, &val, chip->gc.ngpio) {
+		generic_handle_irq(chip->irq_base + offset);
+	}
+
+	status = xgpio_readreg(chip->regs + XGPIO_IPISR_OFFSET);
+	xgpio_writereg(chip->regs + XGPIO_IPISR_OFFSET, channel);
+
+	chained_irq_exit(irqchip, desc);
+}
+
+static struct lock_class_key gpio_lock_class;
+static struct lock_class_key gpio_request_class;
+
+/**
+ * xgpio_irq_setup - Allocate irq for gpio and setup appropriate functions
+ * @np: Device node of the GPIO chip
+ * @chip: Pointer to private gpio channel structure
+ *
+ * Return:
+ * 0 if success, otherwise -1
+ */
+static int xgpio_irq_setup(struct device_node *np, struct xgpio_instance *chip)
+{
+	u32 pin_num;
+	struct resource res;
+	int ret = of_irq_to_resource(np, 0, &res);
+
+	if (ret <= 0) {
+		pr_info("GPIO IRQ not connected\n");
+		return 0;
+	}
+
+	chip->gc.to_irq = xgpio_to_irq;
+	chip->irq_base = irq_alloc_descs(-1, 0, chip->gc.ngpio, 0);
+	if (chip->irq_base < 0) {
+		pr_err("Couldn't allocate IRQ numbers\n");
+		return -1;
+	}
+	chip->irq_domain = irq_domain_add_legacy(np, chip->gc.ngpio,
+						 chip->irq_base, 0,
+						 &irq_domain_simple_ops, NULL);
+	/*
+	 * set the irq chip, handler and irq chip data for callbacks for
+	 * each pin
+	 */
+	for (pin_num = 0; pin_num < chip->gc.ngpio; pin_num++) {
+		u32 gpio_irq = irq_find_mapping(chip->irq_domain, pin_num);
+
+		irq_set_lockdep_class(gpio_irq, &gpio_lock_class,
+				      &gpio_request_class);
+		pr_debug("IRQ Base: %d, Pin %d = IRQ %d\n",
+			 chip->irq_base, pin_num, gpio_irq);
+		irq_set_chip_and_handler(gpio_irq, &xgpio_irqchip,
+					 handle_simple_irq);
+		irq_set_chip_data(gpio_irq, (void *)chip);
+	}
+	irq_set_handler_data(res.start, (void *)chip);
+	irq_set_chained_handler(res.start, xgpio_irqhandler);
+
+	return 0;
+}
+
+/**
  * xgpio_of_probe - Probe method for the GPIO device.
  * @pdev: pointer to the platform device
  *
@@ -434,6 +656,15 @@ static int xgpio_probe(struct platform_device *pdev)
 		goto err_pm_put;
 	}
 
+	status = xgpio_irq_setup(np, chip);
+	if (status) {
+		pr_err("%s: GPIO IRQ initialization failed %d\n",
+		       np->full_name, status);
+		goto err_pm_put;
+	}
+	pr_info("XGpio: %s: registered, base is %d\n", np->full_name,
+		chip->gc.base);
+
 	pm_runtime_put(&pdev->dev);
 	return 0;
 err_pm_put:
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] gpio: xilinx: Add clock adaptation support
  2020-02-17 10:57 [PATCH 1/2] gpio: xilinx: Add clock adaptation support Srinivas Neeli
  2020-02-17 10:57 ` [PATCH 2/2] gpio: xilinx: Add irq support to the driver Srinivas Neeli
@ 2020-02-18 16:02 ` Bartosz Golaszewski
  2020-02-19 11:23   ` Srinivas Neeli
  1 sibling, 1 reply; 7+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 16:02 UTC (permalink / raw)
  To: Srinivas Neeli
  Cc: Michal Simek, shubhrajyoti.datta, sgoud, Linus Walleij, LKML,
	arm-soc, linux-gpio, git

pon., 17 lut 2020 o 11:57 Srinivas Neeli <srinivas.neeli@xilinx.com> napisał(a):
>
> Add support of clock adaptation for AXI GPIO driver.
>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
>  drivers/gpio/gpio-xilinx.c | 105 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 103 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index a9748b5198e6..26753ae58295 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -14,6 +14,8 @@
>  #include <linux/io.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/slab.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/clk.h>
>
>  /* Register Offset Definitions */
>  #define XGPIO_DATA_OFFSET   (0x0)      /* Data register  */
> @@ -38,6 +40,7 @@
>   * @gpio_state: GPIO state shadow register
>   * @gpio_dir: GPIO direction shadow register
>   * @gpio_lock: Lock used for synchronization
> + * @clk: clock resource for this driver
>   */
>  struct xgpio_instance {
>         struct gpio_chip gc;
> @@ -45,7 +48,8 @@ struct xgpio_instance {
>         unsigned int gpio_width[2];
>         u32 gpio_state[2];
>         u32 gpio_dir[2];
> -       spinlock_t gpio_lock[2];
> +       spinlock_t gpio_lock[2];        /* For serializing operations */
> +       struct clk *clk;
>  };
>
>  static inline int xgpio_index(struct xgpio_instance *chip, int gpio)
> @@ -255,6 +259,70 @@ static void xgpio_save_regs(struct xgpio_instance *chip)
>                        chip->gpio_dir[1]);
>  }
>
> +static int xgpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +       int ret = pm_runtime_get_sync(chip->parent);
> +
> +       /*
> +        * If the device is already active pm_runtime_get() will return 1 on
> +        * success, but gpio_request still needs to return 0.
> +        */
> +       return ret < 0 ? ret : 0;
> +}
> +
> +static void xgpio_free(struct gpio_chip *chip, unsigned int offset)
> +{
> +       pm_runtime_put(chip->parent);
> +}
> +
> +static int __maybe_unused xgpio_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       int irq = platform_get_irq(pdev, 0);
> +       struct irq_data *data = irq_get_irq_data(irq);
> +
> +       if (!irqd_is_wakeup_set(data))
> +               return pm_runtime_force_suspend(dev);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused xgpio_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       int irq = platform_get_irq(pdev, 0);
> +       struct irq_data *data = irq_get_irq_data(irq);
> +
> +       if (!irqd_is_wakeup_set(data))
> +               return pm_runtime_force_resume(dev);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused xgpio_runtime_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct xgpio_instance *gpio = platform_get_drvdata(pdev);
> +
> +       clk_disable(gpio->clk);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused xgpio_runtime_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct xgpio_instance *gpio = platform_get_drvdata(pdev);
> +
> +       return clk_enable(gpio->clk);
> +}
> +
> +static const struct dev_pm_ops xgpio_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(xgpio_suspend, xgpio_resume)
> +       SET_RUNTIME_PM_OPS(xgpio_runtime_suspend,
> +                          xgpio_runtime_resume, NULL)
> +};
> +
>  /**
>   * xgpio_of_probe - Probe method for the GPIO device.
>   * @pdev: pointer to the platform device
> @@ -323,6 +391,8 @@ static int xgpio_probe(struct platform_device *pdev)
>         chip->gc.direction_output = xgpio_dir_out;
>         chip->gc.get = xgpio_get;
>         chip->gc.set = xgpio_set;
> +       chip->gc.request = xgpio_request;
> +       chip->gc.free = xgpio_free;
>         chip->gc.set_multiple = xgpio_set_multiple;
>
>         chip->gc.label = dev_name(&pdev->dev);
> @@ -333,15 +403,45 @@ static int xgpio_probe(struct platform_device *pdev)
>                 return PTR_ERR(chip->regs);
>         }
>
> +       chip->clk = devm_clk_get(&pdev->dev, "s_axi_aclk");
> +       if (IS_ERR(chip->clk)) {
> +               if (PTR_ERR(chip->clk) != -ENOENT) {
> +                       if (PTR_ERR(chip->clk) != -EPROBE_DEFER)
> +                               dev_err(&pdev->dev, "Input clock not found\n");
> +                       return PTR_ERR(chip->clk);
> +               }
> +               /*
> +                * Clock framework support is optional, continue on
> +                * anyways if we don't find a matching clock.
> +                */

Why not use devm_clk_get_optional() then?

> +               chip->clk = NULL;
> +       }
> +       status = clk_prepare_enable(chip->clk);
> +       if (status < 0) {
> +               dev_err(&pdev->dev, "Failed to prepare clk\n");
> +               return status;
> +       }
> +       pm_runtime_enable(&pdev->dev);
> +       status = pm_runtime_get_sync(&pdev->dev);
> +       if (status < 0)
> +               goto err_unprepare_clk;
> +
>         xgpio_save_regs(chip);
>
>         status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
>         if (status) {
>                 dev_err(&pdev->dev, "failed to add GPIO chip\n");
> -               return status;
> +               goto err_pm_put;
>         }
>
> +       pm_runtime_put(&pdev->dev);
>         return 0;
> +err_pm_put:
> +       pm_runtime_put(&pdev->dev);
> +err_unprepare_clk:
> +       pm_runtime_disable(&pdev->dev);
> +       clk_unprepare(chip->clk);
> +       return status;
>  }
>
>  static const struct of_device_id xgpio_of_match[] = {
> @@ -356,6 +456,7 @@ static struct platform_driver xgpio_plat_driver = {
>         .driver         = {
>                         .name = "gpio-xilinx",
>                         .of_match_table = xgpio_of_match,
> +                       .pm = &xgpio_dev_pm_ops,
>         },
>  };
>
> --
> 2.7.4
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH 1/2] gpio: xilinx: Add clock adaptation support
  2020-02-18 16:02 ` [PATCH 1/2] gpio: xilinx: Add clock adaptation support Bartosz Golaszewski
@ 2020-02-19 11:23   ` Srinivas Neeli
  0 siblings, 0 replies; 7+ messages in thread
From: Srinivas Neeli @ 2020-02-19 11:23 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Michal Simek, Shubhrajyoti Datta, Srinivas Goud, Linus Walleij,
	LKML, arm-soc, linux-gpio, git

Hi,
I agree ,will address comments in V2.

> -----Original Message-----
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Sent: Tuesday, February 18, 2020 9:33 PM
> To: Srinivas Neeli <sneeli@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; Linus Walleij
> <linus.walleij@linaro.org>; LKML <linux-kernel@vger.kernel.org>; arm-soc
> <linux-arm-kernel@lists.infradead.org>; linux-gpio <linux-
> gpio@vger.kernel.org>; git <git@xilinx.com>
> Subject: Re: [PATCH 1/2] gpio: xilinx: Add clock adaptation support
> 
> pon., 17 lut 2020 o 11:57 Srinivas Neeli <srinivas.neeli@xilinx.com> napisał(a):
> >
> > Add support of clock adaptation for AXI GPIO driver.
> >
> > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> > ---
> >  drivers/gpio/gpio-xilinx.c | 105
> > ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 103 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> > index a9748b5198e6..26753ae58295 100644
> > --- a/drivers/gpio/gpio-xilinx.c
> > +++ b/drivers/gpio/gpio-xilinx.c
> > @@ -14,6 +14,8 @@
> >  #include <linux/io.h>
> >  #include <linux/gpio/driver.h>
> >  #include <linux/slab.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/clk.h>
> >
> >  /* Register Offset Definitions */
> >  #define XGPIO_DATA_OFFSET   (0x0)      /* Data register  */
> > @@ -38,6 +40,7 @@
> >   * @gpio_state: GPIO state shadow register
> >   * @gpio_dir: GPIO direction shadow register
> >   * @gpio_lock: Lock used for synchronization
> > + * @clk: clock resource for this driver
> >   */
> >  struct xgpio_instance {
> >         struct gpio_chip gc;
> > @@ -45,7 +48,8 @@ struct xgpio_instance {
> >         unsigned int gpio_width[2];
> >         u32 gpio_state[2];
> >         u32 gpio_dir[2];
> > -       spinlock_t gpio_lock[2];
> > +       spinlock_t gpio_lock[2];        /* For serializing operations */
> > +       struct clk *clk;
> >  };
> >
> >  static inline int xgpio_index(struct xgpio_instance *chip, int gpio)
> > @@ -255,6 +259,70 @@ static void xgpio_save_regs(struct xgpio_instance
> *chip)
> >                        chip->gpio_dir[1]);  }
> >
> > +static int xgpio_request(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       int ret = pm_runtime_get_sync(chip->parent);
> > +
> > +       /*
> > +        * If the device is already active pm_runtime_get() will return 1 on
> > +        * success, but gpio_request still needs to return 0.
> > +        */
> > +       return ret < 0 ? ret : 0;
> > +}
> > +
> > +static void xgpio_free(struct gpio_chip *chip, unsigned int offset) {
> > +       pm_runtime_put(chip->parent);
> > +}
> > +
> > +static int __maybe_unused xgpio_suspend(struct device *dev) {
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       int irq = platform_get_irq(pdev, 0);
> > +       struct irq_data *data = irq_get_irq_data(irq);
> > +
> > +       if (!irqd_is_wakeup_set(data))
> > +               return pm_runtime_force_suspend(dev);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused xgpio_resume(struct device *dev) {
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       int irq = platform_get_irq(pdev, 0);
> > +       struct irq_data *data = irq_get_irq_data(irq);
> > +
> > +       if (!irqd_is_wakeup_set(data))
> > +               return pm_runtime_force_resume(dev);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused xgpio_runtime_suspend(struct device *dev) {
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct xgpio_instance *gpio = platform_get_drvdata(pdev);
> > +
> > +       clk_disable(gpio->clk);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused xgpio_runtime_resume(struct device *dev) {
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct xgpio_instance *gpio = platform_get_drvdata(pdev);
> > +
> > +       return clk_enable(gpio->clk);
> > +}
> > +
> > +static const struct dev_pm_ops xgpio_dev_pm_ops = {
> > +       SET_SYSTEM_SLEEP_PM_OPS(xgpio_suspend, xgpio_resume)
> > +       SET_RUNTIME_PM_OPS(xgpio_runtime_suspend,
> > +                          xgpio_runtime_resume, NULL) };
> > +
> >  /**
> >   * xgpio_of_probe - Probe method for the GPIO device.
> >   * @pdev: pointer to the platform device @@ -323,6 +391,8 @@ static
> > int xgpio_probe(struct platform_device *pdev)
> >         chip->gc.direction_output = xgpio_dir_out;
> >         chip->gc.get = xgpio_get;
> >         chip->gc.set = xgpio_set;
> > +       chip->gc.request = xgpio_request;
> > +       chip->gc.free = xgpio_free;
> >         chip->gc.set_multiple = xgpio_set_multiple;
> >
> >         chip->gc.label = dev_name(&pdev->dev); @@ -333,15 +403,45 @@
> > static int xgpio_probe(struct platform_device *pdev)
> >                 return PTR_ERR(chip->regs);
> >         }
> >
> > +       chip->clk = devm_clk_get(&pdev->dev, "s_axi_aclk");
> > +       if (IS_ERR(chip->clk)) {
> > +               if (PTR_ERR(chip->clk) != -ENOENT) {
> > +                       if (PTR_ERR(chip->clk) != -EPROBE_DEFER)
> > +                               dev_err(&pdev->dev, "Input clock not found\n");
> > +                       return PTR_ERR(chip->clk);
> > +               }
> > +               /*
> > +                * Clock framework support is optional, continue on
> > +                * anyways if we don't find a matching clock.
> > +                */
> 
> Why not use devm_clk_get_optional() then?
> 
> > +               chip->clk = NULL;
> > +       }
> > +       status = clk_prepare_enable(chip->clk);
> > +       if (status < 0) {
> > +               dev_err(&pdev->dev, "Failed to prepare clk\n");
> > +               return status;
> > +       }
> > +       pm_runtime_enable(&pdev->dev);
> > +       status = pm_runtime_get_sync(&pdev->dev);
> > +       if (status < 0)
> > +               goto err_unprepare_clk;
> > +
> >         xgpio_save_regs(chip);
> >
> >         status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
> >         if (status) {
> >                 dev_err(&pdev->dev, "failed to add GPIO chip\n");
> > -               return status;
> > +               goto err_pm_put;
> >         }
> >
> > +       pm_runtime_put(&pdev->dev);
> >         return 0;
> > +err_pm_put:
> > +       pm_runtime_put(&pdev->dev);
> > +err_unprepare_clk:
> > +       pm_runtime_disable(&pdev->dev);
> > +       clk_unprepare(chip->clk);
> > +       return status;
> >  }
> >
> >  static const struct of_device_id xgpio_of_match[] = { @@ -356,6
> > +456,7 @@ static struct platform_driver xgpio_plat_driver = {
> >         .driver         = {
> >                         .name = "gpio-xilinx",
> >                         .of_match_table = xgpio_of_match,
> > +                       .pm = &xgpio_dev_pm_ops,
> >         },
> >  };
> >
> > --
> > 2.7.4
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] gpio: xilinx: Add irq support to the driver
  2020-02-17 10:57 ` [PATCH 2/2] gpio: xilinx: Add irq support to the driver Srinivas Neeli
@ 2020-02-19 12:39   ` kbuild test robot
  2020-02-19 15:00   ` kbuild test robot
  2020-04-29 13:56   ` Daniel Mack
  2 siblings, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2020-02-19 12:39 UTC (permalink / raw)
  To: Srinivas Neeli
  Cc: kbuild-all, bgolaszewski, michal.simek, shubhrajyoti.datta,
	sgoud, linus.walleij, linux-kernel, linux-arm-kernel, linux-gpio,
	git

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

Hi Srinivas,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.6-rc2]
[also build test ERROR on next-20200219]
[cannot apply to xlnx/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Srinivas-Neeli/gpio-xilinx-Add-clock-adaptation-support/20200219-110158
base:    11a48a5a18c63fd7621bb050228cebf13566e4d8
config: x86_64-randconfig-b001-20200219 (attached as .config)
compiler: gcc-7 (Debian 7.5.0-5) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

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

All errors (new ones prefixed by >>):

>> ERROR: "of_irq_to_resource" [drivers/gpio/gpio-xilinx.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35445 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] gpio: xilinx: Add irq support to the driver
  2020-02-17 10:57 ` [PATCH 2/2] gpio: xilinx: Add irq support to the driver Srinivas Neeli
  2020-02-19 12:39   ` kbuild test robot
@ 2020-02-19 15:00   ` kbuild test robot
  2020-04-29 13:56   ` Daniel Mack
  2 siblings, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2020-02-19 15:00 UTC (permalink / raw)
  To: Srinivas Neeli
  Cc: kbuild-all, bgolaszewski, michal.simek, shubhrajyoti.datta,
	sgoud, linus.walleij, linux-kernel, linux-arm-kernel, linux-gpio,
	git

[-- Attachment #1: Type: text/plain, Size: 2939 bytes --]

Hi Srinivas,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.6-rc2]
[also build test ERROR on next-20200219]
[cannot apply to xlnx/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Srinivas-Neeli/gpio-xilinx-Add-clock-adaptation-support/20200219-110158
base:    11a48a5a18c63fd7621bb050228cebf13566e4d8
config: i386-randconfig-b003-20200219 (attached as .config)
compiler: gcc-7 (Debian 7.5.0-5) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

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

All errors (new ones prefixed by >>):

   ld: drivers/gpio/gpio-xilinx.o: in function `xgpio_irq_setup':
>> drivers/gpio/gpio-xilinx.c:512: undefined reference to `of_irq_to_resource'

vim +512 drivers/gpio/gpio-xilinx.c

   499	
   500	/**
   501	 * xgpio_irq_setup - Allocate irq for gpio and setup appropriate functions
   502	 * @np: Device node of the GPIO chip
   503	 * @chip: Pointer to private gpio channel structure
   504	 *
   505	 * Return:
   506	 * 0 if success, otherwise -1
   507	 */
   508	static int xgpio_irq_setup(struct device_node *np, struct xgpio_instance *chip)
   509	{
   510		u32 pin_num;
   511		struct resource res;
 > 512		int ret = of_irq_to_resource(np, 0, &res);
   513	
   514		if (ret <= 0) {
   515			pr_info("GPIO IRQ not connected\n");
   516			return 0;
   517		}
   518	
   519		chip->gc.to_irq = xgpio_to_irq;
   520		chip->irq_base = irq_alloc_descs(-1, 0, chip->gc.ngpio, 0);
   521		if (chip->irq_base < 0) {
   522			pr_err("Couldn't allocate IRQ numbers\n");
   523			return -1;
   524		}
   525		chip->irq_domain = irq_domain_add_legacy(np, chip->gc.ngpio,
   526							 chip->irq_base, 0,
   527							 &irq_domain_simple_ops, NULL);
   528		/*
   529		 * set the irq chip, handler and irq chip data for callbacks for
   530		 * each pin
   531		 */
   532		for (pin_num = 0; pin_num < chip->gc.ngpio; pin_num++) {
   533			u32 gpio_irq = irq_find_mapping(chip->irq_domain, pin_num);
   534	
   535			irq_set_lockdep_class(gpio_irq, &gpio_lock_class,
   536					      &gpio_request_class);
   537			pr_debug("IRQ Base: %d, Pin %d = IRQ %d\n",
   538				 chip->irq_base, pin_num, gpio_irq);
   539			irq_set_chip_and_handler(gpio_irq, &xgpio_irqchip,
   540						 handle_simple_irq);
   541			irq_set_chip_data(gpio_irq, (void *)chip);
   542		}
   543		irq_set_handler_data(res.start, (void *)chip);
   544		irq_set_chained_handler(res.start, xgpio_irqhandler);
   545	
   546		return 0;
   547	}
   548	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37146 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] gpio: xilinx: Add irq support to the driver
  2020-02-17 10:57 ` [PATCH 2/2] gpio: xilinx: Add irq support to the driver Srinivas Neeli
  2020-02-19 12:39   ` kbuild test robot
  2020-02-19 15:00   ` kbuild test robot
@ 2020-04-29 13:56   ` Daniel Mack
  2 siblings, 0 replies; 7+ messages in thread
From: Daniel Mack @ 2020-04-29 13:56 UTC (permalink / raw)
  To: Srinivas Neeli, bgolaszewski, michal.simek, shubhrajyoti.datta, sgoud
  Cc: linus.walleij, linux-kernel, linux-arm-kernel, linux-gpio, git

Hi Srinivas,

Thanks for these patches. We're using them on a custom board, and I have
some remarks as they didn't work as intended. See below.


On 2/17/20 11:57 AM, Srinivas Neeli wrote:
> Allocate single chip for both channels.
> Add irq support to the driver.
> Supporting rising edge interrupts and in cascade mode supporting
> first channel for interrupts on 32bit machines.
> 
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
>  drivers/gpio/gpio-xilinx.c | 233 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 232 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index 26753ae58295..f6dd316b2c62 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c

[...]

>  /**
> + * xgpiops_irq_mask - Write the specified signal of the GPIO device.
> + * @irq_data: per irq and chip data passed down to chip functions
> + */
> +static void xgpio_irq_mask(struct irq_data *irq_data)
> +{
> +	unsigned long flags;
> +	struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
> +	u32 offset = irq_data->irq - chip->irq_base;
> +	u32 temp;
> +	s32 val;
> +	int index = xgpio_index(chip, 0);
> +
> +	pr_debug("%s: Disable %d irq, irq_enable_mask 0x%x\n",
> +		 __func__, offset, chip->irq_enable);
> +
> +	spin_lock_irqsave(&chip->gpio_lock[index], flags);
> +
> +	chip->irq_enable &= ~BIT(offset);
> +
> +	if (!chip->irq_enable) {
> +		/* Enable per channel interrupt */
> +		temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
> +		val = offset - chip->gpio_width[0] + 1;
> +		if (val > 0)
> +			temp &= 1;
> +		else
> +			temp &= 2;


This is a bit confusing. Why not write

  if (offset <= chip->gpio_width[0])
	temp &= 1;
  else
        temp &= 2;

?

> +		xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
> +
> +		/* Disable global interrupt if channel interrupts are unused */
> +		temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);

You know that interrupts are unused when you get here, right? Why this
extra check?

> +		if (!temp)
> +			xgpio_writereg(chip->regs + XGPIO_GIER_OFFSET,
> +				       ~XGPIO_GIER_IE);
> +	}
> +	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
> +}
> +
> +/**
> + * xgpio_irq_unmask - Write the specified signal of the GPIO device.
> + * @irq_data: per irq and chip data passed down to chip functions
> + */
> +static void xgpio_irq_unmask(struct irq_data *irq_data)
> +{
> +	unsigned long flags;
> +	struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
> +	u32 offset = irq_data->irq - chip->irq_base;
> +	u32 temp;
> +	s32 val;
> +	int index = xgpio_index(chip, 0);
> +
> +	pr_debug("%s: Enable %d irq, irq_enable_mask 0x%x\n",
> +		 __func__, offset, chip->irq_enable);
> +
> +	/* Setup pin as input */
> +	xgpio_dir_in(&chip->gc, offset);
> +
> +	spin_lock_irqsave(&chip->gpio_lock[index], flags);
> +
> +	chip->irq_enable |= BIT(offset);
> +
> +	if (chip->irq_enable) {

As you set a bit in the instruction above, this condition will always be
true. So I guess the check can be omitted.

> +		/* Enable per channel interrupt */
> +		temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
> +		val = offset - (chip->gpio_width[0] - 1);

This is different from the the statement in the mask function, but it
can be simplified as noted above.

> +		if (val > 0)
> +			temp |= 2;
> +		else
> +			temp |= 1;
> +		xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
> +
> +		/* Enable global interrupts */
> +		xgpio_writereg(chip->regs + XGPIO_GIER_OFFSET, XGPIO_GIER_IE);
> +	}
> +
> +	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
> +}

[...]

> +/**
> + * xgpio_irqhandler - Gpio interrupt service routine
> + * @desc: Pointer to interrupt description
> + */
> +static void xgpio_irqhandler(struct irq_desc *desc)
> +{
> +	unsigned int irq = irq_desc_get_irq(desc);
> +	struct xgpio_instance *chip = (struct xgpio_instance *)
> +		irq_get_handler_data(irq);
> +	struct irq_chip *irqchip = irq_desc_get_chip(desc);
> +	u32 offset, status, channel = 1;
> +	unsigned long val;
> +
> +	chained_irq_enter(irqchip, desc);
> +
> +	val = xgpio_readreg(chip->regs);
> +	if (!val) {
> +		channel = 2;
> +		val = xgpio_readreg(chip->regs + XGPIO_CHANNEL_OFFSET);
> +		val = val << chip->gpio_width[0];
> +	}
> +
> +	/* Only rising edge is supported */
> +	val &= chip->irq_enable;
> +	for_each_set_bit(offset, &val, chip->gc.ngpio) {
> +		generic_handle_irq(chip->irq_base + offset);

This needs to include irq_find_mapping(chip->irq_domain, gpio).

> +	}
> +
> +	status = xgpio_readreg(chip->regs + XGPIO_IPISR_OFFSET);

The value assigned here is not used. Typo?

> +	xgpio_writereg(chip->regs + XGPIO_IPISR_OFFSET, channel);

This function causes issues of general IRQ handling that makes the
entire system deadlock for reasons I don't fully grok. I changed the
logic to the following to make it work:

1. Read IPISR
2. Write the read value back to IPISR
3. Depending on the value of IPISR, read the state of either channel 1
   or 2
4. chained_irq_enter()
5. Iterate over bits and call generic_handle_irq()
6. chained_irq_exit()

> +
> +	chained_irq_exit(irqchip, desc);
> +}
> +
> +static struct lock_class_key gpio_lock_class;
> +static struct lock_class_key gpio_request_class;
> +
> +/**
> + * xgpio_irq_setup - Allocate irq for gpio and setup appropriate functions
> + * @np: Device node of the GPIO chip
> + * @chip: Pointer to private gpio channel structure
> + *
> + * Return:
> + * 0 if success, otherwise -1
> + */
> +static int xgpio_irq_setup(struct device_node *np, struct xgpio_instance *chip)
> +{
> +	u32 pin_num;
> +	struct resource res;
> +	int ret = of_irq_to_resource(np, 0, &res);
> +
> +	if (ret <= 0) {
> +		pr_info("GPIO IRQ not connected\n");
> +		return 0;
> +	}
> +
> +	chip->gc.to_irq = xgpio_to_irq;
> +	chip->irq_base = irq_alloc_descs(-1, 0, chip->gc.ngpio, 0);

This should use the devm_ variant to automatically free the resources.

> +	if (chip->irq_base < 0) {
> +		pr_err("Couldn't allocate IRQ numbers\n");
> +		return -1;
> +	}
> +	chip->irq_domain = irq_domain_add_legacy(np, chip->gc.ngpio,
> +						 chip->irq_base, 0,
> +						 &irq_domain_simple_ops, NULL);

This can fail, so the return value should be checked for NULL.

> +	/*
> +	 * set the irq chip, handler and irq chip data for callbacks for
> +	 * each pin
> +	 */
> +	for (pin_num = 0; pin_num < chip->gc.ngpio; pin_num++) {
> +		u32 gpio_irq = irq_find_mapping(chip->irq_domain, pin_num);
> +
> +		irq_set_lockdep_class(gpio_irq, &gpio_lock_class,
> +				      &gpio_request_class);
> +		pr_debug("IRQ Base: %d, Pin %d = IRQ %d\n",
> +			 chip->irq_base, pin_num, gpio_irq);
> +		irq_set_chip_and_handler(gpio_irq, &xgpio_irqchip,
> +					 handle_simple_irq);
> +		irq_set_chip_data(gpio_irq, (void *)chip);
> +	}
> +	irq_set_handler_data(res.start, (void *)chip);
> +	irq_set_chained_handler(res.start, xgpio_irqhandler);

I guess all this can be achieved by setting chip->gc.irq* and let the
GPIO core handle the IRQ chip allocation and setup. There are some
examples in Documentation/driver-api/gpio/driver.rst.

I'm happy to test the next iteration of these patches.


Thanks,
Daniel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-04-29 14:02 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-17 10:57 [PATCH 1/2] gpio: xilinx: Add clock adaptation support Srinivas Neeli
2020-02-17 10:57 ` [PATCH 2/2] gpio: xilinx: Add irq support to the driver Srinivas Neeli
2020-02-19 12:39   ` kbuild test robot
2020-02-19 15:00   ` kbuild test robot
2020-04-29 13:56   ` Daniel Mack
2020-02-18 16:02 ` [PATCH 1/2] gpio: xilinx: Add clock adaptation support Bartosz Golaszewski
2020-02-19 11:23   ` Srinivas Neeli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).