[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Harini Katakam]

Add support for GPIO controller used by Xilinx Zynq

Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
 drivers/gpio/Kconfig     |    7 +
 drivers/gpio/Makefile    |    1 +
 drivers/gpio/gpio-zynq.c |  690 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 698 insertions(+)
 create mode 100644 drivers/gpio/gpio-zynq.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 903f24d..67a22a6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -313,6 +313,13 @@ config GPIO_XTENSA
 	  Say yes here to support the Xtensa internal GPIO32 IMPWIRE (input)
 	  and EXPSTATE (output) ports
 
+config GPIO_ZYNQ
+	bool "Xilinx ZYNQ GPIO support"
+	depends on ARCH_ZYNQ
+	select GENERIC_IRQ_CHIP
+	help
+	 Say yes here to support Xilinx ZYNQ GPIO controller.
+
 config GPIO_VR41XX
 	tristate "NEC VR4100 series General-purpose I/O Uint support"
 	depends on CPU_VR41XX
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 5d50179..439f23a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -99,3 +99,4 @@ obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
 obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
 obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
 obj-$(CONFIG_GPIO_XTENSA)	+= gpio-xtensa.o
+obj-$(CONFIG_GPIO_ZYNQ)		+= gpio-zynq.o
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
new file mode 100644
index 0000000..1f5fdfc
--- /dev/null
+++ b/drivers/gpio/gpio-zynq.c
@@ -0,0 +1,690 @@
+/*
+ * Xilinx Zynq GPIO device driver
+ *
+ * Copyright (C) 2009 - 2014 Xilinx, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License as published by the Free Software
+ * Foundation; either version 2 of the License, or (at your option) any later
+ * version.
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#define DRIVER_NAME "zynq-gpio"
+#define ZYNQ_GPIO_NR_GPIOS	118
+
+static struct irq_domain *irq_domain;
+
+/* Register offsets for the GPIO device */
+
+/* LSW Mask & Data -WO */
+#define ZYNQ_GPIO_DATA_LSW_OFFSET(BANK)	(0x000 + (8 * BANK))
+/* MSW Mask & Data -WO */
+#define ZYNQ_GPIO_DATA_MSW_OFFSET(BANK)	(0x004 + (8 * BANK))
+/* Data Register-RW */
+#define ZYNQ_GPIO_DATA_OFFSET(BANK)	(0x040 + (4 * BANK))
+/* Direction mode reg-RW */
+#define ZYNQ_GPIO_DIRM_OFFSET(BANK)	(0x204 + (0x40 * BANK))
+/* Output enable reg-RW */
+#define ZYNQ_GPIO_OUTEN_OFFSET(BANK)	(0x208 + (0x40 * BANK))
+/* Interrupt mask reg-RO */
+#define ZYNQ_GPIO_INTMASK_OFFSET(BANK)	(0x20C + (0x40 * BANK))
+/* Interrupt enable reg-WO */
+#define ZYNQ_GPIO_INTEN_OFFSET(BANK)	(0x210 + (0x40 * BANK))
+/* Interrupt disable reg-WO */
+#define ZYNQ_GPIO_INTDIS_OFFSET(BANK)	(0x214 + (0x40 * BANK))
+/* Interrupt status reg-RO */
+#define ZYNQ_GPIO_INTSTS_OFFSET(BANK)	(0x218 + (0x40 * BANK))
+/* Interrupt type reg-RW */
+#define ZYNQ_GPIO_INTTYPE_OFFSET(BANK)	(0x21C + (0x40 * BANK))
+/* Interrupt polarity reg-RW */
+#define ZYNQ_GPIO_INTPOL_OFFSET(BANK)	(0x220 + (0x40 * BANK))
+/* Interrupt on any, reg-RW */
+#define ZYNQ_GPIO_INTANY_OFFSET(BANK)	(0x224 + (0x40 * BANK))
+
+/* Read/Write access to the GPIO PS registers */
+static inline u32 zynq_gpio_readreg(void __iomem *offset)
+{
+	return readl_relaxed(offset);
+}
+
+static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
+{
+	writel_relaxed(val, offset);
+}
+
+static unsigned int zynq_gpio_pin_table[] = {
+	31, /* 0 - 31 */
+	53, /* 32 - 53 */
+	85, /* 54 - 85 */
+	117 /* 86 - 117 */
+};
+
+/* Maximum banks */
+#define ZYNQ_GPIO_MAX_BANK	4
+
+/* Disable all interrupts mask */
+#define ZYNQ_GPIO_IXR_DISABLE_ALL	0xFFFFFFFF
+
+/* GPIO pin high */
+#define ZYNQ_GPIO_PIN_HIGH 1
+
+/* Mid pin number of a bank */
+#define ZYNQ_GPIO_MID_PIN_NUM 16
+
+/* GPIO upper 16 bit mask */
+#define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000
+
+/**
+ * struct zynq_gpio - gpio device private data structure
+ * @chip:	instance of the gpio_chip
+ * @base_addr:	base address of the GPIO device
+ * @irq:	irq associated with the controller
+ * @irq_base:	base of IRQ number for interrupt
+ * @clk:	clock resource for this controller
+ */
+struct zynq_gpio {
+	struct gpio_chip chip;
+	void __iomem *base_addr;
+	unsigned int irq;
+	unsigned int irq_base;
+	struct clk *clk;
+};
+
+/**
+ * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
+ * for a given pin in the GPIO device
+ * @pin_num:	gpio pin number within the device
+ * @bank_num:	an output parameter used to return the bank number of the gpio
+ *		pin
+ * @bank_pin_num: an output parameter used to return pin number within a bank
+ *		  for the given gpio pin
+ *
+ * Returns the bank number.
+ */
+static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
+					  unsigned int *bank_num,
+					  unsigned int *bank_pin_num)
+{
+	for (*bank_num = 0; *bank_num < ZYNQ_GPIO_MAX_BANK; (*bank_num)++)
+		if (pin_num <= zynq_gpio_pin_table[*bank_num])
+			break;
+
+	if (!(*bank_num))
+		*bank_pin_num = pin_num;
+	else
+		*bank_pin_num = pin_num %
+				(zynq_gpio_pin_table[*bank_num - 1] + 1);
+}
+
+/**
+ * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ *
+ * This function reads the state of the specified pin of the GPIO device.
+ *
+ * Return: 0 if the pin is low, 1 if pin is high.
+ */
+static int zynq_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
+{
+	unsigned int bank_num, bank_pin_num, data;
+	struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+
+	zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+
+	data = zynq_gpio_readreg(gpio->base_addr +
+				 ZYNQ_GPIO_DATA_OFFSET(bank_num));
+	return (data >> bank_pin_num) & ZYNQ_GPIO_PIN_HIGH;
+}
+
+/**
+ * zynq_gpio_set_value - Modify the state of the pin with specified value
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ * @state:	value used to modify the state of the specified pin
+ *
+ * This function calculates the register offset (i.e to lower 16 bits or
+ * upper 16 bits) based on the given pin number and sets the state of a
+ * gpio pin to the specified value. The state is either 0 or non-zero.
+ */
+static void zynq_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
+				int state)
+{
+	unsigned int reg_offset, bank_num, bank_pin_num;
+	struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+
+	zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+
+	if (bank_pin_num >= ZYNQ_GPIO_MID_PIN_NUM) {
+		/* only 16 data bits in bit maskable reg */
+		bank_pin_num -= ZYNQ_GPIO_MID_PIN_NUM;
+		reg_offset = ZYNQ_GPIO_DATA_MSW_OFFSET(bank_num);
+	} else {
+		reg_offset = ZYNQ_GPIO_DATA_LSW_OFFSET(bank_num);
+	}
+
+	/*
+	 * get the 32 bit value to be written to the mask/data register where
+	 * the upper 16 bits is the mask and lower 16 bits is the data
+	 */
+	if (state)
+		state = 1;
+	state = ~(1 << (bank_pin_num + ZYNQ_GPIO_MID_PIN_NUM)) &
+		((state << bank_pin_num) | ZYNQ_GPIO_UPPER_MASK);
+
+	zynq_gpio_writereg(gpio->base_addr + reg_offset, state);
+}
+
+/**
+ * zynq_gpio_dir_in - Set the direction of the specified GPIO pin as input
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ *
+ * This function uses the read-modify-write sequence to set the direction of
+ * the gpio pin as input.
+ *
+ * Return: 0 always
+ */
+static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
+{
+	unsigned int reg, bank_num, bank_pin_num;
+	struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+
+	zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+	/* clear the bit in direction mode reg to set the pin as input */
+	reg = zynq_gpio_readreg(gpio->base_addr +
+				ZYNQ_GPIO_DIRM_OFFSET(bank_num));
+	reg &= ~(1 << bank_pin_num);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num),
+			   reg);
+
+	return 0;
+}
+
+/**
+ * zynq_gpio_dir_out - Set the direction of the specified GPIO pin as output
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ * @state:	value to be written to specified pin
+ *
+ * This function sets the direction of specified GPIO pin as output, configures
+ * the Output Enable register for the pin and uses zynq_gpio_set to set
+ * the state of the pin to the value specified.
+ *
+ * Return: 0 always
+ */
+static int zynq_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
+			     int state)
+{
+	struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+	unsigned int reg, bank_num, bank_pin_num;
+
+	zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+
+	/* set the GPIO pin as output */
+	reg = zynq_gpio_readreg(gpio->base_addr +
+				ZYNQ_GPIO_DIRM_OFFSET(bank_num));
+	reg |= 1 << bank_pin_num;
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num),
+			   reg);
+
+	/* configure the output enable reg for the pin */
+	reg = zynq_gpio_readreg(gpio->base_addr +
+				ZYNQ_GPIO_OUTEN_OFFSET(bank_num));
+	reg |= 1 << bank_pin_num;
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_OUTEN_OFFSET(bank_num),
+			   reg);
+
+	/* set the state of the pin */
+	zynq_gpio_set_value(chip, pin, state);
+	return 0;
+}
+
+static int zynq_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	return irq_find_mapping(irq_domain, offset);
+}
+
+/**
+ * zynq_gpio_irq_ack - Acknowledge the interrupt of a gpio pin
+ * @irq_data:	irq data containing irq number of gpio pin for the interrupt
+ *		to ack
+ *
+ * This function calculates gpio pin number from irq number and sets the bit
+ * in the Interrupt Status Register of the corresponding bank, to ACK the irq.
+ */
+static void zynq_gpio_irq_ack(struct irq_data *irq_data)
+{
+	struct zynq_gpio *gpio = (struct zynq_gpio *)
+				 irq_data_get_irq_chip_data(irq_data);
+	unsigned int device_pin_num, bank_num, bank_pin_num;
+
+	device_pin_num = irq_data->hwirq;
+	zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTSTS_OFFSET(bank_num),
+			   1 << bank_pin_num);
+}
+
+/**
+ * zynq_gpio_irq_mask - Disable the interrupts for a gpio pin
+ * @irq_data:	per irq and chip data passed down to chip functions
+ *
+ * This function calculates gpio pin number from irq number and sets the
+ * bit in the Interrupt Disable register of the corresponding bank to disable
+ * interrupts for that pin.
+ */
+static void zynq_gpio_irq_mask(struct irq_data *irq_data)
+{
+	struct zynq_gpio *gpio = (struct zynq_gpio *)
+				 irq_data_get_irq_chip_data(irq_data);
+	unsigned int device_pin_num, bank_num, bank_pin_num;
+
+	device_pin_num = irq_data->hwirq;
+	zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
+			   1 << bank_pin_num);
+}
+
+/**
+ * zynq_gpio_irq_unmask - Enable the interrupts for a gpio pin
+ * @irq_data:	irq data containing irq number of gpio pin for the interrupt
+ *		to enable
+ *
+ * This function calculates the gpio pin number from irq number and sets the
+ * bit in the Interrupt Enable register of the corresponding bank to enable
+ * interrupts for that pin.
+ */
+static void zynq_gpio_irq_unmask(struct irq_data *irq_data)
+{
+	struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
+	unsigned int device_pin_num, bank_num, bank_pin_num;
+
+	device_pin_num = irq_data->hwirq;
+	zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTEN_OFFSET(bank_num),
+			   1 << bank_pin_num);
+}
+
+/**
+ * zynq_gpio_set_irq_type - Set the irq type for a gpio pin
+ * @irq_data:	irq data containing irq number of gpio pin
+ * @type:	interrupt type that is to be set for the gpio pin
+ *
+ * This function gets the gpio pin number and its bank from the gpio pin number
+ * and configures the INT_TYPE, INT_POLARITY and INT_ANY registers.
+ *
+ * Return: 0, negative error otherwise.
+ * TYPE-EDGE_RISING,  INT_TYPE - 1, INT_POLARITY - 1,  INT_ANY - 0;
+ * TYPE-EDGE_FALLING, INT_TYPE - 1, INT_POLARITY - 0,  INT_ANY - 0;
+ * TYPE-EDGE_BOTH,    INT_TYPE - 1, INT_POLARITY - NA, INT_ANY - 1;
+ * TYPE-LEVEL_HIGH,   INT_TYPE - 0, INT_POLARITY - 1,  INT_ANY - NA;
+ * TYPE-LEVEL_LOW,    INT_TYPE - 0, INT_POLARITY - 0,  INT_ANY - NA
+ */
+static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
+{
+	struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
+	unsigned int device_pin_num, bank_num, bank_pin_num;
+	unsigned int int_type, int_pol, int_any;
+
+	device_pin_num = irq_data->hwirq;
+	zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+
+	int_type = zynq_gpio_readreg(gpio->base_addr +
+				     ZYNQ_GPIO_INTTYPE_OFFSET(bank_num));
+	int_pol = zynq_gpio_readreg(gpio->base_addr +
+				    ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
+	int_any = zynq_gpio_readreg(gpio->base_addr +
+				    ZYNQ_GPIO_INTANY_OFFSET(bank_num));
+
+	/*
+	 * based on the type requested, configure the INT_TYPE, INT_POLARITY
+	 * and INT_ANY registers
+	 */
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		int_type |= (1 << bank_pin_num);
+		int_pol |= (1 << bank_pin_num);
+		int_any &= ~(1 << bank_pin_num);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		int_type |= (1 << bank_pin_num);
+		int_pol &= ~(1 << bank_pin_num);
+		int_any &= ~(1 << bank_pin_num);
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		int_type |= (1 << bank_pin_num);
+		int_any |= (1 << bank_pin_num);
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		int_type &= ~(1 << bank_pin_num);
+		int_pol |= (1 << bank_pin_num);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		int_type &= ~(1 << bank_pin_num);
+		int_pol &= ~(1 << bank_pin_num);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	zynq_gpio_writereg(gpio->base_addr +
+			   ZYNQ_GPIO_INTTYPE_OFFSET(bank_num), int_type);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTPOL_OFFSET(bank_num),
+			   int_pol);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTANY_OFFSET(bank_num),
+			   int_any);
+	return 0;
+}
+
+static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
+{
+	if (on)
+		zynq_gpio_irq_unmask(data);
+	else
+		zynq_gpio_irq_mask(data);
+
+	return 0;
+}
+
+/* irq chip descriptor */
+static struct irq_chip zynq_gpio_irqchip = {
+	.name		= DRIVER_NAME,
+	.irq_ack	= zynq_gpio_irq_ack,
+	.irq_mask	= zynq_gpio_irq_mask,
+	.irq_unmask	= zynq_gpio_irq_unmask,
+	.irq_set_type	= zynq_gpio_set_irq_type,
+	.irq_set_wake	= zynq_gpio_set_wake,
+};
+
+/**
+ * zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
+ * @irq:	irq number of the gpio bank where interrupt has occurred
+ * @desc:	irq descriptor instance of the 'irq'
+ *
+ * This function reads the Interrupt Status Register of each bank to get the
+ * gpio pin number which has triggered an interrupt. It then acks the triggered
+ * interrupt and calls the pin specific handler set by the higher layer
+ * application for that pin.
+ * Note: A bug is reported if no handler is set for the gpio pin.
+ */
+static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
+{
+	struct zynq_gpio *gpio = (struct zynq_gpio *)irq_get_handler_data(irq);
+	int gpio_irq = gpio->irq_base;
+	unsigned int int_sts, int_enb, bank_num;
+	struct irq_desc *gpio_irq_desc;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+
+	chained_irq_enter(chip, desc);
+
+	for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
+		int_sts = zynq_gpio_readreg(gpio->base_addr +
+					    ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
+		int_enb = zynq_gpio_readreg(gpio->base_addr +
+					    ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
+		int_sts &= ~int_enb;
+
+		for (; int_sts != 0; int_sts >>= 1, gpio_irq++) {
+			if (!(int_sts & 1))
+				continue;
+			gpio_irq_desc = irq_to_desc(gpio_irq);
+			BUG_ON(!gpio_irq_desc);
+			chip = irq_desc_get_chip(gpio_irq_desc);
+			BUG_ON(!chip);
+			chip->irq_ack(&gpio_irq_desc->irq_data);
+
+			/* call the pin specific handler */
+			generic_handle_irq(gpio_irq);
+		}
+		/* shift to first virtual irq of next bank */
+		gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;
+	}
+
+	chip = irq_desc_get_chip(desc);
+	chained_irq_exit(chip, desc);
+}
+
+static int __maybe_unused zynq_gpio_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+	if (!device_may_wakeup(dev)) {
+		if (!pm_runtime_suspended(dev))
+			clk_disable(gpio->clk);
+		return 0;
+	}
+
+	return 0;
+}
+
+static int __maybe_unused zynq_gpio_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+	if (!device_may_wakeup(dev)) {
+		if (!pm_runtime_suspended(dev))
+			return clk_enable(gpio->clk);
+	}
+
+	return 0;
+}
+
+static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+	clk_disable(gpio->clk);
+
+	return 0;
+}
+
+static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+	return clk_enable(gpio->clk);
+}
+
+static int __maybe_unused zynq_gpio_idle(struct device *dev)
+{
+	return pm_schedule_suspend(dev, 1);
+}
+
+static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	int ret;
+
+	ret = pm_runtime_get_sync(chip->dev);
+
+	/*
+	 * 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 zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	pm_runtime_put_sync(chip->dev);
+}
+
+static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
+	SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
+			   zynq_gpio_idle)
+};
+
+/**
+ * zynq_gpio_probe - Initialization method for a zynq_gpio device
+ * @pdev:	platform device instance
+ *
+ * This function allocates memory resources for the gpio device and registers
+ * all the banks of the device. It will also set up interrupts for the gpio
+ * pins.
+ * Note: Interrupts are disabled for all the banks during initialization.
+ *
+ * Return: 0 on success, negative error otherwise.
+ */
+static int zynq_gpio_probe(struct platform_device *pdev)
+{
+	int ret, pin_num, bank_num, gpio_irq;
+	unsigned int irq_num;
+	struct zynq_gpio *gpio;
+	struct gpio_chip *chip;
+	struct resource *res;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, gpio);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	gpio->base_addr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(gpio->base_addr))
+		return PTR_ERR(gpio->base_addr);
+
+	irq_num = platform_get_irq(pdev, 0);
+	gpio->irq = irq_num;
+
+	/* configure the gpio chip */
+	chip = &gpio->chip;
+	chip->label = "zynq_gpio";
+	chip->owner = THIS_MODULE;
+	chip->dev = &pdev->dev;
+	chip->get = zynq_gpio_get_value;
+	chip->set = zynq_gpio_set_value;
+	chip->request = zynq_gpio_request;
+	chip->free = zynq_gpio_free;
+	chip->direction_input = zynq_gpio_dir_in;
+	chip->direction_output = zynq_gpio_dir_out;
+	chip->to_irq = zynq_gpio_to_irq;
+	chip->dbg_show = NULL;
+	chip->base = 0;		/* default pin base */
+	chip->ngpio = ZYNQ_GPIO_NR_GPIOS;
+	chip->can_sleep = 0;
+
+	gpio->irq_base = irq_alloc_descs(-1, 0, chip->ngpio, 0);
+	if (gpio->irq_base < 0) {
+		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+
+	irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
+					   chip->ngpio, gpio->irq_base, 0,
+					   &irq_domain_simple_ops, NULL);
+
+	/* report a bug if gpio chip registration fails */
+	ret = gpiochip_add(chip);
+	if (ret < 0)
+		return ret;
+
+	/* Enable GPIO clock */
+	gpio->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(gpio->clk)) {
+		dev_err(&pdev->dev, "input clock not found.\n");
+		if (gpiochip_remove(chip))
+			dev_err(&pdev->dev, "Failed to remove gpio chip\n");
+		return PTR_ERR(gpio->clk);
+	}
+	ret = clk_prepare_enable(gpio->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to enable clock.\n");
+		if (gpiochip_remove(chip))
+			dev_err(&pdev->dev, "Failed to remove gpio chip\n");
+		return ret;
+	}
+
+	/* disable interrupts for all banks */
+	for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
+		zynq_gpio_writereg(gpio->base_addr +
+				   ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
+				   ZYNQ_GPIO_IXR_DISABLE_ALL);
+	}
+
+	/*
+	 * set the irq chip, handler and irq chip data for callbacks for
+	 * each pin
+	 */
+	for (pin_num = 0; pin_num < min_t(int, ZYNQ_GPIO_NR_GPIOS,
+					  (int)chip->ngpio); pin_num++) {
+		gpio_irq = irq_find_mapping(irq_domain, pin_num);
+		irq_set_chip_and_handler(gpio_irq, &zynq_gpio_irqchip,
+					 handle_simple_irq);
+		irq_set_chip_data(gpio_irq, (void *)gpio);
+		set_irq_flags(gpio_irq, IRQF_VALID);
+	}
+
+	irq_set_handler_data(irq_num, (void *)gpio);
+	irq_set_chained_handler(irq_num, zynq_gpio_irqhandler);
+
+	pm_runtime_enable(&pdev->dev);
+
+	device_set_wakeup_capable(&pdev->dev, 1);
+
+	return 0;
+}
+
+/**
+ * zynq_gpio_remove - Driver removal function
+ * @pdev:	platform device instance
+ *
+ * Return: 0 always
+ */
+static int zynq_gpio_remove(struct platform_device *pdev)
+{
+	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(gpio->clk);
+	device_set_wakeup_capable(&pdev->dev, 0);
+	return 0;
+}
+
+static struct of_device_id zynq_gpio_of_match[] = {
+	{ .compatible = "xlnx,zynq-gpio-1.0", },
+	{ /* end of table */ }
+};
+MODULE_DEVICE_TABLE(of, zynq_gpio_of_match);
+
+static struct platform_driver zynq_gpio_driver = {
+	.driver	= {
+		.name	= DRIVER_NAME,
+		.owner	= THIS_MODULE,
+		.pm	= &zynq_gpio_dev_pm_ops,
+		.of_match_table = zynq_gpio_of_match,
+	},
+	.probe		= zynq_gpio_probe,
+	.remove		= zynq_gpio_remove,
+};
+
+/**
+ * zynq_gpio_init - Initial driver registration call
+ *
+ * Return: value from platform_driver_register
+ */
+static int __init zynq_gpio_init(void)
+{
+	return platform_driver_register(&zynq_gpio_driver);
+}
+
+postcore_initcall(zynq_gpio_init);
+
+MODULE_AUTHOR("Xilinx, Inc.");
+MODULE_DESCRIPTION("Zynq GPIO driver");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Harini Katakam]

Add support for GPIO controller used by Xilinx Zynq

Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
 drivers/gpio/Kconfig     |    7 +
 drivers/gpio/Makefile    |    1 +
 drivers/gpio/gpio-zynq.c |  690 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 698 insertions(+)
 create mode 100644 drivers/gpio/gpio-zynq.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 903f24d..67a22a6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -313,6 +313,13 @@ config GPIO_XTENSA
 	  Say yes here to support the Xtensa internal GPIO32 IMPWIRE (input)
 	  and EXPSTATE (output) ports
 
+config GPIO_ZYNQ
+	bool "Xilinx ZYNQ GPIO support"
+	depends on ARCH_ZYNQ
+	select GENERIC_IRQ_CHIP
+	help
+	 Say yes here to support Xilinx ZYNQ GPIO controller.
+
 config GPIO_VR41XX
 	tristate "NEC VR4100 series General-purpose I/O Uint support"
 	depends on CPU_VR41XX
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 5d50179..439f23a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -99,3 +99,4 @@ obj-$(CONFIG_GPIO_WM8350)	+= gpio-wm8350.o
 obj-$(CONFIG_GPIO_WM8994)	+= gpio-wm8994.o
 obj-$(CONFIG_GPIO_XILINX)	+= gpio-xilinx.o
 obj-$(CONFIG_GPIO_XTENSA)	+= gpio-xtensa.o
+obj-$(CONFIG_GPIO_ZYNQ)		+= gpio-zynq.o
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
new file mode 100644
index 0000000..1f5fdfc
--- /dev/null
+++ b/drivers/gpio/gpio-zynq.c
@@ -0,0 +1,690 @@
+/*
+ * Xilinx Zynq GPIO device driver
+ *
+ * Copyright (C) 2009 - 2014 Xilinx, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License as published by the Free Software
+ * Foundation; either version 2 of the License, or (at your option) any later
+ * version.
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#define DRIVER_NAME "zynq-gpio"
+#define ZYNQ_GPIO_NR_GPIOS	118
+
+static struct irq_domain *irq_domain;
+
+/* Register offsets for the GPIO device */
+
+/* LSW Mask & Data -WO */
+#define ZYNQ_GPIO_DATA_LSW_OFFSET(BANK)	(0x000 + (8 * BANK))
+/* MSW Mask & Data -WO */
+#define ZYNQ_GPIO_DATA_MSW_OFFSET(BANK)	(0x004 + (8 * BANK))
+/* Data Register-RW */
+#define ZYNQ_GPIO_DATA_OFFSET(BANK)	(0x040 + (4 * BANK))
+/* Direction mode reg-RW */
+#define ZYNQ_GPIO_DIRM_OFFSET(BANK)	(0x204 + (0x40 * BANK))
+/* Output enable reg-RW */
+#define ZYNQ_GPIO_OUTEN_OFFSET(BANK)	(0x208 + (0x40 * BANK))
+/* Interrupt mask reg-RO */
+#define ZYNQ_GPIO_INTMASK_OFFSET(BANK)	(0x20C + (0x40 * BANK))
+/* Interrupt enable reg-WO */
+#define ZYNQ_GPIO_INTEN_OFFSET(BANK)	(0x210 + (0x40 * BANK))
+/* Interrupt disable reg-WO */
+#define ZYNQ_GPIO_INTDIS_OFFSET(BANK)	(0x214 + (0x40 * BANK))
+/* Interrupt status reg-RO */
+#define ZYNQ_GPIO_INTSTS_OFFSET(BANK)	(0x218 + (0x40 * BANK))
+/* Interrupt type reg-RW */
+#define ZYNQ_GPIO_INTTYPE_OFFSET(BANK)	(0x21C + (0x40 * BANK))
+/* Interrupt polarity reg-RW */
+#define ZYNQ_GPIO_INTPOL_OFFSET(BANK)	(0x220 + (0x40 * BANK))
+/* Interrupt on any, reg-RW */
+#define ZYNQ_GPIO_INTANY_OFFSET(BANK)	(0x224 + (0x40 * BANK))
+
+/* Read/Write access to the GPIO PS registers */
+static inline u32 zynq_gpio_readreg(void __iomem *offset)
+{
+	return readl_relaxed(offset);
+}
+
+static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
+{
+	writel_relaxed(val, offset);
+}
+
+static unsigned int zynq_gpio_pin_table[] = {
+	31, /* 0 - 31 */
+	53, /* 32 - 53 */
+	85, /* 54 - 85 */
+	117 /* 86 - 117 */
+};
+
+/* Maximum banks */
+#define ZYNQ_GPIO_MAX_BANK	4
+
+/* Disable all interrupts mask */
+#define ZYNQ_GPIO_IXR_DISABLE_ALL	0xFFFFFFFF
+
+/* GPIO pin high */
+#define ZYNQ_GPIO_PIN_HIGH 1
+
+/* Mid pin number of a bank */
+#define ZYNQ_GPIO_MID_PIN_NUM 16
+
+/* GPIO upper 16 bit mask */
+#define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000
+
+/**
+ * struct zynq_gpio - gpio device private data structure
+ * @chip:	instance of the gpio_chip
+ * @base_addr:	base address of the GPIO device
+ * @irq:	irq associated with the controller
+ * @irq_base:	base of IRQ number for interrupt
+ * @clk:	clock resource for this controller
+ */
+struct zynq_gpio {
+	struct gpio_chip chip;
+	void __iomem *base_addr;
+	unsigned int irq;
+	unsigned int irq_base;
+	struct clk *clk;
+};
+
+/**
+ * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
+ * for a given pin in the GPIO device
+ * @pin_num:	gpio pin number within the device
+ * @bank_num:	an output parameter used to return the bank number of the gpio
+ *		pin
+ * @bank_pin_num: an output parameter used to return pin number within a bank
+ *		  for the given gpio pin
+ *
+ * Returns the bank number.
+ */
+static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
+					  unsigned int *bank_num,
+					  unsigned int *bank_pin_num)
+{
+	for (*bank_num = 0; *bank_num < ZYNQ_GPIO_MAX_BANK; (*bank_num)++)
+		if (pin_num <= zynq_gpio_pin_table[*bank_num])
+			break;
+
+	if (!(*bank_num))
+		*bank_pin_num = pin_num;
+	else
+		*bank_pin_num = pin_num %
+				(zynq_gpio_pin_table[*bank_num - 1] + 1);
+}
+
+/**
+ * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ *
+ * This function reads the state of the specified pin of the GPIO device.
+ *
+ * Return: 0 if the pin is low, 1 if pin is high.
+ */
+static int zynq_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
+{
+	unsigned int bank_num, bank_pin_num, data;
+	struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+
+	zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+
+	data = zynq_gpio_readreg(gpio->base_addr +
+				 ZYNQ_GPIO_DATA_OFFSET(bank_num));
+	return (data >> bank_pin_num) & ZYNQ_GPIO_PIN_HIGH;
+}
+
+/**
+ * zynq_gpio_set_value - Modify the state of the pin with specified value
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ * @state:	value used to modify the state of the specified pin
+ *
+ * This function calculates the register offset (i.e to lower 16 bits or
+ * upper 16 bits) based on the given pin number and sets the state of a
+ * gpio pin to the specified value. The state is either 0 or non-zero.
+ */
+static void zynq_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
+				int state)
+{
+	unsigned int reg_offset, bank_num, bank_pin_num;
+	struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+
+	zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+
+	if (bank_pin_num >= ZYNQ_GPIO_MID_PIN_NUM) {
+		/* only 16 data bits in bit maskable reg */
+		bank_pin_num -= ZYNQ_GPIO_MID_PIN_NUM;
+		reg_offset = ZYNQ_GPIO_DATA_MSW_OFFSET(bank_num);
+	} else {
+		reg_offset = ZYNQ_GPIO_DATA_LSW_OFFSET(bank_num);
+	}
+
+	/*
+	 * get the 32 bit value to be written to the mask/data register where
+	 * the upper 16 bits is the mask and lower 16 bits is the data
+	 */
+	if (state)
+		state = 1;
+	state = ~(1 << (bank_pin_num + ZYNQ_GPIO_MID_PIN_NUM)) &
+		((state << bank_pin_num) | ZYNQ_GPIO_UPPER_MASK);
+
+	zynq_gpio_writereg(gpio->base_addr + reg_offset, state);
+}
+
+/**
+ * zynq_gpio_dir_in - Set the direction of the specified GPIO pin as input
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ *
+ * This function uses the read-modify-write sequence to set the direction of
+ * the gpio pin as input.
+ *
+ * Return: 0 always
+ */
+static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
+{
+	unsigned int reg, bank_num, bank_pin_num;
+	struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+
+	zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+	/* clear the bit in direction mode reg to set the pin as input */
+	reg = zynq_gpio_readreg(gpio->base_addr +
+				ZYNQ_GPIO_DIRM_OFFSET(bank_num));
+	reg &= ~(1 << bank_pin_num);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num),
+			   reg);
+
+	return 0;
+}
+
+/**
+ * zynq_gpio_dir_out - Set the direction of the specified GPIO pin as output
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ * @state:	value to be written to specified pin
+ *
+ * This function sets the direction of specified GPIO pin as output, configures
+ * the Output Enable register for the pin and uses zynq_gpio_set to set
+ * the state of the pin to the value specified.
+ *
+ * Return: 0 always
+ */
+static int zynq_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
+			     int state)
+{
+	struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
+	unsigned int reg, bank_num, bank_pin_num;
+
+	zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
+
+	/* set the GPIO pin as output */
+	reg = zynq_gpio_readreg(gpio->base_addr +
+				ZYNQ_GPIO_DIRM_OFFSET(bank_num));
+	reg |= 1 << bank_pin_num;
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num),
+			   reg);
+
+	/* configure the output enable reg for the pin */
+	reg = zynq_gpio_readreg(gpio->base_addr +
+				ZYNQ_GPIO_OUTEN_OFFSET(bank_num));
+	reg |= 1 << bank_pin_num;
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_OUTEN_OFFSET(bank_num),
+			   reg);
+
+	/* set the state of the pin */
+	zynq_gpio_set_value(chip, pin, state);
+	return 0;
+}
+
+static int zynq_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+	return irq_find_mapping(irq_domain, offset);
+}
+
+/**
+ * zynq_gpio_irq_ack - Acknowledge the interrupt of a gpio pin
+ * @irq_data:	irq data containing irq number of gpio pin for the interrupt
+ *		to ack
+ *
+ * This function calculates gpio pin number from irq number and sets the bit
+ * in the Interrupt Status Register of the corresponding bank, to ACK the irq.
+ */
+static void zynq_gpio_irq_ack(struct irq_data *irq_data)
+{
+	struct zynq_gpio *gpio = (struct zynq_gpio *)
+				 irq_data_get_irq_chip_data(irq_data);
+	unsigned int device_pin_num, bank_num, bank_pin_num;
+
+	device_pin_num = irq_data->hwirq;
+	zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTSTS_OFFSET(bank_num),
+			   1 << bank_pin_num);
+}
+
+/**
+ * zynq_gpio_irq_mask - Disable the interrupts for a gpio pin
+ * @irq_data:	per irq and chip data passed down to chip functions
+ *
+ * This function calculates gpio pin number from irq number and sets the
+ * bit in the Interrupt Disable register of the corresponding bank to disable
+ * interrupts for that pin.
+ */
+static void zynq_gpio_irq_mask(struct irq_data *irq_data)
+{
+	struct zynq_gpio *gpio = (struct zynq_gpio *)
+				 irq_data_get_irq_chip_data(irq_data);
+	unsigned int device_pin_num, bank_num, bank_pin_num;
+
+	device_pin_num = irq_data->hwirq;
+	zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
+			   1 << bank_pin_num);
+}
+
+/**
+ * zynq_gpio_irq_unmask - Enable the interrupts for a gpio pin
+ * @irq_data:	irq data containing irq number of gpio pin for the interrupt
+ *		to enable
+ *
+ * This function calculates the gpio pin number from irq number and sets the
+ * bit in the Interrupt Enable register of the corresponding bank to enable
+ * interrupts for that pin.
+ */
+static void zynq_gpio_irq_unmask(struct irq_data *irq_data)
+{
+	struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
+	unsigned int device_pin_num, bank_num, bank_pin_num;
+
+	device_pin_num = irq_data->hwirq;
+	zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTEN_OFFSET(bank_num),
+			   1 << bank_pin_num);
+}
+
+/**
+ * zynq_gpio_set_irq_type - Set the irq type for a gpio pin
+ * @irq_data:	irq data containing irq number of gpio pin
+ * @type:	interrupt type that is to be set for the gpio pin
+ *
+ * This function gets the gpio pin number and its bank from the gpio pin number
+ * and configures the INT_TYPE, INT_POLARITY and INT_ANY registers.
+ *
+ * Return: 0, negative error otherwise.
+ * TYPE-EDGE_RISING,  INT_TYPE - 1, INT_POLARITY - 1,  INT_ANY - 0;
+ * TYPE-EDGE_FALLING, INT_TYPE - 1, INT_POLARITY - 0,  INT_ANY - 0;
+ * TYPE-EDGE_BOTH,    INT_TYPE - 1, INT_POLARITY - NA, INT_ANY - 1;
+ * TYPE-LEVEL_HIGH,   INT_TYPE - 0, INT_POLARITY - 1,  INT_ANY - NA;
+ * TYPE-LEVEL_LOW,    INT_TYPE - 0, INT_POLARITY - 0,  INT_ANY - NA
+ */
+static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
+{
+	struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
+	unsigned int device_pin_num, bank_num, bank_pin_num;
+	unsigned int int_type, int_pol, int_any;
+
+	device_pin_num = irq_data->hwirq;
+	zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
+
+	int_type = zynq_gpio_readreg(gpio->base_addr +
+				     ZYNQ_GPIO_INTTYPE_OFFSET(bank_num));
+	int_pol = zynq_gpio_readreg(gpio->base_addr +
+				    ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
+	int_any = zynq_gpio_readreg(gpio->base_addr +
+				    ZYNQ_GPIO_INTANY_OFFSET(bank_num));
+
+	/*
+	 * based on the type requested, configure the INT_TYPE, INT_POLARITY
+	 * and INT_ANY registers
+	 */
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		int_type |= (1 << bank_pin_num);
+		int_pol |= (1 << bank_pin_num);
+		int_any &= ~(1 << bank_pin_num);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		int_type |= (1 << bank_pin_num);
+		int_pol &= ~(1 << bank_pin_num);
+		int_any &= ~(1 << bank_pin_num);
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		int_type |= (1 << bank_pin_num);
+		int_any |= (1 << bank_pin_num);
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		int_type &= ~(1 << bank_pin_num);
+		int_pol |= (1 << bank_pin_num);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		int_type &= ~(1 << bank_pin_num);
+		int_pol &= ~(1 << bank_pin_num);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	zynq_gpio_writereg(gpio->base_addr +
+			   ZYNQ_GPIO_INTTYPE_OFFSET(bank_num), int_type);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTPOL_OFFSET(bank_num),
+			   int_pol);
+	zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTANY_OFFSET(bank_num),
+			   int_any);
+	return 0;
+}
+
+static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
+{
+	if (on)
+		zynq_gpio_irq_unmask(data);
+	else
+		zynq_gpio_irq_mask(data);
+
+	return 0;
+}
+
+/* irq chip descriptor */
+static struct irq_chip zynq_gpio_irqchip = {
+	.name		= DRIVER_NAME,
+	.irq_ack	= zynq_gpio_irq_ack,
+	.irq_mask	= zynq_gpio_irq_mask,
+	.irq_unmask	= zynq_gpio_irq_unmask,
+	.irq_set_type	= zynq_gpio_set_irq_type,
+	.irq_set_wake	= zynq_gpio_set_wake,
+};
+
+/**
+ * zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
+ * @irq:	irq number of the gpio bank where interrupt has occurred
+ * @desc:	irq descriptor instance of the 'irq'
+ *
+ * This function reads the Interrupt Status Register of each bank to get the
+ * gpio pin number which has triggered an interrupt. It then acks the triggered
+ * interrupt and calls the pin specific handler set by the higher layer
+ * application for that pin.
+ * Note: A bug is reported if no handler is set for the gpio pin.
+ */
+static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
+{
+	struct zynq_gpio *gpio = (struct zynq_gpio *)irq_get_handler_data(irq);
+	int gpio_irq = gpio->irq_base;
+	unsigned int int_sts, int_enb, bank_num;
+	struct irq_desc *gpio_irq_desc;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+
+	chained_irq_enter(chip, desc);
+
+	for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
+		int_sts = zynq_gpio_readreg(gpio->base_addr +
+					    ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
+		int_enb = zynq_gpio_readreg(gpio->base_addr +
+					    ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
+		int_sts &= ~int_enb;
+
+		for (; int_sts != 0; int_sts >>= 1, gpio_irq++) {
+			if (!(int_sts & 1))
+				continue;
+			gpio_irq_desc = irq_to_desc(gpio_irq);
+			BUG_ON(!gpio_irq_desc);
+			chip = irq_desc_get_chip(gpio_irq_desc);
+			BUG_ON(!chip);
+			chip->irq_ack(&gpio_irq_desc->irq_data);
+
+			/* call the pin specific handler */
+			generic_handle_irq(gpio_irq);
+		}
+		/* shift to first virtual irq of next bank */
+		gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;
+	}
+
+	chip = irq_desc_get_chip(desc);
+	chained_irq_exit(chip, desc);
+}
+
+static int __maybe_unused zynq_gpio_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+	if (!device_may_wakeup(dev)) {
+		if (!pm_runtime_suspended(dev))
+			clk_disable(gpio->clk);
+		return 0;
+	}
+
+	return 0;
+}
+
+static int __maybe_unused zynq_gpio_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+	if (!device_may_wakeup(dev)) {
+		if (!pm_runtime_suspended(dev))
+			return clk_enable(gpio->clk);
+	}
+
+	return 0;
+}
+
+static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+	clk_disable(gpio->clk);
+
+	return 0;
+}
+
+static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+	return clk_enable(gpio->clk);
+}
+
+static int __maybe_unused zynq_gpio_idle(struct device *dev)
+{
+	return pm_schedule_suspend(dev, 1);
+}
+
+static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+	int ret;
+
+	ret = pm_runtime_get_sync(chip->dev);
+
+	/*
+	 * 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 zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	pm_runtime_put_sync(chip->dev);
+}
+
+static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
+	SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
+			   zynq_gpio_idle)
+};
+
+/**
+ * zynq_gpio_probe - Initialization method for a zynq_gpio device
+ * @pdev:	platform device instance
+ *
+ * This function allocates memory resources for the gpio device and registers
+ * all the banks of the device. It will also set up interrupts for the gpio
+ * pins.
+ * Note: Interrupts are disabled for all the banks during initialization.
+ *
+ * Return: 0 on success, negative error otherwise.
+ */
+static int zynq_gpio_probe(struct platform_device *pdev)
+{
+	int ret, pin_num, bank_num, gpio_irq;
+	unsigned int irq_num;
+	struct zynq_gpio *gpio;
+	struct gpio_chip *chip;
+	struct resource *res;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, gpio);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	gpio->base_addr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(gpio->base_addr))
+		return PTR_ERR(gpio->base_addr);
+
+	irq_num = platform_get_irq(pdev, 0);
+	gpio->irq = irq_num;
+
+	/* configure the gpio chip */
+	chip = &gpio->chip;
+	chip->label = "zynq_gpio";
+	chip->owner = THIS_MODULE;
+	chip->dev = &pdev->dev;
+	chip->get = zynq_gpio_get_value;
+	chip->set = zynq_gpio_set_value;
+	chip->request = zynq_gpio_request;
+	chip->free = zynq_gpio_free;
+	chip->direction_input = zynq_gpio_dir_in;
+	chip->direction_output = zynq_gpio_dir_out;
+	chip->to_irq = zynq_gpio_to_irq;
+	chip->dbg_show = NULL;
+	chip->base = 0;		/* default pin base */
+	chip->ngpio = ZYNQ_GPIO_NR_GPIOS;
+	chip->can_sleep = 0;
+
+	gpio->irq_base = irq_alloc_descs(-1, 0, chip->ngpio, 0);
+	if (gpio->irq_base < 0) {
+		dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
+		return -ENODEV;
+	}
+
+	irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
+					   chip->ngpio, gpio->irq_base, 0,
+					   &irq_domain_simple_ops, NULL);
+
+	/* report a bug if gpio chip registration fails */
+	ret = gpiochip_add(chip);
+	if (ret < 0)
+		return ret;
+
+	/* Enable GPIO clock */
+	gpio->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(gpio->clk)) {
+		dev_err(&pdev->dev, "input clock not found.\n");
+		if (gpiochip_remove(chip))
+			dev_err(&pdev->dev, "Failed to remove gpio chip\n");
+		return PTR_ERR(gpio->clk);
+	}
+	ret = clk_prepare_enable(gpio->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to enable clock.\n");
+		if (gpiochip_remove(chip))
+			dev_err(&pdev->dev, "Failed to remove gpio chip\n");
+		return ret;
+	}
+
+	/* disable interrupts for all banks */
+	for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
+		zynq_gpio_writereg(gpio->base_addr +
+				   ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
+				   ZYNQ_GPIO_IXR_DISABLE_ALL);
+	}
+
+	/*
+	 * set the irq chip, handler and irq chip data for callbacks for
+	 * each pin
+	 */
+	for (pin_num = 0; pin_num < min_t(int, ZYNQ_GPIO_NR_GPIOS,
+					  (int)chip->ngpio); pin_num++) {
+		gpio_irq = irq_find_mapping(irq_domain, pin_num);
+		irq_set_chip_and_handler(gpio_irq, &zynq_gpio_irqchip,
+					 handle_simple_irq);
+		irq_set_chip_data(gpio_irq, (void *)gpio);
+		set_irq_flags(gpio_irq, IRQF_VALID);
+	}
+
+	irq_set_handler_data(irq_num, (void *)gpio);
+	irq_set_chained_handler(irq_num, zynq_gpio_irqhandler);
+
+	pm_runtime_enable(&pdev->dev);
+
+	device_set_wakeup_capable(&pdev->dev, 1);
+
+	return 0;
+}
+
+/**
+ * zynq_gpio_remove - Driver removal function
+ * @pdev:	platform device instance
+ *
+ * Return: 0 always
+ */
+static int zynq_gpio_remove(struct platform_device *pdev)
+{
+	struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(gpio->clk);
+	device_set_wakeup_capable(&pdev->dev, 0);
+	return 0;
+}
+
+static struct of_device_id zynq_gpio_of_match[] = {
+	{ .compatible = "xlnx,zynq-gpio-1.0", },
+	{ /* end of table */ }
+};
+MODULE_DEVICE_TABLE(of, zynq_gpio_of_match);
+
+static struct platform_driver zynq_gpio_driver = {
+	.driver	= {
+		.name	= DRIVER_NAME,
+		.owner	= THIS_MODULE,
+		.pm	= &zynq_gpio_dev_pm_ops,
+		.of_match_table = zynq_gpio_of_match,
+	},
+	.probe		= zynq_gpio_probe,
+	.remove		= zynq_gpio_remove,
+};
+
+/**
+ * zynq_gpio_init - Initial driver registration call
+ *
+ * Return: value from platform_driver_register
+ */
+static int __init zynq_gpio_init(void)
+{
+	return platform_driver_register(&zynq_gpio_driver);
+}
+
+postcore_initcall(zynq_gpio_init);
+
+MODULE_AUTHOR("Xilinx, Inc.");
+MODULE_DESCRIPTION("Zynq GPIO driver");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

[PATCH 2/2] Devicetree: Add Zynq GPIO devicetree bindings documentation

[Harini Katakam]

Add gpio-zynq bindings documentation.

Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
 .../devicetree/bindings/gpio/gpio-zynq.txt         |   24 ++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-zynq.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-zynq.txt b/Documentation/devicetree/bindings/gpio/gpio-zynq.txt
new file mode 100644
index 0000000..1b6e83f
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-zynq.txt
@@ -0,0 +1,24 @@
+Xilinx Zynq GPIO controller Device Tree Bindings
+-------------------------------------------
+
+Required properties:
+- #gpio-cells 		: Should be two. First cell is used to mention
+			  pin number.
+- compatible		: Should be "xlnx,zynq-gpio-1.0"
+- clocks		: Clock phandles (see clock bindings for details)
+- gpio-controller	: Marks the device node as a GPIO controller.
+- interrupts		: Property with a value describing the interrupt
+			  number.
+- interrupt-parent	: Must be core interrupt controller
+- reg			: Address and length of the register set for the device
+
+Example:
+		gpio@e000a000 {
+			#gpio-cells = <2>;
+			compatible = "xlnx,zynq-gpio-1.0";
+			clocks = <&clkc 42>;
+			gpio-controller ;
+			interrupt-parent = <&intc>;
+			interrupts = <0 20 4>;
+			reg = <0xe000a000 0x1000>;
+		} ;
-- 
1.7.9.5

[PATCH 2/2] Devicetree: Add Zynq GPIO devicetree bindings documentation

[Harini Katakam]

Add gpio-zynq bindings documentation.

Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
 .../devicetree/bindings/gpio/gpio-zynq.txt         |   24 ++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-zynq.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-zynq.txt b/Documentation/devicetree/bindings/gpio/gpio-zynq.txt
new file mode 100644
index 0000000..1b6e83f
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-zynq.txt
@@ -0,0 +1,24 @@
+Xilinx Zynq GPIO controller Device Tree Bindings
+-------------------------------------------
+
+Required properties:
+- #gpio-cells 		: Should be two. First cell is used to mention
+			  pin number.
+- compatible		: Should be "xlnx,zynq-gpio-1.0"
+- clocks		: Clock phandles (see clock bindings for details)
+- gpio-controller	: Marks the device node as a GPIO controller.
+- interrupts		: Property with a value describing the interrupt
+			  number.
+- interrupt-parent	: Must be core interrupt controller
+- reg			: Address and length of the register set for the device
+
+Example:
+		gpio at e000a000 {
+			#gpio-cells = <2>;
+			compatible = "xlnx,zynq-gpio-1.0";
+			clocks = <&clkc 42>;
+			gpio-controller ;
+			interrupt-parent = <&intc>;
+			interrupts = <0 20 4>;
+			reg = <0xe000a000 0x1000>;
+		} ;
-- 
1.7.9.5

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:

> Add support for GPIO controller used by Xilinx Zynq
>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>

This will not be integrated before v3.16 so we have some time to
think things over.

> +config GPIO_ZYNQ
> +       bool "Xilinx ZYNQ GPIO support"
> +       depends on ARCH_ZYNQ
> +       select GENERIC_IRQ_CHIP

I think that rather than selecting GENERIC_IRQ_CHIP,
select my new GPIOLIB_IRQCHIP and use the new helpers
gpiochip_irqchip_add() and gpiochip_set_chained_irqchip().

Look in drivers/gpio/gpio-pl061.c for example usage.

This is available in the "devel" or "for-next" branch of my
gpio tree at
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/

> +#include <linux/clk.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>

These three are auto-included with GPIOLIB_IRQCHIP.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define DRIVER_NAME "zynq-gpio"
> +#define ZYNQ_GPIO_NR_GPIOS     118
> +
> +static struct irq_domain *irq_domain;

Static global variables are a no-no. This goes away with
GPIOLIB_IRQCHIP, but read about state containers in
Documentation/driver-model/design-patterns.txt

> +/* Read/Write access to the GPIO PS registers */
> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
> +{
> +       return readl_relaxed(offset);
> +}
> +
> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
> +{
> +       writel_relaxed(val, offset);
> +}

I think this is unnecessary and confusing indirection.
Just use the readl_relaxed/writel_relaxed functions directly in
the code.

> +static unsigned int zynq_gpio_pin_table[] = {
> +       31, /* 0 - 31 */
> +       53, /* 32 - 53 */
> +       85, /* 54 - 85 */
> +       117 /* 86 - 117 */
> +};

This is an indication that you may be writing a pin control
driver rather than a GPIO driver. Please consule
Documentation/pinctrl.txt for a brief overview of what we
mean by this.

> +/* Maximum banks */
> +#define ZYNQ_GPIO_MAX_BANK     4
> +
> +/* Disable all interrupts mask */
> +#define ZYNQ_GPIO_IXR_DISABLE_ALL      0xFFFFFFFF
> +
> +/* GPIO pin high */
> +#define ZYNQ_GPIO_PIN_HIGH 1

This looks like pin config business.

> +/* Mid pin number of a bank */
> +#define ZYNQ_GPIO_MID_PIN_NUM 16
> +
> +/* GPIO upper 16 bit mask */
> +#define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000
> +
> +/**
> + * struct zynq_gpio - gpio device private data structure
> + * @chip:      instance of the gpio_chip
> + * @base_addr: base address of the GPIO device
> + * @irq:       irq associated with the controller
> + * @irq_base:  base of IRQ number for interrupt
> + * @clk:       clock resource for this controller
> + */
> +struct zynq_gpio {
> +       struct gpio_chip chip;
> +       void __iomem *base_addr;
> +       unsigned int irq;
> +       unsigned int irq_base;

Using a base offset for IRQs is just wrong if you're using irqdomain.
BTW you will be using chip->irqdomain after converting to
GPIOLIB_IRQCHIP.

> +       struct clk *clk;
> +};
> +
> +/**
> + * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
> + * for a given pin in the GPIO device
> + * @pin_num:   gpio pin number within the device
> + * @bank_num:  an output parameter used to return the bank number of the gpio
> + *             pin
> + * @bank_pin_num: an output parameter used to return pin number within a bank
> + *               for the given gpio pin
> + *
> + * Returns the bank number.
> + */
> +static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
> +                                         unsigned int *bank_num,
> +                                         unsigned int *bank_pin_num)
> +{
> +       for (*bank_num = 0; *bank_num < ZYNQ_GPIO_MAX_BANK; (*bank_num)++)
> +               if (pin_num <= zynq_gpio_pin_table[*bank_num])
> +                       break;
> +
> +       if (!(*bank_num))
> +               *bank_pin_num = pin_num;
> +       else
> +               *bank_pin_num = pin_num %
> +                               (zynq_gpio_pin_table[*bank_num - 1] + 1);
> +}

This is reimplementing gpio ranges from the pin control
subsystem. Consult e.g.
drivers/pinctrl/pinctrl-baytrail.c

For another simple driver using GPIO ranges from pin control
for this. And implement it that way.

> +/**
> + * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function reads the state of the specified pin of the GPIO device.
> + *
> + * Return: 0 if the pin is low, 1 if pin is high.
> + */
> +static int zynq_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int bank_num, bank_pin_num, data;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       data = zynq_gpio_readreg(gpio->base_addr +
> +                                ZYNQ_GPIO_DATA_OFFSET(bank_num));
> +       return (data >> bank_pin_num) & ZYNQ_GPIO_PIN_HIGH;
> +}

Instead of fiddling around with the banks like this, consider registering
one gpiochip per bank, so one instance of the device per bank, and
be done with it. Maybe it's hard to do that so not a firm requirement,
but think about it.

(...)
> +/**
> + * zynq_gpio_dir_in - Set the direction of the specified GPIO pin as input
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function uses the read-modify-write sequence to set the direction of
> + * the gpio pin as input.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int reg, bank_num, bank_pin_num;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +       /* clear the bit in direction mode reg to set the pin as input */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg &= ~(1 << bank_pin_num);

In such cases I usually do:

#include <linux/bitops.h>

reg &= ~BIT(bank_pin_num);

(Applied everywhere.)

(...)
> +/**
> + * zynq_gpio_dir_out - Set the direction of the specified GPIO pin as output
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + * @state:     value to be written to specified pin
> + *
> + * This function sets the direction of specified GPIO pin as output, configures
> + * the Output Enable register for the pin and uses zynq_gpio_set to set
> + * the state of the pin to the value specified.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
> +                            int state)
> +{
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +       unsigned int reg, bank_num, bank_pin_num;
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       /* set the GPIO pin as output */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg |= 1 << bank_pin_num;

Like here:
reg |= BIT(bank_pin_num);

> +static int zynq_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       return irq_find_mapping(irq_domain, offset);
> +}

Goes away with GPIOLIB_IRQCHIP which does this in the
gpiolib.

(...)
> +/**
> + * zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
> + * @irq:       irq number of the gpio bank where interrupt has occurred
> + * @desc:      irq descriptor instance of the 'irq'
> + *
> + * This function reads the Interrupt Status Register of each bank to get the
> + * gpio pin number which has triggered an interrupt. It then acks the triggered
> + * interrupt and calls the pin specific handler set by the higher layer
> + * application for that pin.
> + * Note: A bug is reported if no handler is set for the gpio pin.
> + */
> +static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)irq_get_handler_data(irq);
> +       int gpio_irq = gpio->irq_base;

Using a base offset for the IRQ goes totally against the idea of
using an irqdomain!

> +       unsigned int int_sts, int_enb, bank_num;
> +       struct irq_desc *gpio_irq_desc;
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> +       chained_irq_enter(chip, desc);
> +
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               int_sts = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
> +               int_enb = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
> +               int_sts &= ~int_enb;
> +
> +               for (; int_sts != 0; int_sts >>= 1, gpio_irq++) {

Here increase from zero on the offset, then use
irq_find_mapping() to translate the hwirq to a Linux IRQ.

> +                       if (!(int_sts & 1))
> +                               continue;
> +                       gpio_irq_desc = irq_to_desc(gpio_irq);
> +                       BUG_ON(!gpio_irq_desc);
> +                       chip = irq_desc_get_chip(gpio_irq_desc);
> +                       BUG_ON(!chip);
> +                       chip->irq_ack(&gpio_irq_desc->irq_data);
> +
> +                       /* call the pin specific handler */
> +                       generic_handle_irq(gpio_irq);
> +               }
> +               /* shift to first virtual irq of next bank */
> +               gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;

This is also pretty convoluted. Are you sure you don't want to
implement one gpiochip per bank instead? I guess the final "+1"
means there is actually one IRQ per bank even?

> +       }
> +
> +       chip = irq_desc_get_chip(desc);
> +       chained_irq_exit(chip, desc);
> +}

(...)
> +static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       int ret;
> +
> +       ret = pm_runtime_get_sync(chip->dev);
> +
> +       /*
> +        * 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 zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       pm_runtime_put_sync(chip->dev);
> +}
> +
> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
> +                          zynq_gpio_idle)
> +};

Is this runtime PM implementation aligned with Ulf Hansson's recent
new helpers to simplify suspend+runtime PM coexistance?

> +/**
> + * zynq_gpio_probe - Initialization method for a zynq_gpio device
> + * @pdev:      platform device instance
> + *
> + * This function allocates memory resources for the gpio device and registers
> + * all the banks of the device. It will also set up interrupts for the gpio
> + * pins.
> + * Note: Interrupts are disabled for all the banks during initialization.
> + *
> + * Return: 0 on success, negative error otherwise.
> + */
> +static int zynq_gpio_probe(struct platform_device *pdev)
> +{
> +       int ret, pin_num, bank_num, gpio_irq;
> +       unsigned int irq_num;
> +       struct zynq_gpio *gpio;
> +       struct gpio_chip *chip;
> +       struct resource *res;
> +
> +       gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, gpio);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       gpio->base_addr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(gpio->base_addr))
> +               return PTR_ERR(gpio->base_addr);
> +
> +       irq_num = platform_get_irq(pdev, 0);
> +       gpio->irq = irq_num;

>From the code I get the sense that you are passing a single
IRQ as resource but there are actually as many IRQs as there
are banks, is this correct? Then pass *all* IRQs as resources.

> +
> +       /* configure the gpio chip */
> +       chip = &gpio->chip;
> +       chip->label = "zynq_gpio";
> +       chip->owner = THIS_MODULE;
> +       chip->dev = &pdev->dev;
> +       chip->get = zynq_gpio_get_value;
> +       chip->set = zynq_gpio_set_value;
> +       chip->request = zynq_gpio_request;
> +       chip->free = zynq_gpio_free;
> +       chip->direction_input = zynq_gpio_dir_in;
> +       chip->direction_output = zynq_gpio_dir_out;
> +       chip->to_irq = zynq_gpio_to_irq;

Not needed with GPIOLIB_IRQCHIP

> +       chip->dbg_show = NULL;
> +       chip->base = 0;         /* default pin base */

The GPIO numberspace is not the same as the pin number space.
This is only going to work if you have no other GPIO controllers
on your system. Use -1 as base so you get dynamic allocation
of a GPIO number range instead.

Make sure to use the new descriptor API for defining and accessing
GPIOs in all drivers and it will not matter which base you get.
(Good eh? :-)

As it appears you're using device tree, it is all transparent and
you need not worry.

> +       chip->ngpio = ZYNQ_GPIO_NR_GPIOS;
> +       chip->can_sleep = 0;
> +

>From here:

> +       gpio->irq_base = irq_alloc_descs(-1, 0, chip->ngpio, 0);
> +       if (gpio->irq_base < 0) {
> +               dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> +               return -ENODEV;
> +       }
> +
> +       irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
> +                                          chip->ngpio, gpio->irq_base, 0,
> +                                          &irq_domain_simple_ops, NULL);

To here, goes away with GPIOLIB_IRQCHIP

> +
> +       /* report a bug if gpio chip registration fails */
> +       ret = gpiochip_add(chip);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Enable GPIO clock */
> +       gpio->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(gpio->clk)) {
> +               dev_err(&pdev->dev, "input clock not found.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return PTR_ERR(gpio->clk);
> +       }
> +       ret = clk_prepare_enable(gpio->clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Unable to enable clock.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return ret;
> +       }

You should probably get and enable the clock *before* you add the
gpiochip.

> +       /* disable interrupts for all banks */
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               zynq_gpio_writereg(gpio->base_addr +
> +                                  ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
> +                                  ZYNQ_GPIO_IXR_DISABLE_ALL);
> +       }

>From here:

> +       /*
> +        * set the irq chip, handler and irq chip data for callbacks for
> +        * each pin
> +        */
> +       for (pin_num = 0; pin_num < min_t(int, ZYNQ_GPIO_NR_GPIOS,
> +                                         (int)chip->ngpio); pin_num++) {
> +               gpio_irq = irq_find_mapping(irq_domain, pin_num);
> +               irq_set_chip_and_handler(gpio_irq, &zynq_gpio_irqchip,
> +                                        handle_simple_irq);
> +               irq_set_chip_data(gpio_irq, (void *)gpio);
> +               set_irq_flags(gpio_irq, IRQF_VALID);
> +       }
> +
> +       irq_set_handler_data(irq_num, (void *)gpio);
> +       irq_set_chained_handler(irq_num, zynq_gpio_irqhandler);

To here goes away with GPIOLIB_IRQCHIP

> +/**
> + * zynq_gpio_remove - Driver removal function
> + * @pdev:      platform device instance
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_remove(struct platform_device *pdev)
> +{
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       clk_disable_unprepare(gpio->clk);
> +       device_set_wakeup_capable(&pdev->dev, 0);
> +       return 0;

You forgot to remove the gpiochip.

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:

> Add support for GPIO controller used by Xilinx Zynq
>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>

This will not be integrated before v3.16 so we have some time to
think things over.

> +config GPIO_ZYNQ
> +       bool "Xilinx ZYNQ GPIO support"
> +       depends on ARCH_ZYNQ
> +       select GENERIC_IRQ_CHIP

I think that rather than selecting GENERIC_IRQ_CHIP,
select my new GPIOLIB_IRQCHIP and use the new helpers
gpiochip_irqchip_add() and gpiochip_set_chained_irqchip().

Look in drivers/gpio/gpio-pl061.c for example usage.

This is available in the "devel" or "for-next" branch of my
gpio tree at
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/

> +#include <linux/clk.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>

These three are auto-included with GPIOLIB_IRQCHIP.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define DRIVER_NAME "zynq-gpio"
> +#define ZYNQ_GPIO_NR_GPIOS     118
> +
> +static struct irq_domain *irq_domain;

Static global variables are a no-no. This goes away with
GPIOLIB_IRQCHIP, but read about state containers in
Documentation/driver-model/design-patterns.txt

> +/* Read/Write access to the GPIO PS registers */
> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
> +{
> +       return readl_relaxed(offset);
> +}
> +
> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
> +{
> +       writel_relaxed(val, offset);
> +}

I think this is unnecessary and confusing indirection.
Just use the readl_relaxed/writel_relaxed functions directly in
the code.

> +static unsigned int zynq_gpio_pin_table[] = {
> +       31, /* 0 - 31 */
> +       53, /* 32 - 53 */
> +       85, /* 54 - 85 */
> +       117 /* 86 - 117 */
> +};

This is an indication that you may be writing a pin control
driver rather than a GPIO driver. Please consule
Documentation/pinctrl.txt for a brief overview of what we
mean by this.

> +/* Maximum banks */
> +#define ZYNQ_GPIO_MAX_BANK     4
> +
> +/* Disable all interrupts mask */
> +#define ZYNQ_GPIO_IXR_DISABLE_ALL      0xFFFFFFFF
> +
> +/* GPIO pin high */
> +#define ZYNQ_GPIO_PIN_HIGH 1

This looks like pin config business.

> +/* Mid pin number of a bank */
> +#define ZYNQ_GPIO_MID_PIN_NUM 16
> +
> +/* GPIO upper 16 bit mask */
> +#define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000
> +
> +/**
> + * struct zynq_gpio - gpio device private data structure
> + * @chip:      instance of the gpio_chip
> + * @base_addr: base address of the GPIO device
> + * @irq:       irq associated with the controller
> + * @irq_base:  base of IRQ number for interrupt
> + * @clk:       clock resource for this controller
> + */
> +struct zynq_gpio {
> +       struct gpio_chip chip;
> +       void __iomem *base_addr;
> +       unsigned int irq;
> +       unsigned int irq_base;

Using a base offset for IRQs is just wrong if you're using irqdomain.
BTW you will be using chip->irqdomain after converting to
GPIOLIB_IRQCHIP.

> +       struct clk *clk;
> +};
> +
> +/**
> + * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
> + * for a given pin in the GPIO device
> + * @pin_num:   gpio pin number within the device
> + * @bank_num:  an output parameter used to return the bank number of the gpio
> + *             pin
> + * @bank_pin_num: an output parameter used to return pin number within a bank
> + *               for the given gpio pin
> + *
> + * Returns the bank number.
> + */
> +static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
> +                                         unsigned int *bank_num,
> +                                         unsigned int *bank_pin_num)
> +{
> +       for (*bank_num = 0; *bank_num < ZYNQ_GPIO_MAX_BANK; (*bank_num)++)
> +               if (pin_num <= zynq_gpio_pin_table[*bank_num])
> +                       break;
> +
> +       if (!(*bank_num))
> +               *bank_pin_num = pin_num;
> +       else
> +               *bank_pin_num = pin_num %
> +                               (zynq_gpio_pin_table[*bank_num - 1] + 1);
> +}

This is reimplementing gpio ranges from the pin control
subsystem. Consult e.g.
drivers/pinctrl/pinctrl-baytrail.c

For another simple driver using GPIO ranges from pin control
for this. And implement it that way.

> +/**
> + * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function reads the state of the specified pin of the GPIO device.
> + *
> + * Return: 0 if the pin is low, 1 if pin is high.
> + */
> +static int zynq_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int bank_num, bank_pin_num, data;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       data = zynq_gpio_readreg(gpio->base_addr +
> +                                ZYNQ_GPIO_DATA_OFFSET(bank_num));
> +       return (data >> bank_pin_num) & ZYNQ_GPIO_PIN_HIGH;
> +}

Instead of fiddling around with the banks like this, consider registering
one gpiochip per bank, so one instance of the device per bank, and
be done with it. Maybe it's hard to do that so not a firm requirement,
but think about it.

(...)
> +/**
> + * zynq_gpio_dir_in - Set the direction of the specified GPIO pin as input
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function uses the read-modify-write sequence to set the direction of
> + * the gpio pin as input.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int reg, bank_num, bank_pin_num;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +       /* clear the bit in direction mode reg to set the pin as input */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg &= ~(1 << bank_pin_num);

In such cases I usually do:

#include <linux/bitops.h>

reg &= ~BIT(bank_pin_num);

(Applied everywhere.)

(...)
> +/**
> + * zynq_gpio_dir_out - Set the direction of the specified GPIO pin as output
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + * @state:     value to be written to specified pin
> + *
> + * This function sets the direction of specified GPIO pin as output, configures
> + * the Output Enable register for the pin and uses zynq_gpio_set to set
> + * the state of the pin to the value specified.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
> +                            int state)
> +{
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +       unsigned int reg, bank_num, bank_pin_num;
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       /* set the GPIO pin as output */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg |= 1 << bank_pin_num;

Like here:
reg |= BIT(bank_pin_num);

> +static int zynq_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       return irq_find_mapping(irq_domain, offset);
> +}

Goes away with GPIOLIB_IRQCHIP which does this in the
gpiolib.

(...)
> +/**
> + * zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
> + * @irq:       irq number of the gpio bank where interrupt has occurred
> + * @desc:      irq descriptor instance of the 'irq'
> + *
> + * This function reads the Interrupt Status Register of each bank to get the
> + * gpio pin number which has triggered an interrupt. It then acks the triggered
> + * interrupt and calls the pin specific handler set by the higher layer
> + * application for that pin.
> + * Note: A bug is reported if no handler is set for the gpio pin.
> + */
> +static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)irq_get_handler_data(irq);
> +       int gpio_irq = gpio->irq_base;

Using a base offset for the IRQ goes totally against the idea of
using an irqdomain!

> +       unsigned int int_sts, int_enb, bank_num;
> +       struct irq_desc *gpio_irq_desc;
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> +       chained_irq_enter(chip, desc);
> +
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               int_sts = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
> +               int_enb = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
> +               int_sts &= ~int_enb;
> +
> +               for (; int_sts != 0; int_sts >>= 1, gpio_irq++) {

Here increase from zero on the offset, then use
irq_find_mapping() to translate the hwirq to a Linux IRQ.

> +                       if (!(int_sts & 1))
> +                               continue;
> +                       gpio_irq_desc = irq_to_desc(gpio_irq);
> +                       BUG_ON(!gpio_irq_desc);
> +                       chip = irq_desc_get_chip(gpio_irq_desc);
> +                       BUG_ON(!chip);
> +                       chip->irq_ack(&gpio_irq_desc->irq_data);
> +
> +                       /* call the pin specific handler */
> +                       generic_handle_irq(gpio_irq);
> +               }
> +               /* shift to first virtual irq of next bank */
> +               gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;

This is also pretty convoluted. Are you sure you don't want to
implement one gpiochip per bank instead? I guess the final "+1"
means there is actually one IRQ per bank even?

> +       }
> +
> +       chip = irq_desc_get_chip(desc);
> +       chained_irq_exit(chip, desc);
> +}

(...)
> +static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       int ret;
> +
> +       ret = pm_runtime_get_sync(chip->dev);
> +
> +       /*
> +        * 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 zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       pm_runtime_put_sync(chip->dev);
> +}
> +
> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
> +                          zynq_gpio_idle)
> +};

Is this runtime PM implementation aligned with Ulf Hansson's recent
new helpers to simplify suspend+runtime PM coexistance?

> +/**
> + * zynq_gpio_probe - Initialization method for a zynq_gpio device
> + * @pdev:      platform device instance
> + *
> + * This function allocates memory resources for the gpio device and registers
> + * all the banks of the device. It will also set up interrupts for the gpio
> + * pins.
> + * Note: Interrupts are disabled for all the banks during initialization.
> + *
> + * Return: 0 on success, negative error otherwise.
> + */
> +static int zynq_gpio_probe(struct platform_device *pdev)
> +{
> +       int ret, pin_num, bank_num, gpio_irq;
> +       unsigned int irq_num;
> +       struct zynq_gpio *gpio;
> +       struct gpio_chip *chip;
> +       struct resource *res;
> +
> +       gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, gpio);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       gpio->base_addr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(gpio->base_addr))
> +               return PTR_ERR(gpio->base_addr);
> +
> +       irq_num = platform_get_irq(pdev, 0);
> +       gpio->irq = irq_num;

>From the code I get the sense that you are passing a single
IRQ as resource but there are actually as many IRQs as there
are banks, is this correct? Then pass *all* IRQs as resources.

> +
> +       /* configure the gpio chip */
> +       chip = &gpio->chip;
> +       chip->label = "zynq_gpio";
> +       chip->owner = THIS_MODULE;
> +       chip->dev = &pdev->dev;
> +       chip->get = zynq_gpio_get_value;
> +       chip->set = zynq_gpio_set_value;
> +       chip->request = zynq_gpio_request;
> +       chip->free = zynq_gpio_free;
> +       chip->direction_input = zynq_gpio_dir_in;
> +       chip->direction_output = zynq_gpio_dir_out;
> +       chip->to_irq = zynq_gpio_to_irq;

Not needed with GPIOLIB_IRQCHIP

> +       chip->dbg_show = NULL;
> +       chip->base = 0;         /* default pin base */

The GPIO numberspace is not the same as the pin number space.
This is only going to work if you have no other GPIO controllers
on your system. Use -1 as base so you get dynamic allocation
of a GPIO number range instead.

Make sure to use the new descriptor API for defining and accessing
GPIOs in all drivers and it will not matter which base you get.
(Good eh? :-)

As it appears you're using device tree, it is all transparent and
you need not worry.

> +       chip->ngpio = ZYNQ_GPIO_NR_GPIOS;
> +       chip->can_sleep = 0;
> +

>From here:

> +       gpio->irq_base = irq_alloc_descs(-1, 0, chip->ngpio, 0);
> +       if (gpio->irq_base < 0) {
> +               dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> +               return -ENODEV;
> +       }
> +
> +       irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
> +                                          chip->ngpio, gpio->irq_base, 0,
> +                                          &irq_domain_simple_ops, NULL);

To here, goes away with GPIOLIB_IRQCHIP

> +
> +       /* report a bug if gpio chip registration fails */
> +       ret = gpiochip_add(chip);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Enable GPIO clock */
> +       gpio->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(gpio->clk)) {
> +               dev_err(&pdev->dev, "input clock not found.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return PTR_ERR(gpio->clk);
> +       }
> +       ret = clk_prepare_enable(gpio->clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Unable to enable clock.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return ret;
> +       }

You should probably get and enable the clock *before* you add the
gpiochip.

> +       /* disable interrupts for all banks */
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               zynq_gpio_writereg(gpio->base_addr +
> +                                  ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
> +                                  ZYNQ_GPIO_IXR_DISABLE_ALL);
> +       }

>From here:

> +       /*
> +        * set the irq chip, handler and irq chip data for callbacks for
> +        * each pin
> +        */
> +       for (pin_num = 0; pin_num < min_t(int, ZYNQ_GPIO_NR_GPIOS,
> +                                         (int)chip->ngpio); pin_num++) {
> +               gpio_irq = irq_find_mapping(irq_domain, pin_num);
> +               irq_set_chip_and_handler(gpio_irq, &zynq_gpio_irqchip,
> +                                        handle_simple_irq);
> +               irq_set_chip_data(gpio_irq, (void *)gpio);
> +               set_irq_flags(gpio_irq, IRQF_VALID);
> +       }
> +
> +       irq_set_handler_data(irq_num, (void *)gpio);
> +       irq_set_chained_handler(irq_num, zynq_gpio_irqhandler);

To here goes away with GPIOLIB_IRQCHIP

> +/**
> + * zynq_gpio_remove - Driver removal function
> + * @pdev:      platform device instance
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_remove(struct platform_device *pdev)
> +{
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       clk_disable_unprepare(gpio->clk);
> +       device_set_wakeup_capable(&pdev->dev, 0);
> +       return 0;

You forgot to remove the gpiochip.

Yours,
Linus Walleij

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:

> Add support for GPIO controller used by Xilinx Zynq
>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>

This will not be integrated before v3.16 so we have some time to
think things over.

> +config GPIO_ZYNQ
> +       bool "Xilinx ZYNQ GPIO support"
> +       depends on ARCH_ZYNQ
> +       select GENERIC_IRQ_CHIP

I think that rather than selecting GENERIC_IRQ_CHIP,
select my new GPIOLIB_IRQCHIP and use the new helpers
gpiochip_irqchip_add() and gpiochip_set_chained_irqchip().

Look in drivers/gpio/gpio-pl061.c for example usage.

This is available in the "devel" or "for-next" branch of my
gpio tree at
https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/

> +#include <linux/clk.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>

These three are auto-included with GPIOLIB_IRQCHIP.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define DRIVER_NAME "zynq-gpio"
> +#define ZYNQ_GPIO_NR_GPIOS     118
> +
> +static struct irq_domain *irq_domain;

Static global variables are a no-no. This goes away with
GPIOLIB_IRQCHIP, but read about state containers in
Documentation/driver-model/design-patterns.txt

> +/* Read/Write access to the GPIO PS registers */
> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
> +{
> +       return readl_relaxed(offset);
> +}
> +
> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
> +{
> +       writel_relaxed(val, offset);
> +}

I think this is unnecessary and confusing indirection.
Just use the readl_relaxed/writel_relaxed functions directly in
the code.

> +static unsigned int zynq_gpio_pin_table[] = {
> +       31, /* 0 - 31 */
> +       53, /* 32 - 53 */
> +       85, /* 54 - 85 */
> +       117 /* 86 - 117 */
> +};

This is an indication that you may be writing a pin control
driver rather than a GPIO driver. Please consule
Documentation/pinctrl.txt for a brief overview of what we
mean by this.

> +/* Maximum banks */
> +#define ZYNQ_GPIO_MAX_BANK     4
> +
> +/* Disable all interrupts mask */
> +#define ZYNQ_GPIO_IXR_DISABLE_ALL      0xFFFFFFFF
> +
> +/* GPIO pin high */
> +#define ZYNQ_GPIO_PIN_HIGH 1

This looks like pin config business.

> +/* Mid pin number of a bank */
> +#define ZYNQ_GPIO_MID_PIN_NUM 16
> +
> +/* GPIO upper 16 bit mask */
> +#define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000
> +
> +/**
> + * struct zynq_gpio - gpio device private data structure
> + * @chip:      instance of the gpio_chip
> + * @base_addr: base address of the GPIO device
> + * @irq:       irq associated with the controller
> + * @irq_base:  base of IRQ number for interrupt
> + * @clk:       clock resource for this controller
> + */
> +struct zynq_gpio {
> +       struct gpio_chip chip;
> +       void __iomem *base_addr;
> +       unsigned int irq;
> +       unsigned int irq_base;

Using a base offset for IRQs is just wrong if you're using irqdomain.
BTW you will be using chip->irqdomain after converting to
GPIOLIB_IRQCHIP.

> +       struct clk *clk;
> +};
> +
> +/**
> + * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
> + * for a given pin in the GPIO device
> + * @pin_num:   gpio pin number within the device
> + * @bank_num:  an output parameter used to return the bank number of the gpio
> + *             pin
> + * @bank_pin_num: an output parameter used to return pin number within a bank
> + *               for the given gpio pin
> + *
> + * Returns the bank number.
> + */
> +static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
> +                                         unsigned int *bank_num,
> +                                         unsigned int *bank_pin_num)
> +{
> +       for (*bank_num = 0; *bank_num < ZYNQ_GPIO_MAX_BANK; (*bank_num)++)
> +               if (pin_num <= zynq_gpio_pin_table[*bank_num])
> +                       break;
> +
> +       if (!(*bank_num))
> +               *bank_pin_num = pin_num;
> +       else
> +               *bank_pin_num = pin_num %
> +                               (zynq_gpio_pin_table[*bank_num - 1] + 1);
> +}

This is reimplementing gpio ranges from the pin control
subsystem. Consult e.g.
drivers/pinctrl/pinctrl-baytrail.c

For another simple driver using GPIO ranges from pin control
for this. And implement it that way.

> +/**
> + * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function reads the state of the specified pin of the GPIO device.
> + *
> + * Return: 0 if the pin is low, 1 if pin is high.
> + */
> +static int zynq_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int bank_num, bank_pin_num, data;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       data = zynq_gpio_readreg(gpio->base_addr +
> +                                ZYNQ_GPIO_DATA_OFFSET(bank_num));
> +       return (data >> bank_pin_num) & ZYNQ_GPIO_PIN_HIGH;
> +}

Instead of fiddling around with the banks like this, consider registering
one gpiochip per bank, so one instance of the device per bank, and
be done with it. Maybe it's hard to do that so not a firm requirement,
but think about it.

(...)
> +/**
> + * zynq_gpio_dir_in - Set the direction of the specified GPIO pin as input
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function uses the read-modify-write sequence to set the direction of
> + * the gpio pin as input.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int reg, bank_num, bank_pin_num;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +       /* clear the bit in direction mode reg to set the pin as input */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg &= ~(1 << bank_pin_num);

In such cases I usually do:

#include <linux/bitops.h>

reg &= ~BIT(bank_pin_num);

(Applied everywhere.)

(...)
> +/**
> + * zynq_gpio_dir_out - Set the direction of the specified GPIO pin as output
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + * @state:     value to be written to specified pin
> + *
> + * This function sets the direction of specified GPIO pin as output, configures
> + * the Output Enable register for the pin and uses zynq_gpio_set to set
> + * the state of the pin to the value specified.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
> +                            int state)
> +{
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +       unsigned int reg, bank_num, bank_pin_num;
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       /* set the GPIO pin as output */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg |= 1 << bank_pin_num;

Like here:
reg |= BIT(bank_pin_num);

> +static int zynq_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       return irq_find_mapping(irq_domain, offset);
> +}

Goes away with GPIOLIB_IRQCHIP which does this in the
gpiolib.

(...)
> +/**
> + * zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
> + * @irq:       irq number of the gpio bank where interrupt has occurred
> + * @desc:      irq descriptor instance of the 'irq'
> + *
> + * This function reads the Interrupt Status Register of each bank to get the
> + * gpio pin number which has triggered an interrupt. It then acks the triggered
> + * interrupt and calls the pin specific handler set by the higher layer
> + * application for that pin.
> + * Note: A bug is reported if no handler is set for the gpio pin.
> + */
> +static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)irq_get_handler_data(irq);
> +       int gpio_irq = gpio->irq_base;

Using a base offset for the IRQ goes totally against the idea of
using an irqdomain!

> +       unsigned int int_sts, int_enb, bank_num;
> +       struct irq_desc *gpio_irq_desc;
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> +       chained_irq_enter(chip, desc);
> +
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               int_sts = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
> +               int_enb = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
> +               int_sts &= ~int_enb;
> +
> +               for (; int_sts != 0; int_sts >>= 1, gpio_irq++) {

Here increase from zero on the offset, then use
irq_find_mapping() to translate the hwirq to a Linux IRQ.

> +                       if (!(int_sts & 1))
> +                               continue;
> +                       gpio_irq_desc = irq_to_desc(gpio_irq);
> +                       BUG_ON(!gpio_irq_desc);
> +                       chip = irq_desc_get_chip(gpio_irq_desc);
> +                       BUG_ON(!chip);
> +                       chip->irq_ack(&gpio_irq_desc->irq_data);
> +
> +                       /* call the pin specific handler */
> +                       generic_handle_irq(gpio_irq);
> +               }
> +               /* shift to first virtual irq of next bank */
> +               gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;

This is also pretty convoluted. Are you sure you don't want to
implement one gpiochip per bank instead? I guess the final "+1"
means there is actually one IRQ per bank even?

> +       }
> +
> +       chip = irq_desc_get_chip(desc);
> +       chained_irq_exit(chip, desc);
> +}

(...)
> +static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       int ret;
> +
> +       ret = pm_runtime_get_sync(chip->dev);
> +
> +       /*
> +        * 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 zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       pm_runtime_put_sync(chip->dev);
> +}
> +
> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
> +                          zynq_gpio_idle)
> +};

Is this runtime PM implementation aligned with Ulf Hansson's recent
new helpers to simplify suspend+runtime PM coexistance?

> +/**
> + * zynq_gpio_probe - Initialization method for a zynq_gpio device
> + * @pdev:      platform device instance
> + *
> + * This function allocates memory resources for the gpio device and registers
> + * all the banks of the device. It will also set up interrupts for the gpio
> + * pins.
> + * Note: Interrupts are disabled for all the banks during initialization.
> + *
> + * Return: 0 on success, negative error otherwise.
> + */
> +static int zynq_gpio_probe(struct platform_device *pdev)
> +{
> +       int ret, pin_num, bank_num, gpio_irq;
> +       unsigned int irq_num;
> +       struct zynq_gpio *gpio;
> +       struct gpio_chip *chip;
> +       struct resource *res;
> +
> +       gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, gpio);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       gpio->base_addr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(gpio->base_addr))
> +               return PTR_ERR(gpio->base_addr);
> +
> +       irq_num = platform_get_irq(pdev, 0);
> +       gpio->irq = irq_num;

>From the code I get the sense that you are passing a single
IRQ as resource but there are actually as many IRQs as there
are banks, is this correct? Then pass *all* IRQs as resources.

> +
> +       /* configure the gpio chip */
> +       chip = &gpio->chip;
> +       chip->label = "zynq_gpio";
> +       chip->owner = THIS_MODULE;
> +       chip->dev = &pdev->dev;
> +       chip->get = zynq_gpio_get_value;
> +       chip->set = zynq_gpio_set_value;
> +       chip->request = zynq_gpio_request;
> +       chip->free = zynq_gpio_free;
> +       chip->direction_input = zynq_gpio_dir_in;
> +       chip->direction_output = zynq_gpio_dir_out;
> +       chip->to_irq = zynq_gpio_to_irq;

Not needed with GPIOLIB_IRQCHIP

> +       chip->dbg_show = NULL;
> +       chip->base = 0;         /* default pin base */

The GPIO numberspace is not the same as the pin number space.
This is only going to work if you have no other GPIO controllers
on your system. Use -1 as base so you get dynamic allocation
of a GPIO number range instead.

Make sure to use the new descriptor API for defining and accessing
GPIOs in all drivers and it will not matter which base you get.
(Good eh? :-)

As it appears you're using device tree, it is all transparent and
you need not worry.

> +       chip->ngpio = ZYNQ_GPIO_NR_GPIOS;
> +       chip->can_sleep = 0;
> +

>From here:

> +       gpio->irq_base = irq_alloc_descs(-1, 0, chip->ngpio, 0);
> +       if (gpio->irq_base < 0) {
> +               dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> +               return -ENODEV;
> +       }
> +
> +       irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
> +                                          chip->ngpio, gpio->irq_base, 0,
> +                                          &irq_domain_simple_ops, NULL);

To here, goes away with GPIOLIB_IRQCHIP

> +
> +       /* report a bug if gpio chip registration fails */
> +       ret = gpiochip_add(chip);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Enable GPIO clock */
> +       gpio->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(gpio->clk)) {
> +               dev_err(&pdev->dev, "input clock not found.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return PTR_ERR(gpio->clk);
> +       }
> +       ret = clk_prepare_enable(gpio->clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Unable to enable clock.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return ret;
> +       }

You should probably get and enable the clock *before* you add the
gpiochip.

> +       /* disable interrupts for all banks */
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               zynq_gpio_writereg(gpio->base_addr +
> +                                  ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
> +                                  ZYNQ_GPIO_IXR_DISABLE_ALL);
> +       }

>From here:

> +       /*
> +        * set the irq chip, handler and irq chip data for callbacks for
> +        * each pin
> +        */
> +       for (pin_num = 0; pin_num < min_t(int, ZYNQ_GPIO_NR_GPIOS,
> +                                         (int)chip->ngpio); pin_num++) {
> +               gpio_irq = irq_find_mapping(irq_domain, pin_num);
> +               irq_set_chip_and_handler(gpio_irq, &zynq_gpio_irqchip,
> +                                        handle_simple_irq);
> +               irq_set_chip_data(gpio_irq, (void *)gpio);
> +               set_irq_flags(gpio_irq, IRQF_VALID);
> +       }
> +
> +       irq_set_handler_data(irq_num, (void *)gpio);
> +       irq_set_chained_handler(irq_num, zynq_gpio_irqhandler);

To here goes away with GPIOLIB_IRQCHIP

> +/**
> + * zynq_gpio_remove - Driver removal function
> + * @pdev:      platform device instance
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_remove(struct platform_device *pdev)
> +{
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       clk_disable_unprepare(gpio->clk);
> +       device_set_wakeup_capable(&pdev->dev, 0);
> +       return 0;

You forgot to remove the gpiochip.

Yours,
Linus Walleij

Re: [PATCH 2/2] Devicetree: Add Zynq GPIO devicetree bindings documentation

[Linus Walleij]

On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:

> Add gpio-zynq bindings documentation.
>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>

No comments really, but you should create one instance per
bank I think.

Yours,
Linus Walleij

Re: [PATCH 2/2] Devicetree: Add Zynq GPIO devicetree bindings documentation

[Linus Walleij]

On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:

> Add gpio-zynq bindings documentation.
>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>

No comments really, but you should create one instance per
bank I think.

Yours,
Linus Walleij

[PATCH 2/2] Devicetree: Add Zynq GPIO devicetree bindings documentation

[Linus Walleij]

On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:

> Add gpio-zynq bindings documentation.
>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>

No comments really, but you should create one instance per
bank I think.

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Harini Katakam]

Hi Linus,

On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
>
>> Add support for GPIO controller used by Xilinx Zynq
>>
>> Signed-off-by: Harini Katakam <harinik@xilinx.com>
>
> This will not be integrated before v3.16 so we have some time to
> think things over.
>
>> +config GPIO_ZYNQ
>> +       bool "Xilinx ZYNQ GPIO support"
>> +       depends on ARCH_ZYNQ
>> +       select GENERIC_IRQ_CHIP
>
> I think that rather than selecting GENERIC_IRQ_CHIP,
> select my new GPIOLIB_IRQCHIP and use the new helpers
> gpiochip_irqchip_add() and gpiochip_set_chained_irqchip().
>
> Look in drivers/gpio/gpio-pl061.c for example usage.
>
> This is available in the "devel" or "for-next" branch of my
> gpio tree at
> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/
>

OK.

<snip>
>> +/* Read/Write access to the GPIO PS registers */
>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>> +{
>> +       return readl_relaxed(offset);
>> +}
>> +
>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>> +{
>> +       writel_relaxed(val, offset);
>> +}
>
> I think this is unnecessary and confusing indirection.
> Just use the readl_relaxed/writel_relaxed functions directly in
> the code.
>

This is just to be flexible.

<snip>
>> +                       if (!(int_sts & 1))
>> +                               continue;
>> +                       gpio_irq_desc = irq_to_desc(gpio_irq);
>> +                       BUG_ON(!gpio_irq_desc);
>> +                       chip = irq_desc_get_chip(gpio_irq_desc);
>> +                       BUG_ON(!chip);
>> +                       chip->irq_ack(&gpio_irq_desc->irq_data);
>> +
>> +                       /* call the pin specific handler */
>> +                       generic_handle_irq(gpio_irq);
>> +               }
>> +               /* shift to first virtual irq of next bank */
>> +               gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;
>
> This is also pretty convoluted. Are you sure you don't want to
> implement one gpiochip per bank instead? I guess the final "+1"
> means there is actually one IRQ per bank even?
>

There is only one IRQ for all four banks.
I will try to make this logic and other places in the driver you pointed out,
less convoluted.
The handling is (very) slightly different for the upper two banks and
lower two banks.

<snip>
>> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
>> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
>> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
>> +                          zynq_gpio_idle)
>> +};
>
> Is this runtime PM implementation aligned with Ulf Hansson's recent
> new helpers to simplify suspend+runtime PM coexistance?
>

I'm sorry i just looked at them -
pm_runtime_force_suspend/resume are not used here.

>> +       chip->dbg_show = NULL;
>> +       chip->base = 0;         /* default pin base */
>
> The GPIO numberspace is not the same as the pin number space.
> This is only going to work if you have no other GPIO controllers
> on your system. Use -1 as base so you get dynamic allocation
> of a GPIO number range instead.
>
> Make sure to use the new descriptor API for defining and accessing
> GPIOs in all drivers and it will not matter which base you get.
> (Good eh? :-)
>
> As it appears you're using device tree, it is all transparent and
> you need not worry.
>

OK.

<snip>

Thanks for the review!

Regards,
Harini

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Harini Katakam]

Hi Linus,

On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
>
>> Add support for GPIO controller used by Xilinx Zynq
>>
>> Signed-off-by: Harini Katakam <harinik@xilinx.com>
>
> This will not be integrated before v3.16 so we have some time to
> think things over.
>
>> +config GPIO_ZYNQ
>> +       bool "Xilinx ZYNQ GPIO support"
>> +       depends on ARCH_ZYNQ
>> +       select GENERIC_IRQ_CHIP
>
> I think that rather than selecting GENERIC_IRQ_CHIP,
> select my new GPIOLIB_IRQCHIP and use the new helpers
> gpiochip_irqchip_add() and gpiochip_set_chained_irqchip().
>
> Look in drivers/gpio/gpio-pl061.c for example usage.
>
> This is available in the "devel" or "for-next" branch of my
> gpio tree at
> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/
>

OK.

<snip>
>> +/* Read/Write access to the GPIO PS registers */
>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>> +{
>> +       return readl_relaxed(offset);
>> +}
>> +
>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>> +{
>> +       writel_relaxed(val, offset);
>> +}
>
> I think this is unnecessary and confusing indirection.
> Just use the readl_relaxed/writel_relaxed functions directly in
> the code.
>

This is just to be flexible.

<snip>
>> +                       if (!(int_sts & 1))
>> +                               continue;
>> +                       gpio_irq_desc = irq_to_desc(gpio_irq);
>> +                       BUG_ON(!gpio_irq_desc);
>> +                       chip = irq_desc_get_chip(gpio_irq_desc);
>> +                       BUG_ON(!chip);
>> +                       chip->irq_ack(&gpio_irq_desc->irq_data);
>> +
>> +                       /* call the pin specific handler */
>> +                       generic_handle_irq(gpio_irq);
>> +               }
>> +               /* shift to first virtual irq of next bank */
>> +               gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;
>
> This is also pretty convoluted. Are you sure you don't want to
> implement one gpiochip per bank instead? I guess the final "+1"
> means there is actually one IRQ per bank even?
>

There is only one IRQ for all four banks.
I will try to make this logic and other places in the driver you pointed out,
less convoluted.
The handling is (very) slightly different for the upper two banks and
lower two banks.

<snip>
>> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
>> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
>> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
>> +                          zynq_gpio_idle)
>> +};
>
> Is this runtime PM implementation aligned with Ulf Hansson's recent
> new helpers to simplify suspend+runtime PM coexistance?
>

I'm sorry i just looked at them -
pm_runtime_force_suspend/resume are not used here.

>> +       chip->dbg_show = NULL;
>> +       chip->base = 0;         /* default pin base */
>
> The GPIO numberspace is not the same as the pin number space.
> This is only going to work if you have no other GPIO controllers
> on your system. Use -1 as base so you get dynamic allocation
> of a GPIO number range instead.
>
> Make sure to use the new descriptor API for defining and accessing
> GPIOs in all drivers and it will not matter which base you get.
> (Good eh? :-)
>
> As it appears you're using device tree, it is all transparent and
> you need not worry.
>

OK.

<snip>

Thanks for the review!

Regards,
Harini

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Harini Katakam]

Hi Linus,

On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
>
>> Add support for GPIO controller used by Xilinx Zynq
>>
>> Signed-off-by: Harini Katakam <harinik@xilinx.com>
>
> This will not be integrated before v3.16 so we have some time to
> think things over.
>
>> +config GPIO_ZYNQ
>> +       bool "Xilinx ZYNQ GPIO support"
>> +       depends on ARCH_ZYNQ
>> +       select GENERIC_IRQ_CHIP
>
> I think that rather than selecting GENERIC_IRQ_CHIP,
> select my new GPIOLIB_IRQCHIP and use the new helpers
> gpiochip_irqchip_add() and gpiochip_set_chained_irqchip().
>
> Look in drivers/gpio/gpio-pl061.c for example usage.
>
> This is available in the "devel" or "for-next" branch of my
> gpio tree at
> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/
>

OK.

<snip>
>> +/* Read/Write access to the GPIO PS registers */
>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>> +{
>> +       return readl_relaxed(offset);
>> +}
>> +
>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>> +{
>> +       writel_relaxed(val, offset);
>> +}
>
> I think this is unnecessary and confusing indirection.
> Just use the readl_relaxed/writel_relaxed functions directly in
> the code.
>

This is just to be flexible.

<snip>
>> +                       if (!(int_sts & 1))
>> +                               continue;
>> +                       gpio_irq_desc = irq_to_desc(gpio_irq);
>> +                       BUG_ON(!gpio_irq_desc);
>> +                       chip = irq_desc_get_chip(gpio_irq_desc);
>> +                       BUG_ON(!chip);
>> +                       chip->irq_ack(&gpio_irq_desc->irq_data);
>> +
>> +                       /* call the pin specific handler */
>> +                       generic_handle_irq(gpio_irq);
>> +               }
>> +               /* shift to first virtual irq of next bank */
>> +               gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;
>
> This is also pretty convoluted. Are you sure you don't want to
> implement one gpiochip per bank instead? I guess the final "+1"
> means there is actually one IRQ per bank even?
>

There is only one IRQ for all four banks.
I will try to make this logic and other places in the driver you pointed out,
less convoluted.
The handling is (very) slightly different for the upper two banks and
lower two banks.

<snip>
>> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
>> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
>> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
>> +                          zynq_gpio_idle)
>> +};
>
> Is this runtime PM implementation aligned with Ulf Hansson's recent
> new helpers to simplify suspend+runtime PM coexistance?
>

I'm sorry i just looked at them -
pm_runtime_force_suspend/resume are not used here.

>> +       chip->dbg_show = NULL;
>> +       chip->base = 0;         /* default pin base */
>
> The GPIO numberspace is not the same as the pin number space.
> This is only going to work if you have no other GPIO controllers
> on your system. Use -1 as base so you get dynamic allocation
> of a GPIO number range instead.
>
> Make sure to use the new descriptor API for defining and accessing
> GPIOs in all drivers and it will not matter which base you get.
> (Good eh? :-)
>
> As it appears you're using device tree, it is all transparent and
> you need not worry.
>

OK.

<snip>

Thanks for the review!

Regards,
Harini

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
<harinikatakamlinux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:

>>> +/* Read/Write access to the GPIO PS registers */
>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>> +{
>>> +       return readl_relaxed(offset);
>>> +}
>>> +
>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>> +{
>>> +       writel_relaxed(val, offset);
>>> +}
>>
>> I think this is unnecessary and confusing indirection.
>> Just use the readl_relaxed/writel_relaxed functions directly in
>> the code.
>>
>
> This is just to be flexible.

Define exactly what you mean with "flexible" in this context. I
only see unnecessary overhead and hard-to-read code.

>> This is also pretty convoluted. Are you sure you don't want to
>> implement one gpiochip per bank instead? I guess the final "+1"
>> means there is actually one IRQ per bank even?
>
> There is only one IRQ for all four banks.

OK I get it. Then it makes sense to have all banks registered as
one device and the IRQ tied to this one device.

>>> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
>>> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
>>> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
>>> +                          zynq_gpio_idle)
>>> +};
>>
>> Is this runtime PM implementation aligned with Ulf Hansson's recent
>> new helpers to simplify suspend+runtime PM coexistance?
>>
>
> I'm sorry i just looked at them -
> pm_runtime_force_suspend/resume are not used here.

Sorry, I was more thinking of change
717e5d458e3bfca495a38dca61c64f274c049e46
"PM / Runtime: Implement the pm_generic_runtime functions for CONFIG_PM"

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
<harinikatakamlinux@gmail.com> wrote:
> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:

>>> +/* Read/Write access to the GPIO PS registers */
>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>> +{
>>> +       return readl_relaxed(offset);
>>> +}
>>> +
>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>> +{
>>> +       writel_relaxed(val, offset);
>>> +}
>>
>> I think this is unnecessary and confusing indirection.
>> Just use the readl_relaxed/writel_relaxed functions directly in
>> the code.
>>
>
> This is just to be flexible.

Define exactly what you mean with "flexible" in this context. I
only see unnecessary overhead and hard-to-read code.

>> This is also pretty convoluted. Are you sure you don't want to
>> implement one gpiochip per bank instead? I guess the final "+1"
>> means there is actually one IRQ per bank even?
>
> There is only one IRQ for all four banks.

OK I get it. Then it makes sense to have all banks registered as
one device and the IRQ tied to this one device.

>>> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
>>> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
>>> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
>>> +                          zynq_gpio_idle)
>>> +};
>>
>> Is this runtime PM implementation aligned with Ulf Hansson's recent
>> new helpers to simplify suspend+runtime PM coexistance?
>>
>
> I'm sorry i just looked at them -
> pm_runtime_force_suspend/resume are not used here.

Sorry, I was more thinking of change
717e5d458e3bfca495a38dca61c64f274c049e46
"PM / Runtime: Implement the pm_generic_runtime functions for CONFIG_PM"

Yours,
Linus Walleij

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
<harinikatakamlinux@gmail.com> wrote:
> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:

>>> +/* Read/Write access to the GPIO PS registers */
>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>> +{
>>> +       return readl_relaxed(offset);
>>> +}
>>> +
>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>> +{
>>> +       writel_relaxed(val, offset);
>>> +}
>>
>> I think this is unnecessary and confusing indirection.
>> Just use the readl_relaxed/writel_relaxed functions directly in
>> the code.
>>
>
> This is just to be flexible.

Define exactly what you mean with "flexible" in this context. I
only see unnecessary overhead and hard-to-read code.

>> This is also pretty convoluted. Are you sure you don't want to
>> implement one gpiochip per bank instead? I guess the final "+1"
>> means there is actually one IRQ per bank even?
>
> There is only one IRQ for all four banks.

OK I get it. Then it makes sense to have all banks registered as
one device and the IRQ tied to this one device.

>>> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
>>> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
>>> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
>>> +                          zynq_gpio_idle)
>>> +};
>>
>> Is this runtime PM implementation aligned with Ulf Hansson's recent
>> new helpers to simplify suspend+runtime PM coexistance?
>>
>
> I'm sorry i just looked at them -
> pm_runtime_force_suspend/resume are not used here.

Sorry, I was more thinking of change
717e5d458e3bfca495a38dca61c64f274c049e46
"PM / Runtime: Implement the pm_generic_runtime functions for CONFIG_PM"

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Ulf Hansson]

On 27 March 2014 16:25, Harini Katakam <harinik@xilinx.com> wrote:
> Add support for GPIO controller used by Xilinx Zynq
>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
>  drivers/gpio/Kconfig     |    7 +
>  drivers/gpio/Makefile    |    1 +
>  drivers/gpio/gpio-zynq.c |  690 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 698 insertions(+)
>  create mode 100644 drivers/gpio/gpio-zynq.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 903f24d..67a22a6 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -313,6 +313,13 @@ config GPIO_XTENSA
>           Say yes here to support the Xtensa internal GPIO32 IMPWIRE (input)
>           and EXPSTATE (output) ports
>
> +config GPIO_ZYNQ
> +       bool "Xilinx ZYNQ GPIO support"
> +       depends on ARCH_ZYNQ
> +       select GENERIC_IRQ_CHIP
> +       help
> +        Say yes here to support Xilinx ZYNQ GPIO controller.
> +
>  config GPIO_VR41XX
>         tristate "NEC VR4100 series General-purpose I/O Uint support"
>         depends on CPU_VR41XX
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 5d50179..439f23a 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -99,3 +99,4 @@ obj-$(CONFIG_GPIO_WM8350)     += gpio-wm8350.o
>  obj-$(CONFIG_GPIO_WM8994)      += gpio-wm8994.o
>  obj-$(CONFIG_GPIO_XILINX)      += gpio-xilinx.o
>  obj-$(CONFIG_GPIO_XTENSA)      += gpio-xtensa.o
> +obj-$(CONFIG_GPIO_ZYNQ)                += gpio-zynq.o
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> new file mode 100644
> index 0000000..1f5fdfc
> --- /dev/null
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -0,0 +1,690 @@
> +/*
> + * Xilinx Zynq GPIO device driver
> + *
> + * Copyright (C) 2009 - 2014 Xilinx, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License as published by the Free Software
> + * Foundation; either version 2 of the License, or (at your option) any later
> + * version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define DRIVER_NAME "zynq-gpio"
> +#define ZYNQ_GPIO_NR_GPIOS     118
> +
> +static struct irq_domain *irq_domain;
> +
> +/* Register offsets for the GPIO device */
> +
> +/* LSW Mask & Data -WO */
> +#define ZYNQ_GPIO_DATA_LSW_OFFSET(BANK)        (0x000 + (8 * BANK))
> +/* MSW Mask & Data -WO */
> +#define ZYNQ_GPIO_DATA_MSW_OFFSET(BANK)        (0x004 + (8 * BANK))
> +/* Data Register-RW */
> +#define ZYNQ_GPIO_DATA_OFFSET(BANK)    (0x040 + (4 * BANK))
> +/* Direction mode reg-RW */
> +#define ZYNQ_GPIO_DIRM_OFFSET(BANK)    (0x204 + (0x40 * BANK))
> +/* Output enable reg-RW */
> +#define ZYNQ_GPIO_OUTEN_OFFSET(BANK)   (0x208 + (0x40 * BANK))
> +/* Interrupt mask reg-RO */
> +#define ZYNQ_GPIO_INTMASK_OFFSET(BANK) (0x20C + (0x40 * BANK))
> +/* Interrupt enable reg-WO */
> +#define ZYNQ_GPIO_INTEN_OFFSET(BANK)   (0x210 + (0x40 * BANK))
> +/* Interrupt disable reg-WO */
> +#define ZYNQ_GPIO_INTDIS_OFFSET(BANK)  (0x214 + (0x40 * BANK))
> +/* Interrupt status reg-RO */
> +#define ZYNQ_GPIO_INTSTS_OFFSET(BANK)  (0x218 + (0x40 * BANK))
> +/* Interrupt type reg-RW */
> +#define ZYNQ_GPIO_INTTYPE_OFFSET(BANK) (0x21C + (0x40 * BANK))
> +/* Interrupt polarity reg-RW */
> +#define ZYNQ_GPIO_INTPOL_OFFSET(BANK)  (0x220 + (0x40 * BANK))
> +/* Interrupt on any, reg-RW */
> +#define ZYNQ_GPIO_INTANY_OFFSET(BANK)  (0x224 + (0x40 * BANK))
> +
> +/* Read/Write access to the GPIO PS registers */
> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
> +{
> +       return readl_relaxed(offset);
> +}
> +
> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
> +{
> +       writel_relaxed(val, offset);
> +}
> +
> +static unsigned int zynq_gpio_pin_table[] = {
> +       31, /* 0 - 31 */
> +       53, /* 32 - 53 */
> +       85, /* 54 - 85 */
> +       117 /* 86 - 117 */
> +};
> +
> +/* Maximum banks */
> +#define ZYNQ_GPIO_MAX_BANK     4
> +
> +/* Disable all interrupts mask */
> +#define ZYNQ_GPIO_IXR_DISABLE_ALL      0xFFFFFFFF
> +
> +/* GPIO pin high */
> +#define ZYNQ_GPIO_PIN_HIGH 1
> +
> +/* Mid pin number of a bank */
> +#define ZYNQ_GPIO_MID_PIN_NUM 16
> +
> +/* GPIO upper 16 bit mask */
> +#define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000
> +
> +/**
> + * struct zynq_gpio - gpio device private data structure
> + * @chip:      instance of the gpio_chip
> + * @base_addr: base address of the GPIO device
> + * @irq:       irq associated with the controller
> + * @irq_base:  base of IRQ number for interrupt
> + * @clk:       clock resource for this controller
> + */
> +struct zynq_gpio {
> +       struct gpio_chip chip;
> +       void __iomem *base_addr;
> +       unsigned int irq;
> +       unsigned int irq_base;
> +       struct clk *clk;
> +};
> +
> +/**
> + * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
> + * for a given pin in the GPIO device
> + * @pin_num:   gpio pin number within the device
> + * @bank_num:  an output parameter used to return the bank number of the gpio
> + *             pin
> + * @bank_pin_num: an output parameter used to return pin number within a bank
> + *               for the given gpio pin
> + *
> + * Returns the bank number.
> + */
> +static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
> +                                         unsigned int *bank_num,
> +                                         unsigned int *bank_pin_num)
> +{
> +       for (*bank_num = 0; *bank_num < ZYNQ_GPIO_MAX_BANK; (*bank_num)++)
> +               if (pin_num <= zynq_gpio_pin_table[*bank_num])
> +                       break;
> +
> +       if (!(*bank_num))
> +               *bank_pin_num = pin_num;
> +       else
> +               *bank_pin_num = pin_num %
> +                               (zynq_gpio_pin_table[*bank_num - 1] + 1);
> +}
> +
> +/**
> + * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function reads the state of the specified pin of the GPIO device.
> + *
> + * Return: 0 if the pin is low, 1 if pin is high.
> + */
> +static int zynq_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int bank_num, bank_pin_num, data;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       data = zynq_gpio_readreg(gpio->base_addr +
> +                                ZYNQ_GPIO_DATA_OFFSET(bank_num));
> +       return (data >> bank_pin_num) & ZYNQ_GPIO_PIN_HIGH;
> +}
> +
> +/**
> + * zynq_gpio_set_value - Modify the state of the pin with specified value
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + * @state:     value used to modify the state of the specified pin
> + *
> + * This function calculates the register offset (i.e to lower 16 bits or
> + * upper 16 bits) based on the given pin number and sets the state of a
> + * gpio pin to the specified value. The state is either 0 or non-zero.
> + */
> +static void zynq_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
> +                               int state)
> +{
> +       unsigned int reg_offset, bank_num, bank_pin_num;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       if (bank_pin_num >= ZYNQ_GPIO_MID_PIN_NUM) {
> +               /* only 16 data bits in bit maskable reg */
> +               bank_pin_num -= ZYNQ_GPIO_MID_PIN_NUM;
> +               reg_offset = ZYNQ_GPIO_DATA_MSW_OFFSET(bank_num);
> +       } else {
> +               reg_offset = ZYNQ_GPIO_DATA_LSW_OFFSET(bank_num);
> +       }
> +
> +       /*
> +        * get the 32 bit value to be written to the mask/data register where
> +        * the upper 16 bits is the mask and lower 16 bits is the data
> +        */
> +       if (state)
> +               state = 1;
> +       state = ~(1 << (bank_pin_num + ZYNQ_GPIO_MID_PIN_NUM)) &
> +               ((state << bank_pin_num) | ZYNQ_GPIO_UPPER_MASK);
> +
> +       zynq_gpio_writereg(gpio->base_addr + reg_offset, state);
> +}
> +
> +/**
> + * zynq_gpio_dir_in - Set the direction of the specified GPIO pin as input
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function uses the read-modify-write sequence to set the direction of
> + * the gpio pin as input.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int reg, bank_num, bank_pin_num;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +       /* clear the bit in direction mode reg to set the pin as input */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg &= ~(1 << bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num),
> +                          reg);
> +
> +       return 0;
> +}
> +
> +/**
> + * zynq_gpio_dir_out - Set the direction of the specified GPIO pin as output
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + * @state:     value to be written to specified pin
> + *
> + * This function sets the direction of specified GPIO pin as output, configures
> + * the Output Enable register for the pin and uses zynq_gpio_set to set
> + * the state of the pin to the value specified.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
> +                            int state)
> +{
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +       unsigned int reg, bank_num, bank_pin_num;
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       /* set the GPIO pin as output */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg |= 1 << bank_pin_num;
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num),
> +                          reg);
> +
> +       /* configure the output enable reg for the pin */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_OUTEN_OFFSET(bank_num));
> +       reg |= 1 << bank_pin_num;
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_OUTEN_OFFSET(bank_num),
> +                          reg);
> +
> +       /* set the state of the pin */
> +       zynq_gpio_set_value(chip, pin, state);
> +       return 0;
> +}
> +
> +static int zynq_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       return irq_find_mapping(irq_domain, offset);
> +}
> +
> +/**
> + * zynq_gpio_irq_ack - Acknowledge the interrupt of a gpio pin
> + * @irq_data:  irq data containing irq number of gpio pin for the interrupt
> + *             to ack
> + *
> + * This function calculates gpio pin number from irq number and sets the bit
> + * in the Interrupt Status Register of the corresponding bank, to ACK the irq.
> + */
> +static void zynq_gpio_irq_ack(struct irq_data *irq_data)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)
> +                                irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTSTS_OFFSET(bank_num),
> +                          1 << bank_pin_num);
> +}
> +
> +/**
> + * zynq_gpio_irq_mask - Disable the interrupts for a gpio pin
> + * @irq_data:  per irq and chip data passed down to chip functions
> + *
> + * This function calculates gpio pin number from irq number and sets the
> + * bit in the Interrupt Disable register of the corresponding bank to disable
> + * interrupts for that pin.
> + */
> +static void zynq_gpio_irq_mask(struct irq_data *irq_data)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)
> +                                irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
> +                          1 << bank_pin_num);
> +}
> +
> +/**
> + * zynq_gpio_irq_unmask - Enable the interrupts for a gpio pin
> + * @irq_data:  irq data containing irq number of gpio pin for the interrupt
> + *             to enable
> + *
> + * This function calculates the gpio pin number from irq number and sets the
> + * bit in the Interrupt Enable register of the corresponding bank to enable
> + * interrupts for that pin.
> + */
> +static void zynq_gpio_irq_unmask(struct irq_data *irq_data)
> +{
> +       struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTEN_OFFSET(bank_num),
> +                          1 << bank_pin_num);
> +}
> +
> +/**
> + * zynq_gpio_set_irq_type - Set the irq type for a gpio pin
> + * @irq_data:  irq data containing irq number of gpio pin
> + * @type:      interrupt type that is to be set for the gpio pin
> + *
> + * This function gets the gpio pin number and its bank from the gpio pin number
> + * and configures the INT_TYPE, INT_POLARITY and INT_ANY registers.
> + *
> + * Return: 0, negative error otherwise.
> + * TYPE-EDGE_RISING,  INT_TYPE - 1, INT_POLARITY - 1,  INT_ANY - 0;
> + * TYPE-EDGE_FALLING, INT_TYPE - 1, INT_POLARITY - 0,  INT_ANY - 0;
> + * TYPE-EDGE_BOTH,    INT_TYPE - 1, INT_POLARITY - NA, INT_ANY - 1;
> + * TYPE-LEVEL_HIGH,   INT_TYPE - 0, INT_POLARITY - 1,  INT_ANY - NA;
> + * TYPE-LEVEL_LOW,    INT_TYPE - 0, INT_POLARITY - 0,  INT_ANY - NA
> + */
> +static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
> +{
> +       struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +       unsigned int int_type, int_pol, int_any;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +
> +       int_type = zynq_gpio_readreg(gpio->base_addr +
> +                                    ZYNQ_GPIO_INTTYPE_OFFSET(bank_num));
> +       int_pol = zynq_gpio_readreg(gpio->base_addr +
> +                                   ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
> +       int_any = zynq_gpio_readreg(gpio->base_addr +
> +                                   ZYNQ_GPIO_INTANY_OFFSET(bank_num));
> +
> +       /*
> +        * based on the type requested, configure the INT_TYPE, INT_POLARITY
> +        * and INT_ANY registers
> +        */
> +       switch (type) {
> +       case IRQ_TYPE_EDGE_RISING:
> +               int_type |= (1 << bank_pin_num);
> +               int_pol |= (1 << bank_pin_num);
> +               int_any &= ~(1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               int_type |= (1 << bank_pin_num);
> +               int_pol &= ~(1 << bank_pin_num);
> +               int_any &= ~(1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_EDGE_BOTH:
> +               int_type |= (1 << bank_pin_num);
> +               int_any |= (1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               int_type &= ~(1 << bank_pin_num);
> +               int_pol |= (1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_LEVEL_LOW:
> +               int_type &= ~(1 << bank_pin_num);
> +               int_pol &= ~(1 << bank_pin_num);
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       zynq_gpio_writereg(gpio->base_addr +
> +                          ZYNQ_GPIO_INTTYPE_OFFSET(bank_num), int_type);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTPOL_OFFSET(bank_num),
> +                          int_pol);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTANY_OFFSET(bank_num),
> +                          int_any);
> +       return 0;
> +}
> +
> +static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
> +{
> +       if (on)
> +               zynq_gpio_irq_unmask(data);
> +       else
> +               zynq_gpio_irq_mask(data);
> +
> +       return 0;
> +}
> +
> +/* irq chip descriptor */
> +static struct irq_chip zynq_gpio_irqchip = {
> +       .name           = DRIVER_NAME,
> +       .irq_ack        = zynq_gpio_irq_ack,
> +       .irq_mask       = zynq_gpio_irq_mask,
> +       .irq_unmask     = zynq_gpio_irq_unmask,
> +       .irq_set_type   = zynq_gpio_set_irq_type,
> +       .irq_set_wake   = zynq_gpio_set_wake,
> +};
> +
> +/**
> + * zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
> + * @irq:       irq number of the gpio bank where interrupt has occurred
> + * @desc:      irq descriptor instance of the 'irq'
> + *
> + * This function reads the Interrupt Status Register of each bank to get the
> + * gpio pin number which has triggered an interrupt. It then acks the triggered
> + * interrupt and calls the pin specific handler set by the higher layer
> + * application for that pin.
> + * Note: A bug is reported if no handler is set for the gpio pin.
> + */
> +static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)irq_get_handler_data(irq);
> +       int gpio_irq = gpio->irq_base;
> +       unsigned int int_sts, int_enb, bank_num;
> +       struct irq_desc *gpio_irq_desc;
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> +       chained_irq_enter(chip, desc);
> +
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               int_sts = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
> +               int_enb = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
> +               int_sts &= ~int_enb;
> +
> +               for (; int_sts != 0; int_sts >>= 1, gpio_irq++) {
> +                       if (!(int_sts & 1))
> +                               continue;
> +                       gpio_irq_desc = irq_to_desc(gpio_irq);
> +                       BUG_ON(!gpio_irq_desc);
> +                       chip = irq_desc_get_chip(gpio_irq_desc);
> +                       BUG_ON(!chip);
> +                       chip->irq_ack(&gpio_irq_desc->irq_data);
> +
> +                       /* call the pin specific handler */
> +                       generic_handle_irq(gpio_irq);
> +               }
> +               /* shift to first virtual irq of next bank */
> +               gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;
> +       }
> +
> +       chip = irq_desc_get_chip(desc);
> +       chained_irq_exit(chip, desc);
> +}
> +
> +static int __maybe_unused zynq_gpio_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       if (!device_may_wakeup(dev)) {
> +               if (!pm_runtime_suspended(dev))

Would it not be more convenient to use pm_runtime_force_suspend() here?

> +                       clk_disable(gpio->clk);
> +               return 0;
> +       }
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused zynq_gpio_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       if (!device_may_wakeup(dev)) {
> +               if (!pm_runtime_suspended(dev))

Would it not be more convenient to use pm_runtime_force_resume() here?

> +                       return clk_enable(gpio->clk);
> +       }
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       clk_disable(gpio->clk);

You should be able can use clk_disable_unprepare() here.

> +
> +       return 0;
> +}
> +
> +static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       return clk_enable(gpio->clk);

You should be able can use clk_prepare_enable() here.

> +}
> +
> +static int __maybe_unused zynq_gpio_idle(struct device *dev)
> +{
> +       return pm_schedule_suspend(dev, 1);
> +}

You don't need this runtime PM idle callback, just set it to NULL.

> +
> +static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       int ret;
> +
> +       ret = pm_runtime_get_sync(chip->dev);
> +
> +       /*
> +        * 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 zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       pm_runtime_put_sync(chip->dev);

You don't need the "sync" version here, using pm_runtime_put() should be enough.

> +}
> +
> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
> +                          zynq_gpio_idle)

Converting to use pm_runtime_force_suspend|resume(), means you need
use the SET_PM_RUNTIME_PM_OPS macro.

> +};
> +
> +/**
> + * zynq_gpio_probe - Initialization method for a zynq_gpio device
> + * @pdev:      platform device instance
> + *
> + * This function allocates memory resources for the gpio device and registers
> + * all the banks of the device. It will also set up interrupts for the gpio
> + * pins.
> + * Note: Interrupts are disabled for all the banks during initialization.
> + *
> + * Return: 0 on success, negative error otherwise.
> + */
> +static int zynq_gpio_probe(struct platform_device *pdev)
> +{
> +       int ret, pin_num, bank_num, gpio_irq;
> +       unsigned int irq_num;
> +       struct zynq_gpio *gpio;
> +       struct gpio_chip *chip;
> +       struct resource *res;
> +
> +       gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, gpio);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       gpio->base_addr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(gpio->base_addr))
> +               return PTR_ERR(gpio->base_addr);
> +
> +       irq_num = platform_get_irq(pdev, 0);
> +       gpio->irq = irq_num;
> +
> +       /* configure the gpio chip */
> +       chip = &gpio->chip;
> +       chip->label = "zynq_gpio";
> +       chip->owner = THIS_MODULE;
> +       chip->dev = &pdev->dev;
> +       chip->get = zynq_gpio_get_value;
> +       chip->set = zynq_gpio_set_value;
> +       chip->request = zynq_gpio_request;
> +       chip->free = zynq_gpio_free;
> +       chip->direction_input = zynq_gpio_dir_in;
> +       chip->direction_output = zynq_gpio_dir_out;
> +       chip->to_irq = zynq_gpio_to_irq;
> +       chip->dbg_show = NULL;
> +       chip->base = 0;         /* default pin base */
> +       chip->ngpio = ZYNQ_GPIO_NR_GPIOS;
> +       chip->can_sleep = 0;
> +
> +       gpio->irq_base = irq_alloc_descs(-1, 0, chip->ngpio, 0);
> +       if (gpio->irq_base < 0) {
> +               dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> +               return -ENODEV;
> +       }
> +
> +       irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
> +                                          chip->ngpio, gpio->irq_base, 0,
> +                                          &irq_domain_simple_ops, NULL);
> +
> +       /* report a bug if gpio chip registration fails */
> +       ret = gpiochip_add(chip);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Enable GPIO clock */
> +       gpio->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(gpio->clk)) {
> +               dev_err(&pdev->dev, "input clock not found.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return PTR_ERR(gpio->clk);
> +       }
> +       ret = clk_prepare_enable(gpio->clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Unable to enable clock.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return ret;
> +       }
> +
> +       /* disable interrupts for all banks */
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               zynq_gpio_writereg(gpio->base_addr +
> +                                  ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
> +                                  ZYNQ_GPIO_IXR_DISABLE_ALL);
> +       }
> +
> +       /*
> +        * set the irq chip, handler and irq chip data for callbacks for
> +        * each pin
> +        */
> +       for (pin_num = 0; pin_num < min_t(int, ZYNQ_GPIO_NR_GPIOS,
> +                                         (int)chip->ngpio); pin_num++) {
> +               gpio_irq = irq_find_mapping(irq_domain, pin_num);
> +               irq_set_chip_and_handler(gpio_irq, &zynq_gpio_irqchip,
> +                                        handle_simple_irq);
> +               irq_set_chip_data(gpio_irq, (void *)gpio);
> +               set_irq_flags(gpio_irq, IRQF_VALID);
> +       }
> +
> +       irq_set_handler_data(irq_num, (void *)gpio);
> +       irq_set_chained_handler(irq_num, zynq_gpio_irqhandler);
> +

add pm_runtime_set_active() to reflect the current state of the device.

> +       pm_runtime_enable(&pdev->dev);
> +
> +       device_set_wakeup_capable(&pdev->dev, 1);
> +
> +       return 0;
> +}
> +
> +/**
> + * zynq_gpio_remove - Driver removal function
> + * @pdev:      platform device instance
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_remove(struct platform_device *pdev)
> +{
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +

Converting to use clk_prepare|unprepare from the runtime PM callbacks,
means you can simplify the code here by using
pm_runtime_force_suspend().

If you don't like that, at least you need a pm_runtime_get_sync().

Kind regards
Ulf Hansson


> +       clk_disable_unprepare(gpio->clk);
> +       device_set_wakeup_capable(&pdev->dev, 0);
> +       return 0;
> +}
> +
> +static struct of_device_id zynq_gpio_of_match[] = {
> +       { .compatible = "xlnx,zynq-gpio-1.0", },
> +       { /* end of table */ }
> +};
> +MODULE_DEVICE_TABLE(of, zynq_gpio_of_match);
> +
> +static struct platform_driver zynq_gpio_driver = {
> +       .driver = {
> +               .name   = DRIVER_NAME,
> +               .owner  = THIS_MODULE,
> +               .pm     = &zynq_gpio_dev_pm_ops,
> +               .of_match_table = zynq_gpio_of_match,
> +       },
> +       .probe          = zynq_gpio_probe,
> +       .remove         = zynq_gpio_remove,
> +};
> +
> +/**
> + * zynq_gpio_init - Initial driver registration call
> + *
> + * Return: value from platform_driver_register
> + */
> +static int __init zynq_gpio_init(void)
> +{
> +       return platform_driver_register(&zynq_gpio_driver);
> +}
> +
> +postcore_initcall(zynq_gpio_init);
> +
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("Zynq GPIO driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Ulf Hansson]

On 27 March 2014 16:25, Harini Katakam <harinik@xilinx.com> wrote:
> Add support for GPIO controller used by Xilinx Zynq
>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
>  drivers/gpio/Kconfig     |    7 +
>  drivers/gpio/Makefile    |    1 +
>  drivers/gpio/gpio-zynq.c |  690 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 698 insertions(+)
>  create mode 100644 drivers/gpio/gpio-zynq.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 903f24d..67a22a6 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -313,6 +313,13 @@ config GPIO_XTENSA
>           Say yes here to support the Xtensa internal GPIO32 IMPWIRE (input)
>           and EXPSTATE (output) ports
>
> +config GPIO_ZYNQ
> +       bool "Xilinx ZYNQ GPIO support"
> +       depends on ARCH_ZYNQ
> +       select GENERIC_IRQ_CHIP
> +       help
> +        Say yes here to support Xilinx ZYNQ GPIO controller.
> +
>  config GPIO_VR41XX
>         tristate "NEC VR4100 series General-purpose I/O Uint support"
>         depends on CPU_VR41XX
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 5d50179..439f23a 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -99,3 +99,4 @@ obj-$(CONFIG_GPIO_WM8350)     += gpio-wm8350.o
>  obj-$(CONFIG_GPIO_WM8994)      += gpio-wm8994.o
>  obj-$(CONFIG_GPIO_XILINX)      += gpio-xilinx.o
>  obj-$(CONFIG_GPIO_XTENSA)      += gpio-xtensa.o
> +obj-$(CONFIG_GPIO_ZYNQ)                += gpio-zynq.o
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> new file mode 100644
> index 0000000..1f5fdfc
> --- /dev/null
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -0,0 +1,690 @@
> +/*
> + * Xilinx Zynq GPIO device driver
> + *
> + * Copyright (C) 2009 - 2014 Xilinx, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License as published by the Free Software
> + * Foundation; either version 2 of the License, or (at your option) any later
> + * version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define DRIVER_NAME "zynq-gpio"
> +#define ZYNQ_GPIO_NR_GPIOS     118
> +
> +static struct irq_domain *irq_domain;
> +
> +/* Register offsets for the GPIO device */
> +
> +/* LSW Mask & Data -WO */
> +#define ZYNQ_GPIO_DATA_LSW_OFFSET(BANK)        (0x000 + (8 * BANK))
> +/* MSW Mask & Data -WO */
> +#define ZYNQ_GPIO_DATA_MSW_OFFSET(BANK)        (0x004 + (8 * BANK))
> +/* Data Register-RW */
> +#define ZYNQ_GPIO_DATA_OFFSET(BANK)    (0x040 + (4 * BANK))
> +/* Direction mode reg-RW */
> +#define ZYNQ_GPIO_DIRM_OFFSET(BANK)    (0x204 + (0x40 * BANK))
> +/* Output enable reg-RW */
> +#define ZYNQ_GPIO_OUTEN_OFFSET(BANK)   (0x208 + (0x40 * BANK))
> +/* Interrupt mask reg-RO */
> +#define ZYNQ_GPIO_INTMASK_OFFSET(BANK) (0x20C + (0x40 * BANK))
> +/* Interrupt enable reg-WO */
> +#define ZYNQ_GPIO_INTEN_OFFSET(BANK)   (0x210 + (0x40 * BANK))
> +/* Interrupt disable reg-WO */
> +#define ZYNQ_GPIO_INTDIS_OFFSET(BANK)  (0x214 + (0x40 * BANK))
> +/* Interrupt status reg-RO */
> +#define ZYNQ_GPIO_INTSTS_OFFSET(BANK)  (0x218 + (0x40 * BANK))
> +/* Interrupt type reg-RW */
> +#define ZYNQ_GPIO_INTTYPE_OFFSET(BANK) (0x21C + (0x40 * BANK))
> +/* Interrupt polarity reg-RW */
> +#define ZYNQ_GPIO_INTPOL_OFFSET(BANK)  (0x220 + (0x40 * BANK))
> +/* Interrupt on any, reg-RW */
> +#define ZYNQ_GPIO_INTANY_OFFSET(BANK)  (0x224 + (0x40 * BANK))
> +
> +/* Read/Write access to the GPIO PS registers */
> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
> +{
> +       return readl_relaxed(offset);
> +}
> +
> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
> +{
> +       writel_relaxed(val, offset);
> +}
> +
> +static unsigned int zynq_gpio_pin_table[] = {
> +       31, /* 0 - 31 */
> +       53, /* 32 - 53 */
> +       85, /* 54 - 85 */
> +       117 /* 86 - 117 */
> +};
> +
> +/* Maximum banks */
> +#define ZYNQ_GPIO_MAX_BANK     4
> +
> +/* Disable all interrupts mask */
> +#define ZYNQ_GPIO_IXR_DISABLE_ALL      0xFFFFFFFF
> +
> +/* GPIO pin high */
> +#define ZYNQ_GPIO_PIN_HIGH 1
> +
> +/* Mid pin number of a bank */
> +#define ZYNQ_GPIO_MID_PIN_NUM 16
> +
> +/* GPIO upper 16 bit mask */
> +#define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000
> +
> +/**
> + * struct zynq_gpio - gpio device private data structure
> + * @chip:      instance of the gpio_chip
> + * @base_addr: base address of the GPIO device
> + * @irq:       irq associated with the controller
> + * @irq_base:  base of IRQ number for interrupt
> + * @clk:       clock resource for this controller
> + */
> +struct zynq_gpio {
> +       struct gpio_chip chip;
> +       void __iomem *base_addr;
> +       unsigned int irq;
> +       unsigned int irq_base;
> +       struct clk *clk;
> +};
> +
> +/**
> + * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
> + * for a given pin in the GPIO device
> + * @pin_num:   gpio pin number within the device
> + * @bank_num:  an output parameter used to return the bank number of the gpio
> + *             pin
> + * @bank_pin_num: an output parameter used to return pin number within a bank
> + *               for the given gpio pin
> + *
> + * Returns the bank number.
> + */
> +static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
> +                                         unsigned int *bank_num,
> +                                         unsigned int *bank_pin_num)
> +{
> +       for (*bank_num = 0; *bank_num < ZYNQ_GPIO_MAX_BANK; (*bank_num)++)
> +               if (pin_num <= zynq_gpio_pin_table[*bank_num])
> +                       break;
> +
> +       if (!(*bank_num))
> +               *bank_pin_num = pin_num;
> +       else
> +               *bank_pin_num = pin_num %
> +                               (zynq_gpio_pin_table[*bank_num - 1] + 1);
> +}
> +
> +/**
> + * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function reads the state of the specified pin of the GPIO device.
> + *
> + * Return: 0 if the pin is low, 1 if pin is high.
> + */
> +static int zynq_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int bank_num, bank_pin_num, data;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       data = zynq_gpio_readreg(gpio->base_addr +
> +                                ZYNQ_GPIO_DATA_OFFSET(bank_num));
> +       return (data >> bank_pin_num) & ZYNQ_GPIO_PIN_HIGH;
> +}
> +
> +/**
> + * zynq_gpio_set_value - Modify the state of the pin with specified value
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + * @state:     value used to modify the state of the specified pin
> + *
> + * This function calculates the register offset (i.e to lower 16 bits or
> + * upper 16 bits) based on the given pin number and sets the state of a
> + * gpio pin to the specified value. The state is either 0 or non-zero.
> + */
> +static void zynq_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
> +                               int state)
> +{
> +       unsigned int reg_offset, bank_num, bank_pin_num;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       if (bank_pin_num >= ZYNQ_GPIO_MID_PIN_NUM) {
> +               /* only 16 data bits in bit maskable reg */
> +               bank_pin_num -= ZYNQ_GPIO_MID_PIN_NUM;
> +               reg_offset = ZYNQ_GPIO_DATA_MSW_OFFSET(bank_num);
> +       } else {
> +               reg_offset = ZYNQ_GPIO_DATA_LSW_OFFSET(bank_num);
> +       }
> +
> +       /*
> +        * get the 32 bit value to be written to the mask/data register where
> +        * the upper 16 bits is the mask and lower 16 bits is the data
> +        */
> +       if (state)
> +               state = 1;
> +       state = ~(1 << (bank_pin_num + ZYNQ_GPIO_MID_PIN_NUM)) &
> +               ((state << bank_pin_num) | ZYNQ_GPIO_UPPER_MASK);
> +
> +       zynq_gpio_writereg(gpio->base_addr + reg_offset, state);
> +}
> +
> +/**
> + * zynq_gpio_dir_in - Set the direction of the specified GPIO pin as input
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function uses the read-modify-write sequence to set the direction of
> + * the gpio pin as input.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int reg, bank_num, bank_pin_num;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +       /* clear the bit in direction mode reg to set the pin as input */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg &= ~(1 << bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num),
> +                          reg);
> +
> +       return 0;
> +}
> +
> +/**
> + * zynq_gpio_dir_out - Set the direction of the specified GPIO pin as output
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + * @state:     value to be written to specified pin
> + *
> + * This function sets the direction of specified GPIO pin as output, configures
> + * the Output Enable register for the pin and uses zynq_gpio_set to set
> + * the state of the pin to the value specified.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
> +                            int state)
> +{
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +       unsigned int reg, bank_num, bank_pin_num;
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       /* set the GPIO pin as output */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg |= 1 << bank_pin_num;
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num),
> +                          reg);
> +
> +       /* configure the output enable reg for the pin */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_OUTEN_OFFSET(bank_num));
> +       reg |= 1 << bank_pin_num;
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_OUTEN_OFFSET(bank_num),
> +                          reg);
> +
> +       /* set the state of the pin */
> +       zynq_gpio_set_value(chip, pin, state);
> +       return 0;
> +}
> +
> +static int zynq_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       return irq_find_mapping(irq_domain, offset);
> +}
> +
> +/**
> + * zynq_gpio_irq_ack - Acknowledge the interrupt of a gpio pin
> + * @irq_data:  irq data containing irq number of gpio pin for the interrupt
> + *             to ack
> + *
> + * This function calculates gpio pin number from irq number and sets the bit
> + * in the Interrupt Status Register of the corresponding bank, to ACK the irq.
> + */
> +static void zynq_gpio_irq_ack(struct irq_data *irq_data)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)
> +                                irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTSTS_OFFSET(bank_num),
> +                          1 << bank_pin_num);
> +}
> +
> +/**
> + * zynq_gpio_irq_mask - Disable the interrupts for a gpio pin
> + * @irq_data:  per irq and chip data passed down to chip functions
> + *
> + * This function calculates gpio pin number from irq number and sets the
> + * bit in the Interrupt Disable register of the corresponding bank to disable
> + * interrupts for that pin.
> + */
> +static void zynq_gpio_irq_mask(struct irq_data *irq_data)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)
> +                                irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
> +                          1 << bank_pin_num);
> +}
> +
> +/**
> + * zynq_gpio_irq_unmask - Enable the interrupts for a gpio pin
> + * @irq_data:  irq data containing irq number of gpio pin for the interrupt
> + *             to enable
> + *
> + * This function calculates the gpio pin number from irq number and sets the
> + * bit in the Interrupt Enable register of the corresponding bank to enable
> + * interrupts for that pin.
> + */
> +static void zynq_gpio_irq_unmask(struct irq_data *irq_data)
> +{
> +       struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTEN_OFFSET(bank_num),
> +                          1 << bank_pin_num);
> +}
> +
> +/**
> + * zynq_gpio_set_irq_type - Set the irq type for a gpio pin
> + * @irq_data:  irq data containing irq number of gpio pin
> + * @type:      interrupt type that is to be set for the gpio pin
> + *
> + * This function gets the gpio pin number and its bank from the gpio pin number
> + * and configures the INT_TYPE, INT_POLARITY and INT_ANY registers.
> + *
> + * Return: 0, negative error otherwise.
> + * TYPE-EDGE_RISING,  INT_TYPE - 1, INT_POLARITY - 1,  INT_ANY - 0;
> + * TYPE-EDGE_FALLING, INT_TYPE - 1, INT_POLARITY - 0,  INT_ANY - 0;
> + * TYPE-EDGE_BOTH,    INT_TYPE - 1, INT_POLARITY - NA, INT_ANY - 1;
> + * TYPE-LEVEL_HIGH,   INT_TYPE - 0, INT_POLARITY - 1,  INT_ANY - NA;
> + * TYPE-LEVEL_LOW,    INT_TYPE - 0, INT_POLARITY - 0,  INT_ANY - NA
> + */
> +static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
> +{
> +       struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +       unsigned int int_type, int_pol, int_any;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +
> +       int_type = zynq_gpio_readreg(gpio->base_addr +
> +                                    ZYNQ_GPIO_INTTYPE_OFFSET(bank_num));
> +       int_pol = zynq_gpio_readreg(gpio->base_addr +
> +                                   ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
> +       int_any = zynq_gpio_readreg(gpio->base_addr +
> +                                   ZYNQ_GPIO_INTANY_OFFSET(bank_num));
> +
> +       /*
> +        * based on the type requested, configure the INT_TYPE, INT_POLARITY
> +        * and INT_ANY registers
> +        */
> +       switch (type) {
> +       case IRQ_TYPE_EDGE_RISING:
> +               int_type |= (1 << bank_pin_num);
> +               int_pol |= (1 << bank_pin_num);
> +               int_any &= ~(1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               int_type |= (1 << bank_pin_num);
> +               int_pol &= ~(1 << bank_pin_num);
> +               int_any &= ~(1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_EDGE_BOTH:
> +               int_type |= (1 << bank_pin_num);
> +               int_any |= (1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               int_type &= ~(1 << bank_pin_num);
> +               int_pol |= (1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_LEVEL_LOW:
> +               int_type &= ~(1 << bank_pin_num);
> +               int_pol &= ~(1 << bank_pin_num);
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       zynq_gpio_writereg(gpio->base_addr +
> +                          ZYNQ_GPIO_INTTYPE_OFFSET(bank_num), int_type);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTPOL_OFFSET(bank_num),
> +                          int_pol);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTANY_OFFSET(bank_num),
> +                          int_any);
> +       return 0;
> +}
> +
> +static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
> +{
> +       if (on)
> +               zynq_gpio_irq_unmask(data);
> +       else
> +               zynq_gpio_irq_mask(data);
> +
> +       return 0;
> +}
> +
> +/* irq chip descriptor */
> +static struct irq_chip zynq_gpio_irqchip = {
> +       .name           = DRIVER_NAME,
> +       .irq_ack        = zynq_gpio_irq_ack,
> +       .irq_mask       = zynq_gpio_irq_mask,
> +       .irq_unmask     = zynq_gpio_irq_unmask,
> +       .irq_set_type   = zynq_gpio_set_irq_type,
> +       .irq_set_wake   = zynq_gpio_set_wake,
> +};
> +
> +/**
> + * zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
> + * @irq:       irq number of the gpio bank where interrupt has occurred
> + * @desc:      irq descriptor instance of the 'irq'
> + *
> + * This function reads the Interrupt Status Register of each bank to get the
> + * gpio pin number which has triggered an interrupt. It then acks the triggered
> + * interrupt and calls the pin specific handler set by the higher layer
> + * application for that pin.
> + * Note: A bug is reported if no handler is set for the gpio pin.
> + */
> +static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)irq_get_handler_data(irq);
> +       int gpio_irq = gpio->irq_base;
> +       unsigned int int_sts, int_enb, bank_num;
> +       struct irq_desc *gpio_irq_desc;
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> +       chained_irq_enter(chip, desc);
> +
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               int_sts = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
> +               int_enb = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
> +               int_sts &= ~int_enb;
> +
> +               for (; int_sts != 0; int_sts >>= 1, gpio_irq++) {
> +                       if (!(int_sts & 1))
> +                               continue;
> +                       gpio_irq_desc = irq_to_desc(gpio_irq);
> +                       BUG_ON(!gpio_irq_desc);
> +                       chip = irq_desc_get_chip(gpio_irq_desc);
> +                       BUG_ON(!chip);
> +                       chip->irq_ack(&gpio_irq_desc->irq_data);
> +
> +                       /* call the pin specific handler */
> +                       generic_handle_irq(gpio_irq);
> +               }
> +               /* shift to first virtual irq of next bank */
> +               gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;
> +       }
> +
> +       chip = irq_desc_get_chip(desc);
> +       chained_irq_exit(chip, desc);
> +}
> +
> +static int __maybe_unused zynq_gpio_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       if (!device_may_wakeup(dev)) {
> +               if (!pm_runtime_suspended(dev))

Would it not be more convenient to use pm_runtime_force_suspend() here?

> +                       clk_disable(gpio->clk);
> +               return 0;
> +       }
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused zynq_gpio_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       if (!device_may_wakeup(dev)) {
> +               if (!pm_runtime_suspended(dev))

Would it not be more convenient to use pm_runtime_force_resume() here?

> +                       return clk_enable(gpio->clk);
> +       }
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       clk_disable(gpio->clk);

You should be able can use clk_disable_unprepare() here.

> +
> +       return 0;
> +}
> +
> +static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       return clk_enable(gpio->clk);

You should be able can use clk_prepare_enable() here.

> +}
> +
> +static int __maybe_unused zynq_gpio_idle(struct device *dev)
> +{
> +       return pm_schedule_suspend(dev, 1);
> +}

You don't need this runtime PM idle callback, just set it to NULL.

> +
> +static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       int ret;
> +
> +       ret = pm_runtime_get_sync(chip->dev);
> +
> +       /*
> +        * 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 zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       pm_runtime_put_sync(chip->dev);

You don't need the "sync" version here, using pm_runtime_put() should be enough.

> +}
> +
> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
> +                          zynq_gpio_idle)

Converting to use pm_runtime_force_suspend|resume(), means you need
use the SET_PM_RUNTIME_PM_OPS macro.

> +};
> +
> +/**
> + * zynq_gpio_probe - Initialization method for a zynq_gpio device
> + * @pdev:      platform device instance
> + *
> + * This function allocates memory resources for the gpio device and registers
> + * all the banks of the device. It will also set up interrupts for the gpio
> + * pins.
> + * Note: Interrupts are disabled for all the banks during initialization.
> + *
> + * Return: 0 on success, negative error otherwise.
> + */
> +static int zynq_gpio_probe(struct platform_device *pdev)
> +{
> +       int ret, pin_num, bank_num, gpio_irq;
> +       unsigned int irq_num;
> +       struct zynq_gpio *gpio;
> +       struct gpio_chip *chip;
> +       struct resource *res;
> +
> +       gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, gpio);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       gpio->base_addr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(gpio->base_addr))
> +               return PTR_ERR(gpio->base_addr);
> +
> +       irq_num = platform_get_irq(pdev, 0);
> +       gpio->irq = irq_num;
> +
> +       /* configure the gpio chip */
> +       chip = &gpio->chip;
> +       chip->label = "zynq_gpio";
> +       chip->owner = THIS_MODULE;
> +       chip->dev = &pdev->dev;
> +       chip->get = zynq_gpio_get_value;
> +       chip->set = zynq_gpio_set_value;
> +       chip->request = zynq_gpio_request;
> +       chip->free = zynq_gpio_free;
> +       chip->direction_input = zynq_gpio_dir_in;
> +       chip->direction_output = zynq_gpio_dir_out;
> +       chip->to_irq = zynq_gpio_to_irq;
> +       chip->dbg_show = NULL;
> +       chip->base = 0;         /* default pin base */
> +       chip->ngpio = ZYNQ_GPIO_NR_GPIOS;
> +       chip->can_sleep = 0;
> +
> +       gpio->irq_base = irq_alloc_descs(-1, 0, chip->ngpio, 0);
> +       if (gpio->irq_base < 0) {
> +               dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> +               return -ENODEV;
> +       }
> +
> +       irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
> +                                          chip->ngpio, gpio->irq_base, 0,
> +                                          &irq_domain_simple_ops, NULL);
> +
> +       /* report a bug if gpio chip registration fails */
> +       ret = gpiochip_add(chip);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Enable GPIO clock */
> +       gpio->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(gpio->clk)) {
> +               dev_err(&pdev->dev, "input clock not found.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return PTR_ERR(gpio->clk);
> +       }
> +       ret = clk_prepare_enable(gpio->clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Unable to enable clock.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return ret;
> +       }
> +
> +       /* disable interrupts for all banks */
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               zynq_gpio_writereg(gpio->base_addr +
> +                                  ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
> +                                  ZYNQ_GPIO_IXR_DISABLE_ALL);
> +       }
> +
> +       /*
> +        * set the irq chip, handler and irq chip data for callbacks for
> +        * each pin
> +        */
> +       for (pin_num = 0; pin_num < min_t(int, ZYNQ_GPIO_NR_GPIOS,
> +                                         (int)chip->ngpio); pin_num++) {
> +               gpio_irq = irq_find_mapping(irq_domain, pin_num);
> +               irq_set_chip_and_handler(gpio_irq, &zynq_gpio_irqchip,
> +                                        handle_simple_irq);
> +               irq_set_chip_data(gpio_irq, (void *)gpio);
> +               set_irq_flags(gpio_irq, IRQF_VALID);
> +       }
> +
> +       irq_set_handler_data(irq_num, (void *)gpio);
> +       irq_set_chained_handler(irq_num, zynq_gpio_irqhandler);
> +

add pm_runtime_set_active() to reflect the current state of the device.

> +       pm_runtime_enable(&pdev->dev);
> +
> +       device_set_wakeup_capable(&pdev->dev, 1);
> +
> +       return 0;
> +}
> +
> +/**
> + * zynq_gpio_remove - Driver removal function
> + * @pdev:      platform device instance
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_remove(struct platform_device *pdev)
> +{
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +

Converting to use clk_prepare|unprepare from the runtime PM callbacks,
means you can simplify the code here by using
pm_runtime_force_suspend().

If you don't like that, at least you need a pm_runtime_get_sync().

Kind regards
Ulf Hansson


> +       clk_disable_unprepare(gpio->clk);
> +       device_set_wakeup_capable(&pdev->dev, 0);
> +       return 0;
> +}
> +
> +static struct of_device_id zynq_gpio_of_match[] = {
> +       { .compatible = "xlnx,zynq-gpio-1.0", },
> +       { /* end of table */ }
> +};
> +MODULE_DEVICE_TABLE(of, zynq_gpio_of_match);
> +
> +static struct platform_driver zynq_gpio_driver = {
> +       .driver = {
> +               .name   = DRIVER_NAME,
> +               .owner  = THIS_MODULE,
> +               .pm     = &zynq_gpio_dev_pm_ops,
> +               .of_match_table = zynq_gpio_of_match,
> +       },
> +       .probe          = zynq_gpio_probe,
> +       .remove         = zynq_gpio_remove,
> +};
> +
> +/**
> + * zynq_gpio_init - Initial driver registration call
> + *
> + * Return: value from platform_driver_register
> + */
> +static int __init zynq_gpio_init(void)
> +{
> +       return platform_driver_register(&zynq_gpio_driver);
> +}
> +
> +postcore_initcall(zynq_gpio_init);
> +
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("Zynq GPIO driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Ulf Hansson]

On 27 March 2014 16:25, Harini Katakam <harinik@xilinx.com> wrote:
> Add support for GPIO controller used by Xilinx Zynq
>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
>  drivers/gpio/Kconfig     |    7 +
>  drivers/gpio/Makefile    |    1 +
>  drivers/gpio/gpio-zynq.c |  690 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 698 insertions(+)
>  create mode 100644 drivers/gpio/gpio-zynq.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 903f24d..67a22a6 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -313,6 +313,13 @@ config GPIO_XTENSA
>           Say yes here to support the Xtensa internal GPIO32 IMPWIRE (input)
>           and EXPSTATE (output) ports
>
> +config GPIO_ZYNQ
> +       bool "Xilinx ZYNQ GPIO support"
> +       depends on ARCH_ZYNQ
> +       select GENERIC_IRQ_CHIP
> +       help
> +        Say yes here to support Xilinx ZYNQ GPIO controller.
> +
>  config GPIO_VR41XX
>         tristate "NEC VR4100 series General-purpose I/O Uint support"
>         depends on CPU_VR41XX
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 5d50179..439f23a 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -99,3 +99,4 @@ obj-$(CONFIG_GPIO_WM8350)     += gpio-wm8350.o
>  obj-$(CONFIG_GPIO_WM8994)      += gpio-wm8994.o
>  obj-$(CONFIG_GPIO_XILINX)      += gpio-xilinx.o
>  obj-$(CONFIG_GPIO_XTENSA)      += gpio-xtensa.o
> +obj-$(CONFIG_GPIO_ZYNQ)                += gpio-zynq.o
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> new file mode 100644
> index 0000000..1f5fdfc
> --- /dev/null
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -0,0 +1,690 @@
> +/*
> + * Xilinx Zynq GPIO device driver
> + *
> + * Copyright (C) 2009 - 2014 Xilinx, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License as published by the Free Software
> + * Foundation; either version 2 of the License, or (at your option) any later
> + * version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define DRIVER_NAME "zynq-gpio"
> +#define ZYNQ_GPIO_NR_GPIOS     118
> +
> +static struct irq_domain *irq_domain;
> +
> +/* Register offsets for the GPIO device */
> +
> +/* LSW Mask & Data -WO */
> +#define ZYNQ_GPIO_DATA_LSW_OFFSET(BANK)        (0x000 + (8 * BANK))
> +/* MSW Mask & Data -WO */
> +#define ZYNQ_GPIO_DATA_MSW_OFFSET(BANK)        (0x004 + (8 * BANK))
> +/* Data Register-RW */
> +#define ZYNQ_GPIO_DATA_OFFSET(BANK)    (0x040 + (4 * BANK))
> +/* Direction mode reg-RW */
> +#define ZYNQ_GPIO_DIRM_OFFSET(BANK)    (0x204 + (0x40 * BANK))
> +/* Output enable reg-RW */
> +#define ZYNQ_GPIO_OUTEN_OFFSET(BANK)   (0x208 + (0x40 * BANK))
> +/* Interrupt mask reg-RO */
> +#define ZYNQ_GPIO_INTMASK_OFFSET(BANK) (0x20C + (0x40 * BANK))
> +/* Interrupt enable reg-WO */
> +#define ZYNQ_GPIO_INTEN_OFFSET(BANK)   (0x210 + (0x40 * BANK))
> +/* Interrupt disable reg-WO */
> +#define ZYNQ_GPIO_INTDIS_OFFSET(BANK)  (0x214 + (0x40 * BANK))
> +/* Interrupt status reg-RO */
> +#define ZYNQ_GPIO_INTSTS_OFFSET(BANK)  (0x218 + (0x40 * BANK))
> +/* Interrupt type reg-RW */
> +#define ZYNQ_GPIO_INTTYPE_OFFSET(BANK) (0x21C + (0x40 * BANK))
> +/* Interrupt polarity reg-RW */
> +#define ZYNQ_GPIO_INTPOL_OFFSET(BANK)  (0x220 + (0x40 * BANK))
> +/* Interrupt on any, reg-RW */
> +#define ZYNQ_GPIO_INTANY_OFFSET(BANK)  (0x224 + (0x40 * BANK))
> +
> +/* Read/Write access to the GPIO PS registers */
> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
> +{
> +       return readl_relaxed(offset);
> +}
> +
> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
> +{
> +       writel_relaxed(val, offset);
> +}
> +
> +static unsigned int zynq_gpio_pin_table[] = {
> +       31, /* 0 - 31 */
> +       53, /* 32 - 53 */
> +       85, /* 54 - 85 */
> +       117 /* 86 - 117 */
> +};
> +
> +/* Maximum banks */
> +#define ZYNQ_GPIO_MAX_BANK     4
> +
> +/* Disable all interrupts mask */
> +#define ZYNQ_GPIO_IXR_DISABLE_ALL      0xFFFFFFFF
> +
> +/* GPIO pin high */
> +#define ZYNQ_GPIO_PIN_HIGH 1
> +
> +/* Mid pin number of a bank */
> +#define ZYNQ_GPIO_MID_PIN_NUM 16
> +
> +/* GPIO upper 16 bit mask */
> +#define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000
> +
> +/**
> + * struct zynq_gpio - gpio device private data structure
> + * @chip:      instance of the gpio_chip
> + * @base_addr: base address of the GPIO device
> + * @irq:       irq associated with the controller
> + * @irq_base:  base of IRQ number for interrupt
> + * @clk:       clock resource for this controller
> + */
> +struct zynq_gpio {
> +       struct gpio_chip chip;
> +       void __iomem *base_addr;
> +       unsigned int irq;
> +       unsigned int irq_base;
> +       struct clk *clk;
> +};
> +
> +/**
> + * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
> + * for a given pin in the GPIO device
> + * @pin_num:   gpio pin number within the device
> + * @bank_num:  an output parameter used to return the bank number of the gpio
> + *             pin
> + * @bank_pin_num: an output parameter used to return pin number within a bank
> + *               for the given gpio pin
> + *
> + * Returns the bank number.
> + */
> +static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
> +                                         unsigned int *bank_num,
> +                                         unsigned int *bank_pin_num)
> +{
> +       for (*bank_num = 0; *bank_num < ZYNQ_GPIO_MAX_BANK; (*bank_num)++)
> +               if (pin_num <= zynq_gpio_pin_table[*bank_num])
> +                       break;
> +
> +       if (!(*bank_num))
> +               *bank_pin_num = pin_num;
> +       else
> +               *bank_pin_num = pin_num %
> +                               (zynq_gpio_pin_table[*bank_num - 1] + 1);
> +}
> +
> +/**
> + * zynq_gpio_get_value - Get the state of the specified pin of GPIO device
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function reads the state of the specified pin of the GPIO device.
> + *
> + * Return: 0 if the pin is low, 1 if pin is high.
> + */
> +static int zynq_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int bank_num, bank_pin_num, data;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       data = zynq_gpio_readreg(gpio->base_addr +
> +                                ZYNQ_GPIO_DATA_OFFSET(bank_num));
> +       return (data >> bank_pin_num) & ZYNQ_GPIO_PIN_HIGH;
> +}
> +
> +/**
> + * zynq_gpio_set_value - Modify the state of the pin with specified value
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + * @state:     value used to modify the state of the specified pin
> + *
> + * This function calculates the register offset (i.e to lower 16 bits or
> + * upper 16 bits) based on the given pin number and sets the state of a
> + * gpio pin to the specified value. The state is either 0 or non-zero.
> + */
> +static void zynq_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
> +                               int state)
> +{
> +       unsigned int reg_offset, bank_num, bank_pin_num;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       if (bank_pin_num >= ZYNQ_GPIO_MID_PIN_NUM) {
> +               /* only 16 data bits in bit maskable reg */
> +               bank_pin_num -= ZYNQ_GPIO_MID_PIN_NUM;
> +               reg_offset = ZYNQ_GPIO_DATA_MSW_OFFSET(bank_num);
> +       } else {
> +               reg_offset = ZYNQ_GPIO_DATA_LSW_OFFSET(bank_num);
> +       }
> +
> +       /*
> +        * get the 32 bit value to be written to the mask/data register where
> +        * the upper 16 bits is the mask and lower 16 bits is the data
> +        */
> +       if (state)
> +               state = 1;
> +       state = ~(1 << (bank_pin_num + ZYNQ_GPIO_MID_PIN_NUM)) &
> +               ((state << bank_pin_num) | ZYNQ_GPIO_UPPER_MASK);
> +
> +       zynq_gpio_writereg(gpio->base_addr + reg_offset, state);
> +}
> +
> +/**
> + * zynq_gpio_dir_in - Set the direction of the specified GPIO pin as input
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * This function uses the read-modify-write sequence to set the direction of
> + * the gpio pin as input.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
> +{
> +       unsigned int reg, bank_num, bank_pin_num;
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +       /* clear the bit in direction mode reg to set the pin as input */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg &= ~(1 << bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num),
> +                          reg);
> +
> +       return 0;
> +}
> +
> +/**
> + * zynq_gpio_dir_out - Set the direction of the specified GPIO pin as output
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + * @state:     value to be written to specified pin
> + *
> + * This function sets the direction of specified GPIO pin as output, configures
> + * the Output Enable register for the pin and uses zynq_gpio_set to set
> + * the state of the pin to the value specified.
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
> +                            int state)
> +{
> +       struct zynq_gpio *gpio = container_of(chip, struct zynq_gpio, chip);
> +       unsigned int reg, bank_num, bank_pin_num;
> +
> +       zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num);
> +
> +       /* set the GPIO pin as output */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_DIRM_OFFSET(bank_num));
> +       reg |= 1 << bank_pin_num;
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_DIRM_OFFSET(bank_num),
> +                          reg);
> +
> +       /* configure the output enable reg for the pin */
> +       reg = zynq_gpio_readreg(gpio->base_addr +
> +                               ZYNQ_GPIO_OUTEN_OFFSET(bank_num));
> +       reg |= 1 << bank_pin_num;
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_OUTEN_OFFSET(bank_num),
> +                          reg);
> +
> +       /* set the state of the pin */
> +       zynq_gpio_set_value(chip, pin, state);
> +       return 0;
> +}
> +
> +static int zynq_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> +{
> +       return irq_find_mapping(irq_domain, offset);
> +}
> +
> +/**
> + * zynq_gpio_irq_ack - Acknowledge the interrupt of a gpio pin
> + * @irq_data:  irq data containing irq number of gpio pin for the interrupt
> + *             to ack
> + *
> + * This function calculates gpio pin number from irq number and sets the bit
> + * in the Interrupt Status Register of the corresponding bank, to ACK the irq.
> + */
> +static void zynq_gpio_irq_ack(struct irq_data *irq_data)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)
> +                                irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTSTS_OFFSET(bank_num),
> +                          1 << bank_pin_num);
> +}
> +
> +/**
> + * zynq_gpio_irq_mask - Disable the interrupts for a gpio pin
> + * @irq_data:  per irq and chip data passed down to chip functions
> + *
> + * This function calculates gpio pin number from irq number and sets the
> + * bit in the Interrupt Disable register of the corresponding bank to disable
> + * interrupts for that pin.
> + */
> +static void zynq_gpio_irq_mask(struct irq_data *irq_data)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)
> +                                irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
> +                          1 << bank_pin_num);
> +}
> +
> +/**
> + * zynq_gpio_irq_unmask - Enable the interrupts for a gpio pin
> + * @irq_data:  irq data containing irq number of gpio pin for the interrupt
> + *             to enable
> + *
> + * This function calculates the gpio pin number from irq number and sets the
> + * bit in the Interrupt Enable register of the corresponding bank to enable
> + * interrupts for that pin.
> + */
> +static void zynq_gpio_irq_unmask(struct irq_data *irq_data)
> +{
> +       struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTEN_OFFSET(bank_num),
> +                          1 << bank_pin_num);
> +}
> +
> +/**
> + * zynq_gpio_set_irq_type - Set the irq type for a gpio pin
> + * @irq_data:  irq data containing irq number of gpio pin
> + * @type:      interrupt type that is to be set for the gpio pin
> + *
> + * This function gets the gpio pin number and its bank from the gpio pin number
> + * and configures the INT_TYPE, INT_POLARITY and INT_ANY registers.
> + *
> + * Return: 0, negative error otherwise.
> + * TYPE-EDGE_RISING,  INT_TYPE - 1, INT_POLARITY - 1,  INT_ANY - 0;
> + * TYPE-EDGE_FALLING, INT_TYPE - 1, INT_POLARITY - 0,  INT_ANY - 0;
> + * TYPE-EDGE_BOTH,    INT_TYPE - 1, INT_POLARITY - NA, INT_ANY - 1;
> + * TYPE-LEVEL_HIGH,   INT_TYPE - 0, INT_POLARITY - 1,  INT_ANY - NA;
> + * TYPE-LEVEL_LOW,    INT_TYPE - 0, INT_POLARITY - 0,  INT_ANY - NA
> + */
> +static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
> +{
> +       struct zynq_gpio *gpio = irq_data_get_irq_chip_data(irq_data);
> +       unsigned int device_pin_num, bank_num, bank_pin_num;
> +       unsigned int int_type, int_pol, int_any;
> +
> +       device_pin_num = irq_data->hwirq;
> +       zynq_gpio_get_bank_pin(device_pin_num, &bank_num, &bank_pin_num);
> +
> +       int_type = zynq_gpio_readreg(gpio->base_addr +
> +                                    ZYNQ_GPIO_INTTYPE_OFFSET(bank_num));
> +       int_pol = zynq_gpio_readreg(gpio->base_addr +
> +                                   ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
> +       int_any = zynq_gpio_readreg(gpio->base_addr +
> +                                   ZYNQ_GPIO_INTANY_OFFSET(bank_num));
> +
> +       /*
> +        * based on the type requested, configure the INT_TYPE, INT_POLARITY
> +        * and INT_ANY registers
> +        */
> +       switch (type) {
> +       case IRQ_TYPE_EDGE_RISING:
> +               int_type |= (1 << bank_pin_num);
> +               int_pol |= (1 << bank_pin_num);
> +               int_any &= ~(1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               int_type |= (1 << bank_pin_num);
> +               int_pol &= ~(1 << bank_pin_num);
> +               int_any &= ~(1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_EDGE_BOTH:
> +               int_type |= (1 << bank_pin_num);
> +               int_any |= (1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               int_type &= ~(1 << bank_pin_num);
> +               int_pol |= (1 << bank_pin_num);
> +               break;
> +       case IRQ_TYPE_LEVEL_LOW:
> +               int_type &= ~(1 << bank_pin_num);
> +               int_pol &= ~(1 << bank_pin_num);
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       zynq_gpio_writereg(gpio->base_addr +
> +                          ZYNQ_GPIO_INTTYPE_OFFSET(bank_num), int_type);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTPOL_OFFSET(bank_num),
> +                          int_pol);
> +       zynq_gpio_writereg(gpio->base_addr + ZYNQ_GPIO_INTANY_OFFSET(bank_num),
> +                          int_any);
> +       return 0;
> +}
> +
> +static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
> +{
> +       if (on)
> +               zynq_gpio_irq_unmask(data);
> +       else
> +               zynq_gpio_irq_mask(data);
> +
> +       return 0;
> +}
> +
> +/* irq chip descriptor */
> +static struct irq_chip zynq_gpio_irqchip = {
> +       .name           = DRIVER_NAME,
> +       .irq_ack        = zynq_gpio_irq_ack,
> +       .irq_mask       = zynq_gpio_irq_mask,
> +       .irq_unmask     = zynq_gpio_irq_unmask,
> +       .irq_set_type   = zynq_gpio_set_irq_type,
> +       .irq_set_wake   = zynq_gpio_set_wake,
> +};
> +
> +/**
> + * zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
> + * @irq:       irq number of the gpio bank where interrupt has occurred
> + * @desc:      irq descriptor instance of the 'irq'
> + *
> + * This function reads the Interrupt Status Register of each bank to get the
> + * gpio pin number which has triggered an interrupt. It then acks the triggered
> + * interrupt and calls the pin specific handler set by the higher layer
> + * application for that pin.
> + * Note: A bug is reported if no handler is set for the gpio pin.
> + */
> +static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
> +{
> +       struct zynq_gpio *gpio = (struct zynq_gpio *)irq_get_handler_data(irq);
> +       int gpio_irq = gpio->irq_base;
> +       unsigned int int_sts, int_enb, bank_num;
> +       struct irq_desc *gpio_irq_desc;
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +
> +       chained_irq_enter(chip, desc);
> +
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               int_sts = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTSTS_OFFSET(bank_num));
> +               int_enb = zynq_gpio_readreg(gpio->base_addr +
> +                                           ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
> +               int_sts &= ~int_enb;
> +
> +               for (; int_sts != 0; int_sts >>= 1, gpio_irq++) {
> +                       if (!(int_sts & 1))
> +                               continue;
> +                       gpio_irq_desc = irq_to_desc(gpio_irq);
> +                       BUG_ON(!gpio_irq_desc);
> +                       chip = irq_desc_get_chip(gpio_irq_desc);
> +                       BUG_ON(!chip);
> +                       chip->irq_ack(&gpio_irq_desc->irq_data);
> +
> +                       /* call the pin specific handler */
> +                       generic_handle_irq(gpio_irq);
> +               }
> +               /* shift to first virtual irq of next bank */
> +               gpio_irq = gpio->irq_base + zynq_gpio_pin_table[bank_num] + 1;
> +       }
> +
> +       chip = irq_desc_get_chip(desc);
> +       chained_irq_exit(chip, desc);
> +}
> +
> +static int __maybe_unused zynq_gpio_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       if (!device_may_wakeup(dev)) {
> +               if (!pm_runtime_suspended(dev))

Would it not be more convenient to use pm_runtime_force_suspend() here?

> +                       clk_disable(gpio->clk);
> +               return 0;
> +       }
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused zynq_gpio_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       if (!device_may_wakeup(dev)) {
> +               if (!pm_runtime_suspended(dev))

Would it not be more convenient to use pm_runtime_force_resume() here?

> +                       return clk_enable(gpio->clk);
> +       }
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       clk_disable(gpio->clk);

You should be able can use clk_disable_unprepare() here.

> +
> +       return 0;
> +}
> +
> +static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +
> +       return clk_enable(gpio->clk);

You should be able can use clk_prepare_enable() here.

> +}
> +
> +static int __maybe_unused zynq_gpio_idle(struct device *dev)
> +{
> +       return pm_schedule_suspend(dev, 1);
> +}

You don't need this runtime PM idle callback, just set it to NULL.

> +
> +static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +       int ret;
> +
> +       ret = pm_runtime_get_sync(chip->dev);
> +
> +       /*
> +        * 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 zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       pm_runtime_put_sync(chip->dev);

You don't need the "sync" version here, using pm_runtime_put() should be enough.

> +}
> +
> +static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
> +       SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend, zynq_gpio_runtime_resume,
> +                          zynq_gpio_idle)

Converting to use pm_runtime_force_suspend|resume(), means you need
use the SET_PM_RUNTIME_PM_OPS macro.

> +};
> +
> +/**
> + * zynq_gpio_probe - Initialization method for a zynq_gpio device
> + * @pdev:      platform device instance
> + *
> + * This function allocates memory resources for the gpio device and registers
> + * all the banks of the device. It will also set up interrupts for the gpio
> + * pins.
> + * Note: Interrupts are disabled for all the banks during initialization.
> + *
> + * Return: 0 on success, negative error otherwise.
> + */
> +static int zynq_gpio_probe(struct platform_device *pdev)
> +{
> +       int ret, pin_num, bank_num, gpio_irq;
> +       unsigned int irq_num;
> +       struct zynq_gpio *gpio;
> +       struct gpio_chip *chip;
> +       struct resource *res;
> +
> +       gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, gpio);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       gpio->base_addr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(gpio->base_addr))
> +               return PTR_ERR(gpio->base_addr);
> +
> +       irq_num = platform_get_irq(pdev, 0);
> +       gpio->irq = irq_num;
> +
> +       /* configure the gpio chip */
> +       chip = &gpio->chip;
> +       chip->label = "zynq_gpio";
> +       chip->owner = THIS_MODULE;
> +       chip->dev = &pdev->dev;
> +       chip->get = zynq_gpio_get_value;
> +       chip->set = zynq_gpio_set_value;
> +       chip->request = zynq_gpio_request;
> +       chip->free = zynq_gpio_free;
> +       chip->direction_input = zynq_gpio_dir_in;
> +       chip->direction_output = zynq_gpio_dir_out;
> +       chip->to_irq = zynq_gpio_to_irq;
> +       chip->dbg_show = NULL;
> +       chip->base = 0;         /* default pin base */
> +       chip->ngpio = ZYNQ_GPIO_NR_GPIOS;
> +       chip->can_sleep = 0;
> +
> +       gpio->irq_base = irq_alloc_descs(-1, 0, chip->ngpio, 0);
> +       if (gpio->irq_base < 0) {
> +               dev_err(&pdev->dev, "Couldn't allocate IRQ numbers\n");
> +               return -ENODEV;
> +       }
> +
> +       irq_domain = irq_domain_add_legacy(pdev->dev.of_node,
> +                                          chip->ngpio, gpio->irq_base, 0,
> +                                          &irq_domain_simple_ops, NULL);
> +
> +       /* report a bug if gpio chip registration fails */
> +       ret = gpiochip_add(chip);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Enable GPIO clock */
> +       gpio->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(gpio->clk)) {
> +               dev_err(&pdev->dev, "input clock not found.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return PTR_ERR(gpio->clk);
> +       }
> +       ret = clk_prepare_enable(gpio->clk);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Unable to enable clock.\n");
> +               if (gpiochip_remove(chip))
> +                       dev_err(&pdev->dev, "Failed to remove gpio chip\n");
> +               return ret;
> +       }
> +
> +       /* disable interrupts for all banks */
> +       for (bank_num = 0; bank_num < ZYNQ_GPIO_MAX_BANK; bank_num++) {
> +               zynq_gpio_writereg(gpio->base_addr +
> +                                  ZYNQ_GPIO_INTDIS_OFFSET(bank_num),
> +                                  ZYNQ_GPIO_IXR_DISABLE_ALL);
> +       }
> +
> +       /*
> +        * set the irq chip, handler and irq chip data for callbacks for
> +        * each pin
> +        */
> +       for (pin_num = 0; pin_num < min_t(int, ZYNQ_GPIO_NR_GPIOS,
> +                                         (int)chip->ngpio); pin_num++) {
> +               gpio_irq = irq_find_mapping(irq_domain, pin_num);
> +               irq_set_chip_and_handler(gpio_irq, &zynq_gpio_irqchip,
> +                                        handle_simple_irq);
> +               irq_set_chip_data(gpio_irq, (void *)gpio);
> +               set_irq_flags(gpio_irq, IRQF_VALID);
> +       }
> +
> +       irq_set_handler_data(irq_num, (void *)gpio);
> +       irq_set_chained_handler(irq_num, zynq_gpio_irqhandler);
> +

add pm_runtime_set_active() to reflect the current state of the device.

> +       pm_runtime_enable(&pdev->dev);
> +
> +       device_set_wakeup_capable(&pdev->dev, 1);
> +
> +       return 0;
> +}
> +
> +/**
> + * zynq_gpio_remove - Driver removal function
> + * @pdev:      platform device instance
> + *
> + * Return: 0 always
> + */
> +static int zynq_gpio_remove(struct platform_device *pdev)
> +{
> +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> +

Converting to use clk_prepare|unprepare from the runtime PM callbacks,
means you can simplify the code here by using
pm_runtime_force_suspend().

If you don't like that, at least you need a pm_runtime_get_sync().

Kind regards
Ulf Hansson


> +       clk_disable_unprepare(gpio->clk);
> +       device_set_wakeup_capable(&pdev->dev, 0);
> +       return 0;
> +}
> +
> +static struct of_device_id zynq_gpio_of_match[] = {
> +       { .compatible = "xlnx,zynq-gpio-1.0", },
> +       { /* end of table */ }
> +};
> +MODULE_DEVICE_TABLE(of, zynq_gpio_of_match);
> +
> +static struct platform_driver zynq_gpio_driver = {
> +       .driver = {
> +               .name   = DRIVER_NAME,
> +               .owner  = THIS_MODULE,
> +               .pm     = &zynq_gpio_dev_pm_ops,
> +               .of_match_table = zynq_gpio_of_match,
> +       },
> +       .probe          = zynq_gpio_probe,
> +       .remove         = zynq_gpio_remove,
> +};
> +
> +/**
> + * zynq_gpio_init - Initial driver registration call
> + *
> + * Return: value from platform_driver_register
> + */
> +static int __init zynq_gpio_init(void)
> +{
> +       return platform_driver_register(&zynq_gpio_driver);
> +}
> +
> +postcore_initcall(zynq_gpio_init);
> +
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("Zynq GPIO driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Michal Simek]

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

Hi Linus,

On 03/31/2014 10:22 AM, Linus Walleij wrote:
> On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
> <harinikatakamlinux@gmail.com> wrote:
>> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
> 
>>>> +/* Read/Write access to the GPIO PS registers */
>>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>>> +{
>>>> +       return readl_relaxed(offset);
>>>> +}
>>>> +
>>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>>> +{
>>>> +       writel_relaxed(val, offset);
>>>> +}
>>>
>>> I think this is unnecessary and confusing indirection.
>>> Just use the readl_relaxed/writel_relaxed functions directly in
>>> the code.
>>>
>>
>> This is just to be flexible.
> 
> Define exactly what you mean with "flexible" in this context. I
> only see unnecessary overhead and hard-to-read code.

We have just passed this discussion for watchdog driver
here: https://lkml.org/lkml/2014/4/1/843

Are you ok with doing it in this way?

static inline u32 zynq_gpio_readreg(struct zynq_gpio *gpio, u32 offset)
{
       return readl_relaxed(gpio->base_addr + offset);
}

static inline void zynq_gpio_writereg(struct zynq_gpio *gpio, u32 offset, u32 val)
{
      writel_relaxed(val, gpio->base_addr + offset);
}

Or even like this to be able to handle error cases.

static inline int zynq_gpio_readreg(struct zynq_gpio *gpio, u32 offset, u32 *val)
{
	*val = readl_relaxed(gpio->base_addr + offset);
	return 0;
}

static inline int zynq_gpio_writereg(struct zynq_gpio *gpio, u32 offset, u32 val)
{
	writel_relaxed(val, gpio->base_addr + offset);
	return 0;
}

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Michal Simek]

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

Hi Linus,

On 03/31/2014 10:22 AM, Linus Walleij wrote:
> On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
> <harinikatakamlinux@gmail.com> wrote:
>> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
> 
>>>> +/* Read/Write access to the GPIO PS registers */
>>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>>> +{
>>>> +       return readl_relaxed(offset);
>>>> +}
>>>> +
>>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>>> +{
>>>> +       writel_relaxed(val, offset);
>>>> +}
>>>
>>> I think this is unnecessary and confusing indirection.
>>> Just use the readl_relaxed/writel_relaxed functions directly in
>>> the code.
>>>
>>
>> This is just to be flexible.
> 
> Define exactly what you mean with "flexible" in this context. I
> only see unnecessary overhead and hard-to-read code.

We have just passed this discussion for watchdog driver
here: https://lkml.org/lkml/2014/4/1/843

Are you ok with doing it in this way?

static inline u32 zynq_gpio_readreg(struct zynq_gpio *gpio, u32 offset)
{
       return readl_relaxed(gpio->base_addr + offset);
}

static inline void zynq_gpio_writereg(struct zynq_gpio *gpio, u32 offset, u32 val)
{
      writel_relaxed(val, gpio->base_addr + offset);
}

Or even like this to be able to handle error cases.

static inline int zynq_gpio_readreg(struct zynq_gpio *gpio, u32 offset, u32 *val)
{
	*val = readl_relaxed(gpio->base_addr + offset);
	return 0;
}

static inline int zynq_gpio_writereg(struct zynq_gpio *gpio, u32 offset, u32 val)
{
	writel_relaxed(val, gpio->base_addr + offset);
	return 0;
}

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Michal Simek]

Hi Linus,

On 03/31/2014 10:22 AM, Linus Walleij wrote:
> On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
> <harinikatakamlinux@gmail.com> wrote:
>> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
> 
>>>> +/* Read/Write access to the GPIO PS registers */
>>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>>> +{
>>>> +       return readl_relaxed(offset);
>>>> +}
>>>> +
>>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>>> +{
>>>> +       writel_relaxed(val, offset);
>>>> +}
>>>
>>> I think this is unnecessary and confusing indirection.
>>> Just use the readl_relaxed/writel_relaxed functions directly in
>>> the code.
>>>
>>
>> This is just to be flexible.
> 
> Define exactly what you mean with "flexible" in this context. I
> only see unnecessary overhead and hard-to-read code.

We have just passed this discussion for watchdog driver
here: https://lkml.org/lkml/2014/4/1/843

Are you ok with doing it in this way?

static inline u32 zynq_gpio_readreg(struct zynq_gpio *gpio, u32 offset)
{
       return readl_relaxed(gpio->base_addr + offset);
}

static inline void zynq_gpio_writereg(struct zynq_gpio *gpio, u32 offset, u32 val)
{
      writel_relaxed(val, gpio->base_addr + offset);
}

Or even like this to be able to handle error cases.

static inline int zynq_gpio_readreg(struct zynq_gpio *gpio, u32 offset, u32 *val)
{
	*val = readl_relaxed(gpio->base_addr + offset);
	return 0;
}

static inline int zynq_gpio_writereg(struct zynq_gpio *gpio, u32 offset, u32 val)
{
	writel_relaxed(val, gpio->base_addr + offset);
	return 0;
}

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140402/4b3a7af1/attachment.sig>

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Sören Brinkmann]

On Mon, 2014-03-31 at 11:23AM +0200, Ulf Hansson wrote:
> On 27 March 2014 16:25, Harini Katakam <harinik@xilinx.com> wrote:
[...]
> > +static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> > +
> > +       clk_disable(gpio->clk);
> 
> You should be able can use clk_disable_unprepare() here.
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> > +
> > +       return clk_enable(gpio->clk);
> 
> You should be able can use clk_prepare_enable() here.

Is there some common practice regarding this? As I understand it, we
want to ensure the clock to be gated during suspend, which should happen
with clk_disable(). Why would we also unprepare the clock? We are highly
likely to use it again once we resume.

	Thanks,
	Sören

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Sören Brinkmann]

On Mon, 2014-03-31 at 11:23AM +0200, Ulf Hansson wrote:
> On 27 March 2014 16:25, Harini Katakam <harinik@xilinx.com> wrote:
[...]
> > +static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> > +
> > +       clk_disable(gpio->clk);
> 
> You should be able can use clk_disable_unprepare() here.
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> > +
> > +       return clk_enable(gpio->clk);
> 
> You should be able can use clk_prepare_enable() here.

Is there some common practice regarding this? As I understand it, we
want to ensure the clock to be gated during suspend, which should happen
with clk_disable(). Why would we also unprepare the clock? We are highly
likely to use it again once we resume.

	Thanks,
	Sören

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Sören Brinkmann]

On Mon, 2014-03-31 at 11:23AM +0200, Ulf Hansson wrote:
> On 27 March 2014 16:25, Harini Katakam <harinik@xilinx.com> wrote:
[...]
> > +static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> > +
> > +       clk_disable(gpio->clk);
> 
> You should be able can use clk_disable_unprepare() here.
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> > +
> > +       return clk_enable(gpio->clk);
> 
> You should be able can use clk_prepare_enable() here.

Is there some common practice regarding this? As I understand it, we
want to ensure the clock to be gated during suspend, which should happen
with clk_disable(). Why would we also unprepare the clock? We are highly
likely to use it again once we resume.

	Thanks,
	S?ren

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Wed, Apr 2, 2014 at 7:56 AM, Michal Simek <monstr@monstr.eu> wrote:
> On 03/31/2014 10:22 AM, Linus Walleij wrote:
>> On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
>> <harinikatakamlinux@gmail.com> wrote:
>>> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
>>
>>>>> +/* Read/Write access to the GPIO PS registers */
>>>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>>>> +{
>>>>> +       return readl_relaxed(offset);
>>>>> +}
>>>>> +
>>>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>>>> +{
>>>>> +       writel_relaxed(val, offset);
>>>>> +}
>>>>
>>>> I think this is unnecessary and confusing indirection.
>>>> Just use the readl_relaxed/writel_relaxed functions directly in
>>>> the code.
>>>>
>>>
>>> This is just to be flexible.
>>
>> Define exactly what you mean with "flexible" in this context. I
>> only see unnecessary overhead and hard-to-read code.
>
> We have just passed this discussion for watchdog driver
> here: https://lkml.org/lkml/2014/4/1/843
>
> Are you ok with doing it in this way?

No :-)

Subsystem maintainers do not necessarily agree on such issues.

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Wed, Apr 2, 2014 at 7:56 AM, Michal Simek <monstr@monstr.eu> wrote:
> On 03/31/2014 10:22 AM, Linus Walleij wrote:
>> On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
>> <harinikatakamlinux@gmail.com> wrote:
>>> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
>>
>>>>> +/* Read/Write access to the GPIO PS registers */
>>>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>>>> +{
>>>>> +       return readl_relaxed(offset);
>>>>> +}
>>>>> +
>>>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>>>> +{
>>>>> +       writel_relaxed(val, offset);
>>>>> +}
>>>>
>>>> I think this is unnecessary and confusing indirection.
>>>> Just use the readl_relaxed/writel_relaxed functions directly in
>>>> the code.
>>>>
>>>
>>> This is just to be flexible.
>>
>> Define exactly what you mean with "flexible" in this context. I
>> only see unnecessary overhead and hard-to-read code.
>
> We have just passed this discussion for watchdog driver
> here: https://lkml.org/lkml/2014/4/1/843
>
> Are you ok with doing it in this way?

No :-)

Subsystem maintainers do not necessarily agree on such issues.

Yours,
Linus Walleij

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Wed, Apr 2, 2014 at 7:56 AM, Michal Simek <monstr@monstr.eu> wrote:
> On 03/31/2014 10:22 AM, Linus Walleij wrote:
>> On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
>> <harinikatakamlinux@gmail.com> wrote:
>>> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
>>
>>>>> +/* Read/Write access to the GPIO PS registers */
>>>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>>>> +{
>>>>> +       return readl_relaxed(offset);
>>>>> +}
>>>>> +
>>>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>>>> +{
>>>>> +       writel_relaxed(val, offset);
>>>>> +}
>>>>
>>>> I think this is unnecessary and confusing indirection.
>>>> Just use the readl_relaxed/writel_relaxed functions directly in
>>>> the code.
>>>>
>>>
>>> This is just to be flexible.
>>
>> Define exactly what you mean with "flexible" in this context. I
>> only see unnecessary overhead and hard-to-read code.
>
> We have just passed this discussion for watchdog driver
> here: https://lkml.org/lkml/2014/4/1/843
>
> Are you ok with doing it in this way?

No :-)

Subsystem maintainers do not necessarily agree on such issues.

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Mon, Apr 7, 2014 at 9:00 PM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Mon, 2014-03-31 at 11:23AM +0200, Ulf Hansson wrote:
>> On 27 March 2014 16:25, Harini Katakam <harinik@xilinx.com> wrote:
> [...]
>> > +static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
>> > +{
>> > +       struct platform_device *pdev = to_platform_device(dev);
>> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>> > +
>> > +       clk_disable(gpio->clk);
>>
>> You should be able can use clk_disable_unprepare() here.
>>
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
>> > +{
>> > +       struct platform_device *pdev = to_platform_device(dev);
>> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>> > +
>> > +       return clk_enable(gpio->clk);
>>
>> You should be able can use clk_prepare_enable() here.
>
> Is there some common practice regarding this? As I understand it, we
> want to ensure the clock to be gated during suspend, which should happen
> with clk_disable(). Why would we also unprepare the clock? We are highly
> likely to use it again once we resume.

enable() is fastpath (e.g. in IRQs disabled context) whereas
prepare() is slowpath() in normal, threaded context.

As [runtime]_suspend/resume happens in normal context you
want to make sure clocks are both disabled/unprepared
and prepared/enabled to make sure any clocks that can only
be accessed in slowpath are also turned off/on.

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Mon, Apr 7, 2014 at 9:00 PM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Mon, 2014-03-31 at 11:23AM +0200, Ulf Hansson wrote:
>> On 27 March 2014 16:25, Harini Katakam <harinik@xilinx.com> wrote:
> [...]
>> > +static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
>> > +{
>> > +       struct platform_device *pdev = to_platform_device(dev);
>> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>> > +
>> > +       clk_disable(gpio->clk);
>>
>> You should be able can use clk_disable_unprepare() here.
>>
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
>> > +{
>> > +       struct platform_device *pdev = to_platform_device(dev);
>> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>> > +
>> > +       return clk_enable(gpio->clk);
>>
>> You should be able can use clk_prepare_enable() here.
>
> Is there some common practice regarding this? As I understand it, we
> want to ensure the clock to be gated during suspend, which should happen
> with clk_disable(). Why would we also unprepare the clock? We are highly
> likely to use it again once we resume.

enable() is fastpath (e.g. in IRQs disabled context) whereas
prepare() is slowpath() in normal, threaded context.

As [runtime]_suspend/resume happens in normal context you
want to make sure clocks are both disabled/unprepared
and prepared/enabled to make sure any clocks that can only
be accessed in slowpath are also turned off/on.

Yours,
Linus Walleij

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Linus Walleij]

On Mon, Apr 7, 2014 at 9:00 PM, S?ren Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Mon, 2014-03-31 at 11:23AM +0200, Ulf Hansson wrote:
>> On 27 March 2014 16:25, Harini Katakam <harinik@xilinx.com> wrote:
> [...]
>> > +static int __maybe_unused zynq_gpio_runtime_suspend(struct device *dev)
>> > +{
>> > +       struct platform_device *pdev = to_platform_device(dev);
>> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>> > +
>> > +       clk_disable(gpio->clk);
>>
>> You should be able can use clk_disable_unprepare() here.
>>
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
>> > +{
>> > +       struct platform_device *pdev = to_platform_device(dev);
>> > +       struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>> > +
>> > +       return clk_enable(gpio->clk);
>>
>> You should be able can use clk_prepare_enable() here.
>
> Is there some common practice regarding this? As I understand it, we
> want to ensure the clock to be gated during suspend, which should happen
> with clk_disable(). Why would we also unprepare the clock? We are highly
> likely to use it again once we resume.

enable() is fastpath (e.g. in IRQs disabled context) whereas
prepare() is slowpath() in normal, threaded context.

As [runtime]_suspend/resume happens in normal context you
want to make sure clocks are both disabled/unprepared
and prepared/enabled to make sure any clocks that can only
be accessed in slowpath are also turned off/on.

Yours,
Linus Walleij

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Michal Simek]

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

On 04/10/2014 07:52 PM, Linus Walleij wrote:
> On Wed, Apr 2, 2014 at 7:56 AM, Michal Simek <monstr@monstr.eu> wrote:
>> On 03/31/2014 10:22 AM, Linus Walleij wrote:
>>> On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
>>> <harinikatakamlinux@gmail.com> wrote:
>>>> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
>>>
>>>>>> +/* Read/Write access to the GPIO PS registers */
>>>>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>>>>> +{
>>>>>> +       return readl_relaxed(offset);
>>>>>> +}
>>>>>> +
>>>>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>>>>> +{
>>>>>> +       writel_relaxed(val, offset);
>>>>>> +}
>>>>>
>>>>> I think this is unnecessary and confusing indirection.
>>>>> Just use the readl_relaxed/writel_relaxed functions directly in
>>>>> the code.
>>>>>
>>>>
>>>> This is just to be flexible.
>>>
>>> Define exactly what you mean with "flexible" in this context. I
>>> only see unnecessary overhead and hard-to-read code.
>>
>> We have just passed this discussion for watchdog driver
>> here: https://lkml.org/lkml/2014/4/1/843
>>
>> Are you ok with doing it in this way?
> 
> No :-)
> 
> Subsystem maintainers do not necessarily agree on such issues.

I think your sentence is right.  :-)
But what to do to convince you to agree with it?
We can use readl/writel directly (or relaxed versions) but it will
just end up that we will have a patch in our xilinx git tree
which won't be in the mainline.

Having central point for IO access functions it not an unused technique.
I was able to find out some cases in drivers/gpio/
gpio-bt8xx.c, gpio-msm-v1.c, gpio-tegra.c, gpio-xilinx.c (almost similar reason
here :-))
and just one in pinctrl-spear.h.
But I have to admit most of gpio/pinmux drivers are using them directly
but on the other hand they are not FPGA based. :-)

BTW: Shouldn't be __raw_ versions replaced by _relaxed?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Michal Simek]

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

On 04/10/2014 07:52 PM, Linus Walleij wrote:
> On Wed, Apr 2, 2014 at 7:56 AM, Michal Simek <monstr@monstr.eu> wrote:
>> On 03/31/2014 10:22 AM, Linus Walleij wrote:
>>> On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
>>> <harinikatakamlinux@gmail.com> wrote:
>>>> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
>>>
>>>>>> +/* Read/Write access to the GPIO PS registers */
>>>>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>>>>> +{
>>>>>> +       return readl_relaxed(offset);
>>>>>> +}
>>>>>> +
>>>>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>>>>> +{
>>>>>> +       writel_relaxed(val, offset);
>>>>>> +}
>>>>>
>>>>> I think this is unnecessary and confusing indirection.
>>>>> Just use the readl_relaxed/writel_relaxed functions directly in
>>>>> the code.
>>>>>
>>>>
>>>> This is just to be flexible.
>>>
>>> Define exactly what you mean with "flexible" in this context. I
>>> only see unnecessary overhead and hard-to-read code.
>>
>> We have just passed this discussion for watchdog driver
>> here: https://lkml.org/lkml/2014/4/1/843
>>
>> Are you ok with doing it in this way?
> 
> No :-)
> 
> Subsystem maintainers do not necessarily agree on such issues.

I think your sentence is right.  :-)
But what to do to convince you to agree with it?
We can use readl/writel directly (or relaxed versions) but it will
just end up that we will have a patch in our xilinx git tree
which won't be in the mainline.

Having central point for IO access functions it not an unused technique.
I was able to find out some cases in drivers/gpio/
gpio-bt8xx.c, gpio-msm-v1.c, gpio-tegra.c, gpio-xilinx.c (almost similar reason
here :-))
and just one in pinctrl-spear.h.
But I have to admit most of gpio/pinmux drivers are using them directly
but on the other hand they are not FPGA based. :-)

BTW: Shouldn't be __raw_ versions replaced by _relaxed?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH 1/2] GPIO: Add driver for Zynq GPIO controller

[Michal Simek]

On 04/10/2014 07:52 PM, Linus Walleij wrote:
> On Wed, Apr 2, 2014 at 7:56 AM, Michal Simek <monstr@monstr.eu> wrote:
>> On 03/31/2014 10:22 AM, Linus Walleij wrote:
>>> On Sat, Mar 29, 2014 at 5:44 AM, Harini Katakam
>>> <harinikatakamlinux@gmail.com> wrote:
>>>> On Sat, Mar 29, 2014 at 3:20 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>> On Thu, Mar 27, 2014 at 4:25 PM, Harini Katakam <harinik@xilinx.com> wrote:
>>>
>>>>>> +/* Read/Write access to the GPIO PS registers */
>>>>>> +static inline u32 zynq_gpio_readreg(void __iomem *offset)
>>>>>> +{
>>>>>> +       return readl_relaxed(offset);
>>>>>> +}
>>>>>> +
>>>>>> +static inline void zynq_gpio_writereg(void __iomem *offset, u32 val)
>>>>>> +{
>>>>>> +       writel_relaxed(val, offset);
>>>>>> +}
>>>>>
>>>>> I think this is unnecessary and confusing indirection.
>>>>> Just use the readl_relaxed/writel_relaxed functions directly in
>>>>> the code.
>>>>>
>>>>
>>>> This is just to be flexible.
>>>
>>> Define exactly what you mean with "flexible" in this context. I
>>> only see unnecessary overhead and hard-to-read code.
>>
>> We have just passed this discussion for watchdog driver
>> here: https://lkml.org/lkml/2014/4/1/843
>>
>> Are you ok with doing it in this way?
> 
> No :-)
> 
> Subsystem maintainers do not necessarily agree on such issues.

I think your sentence is right.  :-)
But what to do to convince you to agree with it?
We can use readl/writel directly (or relaxed versions) but it will
just end up that we will have a patch in our xilinx git tree
which won't be in the mainline.

Having central point for IO access functions it not an unused technique.
I was able to find out some cases in drivers/gpio/
gpio-bt8xx.c, gpio-msm-v1.c, gpio-tegra.c, gpio-xilinx.c (almost similar reason
here :-))
and just one in pinctrl-spear.h.
But I have to admit most of gpio/pinmux drivers are using them directly
but on the other hand they are not FPGA based. :-)

BTW: Shouldn't be __raw_ versions replaced by _relaxed?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140411/be9bda66/attachment.sig>